Bug 204720 – [iOS] Calls to device orientation API should be done in the UI process

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

The device orientation API on iOS is communicating with locationd on iOS. Since mach lookup to this daemon will be closed, the calls to this API should be moved from the WebContent process to the UI process.

[Per Arne Vollan]

Created attachment 384552 [details]
Patch

[Per Arne Vollan]

Created attachment 384555 [details]
Patch

[Per Arne Vollan]

Created attachment 384558 [details]
Patch

[Per Arne Vollan]

Created attachment 384559 [details]
Patch

[Per Arne Vollan]

Created attachment 384560 [details]
Patch

[Per Arne Vollan]

Created attachment 384561 [details]
Patch

[Per Arne Vollan]

Created attachment 384562 [details]
Patch

[Per Arne Vollan]

Created attachment 384563 [details]
Patch

[Per Arne Vollan]

Created attachment 384621 [details]
Patch

[Per Arne Vollan]

Created attachment 384625 [details]
Patch

[Per Arne Vollan]

Created attachment 384632 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/57564315>

[Chris Dumez]

Comment on attachment 384632 [details]
Patch

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

> Source/WebCore/dom/DeviceOrientationClient.h:50
> +    virtual void setSharedDeviceOrientationClient(RefPtr<SharedDeviceOrientationClient>) { };

RefPtr<>&&

Maybe even Ref<>&& since it does not look like this can ever be called with nullptr.

> Source/WebCore/dom/Document.cpp:617
> +        m_deviceOrientationClient->setSharedDeviceOrientationClient(page()->sharedDeviceOrientationClient());

Can we pass page()->sharedDeviceOrientationClient() when constructing m_deviceOrientationClient?  Why do we need a separate setter?

> Source/WebCore/page/Page.h:727
> +    RefPtr<SharedDeviceOrientationClient> sharedDeviceOrientationClient() const { return m_sharedDeviceOrientationClient; }

I don't think this should return a RefPtr, you are not transferring ownership.

> Source/WebCore/page/Page.h:1002
> +    RefPtr<SharedDeviceOrientationClient> m_sharedDeviceOrientationClient;

Looks like this can never be null, so why not use Ref<>?

> Source/WebCore/platform/ios/DeviceOrientationClientIOS.h:53
> +    void setSharedDeviceOrientationClient(RefPtr<SharedDeviceOrientationClient> sharedDeviceOrientationClient) override { m_sharedDeviceOrientationClient = sharedDeviceOrientationClient; }

RefPtr<>&& and WTFMove().

> Source/WebCore/platform/ios/SharedDeviceOrientationClient.h:38
> +    SharedDeviceOrientationClient() { }

Should probably be protected, not public, and = default;

> Source/WebCore/platform/ios/SharedDeviceOrientationClient.h:41
> +    virtual void startUpdating(WebCoreMotionManagerClient&) { }

= 0;

> Source/WebCore/platform/ios/SharedDeviceOrientationClient.h:42
> +    virtual void stopUpdating(WebCoreMotionManagerClient&) { }

ditto.

> Source/WebCore/platform/ios/SharedDeviceOrientationClient.h:44
> +    virtual void deviceOrientationChanged(double, double, double, double, double) { }

ditto.

> Source/WebCore/platform/ios/WebCoreMotionManagerClient.h:34
> +    virtual void orientationChanged(double, double, double, double, double) { };

= 0;

> Source/WebKit/UIProcess/WebPageProxy.cpp:524
> +    stopUpdatingDeviceOrientation();

Shouldn't this be in WebPageProxy::close() instead?

An alternative proposal would be to use a WeakHashSet in the WebCoreMotionManager, so that the clients do not even need to unregister themselves.

> Source/WebKit/UIProcess/WebPageProxy.h:2191
> +    void orientationChanged(double, double, double, double, double) override;

final

> Source/WebKit/UIProcess/WebPageProxy.h:2653
> +    WebCoreMotionManager* m_motionManager { nullptr };

This data member seems useless, why not always use [WebCoreMotionManager sharedManager]?

