| Summary: | [WebVR] Fix VRDisplayEvent implementation | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Sergio Villar Senin <svillar> | ||||||
| Component: | New Bugs | Assignee: | Sergio Villar Senin <svillar> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | commit-queue, dino, svillar, webkit-bug-importer, zan | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Sergio Villar Senin
2018-07-05 01:39:19 PDT
Created attachment 344313 [details]
Patch
Comment on attachment 344313 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344313&action=review > Source/WebCore/Modules/webvr/VRDisplayEvent.h:36 > + static Ref<VRDisplayEvent> create(const AtomicString& type, const RefPtr<VRDisplay>& display, const std::optional<VRDisplayEventReason>& reason) Can display and reason arguments be rvalue references, enabling moving them down to the constructor and there into the member variables? > Source/WebCore/Modules/webvr/VRDisplayEvent.h:54 > + std::optional<VRDisplayEventReason> reason() const { return m_reason; } Can this return a const reference to the std::optional<>? It would then be up to the caller to copy it if necessary. (In reply to Zan Dobersek from comment #2) > Comment on attachment 344313 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=344313&action=review > > > Source/WebCore/Modules/webvr/VRDisplayEvent.h:36 > > + static Ref<VRDisplayEvent> create(const AtomicString& type, const RefPtr<VRDisplay>& display, const std::optional<VRDisplayEventReason>& reason) > > Can display and reason arguments be rvalue references, enabling moving them > down to the constructor and there into the member variables? The reason can, but the VRDisplay cannot as they belong to somewhere else > > Source/WebCore/Modules/webvr/VRDisplayEvent.h:54 > > + std::optional<VRDisplayEventReason> reason() const { return m_reason; } > > Can this return a const reference to the std::optional<>? It would then be > up to the caller to copy it if necessary. OK Created attachment 344316 [details]
Patch for landing
(In reply to Zan Dobersek from comment #2) > Comment on attachment 344313 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=344313&action=review > > > Source/WebCore/Modules/webvr/VRDisplayEvent.h:36 > > + static Ref<VRDisplayEvent> create(const AtomicString& type, const RefPtr<VRDisplay>& display, const std::optional<VRDisplayEventReason>& reason) > > Can display and reason arguments be rvalue references, enabling moving them > down to the constructor and there into the member variables? > > > Source/WebCore/Modules/webvr/VRDisplayEvent.h:54 > > + std::optional<VRDisplayEventReason> reason() const { return m_reason; } > > Can this return a const reference to the std::optional<>? It would then be > up to the caller to copy it if necessary. I forgot to add this last change, I'll add it to a follow-up patch Committed r233515: <https://trac.webkit.org/changeset/233515> |