RESOLVED FIXED 204720
[iOS] Calls to device orientation API should be done in the UI process
https://bugs.webkit.org/show_bug.cgi?id=204720
Summary [iOS] Calls to device orientation API should be done in the UI process
Per Arne Vollan
Reported 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.
Attachments
Patch (45.21 KB, patch)
2019-11-30 15:58 PST, Per Arne Vollan
no flags
Patch (44.88 KB, patch)
2019-11-30 16:16 PST, Per Arne Vollan
no flags
Patch (46.78 KB, patch)
2019-11-30 16:47 PST, Per Arne Vollan
no flags
Patch (46.74 KB, patch)
2019-11-30 16:54 PST, Per Arne Vollan
no flags
Patch (46.74 KB, patch)
2019-11-30 17:01 PST, Per Arne Vollan
no flags
Patch (46.90 KB, patch)
2019-11-30 17:17 PST, Per Arne Vollan
no flags
Patch (46.83 KB, patch)
2019-11-30 17:26 PST, Per Arne Vollan
no flags
Patch (46.80 KB, patch)
2019-11-30 17:37 PST, Per Arne Vollan
no flags
Patch (46.89 KB, patch)
2019-12-02 07:56 PST, Per Arne Vollan
no flags
Patch (48.40 KB, patch)
2019-12-02 08:39 PST, Per Arne Vollan
no flags
Patch (48.43 KB, patch)
2019-12-02 09:24 PST, Per Arne Vollan
no flags
Patch (43.56 KB, patch)
2019-12-03 12:37 PST, Per Arne Vollan
no flags
Patch (59.50 KB, patch)
2019-12-04 12:27 PST, Per Arne Vollan
no flags
Patch (59.50 KB, patch)
2019-12-04 12:57 PST, Per Arne Vollan
no flags
Patch (59.60 KB, patch)
2019-12-04 14:55 PST, Per Arne Vollan
no flags
Patch (57.73 KB, patch)
2019-12-05 13:52 PST, Per Arne Vollan
no flags
Patch (57.32 KB, patch)
2019-12-05 14:33 PST, Per Arne Vollan
no flags
Patch (59.32 KB, patch)
2019-12-05 14:58 PST, Per Arne Vollan
achristensen: review+
Patch (59.40 KB, patch)
2019-12-06 15:35 PST, Per Arne Vollan
no flags
Per Arne Vollan
Comment 1 2019-11-30 15:58:00 PST
Per Arne Vollan
Comment 2 2019-11-30 16:16:59 PST
Per Arne Vollan
Comment 3 2019-11-30 16:47:12 PST
Per Arne Vollan
Comment 4 2019-11-30 16:54:11 PST
Per Arne Vollan
Comment 5 2019-11-30 17:01:28 PST
Per Arne Vollan
Comment 6 2019-11-30 17:17:50 PST
Per Arne Vollan
Comment 7 2019-11-30 17:26:12 PST
Per Arne Vollan
Comment 8 2019-11-30 17:37:43 PST
Per Arne Vollan
Comment 9 2019-12-02 07:56:02 PST
Per Arne Vollan
Comment 10 2019-12-02 08:39:39 PST
Per Arne Vollan
Comment 11 2019-12-02 09:24:55 PST
Radar WebKit Bug Importer
Comment 12 2019-12-02 11:17:30 PST
Chris Dumez
Comment 13 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
Per Arne Vollan
Comment 14 2019-12-02 14:43:04 PST
I will update the patch according to the review comments. Thanks for reviewing!
Per Arne Vollan
Comment 15 2019-12-03 12:37:55 PST
Per Arne Vollan
Comment 16 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?
Chris Dumez
Comment 17 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.
Chris Dumez
Comment 18 2019-12-03 12:54:10 PST
Comment on attachment 384737 [details] Patch r- while previous review comments are addressed.
Per Arne Vollan
Comment 19 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!
Per Arne Vollan
Comment 20 2019-12-04 12:27:02 PST
Per Arne Vollan
Comment 21 2019-12-04 12:57:42 PST
Per Arne Vollan
Comment 22 2019-12-04 14:55:01 PST
Alex Christensen
Comment 23 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.
Per Arne Vollan
Comment 24 2019-12-05 13:52:22 PST
Per Arne Vollan
Comment 25 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!
Per Arne Vollan
Comment 26 2019-12-05 14:33:06 PST
Per Arne Vollan
Comment 27 2019-12-05 14:58:55 PST
Alex Christensen
Comment 28 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.
Per Arne Vollan
Comment 29 2019-12-06 15:35:40 PST
WebKit Commit Bot
Comment 30 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>
Note You need to log in before you can comment on or make changes to this bug.