604463 - Request for session store to retain the browsing history and scroll offsets

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Mozilla/5.0 (Maemo;Linux armv71; rv:2.0b8pre) Gecko/20101014 Firefox/4.0b8pre Fennec/4.0b2pre
Mozilla/5.0 (Android; Linux armv71; rv2.0b8pre) Gecko/20101014 Firefox/4.0b8pre Fennec/4.0b2pre

1. go to www.google.com in another tab
2. do a search for blah
3. click on the first link
4. close the link
5. click the undo tab close icon in the tab panel
6. go to the control panel to go back a page

Expected: the control panel would be able to go back
Actual: the control panel cannot go back a page.

Note: 
1. it would be helpful to have a back history for at least the last undo'ed tab if we're only going to have one tab that we can undo closing.  Use case is if the user accidentally clicked the close tab instead of switching to a tab.  The person loses all history associated with it.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Session store is the underlying mechanism, so changing the summary to fit.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: wip 1

This patch is kind of a brute-force approach. I collect the session history during the OnLocation change and send it to the parent. SessionStroe listens for the new Content:SessionHistory message and uses it to store the session history in the tab.

We also use Content:SessionHistory as the "save state to memory ASAP" message too.

During a restore, SessionStore now sends the LoadURI message manually, so we can send along the session history data too. The child script WebNavigation handler looks for the extra data and uses it to restore the session history.

STATUS:
* Session history is saved to the sessionstore file. I don't know what performance impact is yet. Need to tests that.
* Restoring the session seems to be broken. Debugging that now.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Also, this patch stores scroll positions too (bug 638167)

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Scroll offsets are part of the session history. Saving one gets the other for free.

Comment 8

[Cristian Nicolae (:xti)]

TC https://litmus.mozilla.org/show_test.cgi?id=13667 fails due to this bug.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

This patch actually seems to work! Much of the serialization / deserialization came directly from desktop session store code. We needed to split the code up (parent and child). Basic flow:
1. On a location change, we serialize the session history into JSON and send a message to the parent
2. SessionStore component listens for the message and stores the history with the session store data
3. When we restart after a crash, we do a special "LoadURI" message that also has the session history
4. When the child receives the "LoadURI" message:
  * Stop the current load - we only used the URL to trick the rest of the code into working correctly (local and remote stuff)
  * Load the session history
  * Activate the right history item and reload it. This makes it appear in the browser but doesn't add a second history item.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

We'll find the desktop serialization code in here:
http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#1719
http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#2987

Comment 11

[Matt Brubeck (:mbrubeck)]

Comment on attachment 540807 [details] [diff] [review]
patch

Review of attachment 540807 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me aside from some minor issues below, but I'd like to re-review the final patch before checkin.

::: mobile/chrome/content/bindings/browser.js
@@ +254,5 @@
> +      // want the load added to the history anyway. We reload after resetting history
> +      this._webNavigation.stop(this._webNavigation.STOP_ALL);
> +
> +      // We need to wait for the sessionHistory to be initialized and there
> +      // is no good way to do this. We'll try a wait loop like desktop

This doesn't look like a wait-loop to me - it looks like we just always wait 300ms and then try once, unless I'm missing something.

@@ +325,5 @@
> +    if (aEntry.stateData)
> +      shEntry.stateData = aEntry.stateData;
> +
> +    if (aEntry.scroll) {
> +      let scrollPos = (aEntry.scroll || "0,0").split(",");

The "if" and the "||" are redundant (not your code, but should be fixed for clarity anyway).

Comment 12

[Wesley Johnston (:wesj)]

Comment on attachment 540807 [details] [diff] [review]
patch

Review of attachment 540807 [details] [diff] [review]:
-----------------------------------------------------------------

With Matt's comments looks good to me.

::: mobile/chrome/content/bindings/browser.js
@@ +96,5 @@
> +
> +    let x = {}, y = {};
> +    aEntry.getScrollPosition(x, y);
> +    if (x.value != 0 || y.value != 0)
> +      entry.scroll = x.value + "," + y.value;

Know its copied, but this seems like an odd way to serialize this, and fixing it would simplify the De-serializing code too.

@@ +130,5 @@
> +
> +    if (aEntry.childCount > 0) {
> +      entry.children = [];
> +      for (let i = 0; i < aEntry.childCount; i++) {
> +        var child = aEntry.GetChildAt(i);

Any reason not use let here?

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #11)

> ::: mobile/chrome/content/bindings/browser.js
> @@ +254,5 @@
> > +      // want the load added to the history anyway. We reload after resetting history
> > +      this._webNavigation.stop(this._webNavigation.STOP_ALL);
> > +
> > +      // We need to wait for the sessionHistory to be initialized and there
> > +      // is no good way to do this. We'll try a wait loop like desktop
> 
> This doesn't look like a wait-loop to me - it looks like we just always wait
> 300ms and then try once, unless I'm missing something.

Oops. I added a real looper (10 loops max, like desktop)

> > +    if (aEntry.scroll) {
> > +      let scrollPos = (aEntry.scroll || "0,0").split(",");
> 
> The "if" and the "||" are redundant (not your code, but should be fixed for
> clarity anyway).

I removed the ( || "0,0") part 

(In reply to comment #12)

> ::: mobile/chrome/content/bindings/browser.js

> > +    aEntry.getScrollPosition(x, y);
> > +    if (x.value != 0 || y.value != 0)
> > +      entry.scroll = x.value + "," + y.value;
> 
> Know its copied, but this seems like an odd way to serialize this, and
> fixing it would simplify the De-serializing code too.

I left this as-is since it matches the deserialization format that desktop uses.

> > +      for (let i = 0; i < aEntry.childCount; i++) {
> > +        var child = aEntry.GetChildAt(i);
> 
> Any reason not use let here?

Changed to let

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 2

With changes

Comment 15

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/b25a0fc03c3a

Comment 16

[Andreea Pod]

Verified fixed on Nightly: Mozilla /5.0 (Android;Linux armv7l;rv:8.0a1) Gecko/20110713 Firefox/8.0a1 Fennec/8.0a1 

and Aurora: Mozilla /5.0 (Android;Linux armv7l;rv:7.0a2) Gecko/20110712 Firefox/7.0a2 Fennec/7.0a2

Device: LG Optimus 2X