yeah this is a good idea.

Created attachment 20512 [details] [review]
the beginnings of an implementation

My patch has some infrastructure work to get more info about the
session history items, and saves the info in the session file. Now if
I could find a way to re-populate the session history with the entries
on loading the session...

What's the prob exactly ? My impression is that we are going to get
problems with embed initialization here ...

Adding the PATCH keyword.

Removing PATCH keyword, since the patch is not complete and even
doesn't apply cleanly anymore.

Comment on attachment 20512 [details] [review]
the beginnings of an implementation

Marking attachments in open bugs.

Would this RFE also apply to 'crashed' sessions, so that Back and Forward would
retain the history per page?

Yes, since the session saving code is the same for crashed sessions and
session-managed sessions.

-> History

Mass reassigning of Epiphany bugs to epiphany-maint@b.g.o

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

Created attachment 33661 [details] [review]
another half-finished patch

This is a half-finished, non-compiling patch.

Since I could not find ANY way to create a new instance of nsISHEntry, I had to
abandon it. It seems there is NO way to implement this currently :(

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

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

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

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

Any chance of this feature being implemented with the webkit backend?

Sounds like bug 706747 is a DUPLICATED of this bug ?

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

Created attachment 319027 [details] [review]
embed: Remove unused EphyEmbedShell::prepare-close signal

Created attachment 319028 [details] [review]
ephy-session: always set dont_save to false in ephy_session_close()

Created attachment 319029 [details] [review]
ephy-session: Never save the session if the policy is set to never

Created attachment 319030 [details] [review]
ephy-session: Remove filename parameter from ephy_session_save()

Created attachment 319031 [details] [review]
ephy-session: do not start the save task immediately

Created attachment 319032 [details] [review]
ephy-session: Save the session also when active tab changes

Created attachment 319033 [details] [review]
ephy-session: save and restore web view session state

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

We still need to save/restore the session on recently closed tabs too, but I'll use a different bug for that.

Review of attachment 319027 [details] [review]:

OK

Review of attachment 319028 [details] [review]:

It's a bit confusing that ephy_session_close just causes further session changes to be discarded... I also don't understand why this behavior was previously specific to EPHY_PREFS_RESTORE_SESSION_POLICY_ALWAYS.

Review of attachment 319029 [details] [review]:

OK

Review of attachment 319030 [details] [review]:

::: src/ephy-session.c
@@ +827,3 @@
+		GFile *session_file;
+
+		session_file = get_session_file (SESSION_STATE);

Can't you also get rid of this parameter to get_session_file(), and then simplify the implementation of get_session_file()?

Review of attachment 319031 [details] [review]:

::: src/ephy-session.c
@@ +546,3 @@
+		 */
+		g_source_remove (priv->save_source_id);
+		ephy_session_save_idle_cb (session);

I think it's worthwhile to defensively set priv->save_source_id = 0 here, even though it is not currently necessary.

Review of attachment 319032 [details] [review]:

::: src/ephy-session.c
@@ +436,3 @@
 	g_signal_connect (notebook, "page-reordered",
 			  G_CALLBACK (notebook_page_reordered_cb), session);
+	g_signal_connect_after (notebook, "switch-page",

I might have used g_signal_connect_data() here, with G_CONNECT_AFTER | G_CONNECT_SWAPPED, to call ephy_session_save() directly and avoid notebook_switch_page_cb(). But this is fine too.

Review of attachment 319033 [details] [review]:

::: embed/ephy-embed.c
@@ +649,3 @@
+  item = webkit_back_forward_list_get_current_item (webkit_web_view_get_back_forward_list (WEBKIT_WEB_VIEW (web_view)));
+  if (item)
+    webkit_web_view_go_to_back_forward_list_item (WEBKIT_WEB_VIEW (web_view), item);

Why do you need to check the back/forward list? Shouldn't the final item in the back/forward list always match delayed_request?

@@ +886,3 @@
  * @embed: a #EphyEmbed
  * @request: a #WebKitNetworkRequest
+ * @state: a #WebKitWebViewSessionState

(nullable)

::: src/ephy-session.c
@@ +599,3 @@
 	session_tab->title = g_strdup (ephy_embed_get_title (embed));
 	session_tab->loading = ephy_web_view_is_loading (web_view) && !ephy_embed_has_load_pending (embed);
+	session_tab->state = webkit_web_view_get_session_state (WEBKIT_WEB_VIEW (web_view));//webkit_web_view_session_state_serialize(state);

Remove the leftover comment.

@@ +1176,3 @@
+			if (item)
+			{
+				webkit_web_view_go_to_back_forward_list_item (WEBKIT_WEB_VIEW (web_view), item);

Same question here.

(In reply to Michael Catanzaro from comment #33)
> Review of attachment 319030 [details] [review] [review]:
> 
> ::: src/ephy-session.c
> @@ +827,3 @@
> +		GFile *session_file;
> +
> +		session_file = get_session_file (SESSION_STATE);
> 
> Can't you also get rid of this parameter to get_session_file(), and then
> simplify the implementation of get_session_file()?

No, because we support loading the session from any file path, and this method is also used in that case in the load methods.

(In reply to Michael Catanzaro from comment #31)
> Review of attachment 319028 [details] [review] [review]:
> 
> It's a bit confusing that ephy_session_close just causes further session
> changes to be discarded... I also don't understand why this behavior was
> previously specific to EPHY_PREFS_RESTORE_SESSION_POLICY_ALWAYS.

Yes, we could probably use a better name. The thing is that the session watches the state of the windows and tabs. When closing the application, tabs are removed and other stuff might change, but we don't want to save that changes or we will always end up with an empty session. This also probably made more sense if save was actually public and could be called from other methods, but it's not the case so we could probably disconnect all the signals in close to ensure we don't save the session after close. But anyway, current solution is simple enough and it just works.

(In reply to Michael Catanzaro from comment #34)
> Review of attachment 319031 [details] [review] [review]:
> 
> ::: src/ephy-session.c
> @@ +546,3 @@
> +		 */
> +		g_source_remove (priv->save_source_id);
> +		ephy_session_save_idle_cb (session);
> 
> I think it's worthwhile to defensively set priv->save_source_id = 0 here,
> even though it is not currently necessary.

It's the first thing ephy_session_save_idle_cb does and it will always do that, but I don't mind making it explicit here for clarity.

(In reply to Michael Catanzaro from comment #36)
> Review of attachment 319033 [details] [review] [review]:
> 
> ::: embed/ephy-embed.c
> @@ +649,3 @@
> +  item = webkit_back_forward_list_get_current_item
> (webkit_web_view_get_back_forward_list (WEBKIT_WEB_VIEW (web_view)));
> +  if (item)
> +    webkit_web_view_go_to_back_forward_list_item (WEBKIT_WEB_VIEW
> (web_view), item);
> 
> Why do you need to check the back/forward list? Shouldn't the final item in
> the back/forward list always match delayed_request?

No, if delayed_state is NULL (that will happen for sure the first time your run this code because your current state file doesn't have state data, and it could happen if for some reason something fails when serializing state), then the bf list will be empty. It can also happen if delayed_state is not NULL but webkit_web_view_restore_session_state fails, for example if serialized data is invalid or corrupt. So, if we have an item in the bf list it means the session was correctly restored.

> @@ +886,3 @@
>   * @embed: a #EphyEmbed
>   * @request: a #WebKitNetworkRequest
> + * @state: a #WebKitWebViewSessionState
> 
> (nullable)

Ok.

> ::: src/ephy-session.c
> @@ +599,3 @@
>  	session_tab->title = g_strdup (ephy_embed_get_title (embed));
>  	session_tab->loading = ephy_web_view_is_loading (web_view) &&
> !ephy_embed_has_load_pending (embed);
> +	session_tab->state = webkit_web_view_get_session_state (WEBKIT_WEB_VIEW
> (web_view));//webkit_web_view_session_state_serialize(state);
> 
> Remove the leftover comment.

Oops

> @@ +1176,3 @@
> +			if (item)
> +			{
> +				webkit_web_view_go_to_back_forward_list_item (WEBKIT_WEB_VIEW
> (web_view), item);
> 
> Same question here.

Same answer.

(In reply to Carlos Garcia Campos from comment #40)
> No, if delayed_state is NULL (that will happen for sure the first time your
> run this code because your current state file doesn't have state data, and
> it could happen if for some reason something fails when serializing state),
> then the bf list will be empty.

Sure.

> It can also happen if delayed_state is not
> NULL but webkit_web_view_restore_session_state fails, for example if
> serialized data is invalid or corrupt.

Sure.

> So, if we have an item in the bf list
> it means the session was correctly restored.

Sure, but so what? I understand that this code:

+			item = webkit_back_forward_list_get_current_item (bf_list);
+			if (item)
+			{
+				webkit_web_view_go_to_back_forward_list_item (WEBKIT_WEB_VIEW (web_view), item);
+			}
+			else
+			{
+				ephy_web_view_load_url (web_view, url);
+			}

works and is valid, but why is it better to check the b/f list than to simply call ephy_web_view_load_url (web_view, url). The last item in the WebKit session's b/f list should be the same as the one saved in the Epiphany session XML, so I don't see much value in checking the b/f list at all. (This is a fairly minor point.)

(In reply to Michael Catanzaro from comment #41)
> (In reply to Carlos Garcia Campos from comment #40)
> > No, if delayed_state is NULL (that will happen for sure the first time your
> > run this code because your current state file doesn't have state data, and
> > it could happen if for some reason something fails when serializing state),
> > then the bf list will be empty.
> 
> Sure.
> 
> > It can also happen if delayed_state is not
> > NULL but webkit_web_view_restore_session_state fails, for example if
> > serialized data is invalid or corrupt.
> 
> Sure.
> 
> > So, if we have an item in the bf list
> > it means the session was correctly restored.
> 
> Sure, but so what? I understand that this code:
> 
> +			item = webkit_back_forward_list_get_current_item (bf_list);
> +			if (item)
> +			{
> +				webkit_web_view_go_to_back_forward_list_item (WEBKIT_WEB_VIEW
> (web_view), item);
> +			}
> +			else
> +			{
> +				ephy_web_view_load_url (web_view, url);
> +			}
> 
> works and is valid, but why is it better to check the b/f list than to
> simply call ephy_web_view_load_url (web_view, url). The last item in the
> WebKit session's b/f list should be the same as the one saved in the
> Epiphany session XML, so I don't see much value in checking the b/f list at
> all. (This is a fairly minor point.)

When you restore the session, the bf list current item is the one you want to load, but there's nothing loaded in the webview, so if you do load_url, a new entry will be added to the bf list, while we want to keep the current one.

I agree that maybe it doesn't look obvious that you need to do a bf navigation, and I've noticed today that it causes problems, for example today after restoring my session, some of the tabs were loaded with old content (for example this one had a new comment that I didn't see until I manually reloaded the page). I'm not sure if that's caused by the bf navigation, I assumed the page cache was empty. This is what WebKit does internally when navigate is true in WebPageProxy::restoreFromSessionState() after restoring the session.

// FIXME: Navigating should be separate from state restoration.
if (navigate) {
    m_sessionRestorationRenderTreeSize = sessionState.renderTreeSize;
    if (!m_sessionRestorationRenderTreeSize)
        m_hitRenderTreeSizeThreshold = true; // If we didn't get data on renderTreeSize, just don't fire the milestone.
    if (!sessionState.provisionalURL.isNull())
        return loadRequest(sessionState.provisionalURL);

    if (hasBackForwardList) {
        // FIXME: Do we have to null check the back forward list item here?
    if (WebBackForwardListItem* item = m_backForwardList->currentItem())
            return goToBackForwardItem(item);
    }
}

So, to make it easier, we could add a specific load method. I didn't add navigate parameter to our API because of the FIXME above. So, maybe, webkit_web_view_load_from_session_state (WebKitWebView *, WebKitWebViewSessionState *)

That would allow us to use any other internal information of the SessionState without exposing it to the user.

(In reply to Carlos Garcia Campos from comment #42)
> When you restore the session, the bf list current item is the one you want
> to load, but there's nothing loaded in the webview, so if you do load_url, a
> new entry will be added to the bf list, while we want to keep the current
> one.

OK, would be good to add a comment to warn about this.

(In reply to Carlos Garcia Campos from comment #43)
> I agree that maybe it doesn't look obvious that you need to do a bf
> navigation, and I've noticed today that it causes problems, for example
> today after restoring my session, some of the tabs were loaded with old
> content (for example this one had a new comment that I didn't see until I
> manually reloaded the page).

Hm, that is a big problem. It seems our session state mechanism is TOO good.

> So, to make it easier, we could add a specific load method. I didn't add
> navigate parameter to our API because of the FIXME above. So, maybe,
> webkit_web_view_load_from_session_state (WebKitWebView *,
> WebKitWebViewSessionState *)
> 
> That would allow us to use any other internal information of the
> SessionState without exposing it to the user.

Sounds good.

(In reply to Michael Catanzaro from comment #44)
> (In reply to Carlos Garcia Campos from comment #42)
> > When you restore the session, the bf list current item is the one you want
> > to load, but there's nothing loaded in the webview, so if you do load_url, a
> > new entry will be added to the bf list, while we want to keep the current
> > one.
> 
> OK, would be good to add a comment to warn about this.
> 
> (In reply to Carlos Garcia Campos from comment #43)
> > I agree that maybe it doesn't look obvious that you need to do a bf
> > navigation, and I've noticed today that it causes problems, for example
> > today after restoring my session, some of the tabs were loaded with old
> > content (for example this one had a new comment that I didn't see until I
> > manually reloaded the page).
> 
> Hm, that is a big problem. It seems our session state mechanism is TOO good.

I'll check it in detail, it could be that the page was loaded from the disk cache, bypassing the revalidation because of the bf navigation.

> > So, to make it easier, we could add a specific load method. I didn't add
> > navigate parameter to our API because of the FIXME above. So, maybe,
> > webkit_web_view_load_from_session_state (WebKitWebView *,
> > WebKitWebViewSessionState *)
> > 
> > That would allow us to use any other internal information of the
> > SessionState without exposing it to the user.
> 
> Sounds good.

(In reply to Carlos Garcia Campos from comment #45)
> (In reply to Michael Catanzaro from comment #44)
> > (In reply to Carlos Garcia Campos from comment #42)
> > > When you restore the session, the bf list current item is the one you want
> > > to load, but there's nothing loaded in the webview, so if you do load_url, a
> > > new entry will be added to the bf list, while we want to keep the current
> > > one.
> > 
> > OK, would be good to add a comment to warn about this.
> > 
> > (In reply to Carlos Garcia Campos from comment #43)
> > > I agree that maybe it doesn't look obvious that you need to do a bf
> > > navigation, and I've noticed today that it causes problems, for example
> > > today after restoring my session, some of the tabs were loaded with old
> > > content (for example this one had a new comment that I didn't see until I
> > > manually reloaded the page).
> > 
> > Hm, that is a big problem. It seems our session state mechanism is TOO good.
> 
> I'll check it in detail, it could be that the page was loaded from the disk
> cache, bypassing the revalidation because of the bf navigation.
> 

This is exactly what happens, see https://bugs.webkit.org/show_bug.cgi?id=142831 and https://bugs.webkit.org/show_bug.cgi?id=153230

All patches pushed to git master, depending on 2.11.4, since it contains important fixes in session serializing/deserializing code to prevent data corruption.

I am concerned about the cache revalidation issue we have introduced with this patchset. I trust you will try to fix that before 3.20.