1282902 - Session store incorrectly scales about:firefox if device was rotated between saving and restoring that tab

Status: VERIFIED FIXED

(Firefox for Android :: Toolbar, defect, P3)

Description

[Jan Henning [:JanH]]

Splitting this off from bug 1279443:

If I rotate my phone from portrait to landscape or vice versa between closing and restarting the browser, about:firefox is displayed at a wrong zoom level (zoomed in when going from portrait to landscape, zoomed out when going the other way). This is because while Firefox is running, that page behaves like a standard mobile page with "width=device-width", i.e. its resolution doesn't change when the window width changes on device rotation and instead always remains at ~0.6667. [1]
When queried through getViewportInfo() however, that page is reported as autoSize = false [2], which is the signal for the session store to scale the stored zoom level if the window width has changed between saving and restoring of a tab.

[1] <meta name="viewport" content="width=480; initial-scale=.6667; user-scalable=no"/> (https://dxr.mozilla.org/mozilla-central/rev/016e0f47e8ad66ba6eb11fe28958e3a69ef9e53d/mobile/android/chrome/content/about.xhtml#18)
[2] Where I think that info is ultimately coming from: https://dxr.mozilla.org/mozilla-central/rev/016e0f47e8ad66ba6eb11fe28958e3a69ef9e53d/dom/base/nsDocument.cpp#8131

I think this basically boils down to:
- Either the behaviour of autoSize isn't correct, and it ought to return "true" for about:firefox's meta viewport string, too.
- Or else the session store needs some different property which properly models whether or not a document's resolution needs to be scaled when the window width changes.
- And as a possible stopgap, make a specific exception for "about:firefox" in the session store's scaling code.

Comment 1

[Jan Henning [:JanH]]

After some playing around, it seems that incorporating the viewport width change ratio into the calculation (compare the MVM's native code: https://dxr.mozilla.org/mozilla-central/rev/e45890951ce77c3df05575bd54072b9f300d77b0/layout/base/MobileViewportManager.cpp#234) might be the missing puzzle piece.

Comment 2

[Jan Henning [:JanH]]

Argh, it turns out the session store's zoom restore function runs before dom-meta-added, meaning the viewport returned via getViewportInfo is bogus and cannot be used for recalculating the stored resolution level at that time.

Maybe I'll look into how easily setRestoreResolution() could be expanded to also take the previous display and viewport widths as parameters after all and let the MVM worry about correctly recalculating that value.

Comment 3

[Jan Henning [:JanH]]

I've got an implementation that seems to be working now, albeit with one remaining problem:

In order to have the session store use exactly the same values as the MVM (compare https://dxr.mozilla.org/mozilla-central/rev/b69a5bbb5e40bd426e35222baa600b481e50d265/layout/base/MobileViewportManager.cpp#344), I need the size of the content window in device pixels. However unfortunately all that I seem to be able to access from the JS side is window dimensions in CSS pixels, which introduces some rounding errors [1].

[1] There is window.gScreenWidth, however that is just the result of "window.outerWidth * window.devicePixelRatio" and therefore suffers from the same rounding problem.

Comment 4

[Jan Henning [:JanH]]

Attachment: Bug 1282902 - fix getViewportInfo

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

Comment 5

[Jan Henning [:JanH]]

Attachment: Bug 1282902 - Enable MVM logging

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

Comment 6

[Jan Henning [:JanH]]

Attachment: Bug 1282902 - Part 2 - Extract resolution scaling into separate function.

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

Comment 7

[Jan Henning [:JanH]]

Attachment: Bug 1282902 - Directly pass widths and resolution to recalc function instead of complete viewport

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

Comment 8

[Jan Henning [:JanH]]

Attachment: Bug 1282902 - Part 3 - Let the MobileViewportManager recalculate the saved resolution if the display width changed before restoring.

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

Comment 9

[Kartikaya Gupta (email:[e-mail]) (away until Feb-2020)]

Can you give an example of what kind of values you're getting from window.devicePixelRatio and gScreenWidth and what the rounding error is?

Comment 10

[Jan Henning [:JanH]]

On my phone, it's 1.5, so in landscape mode if I remember correctly:
800 display px ->
533 CSS px ->
799.5 display px according to gScreenWidth

So the error is not very big, but it can mean that e.g. a typical mobile page, whose (unzoomed) resolution should always remain at 1 even after rotation, suddenly gets scaled slightly upon session restore if the device was rotated since the last session save. Actually thinking of it, this might happen even without rotating the phone simply because the viewport dimensions as calculated by the session store and the MVM differ (because getViewportInfo uses the display dimensions passed by the caller), so the cssViewportChangeRatio is suddenly != 1 and starts scaling the resolution.

There doesn't seem to be anything available from DOMWindowUtils, however I've written a new helper methods which looks like it should work (haven't had the time to actually compile and try it yet, possibly tomorrow or so).

Comment 11

[Jan Henning [:JanH]]

Attachment: Bug 1282902 - Part 1 - Add a helper method for getting the content viewer size in device pixels.

The intention is for the session store to record the current window size and then pass it to the MobileViewportManager for restoring, so the latter can correctly scale the stored resolution if the display width has since changed. However currently all window dimensions available from the JS side are measured in CSS pixels rounded to integers. Transforming those values back into display pixels by multiplying them with "window.devicePixelRatio" (or accessing window.gScreenWidth/Height, which does the same thing internally) unfortunately introduces some rounding errors.

Therefore this patch introduces a new helper method in DOMWindowUtils which allows to access the content window width as measured in device pixels from the JS side, too.

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

Comment 12

[Jan Henning [:JanH]]

Comment on attachment 8766915 [details]
Bug 1282902 - Part 2 - Extract resolution scaling into separate function.

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

Comment 13

[Jan Henning [:JanH]]

Comment on attachment 8766917 [details]
Bug 1282902 - Part 3 - Let the MobileViewportManager recalculate the saved resolution if the display width changed before restoring.

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

Comment 14

[Kartikaya Gupta (email:[e-mail]) (away until Feb-2020)]

Comment on attachment 8767470 [details]
Bug 1282902 - Part 1 - Add a helper method for getting the content viewer size in device pixels.

https://reviewboard.mozilla.org/r/61938/#review58842

Comment 15

[Kartikaya Gupta (email:[e-mail]) (away until Feb-2020)]

Comment on attachment 8766915 [details]
Bug 1282902 - Part 2 - Extract resolution scaling into separate function.

https://reviewboard.mozilla.org/r/61634/#review58840

::: layout/base/MobileViewportManager.h:62
(Diff revision 2)
>    /* Helper to clamp the given zoom by the min/max in the viewport info. */
>    mozilla::CSSToScreenScale ClampZoom(const mozilla::CSSToScreenScale& aZoom,
>                                        const nsViewportInfo& aViewportInfo);
>  
> +  /* Helper to update the given resolution according to changed display and viewport widths. */
> +  mozilla::LayoutDeviceToLayerScale ScaleResolutionWithDisplayWidth(const mozilla::LayoutDeviceToLayerScale& aRes,

Stick a linebreak between the return type and the function name (and unindent the subsequent lines appropriately) so that the lines are less long.

Comment 16

[Kartikaya Gupta (email:[e-mail]) (away until Feb-2020)]

Comment on attachment 8766917 [details]
Bug 1282902 - Part 3 - Let the MobileViewportManager recalculate the saved resolution if the display width changed before restoring.

https://reviewboard.mozilla.org/r/61638/#review58844

So this looks fine, but I'd like to keep using the strongly-typed units as noted below, rather than using the raw types like float and uint32_t.

::: dom/base/nsDOMWindowUtils.cpp:568
(Diff revision 2)
>    nsIPresShell* presShell = GetPresShell();
>    if (!presShell) {
>      return NS_ERROR_FAILURE;
>    }
>  
> -  presShell->SetRestoreResolution(aResolution);
> +  presShell->SetRestoreResolution(aResolution, aDisplayWidth, aDisplayHeight);

I'd prefer if you wrapped the aDisplayWidth/Height values into a LayoutDeviceIntSize here and passed that to presShell->SetRestoreResolution instead. We're trying to use the strongly-typed units wherever possible rather than "float" and "int". The MVM function signature should also take a LayoutDeviceIntSize.

::: dom/interfaces/base/nsIDOMWindowUtils.idl:231
(Diff revision 2)
>     * Set a resolution on the presShell which is the "restored" from history.
>     * This resolution should be used when painting for the first time. Calling
>     * this too late may have no effect.

Update comment to refer to the display size as well.

::: layout/base/MobileViewportManager.h:67
(Diff revision 2)
> -  mozilla::LayoutDeviceToLayerScale ScaleResolutionWithDisplayWidth(const mozilla::LayoutDeviceToLayerScale& aRes,
> +  mozilla::LayoutDeviceToLayerScale ScaleResolutionWithDisplayWidth(const float& aRes,
>                                                                      const float& aDisplayWidthChangeRatio,
> -                                                                    const mozilla::CSSSize& aNewViewport,
> -                                                                    const mozilla::CSSSize& aOldViewport);
> +                                                                    const float& aNewViewportWidth,
> +                                                                    const float& aOldViewportWidth);

Here too I would like to keep using the strongly-typed units rather than "float". It looks like the call sites have the strongly-typed units, so I'm not sure if you had a good reason for changing this.

Comment 17

[Jan Henning [:JanH]]

https://reviewboard.mozilla.org/r/61638/#review58844

> Here too I would like to keep using the strongly-typed units rather than "float". It looks like the call sites have the strongly-typed units, so I'm not sure if you had a good reason for changing this.

I possibly might have had a reason in a previous approach (where the session store directly stored only the display and viewport width), but you're right, I no longer have. Thanks for catching this.

Comment 18

[Jan Henning [:JanH]]

Attachment: Bug 1282902 - Part 4 - Include zoom level recalculation in session store test.

Because we can't actually rotate the emulator within the test, we just manipulate the session store's stored display size to pretend that the device was rotated between tab data capturing and restoring.

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

Comment 19

[Jan Henning [:JanH]]

Comment on attachment 8767470 [details]
Bug 1282902 - Part 1 - Add a helper method for getting the content viewer size in device pixels.

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

Comment 20

[Jan Henning [:JanH]]

Comment on attachment 8766915 [details]
Bug 1282902 - Part 2 - Extract resolution scaling into separate function.

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

Comment 21

[Jan Henning [:JanH]]

Comment on attachment 8766917 [details]
Bug 1282902 - Part 3 - Let the MobileViewportManager recalculate the saved resolution if the display width changed before restoring.

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

Comment 22

[Kartikaya Gupta (email:[e-mail]) (away until Feb-2020)]

Comment on attachment 8766917 [details]
Bug 1282902 - Part 3 - Let the MobileViewportManager recalculate the saved resolution if the display width changed before restoring.

https://reviewboard.mozilla.org/r/61638/#review59064

Looks good thanks!

::: layout/base/MobileViewportManager.cpp:202
(Diff revision 3)
>    LayoutDeviceToLayerScale res(mPresShell->GetResolution());
>  
>    if (mIsFirstPaint) {
>      CSSToScreenScale defaultZoom;
>      if (mRestoreResolution) {
> -      defaultZoom = CSSToScreenScale(mRestoreResolution.value() * cssToDev.scale);
> +    LayoutDeviceToLayerScale restoreResolution =

You should be able to write more concisely as LayoutDeviceToLayerScale restoreResolution(mRestoreResolution.value());

Comment 23

[Kartikaya Gupta (email:[e-mail]) (away until Feb-2020)]

Comment on attachment 8767756 [details]
Bug 1282902 - Part 4 - Include zoom level recalculation in session store test.

https://reviewboard.mozilla.org/r/62158/#review59066

This looks fine to me but i have never written one of these tests before so if you want a more comprehensive review it would be good to get a second opinion as well. Your call.

Comment 24

[Jan Henning [:JanH]]

https://reviewboard.mozilla.org/r/61638/#review59064

> You should be able to write more concisely as LayoutDeviceToLayerScale restoreResolution(mRestoreResolution.value());

Thanks, I was wondering whether there wasn't some shorter syntax for this.

Comment 25

[Jan Henning [:JanH]]

Comment on attachment 8766917 [details]
Bug 1282902 - Part 3 - Let the MobileViewportManager recalculate the saved resolution if the display width changed before restoring.

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

Comment 26

[Jan Henning [:JanH]]

Comment on attachment 8767756 [details]
Bug 1282902 - Part 4 - Include zoom level recalculation in session store test.

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

Comment 27

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8767756 [details]
Bug 1282902 - Part 4 - Include zoom level recalculation in session store test.

https://reviewboard.mozilla.org/r/62158/#review59706

Comment 28

[Jan Henning [:JanH]]

Comment on attachment 8767470 [details]
Bug 1282902 - Part 1 - Add a helper method for getting the content viewer size in device pixels.

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

Comment 29

[Jan Henning [:JanH]]

Comment on attachment 8766915 [details]
Bug 1282902 - Part 2 - Extract resolution scaling into separate function.

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

Comment 30

[Jan Henning [:JanH]]

Comment on attachment 8766917 [details]
Bug 1282902 - Part 3 - Let the MobileViewportManager recalculate the saved resolution if the display width changed before restoring.

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

Comment 31

[Jan Henning [:JanH]]

Comment on attachment 8767756 [details]
Bug 1282902 - Part 4 - Include zoom level recalculation in session store test.

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

Comment 32

[Jan Henning [:JanH]]

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

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/68791be33a94
Part 1 - Add a helper method for getting the content viewer size in device pixels. r=kats
https://hg.mozilla.org/integration/fx-team/rev/c3b2e397148f
Part 2 - Extract resolution scaling into separate function. r=kats
https://hg.mozilla.org/integration/fx-team/rev/93aa042c0fd8
Part 3 - Let the MobileViewportManager recalculate the saved resolution if the display width changed before restoring. r=kats
https://hg.mozilla.org/integration/fx-team/rev/9329897c803d
Part 4 - Include zoom level recalculation in session store test. r=sebastian

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/68791be33a94
https://hg.mozilla.org/mozilla-central/rev/c3b2e397148f
https://hg.mozilla.org/mozilla-central/rev/93aa042c0fd8
https://hg.mozilla.org/mozilla-central/rev/9329897c803d

Comment 35

[Kartikaya Gupta (email:[e-mail]) (away until Feb-2020)]

Jan, do you want to uplift this to Fx 49?

Comment 36

[Jan Henning [:JanH]]

Yes, I just wanted to leave it a few days on Nightly to be on the safe side. But compared to bug 1279443 I think this one has less potential to break things in a subtle way, so I might as well request it now.

Comment 37

[Jan Henning [:JanH]]

Comment on attachment 8767470 [details]
Bug 1282902 - Part 1 - Add a helper method for getting the content viewer size in device pixels.

Approval Request Comment
[Feature/regressing bug #]: Mobile session store, bug 810981
[User impact if declined]: For some pages, a tab will be restored at an incorrect zoom level if the device is rotated between closing and restoring of that tab. This also applies to tabs unloaded in background or if the whole session is restored after Firefox has been killed.
[Describe test coverage new/current, TreeHerder]: new and existing mochitests, manual
[Risks and why]: Low, that code is well understood and only small refactoring was necessary to share the existing C++ implementation with the session store and remove some code duplication.
[String/UUID change made/needed]: none

Comment 38

[Jan Henning [:JanH]]

Comment on attachment 8766915 [details]
Bug 1282902 - Part 2 - Extract resolution scaling into separate function.

(see above)

Comment 39

[Jan Henning [:JanH]]

Comment on attachment 8766917 [details]
Bug 1282902 - Part 3 - Let the MobileViewportManager recalculate the saved resolution if the display width changed before restoring.

(see above)

Comment 40

[Jan Henning [:JanH]]

Comment on attachment 8767756 [details]
Bug 1282902 - Part 4 - Include zoom level recalculation in session store test.

(see above)

Comment 41

[Sorina Florean [:sflorean]]

Verified as fixed in build 50.0a1 2016-07-14;
Device: Motorola Razr (Android 4.4.4) and LG G4 (Android 5.1).

Comment 42

[Gerry Chang [:gchang]]

Comment on attachment 8766915 [details]
Bug 1282902 - Part 2 - Extract resolution scaling into separate function.

These patches fix a device rotating issue and also add tests. Take it in 49 aurora.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/86acc0c87f52
https://hg.mozilla.org/releases/mozilla-aurora/rev/2d8cb00e2c85
https://hg.mozilla.org/releases/mozilla-aurora/rev/972c9d41b6d3
https://hg.mozilla.org/releases/mozilla-aurora/rev/e6ea844380a6

Comment 44

[Sorina Florean [:sflorean]]

Verified as fixed in build 49.0a2 2016-07-29;
Device: Motorola Razr (Android 4.4.4) and Nexus 5 (Android 6.0.1).