Created attachment 232334 [details] [review]
Patch

This requires a WebKit patch not landed yet, see:

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

Created attachment 233212 [details] [review]
Updated patch

Patch updated to use the new api to set the web extensions dir.

Review of attachment 233212 [details] [review]:

OK, quick review, although I know this needs to be updated once again. The only thing that worries me is the sync method, but there's already a FIXME about that.

::: embed/ephy-embed-single.c
@@ +656,3 @@
+                                                         NULL,
+                                                         NULL);
+  }

Let's do this in EphyEmbedShell, EphyEmbedSingle is on its way out.

::: embed/ephy-web-view.c
@@ +3497,3 @@
+  single = EPHY_EMBED_SINGLE (ephy_embed_shell_get_embed_single (ephy_embed_shell_get_default ()));
+
+  /* FIXME: This should be async */

Any reason we are not doing it in the first patch?

::: embed/web-extension/ephy-web-extension.h
@@ +20,3 @@
+#define EPHY_WEB_EXTENSION_OBJECT_PATH     "/org/gnome/Epiphany/WebExtension"
+#define EPHY_WEB_EXTENSION_FORMS_INTERFACE "org.gnome.Epiphany.WebExtension.Forms"
+

This is missing the usual guards against multiple definition.

(In reply to comment #3)
> Review of attachment 233212 [details] [review]:
> 
> OK, quick review, although I know this needs to be updated once again.

Actually, I've just noticed that I had already updated the patch to use webkit_web_context_set_web_extensions_directory().

> The only
> thing that worries me is the sync method, but there's already a FIXME about
> that.
> 
> ::: embed/ephy-embed-single.c
> @@ +656,3 @@
> +                                                         NULL,
> +                                                         NULL);
> +  }
> 
> Let's do this in EphyEmbedShell, EphyEmbedSingle is on its way out.

Ok.

> ::: embed/ephy-web-view.c
> @@ +3497,3 @@
> +  single = EPHY_EMBED_SINGLE (ephy_embed_shell_get_embed_single
> (ephy_embed_shell_get_default ()));
> +
> +  /* FIXME: This should be async */
> 
> Any reason we are not doing it in the first patch?

Because the method is already sync and it contains wk1 and wk2 code, keping it sync we don't need to change the caller. My idea was to make it async when we remove the wk1 code. 

> ::: embed/web-extension/ephy-web-extension.h
> @@ +20,3 @@
> +#define EPHY_WEB_EXTENSION_OBJECT_PATH     "/org/gnome/Epiphany/WebExtension"
> +#define EPHY_WEB_EXTENSION_FORMS_INTERFACE
> "org.gnome.Epiphany.WebExtension.Forms"
> +
> 
> This is missing the usual guards against multiple definition.

Good catch!

Created attachment 236570 [details] [review]
Updated patch

Review of attachment 236570 [details] [review]:

Cool, let's do it. BTW you probably need to bump up the WebKit requirement too?

I'm wondering what we're going to do regarding the D-Bus interfaces.

At this moment we're defining 1 interface "org.gnome.Epiphany.WebExtension.Forms" with 1 method "HasModifiedForms".

But for the web apps I think we'll need a new interface "org.gnome.Epiphany.WebExtension.WebApps" with a method "GetBestIcon" or something like that.

If we're going to have several interfaces priv->web_extension should be renamed to something like priv->web_extension_forms and rename ephy_embed_shell_get_web_extension_proxy to something like ephy_embed_shell_get_web_extension_forms_proxy.

The other option is that we just define a generic interface and use it for both forms, web apps and any other stuff we might need in the future.

What do you think?

I think having a single interface for everything will make the whole thing a lot easier

Created attachment 236575 [details] [review]
Another update

Patch updated to use a single generic interface and to bump wk requirements

looks great

Review of attachment 236575 [details] [review]:

Sure. I didn't mean to hold this off with my last comment, when I mark a patch as accepted-commit-now it is always fine to do further changes in git.

Comment on attachment 236575 [details] [review]
Another update

Pushed.

I just wanted to point out, given that nobody has yet, that are you're using a well-known name on the session bus, so this breaks hard with multiple web processes running (eg. epiphany in application mode).

You need to use a different naming scheme (for example add the epiphany PID to the name), or use a private channel instead of the session bus.

You are right, it was a temporary solution that I thought it was going to be a problem only when supporting multiple web processes, but it's indeed a problem for web apps or private instances :-(

Created attachment 236781 [details] [review]
Fix for the service name

This is the easy fix for the problem, in the future we might want to use private dbus connection instead of the session bus.

Review of attachment 236781 [details] [review]:

Alright.