Bug 187337 – [WebVR] Fix VRDisplayEvent implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sergio Villar Senin]

[WebVR] Fix VRDisplayEvent implementation

[Sergio Villar Senin]

Created attachment 344313 [details]
Patch

[Zan Dobersek]

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.

[Sergio Villar Senin]

(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

[Sergio Villar Senin]

Created attachment 344316 [details]
Patch for landing

[Sergio Villar Senin]

(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

[Sergio Villar Senin]

Committed r233515: <https://trac.webkit.org/changeset/233515>

[Radar WebKit Bug Importer]

<rdar://problem/41843918>