Created attachment 180069 [details] [review]
Half-working port of Epiphany to GtkApplication

Use GtkApplication to ensure unicity instead of custom DBus service registration.
The primary instance takes care of command-line arguments parsing passed to the
secondary instances.
To ensure the application life-cycle, new windows are attached to the application
(TODO: attach bookmarks, settings... windows too).
Also the application is held when there is commands in the queue, so that it wont
quit even if no window has been previously created.

TODO: find out why the mainloop stall after a "open uri" command is enqueued fram a remote instance
TODO: attach bookmark, settings and history windows to the application
TODO: expose Actions

I need some of your light to help me debug a mainloop issue stalling after a remote instance ask for an uri opening.

Here is the workflow :

- a new instance is launched, become the primary instance, is initialized in the startup_epiphany handler, then takes care of its local command line args in the command_line_cb, fired in response to the "command-line_signal". That works fine.

- another epiphany invocation is issued by the user (ex. epiphany http://google.fr)
- GApllication notices there is already a running instance, so send it the cmd line arguments then quit
- the primary instance receive the args, and is notified by the "command-line" signal, handled by command_line_cb again.
- those arguments are parsed into commands (EPHY_SESSION_CMD_OPEN_URIS mostly), that are then queued.

And then... the mainloop hangs before the command is poped up from the session queue to be executed.

If any of you has a theory/solution, I'll be very glad !

Here is the stack trace when the loop is hanging :

+ Trace 225910

Seems like something stange happens in eggsmclient. Any hints guys ?

You don't want to use GOptionContext to parse command line args passed via GApplication. Lots of code assumes that things triggered from a GOptionContext will only happen once, and only right at the start of the program. Eg, the option group returned by egg_sm_client_get_option_group() connects to the SM server after the command line has been parsed, and (apparently) messes things up if that code gets run a second time.

(It's also possible, depending on the details of GApplication, that if you start one epiphany, and then do "epiphany --help" from another terminal, that it will cause the first instance to print command-line help and then exit.)

I don't know exactly what the right way to do command-line handling in GApplication is, but this isn't it.

Thanks for your input Dan.

GOption is indeed not really fit for multiple invocations. Even more, a lot of libs use it parse-hooks facility to do initialization, thus modify the global state.

EggSmClient is indeed one of those, so I modified our local copy in order to only invoke its init once.
Also, I decided to create a GOptionContext for each invocation, so to avoid the (broken?) behaviour on remaining arguments (the pointer to the string array is handled badly there).
That seems to work fine !

A more future-proof solution might be to do the parsing locally, even on the secondary instances, and communication with the primary by the mean of actions exported with the GApplication facilities.

Created attachment 180499 [details] [review]
Working patch of port of Epiphany to GtkApplication

This patches fixes (hopefully) the mainloop hanging found in the previous patch.

Please review, comment or bash !

At a later stage, I'd like to implement an EphyApplication class deriving from GtkApplication, which would also take care of 
- the session manager handling (extracted from EphySession)
- the EphyShell access, avoiding the current singleton

Also, the action currently defined in ephy-activation need to be converted in GActions, exposed on the bus via GApplication.

Created attachment 182282 [details] [review]
Use GtkApplication subclass to ensure unicity instead of custom DBus service registration.

The primary instance takes care of command-line arguments parsing passed to the
secondary instances.
To ensure the application life-cycle, new windows are attached to the application
Also the application is held when there is commands in the queue, so that it wont quit even if no window has been previously created.

Move some utilities procedures from main into ephy-string and ephy-file-helper

Created attachment 182297 [details] [review]
Remove all direct accesses to the ephy_shell global variable

https://bugzilla.gnome.org/show_bug.cgi?id=628364

Created attachment 182298 [details] [review]
Do not instanciate a global EphyShell instance. Instead set a field of GtkApplication.

https://bugzilla.gnome.org/show_bug.cgi?id=628364

Created attachment 182299 [details] [review]
Replace ephy_shell_get_default calls by ephy_application_get_shell

https://bugzilla.gnome.org/show_bug.cgi?id=628364

Created attachment 182406 [details] [review]
Use GtkApplication to ensure unicity

Use GtkApplication subclass to ensure unicity instead of custom DBus service registration.
This version of the patch delegates command-line arguments parsing to the secondary instances. These then communicate with the primary instance by the mean of exposed GActions (currently "Open", "Open Bookmark editor" and "Load session")
To ensure the application life-cycle, new windows are attached to the application
Also the application is held when there is commands in the queue, so that it wont quit even if no window has been previously created.

Move some utilities procedures from main into ephy-string and ephy-file-helper

The latest attached patch let the secondary instances doing the command-line parsing. These then communicate with the primary instance using the exposed GApplication's actions (via DBus).

This has many advantages:
- avoid GOption non-reentrancy issues
- let the primary instance mainloop free from "heavy" parsing
- force the extraction of the actions, which could be used in other contexts
- DBus API for free
- future dev of GApplication might bring "activation" and nice gnome-shell "jump lists" using those actions

Created attachment 182749 [details] [review]
Move desktop session manager handling from EphySession into a GtkApplication subclass, inherited by EphyApplication.

Created attachment 182750 [details] [review]
Use a interface trait for session management instead of subclassing GtkApplication. Fix Typo

Created attachment 182751 [details] [review]
Make the tests compliant with the new GtkApplication

Review of attachment 182282 [details] [review]:

I had a look to this patch. Bear in mind that I am not very familiar with the epiphany codebase, yet I see some stuff worth commenting. I didn't test it either yet.

Basically:

- there are a lot of whitespace noise changes, please review before submitting. Same for coding style.
- Style fixes or changes that you find necessary should be submitted separately.
- Code refactoring should be done only if needed, and in such case, please use separate patches.

::: lib/ephy-file-helpers.h
@@ +90,1 @@
 

Moving stuff like this around, when necessary, is better done in a single standalone patch.

::: lib/ephy-string.c
@@ +504,3 @@
+*/
+static char*
+ephy_string_to_uri(char *input, GError **error)

Freeing a parameter is not a standard practice. Better receive a const char * and return a newly allocated one. Leave the memory management to the method user (even if this is a static method).

@@ +551,3 @@
+ */
+gboolean
+        return NULL;

Modifying an array contents in place is not a very common practice, as far as I know. Just return a linked list or some other structure.

On the other hand, there is no reason to do this refactoring. When working on a well established codebase, it is a better idea to do this type of refactorings only if you need to use the same or similar code somewhere else. In such case, writing a single refactoring patch is advisable. However, we don't need this here, and it makes understanding the core idea of the patch a bit harder.

::: src/Makefile.am
@@ +16,3 @@
 NOINST_H_FILES = \
+	ephy-goption.h				    \
+	ephy-application.h				\

Too much spacing I guess.

@@ +60,1 @@
 	ephy-activation.c			\

Ditto.

::: src/ephy-application.c
@@ +226,3 @@
+ephy_application_init (EphyApplication *application)
+{
+	application->priv = EPHY_APPLICATION_GET_PRIVATE(application);

Missing space before the opening parenthesis.

::: src/ephy-session.c
@@ +41,3 @@
 #include "ephy-stock-icons.h"
 #include "ephy-window.h"
+#include "ephy-application.h"

This appears to be unnecessary..

@@ -512,3 @@
-}
-
-	{

I don't understand why you needed to shuffle this code around.

@@ +520,3 @@
 				    EPHY_SESSION_CMD_MAYBE_OPEN_WINDOW_RESTORE,
 				    NULL, NULL, user_time, FALSE);
+

Not needed.

@@ +535,3 @@
 
+
+

Not needed.

@@ +551,3 @@
 	priv = session->priv;
 
 	shell = ephy_shell_get_default ();

Not needed.

@@ +710,3 @@
 	g_assert (cmd != NULL);
 
+	LOG ("dispatching queue cmd:%d\n", cmd->command);

This change is unnecessary.

@@ +1752,3 @@
 	GList *element;
 	SessionCommand *cmd;
+	

This should not be in the patch.

::: src/ephy-shell.c
@@ +58,3 @@
 #include "ephy-stock-icons.h"
 #include "ephy-web-view.h"
+#include "ephy-application.h"

I don't understand whether this include is really needed here?

(In reply to comment #9)
> Created an attachment (id=182297) [details] [review]
> Remove all direct accesses to the ephy_shell global variable
> 
> https://bugzilla.gnome.org/show_bug.cgi?id=628364

(In reply to comment #10)
> Created an attachment (id=182298) [details] [review]
> Do not instanciate a global EphyShell instance. Instead set a field of
> GtkApplication.
> 
> https://bugzilla.gnome.org/show_bug.cgi?id=628364

(In reply to comment #11)
> Created an attachment (id=182299) [details] [review]
> Replace ephy_shell_get_default calls by ephy_application_get_shell
> 
> https://bugzilla.gnome.org/show_bug.cgi?id=628364

This seems like a good idea, but I think you could perfectly fine squash these three commits together and avoid doing intermediate changes, since there is no reason to go from global_shell -> shell_get_default() -> app_get_default_shell() in one series of patches.

Hi Claudio,

Thanks for your review. I'll provide a fix for most of your concerns regarding style ASAP.
Here are some comments on the remaining points:

> 
> ::: lib/ephy-file-helpers.h
> @@ +90,1 @@
> 
> 
> Moving stuff like this around, when necessary, is better done in a single
> standalone patch.

Alright.

> 
> ::: lib/ephy-string.c
> @@ +504,3 @@
> +*/
> +static char*
> +ephy_string_to_uri(char *input, GError **error)
> 
> Freeing a parameter is not a standard practice. Better receive a const char *
> and return a newly allocated one. Leave the memory management to the method
> user (even if this is a static method).
> 

Agreed, this implementation was already present. Just move it around and planned to modify it.

> @@ +551,3 @@
> + */
> +gboolean
> +        return NULL;
> 
> Modifying an array contents in place is not a very common practice, as far as I
> know. Just return a linked list or some other structure.
> 

Same here.

> On the other hand, there is no reason to do this refactoring. When working on a
> well established codebase, it is a better idea to do this type of refactorings
> only if you need to use the same or similar code somewhere else. In such case,
> writing a single refactoring patch is advisable. However, we don't need this
> here, and it makes understanding the core idea of the patch a bit harder.
> 

Understood.

Created attachment 190319 [details] [review]
Implement GtkApplication based activation and uniqueness

This replaces the existing dbus-glib activation and uniqueness code.
The changes are kept to the minimum necessary to make all the features
work, but there are still some optimizations possible (like doing most
of the initialization in ephy_application_startup() when we know we
are not remoting). These changes are left for later to avoid making
this patch huge.

Command-line parameter parsing is done in the main method and
parameters are passed to the application through a
EphyApplicationStartupContext structure, which is later passed as a
GVariant to the primare instance. This way we avoid moving the
GOption code out of the place where it's intended to run: in the
main() method.

Based in work by Alexandre Mazari.

Created attachment 190322 [details] [review]
Implement GtkApplication based activation and uniqueness

This replaces the existing dbus-glib activation and uniqueness code.
The changes are kept to the minimum necessary to make all the features
work, but there are still some optimizations possible (like doing most
of the initialization in ephy_application_startup() when we know we
are not remoting). These changes are left for later to avoid making
this patch huge.

Command-line parameter parsing is done in the main method and
parameters are passed to the application through a
EphyApplicationStartupContext structure, which is later passed as a
GVariant to the primare instance. This way we avoid moving the
GOption code out of the place where it's intended to run: in the
main() method.

Based in work by Alexandre Mazari.

Comment on attachment 190319 [details] [review]
Implement GtkApplication based activation and uniqueness

missing files

Created attachment 190351 [details] [review]
This is a new version, removing some cruft I left yesterday.

Implement GtkApplication based activation and uniqueness

This replaces the existing dbus-glib activation and uniqueness code.
The changes are kept to the minimum necessary to make all the features
work, but there are still some optimizations possible (like doing most
of the initialization in ephy_application_startup() when we know we
are not remoting). These changes are left for later to avoid making
this patch huge.

Command-line parameter parsing is done in the main method and
parameters are passed to the application through a
EphyApplicationStartupContext structure, which is later passed as a
GVariant to the primare instance. This way we avoid moving the
GOption code out of the place where it's intended to run: in the
main() method.

Based in work by Alexandre Mazari.

Review of attachment 190351 [details] [review]:

::: src/ephy-application.c
@@ +32,3 @@
+#define EPHY_APPLICATION_GET_PRIVATE(object)(G_TYPE_INSTANCE_GET_PRIVATE ((object), EPHY_TYPE_APPLICATION, EphyApplicationPrivate))
+struct _EphyApplicationPrivate
+{

Brace.

@@ +50,3 @@
+ * @bookmark_url: A URL to be added to the bookmarks.
+ * @arguments: A %NULL-terminated array of URLs and file URIs to be opened.
+ * @user_time: THe user time when the EphyApplication startup was invoked.

The.

@@ +60,3 @@
+ephy_application_startup_context_new (gboolean new_tab,
+                                      gboolean new_window,
+                                      gboolean bookmarks_editor,

These three could be flags probably, more extensible.

@@ +67,3 @@
+                                      guint32 user_time)
+{
+  EphyApplicationStartupContext *ctx = g_new0 (EphyApplicationStartupContext, 1);

I believe generally we use g_slice to alocate structures, but meh...

@@ +110,3 @@
+
+  shell = ephy_shell_get_default ();
+  g_assert (shell != NULL);

Don't really want to get into philosophical debates, but this seems like it's basically impossible, so the assert does not seem very useful.

@@ +112,3 @@
+  g_assert (shell != NULL);
+  session = EPHY_SESSION (ephy_shell_get_session (shell));
+  g_assert (session != NULL);

Same?

@@ +120,3 @@
+                                EPHY_SESSION_CMD_OPEN_BOOKMARKS_EDITOR,
+                                NULL, NULL, ctx->user_time, FALSE);
+  else if (ctx->session_filename != NULL)

The { style is all wrong in the rest of the function. Also is repeated throughout the file, so just go over it again I guess.

@@ +134,3 @@
+      GString *options;
+
+      options = g_string_sized_new (64);

Drop the microoptimization :P

@@ +221,3 @@
+                                          ctx->new_window,
+                                          ctx->bookmarks_editor,
+                                          UNNULLIFY(ctx->bookmarks_filename),

To be honest I don't mind just having s ? s : "", here, but if there's any way of not doing this kind of thing, better.

::: src/ephy-application.h
@@ +45,3 @@
+
+struct _EphyApplication
+{

I guess the brace should go in the previous line.

@@ +52,3 @@
+
+struct _EphyApplicationClass
+{

Same here.

@@ +57,3 @@
+
+typedef struct {
+  gchar **arguments;

char instead of gchar (see HACKING).

::: src/ephy-main.c
@@ +421,3 @@
+	application = ephy_application_new ();
+	ephy_shell_set_application (ephy_shell_get_default (),
+				    application);

If the Shell only can have one app, which can only be set once, perhaps it's better for now to just make the Shell create it and use the getter here.

::: src/ephy-shell.c
@@ +206,2 @@
 	shell->priv = EPHY_SHELL_GET_PRIVATE (shell);
+	shell->priv->application = NULL;

This is not really needed and we don't do it for the other members, so let's drop it.

Another follow-up to this can be to get rid of the EphyEmbedShell _trak_object API, I suppose. I understand GtkApplication has something equivalent already built-in.

Review of attachment 190351 [details] [review]:

::: src/ephy-application.c
@@ +60,3 @@
+ephy_application_startup_context_new (gboolean new_tab,
+                                      gboolean new_window,
+                                      gboolean bookmarks_editor,

Sure.

@@ +67,3 @@
+                                      guint32 user_time)
+{
+  EphyApplicationStartupContext *ctx = g_new0 (EphyApplicationStartupContext, 1);

Hm, I thought all these functions were using g_slice() internally..

@@ +110,3 @@
+
+  shell = ephy_shell_get_default ();
+  g_assert (shell != NULL);

This code is verbatim from the existing queue_commands() in main(), so I preferred to leave is as is. It can be fixed now or later, as you prefer.

@@ +112,3 @@
+  g_assert (shell != NULL);
+  session = EPHY_SESSION (ephy_shell_get_session (shell));
+  g_assert (session != NULL);

Same as above.

@@ +134,3 @@
+      GString *options;
+
+      options = g_string_sized_new (64);

Same as above comments.

@@ +221,3 @@
+                                          ctx->new_window,
+                                          ctx->bookmarks_editor,
+                                          UNNULLIFY(ctx->bookmarks_filename),

It's possible to instead use a dictionary variant and only append these that are not NULL. That could avoid these hacks.

::: src/ephy-main.c
@@ +421,3 @@
+	application = ephy_application_new ();
+	ephy_shell_set_application (ephy_shell_get_default (),
+				    application);

Sure.

::: src/ephy-shell.c
@@ +206,2 @@
 	shell->priv = EPHY_SHELL_GET_PRIVATE (shell);
+	shell->priv->application = NULL;

OK.

In the cases where you are just copying code around I guess it's better to leave it as it is and then change it later if we feel like it.

Created attachment 190448 [details] [review]
Implement GtkApplication based activation and uniqueness

- Hopefully fixed all style issues and typos. I also fixed those in
  the methods copied to avoid having commits later that only fix indentation.
- Use gslice functions where appropriate.
- Use flags instead of multiple boolean parameters in the context structure.
- Let the shell instantiate the EphyApplication and get it from there. Remove
  the setter as well.
- (the big change) use a GVariant dictionary array that is filed only
  with the command-line parameters that are actually passed on instantiation.
  This avoids filing up the variant with empty strings. However (as it can be
  seen) the packing/unpacking of the variant is a bit more complex; nevertheless
  way more readable.
- The recover session command is called always (apparently it's needed
  even for remote instantiations; it's the way it was in the original queue_commands()
  method.)

This replaces the existing dbus-glib activation and uniqueness code.
The changes are kept to the minimum necessary to make all the features
work, but there are still some optimizations possible (like doing most
of the initialization in ephy_application_startup() when we know we
are not remoting). These changes are left for later to avoid making
this patch huge.

Command-line parameter parsing is done in the main method and
parameters are passed to the application through a
EphyApplicationStartupContext structure, which is later passed as a
GVariant to the primare instance. This way we avoid moving the
GOption code out of the place where it's intended to run: in the
main() method.

Based in work by Alexandre Mazari.

Review of attachment 190448 [details] [review]:

Looks good!

::: src/ephy-shell.c
@@ -800,0 +808,17 @@
+/**
+ * ephy_shell_get_application:
+ * @shell: A #EphyApplication
... 14 more ...

I think it makes more sense to do this in _init and just return it here. You can fix before landing.

Attachment 190448 [details] pushed as 8415cf6 - Implement GtkApplication based activation and uniqueness