Created attachment 149245 [details]
0.0.2 of tab-restore, detach fixed, limited array of MAX_ELEM entries used

Created attachment 149246 [details]
0.0.1c (as port to C of 0.0.1 in python) of tab-restore, detach broken, unlimited GList used

Created attachment 149247 [details] [review]
0.0.1c of tab-restore in form of a patch against git master

Review of attachment 149247 [details] [review]:

Some comments so you can produce a second iteration of it.
Tip:
git commit --amend

so you can add to your latest commit whatever you change.

::: extensions/tab-restore/ephy-tabrestore-extension.c
@@ +77,3 @@
+
+static void
+ephy_sample_extension_finalize (GObject *object)

You can rename all the "_sample_"s to "_tabrestore_" or something you like.

@@ +157,3 @@
+
+static void
+impl_attach_tab (EphyExtension *ext,

You can also remove this two functions since you are not using them at all (and their mention in the init() function).

@@ +174,3 @@
+	/* attach the url loaded in the tab to the url-list */
+	EphyWebView* webview = ephy_embed_get_web_view(embed);
+	const char *temp = ephy_web_view_get_location (webview, 1);

That 1 should be TRUE.

@@ +195,3 @@
+
+static void
+ephy_sample_extension_get_property (GObject *object,

I think you can remove this two functions (get/set) and their mention in ephy_sample_extension_class_init()

Created attachment 149248 [details]
0.0.1c of tab-restore in form of a patch against git master

Created attachment 149249 [details] [review]
0.0.1c of tab-restore in form of a patch against git master including configure.ac

Created attachment 149250 [details]
0.0.2 of tab-restore, patch against git, fixes detach and uses ringbuffer

Review of attachment 149249 [details] [review]:

::: extensions/tab-restore/ephy-tabrestore-extension.c
@@ +42,3 @@
+{
+	PROP_0
+};

You can nuke since you have no properties and you are not going to use it (the enum).

@@ +83,3 @@
+	EphyTabRestoreExtension *extension = EPHY_TABRESTORE_EXTENSION (object);
+	extension->priv = EPHY_TABRESTORE_EXTENSION_GET_PRIVATE (extension);
+	g_list_free_1(extension->priv->urls); /* Dissolve list after use */

You want g_list_free, since free_1 only frees the element where the list is pointing *now*. According to glib docs at least :).

Created attachment 149253 [details] [review]
0.0.2 of tab-restore including correct function-names, ringbuffer and detach-fix

Created attachment 149256 [details] [review]
0.0.3 Clean up in accordance with comment 4

Created attachment 149283 [details] [review]
Adds sensitivity changes of the action, and does this respecting the global list

Created attachment 149285 [details] [review]
Introducing tab-restore with global url list

Created attachment 149286 [details] [review]
Introducing tab-restore with global url list

I think the patch of Dec 07 should be in an acceptable state and I'd like to kindly ask for you integration into epiphany-extensions.

I furthermore worked on augmenting it for restoring a whole tab instead of the URL only. Unfortunately this leads to a segfault in my code in file ephy-tabrestore-extension.c, line 146, so I must misunderstand the API. Hints are appreciated.

Created attachment 150059 [details] [review]
Segfaults! Attached since I need help here. Please see my comment.

Review of attachment 149286 [details] [review]:

Hi Daniel, sorry for not reviewing before... holidays where hectic :-).

I think this is looking much better now, we will surely get it done in time for 2.30 if we can test it enough to be bug free. I've added some comments to your current patch, the main being your use of a char array instead of a GList. I think you should use the later, is there any reason you didn't?

Also we should try to get some opinions on the scope of restoring, if it should be per-window or global. I feel global would make more sense, like it is now.

Thanks for your hard work!

::: extensions/tab-restore/ephy-tabrestore-extension.c
@@ +45,3 @@
+struct _EphyTabRestoreExtensionPrivate
+{
+	char *urls[MAX_ELEM];	

Why not a GList for this? wouldn't you save the trouble of maintaining current and next index numbers that way?

@@ +93,3 @@
+	}
+	extension->priv->current = 9;
+	extension->priv->next = 0;

If you have MAX_ELEM defined, this 9 and 0 are hardcoded. I insist I feel it would be easier with a GList or GSList, it's not really expensive for performance so feel free to try that.

@@ +120,3 @@
+		WindowData *data = NULL;
+		data = (WindowData *) g_object_get_data (G_OBJECT (window), WINDOW_DATA_KEY);
+		gtk_action_group_set_sensitive(data->action_group, *bool_flag);

You have a different indentation here.