> Source/WebKit/UIProcess/WebPageProxy.messages.in:590
> +    StartUpdatingDeviceOrientation();

These should probably not be on the page, but rather on an object that lives in the UIProcess.

> Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:1500
> +    [m_motionManager removeOrientationClient:this];

Shouldn't we make sure m_motionManager is non-null? just in case? This IPC comes from the Webprocess, which is not trusted. Crashing the UIProcess because the WebProcess sends an unexpected IPC would not be acceptable IMO.

> Source/WebKit/WebProcess/WebCoreSupport/WebSharedDeviceOrientationClient.cpp:49
> +    if (m_clients.isEmpty() && m_page)

Something is wrong here, you already returned early on the previous line if m_clients.isEmpty(). I think you meant !m_clients.isEmpty(), not m_clients.isEmpty(). Also, given the early return, this should probably simply be:
if (m_page)

> Source/WebKit/WebProcess/WebCoreSupport/WebSharedDeviceOrientationClient.cpp:55
> +    for (auto client : m_clients)

auto*

> Source/WebKit/WebProcess/WebCoreSupport/WebSharedDeviceOrientationClient.cpp:59
> +};

no ;

> Source/WebKit/WebProcess/WebCoreSupport/WebSharedDeviceOrientationClient.h:40
> +    WTF_MAKE_FAST_ALLOCATED;

Not needed, since it subclasses RefCounted.

> Source/WebKit/WebProcess/WebCoreSupport/WebSharedDeviceOrientationClient.h:42
> +    WebSharedDeviceOrientationClient(WebPage& page)

Should be private, with a factory function, since this subclasses RefCounted.

> Source/WebKit/WebProcess/WebCoreSupport/WebSharedDeviceOrientationClient.h:43
> +        : m_page(WTF::makeWeakPtr(page))

WTF:: unnecessary

> Source/WebKit/WebProcess/WebCoreSupport/WebSharedDeviceOrientationClient.h:47
> +    void startUpdating(WebCore::WebCoreMotionManagerClient&) override;

final

> Source/WebKit/WebProcess/WebCoreSupport/WebSharedDeviceOrientationClient.h:48
> +    void stopUpdating(WebCore::WebCoreMotionManagerClient&) override;

final

> Source/WebKit/WebProcess/WebCoreSupport/WebSharedDeviceOrientationClient.h:50
> +    void deviceOrientationChanged(double, double, double, double, double) override;

final

> Source/WebKit/WebProcess/WebCoreSupport/WebSharedDeviceOrientationClient.h:53
> +    WTF::WeakPtr<WebPage> m_page;

WTF:: is unnecessary.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:510
> +    pageConfiguration.sharedDeviceOrientationClient = new WebSharedDeviceOrientationClient(*this);

I believe the new pattern is to use makeUnique<>() or Foo::create(), not new. See examples above. The ones that still use new are old leftovers. You can have several pages per WebProcess, especially with process caching, why leak this?

> Source/WebKit/WebProcess/WebPage/WebPage.messages.in:588
> +    DeviceOrientationChanged(double alpha, double beta, double gamma, double compassHeading, double compassAccuracy)

It feels like this should be in WebSharedDeviceOrientationClient.messages.in

[Per Arne Vollan]

I will update the patch according to the review comments. Thanks for reviewing!

[Per Arne Vollan]

Created attachment 384737 [details]
Patch

[Per Arne Vollan]

