Bug 105631 - [GTK] Add support for loading web process extensions
: [GTK] Add support for loading web process extensions
Status: RESOLVED FIXED
: WebKit
WebKit2
: 528+ (Nightly build)
: PC Linux
: P2 Normal
Assigned To:
:
: Gtk
:
: 106462
  Show dependency treegraph
 
Reported: 2012-12-21 05:39 PST by
Modified: 2013-01-10 03:29 PST (History)


Attachments
Patch (56.74 KB, patch)
2012-12-21 05:57 PST, Carlos Garcia Campos
buildbot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Updated patch (59.17 KB, patch)
2012-12-29 01:59 PST, Carlos Garcia Campos
sam: review-
Review Patch | Details | Formatted Diff | Diff
New patch (65.29 KB, patch)
2013-01-02 06:47 PST, Carlos Garcia Campos
gns: review+
Review Patch | Details | Formatted Diff | Diff
Patch for landing (65.21 KB, patch)
2013-01-10 02:49 PST, Carlos Garcia Campos
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-12-21 05:39:24 PST
Add support for loading plugins for the web process (web extensions) using injected bundle.
------- Comment #1 From 2012-12-21 05:57:07 PST -------
Created an attachment (id=180511) [details]
Patch
------- Comment #2 From 2012-12-21 06:00:11 PST -------
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
------- Comment #3 From 2012-12-21 07:24:11 PST -------
(From update of attachment 180511 [details])
Attachment 180511 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15454419

New failing tests:
svg/as-image/img-preserveAspectRatio-support-2.html
------- Comment #4 From 2012-12-29 01:59:36 PST -------
Created an attachment (id=180924) [details]
Updated patch

Removed the single header check added to the main web extension header by mistake.
------- Comment #5 From 2012-12-29 02:00:29 PST -------
See this ephy bug for an example of how this would be used by apps:

