Bug 133053 – [WebKit2] Implement ScriptMessageHandlers

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

[WebKit2] Implement ScriptMessageHandlers

[Sam Weinig]

Created attachment 231659 [details]
Patch

[WebKit Commit Bot]

Attachment 231659 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/UserContent/WebScriptMessageHandler.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/page/WebKitNamespace.h:30:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/page/UserContentController.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/page/UserMessageHandlersNamespace.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/page/UserMessageHandler.cpp:60:  Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]
ERROR: Source/WebKit2/UIProcess/UserContent/WebUserContentControllerProxy.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebKit2/WebProcess/UserContent/WebUserContentController.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/bindings/js/JSDOMWindowBase.cpp:42:  Bad include order. Mixing system and custom headers.  [build/include_order] [4]
Total errors found: 8 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Created attachment 231663 [details]
Patch

[Anders Carlsson]

Comment on attachment 231663 [details]
Patch

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_28UserMessageHandlerDescriptorE
> +__ZN7WebCore21UserContentController34removeUserMessageHandlerDescriptorEPNS_28UserMessageHandlerDescriptorE
> +__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/WebUserContentControllerProxyMessageReceiver.cpp; sourceTree = "<group>"; };
> +		7C361D77192803BD0036A59D /* WebUserContentControllerProxyMessages.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = WebUserContentControllerProxyMessages.h; path = ../../../../../../../../../../../Builds/Debug/DerivedSources/WebKit2/WebUserContentControllerProxyMessages.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!.

[Csaba Osztrogonác]

Comment on attachment 231663 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231663&action=review

> Source/WebKit2/WebProcess/UserContent/WebUserContentController.h:62
> +    HashMap<uint64_t, RefPtr<WebUserMessageHandlerDescriptorProxy>> m_userMessageHandlerDescriptors;

It will break the !ENABLE(USER_MESSAGE_HANDLERS) build, because
this class is inside this guard, we can't use it outside.

[Csaba Osztrogonác]

I didn't want to remove the r+ intentionally, but my comment conflicted with the r+.

[Sam Weinig]

Created attachment 231669 [details]
Patch

[Anders Carlsson]

Comment on attachment 231669 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=231669&action=review

> Source/WebKit2/UIProcess/API/Cocoa/WKUserContentController.mm:110
> +        id body = [value toDictionary];

This doesn't handle every object. At the very least we should handle arrays.

[Sam Weinig]

Committed r169023: <http://trac.webkit.org/changeset/169023>

[Csaba Osztrogonác]

(In reply to comment #9)
> Committed r169023: <http://trac.webkit.org/changeset/169023>

Mac rebaselines landed in https://trac.webkit.org/changeset/169044 to try to
unbreak red buildbots and dead EWS bots. Could you possibly verify if the
new results are correct or not? (if you don't have time to wait for EWS ...)