[webkit-reviews] review granted: [Bug 186192] [GTK][WPE] Add API to run javascript from a WebKitWebView in an isolated world : [Attachment 341754] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 186192: [GTK][WPE] Add API to run javascript from a WebKitWebView in an
isolated world
https://bugs.webkit.org/show_bug.cgi?id=186192

Attachment 341754: Patch

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




--- 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.