https://bugzilla.gnome.org/show_bug.cgi?id=684437
------- Comment #6 From 2012-12-30 11:01:13 PST -------
I see this kind of uses the InjectedBundle, but also kind of doesn't (you have extra code in WebProcessSoup).  Why can't this all be done on top of the InjectedBundle mechanism and the WebKit2 API?
------- Comment #7 From 2012-12-31 00:42:37 PST -------
(In reply to comment #6)
> I see this kind of uses the InjectedBundle, but also kind of doesn't (you have extra code in WebProcessSoup).  Why can't this all be done on top of the InjectedBundle mechanism and the WebKit2 API?

This is just a first patch, we will use the injected bundle C API to implement things like bug #79918 and bug #99695. But instead of allowing apps to install their own injected bundle we use a builtin injected bundle, like qt does as well, to implement a web extensions system. This also allows us to expose the GObject DOM bindings API we use in WebKit1. And as we do for the UI process we expose our custom high level GTK+ API for the web process instead of the C API one.
------- Comment #8 From 2012-12-31 07:04:59 PST -------
(In reply to comment #7)
> (In reply to comment #6)
> > I see this kind of uses the InjectedBundle, but also kind of doesn't (you have extra code in WebProcessSoup).  Why can't this all be done on top of the InjectedBundle mechanism and the WebKit2 API?
> 
> This is just a first patch, we will use the injected bundle C API to implement things like bug #79918 and bug #99695. But instead of allowing apps to install their own injected bundle we use a builtin injected bundle, like qt does as well, to implement a web extensions system. This also allows us to expose the GObject DOM bindings API we use in WebKit1. And as we do for the UI process we expose our custom high level GTK+ API for the web process instead of the C API one.

That doesn't exactly answer why you need to change WebProcessSoup?
------- Comment #9 From 2012-12-31 07:10:46 PST -------
(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > I see this kind of uses the InjectedBundle, but also kind of doesn't (you have extra code in WebProcessSoup).  Why can't this all be done on top of the InjectedBundle mechanism and the WebKit2 API?
> > 
> > This is just a first patch, we will use the injected bundle C API to implement things like bug #79918 and bug #99695. But instead of allowing apps to install their own injected bundle we use a builtin injected bundle, like qt does as well, to implement a web extensions system. This also allows us to expose the GObject DOM bindings API we use in WebKit1. And as we do for the UI process we expose our custom high level GTK+ API for the web process instead of the C API one.
> 
> That doesn't exactly answer why you need to change WebProcessSoup?

Yes, to create the builtin injected bundle when the UI process hasn't passed a injected bundle path (typically WTR). This is what qt does too, see WebProcessQt.
------- Comment #10 From 2012-12-31 07:18:15 PST -------
(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (In reply to comment #6)
> > > > I see this kind of uses the InjectedBundle, but also kind of doesn't (you have extra code in WebProcessSoup).  Why can't this all be done on top of the InjectedBundle mechanism and the WebKit2 API?
> > > 
> > > This is just a first patch, we will use the injected bundle C API to implement things like bug #79918 and bug #99695. But instead of allowing apps to install their own injected bundle we use a builtin injected bundle, like qt does as well, to implement a web extensions system. This also allows us to expose the GObject DOM bindings API we use in WebKit1. And as we do for the UI process we expose our custom high level GTK+ API for the web process instead of the C API one.
> > 
> > That doesn't exactly answer why you need to change WebProcessSoup?
> 
> Yes, to create the builtin injected bundle when the UI process hasn't passed a injected bundle path (typically WTR). This is what qt does too, see WebProcessQt.

Of course we could do the same installing our own injected bundle lib, and that was indeed my initial idea, but then I looked at the bultin bundle used by Qt and I thought it was a good idea that simplifies the whole thing.
------- Comment #11 From 2012-12-31 11:36:17 PST -------
(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (In reply to comment #7)
> > > > (In reply to comment #6)
> > > > > I see this kind of uses the InjectedBundle, but also kind of doesn't (you have extra code in WebProcessSoup).  Why can't this all be done on top of the InjectedBundle mechanism and the WebKit2 API?
> > > > 
> > > > This is just a first patch, we will use the injected bundle C API to implement things like bug #79918 and bug #99695. But instead of allowing apps to install their own injected bundle we use a builtin injected bundle, like qt does as well, to implement a web extensions system. This also allows us to expose the GObject DOM bindings API we use in WebKit1. And as we do for the UI process we expose our custom high level GTK+ API for the web process instead of the C API one.
> > > 
> > > That doesn't exactly answer why you need to change WebProcessSoup?
> > 
> > Yes, to create the builtin injected bundle when the UI process hasn't passed a injected bundle path (typically WTR). This is what qt does too, see WebProcessQt.
> 
> Of course we could do the same installing our own injected bundle lib, and that was indeed my initial idea, but then I looked at the bultin bundle used by Qt and I thought it was a good idea that simplifies the whole thing.

I'm not clear on how it simplifies things, it certainly makes things harder on those of us working on core functionality by not using the abstractions we have in place.  Given that this can be done without modifying core code, please do it that way.
------- Comment #12 From 2013-01-02 06:47:45 PST -------
Created an attachment (id=181029) [details]
New patch

Use an injected bundle lib instead of the builtin bundle.
------- Comment #13 From 2013-01-09 05:06:36 PST -------
(From update of attachment 181029 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=181029&action=review

Looks good to me, would be good to have a facility to let applications specify a private modules path so that we don't end up with a lot of modules that won't be used loaded unnecessarily, or with applications relying on others' modules. Other than that the API looks great to me, too.

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebExtension.cpp:117
> + *
> + * Get the web page of the given @page_id.
> + *
> + * Returns: the #WebKitWebPage for the given @page_id, or %NULL if the
> + *    identifier doesn't correspond to an exsiting web page.

Be useful to state the WebExtension owns the object that is returned and annotate transfer none.
------- Comment #14 From 2013-01-10 02:49:58 PST -------
Created an attachment (id=182101) [details]
Patch for landing
------- Comment #15 From 2013-01-10 03:29:13 PST -------
(From update of attachment 182101 [details])
Clearing flags on attachment: 182101

Committed r139305: <http://trac.webkit.org/changeset/139305>
------- Comment #16 From 2013-01-10 03:29:19 PST -------
All reviewed patches have been landed.  Closing bug.