Bug 186192 - [GTK][WPE] Add API to run javascript from a WebKitWebView in an isolated world
Summary: [GTK][WPE] Add API to run javascript from a WebKitWebView in an isolated world
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2018-06-01 06:06 PDT by Carlos Garcia Campos
Modified: 2018-06-11 00:26 PDT (History)
9 users (show)

See Also:


Attachments
Patch (32.62 KB, patch)
2018-06-01 06:13 PDT, Carlos Garcia Campos
mcatanzaro: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-sierra (3.03 MB, application/zip)
2018-06-01 07:50 PDT, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2018-06-01 06:06:22 PDT
We also need to add API to create isolated worlds with a name, since the web view will use the world name.
Comment 1 Carlos Garcia Campos 2018-06-01 06:13:07 PDT
Created attachment 341754 [details]
Patch
Comment 2 EWS Watchlist 2018-06-01 06:15:08 PDT
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
Comment 3 EWS Watchlist 2018-06-01 07:50:53 PDT
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
Comment 4 EWS Watchlist 2018-06-01 07:50:54 PDT
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
Comment 5 Michael Catanzaro 2018-06-01 10:38:07 PDT
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.
Comment 6 Carlos Garcia Campos 2018-06-03 03:03:06 PDT
Adding wk2 owners to the CC.
Comment 7 Carlos Garcia Campos 2018-06-03 03:09:07 PDT
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.
Comment 8 Michael Catanzaro 2018-06-03 07:11:06 PDT
(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.
Comment 9 Carlos Garcia Campos 2018-06-04 22:34:07 PDT
Ping owners, please.
Comment 10 Carlos Garcia Campos 2018-06-08 08:37:41 PDT
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.
Comment 11 youenn fablet 2018-06-08 08:49:43 PDT
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.
Comment 12 Carlos Garcia Campos 2018-06-08 09:01:11 PDT
(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.
Comment 13 Michael Catanzaro 2018-06-08 10:28:41 PDT
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.
Comment 14 Carlos Garcia Campos 2018-06-11 00:26:29 PDT
Committed r232671: <https://trac.webkit.org/changeset/232671>