Closed Bug 1264805 Opened 4 years ago Closed 4 years ago

[Fennec] Disabling Reader Mode reloads the page

Categories

(Toolkit :: Reader Mode, defect)

defect
Not set
Points:
5

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: timdream, Assigned: timdream, Mentored)

References

(Blocks 1 open bug)

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.
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.
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 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+
Do I need to add new tests? Is the Try syntax in comment 2 correct? Thanks!
Flags: needinfo?(margaret.leibovic)
(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)
https://hg.mozilla.org/mozilla-central/rev/5655a04459af
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.