[webkit-reviews] review granted: [Bug 133053] [WebKit2] Implement ScriptMessageHandlers : [Attachment 231663] Patch

[bugzilla-daemon at webkit.org]

Anders Carlsson <andersca at apple.com> has granted Sam Weinig <sam at webkit.org>'s
request for review:
Bug 133053: [WebKit2] Implement ScriptMessageHandlers
https://bugs.webkit.org/show_bug.cgi?id=133053

Attachment 231663: Patch
https://bugs.webkit.org/attachment.cgi?id=231663&action=review

------- Additional Comments from Anders Carlsson <andersca at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=231663&action=review


> Source/WebCore/WebCore.exp.in:2210
>  __ZN7WebCore20applicationIsHRBlockEv
>  __ZN7WebCore20builtInPDFPluginNameEv
>  __ZN7WebCore21DeviceOrientationData6createEbdbdbdbb
>
+__ZN7WebCore21UserContentController13addUserScriptERNS_15DOMWrapperWorldENSt3_
_110unique_ptrINS_10UserScriptENS3_14default_deleteIS5_EEEE
>
+__ZN7WebCore21UserContentController17removeUserScriptsERNS_15DOMWrapperWorldE
>
+__ZN7WebCore21UserContentController31addUserMessageHandlerDescriptorEPNS_28Use
rMessageHandlerDescriptorE
>
+__ZN7WebCore21UserContentController34removeUserMessageHandlerDescriptorEPNS_28
UserMessageHandlerDescriptorE
> +__ZN7WebCore21UserContentController6createEv
> +__ZN7WebCore21UserContentControllerD1Ev
>  __ZN7WebCore21applicationIsApertureEv
>  __ZN7WebCore21applicationIsVersionsEv
>  __ZN7WebCore21reportThreadViolationEPKcNS_20ThreadViolationRoundE

Please remove this.

> Source/WebCore/page/DOMWindow.cpp:750
> +	   return 0;

nullptr.

> Source/WebCore/page/UserContentController.h:74
> +    void addUserMessageHandlerDescriptor(UserMessageHandlerDescriptor*);
> +    void removeUserMessageHandlerDescriptor(UserMessageHandlerDescriptor*);

These should be references.

> Source/WebCore/page/UserMessageHandler.cpp:49
> +    m_descriptor->client().didPostMessage(*this, value.get());

You can just pass prpValue.get() directly here and get rid of the RefPtr
variable. (And rename prpValue to value).

> Source/WebCore/page/UserMessageHandler.h:39
> +    static PassRef<UserMessageHandler> create(Frame* frame,
UserMessageHandlerDescriptor& descriptor)

Frame&.

> Source/WebCore/page/UserMessageHandler.h:51
> +    explicit UserMessageHandler(Frame*, UserMessageHandlerDescriptor&);

This doesn't have to be explicit.

> Source/WebCore/page/UserMessageHandlersNamespace.h:47
> +    static PassRef<UserMessageHandlersNamespace> create(Frame* frame)

Frame&.

> Source/WebCore/page/UserMessageHandlersNamespace.h:57
> +    explicit UserMessageHandlersNamespace(Frame*);

Frame&.

> Source/WebCore/page/WebKitNamespace.h:43
> +    static PassRefPtr<WebKitNamespace> create(Frame* frame)

Frame&.

> Source/WebCore/page/WebKitNamespace.h:53
> +    explicit WebKitNamespace(Frame*);

Frame&.

> Source/WebKit2/UIProcess/API/Cocoa/WKScriptMessage.mm:46
> +    _body = body;

I think you want to copy the body as well.

> Source/WebKit2/UIProcess/API/Cocoa/WKUserContentController.mm:127
> +	   [NSException raise:NSInvalidArgumentException format:@"Attempt to
add ScriptMessageHandler with name '%@' when one already exists.", name];

No need to capitalize ScriptMessageHandler, can just be "script message
handler".

> Source/WebKit2/UIProcess/UserContent/WebUserContentControllerProxy.cpp:133
> +	   WebProcessProxy* webProcess =
WebProcessProxy::fromConnection(connection);

Wonky indentation.

> Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:2985
> +		7C361D76192803BD0036A59D /*
WebUserContentControllerProxyMessageReceiver.cpp */ = {isa = PBXFileReference;
fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name =
WebUserContentControllerProxyMessageReceiver.cpp; path =
../../../../../../../../../../../Builds/Debug/DerivedSources/WebKit2/WebUserCon
tentControllerProxyMessageReceiver.cpp; sourceTree = "<group>"; };
> +		7C361D77192803BD0036A59D /*
WebUserContentControllerProxyMessages.h */ = {isa = PBXFileReference;
fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name =
WebUserContentControllerProxyMessages.h; path =
../../../../../../../../../../../Builds/Debug/DerivedSources/WebKit2/WebUserCon
tentControllerProxyMessages.h; sourceTree = "<group>"; };

These have the wrong path.

> Source/WebKit2/WebProcess/UserContent/WebUserContentController.cpp:127
> +	       webPage->pageID(),
> +	       webFrame->frameID(),
> +	       m_identifier,
> +	       IPC::DataReference(value->data())
> +	   ), m_controller->identifier());

This is not how we indent things in WebKit!.