Last Comment Bug 604463 - Request for session store to retain the browsing history and scroll offsets
: Request for session store to retain the browsing history and scroll offsets
Status: VERIFIED FIXED
[fennec-sessionstore]
: verified-aurora
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: x86 Mac OS X
: -- enhancement (vote)
: Firefox 7
Assigned To: Mark Finkle (:mfinkle) (use needinfo?)
:
Mentors:
: 616338 638167 653669 (view as bug list)
Depends on: 666577 668379
Blocks: 639631 685127
  Show dependency treegraph
 
Reported: 2010-10-14 13:01 PDT by Naoki Hirata :nhirata (please use needinfo instead of cc)
Modified: 2013-11-11 22:07 PST (History)
11 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
wip 1 (14.65 KB, patch)
2011-05-05 06:13 PDT, Mark Finkle (:mfinkle) (use needinfo?)
no flags Details | Diff | Splinter Review
patch (18.61 KB, patch)
2011-06-21 10:48 PDT, Mark Finkle (:mfinkle) (use needinfo?)
wjohnston2000: review+
mbrubeck: review-
Details | Diff | Splinter Review
patch 2 (18.79 KB, patch)
2011-06-21 21:18 PDT, Mark Finkle (:mfinkle) (use needinfo?)
mbrubeck: review+
Details | Diff | Splinter Review

Description Naoki Hirata :nhirata (please use needinfo instead of cc) 2010-10-14 13:01:02 PDT
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?) 2010-10-14 16:31:31 PDT
Session store is the underlying mechanism, so changing the summary to fit.
Comment 2 Mark Finkle (:mfinkle) (use needinfo?) 2010-12-02 21:56:20 PST
*** Bug 616338 has been marked as a duplicate of this bug. ***
Comment 3 Mark Finkle (:mfinkle) (use needinfo?) 2011-05-05 06:13:05 PDT
Created attachment 530290 [details] [diff] [review]
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?) 2011-05-05 07:23:26 PDT
Also, this patch stores scroll positions too (bug 638167)
Comment 5 Mark Finkle (:mfinkle) (use needinfo?) 2011-05-19 21:44:49 PDT
Scroll offsets are part of the session history. Saving one gets the other for free.
Comment 6 Mark Finkle (:mfinkle) (use needinfo?) 2011-05-19 21:45:08 PDT
*** Bug 638167 has been marked as a duplicate of this bug. ***
Comment 7 Mark Finkle (:mfinkle) (use needinfo?) 2011-05-19 21:45:58 PDT
*** Bug 653669 has been marked as a duplicate of this bug. ***
Comment 8 Cristian Nicolae (:xti) 2011-05-23 07:16:18 PDT
TC https://litmus.mozilla.org/show_test.cgi?id=13667 fails due to this bug.
Comment 9 Mark Finkle (:mfinkle) (use needinfo?) 2011-06-21 10:48:16 PDT
Created attachment 540807 [details] [diff] [review]
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 11 Matt Brubeck (:mbrubeck) 2011-06-21 13:45:16 PDT
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) 2011-06-21 14:23:31 PDT
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?) 2011-06-21 21:18:14 PDT
(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?) 2011-06-21 21:18:43 PDT
Created attachment 540965 [details] [diff] [review]
patch 2

With changes
Comment 16 Andreea Pod 2011-07-13 06:52:01 PDT
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

Note You need to log in before you can comment on or make changes to this bug.