Bug 187343 - [WebVR] Add support for connect/disconnect and mount/unmount device events
Summary: [WebVR] Add support for connect/disconnect and mount/unmount device events
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-07-05 07:22 PDT by Sergio Villar Senin
Modified: 2018-07-16 07:25 PDT (History)
7 users (show)

See Also:


Attachments
Patch (29.63 KB, patch)
2018-07-05 08:23 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (29.34 KB, patch)
2018-07-16 03:37 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch for landing (29.35 KB, patch)
2018-07-16 06:44 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergio Villar Senin 2018-07-05 07:22:57 PDT
[WebVR] Add support for connect/disconnect and mount/unmount device events
Comment 1 Sergio Villar Senin 2018-07-05 08:23:32 PDT
Created attachment 344328 [details]
Patch
Comment 2 Zan Dobersek 2018-07-06 01:52:54 PDT
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)?
Comment 3 Zan Dobersek 2018-07-06 01:56:49 PDT
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.
Comment 4 Sergio Villar Senin 2018-07-16 01:27:55 PDT
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
Comment 5 Sergio Villar Senin 2018-07-16 03:37:33 PDT
Created attachment 345087 [details]
Patch
Comment 6 Zan Dobersek 2018-07-16 04:34:48 PDT
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.
Comment 7 Sergio Villar Senin 2018-07-16 06:31:29 PDT
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.
Comment 8 Sergio Villar Senin 2018-07-16 06:35:45 PDT
(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.
Comment 9 Zan Dobersek 2018-07-16 06:41:46 PDT
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.
Comment 10 Sergio Villar Senin 2018-07-16 06:44:20 PDT
Created attachment 345093 [details]
Patch for landing
Comment 11 WebKit Commit Bot 2018-07-16 07:24:29 PDT
Comment on attachment 345093 [details]
Patch for landing

Clearing flags on attachment: 345093

Committed r233846: <https://trac.webkit.org/changeset/233846>
Comment 12 WebKit Commit Bot 2018-07-16 07:24:31 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2018-07-16 07:25:26 PDT
<rdar://problem/42239689>