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

VERIFIED FIXED in Firefox 7

Status

Fennec Graveyard
General
--
enhancement
VERIFIED FIXED
7 years ago
4 years ago

People

(Reporter: nhirata, Assigned: mfinkle)

Tracking

(Blocks: 2 bugs, {verified-aurora})

Trunk
Firefox 7
x86
Mac OS X
verified-aurora
Dependency tree / graph

Details

(Whiteboard: [fennec-sessionstore])

Attachments

(1 attachment, 2 obsolete attachments)

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.
(Assignee)

Updated

7 years ago
tracking-fennec: --- → ?
Summary: Request for undo'ed tab to retain the browsing history → Request for session store to retain the browsing history
Session store is the underlying mechanism, so changing the summary to fit.
(Assignee)

Updated

7 years ago
Assignee: nobody → mark.finkle
tracking-fennec: ? → 2.0-
(Assignee)

Updated

7 years ago
Duplicate of this bug: 616338
(Assignee)

Updated

6 years ago
tracking-fennec: 2.0- → 2.0next+
(Assignee)

Updated

6 years ago
Whiteboard: [fennec-sessionstore]
(Assignee)

Updated

6 years ago
Whiteboard: [fennec-sessionstore] → [fennec-6] [fennec-sessionstore]
tracking-fennec: 2.0next+ → 6+
Whiteboard: [fennec-6] [fennec-sessionstore] → [fennec-sessionstore]
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.
Also, this patch stores scroll positions too (bug 638167)
Scroll offsets are part of the session history. Saving one gets the other for free.
Summary: Request for session store to retain the browsing history → Request for session store to retain the browsing history and scroll offsets
(Assignee)

Updated

6 years ago
Duplicate of this bug: 638167
(Assignee)

Updated

6 years ago
Duplicate of this bug: 653669
TC https://litmus.mozilla.org/show_test.cgi?id=13667 fails due to this bug.
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.
Attachment #530290 - Attachment is obsolete: true
Attachment #540807 - Flags: review?(wjohnston)
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
(Assignee)

Updated

6 years ago
Attachment #540807 - Flags: review?(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).
Attachment #540807 - Flags: review?(mbrubeck) → review-
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?
Attachment #540807 - Flags: review?(wjohnston) → review+
(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
Created attachment 540965 [details] [diff] [review]
patch 2

With changes
Attachment #540807 - Attachment is obsolete: true
Attachment #540965 - Flags: review?(mbrubeck)
Attachment #540965 - Flags: review?(mbrubeck) → review+
(Assignee)

Updated

6 years ago
Whiteboard: [fennec-sessionstore] → [fennec-sessionstore][inbound]
http://hg.mozilla.org/mozilla-central/rev/b25a0fc03c3a
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fennec-sessionstore][inbound] → [fennec-sessionstore]
Target Milestone: --- → Firefox 7
Depends on: 666577
Depends on: 668379
(Assignee)

Updated

6 years ago
tracking-fennec: 6+ → 7+
status-firefox7: --- → fixed

Comment 16

6 years ago
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
Status: RESOLVED → VERIFIED

Updated

6 years ago
Keywords: verified-aurora

Updated

6 years ago
Flags: in-litmus?(kbrosnan)
Blocks: 685127
Blocks: 639631
Flags: in-litmus?(kbrosnan)
You need to log in before you can comment on or make changes to this bug.