(In reply to Chris Dumez from comment #13)
> Comment on attachment 384632 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384632&action=review
> 
> > Source/WebCore/dom/DeviceOrientationClient.h:50
> > +    virtual void setSharedDeviceOrientationClient(RefPtr<SharedDeviceOrientationClient>) { };
> 
> RefPtr<>&&
> 
> Maybe even Ref<>&& since it does not look like this can ever be called with
> nullptr.
> 
> > Source/WebCore/dom/Document.cpp:617
> > +        m_deviceOrientationClient->setSharedDeviceOrientationClient(page()->sharedDeviceOrientationClient());
> 
> Can we pass page()->sharedDeviceOrientationClient() when constructing
> m_deviceOrientationClient?  Why do we need a separate setter?
> 

Good point, I moved this to the constructor instead. I kept the RefPtr, since it seems to be possible that the Document's page can be null, when the Document is constructed.

> > Source/WebCore/page/Page.h:727
> > +    RefPtr<SharedDeviceOrientationClient> sharedDeviceOrientationClient() const { return m_sharedDeviceOrientationClient; }
> 
> I don't think this should return a RefPtr, you are not transferring
> ownership.
> 

Fixed.

> > Source/WebCore/page/Page.h:1002
> > +    RefPtr<SharedDeviceOrientationClient> m_sharedDeviceOrientationClient;
> 
> Looks like this can never be null, so why not use Ref<>?
> 

This will be null in legacy WebKit.

> > Source/WebCore/platform/ios/DeviceOrientationClientIOS.h:53
> > +    void setSharedDeviceOrientationClient(RefPtr<SharedDeviceOrientationClient> sharedDeviceOrientationClient) override { m_sharedDeviceOrientationClient = sharedDeviceOrientationClient; }
> 
> RefPtr<>&& and WTFMove().
> 

Fixed in the constructor of DeviceOrientationClientIOS.

> > Source/WebCore/platform/ios/SharedDeviceOrientationClient.h:38
> > +    SharedDeviceOrientationClient() { }
> 
> Should probably be protected, not public, and = default;
> 

Fixed.

> > Source/WebCore/platform/ios/SharedDeviceOrientationClient.h:41
> > +    virtual void startUpdating(WebCoreMotionManagerClient&) { }
> 
> = 0;
> 
> > Source/WebCore/platform/ios/SharedDeviceOrientationClient.h:42
> > +    virtual void stopUpdating(WebCoreMotionManagerClient&) { }
> 
> ditto.
> 
> > Source/WebCore/platform/ios/SharedDeviceOrientationClient.h:44
> > +    virtual void deviceOrientationChanged(double, double, double, double, double) { }
> 
> ditto.
> 
> > Source/WebCore/platform/ios/WebCoreMotionManagerClient.h:34
> > +    virtual void orientationChanged(double, double, double, double, double) { };
> 
> = 0;
> 

Fixed.

> > Source/WebKit/UIProcess/WebPageProxy.cpp:524
> > +    stopUpdatingDeviceOrientation();
> 
> Shouldn't this be in WebPageProxy::close() instead?
> 

Fixed.

> An alternative proposal would be to use a WeakHashSet in the
> WebCoreMotionManager, so that the clients do not even need to unregister
> themselves.
> 
> > Source/WebKit/UIProcess/WebPageProxy.h:2191
> > +    void orientationChanged(double, double, double, double, double) override;
> 
> final
> 

Fixed.

> > Source/WebKit/UIProcess/WebPageProxy.h:2653
> > +    WebCoreMotionManager* m_motionManager { nullptr };
> 
> This data member seems useless, why not always use [WebCoreMotionManager
> sharedManager]?
> 

This is to avoid creating a WebCoreMotionManager when it is not needed.

> > Source/WebKit/UIProcess/WebPageProxy.messages.in:590
> > +    StartUpdatingDeviceOrientation();
> 
> These should probably not be on the page, but rather on an object that lives
> in the UIProcess.
> 
> > Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:1500
> > +    [m_motionManager removeOrientationClient:this];
> 
> Shouldn't we make sure m_motionManager is non-null? just in case? This IPC
> comes from the Webprocess, which is not trusted. Crashing the UIProcess
> because the WebProcess sends an unexpected IPC would not be acceptable IMO.
> 

Fixed.

> > Source/WebKit/WebProcess/WebCoreSupport/WebSharedDeviceOrientationClient.cpp:49
> > +    if (m_clients.isEmpty() && m_page)
> 
> Something is wrong here, you already returned early on the previous line if
> m_clients.isEmpty(). I think you meant !m_clients.isEmpty(), not
> m_clients.isEmpty(). Also, given the early return, this should probably
> simply be:
> if (m_page)
> 

Fixed. I forgot to remove the client from the hash set.

> > Source/WebKit/WebProcess/WebCoreSupport/WebSharedDeviceOrientationClient.cpp:55
> > +    for (auto client : m_clients)
> 
> auto*
> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebSharedDeviceOrientationClient.cpp:59
> > +};
> 
> no ;
> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebSharedDeviceOrientationClient.h:40
> > +    WTF_MAKE_FAST_ALLOCATED;
> 
> Not needed, since it subclasses RefCounted.
> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebSharedDeviceOrientationClient.h:42
> > +    WebSharedDeviceOrientationClient(WebPage& page)
> 
> Should be private, with a factory function, since this subclasses RefCounted.
> 

