787078 - Going to back to Reader is broken

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Reader View, defect, P1)

Description

[Lucas Rocha (:lucasr)]

Attachment: Load Reader UI on pageshow instead of DOMContentLoaded

Seems like this is a regression from bug 778582. This patch fixes this.

Comment 1

[Lucas Rocha (:lucasr)]

Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/43dd8252f52d

Comment 2

[Lucas Rocha (:lucasr)]

Comment on attachment 656881 [details] [diff] [review]
Load Reader UI on pageshow instead of DOMContentLoaded

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Small regression from bug 778582.
User impact if declined: If user presses back button to go back to Reader Mode from another page, Reader will show empty content.
Testing completed (on m-c, etc.): Local testing, no regressions.
Risk to taking this patch (and alternatives if risky): Low, just moving existing code to be run on "pageshow"  instead of "DOMContentLoaded".
String or UUID changes made by this patch: None.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/43dd8252f52d

Comment 4

[Lucas Rocha (:lucasr)]

My patch exposed a wider (pre-existing) issue with how we initialize reader UI. When pressing back we might be simply reusing the existing content document for reader. If we initialize reader UI on pageshow, we'll end up with duplicate event handlers in the UI because the reused document already has them. As a side effect, we were always reloading the article from URL or cache when going back, which is not really necessary if we're on the same document.

This is an issue that pre-dates the fix for bug 778582 btw. We'll have to back out the patch that has already been landed for this bug and simply do a simpler uninit() that doesn't remove event listeners in the DOM. This makes going back to reader be instantaneous and fixes this bug properly.

I'll submit new patches now.

Comment 5

[Lucas Rocha (:lucasr)]

Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/0a93ae68184e

Comment 6

[Lucas Rocha (:lucasr)]

Attachment: Fix back behaviour in Reader

This patch makes back to Reader be instant and fixes the original problem reported in this bug. I've also made sure we're not holding any strong references to the elements (document, window, and some inner page elements) that Reader deals with to avoid any memory leaks.

Comment 7

[Lucas Rocha (:lucasr)]

Attachment: Always return a result from a Reader:FaviconRequest

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 657294 [details] [diff] [review]
Fix back behaviour in Reader

Sorry I didn't notice the need for WeakReference the first time.

Comment 9

[Lucas Rocha (:lucasr)]

Pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0777a56f6a7d
https://hg.mozilla.org/integration/mozilla-inbound/rev/371aea049dcb

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0a93ae68184e
https://hg.mozilla.org/mozilla-central/rev/0777a56f6a7d
https://hg.mozilla.org/mozilla-central/rev/371aea049dcb

Comment 11

[Lucas Rocha (:lucasr)]

Comment on attachment 657295 [details] [diff] [review]
Always return a result from a Reader:FaviconRequest

[Approval Request Comment]
User impact if declined: Reader page might not get favicon if user moves to another page before the favicon request gets a response.
Testing completed (on m-c, etc.): Landed in Nightly, no issues found.
Risk to taking this patch (and alternatives if risky): Very low, this patch ensure a return is always given to a Reader:FaviconRequest
String or UUID changes made by this patch: None

Comment 12

[Lucas Rocha (:lucasr)]

Comment on attachment 657294 [details] [diff] [review]
Fix back behaviour in Reader

[Approval Request Comment]
User impact if declined: Fixes a subtle regression from bug 778582. On the other hand, it fixes a performance bug when going back to Reader that has been around for a long time (way before bug 778582 landed).
Testing completed (on m-c, etc.): Landed in Nightly, no issues found.
Risk to taking this patch (and alternatives if risky): Low, this patch is about removing AboutReader:uninit() (because it doesn't make sense to have it anyway) and changes AboutReader to avoid memory leaks.
String or UUID changes made by this patch: None.

Comment 14

[Cristian Nicolae (:xti)]

This issue is not reproducible anymore on the latest Nightly. Closing bug as verified fixed on:

Firefox 18.0a1 (2012-09-03)
Device: Galaxy Note
OS: Android 4.0.4

Comment 15

[Lucas Rocha (:lucasr)]

Pushed to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/50f2c5b9621a
https://hg.mozilla.org/releases/mozilla-aurora/rev/3e7b6ed1bbd3

Pushed to beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/6b778c9a38c0
https://hg.mozilla.org/releases/mozilla-beta/rev/27cbeb1cb920