[webkit-reviews] review granted: [Bug 188883] [GTK][WPE] Add API to inject/register user content in isolated worlds : [Attachment 347918] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Aug 23 09:23:53 PDT 2018


Michael Catanzaro <mcatanzaro at igalia.com> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 188883: [GTK][WPE] Add API to inject/register user content in isolated
worlds
https://bugs.webkit.org/show_bug.cgi?id=188883

Attachment 347918: Patch

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




--- Comment #4 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 347918
  --> https://bugs.webkit.org/attachment.cgi?id=347918
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitUserContent.cpp:44
> +    return *map.get().ensure(worldName, [&] { return
API::UserContentWorld::worldWithName(String::fromUTF8(worldName));
}).iterator->value;

Nit: why [&]? It looks like the only thing you're capturing here is worldName.
I would capture it explicitly.

> Source/WebKit/UIProcess/API/glib/WebKitUserContent.cpp:179
> + * webkit_user_style_sheet_new_for_world:

Hm... do we need it?

We definitely need webkit_user_script_new_for_world().

And it probably makes sense to add webkit_user_style_sheet_new_for_world() at
the same time.

But would there be any real use case for it? The use case for running JS in
isolated worlds is to prevent the JS from being accessible to the web page.
Normally with a style you do want it to be accessible.

> Source/WebKit/UIProcess/API/glib/WebKitUserContentManager.cpp:304
> + * #WebKitUserContentManager::script-message-received signal,

Comma splice; use a semicolon

> Source/WebKit/UIProcess/API/glib/WebKitUserContentManager.cpp:308
> + * See also webkit_user_content_manager_register_script_message_handler()

.

> Source/WebKit/WebProcess/UserContent/WebUserContentController.cpp:112
> -	   worldMap().ensure(world.first, [&] { return
std::make_pair(InjectedBundleScriptWorld::create(world.second), 1); });
> +	   worldMap().ensure(world.first, [&] {
> +#if PLATFORM(GTK) || PLATFORM(WPE)
> +	       // The GLib API doesn't allow to create script worlds from the
UI process, and the
> +	       // world name is used as the identifier. Use the existing world
if found.
> +	       if (auto* existingWorld =
InjectedBundleScriptWorld::find(world.second))
> +		   return
std::make_pair(Ref<InjectedBundleScriptWorld>(*existingWorld), 1);
> +#endif
> +	       return
std::make_pair(InjectedBundleScriptWorld::create(world.second), 1);
> +	   });

I don't understand your explanation; can you try explaining it again? It's
expected for WebUserContentController::addUserContentWorlds to use existing
worlds, rather than add new worlds, only on WPE/GTK?

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitUserContentManager.cpp:365
> +    // Check that the "window.webkit.messageHandlers" namespace doesn't
exist in isolated worlds.

Is this comment correct? If it were, I would expect the JS to return 'n',
correct? But instead it fails to execute, because you can't execute JS in a
world that doesn't exist yet?

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitUserContentManager.cpp:384
> +    // Post message in nain world should fail.

main

If you want to test even more, you could proceed to hook up the msg handler for
the main world, and then verify that the proper handler gets called when you
post the message from WebExtensionTestScriptWorld and the main world.


More information about the webkit-reviews mailing list