Created attachment 321758 [details] [review]
Open tabs in the order they are clicked

Here is my attempt of fixing this behavior.

The new tabs work as intended, still I'm not sure this is the best way to achieve this so I'm open to ideas.

Review of attachment 321758 [details] [review]:

Yaaaay, I trained myself to open tabs backwards as a workaround for this. Shouldn't have had to do that.

Unfortunately, I don't think this is likely to make 3.20, as there's no way to notice when a new tab is opened if it doesn't fit into the visible area of the notebook, because GtkNotebook is not very good. That's a preexisting issue that we should look into separately, and which I fear might be some GtkNotebook GSoC project in and of itself, but it's exacerbated by this patch. We'll have to think about how to handle that (in a different bug).

Somehow I saw it open a tab to the left of the current tab once when testing your patch. Not sure why, that's not good.

Anyway, this introduced a bunch of critical warnings:

** (epiphany:26503): CRITICAL **: ephy_embed_set_parent_embed: assertion 'EPHY_IS_EMBED (parent_embed)' failed

** (epiphany:26503): CRITICAL **: ephy_embed_set_parent_embed: assertion 'EPHY_IS_EMBED (parent_embed)' failed

** (epiphany:26503): CRITICAL **: ephy_embed_set_parent_embed: assertion 'EPHY_IS_EMBED (parent_embed)' failed

** (epiphany:26503): CRITICAL **: ephy_embed_set_parent_embed: assertion 'EPHY_IS_EMBED (parent_embed)' failed

** (epiphany:26503): CRITICAL **: ephy_embed_set_parent_embed: assertion 'EPHY_IS_EMBED (parent_embed)' failed

** (epiphany:26503): CRITICAL **: ephy_embed_set_parent_embed: assertion 'EPHY_IS_EMBED (parent_embed)' failed

** (epiphany:26503): CRITICAL **: ephy_embed_set_parent_embed: assertion 'EPHY_IS_EMBED (parent_embed)' failed

** (epiphany:26503): CRITICAL **: ephy_embed_set_parent_embed: assertion 'EPHY_IS_EMBED (parent_embed)' failed

If you have trouble figuring out why, you can get a stack trace to the critical like so:

$ G_DEBUG=fatal-criticals jhbuild run epiphany

(watch it crash)

$ coredumpctl

(verify enough time has passed that the crash has been processed, should only take a second or two)

$ coredumpctl gdb

Then you get a backtrace to the crash, without the pain of running epiphany itself in gdb. Alternatively, you could just run epiphany in gdb, but be patient as it will take a while to start:

$ G_DEBUG=fatal-criticals jhbuild run gdb epiphany

But I recommend just using coredumpctl. I guess you're on Fedora? If so, and if you haven't enabled coredumpctl yet, you have to do this to enable it:

$ sudo systemctl stop abrt-ccpp
$ sudo systemctl disable abrt-ccpp
$ sudo systemctl enable abrt-vmcore
$ sudo systemctl start abrt-vmcore

Then abrt will forward coredumps to coredumpctl; otherwise it creates coredumps in the current working directory if ulimit is set appropriately, which is pretty primitive.

::: embed/ephy-embed.c
@@ +87,3 @@
+
+  EphyEmbed *parent_embed;
+  guint children_offset;

This name is too confusing. It would make sense in the context of EphyShell, but not here. I'm having trouble thinking of a better name, though; maybe:

guint embeds_from_parent;

? (Same for the function names.)

@@ +798,3 @@
   embed->seq_message_id = 1;
   embed->tab_message_id = ephy_embed_statusbar_get_context_id (embed, EPHY_EMBED_STATUSBAR_TAB_MESSAGE_CONTEXT_DESCRIPTION);
+  embed->parent_embed = NULL;

The instance struct happens to be 0-initialized by GObject, so you don't need this. Good that you were careful, though, since you know that in general structs are not 0-initialized. :)

@@ +943,3 @@
+
+void
+ephy_embed_set_parent_embed (EphyEmbed *embed, EphyEmbed *parent_embed)

Normally we do only one parameter per line; I see there are some cases of multiple parameters per line in this file, but I prefer not to add more:

ephy_embed_set_parent_embed (EphyEmbed *embed,
                             EphyEmbed *parent_embed)

@@ +948,3 @@
+  g_return_if_fail (EPHY_IS_EMBED (parent_embed));
+
+  embed->parent_embed = parent_embed;

Since you don't take ownership, this is pretty risky; as you've discovered, the parent_embed could be destroyed, then you'll be left with a dangling pointer to freed memory which will be returned by ephy_embed_get_parent_embed(). The normal solution to this is to ref it:

if (parent_embed)
  g_object_ref (parent_embed);
if (embed->parent_embed)
  g_object_unref (embed->parent_embed);
