810981 - Restore scroll position when restoring tabs from previous session

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Taras reports: if you're reading a long article in Fennec, and then switch apps and do something so fennec gets oom-killed, switching back into fennec takes you back to the top of the page after the page is session-restored. We should save the scroll position in the session restore data and use it to scroll back to where the user was.

Comment 1

[(dormant account)]

I was using reader mode. I think we should serialize the reader mode view when app gets backgrounded, so it doesn't have to get reloaded from network.

Comment 2

[Eugen Sawin [:esawin]]

There is a more general issue with scroll position not being restored from session storage.

STR 1
1. Enable tabs restoring.
2. Open a page and scroll on it.
3. Close Fennec using the Android task manager.
4. Launch Fennec again.

STR 2
1. Open a page and scroll on it.
2. Change into guest mode (Tools/New Guest Session).
3. Exit guest session.

In both cases the following holds.

Expected
Tabs are restored with pages scrolled to previous positions.

Actual
Tabs are restored with scroll position reset to top left.

Do we want to track this in this bug and change the summary or spin off a new bug?

Comment 3

[Kartikaya Gupta (email:[e-mail])]

I'm fine repurposing this bug to track the general issue.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

* This could make us a better on-the-go article reader
* Due to the different interaction model, desktop does not have this problem.
* We should match user expectations – Android arbitrarily kills things and this (presumably) does not match the user's model of how Firefox is supposed to work.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Eugen, I might dig into this at some point. Do you have any suggestions on where to get started?

Comment 7

[:Margaret Leibovic]

I would look at the session restore code: 
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/SessionStore.js?force=1#759

Comment 8

[Eugen Sawin [:esawin]]

As Margaret said, SessionStore.js is a good place to start. Although, I'm not particularly helpful on this as a mentor as I've only worked on zoom level sessions which do not use SessionStore.js.

Comment 9

[Jan Henning [:JanH]]

I hope this doesn't lead too far down a rabbit hole into the C++ side of things, but in any case I'll try looking at it.

Comment 10

[Jan Henning [:JanH]]

It seems that both we and Desktop have code to both serialise and deserialise the scroll position of a nsISHEntry, however for unknown reasons, nsISHEntry.getScrollPosition() always returns (0, 0), so scroll positions for history entries are never saved.
At a first glance, desktop works around this by saving the scroll position of the currently loaded page as a property of the tab object, similar to how we do form data saving.
I'll probably look at doing something similar for us, too.

Comment 11

[:Margaret Leibovic]