Fixed.

> > Source/WebKit/WebProcess/WebCoreSupport/WebSharedDeviceOrientationClient.h:43
> > +        : m_page(WTF::makeWeakPtr(page))
> 
> WTF:: unnecessary
> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebSharedDeviceOrientationClient.h:47
> > +    void startUpdating(WebCore::WebCoreMotionManagerClient&) override;
> 
> final
> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebSharedDeviceOrientationClient.h:48
> > +    void stopUpdating(WebCore::WebCoreMotionManagerClient&) override;
> 
> final
> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebSharedDeviceOrientationClient.h:50
> > +    void deviceOrientationChanged(double, double, double, double, double) override;
> 
> final
> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebSharedDeviceOrientationClient.h:53
> > +    WTF::WeakPtr<WebPage> m_page;
> 
> WTF:: is unnecessary.
> 

Fixed.

> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:510
> > +    pageConfiguration.sharedDeviceOrientationClient = new WebSharedDeviceOrientationClient(*this);
> 
> I believe the new pattern is to use makeUnique<>() or Foo::create(), not
> new. See examples above. The ones that still use new are old leftovers. You
> can have several pages per WebProcess, especially with process caching, why
> leak this?
> 

Fixed.

> > Source/WebKit/WebProcess/WebPage/WebPage.messages.in:588
> > +    DeviceOrientationChanged(double alpha, double beta, double gamma, double compassHeading, double compassAccuracy)
> 
> It feels like this should be in WebSharedDeviceOrientationClient.messages.in

I kept this on the page for now, since we pass the SharedDeviceOrientationClient in the page configuration when creating the page, relating it to the page concept. Are you OK with that?

[Chris Dumez]

