Bug 173342 – Add SPI for immediate injection of user scripts

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Add SPI for immediate injection of user scripts

[Alex Christensen]

Created attachment 312831 [details]
Patch

[Alex Christensen]

Created attachment 312844 [details]
Patch

[Build Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Brady Eidson]

Comment on attachment 312844 [details]
Patch

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

> Source/WebKit2/UIProcess/API/C/WKPageGroup.cpp:101
> +    const bool immediately = false;

Are you doing all of these const bool variables to make the code more readable?

Because, if so, we usually prefer to create a 2-state enum class.

(This pattern is weird...)

> Source/WebKit2/UIProcess/API/C/WKUserContentControllerRef.cpp:55
> +    const bool immediately = false;

Ditto...

> Source/WebKit2/UIProcess/API/Cocoa/WKUserContentController.mm:87
> +    const bool immediately = false;

Ditto...

> Source/WebKit2/UIProcess/API/Cocoa/WKUserContentController.mm:178
> +    const bool immediately = true;

Ditto...

> Source/WebKit2/UIProcess/API/gtk/WebKitUserContentManager.cpp:162
> +    const bool immediately = false;

Ditto...

> Source/WebKit2/UIProcess/UserContent/WebUserContentControllerProxy.cpp:157
> +void WebUserContentControllerProxy::addUserScript(API::UserScript& userScript, bool immediately)

Should be that same enum class mentioned above.

> Source/WebKit2/WebProcess/UserContent/WebUserContentController.cpp:388
> +    const bool immediately = false;

Ditto...

[Alex Christensen]

Yes. Any other comments?

[Brady Eidson]

Nah

[Alex Christensen]

Created attachment 312904 [details]
Patch

[Brady Eidson]

Comment on attachment 312904 [details]
Patch

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

r+ with one change (and green bots)

> Source/WebCore/page/Frame.cpp:721
> +    if (script.injectedFrames() == InjectInTopFrameOnly && ownerElement())
> +        return;

Checking ownerElement() is not a valid check for main-frameness

Use !isMainFrame() instead.

[Alex Christensen]

Created attachment 312911 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 312911 [details]
Patch

Clearing flags on attachment: 312911

Committed r218285: <http://trac.webkit.org/changeset/218285>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 173391

[youenn fablet]

Created attachment 344770 [details]
WIP Rebasing

[Alex Christensen]

Created attachment 344785 [details]
Patch

[youenn fablet]

Comment on attachment 344785 [details]
Patch

LGTM with happy bots.

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

> Source/WebKit/UIProcess/UserContent/WebUserContentControllerProxy.cpp:180
> +        process->send(Messages::WebUserContentController::AddUserScripts({ { userScript.identifier(), world->identifier(), userScript.userScript() } }, immediately), m_identifier.toUInt64());

s/m_identifier/Identifier()/

[Alex Christensen]

Created attachment 344790 [details]
Patch

[Jessie Berlin]

Comment on attachment 344785 [details]
Patch

From Geoff: AddUserScriptImmediately -> InjectUserScriptImmediately

[Alex Christensen]

Created attachment 344795 [details]
Patch

[Alex Christensen]

Created attachment 344798 [details]
Patch

[Alex Christensen]

Created attachment 344802 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 344802 [details]
Patch

Clearing flags on attachment: 344802

Committed r233752: <https://trac.webkit.org/changeset/233752>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42101041>