Bug 103377

Summary: [GTK] Add WebKit2 API for isolated worlds
Product: WebKit Reporter: Zan Dobersek <zan>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cgarcia, commit-queue, gustavo, mrobinson, svillar, whateley
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 119743, 120252, 120268    
Bug Blocks:    
Attachments:
Description Flags
Patch andersca: review+

Description Zan Dobersek 2012-11-27 01:21:09 PST
As requested on webkit-gtk mailing list.
http://lists.webkit.org/pipermail/webkit-gtk/2012-November/001293.html

There was at one time a desire to add this API to WebKit1, covered by bug #31508.
Comment 1 Carlos Garcia Campos 2013-08-22 06:09:13 PDT
Created attachment 209352 [details]
Patch
Comment 2 Carlos Garcia Campos 2013-08-22 08:02:01 PDT
Patch doesn't apply because it depends on patch applied to bug #119743
Comment 3 Anders Carlsson 2013-08-23 05:37:18 PDT
Comment on attachment 209352 [details]
Patch

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

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitScriptWorld.cpp:41
> +    DEFINE_STATIC_LOCAL(ScriptWorldMap, map, ());

This can be NeverDestroyed<ScriptWorldMap> map if you include wtf/NeverDestroyed.h
Comment 4 Carlos Garcia Campos 2013-08-23 05:46:13 PDT
(In reply to comment #3)
> (From update of attachment 209352 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209352&action=review

Thanks for the review. 

> > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitScriptWorld.cpp:41
> > +    DEFINE_STATIC_LOCAL(ScriptWorldMap, map, ());
> 
> This can be NeverDestroyed<ScriptWorldMap> map if you include wtf/NeverDestroyed.h

Sure, I didn't know NeverDestroyed.
Comment 5 Gustavo Noronha (kov) 2013-08-23 06:00:35 PDT
Comment on attachment 209352 [details]
Patch

The API looks sensible to me, +1.
Comment 6 Martin Robinson 2013-08-23 07:40:20 PDT
Comment on attachment 209352 [details]
Patch

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

Looks good to me as well.

> Source/WebKit2/UIProcess/API/gtk/tests/WebExtensionTest.cpp:154
> +        const char *script;

Nit: asterisk location

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitScriptWorld.cpp:88
> +    return scriptWorlds().get(scriptWorld);

This returns null if the world isn't in the map?
Comment 7 Carlos Garcia Campos 2013-08-24 04:08:26 PDT
(In reply to comment #6)
> (From update of attachment 209352 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209352&action=review
> 
> Looks good to me as well.

Thanks!

> > Source/WebKit2/UIProcess/API/gtk/tests/WebExtensionTest.cpp:154
> > +        const char *script;
> 
> Nit: asterisk location

Ooops

> > Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitScriptWorld.cpp:88
> > +    return scriptWorlds().get(scriptWorld);
> 
> This returns null if the world isn't in the map?

Yes.
Comment 8 Carlos Garcia Campos 2013-08-24 05:01:06 PDT
Committed r154545: <http://trac.webkit.org/changeset/154545>
Comment 9 WebKit Commit Bot 2013-08-24 12:58:31 PDT
Re-opened since this is blocked by bug 120252
Comment 10 Carlos Garcia Campos 2013-08-26 08:08:26 PDT
Committed r154603: <http://trac.webkit.org/changeset/154603>