Bug 187343 – [WebVR] Add support for connect/disconnect and mount/unmount device events

• Component: New Bugs
• Status: RESOLVED FIXED

[Sergio Villar Senin]

[WebVR] Add support for connect/disconnect and mount/unmount device events

[Sergio Villar Senin]

Created attachment 344328 [details]
Patch

[Zan Dobersek]

Comment on attachment 344328 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=344328&action=review

> Source/WebCore/Modules/webvr/NavigatorWebVR.cpp:62
> +        Vector<Ref<VRDisplay>> oldDisplays;
> +        oldDisplays.reserveInitialCapacity(m_displays.size());
> +        for (unsigned i = 0; i < m_displays.size(); ++i)
> +            oldDisplays.uncheckedAppend(m_displays[i].copyRef());
> +        m_displays.clear();

Can this move from m_displays into oldDisplays? I think it has the same effect.

    auto oldDisplays = WTFMove(m_displays);

> Source/WebCore/Modules/webvr/NavigatorWebVR.cpp:73
> +            for (unsigned i = 0; i < oldDisplays.size() && newDisplay; ++i) {
> +                if (platformDisplay->getDisplayInfo().displayIdentifier() == oldDisplays[i]->displayId()) {
> +                    newDisplay = false;
> +                    m_displays.append(oldDisplays[i].copyRef());
> +                }
> +            }

Use a range-based for-loop, but break from it after finding an old display.

> Source/WebCore/platform/vr/VRManager.cpp:57
> +    for (auto display : displays) {

auto&. How should the WeakPtr be handled though? Maybe it should be asserted it's non-null?

> Source/WebCore/platform/vr/VRManager.cpp:76
> +    for (auto display : m_displays)
> +        display.value->notifyVRPlatformDisplayEvent(VRPlatformDisplay::Event::Disconnected);

Ditto.

> Source/WebCore/platform/vr/VRManager.cpp:81
> +    for (auto display : displays)
> +        m_displays.add(display->getDisplayInfo().displayIdentifier(), display);

Ditto.

Also, HashMap::add() doesn't set the display value if the key is already in the map. Meaning a potentially stale WeakPtr can remain in the HashMap if the underlying display object (and the corresponding WeakPtrFactory) changes. Should this use HashMap::set() instead?

Would it make more sense for this method to create a fresh HashMap, fill it with all the live display objects, dispatch connected/mounted events according to the change of state, disconnect any displays that are in m_displays but not in the new HashMap, and then move over the new HashMap into the m_displays object?

> Source/WebCore/platform/vr/VRPlatformDisplay.cpp:28
> +    ASSERT(client && m_client);

This correct? Shouldn't one be null and the other one non-null, i.e. (!!m_client ^ !!client)?

[Zan Dobersek]

Comment on attachment 344328 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=344328&action=review

> Source/WebCore/platform/vr/VRPlatformDisplayClient.h:30
> +class VRPlatformDisplayClient {
> +public:
> +    virtual void VRPlatformDisplayConnected() { }
> +    virtual void VRPlatformDisplayDisconnected() { }
> +    virtual void VRPlatformDisplayMounted() { }
> +    virtual void VRPlatformDisplayUnmounted() { }
> +};

Empty line after namespace.

The class needs a virtual destructor. That's why the Cocoa builds are failing.

Nit: method names are odd-looking with the starting 'VR' being capitalized. I'd just remove that and use 'platformDisplayConnected()' etc.

[Sergio Villar Senin]

Comment on attachment 344328 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=344328&action=review

>> Source/WebCore/platform/vr/VRManager.cpp:81
>> +        m_displays.add(display->getDisplayInfo().displayIdentifier(), display);
> 
> Ditto.
> 
> Also, HashMap::add() doesn't set the display value if the key is already in the map. Meaning a potentially stale WeakPtr can remain in the HashMap if the underlying display object (and the corresponding WeakPtrFactory) changes. Should this use HashMap::set() instead?
> 
> Would it make more sense for this method to create a fresh HashMap, fill it with all the live display objects, dispatch connected/mounted events according to the change of state, disconnect any displays that are in m_displays but not in the new HashMap, and then move over the new HashMap into the m_displays object?

Agree on the ::set() thing.

Regarding your suggestion I think both approaches are similar, I don't have a strong opinion there.

>> Source/WebCore/platform/vr/VRPlatformDisplay.cpp:28
>> +    ASSERT(client && m_client);
> 
> This correct? Shouldn't one be null and the other one non-null, i.e. (!!m_client ^ !!client)?

Indeed the condition is totally wrong

>> Source/WebCore/platform/vr/VRPlatformDisplayClient.h:30
>> +};
> 
> Empty line after namespace.
> 
> The class needs a virtual destructor. That's why the Cocoa builds are failing.
> 
> Nit: method names are odd-looking with the starting 'VR' being capitalized. I'd just remove that and use 'platformDisplayConnected()' etc.

ack

[Sergio Villar Senin]

Created attachment 345087 [details]
Patch

[Zan Dobersek]

Comment on attachment 345087 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=345087&action=review

> Source/WebCore/Modules/webvr/VRDisplay.h:92
> +    // VRPlatformDisplayClient implementation.
> +    void platformDisplayConnected() override;
> +    void platformDisplayDisconnected() override;
> +    void platformDisplayMounted() override;
> +    void platformDisplayUnmounted() override;

These should be private, if possible.

> Source/WebCore/platform/vr/VRManager.cpp:56
> +    auto displays = m_platformManager->getVRDisplays();

You can inline this into the range-based for-loop:

    for (auto& display : m_platformManager->getVRDisplays())

> Source/WebCore/platform/vr/VRPlatformDisplayClient.h:32
> +    virtual ~VRPlatformDisplayClient() { }

Nit: this virtual dtor should be above the methods.

[Sergio Villar Senin]

Comment on attachment 345087 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=345087&action=review

>> Source/WebCore/platform/vr/VRManager.cpp:56
>> +    auto displays = m_platformManager->getVRDisplays();
> 
> You can inline this into the range-based for-loop:
> 
>     for (auto& display : m_platformManager->getVRDisplays())

Yeah but you have to return displays anyway, so I decided to keep it outside.

[Sergio Villar Senin]

(In reply to Zan Dobersek from comment #6)
> Comment on attachment 345087 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345087&action=review
> 
> > Source/WebCore/Modules/webvr/VRDisplay.h:92
> > +    // VRPlatformDisplayClient implementation.
> > +    void platformDisplayConnected() override;
> > +    void platformDisplayDisconnected() override;
> > +    void platformDisplayMounted() override;
> > +    void platformDisplayUnmounted() override;
> 
> These should be private, if possible.

Ideally yes, but not in this case as we need to explicitly call these methods over a VRDisplay, in NavigatorWebVR for example.

[Zan Dobersek]

Comment on attachment 345087 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=345087&action=review

>>> Source/WebCore/platform/vr/VRManager.cpp:56
>>> +    auto displays = m_platformManager->getVRDisplays();
>> 
>> You can inline this into the range-based for-loop:
>> 
>>     for (auto& display : m_platformManager->getVRDisplays())
> 
> Yeah but you have to return displays anyway, so I decided to keep it outside.

Didn't notice that, sorry.

[Sergio Villar Senin]

Created attachment 345093 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 345093 [details]
Patch for landing

Clearing flags on attachment: 345093

Committed r233846: <https://trac.webkit.org/changeset/233846>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42239689>