Closed
Bug 787078
Opened 12 years ago
Closed 12 years ago
Going to back to Reader is broken
Categories
(Firefox for Android Graveyard :: Reader View, defect, P1)
Tracking
(firefox16 verified, firefox17 verified, firefox18 verified)
VERIFIED
FIXED
Firefox 18
People
(Reporter: lucasr, Assigned: lucasr)
References
Details
Attachments
(2 files, 1 obsolete file)
5.45 KB,
patch
|
mfinkle
:
review+
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
1.65 KB,
patch
|
mfinkle
:
review+
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Seems like this is a regression from bug 778582. This patch fixes this.
Attachment #656881 -
Flags: review?(mark.finkle)
Updated•12 years ago
|
Attachment #656881 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Comment 1•12 years ago
|
||
Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/43dd8252f52d
Assignee | ||
Comment 2•12 years ago
|
||
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.
Attachment #656881 -
Flags: approval-mozilla-beta?
Attachment #656881 -
Flags: approval-mozilla-aurora?
Comment 3•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/43dd8252f52d
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
Assignee | ||
Comment 4•12 years ago
|
||
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Updated•12 years ago
|
Attachment #656881 -
Flags: approval-mozilla-beta?
Attachment #656881 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 5•12 years ago
|
||
Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/0a93ae68184e
Assignee | ||
Comment 6•12 years ago
|
||
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.
Attachment #657294 -
Flags: review?(mark.finkle)
Assignee | ||
Comment 7•12 years ago
|
||
Attachment #656881 -
Attachment is obsolete: true
Attachment #657295 -
Flags: review?(mark.finkle)
Comment 8•12 years ago
|
||
Comment on attachment 657294 [details] [diff] [review] Fix back behaviour in Reader Sorry I didn't notice the need for WeakReference the first time.
Attachment #657294 -
Flags: review?(mark.finkle) → review+
Updated•12 years ago
|
Attachment #657295 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Comment 9•12 years ago
|
||
Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/0777a56f6a7d https://hg.mozilla.org/integration/mozilla-inbound/rev/371aea049dcb
Comment 10•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0a93ae68184e https://hg.mozilla.org/mozilla-central/rev/0777a56f6a7d https://hg.mozilla.org/mozilla-central/rev/371aea049dcb
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 11•12 years ago
|
||
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
Attachment #657295 -
Flags: approval-mozilla-beta?
Attachment #657295 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 12•12 years ago
|
||
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.
Attachment #657294 -
Flags: approval-mozilla-beta?
Attachment #657294 -
Flags: approval-mozilla-aurora?
Comment 14•12 years ago
|
||
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
Status: RESOLVED → VERIFIED
status-firefox18:
--- → verified
Updated•12 years ago
|
Attachment #657294 -
Flags: approval-mozilla-beta?
Attachment #657294 -
Flags: approval-mozilla-beta+
Attachment #657294 -
Flags: approval-mozilla-aurora?
Attachment #657294 -
Flags: approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #657295 -
Flags: approval-mozilla-beta?
Attachment #657295 -
Flags: approval-mozilla-beta+
Attachment #657295 -
Flags: approval-mozilla-aurora?
Attachment #657295 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 15•12 years ago
|
||
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
Updated•12 years ago
|
status-firefox16:
--- → fixed
status-firefox17:
--- → fixed
Updated•12 years ago
|
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•