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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=186192

Michael Catanzaro <mcatanzaro at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #341754|review?                     |review+
              Flags|                            |

--- Comment #5 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 341754
  --> https://bugs.webkit.org/attachment.cgi?id=341754
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180601/f3cc774d/attachment-0001.html>