Bug 204720 - [iOS] Calls to device orientation API should be done in the UI process
Summary: [iOS] Calls to device orientation API should be done in the UI process
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-11-30 14:31 PST by Per Arne Vollan
Modified: 2019-12-10 17:24 PST (History)
11 users (show)

See Also:


Attachments
Patch (45.21 KB, patch)
2019-11-30 15:58 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (44.88 KB, patch)
2019-11-30 16:16 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (46.78 KB, patch)
2019-11-30 16:47 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (46.74 KB, patch)
2019-11-30 16:54 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (46.74 KB, patch)
2019-11-30 17:01 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (46.90 KB, patch)
2019-11-30 17:17 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (46.83 KB, patch)
2019-11-30 17:26 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (46.80 KB, patch)
2019-11-30 17:37 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (46.89 KB, patch)
2019-12-02 07:56 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (48.40 KB, patch)
2019-12-02 08:39 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (48.43 KB, patch)
2019-12-02 09:24 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (43.56 KB, patch)
2019-12-03 12:37 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (59.50 KB, patch)
2019-12-04 12:27 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (59.50 KB, patch)
2019-12-04 12:57 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (59.60 KB, patch)
2019-12-04 14:55 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (57.73 KB, patch)
2019-12-05 13:52 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (57.32 KB, patch)
2019-12-05 14:33 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (59.32 KB, patch)
2019-12-05 14:58 PST, Per Arne Vollan
achristensen: review+
Details | Formatted Diff | Diff
Patch (59.40 KB, patch)
2019-12-06 15:35 PST, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2019-11-30 14:31:44 PST
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.
Comment 1 Per Arne Vollan 2019-11-30 15:58:00 PST
Created attachment 384552 [details]
Patch
Comment 2 Per Arne Vollan 2019-11-30 16:16:59 PST
Created attachment 384555 [details]
Patch
Comment 3 Per Arne Vollan 2019-11-30 16:47:12 PST
Created attachment 384558 [details]
Patch
Comment 4 Per Arne Vollan 2019-11-30 16:54:11 PST
Created attachment 384559 [details]
Patch
Comment 5 Per Arne Vollan 2019-11-30 17:01:28 PST
Created attachment 384560 [details]
Patch
Comment 6 Per Arne Vollan 2019-11-30 17:17:50 PST
Created attachment 384561 [details]
Patch
Comment 7 Per Arne Vollan 2019-11-30 17:26:12 PST
Created attachment 384562 [details]
Patch
Comment 8 Per Arne Vollan 2019-11-30 17:37:43 PST
Created attachment 384563 [details]
Patch
Comment 9 Per Arne Vollan 2019-12-02 07:56:02 PST
Created attachment 384621 [details]
Patch
Comment 10 Per Arne Vollan 2019-12-02 08:39:39 PST
Created attachment 384625 [details]
Patch
Comment 11 Per Arne Vollan 2019-12-02 09:24:55 PST
Created attachment 384632 [details]
Patch
Comment 12 Radar WebKit Bug Importer 2019-12-02 11:17:30 PST
<rdar://problem/57564315>
Comment 13 Chris Dumez 2019-12-02 12:42:32 PST
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
Comment 14 Per Arne Vollan 2019-12-02 14:43:04 PST
I will update the patch according to the review comments. Thanks for reviewing!
Comment 15 Per Arne Vollan 2019-12-03 12:37:55 PST
Created attachment 384737 [details]
Patch
Comment 16 Per Arne Vollan 2019-12-03 12:48:29 PST
(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?
Comment 17 Chris Dumez 2019-12-03 12:53:51 PST
(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.
Comment 18 Chris Dumez 2019-12-03 12:54:10 PST
Comment on attachment 384737 [details]
Patch

r- while previous review comments are addressed.
Comment 19 Per Arne Vollan 2019-12-03 13:24:27 PST
(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!
Comment 20 Per Arne Vollan 2019-12-04 12:27:02 PST
Created attachment 384833 [details]
Patch
Comment 21 Per Arne Vollan 2019-12-04 12:57:42 PST
Created attachment 384837 [details]
Patch
Comment 22 Per Arne Vollan 2019-12-04 14:55:01 PST
Created attachment 384848 [details]
Patch
Comment 23 Alex Christensen 2019-12-04 21:59:05 PST
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.
Comment 24 Per Arne Vollan 2019-12-05 13:52:22 PST
Created attachment 384941 [details]
Patch
Comment 25 Per Arne Vollan 2019-12-05 13:57:59 PST
(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!
Comment 26 Per Arne Vollan 2019-12-05 14:33:06 PST
Created attachment 384952 [details]
Patch
Comment 27 Per Arne Vollan 2019-12-05 14:58:55 PST
Created attachment 384965 [details]
Patch
Comment 28 Alex Christensen 2019-12-06 14:36:48 PST
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.
Comment 29 Per Arne Vollan 2019-12-06 15:35:40 PST
Created attachment 385051 [details]
Patch
Comment 30 WebKit Commit Bot 2019-12-06 16:19:30 PST
Comment on attachment 385051 [details]
Patch

Clearing flags on attachment: 385051

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