embed->parent_embed = parent_embed;

(Your code would not need to check for parent_embed to be nonnull, since you have a g_return_if_fail precondition to check for that, but I show it here to illustrate the general technique.)

Note that the new value is reffed before the old value is unreffed to make the function idempotent; don't want to accidentally delete the parent_embed immediately before reffing it if the same one is set twice. I think that should never happen with your code, but so what, this is a generic interface and it's best to be safe. This is a very common mistake in C++ code, where the mistake is much less obvious than it is in C, because the refs and unrefs in C++ are often hidden behind the copy assignment operator of std::shared_ptr; you need a temporary std::shared_ptr object to write such a setter function safely.

Anyway, the code above is such a common pattern, we have a macro to simplify it:

g_set_object (&embed->parent_embed, parent_embed);

That should fix your issue that caused you to add the errant g_object_ref() later in this patch, and at the right place. But you also need to unref it in ephy_embed_dispose(), so it's not leaked when the current EphyEmbed is destroyed:

if (embed->parent_embed) {
  g_object_unref (parent_embed);
  parent_embed = NULL;
}

You have to set it to NULL, because dispose is used for breaking reference cycles and it can run any number of times; you don't want to accidentally double-free it. Again, that's such a common pattern, we have a function to simplify that for you:

g_clear_object (&embed->parent_embed);

So the above is one way to handle this safely.

But, as we discussed on IRC, you probably don't need to take ownership here at all; instead, you just want to return NULL when parent_embed has been destroyed, rather than a dangling pointer. This has the advantage of not unnecessarily keeping the EphyEmbed GObject instance alive even after gtk_widget_destroy() has been called on it. Your embed->parent_embed pointer is not going to magically NULL itself out on its own, but since parent_embed is a GObject, you can get it parent_embed to magically NULL the pointer for you when it is destroyed by using a weak pointer:

g_object_add_weak_pointer (parent_embed, &embed->parent_embed);

So I think that's all we need here. But now, as I mentioned on IRC, what happens if the current embed is destroyed before parent_embed: then &embed->parent_embed will have been freed, some new memory could potentially have been allocated at that address, and the address will be clobbered when parent_embed is destroyed unless you remove the weak pointer. The right place to do that is ephy_embed_dispose:

if (embed->parent_embed) {
  g_object_remove_weak_pointer (G_OBJECT (embed->parent_embed), (gpointer *)&embed->parent_embed);
  embed->parent_embed = NULL;
}

Look up the descriptions of these functions to get a feel for how they all work; you'll be using them in the future:

https://developer.gnome.org/gobject/unstable/gobject-The-Base-Object-Type.html

@@ +960,3 @@
+
+void
+ephy_embed_set_children_offset (EphyEmbed *embed, guint children_offset)

Ditto.

::: embed/ephy-embed.h
@@ +46,3 @@
 gboolean         ephy_embed_has_load_pending         (EphyEmbed *embed);
 const char      *ephy_embed_get_title                (EphyEmbed *embed);
+EphyEmbed*       ephy_embed_get_parent_embed         (EphyEmbed *embed);

The style above was right:

EphyEmbed       *ephy_embed_get_parent_embed

GObject headers are a bit weird if you're not used to them. :)

::: src/ephy-shell.c
@@ +665,1 @@
       GtkWidget *nb = ephy_window_get_notebook (window);

First off, "nb" is too laconic for our code style, write out "notebook" instead.

Next:

ephy-shell.c: In function ‘ephy_shell_new_tab_full’:
ephy-shell.c:665:7: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
       GtkWidget *nb = ephy_window_get_notebook (window);
       ^
cc1: all warnings being treated as errors

Declare it at the top of the conditional instead. Since offset is only needed within the conditional, you should declare it there too. The current code doesn't always do this, but I prefer to limit the scope of variables as far as possible.

Also, since you'll probably be working on Epiphany more in the future, add to your jhbuildrc:

module_autogenargs['epiphany'] = '--enable-Werror --enable-debug'

--enable-Werror turns warnings into errors, so you don't introduce them accidentally. Pull the latest jhbuild, make, make install, as older versions of jhbuild pass --disable-Werror after your module_autogenargs instead of before.