(In reply to Jan Henning [:JanH] from comment #10)
> It seems that both we and Desktop have code to both serialise and
> deserialise the scroll position of a nsISHEntry, however for unknown
> reasons, nsISHEntry.getScrollPosition() always returns (0, 0), so scroll
> positions for history entries are never saved.

Maybe kats or someone else from the platform team can help explain what's going on here.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Off the top of my head I'm not sure. From a quick look around the code I *believe* the scroll position is saved in the mOSHE->SetScrollPosition calls in docshell/base/nsDocShell.cpp, but I see only two calls to that function, one for pushState/replaceState and one for "short-circuit loads" which appear to be in-document only. So I don't know that the scroll position is saved in the session history across page navigation.

There's also code in nsGfxScrollFrame::SaveState/RestoreState which saves and restores the scroll position when the frame tree is rebuilt, but that also shouldn't happen across page navigations unless it's in the bfcache. Again, not 100% sure on this.

Based on comment 10 I think doing what desktop does and saving it in the tab object sounds reasonable. Feel free to needinfo me again if you want me to look at this in greater detail, but it probably won't happen this week.

Comment 13

[Jan Henning [:JanH]]

Just a quick status update, I have implemented some basic scroll position and zoom level capturing and restoring code, which seems to be working quite well.
I'm still looking on ironing out a few rough edges - scroll position restoring doesn't seem to be working for pages with framesets yet, and part of the zoom level restoring could benefit from some additional support by the PresShell and/or the MobileViewportManager, because the session store doesn't know anything about how to adjust the zoom level/resolution if the screen orientation has changed between data capturing and restoring.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

What is the expected behaviour if the screen orientation changed between capture/restore? Should it behave the same as if you restored without rotating, and then rotated the phone? Or do you want to restore to the resolution that was captured, even if it doesn't fit quite right?

Comment 15

[Jan Henning [:JanH]]

Attachment: portrait desktop page restored in landscape mode.png

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #14)
> What is the expected behaviour if the screen orientation changed between
> capture/restore? Should it behave the same as if you restored without
> rotating, and then rotated the phone?

Exactly, that would be my desired behaviour.

> Or do you want to restore to the
> resolution that was captured, even if it doesn't fit quite right?

This is what is currently happening with my code. As far as I can see, this is not a problem for pages where the viewport scales to the device width (i.e. probably most/all? mobile pages, if I'm understanding that right), because there the reported resolution [1] doesn't seem to change on rotation, but for desktop pages for example, the resolution is adjusted when the screen width changes upon rotation.

With my current approach, a desktop page captured in portrait mode and restored in landscape mode might e.g. end up looking like in the attached screenshot.

I'm not terribly familiar with the native code, so there might be a better approach to solve this, but this is what I was thinking about so far:
- As a minimal solution, an expanded version of setResolutionAndScaleTo(), which would also take the previous screen dimensions as a parameter. If the previous screen dimension matches the current one, it would be equivalent to a simple call to setResolutionAndScaleTo(aResolution), if not, it would recalculate the resolution based on the old and new screen dimension before scaling the page.
- Ideally, some way of getting the session store's data about the old screen size and zoom level (and possibly the scroll position as well?) into the PresShell/MobileDeviceManager before the first paint/load event, so that SetInitialViewport() could just directly scale the page to the correct level. Otherwise, the MobileDeviceManager would end up scaling the page to its default initial level on first-paint/load, only to be immediately overridden by the session store's call to setResolutionAndScaleTo during pageshow.

[1] I'm currently using nsIDOMWindowUtils.getResolution() to capture the zoom level.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

(In reply to Jan Henning [:JanH] from comment #15)
> - As a minimal solution, an expanded version of setResolutionAndScaleTo(),
> which would also take the previous screen dimensions as a parameter. If the
> previous screen dimension matches the current one, it would be equivalent to
> a simple call to setResolutionAndScaleTo(aResolution), if not, it would
> recalculate the resolution based on the old and new screen dimension before
> scaling the page.

This code to "recalculate the resolution" is pretty straightforward, it is at [1]. If you have the old and new screen dimensions in your restore code, you could probably just adjust the captured resolution by the same ratio before you restore it via SetResolutionAndScaleTo. That might be a little cleaner than trying to plumb it in through a new expanded version of setResolutionAndScaleTo, although I don't have all this code paged into my head right now so I might be missing something.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/MobileViewportManager.cpp?rev=8fa535b41cad#211

> - Ideally, some way of getting the session store's data about the old screen
> size and zoom level (and possibly the scroll position as well?) into the
> PresShell/MobileDeviceManager before the first paint/load event, so that
> SetInitialViewport() could just directly scale the page to the correct
> level. Otherwise, the MobileDeviceManager would end up scaling the page to
> its default initial level on first-paint/load, only to be immediately
> overridden by the session store's call to setResolutionAndScaleTo during
> pageshow.

We could add something for this, yeah. We could set a desired initial resolution in the MobileViewportManager and the mIsFirstPaint branch of the UpdateResolution function could use that resolution instead of the default zoom that it picks up from the viewport info. It would probably still need to clamp that provided resolution by the min/max zooms in the viewport info.

Comment 17

[Jan Henning [:JanH]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #16)
> This code to "recalculate the resolution" is pretty straightforward, it is
> at [1]. If you have the old and new screen dimensions in your restore code,
> you could probably just adjust the captured resolution by the same ratio
> before you restore it via SetResolutionAndScaleTo. That might be a little
> cleaner than trying to plumb it in through a new expanded version of
> setResolutionAndScaleTo, although I don't have all this code paged into my
> head right now so I might be missing something.

I was trying to avoid duplicating that code, but you're right, feeding a fake previous display size and zoom level to the MobileViewportManager doesn't seem super-trivial, either. Also, the session store wouldn't have to calculate a zoom level from scratch, but just adjust a pre-existing one, so the calculation should indeed be quite simple. I hope that looking for autoSize in getViewportInfo (https://dxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIDOMWindowUtils.idl#118) is enough to catch those cases where we *don't* want to adjust the zoom level after a display orientation change. I guess I'll just give it a try and see whether I can get it working satisfactorily.

> > - Ideally, some way of getting the session store's data about the old screen
> > size and zoom level (and possibly the scroll position as well?) into the
> > PresShell/MobileDeviceManager before the first paint/load event, so that
> > SetInitialViewport() could just directly scale the page to the correct
> > level. Otherwise, the MobileDeviceManager would end up scaling the page to
> > its default initial level on first-paint/load, only to be immediately
> > overridden by the session store's call to setResolutionAndScaleTo during
> > pageshow.
> 
> We could add something for this, yeah. We could set a desired initial
> resolution in the MobileViewportManager and the mIsFirstPaint branch of the
> UpdateResolution function could use that resolution instead of the default
> zoom that it picks up from the viewport info.

That would be nice.

> It would probably still need
> to clamp that provided resolution by the min/max zooms in the viewport info.

Does this account for the user option to override any meta viewport zoom restrictions? Besides, the session store would capture the resolution as it actually existed, so as long as that value is suitably recalculated to account for any display size/orientation changes, there shouldn't be any harm in restoring it "as is". But we can just try this out anyway when implementing it...

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Yes, it should account for the user override. That gets applied at [1] so the values in the viewport info will already be wider by the time MobileViewportManager gets it.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp?rev=85ce8cb0639a#8108

Comment 19

[Jan Henning [:JanH]]

Getting scroll position restoring to work on pages with frames was pretty straightforward - just move the top-level event filtering code I've removed in bug 1269189 to the load/pageshow events, so I'm restoring the scroll position only once all subframes have loaded and voilà, it's working.

There's still something flaky about restoring the zoom level for pages with framesets, though:
- If I'm disabling scroll position restoring, calling setResolutionAndScaleTo() when receiving the top level document's pageshow event doesn't seem to have any effect
- If I'm also restoring the scroll positions after calling setResolutionAndScaleTo(), there's something weird going on. The resolution change is still being ignored, but for unfathomable reasons I end up with a viewport that's covering only part of my display size.

Comment 20

[Jan Henning [:JanH]]

Attachment: zoomed page with frames before restoring.png

http://www.railfaneurope.net/pix_frameset.html zoomed in

Comment 21

[Jan Henning [:JanH]]

Attachment: zoomed page with frames after restoring.png

After closing the page and restoring it again - my session store code is calling setResolutionAndScaleTo and then ScrollPosition.jsm's restoreTree() when receiving the top-level pageshow event from the browser.

Comment 22

[Kartikaya Gupta (email:[e-mail])]

Could you post the patches you have so far? I'm having trouble following your description of the changes and it would probably be easier to just look at the code. Even just a github link or try push is fine.

Comment 23

[Jan Henning [:JanH]]

WIP version:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7626ffd70f52

Comment 24

[Kartikaya Gupta (email:[e-mail])]

I applied your patches and ran them. Your restoreZoom function is doing the right thing, but the problem is that the C++ APZ code is maintaining the "visual zoom level" rather than updating the visual zoom from what you specify. I think if we fix bug 1270019 that will take care of this, because if the zoom is set before the APZ gets the first-paint notification, then it will use whatever zoom is provided to it.

Comment 25

[Jan Henning [:JanH]]

Attachment: MozReview Request: Bug 810981 - Part 1 - Record current scroll position in mobile session store. r=margaret

This copies the approach we've taken for form data saving and applies it to recording the current scroll position of the page, too.

This means that after receiving a scroll event, we capture the scroll position for the top level document and all direct child frames and include it in the session store data. Because compared to the form data input events the scroll event can fire at a relatively high rate, we throttle the scroll position capturing using timeouts to run at most twice per second.

Review commit: https://reviewboard.mozilla.org/r/53736/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53736/

Comment 26

[Jan Henning [:JanH]]

Attachment: MozReview Request: Bug 810981 - Part 2 - Save the current zoom level when scrolling. r=kats r=margaret

Since we're now recording the scroll position in the session data, it makes sense to store the zoom level as well.

To do this, we make use of the new facility introduced in bug 1270019, which allows us to provide a desired initial resolution to the MobileViewportManager. For this to work, we need to send our desired zoom level before the MVM calculates the initial page resolution, i.e. before first paint/page load. Therefore, we now have browser.js notify us of location change events, so we can set the zoom level to restore early enough.

This also means that we can no longer restore the scroll position on load, because the MobileViewportManager applies the resolution we provide it via utils.setRestoreResolution() only after the first paint or page load ( whichever happens earlier). In the latter case, our JS load event handler will run shortly before the MVM has applied the desired zoom level in its own event handling code, which means that any scroll attempt which depends on the page already being zoomed in will fail to apply.
Therefore, the scroll position restoring needs to be moved to a later point in time, i.e. in this case the "pageshow" event.

Review commit: https://reviewboard.mozilla.org/r/53738/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53738/

Comment 27

[Jan Henning [:JanH]]

Attachment: MozReview Request: Bug 810981 - Part 3 - Recalculate zoom level before restoring to a different screen orientation. r=kats

On pages that aren't "width=device-width" or similar, Gecko adjusts the resolution when the display dimensions change, e.g. because the device has been rotated. The session store needs to do something similar when restoring a page if the device orientation has changed since the moment the tab state was captured.

Therefore, we now include the width of the browser window in the saved zoom data and use it to scale the zoom level as necessary when restoring a tab.

Review commit: https://reviewboard.mozilla.org/r/53740/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53740/

Comment 28

[Jan Henning [:JanH]]

Attachment: MozReview Request: Bug 810981 - Part 4 - Test session store scroll position and zoom level handling. r=margaret

Review commit: https://reviewboard.mozilla.org/r/53742/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53742/

Comment 29

[Jan Henning [:JanH]]

Comment on attachment 8754136 [details]
MozReview Request: Bug 810981 - Part 1 - Record current scroll position in mobile session store. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53736/diff/1-2/

Comment 30

[Jan Henning [:JanH]]

Comment on attachment 8754137 [details]
MozReview Request: Bug 810981 - Part 2 - Save the current zoom level when scrolling. r=kats r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53738/diff/1-2/

Comment 31

[Jan Henning [:JanH]]

Comment on attachment 8754138 [details]
MozReview Request: Bug 810981 - Part 3 - Recalculate zoom level before restoring to a different screen orientation. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53740/diff/1-2/

Comment 32

[Jan Henning [:JanH]]

Comment on attachment 8754139 [details]
MozReview Request: Bug 810981 - Part 4 - Test session store scroll position and zoom level handling. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53742/diff/1-2/

Comment 33

[:Margaret Leibovic]

Comment on attachment 8754136 [details]
MozReview Request: Bug 810981 - Part 1 - Record current scroll position in mobile session store. r=margaret

https://reviewboard.mozilla.org/r/53736/#review52282

Nice.

::: mobile/android/components/SessionStore.js:320
(Diff revision 2)
> +        if (!this._scrollSavePending) {
> +          this._scrollSavePending =
> +            window.setTimeout((function(window, browser) {
> +              this._scrollSavePending = null;
> +              this.onTabScroll(window, browser);
> +            }).bind(this), 500, window, browser);

If you want to avoid the bind call and passing extra paramaeters, you can use an arrow function here:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions

Comment 34

[:Margaret Leibovic]

Comment on attachment 8754137 [details]
MozReview Request: Bug 810981 - Part 2 - Save the current zoom level when scrolling. r=kats r=margaret

https://reviewboard.mozilla.org/r/53738/#review52292

I think you should get kats to review this change, since I'm not as familiar with the zoom logic details.

::: mobile/android/components/SessionStore.js:194
(Diff revision 2)
>          }
>          break;
>        }
> +      case "Session:RestoreZoom": {
> +        let window = Services.wm.getMostRecentWindow("navigator:browser");
> +        let browser = window.BrowserApp.getTabForId(aData).browser;

Can we send the browser as the notification data, instead of just the tab id? Then we wouldn't need to get the window here.

Comment 35

[:Margaret Leibovic]

Comment on attachment 8754138 [details]
MozReview Request: Bug 810981 - Part 3 - Recalculate zoom level before restoring to a different screen orientation. r=kats

I also think kats would be a better reviewer for this.

Comment 36

[:Margaret Leibovic]

Comment on attachment 8754139 [details]
MozReview Request: Bug 810981 - Part 4 - Test session store scroll position and zoom level handling. r=margaret

https://reviewboard.mozilla.org/r/53742/#review52298

Nice tests!

Comment 37

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8754137 [details]
MozReview Request: Bug 810981 - Part 2 - Save the current zoom level when scrolling. r=kats r=margaret

https://reviewboard.mozilla.org/r/53738/#review52316

The zoom-related bits look fine to me, although I don't trust myself to spot any flaws in the overall save/restore architecture.

Comment 38

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8754138 [details]
MozReview Request: Bug 810981 - Part 3 - Recalculate zoom level before restoring to a different screen orientation. r=kats

https://reviewboard.mozilla.org/r/53740/#review52318

::: mobile/android/components/SessionStore.js:1339
(Diff revision 2)
>    * Restores the zoom level of the window. This needs to be called before
>    * first paint/load (whichever comes first) to take any effect.
>    */
>    _restoreZoom: function ss_restoreZoom(aScrollData, aBrowser) {
>      if (aScrollData && aScrollData.zoom) {
> +      aScrollData.zoom.resolution = this._recalculateZoom(aScrollData.zoom);

I feel it would be better to use a local variable rather than clobbering the existing value in-place, just in case this function is called multiple times on the same object.

Comment 39

[Jan Henning [:JanH]]

Comment on attachment 8754136 [details]
MozReview Request: Bug 810981 - Part 1 - Record current scroll position in mobile session store. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53736/diff/2-3/

Comment 40

[Jan Henning [:JanH]]

Comment on attachment 8754137 [details]
MozReview Request: Bug 810981 - Part 2 - Save the current zoom level when scrolling. r=kats r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53738/diff/2-3/

Comment 41

[Jan Henning [:JanH]]

Comment on attachment 8754138 [details]
MozReview Request: Bug 810981 - Part 3 - Recalculate zoom level before restoring to a different screen orientation. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53740/diff/2-3/

Comment 42

[Jan Henning [:JanH]]

Comment on attachment 8754139 [details]
MozReview Request: Bug 810981 - Part 4 - Test session store scroll position and zoom level handling. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53742/diff/2-3/

Comment 43

[Jan Henning [:JanH]]

https://reviewboard.mozilla.org/r/53738/#review52292

> Can we send the browser as the notification data, instead of just the tab id? Then we wouldn't need to get the window here.

Good point, the browser can be sent via the `aSubject`.

Comment 44

[Jan Henning [:JanH]]

https://reviewboard.mozilla.org/r/53736/#review52282

> If you want to avoid the bind call and passing extra paramaeters, you can use an arrow function here:
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions

Seems the MDN documentation on setTimeout could do with a bit of updating... thanks for pointing this out.

Comment 45

[Jan Henning [:JanH]]

https://reviewboard.mozilla.org/r/53740/#review52318

> I feel it would be better to use a local variable rather than clobbering the existing value in-place, just in case this function is called multiple times on the same object.

Good point. That function should only ever be called once, but if a session save is triggered before `onTabScroll()` had a chance to run again, the stored zoom level and display width would be out of sync with each other.

Comment 46

[:Margaret Leibovic]

Comment on attachment 8754137 [details]
MozReview Request: Bug 810981 - Part 2 - Save the current zoom level when scrolling. r=kats r=margaret

https://reviewboard.mozilla.org/r/53738/#review52906

I'm having a tough time following this session restore logic myself. This seems to fit with our existing patterns, but I want to make sure we're not adding more complexity for the sake of going along with what's already in place.

::: mobile/android/chrome/content/browser.js:4471
(Diff revision 3)
>      if (!sameDocument) {
>        // XXX This code assumes that this is the earliest hook we have at which
>        // browser.contentDocument is changed to the new document we're loading
>        this.contentDocumentIsDisplayed = false;
>        this.hasTouchListener = false;
> +      Services.obs.notifyObservers(this.browser, "Session:RestoreZoom", null);

Why do we need this notification, separately from the "Session:Restore" notification? Is it because we always restore the zoom on every location change, not just during session restore?

If I follow the logic below correctly, it seems like we only do the actual restore during session restore, though.

::: mobile/android/components/SessionStore.js:195
(Diff revision 3)
>          break;
>        }
> +      case "Session:RestoreZoom": {
> +        let browser = aSubject;
> +        if (browser.__SS_restoreDataOnLocationChange) {
> +          delete browser.__SS_restoreDataOnLocationChange;

It's confusing that this flag mentions location change, but the "Session:RestoreZoom" notification is not obviously coupled to location change.

Could you add some comments to clarify what's going on here?

::: mobile/android/components/SessionStore.js:1258
(Diff revision 3)
>  
>      // Restoring text data and scroll position requires waiting for the content
>      // to load. So we set a flag and delay this until the appropriate event.
> +    aBrowser.__SS_restoreDataOnLocationChange = true;
>      aBrowser.__SS_restoreDataOnLoad = true;
> +    aBrowser.__SS_restoreDataOnPageshow = true;

It feels a bit fragile to continue adding more of these flags. You should also update the comment above to explain why these are necessary and what their intended purpose is.

Comment 47

[Jan Henning [:JanH]]

https://reviewboard.mozilla.org/r/53738/#review52906

> Why do we need this notification, separately from the "Session:Restore" notification? Is it because we always restore the zoom on every location change, not just during session restore?
> 
> If I follow the logic below correctly, it seems like we only do the actual restore during session restore, though.

`Session:Restore` is only sent during app startup if we are automatically restoring last session's tabs (the tabs restore setting set to "Always restore").

> If I follow the logic below correctly, it seems like we only do the actual restore during session restore, though.

Yes, but there are other occasions, too, where we are restoring tabs with their history apart from the full session restore on startup:
- The Recent Tabs panel
- The "Undo close tab" snackbar

To expand on this, when a document loads, the [`MobileViewportManager`](https://dxr.mozilla.org/mozilla-central/source/layout/base/MobileViewportManager.cpp) set the document's initial resolution on either first paint or load, whichever happens earlier.
My original implementation then called `setResolutionAndScaleTo()` after pageshow to set the restored zoom level, however that had some drawbacks:
- After Gecko had probably already started painting the document at the default resolution, we'd then override that with a new zoom level and would have to paint everything again.
- It gave some strange results and didn't really work on pages with frames.

So instead, kats thankfully provided an API to set a desired initial resolution, which would be taken into account by the `MobileViewportManager` during its initial resolutin calculation. The caveat with that is that we *have* to call that API before this initial resolution calculation happens (as mentioned above, either on load or on first paint), otherwise it won't have any effect.
Unfortunately calling this directly after restoring the history in `_restoreTab()` seems to be too early, so the earliest opportunity we have is when we receive a location change event.

And finally, piggybacking on browser.js's web progress listeners in order to be notified about location change events seemed the easiest option here.

> It's confusing that this flag mentions location change, but the "Session:RestoreZoom" notification is not obviously coupled to location change.
> 
> Could you add some comments to clarify what's going on here?

Alternatively, I could rename the notification to something like "Session:NotifyLocationChange"?

> It feels a bit fragile to continue adding more of these flags. You should also update the comment above to explain why these are necessary and what their intended purpose is.

Aye aye.
Ideally we'd be able to do all of these things directly in `_restoreTab()`, but in practice unfortunately a number of things can only be done after tab loading has progressed far enough, with "far enough" also depending on what exactly we want to do.
I'll add separate comments for each of these flags, though.

Comment 48

[Jan Henning [:JanH]]

Comment on attachment 8754136 [details]
MozReview Request: Bug 810981 - Part 1 - Record current scroll position in mobile session store. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53736/diff/3-4/

Comment 49

[Jan Henning [:JanH]]

Comment on attachment 8754137 [details]
MozReview Request: Bug 810981 - Part 2 - Save the current zoom level when scrolling. r=kats r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53738/diff/3-4/

Comment 50

[Jan Henning [:JanH]]

Comment on attachment 8754138 [details]
MozReview Request: Bug 810981 - Part 3 - Recalculate zoom level before restoring to a different screen orientation. r=kats

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53740/diff/3-4/

Comment 51

[Jan Henning [:JanH]]

Comment on attachment 8754139 [details]
MozReview Request: Bug 810981 - Part 4 - Test session store scroll position and zoom level handling. r=margaret

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53742/diff/3-4/

Comment 52

[Jan Henning [:JanH]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca7eb7528fc3

Comment 53

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5e6a99ab5e9a
https://hg.mozilla.org/integration/fx-team/rev/b92caaa61e80
https://hg.mozilla.org/integration/fx-team/rev/b37f2c10a777
https://hg.mozilla.org/integration/fx-team/rev/8682f94befeb

Comment 54

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5e6a99ab5e9a
https://hg.mozilla.org/mozilla-central/rev/b92caaa61e80
https://hg.mozilla.org/mozilla-central/rev/b37f2c10a777
https://hg.mozilla.org/mozilla-central/rev/8682f94befeb

Comment 55

[Jan Henning [:JanH]]

Release Note Request (optional, but appreciated)
[Why is this notable]: Tab zombification (when Firefox unloads a background tab under memory pressure) no longer means loosing your scroll position. Also works for closing and restoring a tab and closing and reopening Firefox as a whole.
[Suggested wording]: Remember scroll position and zoom level for open tabs.
[Links (documentation, blog post, etc)]: none