(In reply to Per Arne Vollan from comment #16)
> (In reply to Chris Dumez from comment #13)
> > Comment on attachment 384632 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=384632&action=review
> > 
> > > Source/WebCore/dom/DeviceOrientationClient.h:50
> > > +    virtual void setSharedDeviceOrientationClient(RefPtr<SharedDeviceOrientationClient>) { };
> > 
> > RefPtr<>&&
> > 
> > Maybe even Ref<>&& since it does not look like this can ever be called with
> > nullptr.
> > 
> > > Source/WebCore/dom/Document.cpp:617
> > > +        m_deviceOrientationClient->setSharedDeviceOrientationClient(page()->sharedDeviceOrientationClient());
> > 
> > Can we pass page()->sharedDeviceOrientationClient() when constructing
> > m_deviceOrientationClient?  Why do we need a separate setter?
> > 
> 
> Good point, I moved this to the constructor instead. I kept the RefPtr,
> since it seems to be possible that the Document's page can be null, when the
> Document is constructed.
> 
> > > Source/WebCore/page/Page.h:727
> > > +    RefPtr<SharedDeviceOrientationClient> sharedDeviceOrientationClient() const { return m_sharedDeviceOrientationClient; }
> > 
> > I don't think this should return a RefPtr, you are not transferring
> > ownership.
> > 
> 
> Fixed.
> 
> > > Source/WebCore/page/Page.h:1002
> > > +    RefPtr<SharedDeviceOrientationClient> m_sharedDeviceOrientationClient;
> > 
> > Looks like this can never be null, so why not use Ref<>?
> > 
> 
> This will be null in legacy WebKit.
> 
> > > Source/WebCore/platform/ios/DeviceOrientationClientIOS.h:53
> > > +    void setSharedDeviceOrientationClient(RefPtr<SharedDeviceOrientationClient> sharedDeviceOrientationClient) override { m_sharedDeviceOrientationClient = sharedDeviceOrientationClient; }
> > 
> > RefPtr<>&& and WTFMove().
> > 
> 
> Fixed in the constructor of DeviceOrientationClientIOS.
> 
> > > Source/WebCore/platform/ios/SharedDeviceOrientationClient.h:38
> > > +    SharedDeviceOrientationClient() { }
> > 
> > Should probably be protected, not public, and = default;
> > 
> 
> Fixed.
> 
> > > Source/WebCore/platform/ios/SharedDeviceOrientationClient.h:41
> > > +    virtual void startUpdating(WebCoreMotionManagerClient&) { }
> > 
> > = 0;
> > 
> > > Source/WebCore/platform/ios/SharedDeviceOrientationClient.h:42
> > > +    virtual void stopUpdating(WebCoreMotionManagerClient&) { }
> > 
> > ditto.
> > 
> > > Source/WebCore/platform/ios/SharedDeviceOrientationClient.h:44
> > > +    virtual void deviceOrientationChanged(double, double, double, double, double) { }
> > 
> > ditto.
> > 
> > > Source/WebCore/platform/ios/WebCoreMotionManagerClient.h:34
> > > +    virtual void orientationChanged(double, double, double, double, double) { };
> > 
> > = 0;
> > 
> 
> Fixed.
> 
> > > Source/WebKit/UIProcess/WebPageProxy.cpp:524
> > > +    stopUpdatingDeviceOrientation();
> > 
> > Shouldn't this be in WebPageProxy::close() instead?
> > 
> 
> Fixed.
> 
> > An alternative proposal would be to use a WeakHashSet in the
> > WebCoreMotionManager, so that the clients do not even need to unregister
> > themselves.
> > 
> > > Source/WebKit/UIProcess/WebPageProxy.h:2191
> > > +    void orientationChanged(double, double, double, double, double) override;
> > 
> > final
> > 
> 
> Fixed.
> 
> > > Source/WebKit/UIProcess/WebPageProxy.h:2653
> > > +    WebCoreMotionManager* m_motionManager { nullptr };
> > 
> > This data member seems useless, why not always use [WebCoreMotionManager
> > sharedManager]?
> > 
> 
> This is to avoid creating a WebCoreMotionManager when it is not needed.

Can you please elaborate? I still don't see how this data member makes any difference with regards to creating a WebCoreMotionManager.

> 
> > > Source/WebKit/UIProcess/WebPageProxy.messages.in:590
> > > +    StartUpdatingDeviceOrientation();
> > 
> > These should probably not be on the page, but rather on an object that lives
> > in the UIProcess.
> > 
> > > Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:1500
> > > +    [m_motionManager removeOrientationClient:this];
> > 
> > Shouldn't we make sure m_motionManager is non-null? just in case? This IPC
> > comes from the Webprocess, which is not trusted. Crashing the UIProcess
> > because the WebProcess sends an unexpected IPC would not be acceptable IMO.
> > 
> 
> Fixed.
> 
> > > Source/WebKit/WebProcess/WebCoreSupport/WebSharedDeviceOrientationClient.cpp:49
> > > +    if (m_clients.isEmpty() && m_page)
> > 
> > Something is wrong here, you already returned early on the previous line if
> > m_clients.isEmpty(). I think you meant !m_clients.isEmpty(), not
> > m_clients.isEmpty(). Also, given the early return, this should probably
> > simply be:
> > if (m_page)
> > 
> 
> Fixed. I forgot to remove the client from the hash set.
> 
> > > Source/WebKit/WebProcess/WebCoreSupport/WebSharedDeviceOrientationClient.cpp:55
> > > +    for (auto client : m_clients)
> > 
> > auto*
> > 
> > > Source/WebKit/WebProcess/WebCoreSupport/WebSharedDeviceOrientationClient.cpp:59
> > > +};
> > 
> > no ;
> > 
> > > Source/WebKit/WebProcess/WebCoreSupport/WebSharedDeviceOrientationClient.h:40
> > > +    WTF_MAKE_FAST_ALLOCATED;
> > 
> > Not needed, since it subclasses RefCounted.
> > 
> > > Source/WebKit/WebProcess/WebCoreSupport/WebSharedDeviceOrientationClient.h:42
> > > +    WebSharedDeviceOrientationClient(WebPage& page)
> > 
> > Should be private, with a factory function, since this subclasses RefCounted.
> > 
> 
> Fixed.
> 
> > > Source/WebKit/WebProcess/WebCoreSupport/WebSharedDeviceOrientationClient.h:43
> > > +        : m_page(WTF::makeWeakPtr(page))
> > 
> > WTF:: unnecessary
> > 
> > > Source/WebKit/WebProcess/WebCoreSupport/WebSharedDeviceOrientationClient.h:47
> > > +    void startUpdating(WebCore::WebCoreMotionManagerClient&) override;
> > 
> > final
> > 
> > > Source/WebKit/WebProcess/WebCoreSupport/WebSharedDeviceOrientationClient.h:48
> > > +    void stopUpdating(WebCore::WebCoreMotionManagerClient&) override;
> > 
> > final
> > 
> > > Source/WebKit/WebProcess/WebCoreSupport/WebSharedDeviceOrientationClient.h:50
> > > +    void deviceOrientationChanged(double, double, double, double, double) override;
> > 
> > final
> > 
> > > Source/WebKit/WebProcess/WebCoreSupport/WebSharedDeviceOrientationClient.h:53
> > > +    WTF::WeakPtr<WebPage> m_page;
> > 
> > WTF:: is unnecessary.
> > 
> 
> Fixed.
> 
> > > Source/WebKit/WebProcess/WebPage/WebPage.cpp:510
> > > +    pageConfiguration.sharedDeviceOrientationClient = new WebSharedDeviceOrientationClient(*this);
> > 
> > I believe the new pattern is to use makeUnique<>() or Foo::create(), not
> > new. See examples above. The ones that still use new are old leftovers. You
> > can have several pages per WebProcess, especially with process caching, why
> > leak this?
> > 
> 
> Fixed.
> 
> > > Source/WebKit/WebProcess/WebPage/WebPage.messages.in:588
> > > +    DeviceOrientationChanged(double alpha, double beta, double gamma, double compassHeading, double compassAccuracy)
> > 
> > It feels like this should be in WebSharedDeviceOrientationClient.messages.in
> 
> I kept this on the page for now, since we pass the
> SharedDeviceOrientationClient in the page configuration when creating the
> page, relating it to the page concept. Are you OK with that?

No, not really. We should avoid putting all the IPC on the page as much as possible.

[Chris Dumez]

Comment on attachment 384737 [details]
Patch

r- while previous review comments are addressed.

[Per Arne Vollan]

(In reply to Chris Dumez from comment #17)
> (In reply to Per Arne Vollan from comment #16)
> > (In reply to Chris Dumez from comment #13)
> > > Comment on attachment 384632 [details]
> > > Patch
> > > 

> > > > Source/WebKit/WebProcess/WebPage/WebPage.messages.in:588
> > > > +    DeviceOrientationChanged(double alpha, double beta, double gamma, double compassHeading, double compassAccuracy)
> > > 
> > > It feels like this should be in WebSharedDeviceOrientationClient.messages.in
> > 
> > I kept this on the page for now, since we pass the
> > SharedDeviceOrientationClient in the page configuration when creating the
> > page, relating it to the page concept. Are you OK with that?
> 
> No, not really. We should avoid putting all the IPC on the page as much as
> possible.

That makes sense, I will update the patch. Thanks for reviewing!

[Per Arne Vollan]

Created attachment 384833 [details]
Patch

[Per Arne Vollan]

Created attachment 384837 [details]
Patch

[Per Arne Vollan]

Created attachment 384848 [details]
Patch

[Alex Christensen]

Comment on attachment 384848 [details]
Patch

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

Should there be a sandbox change with this?

> Source/WebCore/platform/ios/DeviceOrientationClientIOS.mm:40
> +    , m_updating(false)

This should be an initializer in the header.  Same with m_motionManager.

> Source/WebCore/platform/ios/SharedDeviceOrientationClient.h:36
> +class SharedDeviceOrientationClient : public RefCounted<SharedDeviceOrientationClient> {

I don't think SharedDeviceOrientationClient is a good name for this, especially since we already have a DeviceOrientationClient which is something different.  Could this be called "DeviceOrientationUpdateProvider" or something like that?

> Source/WebCore/platform/ios/WebCoreMotionManager.mm:251
> +        Vector<WebCoreMotionManagerClient*> orientationClients;

Could we make these also WeakPtr's?  There are some objects that destroy other objects during iteration, and even if this isn't one of those now, there's no reason to use a raw pointer here.

> Source/WebCore/platform/ios/WebCoreMotionManagerClient.h:34
> +namespace WebCore {
> +
> +class WebCoreMotionManagerClient : public CanMakeWeakPtr<WebCoreMotionManagerClient> {

This is already in the namespace WebCore.  It doesn't need WebCore in its name.

> Source/WebKit/UIProcess/ios/WebSharedDeviceOrientationClientProxy.mm:57
> +    [[WebCoreMotionManager sharedManager] removeOrientationClient:this];

This class must be more robust.  Upon destruction, if we haven't received a message from the web process saying to remove this, we should remove this anyways.

[Per Arne Vollan]

Created attachment 384941 [details]
Patch

[Per Arne Vollan]

(In reply to Alex Christensen from comment #23)
> Comment on attachment 384848 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384848&action=review
> 
> Should there be a sandbox change with this?
> 

This will be done in a follow-up patch.

> > Source/WebCore/platform/ios/DeviceOrientationClientIOS.mm:40
> > +    , m_updating(false)
> 
> This should be an initializer in the header.  Same with m_motionManager.
> 

Fixed.

> > Source/WebCore/platform/ios/SharedDeviceOrientationClient.h:36
> > +class SharedDeviceOrientationClient : public RefCounted<SharedDeviceOrientationClient> {
> 
> I don't think SharedDeviceOrientationClient is a good name for this,
> especially since we already have a DeviceOrientationClient which is
> something different.  Could this be called "DeviceOrientationUpdateProvider"
> or something like that?
> 

Thanks, that is much better!

> > Source/WebCore/platform/ios/WebCoreMotionManager.mm:251
> > +        Vector<WebCoreMotionManagerClient*> orientationClients;
> 
> Could we make these also WeakPtr's?  There are some objects that destroy
> other objects during iteration, and even if this isn't one of those now,
> there's no reason to use a raw pointer here.
> 

Done.

> > Source/WebCore/platform/ios/WebCoreMotionManagerClient.h:34
> > +namespace WebCore {
> > +
> > +class WebCoreMotionManagerClient : public CanMakeWeakPtr<WebCoreMotionManagerClient> {
> 
> This is already in the namespace WebCore.  It doesn't need WebCore in its
> name.
> 

Fixed.

> > Source/WebKit/UIProcess/ios/WebSharedDeviceOrientationClientProxy.mm:57
> > +    [[WebCoreMotionManager sharedManager] removeOrientationClient:this];
> 
> This class must be more robust.  Upon destruction, if we haven't received a
> message from the web process saying to remove this, we should remove this
> anyways.

WebCoreMotionManager has a weak set of orientation clients. Would this be sufficient to avoid removing the client in the destructor?

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 384952 [details]
Patch

[Per Arne Vollan]

Created attachment 384965 [details]
Patch

[Alex Christensen]

Comment on attachment 384965 [details]
Patch

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

> Source/WebCore/platform/ios/WebCoreMotionManager.mm:254
> +            orientationClients.append(makeWeakPtr(&client));

This could be uncheckedAppend.

[Per Arne Vollan]

Created attachment 385051 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 385051 [details]
Patch

Clearing flags on attachment: 385051

Committed r253231: <https://trac.webkit.org/changeset/253231>