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

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen 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 384848: Patch

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




--- Comment #23 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 384848
  --> https://bugs.webkit.org/attachment.cgi?id=384848
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.