[Webkit-unassigned] [Bug 188883] [GTK][WPE] Add API to inject/register user content in isolated worlds

[bugzilla-daemon at webkit.org]

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

--- Comment #5 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to Michael Catanzaro from comment #4)
> Comment on attachment 347918 [details]
> 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.

You are right.

> > Source/WebKit/UIProcess/API/glib/WebKitUserContent.cpp:179
> > + * webkit_user_style_sheet_new_for_world:
> 
> Hm... do we need it?

I don't know but we are also creating the user style sheets unconditionally in the main world. So, I just want to make sure the user content API is consistent and avoid having to add this in the future.

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

Exactly.

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

Yes, I don't know the use case to be honest.

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

Right. We need to inject the scripts or message handers in the isolated world created by the web extension, but WebUserContentController always creates a new one.

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

No, it fails because it raises an exception. We could improve it by catching the exception and returning 'n', but I don't think it's worth it.

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

Tests can be improved, but I had to do this yesterday in a hurry because we are close to the release (and already frozen indeed).

-- 
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/20180824/a3127c28/attachment-0001.html>