Closed Bug 1265818 Opened 7 years ago Closed 6 years ago

Session store doesn't save nsILayoutHistoryState/nsPresState

Categories

(Firefox :: Session Restore, defect)

Product:
Firefox
Component:
Session Restore
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox48 --- affected
firefox55 --- fixed

People

(Reporter: JanH, Assigned: JanH)

References

(Depends on 3 open bugs, Blocks 3 open bugs)

Details

Attachments

(6 files)

Bug 1265818 - Part 1 - Convert nsILayoutHistoryState header to IDL.
59 bytes, text/x-review-board-request
jst
: review+
Details
Bug 1265818 - Part 2 - Add scriptable methods for getting and setting PresStates from JS.
59 bytes, text/x-review-board-request
jst
: review+
Details
Bug 1265818 - Part 3 - Store and restore the LayoutHistoryState through SessionHistoy.jsm.
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details
Bug 1265818 - Part 4 - Test that scroll positions for previous session history entries are restored on Desktop.
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details
Bug 1265818 - Part 5 - Tidy up and improve mobile scroll position test.
59 bytes, text/x-review-board-request
sebastian
: review+
Details
Bug 1265818 - Part 6 - Test PresState restoring on Android, too.
59 bytes, text/x-review-board-request
sebastian
: review+
Details 
The session store code on both desktop (https://dxr.mozilla.org/mozilla-central/rev/67ac40fb8f680ea5e03805552187ba1b5e8392a1/browser/components/sessionstore/SessionHistory.jsm#170) and mobile (https://dxr.mozilla.org/mozilla-central/rev/67ac40fb8f680ea5e03805552187ba1b5e8392a1/mobile/android/components/SessionStore.js#810) has code to serialise and deserialise the scroll position of a SHEntry, however in practice it seems that code never runs because when queried, the returned scroll position is always (0, 0) [1].

Desktop works around this by saving the scroll position of the currently loaded page, but this still means that scroll positions for all other history entries of a tab get lost when Firefox is closed.

[1] Looking at the session store's save files, I've never seen a history entry containing a scroll position, only the tabs objects on desktop. Also, with a breakpoint set on those lines, the breakpoint was never triggered during browsing, suggesting x and y as returned by getScrollPosition() were always 0.
Blocks: 1000251
Didn't look closely enough, bug 1000251 is about zoom level only.
No longer blocks: 1000251
Blocks: ss-feature
Summary: nsISHEntry.getScrollPosition() always returns (0, 0) → Scroll position is not saved in nsISHEntry
I've looked through the code again and I think I've finally somewhat figured out what is happening.

The scroll position that is saved directly within the SHEntry and that is being serialised and deserialised by the session store as of today is intended only for the case were we're navigating within the same document,
- either through anchor links
- or else through the history API (pushState). [1]

What I'm in fact after is saving and restoring previous scroll positions as held by the LayoutHistoryState and its corresponding PresState as well. For Android, also saving the pinch zoom state would be a nice bonus.

[1] From my limited understanding I've gained so far I assume this is because the LayoutHistoryState/PresState is saved per document only [2] and therefore cannot save multiple scroll positions when navigating within the same document.

[2] https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/layout/base/nsPresState.h#7
Summary: Scroll position is not saved in nsISHEntry → Session store doesn't save nsILayoutEntries/nsPresState
(In reply to Jan Henning [:JanH] from comment #2)
> For Android, also saving the pinch zoom state would be a nice bonus.

Which probably requires bug 1312605 to be fixed before anything like that can be properly implemented.
See Also: → 1312605
I might have something presentable (and hopefully not too horribly hacky) soon [1], but before going any further, I'd like to unify the session history handling between Desktop and Android again, so I won't have to work on this in two places.

[1] I've added scriptable methods to nsILayoutHistoryState, so what remains is hooking this up to SessionHistory.jsm and then hoping this actually works as expected.
Assignee: nobody → jh+bugzilla
Component: General → Document Navigation
Depends on: 1340828
Summary: Session store doesn't save nsILayoutEntries/nsPresState → Session store doesn't save nsILayoutHistoryState/nsPresState
Depends on: 1312605
See Also: 1312605 → 
Just a quick status update - I'll need a little bit of time to sort out my bug queue and give this and bug 1340828 some final polishing before review, but after adding the necessary code to SessionHistory.jsm this seems to be working quite nicely.

(In reply to Jan Henning [:JanH] from comment #4)
> (and hopefully not too horribly hacky)

My main concern was how to convert the nsILayoutHistoryState.h header into an IDL file without having to include all of the former interface definition as a raw code fragment, but after getting a little more familiar with how IDF files work in the first place, I've eventually figured it out.
Component: Document Navigation → Session Restore
Product: Core → Firefox
I've looked at the patches and saw no real red flags, but I'd like to review this once jst has r+'d the first two.

Aside from that, major kudos for fixing this on desktop AND mobile in one swoop - awesome! I'm looking forward to seeing the progress here!
Comment on attachment 8844596 [details]
Bug 1265818 - Part 1 - Convert nsILayoutHistoryState header to IDL.

https://reviewboard.mozilla.org/r/117248/#review121282

r=jst
Attachment #8844596 - Flags: review?(jstenback+bmo) → review+
Comment on attachment 8844597 [details]
Bug 1265818 - Part 2 - Add scriptable methods for getting and setting PresStates from JS.

https://reviewboard.mozilla.org/r/114180/#review121284
Attachment #8844597 - Flags: review?(jstenback+bmo) → review+
Comment on attachment 8844598 [details]
Bug 1265818 - Part 3 - Store and restore the LayoutHistoryState through SessionHistoy.jsm.

https://reviewboard.mozilla.org/r/114182/#review121510

There is absolutely no issue with the structure of the code here, but I'd like to see a number of small improvement before I can r+.
Thanks!

::: toolkit/modules/sessionstore/SessionHistory.jsm:200
(Diff revision 2)
>      if (shEntry.scrollRestorationIsManual) {
>        entry.scrollRestorationIsManual = true;
>      } else {
>        let x = {}, y = {};
>        shEntry.getScrollPosition(x, y);
> -      if (x.value != 0 || y.value != 0)
> +      if (x.value != 0 || y.value != 0) {

While you're here, can you change this to use strict operators (`!==`)? Thanks!

::: toolkit/modules/sessionstore/SessionHistory.jsm:211
(Diff revision 2)
> +        let keys = layoutHistoryState.getKeys();
> +        let presState = [];
> +        for (let i = 0; i < keys.length; i++) {
> +          presState.push(this._serializePresState(layoutHistoryState, keys[i]));
> +        }
> +        entry.presState = presState;

Why not make this:

```js
// It's very well possible you don't need the array literal icw the spread operator, but that's easy to quickly check.
entry.presState = [...layoutHistoryState.getKeys()].map(key => this._serializePresState(layoutHistoryState, key));
```

::: toolkit/modules/sessionstore/SessionHistory.jsm:291
(Diff revision 2)
> +   *        nsILayoutHistoryState instance
> +   * @param key
> +   *        The key of the presState to be retrieved.
> +   * @return object
> +   */
> +  _serializePresState(layoutHistoryState, key) {

Since you're not returning a string here, this is not strictly serializing a state. Can you rename this method to `_getSerializablePresState()`?

::: toolkit/modules/sessionstore/SessionHistory.jsm:292
(Diff revision 2)
> +   * @param key
> +   *        The key of the presState to be retrieved.
> +   * @return object
> +   */
> +  _serializePresState(layoutHistoryState, key) {
> +    let presState = {key};

I'd like a better, more descriptive name for this argument.

::: toolkit/modules/sessionstore/SessionHistory.jsm:296
(Diff revision 2)
> +  _serializePresState(layoutHistoryState, key) {
> +    let presState = {key};
> +    let x = {}, y = {}, scrollOriginDowngrade = {}, res = {}, scaleToRes = {};
> +
> +    layoutHistoryState.getPresState(key, x, y, scrollOriginDowngrade, res, scaleToRes);
> +    if (x.value != 0 || y.value != 0) {

When you're comparing to `0` specifically in JS, please use the strict operator (`!==`)

::: toolkit/modules/sessionstore/SessionHistory.jsm:299
(Diff revision 2)
> +
> +    layoutHistoryState.getPresState(key, x, y, scrollOriginDowngrade, res, scaleToRes);
> +    if (x.value != 0 || y.value != 0) {
> +      presState.scroll = x.value + "," + y.value;
> +    }
> +    if (!scrollOriginDowngrade.value) {

So we're also not saving a value of `0`?

::: toolkit/modules/sessionstore/SessionHistory.jsm:305
(Diff revision 2)
> +      presState.scrollOriginDowngrade = scrollOriginDowngrade.value;
> +    }
> +    if (res.value != 1.0) {
> +      presState.res = res.value;
> +    }
> +    if (scaleToRes.value) {

So we're also not saving a value of `0`?

::: toolkit/modules/sessionstore/SessionHistory.jsm:436
(Diff revision 2)
>      if (entry.scrollRestorationIsManual) {
>        shEntry.scrollRestorationIsManual = true;
> -    } else if (entry.scroll) {
> -      var scrollPos = (entry.scroll || "0,0").split(",");
> -      scrollPos = [parseInt(scrollPos[0]) || 0, parseInt(scrollPos[1]) || 0];
> +    } else {
> +      if (entry.scroll) {
> +        let scrollPos = this._deserializeScrollPosition(entry.scroll);
> -      shEntry.setScrollPosition(scrollPos[0], scrollPos[1]);
> +        shEntry.setScrollPosition(scrollPos[0], scrollPos[1]);

While you're here, can you change this to:
```js
shEntry.setScrollPosition(...this._deserializeScrollPosition(entry.scroll));
```
?

::: toolkit/modules/sessionstore/SessionHistory.jsm:441
(Diff revision 2)
> -      shEntry.setScrollPosition(scrollPos[0], scrollPos[1]);
> +        shEntry.setScrollPosition(scrollPos[0], scrollPos[1]);
> -    }
> +      }
>  
> +      if (entry.presState) {
> +        shEntry.initLayoutHistoryState();
> +        let layoutHistoryState = shEntry.layoutHistoryState;

Perhaps it'd make sense to have `initLayoutHistoryState()` to return `layoutHistoryState`, instead of just `void`?

::: toolkit/modules/sessionstore/SessionHistory.jsm:443
(Diff revision 2)
>  
> +      if (entry.presState) {
> +        shEntry.initLayoutHistoryState();
> +        let layoutHistoryState = shEntry.layoutHistoryState;
> +
> +        for (let i = 0; i < entry.presState.length; i++) {

Please use for...of: `for (let presState of entry.presState) {}`

::: toolkit/modules/sessionstore/SessionHistory.jsm:529
(Diff revision 2)
> +   */
> +  _deserializePresState(layoutHistoryState, presState) {
> +    let key = presState.key;
> +    let scrollPos = this._deserializeScrollPosition(presState.scroll);
> +    let scrollOriginDowngrade =
> +      presState.scrollOriginDowngrade == undefined ? true : presState.scrollOriginDowngrade;

Perhaps doing
```js
typeof presState.scrollOriginDowngrade == "boolean" ?
  presState.scrollOriginDowngrade : true;
```
would be clearer?

::: toolkit/modules/sessionstore/SessionHistory.jsm:534
(Diff revision 2)
> +      presState.scrollOriginDowngrade == undefined ? true : presState.scrollOriginDowngrade;
> +    let res = presState.res || 1.0;
> +    let scaleToRes = presState.scaleToRes || false;
> +
> +    layoutHistoryState.addNewPresState(key, scrollPos[0], scrollPos[1],
> +                                       scrollOriginDowngrade, res, scaleToRes);

Taking my previous comment into account, I think you can shorten this to:
```js
layoutHistoryState.addNewPresState(presState.key,
  ...this._deserializeScrollPosition(presState.scroll),
  scrollOriginDowngrade, res || 1, scaleToRes || false);
```
What do you think?

::: toolkit/modules/sessionstore/SessionHistory.jsm:547
(Diff revision 2)
> +   *        Object containing serialized scroll position data.
> +   * @return An array containing the scroll position's x and y coordinates.
> +   */
> +  _deserializeScrollPosition(scroll) {
> +    let scrollPos = (scroll || "0,0").split(",");
> +    return [parseInt(scrollPos[0]) || 0, parseInt(scrollPos[1]) || 0];

When using `parseInt` in JS, don't forget to add the 'base' argument: `parseInt(scrollPos[0], 10);`

You can also shorten this to:
```js
_deserializeScrollPosition(scroll = "0,0") {
  return scroll.split(",").map(pos => parseInt(pos, 10) || 0);
},
```
Attachment #8844598 - Flags: review?(mdeboer) → review-
Comment on attachment 8844599 [details]
Bug 1265818 - Part 4 - Test that scroll positions for previous session history entries are restored on Desktop.

https://reviewboard.mozilla.org/r/114184/#review121514

Nice!
Attachment #8844599 - Flags: review?(mdeboer) → review+
Comment on attachment 8844600 [details]
Bug 1265818 - Part 5 - Tidy up and improve mobile scroll position test.

https://reviewboard.mozilla.org/r/114186/#review121598
Attachment #8844600 - Flags: review?(s.kaspari) → review+
Comment on attachment 8844601 [details]
Bug 1265818 - Part 6 - Test PresState restoring on Android, too.

https://reviewboard.mozilla.org/r/114188/#review121600
Attachment #8844601 - Flags: review?(s.kaspari) → review+
Comment on attachment 8844598 [details]
Bug 1265818 - Part 3 - Store and restore the LayoutHistoryState through SessionHistoy.jsm.

https://reviewboard.mozilla.org/r/114182/#review121510

> Why not make this:
> 
> ```js
> // It's very well possible you don't need the array literal icw the spread operator, but that's easy to quickly check.
> entry.presState = [...layoutHistoryState.getKeys()].map(key => this._serializePresState(layoutHistoryState, key));
> ```

`getKeys()` is returning an array, so I can indeed directly call `map()`.

> I'd like a better, more descriptive name for this argument.

I suppose I could call it `stateKey`, since that's its [full name on the C++-side](https://dxr.mozilla.org/mozilla-central/rev/23a4b7430dd7e83a2809bf3dc41471f154301eda/dom/base/nsContentUtils.cpp#2619). It doesn't sound that much more descriptive, though, so I'm not really sure...

> So we're also not saving a value of `0`?

Actually in this case we are because of the added `!`, but that aside, it's a boolean value that is true by default, so I'm only interested in it if it turns out to be false. Unless you've got a general objection against not storing default values, I guess I could make that assumption more explicit, though...

> So we're also not saving a value of `0`?

See above, I only want to save this if it's `true`.

> Perhaps it'd make sense to have `initLayoutHistoryState()` to return `layoutHistoryState`, instead of just `void`?

Good idea.
Comment on attachment 8844597 [details]
Bug 1265818 - Part 2 - Add scriptable methods for getting and setting PresStates from JS.

InitLayoutHistoryState has changed slightly to return a reference to the LayoutHistoryState in one go.
Attachment #8844597 - Flags: review+ → review?(jstenback+bmo)
Comment on attachment 8844598 [details]
Bug 1265818 - Part 3 - Store and restore the LayoutHistoryState through SessionHistoy.jsm.

https://reviewboard.mozilla.org/r/114182/#review125408

Great! One thing I realized is that we have a 'browser/components/sessionstore/ContentRestore.jsm' module that uses 'toolkit/modules/sessionstore/ScrollPosition.jsm' and is used in both Firefox Desktop and Android. How much of this will be overlapping functionality when this lands?

::: toolkit/modules/sessionstore/SessionHistory.jsm:213
(Diff revisions 2 - 3)
>        let layoutHistoryState = shEntry.layoutHistoryState;
>        if (layoutHistoryState && layoutHistoryState.hasStates) {
> -        let keys = layoutHistoryState.getKeys();
> -        let presState = [];
> -        for (let i = 0; i < keys.length; i++) {
> -          presState.push(this._serializePresState(layoutHistoryState, keys[i]));
> +        let presStates = layoutHistoryState.getKeys().map(key =>
> +          this._getSerializablePresState(layoutHistoryState, key));
> +        // Only keep presState entries that contain more than the key itself.
> +        presStates = presStates.filter(presState => Object.keys(presState).length > 1);

nit: `Object.getOwnPropertyNames(presState)` and you can chain this with the previous statement as well (whilst keeping the comment above).
Attachment #8844598 - Flags: review?(mdeboer) → review+
Flags: needinfo?(jh+bugzilla)
Comment on attachment 8844597 [details]
Bug 1265818 - Part 2 - Add scriptable methods for getting and setting PresStates from JS.

Need to fix my logic slightly.
Attachment #8844597 - Flags: review?(jstenback+bmo)
Comment on attachment 8844598 [details]
Bug 1265818 - Part 3 - Store and restore the LayoutHistoryState through SessionHistoy.jsm.

https://reviewboard.mozilla.org/r/114182/#review125408

No overlap, they're complementing each other. A history entry's LayoutHistoryState is only updated when we navigate away from it, so the session store still needs to keep track of the position (and pinch zoom on Android) for the *current* page.
Attachment #8844597 - Flags: review?(jstenback+bmo)
Flags: needinfo?(jh+bugzilla)
Comment on attachment 8844597 [details]
Bug 1265818 - Part 2 - Add scriptable methods for getting and setting PresStates from JS.

https://reviewboard.mozilla.org/r/114180/#review126028

I believe this does the right thing, but please fix the few issues I mentioned. Thanks!

::: docshell/shistory/nsSHEntry.cpp:306
(Diff revision 5)
> +    historyState = NS_NewLayoutHistoryState();
> +    rv = SetLayoutHistoryState(historyState);
> +  } else {
> +    rv = NS_OK;
> +  }
> +

Please move the declaration of rv inside the if (!mShared...) block and use NS_ENSURE_SUCCESS(rv, rv); after the call to SetLayoutHistoryState(). That way you can simply return GetLayoutHistoryState() at the end of this function, fewer lines, easier to read, etc.

::: layout/base/nsLayoutHistoryState.cpp:72
(Diff revision 5)
> +    }
> +    return NS_OK;
> +  }
> +
> +  return NS_ERROR_FAILURE;
> +}

Please bail early in this function if !HasStates() and move the bulk this code out of the if statement. IOW, reverse the error handling here.

::: layout/base/nsLayoutHistoryState.cpp:93
(Diff revision 5)
> +
> +    return NS_OK;
> +  }
> +
> +  return NS_ERROR_FAILURE;
> +}

Same here please.
Attachment #8844597 - Flags: review?(jstenback+bmo) → review-
Depends on: 1350567
Comment on attachment 8844598 [details]
Bug 1265818 - Part 3 - Store and restore the LayoutHistoryState through SessionHistoy.jsm.

https://reviewboard.mozilla.org/r/114182/#review126090

::: browser/components/sessionstore/content/content-sessionStore.js:336
(Diff revision 5)
>    },
>  
>    OnHistoryNewEntry(newURI, oldIndex) {
> -    this.collectFrom(oldIndex);
> +    // The PresState is added only when navigating away from the current
> +    // page, so we now need to collect the previous entry again to catch it.
> +    this.collectFrom(oldIndex - 1);

Actually this seeming innocuous line seems to cause quite some trouble, especially in relation to GroupedSHistory, so I've split this off into a [separate bug](https://bugzilla.mozilla.org/show_bug.cgi?id=1350567).
I hope that change of plans in Part 3 above is alright with you?
Flags: needinfo?(mdeboer)
Comment on attachment 8844597 [details]
Bug 1265818 - Part 2 - Add scriptable methods for getting and setting PresStates from JS.

https://reviewboard.mozilla.org/r/114180/#review126094

Looks great, thanks!

::: layout/base/nsLayoutHistoryState.cpp:85
(Diff revision 6)
> +{
> +  nsPresState* state = GetState(nsCString(aKey));
> +
> +  if (!state) {
> +      return NS_ERROR_FAILURE;
> +  }

Whitespace nit, reduce indentation to 2 spaces to be consistent with the rest of the code.
Attachment #8844597 - Flags: review?(jstenback+bmo) → review+
Jan, I'm absolutely OK with narrowing down the scope of this patch to tackle it properly in bug 1350567.
Flags: needinfo?(mdeboer)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 628026bb2090 -d ce66e1f2a032: rebasing 385396:628026bb2090 "Bug 1265818 - Part 1 - Convert nsILayoutHistoryState header to IDL. r=jst"
rebasing 385397:7a39ae8459e2 "Bug 1265818 - Part 2 - Add scriptable methods for getting and setting PresStates from JS. r=jst"
rebasing 385398:41a2f22ce033 "Bug 1265818 - Part 3 - Store and restore the LayoutHistoryState through SessionHistoy.jsm. r=mikedeboer"
merging browser/components/sessionstore/content/content-sessionStore.js
rebasing 385399:a5f36269683f "Bug 1265818 - Part 4 - Test that scroll positions for previous session history entries are restored on Desktop. r=mikedeboer"
merging browser/components/sessionstore/test/browser.ini
rebasing 385400:ea7b2a45b77e "Bug 1265818 - Part 5 - Tidy up and improve mobile scroll position test. r=sebastian"
merging mobile/android/tests/browser/chrome/chrome.ini
warning: conflicts while merging mobile/android/tests/browser/chrome/chrome.ini! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/113a43981ef1
Part 1 - Convert nsILayoutHistoryState header to IDL. r=jst
https://hg.mozilla.org/integration/autoland/rev/77f83a554db1
Part 2 - Add scriptable methods for getting and setting PresStates from JS. r=jst
https://hg.mozilla.org/integration/autoland/rev/15c13c19625c
Part 3 - Store and restore the LayoutHistoryState through SessionHistoy.jsm. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/f90ffc464196
Part 4 - Test that scroll positions for previous session history entries are restored on Desktop. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/4bebc1d2c56e
Part 5 - Tidy up and improve mobile scroll position test. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/e2240273ffc2
Part 6 - Test PresState restoring on Android, too. r=sebastian
Depends on: 1351461
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e53b8baa32e
followup: touch CLOBBER after converting nsILayoutHistoryState.h to .idl. rs=mccr8
See Also: → 1400578
Depends on: 1462794
You need to log in before you can comment on or make changes to this bug.