Closed Bug 1270019 Opened 5 years ago Closed 5 years ago

Provide a way for the session store to set the initial resolution before first-paint/load

Categories

(Firefox for Android Graveyard :: Toolbar, defect)

All
Android
defect
Not set
normal

Tracking

(firefox49 fixed)

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 --- fixed

People

(Reporter: JanH, Assigned: kats)

References

Details

Attachments

(2 files, 1 obsolete file)

When restoring a page, there's no point in setting an initial resolution before first paint/on load, only for it to be immediately overridden by the session store calling setResolutionAndScaleTo() with the resolution used during the last session. Therefore it would be nice if there was a way to get that info to the APZ code so the MobileViewportManager could take it into account when setting the initial page resolution (https://dxr.mozilla.org/mozilla-central/rev/0a25833062a880f369e6f9f622413a94cc671bf4/layout/base/MobileViewportManager.cpp#158).

From the session store's point of view, the ideal place to pass this information would probably be somewhere in _restoreTab/_restoreHistory (https://dxr.mozilla.org/mozilla-central/rev/0a25833062a880f369e6f9f622413a94cc671bf4/mobile/android/components/SessionStore.js#1116) - also after restoring the full history data, the current history entry is reloaded anyway.

Hopefully this would also sidestep the issues observed in bug 810981 comment 19 with setting the zoom level on pages with framesets.
Assignee: nobody → bugmail.mozilla
Attached patch WIP (obsolete) — Splinter Review
I haven't tested this yet, but if you want to get a head start feel free to apply this and call utils.setRestoreResolution(...) which should do the trick. You have to make sure to do that call early in page load (before the onload or before-first-paint events are fired). The earlier, the better, so maybe somewhere around [1]?

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=8a611f83c0e4#4405
Attached patch GlueSplinter Review
This is the glue that I used on top of your three patches from bug 810981 and my WIP, and it seemed to work ok. I also tried the restoreZoom call during the DOMContentLoaded and it worked there, although I worry that on some pages we do the first-paint before DOMContentLoaded is fired. I'm not too sure about that though - if we know that DOMContentLoaded always happens before the first paint then should be safe in doing it there.

I'll leave the ball in your court for now to test this out more. Let me know if you find any issues with my WIP patch - if you don't find any I can request review on it.
Flags: needinfo?(jh+bugzilla)
Attached patch WIP v2Splinter Review
Now without stupid shadow variable
Attachment #8749440 - Attachment is obsolete: true
It seems to be working very nicely now, including on pages with framesets, so thanks very much.

Calling this directly after restoring the history is unfortunately too early, so I'll have to call it from one of the events that happens afterwards. I think DOMTitleChanged happens slightly earlier than DOMContentLoaded, although I'm not sure either whether it's always guaranteed to be called before the first paint, so I'll probably go for onLocationChange.
Flags: needinfo?(jh+bugzilla)
Tested it a bit and on some pages DOMTitleChanged happens to late in relation to the initial resolution setting, so location change it is.
This is an alternative approach using notifyObservers to call the session store on a location change event:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9d652ca8985
Attachment #8749696 - Flags: review?(rbarker)
Attachment #8749696 - Flags: review?(rbarker) → review+
https://hg.mozilla.org/mozilla-central/rev/1b2d84bdba96
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.