@@ +130,3 @@
+	LOG ("EphyTabRestoreExtension _restore_tab_cb");
+	
+	EphyTabRestoreExtension *extension = g_object_get_data (G_OBJECT (window), EPHY_TABRESTORE_EXTENSION_DATA_KEY);

I think you don't really need this key. You currently use it because you need access to priv->urls, priv->current and priv->next. IMHO by using the GList I suggest it would be enough to pass such list as the user_data of this callback (you wouldn't have use for current and next anyway if you are using a GList).
Note that GList *my_list is always a pointer to the current my_list element, this plus my_list->next would fit your needs. Although maybe I'm just not seeing the why behind your method, feel free to disagree and explain it :^). Still, I strongly suggest you at least try using a GList.

@@ +147,3 @@
+		ephy_shell_new_tab (shell, window, NULL, temp, EPHY_NEW_TAB_IN_EXISTING_WINDOW |
+				EPHY_NEW_TAB_OPEN_PAGE |
+				EPHY_NEW_TAB_JUMP);

Where NULL it's the embed from where you "come". For example if you are in tab "1" and create a new one, then @previous_embed would be "1" and that causes Epiphany to know that it should copy browsing history from that tab for example.
So the FIXME is valid, you should pass the current page embed (gtk_notebook_get_curren_page() and from there get the EphyEmbed I mean) instead of NULL.

@@ +219,3 @@
+
+	/* Every window needs to register at the extension */
+	extension->priv->regwindows = g_list_append(extension->priv->regwindows, window);

Here and in a few other places you call functions like func(), notice that the convention in Epiphany and other GNOME programs is to do func (), with a space after the name and before the ().

::: extensions/tab-restore/ephy-tabrestore-extension.h
@@ +58,3 @@
+GType	ephy_tabrestore_extension_get_type		(void);
+
+GType	ephy_tabrestore_extension_register_type	(GTypeModule *module);

Peeve about alignment of the () in both funcs :-P

Review of attachment 150059 [details] [review]:

Well, after a quick look I think the problem could be that you need Epiphany itself not dispose the Embed on tab close. Although I think g_object_ref () should be enough I wouldn't bet on it.
Did you see epiphany's code executed when a tab is closed?

You said you have a crash, I couldn't find the line. It's easier if you just paste the code corresponding to that line and the line number since the number alone is sometimes tricky. Where's the crash precisely?

::: extensions/tab-restore/ephy-tabrestore-extension.c
@@ +106,3 @@
 	/* Dissolve all elements */
 	for(i = 0; i < MAX_ELEM; i++) {
+		g_object_unref(extension->priv->embeds[i]); // FIXME: Is it still Per definition NO-OP on NULL?

I think g_object_unref fails if the object is NULL. This is usually the way it's done:

if (some_object)
    g_object_unref (some_object);

Diego, thank you for your comments. I corrected the issues you mentioned and of course I fully agree that the number 9 shouldn't be hardcoded. I also added the previous embed mentioned in the FIXME of the previous version and corrected the coding-style nitpicks, hope I found and corrected all of them!

I don't agree on using GList. As you can see from the previous discussion in this thread my first versions used GList. I dropped this since GList does not support the limitation to a fixed number of elements in the list. Hence we would either create a great memory-sink (especially once we save Embeds instead of URLs) or we'd manually throw away the oldest element. If the latter I don't see the benefit of using GList anymore since implementing the ringbuffer is not more complicated than that and seems to work quite fine. Also any GList implementation would be less efficient if it takes MAX_ELEM into account while the current implementation seems to be the most efficient easy implementation.

Regarding the second patch with the segfaults on saving and restoring Embeds: It segfaults at line: ephy_embed_container_add_child(EPHY_EMBED_CONTAINER(window), embed, 0, TRUE); More help would be highly appreciated here!

Created attachment 154760 [details] [review]
Introducing tab-restore with global url list

Any opinions, remarks, news?

Am I a bad maintainer or what? Sorry Daniel, I'll get back to you this week, probably tomorrow.

Hey Daniel, I looked at your patch and it seems quite good. I think it's mostly ready now. I saw your comment about GList, makes sense.

About per-window or global, I think global is the way to go, this is the rationale for the record:

Global:
- Closed windows get added to the list
- You can close in window A and restore in window C
- You don't loose the closed url if it was the last tab in the window

Per window:
- Close windows are lost
- You get a different menu in every window
- You have to /think/ on which window you are

Let me get an extra pair of eyes on the patch and we are good to go. Do you have commit access?

Thanks for taking time for a review. I do not have commit access. Extra pair of eyes sounds good to me.

*** Bug 672779 has been marked as a duplicate of this bug. ***

bug 128184 should also be marked. 

Can the patch just be merged? What happened to the extra pair of eyes?

*** This bug has been marked as a duplicate of bug 128184 ***