1269189 - Session store doesn't track browsing on pages with frames

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jan Henning [:JanH]]

I don't quite know why we've restricted the DOMTitleChanged listener to top-level events only (see bug 969700 - Brian, can you still remember why?), but it means we don't track browsing that happens on a page with frames, e.g. http://www.railfaneurope.net/pix_frameset.html.

With (https://dxr.mozilla.org/mozilla-central/rev/1461a4071341c282afcf7b72e33036412d2251d4/mobile/android/components/SessionStore.js#272) commented out, the session store starts tracking page navigation within subframes and restoring closed/unloaded/... pages with frames starts working beautifully, but I'm wondering whether there are any unwanted side effects...

Comment 1

[Brian Nicholson (:bnicholson)]

Looking at the patch in bug 969700, the previous implementation was probably throwing a bunch of errors since BrowserApp.getBrowserForDocument() will return null for any browsers that aren't tabs. We didn't support subframe restoring at the time (do we now?), so there wasn't any reason to continue if the browser wasn't a top-level frame.

Comment 2

[Jan Henning [:JanH]]

Thanks, in that case I guess I can risk going forward with this. I don't quite know where the magic is happening, either, but it's definitively working - presumably somewhere within the history restore part.

Comment 3

[Jan Henning [:JanH]]

Attachment: MozReview Request: Bug 1269189 - Let mobile session store capture frameset navigation, too. r=bnicholson

Since the history restore code now apparently supports restoring subframes, too, we should let the mobile session store capture frameset navigation as well, so we can preserve the full browsing history.

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

Comment 4

[Brian Nicholson (:bnicholson)]

Comment on attachment 8747634 [details]
MozReview Request: Bug 1269189 - Let mobile session store capture frameset navigation, too. r=bnicholson

https://reviewboard.mozilla.org/r/49977/#review46793

Looks fine to me!

Comment 5

[Jan Henning [:JanH]]

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

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/7cddc34ca303

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7cddc34ca303