Bug 186192 – [GTK][WPE] Add API to run javascript from a WebKitWebView in an isolated world

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

We also need to add API to create isolated worlds with a name, since the web view will use the world name.

[Carlos Garcia Campos]

Created attachment 341754 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

Comment on attachment 341754 [details]
Patch

Attachment 341754 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7924630

New failing tests:
imported/w3c/web-platform-tests/streams/piping/error-propagation-forward.html

[EWS Watchlist]

Created attachment 341760 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Michael Catanzaro]

Comment on attachment 341754 [details]
Patch

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

r=me for the WPE/GTK bits. There are a couple bits that require owner approval before you can land it, though.

> Source/WebCore/bindings/js/ScriptController.h:93
> +    WEBCORE_EXPORT JSC::JSValue executeScriptInWorld(DOMWrapperWorld&, const String& script, bool forceUserGesture = false, ExceptionDetails* = nullptr);

ExceptionDetails* is OK, but const std::optional<ExceptionDetails>& would be better, right?

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3412
> + * If WebKitSettings:enable-javascript is FALSE, this method will do nothing.

Currently, but we might want to change the behavior in the future, right? Because API users probably always want it to work regardless of the enable-javascript setting. Documenting that the functionality could change in the future might be useful. I see that you copied it from the webkit_web_view_run_javascript() docs, though.

> Source/WebKit/UIProcess/API/gtk/WebKitWebView.h:450
> +                                                      const gchar               *world_name,

Why is using a name better than using a WebKitScriptWorld here? I guess the WebKitScriptWorld is a web process type, so that's probably not possible?

> Source/WebKit/UIProcess/WebPageProxy.h:877
>      void runJavaScriptInMainFrame(const String&, bool, WTF::Function<void (API::SerializedScriptValue*, bool hadException, const WebCore::ExceptionDetails&, CallbackBase::Error)>&& callbackFunction);
> +    void runJavaScriptInMainFrameScriptWorld(const String&, bool, const String& worldName, WTF::Function<void(API::SerializedScriptValue*, bool hadException, const WebCore::ExceptionDetails&, CallbackBase::Error)>&& callbackFunction);

Makes sense to me, since it's a natural extension of runJavaScriptInMainFrame. I trust you'll get a WK2 owner to approve it.

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitScriptWorld.cpp:143
> + * The #WebKitScriptWorld is created with a generated unique name, use

It's a comma splice, either use a semicolon or convert it into two sentences.

That sentence was a comma splice, too. "It's a comma splice" and "Either use a semicolon or..." are two independent clauses that can stand on their own as separate sentences, so they can't be joined with a comma. We both do this in Bugzilla comments rather a lot. I think I've started to pick up your Bugzilla writing style. :P Anyway, try to avoid it in the documentation.

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitScriptWorld.cpp:163
> + * isolated worlds have access to the DOM but not to other variable

variables

> Source/WebKit/WebProcess/InjectedBundle/InjectedBundleScriptWorld.h:45
> +    static InjectedBundleScriptWorld* find(const String&);

This could be std::optional<InjectedBundleScriptWorld>. The point of std::optional is that we don't need to use a raw pointer for optional results anymore.

[Carlos Garcia Campos]

Adding wk2 owners to the CC.

[Carlos Garcia Campos]

Comment on attachment 341754 [details]
Patch

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

>> Source/WebCore/bindings/js/ScriptController.h:93
>> +    WEBCORE_EXPORT JSC::JSValue executeScriptInWorld(DOMWrapperWorld&, const String& script, bool forceUserGesture = false, ExceptionDetails* = nullptr);
> 
> ExceptionDetails* is OK, but const std::optional<ExceptionDetails>& would be better, right?

evaluateInworld expects ExceptionDetails*

>> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:3412
>> + * If WebKitSettings:enable-javascript is FALSE, this method will do nothing.
> 
> Currently, but we might want to change the behavior in the future, right? Because API users probably always want it to work regardless of the enable-javascript setting. Documenting that the functionality could change in the future might be useful. I see that you copied it from the webkit_web_view_run_javascript() docs, though.

Yes, we'll see how we can do that.

>> Source/WebKit/UIProcess/API/gtk/WebKitWebView.h:450
>> +                                                      const gchar               *world_name,
> 
> Why is using a name better than using a WebKitScriptWorld here? I guess the WebKitScriptWorld is a web process type, so that's probably not possible?

Right, WebKitScriptWorld creates an InjectedBundleScriptWorld, only available in the WebProcess.

>> Source/WebKit/WebProcess/InjectedBundle/InjectedBundleScriptWorld.h:45
>> +    static InjectedBundleScriptWorld* find(const String&);
> 
> This could be std::optional<InjectedBundleScriptWorld>. The point of std::optional is that we don't need to use a raw pointer for optional results anymore.

We don't want any copy here.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #7)
> > This could be std::optional<InjectedBundleScriptWorld>. The point of std::optional is that we don't need to use a raw pointer for optional results anymore.
> 
> We don't want any copy here.

Copy for return values is preferred since C+++ 11 because return value optimization is guaranteed by the standard.

[Carlos Garcia Campos]

Ping owners, please.

[Carlos Garcia Campos]

Alex, or any other wk2 owner, please. Note that this patch doesn't affect other ports at all, it adds a new method to WebPageProxy that is only used by GTK+ and WPE ports.

[youenn fablet]

Comment on attachment 341754 [details]
Patch

Patch looks good to me.
Could you detail a little bit more the use case for adding this new capability?

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

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:2958
> +void WebPage::runJavaScriptInMainFrameScriptWorld(const String& script, bool forceUserGesture, const String& worldName, CallbackID callbackID)

This code looks somewhat similar to WebPage::runJavaScriptInMainFrame.
Maybe we can do some refactoring to share more code.

[Carlos Garcia Campos]

(In reply to youenn fablet from comment #11)
> Comment on attachment 341754 [details]
> Patch
> 
> Patch looks good to me.

Thanks youenn!

> Could you detail a little bit more the use case for adding this new
> capability?

Yes, in epiphany we expose some custom js code in the web process, using an isolated world, so that the internal js is not available from the web. But we still need to use that internal js from the UI process via webkit_eeb_view_run_javascript().

> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341754&action=review
> 
> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:2958
> > +void WebPage::runJavaScriptInMainFrameScriptWorld(const String& script, bool forceUserGesture, const String& worldName, CallbackID callbackID)
> 
> This code looks somewhat similar to WebPage::runJavaScriptInMainFrame.
> Maybe we can do some refactoring to share more code.

Yes, I thought about it, indeed, but I thought it could end up being more complex in the end. I'll give it another try.

[Michael Catanzaro]

Have you thought about how to handle webkit_web_view_run_javascript_from_resource()? You'll just have to load the content of the script from the resource manually, I suppose? We started using this function in Epiphany during the past week.

[Carlos Garcia Campos]

Committed r232671: <https://trac.webkit.org/changeset/232671>