@@ +670,3 @@
+      parent_embed = previous_embed;
+      while (parent_embed != NULL)
+      {

In this file (and most files) we put braces on the line above:

while (parent_embed != NULL) {

or my preference:

while (parent_embed) {

::: src/ephy-window.c
@@ +2780,3 @@
 {
+	guint offset;
+	EphyEmbed* parent_tab = ephy_embed_get_parent_embed (tab);

EphyEmbed *parent_tab

@@ +2789,3 @@
+	}
+
+	g_object_ref (tab);

Well, as you found this makes it not crash, which is goooood. :)

But you don't unref it anywhere, so it is leaked: a ref should always be paired with an unref. Fortunately, it turns out the call to gtk_widget_destroy() below is sufficient to prevent the entire web process from being leaked, but we don't want to lose the memory used for the EphyEmbed instance either.

As discussed above, this should be solved in the EphyEmbed class itself.

(In reply to Michael Catanzaro from comment #2)
> That should fix your issue that caused you to add the errant g_object_ref()
> later in this patch, and at the right place. But you also need to unref it
> in ephy_embed_dispose(), so it's not leaked when the current EphyEmbed is
> destroyed:
> 
> if (embed->parent_embed) {
>   g_object_unref (parent_embed);
>   parent_embed = NULL;
> }

Sorry, typo:

if (embed->parent_embed) {
  g_object_unref (embed->parent_embed);
  embed->parent_embed = NULL;
}

Created attachment 321865 [details] [review]
Open tabs in the order they are clicked

(In reply to Michael Catanzaro from comment #2) 
> Anyway, this introduced a bunch of critical warnings:
> 
> ** (epiphany:26503): CRITICAL **: ephy_embed_set_parent_embed: assertion
> 'EPHY_IS_EMBED (parent_embed)' failed

Oops. This was caused by setting the parent_embed outside the conditional, thus causing some NULL values to be set if previous_embed was NULL (should be okay now).
 
> Your embed->parent_embed pointer
> is not going to magically NULL itself out on its own, but since parent_embed
> is a GObject, you can get it parent_embed to magically NULL the pointer for
> you when it is destroyed by using a weak pointer:

I followed your instructions and made a use of add/remove weak pointer.
 
> Look up the descriptions of these functions to get a feel for how they all
> work; you'll be using them in the future:
> 
> https://developer.gnome.org/gobject/unstable/gobject-The-Base-Object-Type.
> html

I will definitely have a look at this!
 
> Also, since you'll probably be working on Epiphany more in the future, add
> to your jhbuildrc:
> 
> module_autogenargs['epiphany'] = '--enable-Werror --enable-debug'
> 
> --enable-Werror turns warnings into errors, so you don't introduce them
> accidentally. 

Neat :)

> Somehow I saw it open a tab to the left of the current tab once when testing
> your patch. Not sure why, that's not good.

That's bad, but I can't figure out how this is even possible (the offset value should never drop lower than one). Do you happen to remember the scenario which caused this?

P.S. Thanks for the great review and all the useful info! (and sorry for my late response)

Created attachment 321872 [details] [review]
Open tabs in the order they are clicked

In my rush I forgot to update the header file.

(In reply to Gabriel Ivascu from comment #4)
> That's bad, but I can't figure out how this is even possible (the offset
> value should never drop lower than one). Do you happen to remember the
> scenario which caused this?

Nope :(

Review of attachment 321872 [details] [review]:

OK, this looks a lot better.

I'd like Carlos to review this though. If he doesn't notice this comment, please introduce yourself in #epiphany, he is KaL on IRC.

::: embed/ephy-embed.c
@@ +953,3 @@
+  g_return_if_fail (EPHY_IS_EMBED (parent_embed));
+
+  g_object_add_weak_pointer (G_OBJECT (parent_embed), (gpointer *)&embed->parent_embed);

But you might have a weak pointer on embed->parent_embed already, so this could lead to memory corruption; you need to check here if embed->parent_embed is nonnull, and remove its weak pointer if so.

::: src/ephy-shell.c
@@ -659,3 @@
   if (flags & EPHY_NEW_TAB_APPEND_AFTER) {
     if (previous_embed) {
-      GtkWidget *nb = ephy_window_get_notebook (window);

Oops, I missed that this *nb widget existed here before, I thought you had added it new. I guess it's fine to rename it in this patch, though, since the change is so small and it's a nice cleanup.

By the way, we still need to decide how to make it possible to notice tabs being opened beyond the visible area of the notebook, so this is unfortunately blocked on that... but credit to you for this tricky work, regardless!

Created attachment 321895 [details] [review]
Open tabs in the order they are clicked

(In reply to Michael Catanzaro from comment #7)
> ::: embed/ephy-embed.c
> @@ +953,3 @@
> +  g_return_if_fail (EPHY_IS_EMBED (parent_embed));
> +
> +  g_object_add_weak_pointer (G_OBJECT (parent_embed), (gpointer
> *)&embed->parent_embed);
> 
> But you might have a weak pointer on embed->parent_embed already, so this
> could lead to memory corruption; you need to check here if
> embed->parent_embed is nonnull, and remove its weak pointer if so.

Done. Also added early return condition.

Review of attachment 321895 [details] [review]:

I think this makes sense, but it's probably too late in the cycle to change this behvior, we will have to wit for the next cycle.

::: embed/ephy-embed.c
@@ +86,3 @@
   gulong progress_update_handler_id;
+
+  EphyEmbed *parent_embed;

Since this is EphyEmbed, this could be just parent

@@ +87,3 @@
+
+  EphyEmbed *parent_embed;
+  guint embeds_from_parent;

I think this name is a bit confusing, I guess this is actually the distance to the parent?

@@ +939,3 @@
+
+EphyEmbed *
+ephy_embed_get_parent_embed (EphyEmbed *embed)

ephy_embed_get_parent()

@@ +947,3 @@
+
+void
+ephy_embed_set_parent_embed (EphyEmbed *embed,

ephy_embed_set_parent()

@@ +966,3 @@
+
+guint
+ephy_embed_get_embeds_from_parent (EphyEmbed *embed)

get_distance_to_parent?

@@ +974,3 @@
+
+void
+ephy_embed_set_embeds_from_parent (EphyEmbed *embed,

set_distance_to_parent?

::: src/ephy-shell.c
@@ +670,3 @@
     if (previous_embed) {
+      GtkWidget *notebook = ephy_window_get_notebook (window);
+      guint offset = ephy_embed_get_embeds_from_parent (previous_embed);

Is this still valid if any of the tabs is removed or reordered?

@@ +682,3 @@
+        ephy_embed_set_embeds_from_parent (previous_embed, offset + 1);
+        previous_embed = ephy_embed_get_parent_embed (previous_embed);
+      }

I'm not sure I understand this loop

(In reply to Carlos Garcia Campos from comment #10)
> Review of attachment 321895 [details] [review] [review]:
>
> +  guint embeds_from_parent;
> 
> I think this name is a bit confusing, I guess this is actually the distance
> to the parent?

This value basically represents the distance of the next tab to be opened from the current tab to the current tab's position.

E.g.
If tab A has this value == 1 then the next tab you open from A will have its position == A's position + 1.
If tab A has this value == 2 then the next tab you open from A will have its position == A's position + 2.

So that's why I chose to name it 'children_offset' in the first place but I guess 'distance_to_next_child' would be more appropriate.

> @@ +670,3 @@
>      if (previous_embed) {
> +      GtkWidget *notebook = ephy_window_get_notebook (window);
> +      guint offset = ephy_embed_get_embeds_from_parent (previous_embed);
> 
> Is this still valid if any of the tabs is removed or reordered?

There should be no problem in removing a tab. I took care of this in ephy_window_close_tab() by decrementing the offset.

But I haven't really considered reordering tabs.. or dragging a tab to a new window..?

Turns out there are many ways to open/close/move tabs around; every one of them should be handled properly so I guess we should decide a behavior for each.

> @@ +682,3 @@
> +        ephy_embed_set_embeds_from_parent (previous_embed, offset + 1);
> +        previous_embed = ephy_embed_get_parent_embed (previous_embed);
> +      }
> 
> I'm not sure I understand this loop

Well, if the parent tab has a parent of its own (was opened from another tab too) then you have to increment the grandparent's offset too. That's why you have to go back in the parents line and increment every offset.

Let me give you an example:

Say you have tabs A B.
(By default every tab has offset == 1)

If you open links 1 2 from tab A you'll get: A 1 2 B.
(A's offset will now be 3)

If you now open links C D from tab 1 you'll get: A 1 C D 2 B.
(A's offset is still 3 at this moment; note that 1's offset will also be 3, but that's not important in this scenario)

So what happens if you decide to open other tabs from tab A? Say you now open link 3 from tab A. By my judgement, tab 3 should be opened to the right of tab 2 (this being the last tab opened from A). For that to happen tabs C and D need to be skipped. But given that we didn't update A's offset when tabs C and D were opened, tab 3 will be opened to the right of tab C instead of tab 2!

The correct way to go is: when you open tabs C and D go back to the parent (tab 1) and increment its offset, but also go back to tab 1's parent (tab A) and increment its offset too.

So this is what this loop is used for; it loops through every parent in the parents line and increments their offset. Same thing for closing a tab, only that you decrement the offset there (see the loop in ephy_window_close_tab()).

Hope I made it somehow clear now.

P.S. In the above scenario tabs C and D are the children, tab 1 is the parent, tab A is the grandfather.

I want to add that all this is just my idea of the desired behavior of opening tab, so if you, Michael or anyone else decide to switch to different behavior please let me know. But as I've previously said there are many ways in which you can play with tabs so we need a consistent behavior for each of them (I'll try to come up with a new patch these days to cover all the cases).

Unfortunately this bug is stuck until GtkNotebook grows some animation to make clear when tabs are added at the edge of the notebook. :(

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/epiphany/issues/238.