Jan Henning [:JanH] Assignee
	Description • 8 years ago

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.

	Jan Henning [:JanH] Assignee
	Comment 1 • 8 years ago

Didn't look closely enough, bug 1000251 is about zoom level only.

	Jan Henning [:JanH] Assignee
	Comment 2 • 7 years ago

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

	Jan Henning [:JanH] Assignee
	Comment 3 • 7 years ago

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

	Jan Henning [:JanH] Assignee
	Comment 4 • 7 years ago

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.

	Jan Henning [:JanH] Assignee
	Comment 6 • 7 years ago

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.

	Mike de Boer [:mikedeboer]
	Comment 13 • 7 years ago

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!

	Johnny Stenback (:jst)
	Comment 19 • 7 years ago
mozreview-review

Comment on attachment 8844596 [details]
Bug 1265818 - Part 1 - Convert nsILayoutHistoryState header to IDL.

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

r=jst

	Johnny Stenback (:jst)
	Comment 20 • 7 years ago
mozreview-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

	Mike de Boer [:mikedeboer]
	Comment 21 • 7 years ago
mozreview-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);
},
```

	Mike de Boer [:mikedeboer]
	Comment 22 • 7 years ago
mozreview-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!

	Sebastian Kaspari (:sebastian; :pocmo)
	Comment 23 • 7 years ago
mozreview-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

	Sebastian Kaspari (:sebastian; :pocmo)
	Comment 24 • 7 years ago
mozreview-review

Comment on attachment 8844601 [details]
Bug 1265818 - Part 6 - Test PresState restoring on Android, too.

https://reviewboard.mozilla.org/r/114188/#review121600

	Jan Henning [:JanH] Assignee
	Comment 25 • 7 years ago
mozreview-review-reply

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.

	Jan Henning [:JanH] Assignee
	Comment 32 • 7 years ago

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.

	Mike de Boer [:mikedeboer]
	Comment 33 • 7 years ago
mozreview-review

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

	Jan Henning [:JanH] Assignee
	Comment 34 • 7 years ago

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.

	Jan Henning [:JanH] Assignee
	Comment 35 • 7 years ago
mozreview-review-reply

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.

	Johnny Stenback (:jst)
	Comment 47 • 7 years ago
mozreview-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/#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.

	Jan Henning [:JanH] Assignee
	Comment 48 • 7 years ago
mozreview-review

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

	Jan Henning [:JanH] Assignee
	Comment 49 • 7 years ago

I hope that change of plans in Part 3 above is alright with you?

	Johnny Stenback (:jst)
	Comment 56 • 7 years ago
mozreview-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/#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.

	Mike de Boer [:mikedeboer]
	Comment 62 • 7 years ago

Jan, I'm absolutely OK with narrowing down the scope of this patch to tackle it properly in bug 1350567.

	Mozilla Autoland
	Comment 69 • 7 years ago

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)

	Pulsebot
	Comment 76 • 7 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 77 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/113a43981ef1
https://hg.mozilla.org/mozilla-central/rev/77f83a554db1
https://hg.mozilla.org/mozilla-central/rev/15c13c19625c
https://hg.mozilla.org/mozilla-central/rev/f90ffc464196
https://hg.mozilla.org/mozilla-central/rev/4bebc1d2c56e
https://hg.mozilla.org/mozilla-central/rev/e2240273ffc2

	Pulsebot
	Comment 78 • 7 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 79 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/4e53b8baa32e