WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(17)
View All
Add attachment
proposed patch, testcase, etc.
Per Arne Vollan
Comment 1
2019-11-30 15:58:00 PST
Created
attachment 384552
[details]
Patch
Per Arne Vollan
Comment 2
2019-11-30 16:16:59 PST
Created
attachment 384555
[details]
Patch
Per Arne Vollan
Comment 3
2019-11-30 16:47:12 PST
Created
attachment 384558
[details]
Patch
Per Arne Vollan
Comment 4
2019-11-30 16:54:11 PST
Created
attachment 384559
[details]
Patch
Per Arne Vollan
Comment 5
2019-11-30 17:01:28 PST
Created
attachment 384560
[details]
Patch
Per Arne Vollan
Comment 6
2019-11-30 17:17:50 PST
Created
attachment 384561
[details]
Patch
Per Arne Vollan
Comment 7
2019-11-30 17:26:12 PST
Created
attachment 384562
[details]
Patch
Per Arne Vollan
Comment 8
2019-11-30 17:37:43 PST
Created
attachment 384563
[details]
Patch
Per Arne Vollan
Comment 9
2019-12-02 07:56:02 PST
Created
attachment 384621
[details]
Patch
Per Arne Vollan
Comment 10
2019-12-02 08:39:39 PST
Created
attachment 384625
[details]
Patch
Per Arne Vollan
Comment 11
2019-12-02 09:24:55 PST
Created
attachment 384632
[details]
Patch
Radar WebKit Bug Importer
Comment 12
2019-12-02 11:17:30 PST
<
rdar://problem/57564315
>
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
Created
attachment 384737
[details]
Patch
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
Created
attachment 384833
[details]
Patch
Per Arne Vollan
Comment 21
2019-12-04 12:57:42 PST
Created
attachment 384837
[details]
Patch
Per Arne Vollan
Comment 22
2019-12-04 14:55:01 PST
Created
attachment 384848
[details]
Patch
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
Created
attachment 384941
[details]
Patch
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
Created
attachment 384952
[details]
Patch
Per Arne Vollan
Comment 27
2019-12-05 14:58:55 PST
Created
attachment 384965
[details]
Patch
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
Created
attachment 385051
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug