Closed
Bug 1264805
Opened 8 years ago
Closed 8 years ago
[Fennec] Disabling Reader Mode reloads the page
Categories
(Toolkit :: Reader Mode, defect)
Toolkit
Reader Mode
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: timdream, Assigned: timdream, Mentored)
References
Details
(Whiteboard: [outreachy-12])
Attachments
(1 file)
This bug represents the Fennec fix of the same bug. +++ This bug was initially created as a clone of Bug #1184950 +++ STR: 1. Go to a page. 2. Enable Reader Mode. 3. Disable Reader Mode. 4. Wait for the page to reload. (...I noticed this because the page reloaded VERY slowly...which made no sense to me) Expected Result: The page should immediately disable reader mode and re-render with the original content without making network connections (everything should be cached, right?) This is particularly problematic for pages that you *CANNOT* reload, e.g. a page you POSTed to.
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/47583/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/47583/
Attachment #8743123 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 2•8 years ago
|
||
It is unclear to me which tests to run and how to run them locally. Let me find out from this try result https://treeherder.mozilla.org/#/jobs?repo=try&revision=79609834189e and MDN when I know what to look for.
Assignee | ||
Comment 3•8 years ago
|
||
BTW the page still reloads on my phone, even when the patch is applied (and I can verify it works based by observing session history), are we purging BFCache on Fennec differently? Are we able to adjust that somehow?
Comment 4•8 years ago
|
||
Comment on attachment 8743123 [details] MozReview Request: Bug 1264805 - [Fennec] Use goBack to leave the reader view when possible, r=margaret https://reviewboard.mozilla.org/r/47583/#review44547 ::: mobile/android/chrome/content/Reader.js:172 (Diff revision 1) > - browser.loadURI(originalURL); > - } > Reader._buttonHistogram.add(Reader._buttonHistogramValues.TAP_EXIT); > } else { > - browser.messageManager.sendAsyncMessage("Reader:ParseDocument", { url: url }); > Reader._buttonHistogram.add(Reader._buttonHistogramValues.TAP_ENTER); Technically it would be nice to move this telemetry to where the toggling is happening, but I actually want to change this around anyway (bug 1266163), so it's fine to leave it like this for now.
Attachment #8743123 -
Flags: review?(margaret.leibovic) → review+
Assignee | ||
Comment 5•8 years ago
|
||
Do I need to add new tests? Is the Try syntax in comment 2 correct? Thanks!
Flags: needinfo?(margaret.leibovic)
Comment 6•8 years ago
|
||
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #5) > Do I need to add new tests? Is the Try syntax in comment 2 correct? Thanks! I think this looks good to me... I normally do -u all, but mcomella wrote a handy hg extension that will come up with the correct try synxtax for you: https://github.com/mcomella/push-to-try-android#run
Flags: needinfo?(margaret.leibovic)
Comment 9•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5655a04459af
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•