[webkit-reviews] review denied: [Bug 204720] [iOS] Calls to device orientation API should be done in the UI process : [Attachment 384632] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied Per Arne Vollan <pvollan at apple.com>'s
request for review:
Bug 204720: [iOS] Calls to device orientation API should be done in the UI
process
https://bugs.webkit.org/show_bug.cgi?id=204720

Attachment 384632: Patch

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




--- Comment #13 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 384632
  --> https://bugs.webkit.org/attachment.cgi?id=384632
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()->sharedDevic
eOrientationClient());

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