Bug 105631 - [GTK] Add support for loading web process extensions
Summary: [GTK] Add support for loading web process extensions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks: 106462
  Show dependency treegraph
 
Reported: 2012-12-21 05:39 PST by Carlos Garcia Campos
Modified: 2013-01-10 03:29 PST (History)
12 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2012-12-21 05:39:24 PST
Add support for loading plugins for the web process (web extensions) using injected bundle.
Comment 1 Carlos Garcia Campos 2012-12-21 05:57:07 PST
Created attachment 180511 [details]
Patch
Comment 2 WebKit Review Bot 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 Build Bot 2012-12-21 07:24:11 PST
Comment on attachment 180511 [details]
Patch

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 Carlos Garcia Campos 2012-12-29 01:59:36 PST
Created attachment 180924 [details]
Updated patch

Removed the single header check added to the main web extension header by mistake.
Comment 5 Carlos Garcia Campos 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 Sam Weinig 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 Carlos Garcia Campos 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 Sam Weinig 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 Carlos Garcia Campos 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 Carlos Garcia Campos 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 Sam Weinig 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 Carlos Garcia Campos 2013-01-02 06:47:45 PST
Created attachment 181029 [details]
New patch

Use an injected bundle lib instead of the builtin bundle.
Comment 13 Gustavo Noronha (kov) 2013-01-09 05:06:36 PST
Comment on attachment 181029 [details]
New patch

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 Carlos Garcia Campos 2013-01-10 02:49:58 PST
Created attachment 182101 [details]
Patch for landing
Comment 15 WebKit Review Bot 2013-01-10 03:29:13 PST
Comment on attachment 182101 [details]
Patch for landing

Clearing flags on attachment: 182101

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