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.

This simple patch uses the browser's goForward/goBack whenever possible. While not a perfect solution, it seems to improve things, especially when disabling reader mode. Depending on the complexity of the page (ads etc) it doesn't always completely remove the need to reload parts of the page, however, so I'm not sure how to quantify the improvement.

We used to have an approach like this in our initial mobile implementation, but I think we got rid of it because there was a weird interaction with the back button. But that may have also been due to the fact that we used to support hitting the back button to close the font styles popup. I don't have time right now, but it could be worth looking into the hg history to read about what went into this decision.

(In reply to :Margaret Leibovic from comment #2) > We used to have an approach like this in our initial mobile implementation, > but I think we got rid of it because there was a weird interaction with the > back button. But that may have also been due to the fact that we used to > support hitting the back button to close the font styles popup. Thanks! On the face of it I don't see how there could be any negative side effects on the back button behaviour - the patch in effect simulates clicking it, which is something the user can already choose to do. > I don't have time right now, but it could be worth looking into the hg > history to read about what went into this decision. Looking at the log for browser/modules/ReaderParent.jsm and mobile/android/chrome/content/Reader.js, I couldn't find anything related. Are there older (now deleted) files to look at?

Comment on attachment 8635426 [details] [diff] [review] readermode.diff Review of attachment 8635426 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/modules/ReaderParent.jsm @@ +220,5 @@ > let win = event.target.ownerDocument.defaultView; > let browser = win.gBrowser.selectedBrowser; > let url = browser.currentURI.spec; > + let sessionHistory = browser.webNavigation.sessionHistory; > + let index = sessionHistory.index; You don't have access to nsIWebNavigation or nsISHistory from the parent (in e10s, without using CPOWs), this needs to be implemented in the child if you decide to go with that solution.

(In reply to Tim Taubert [:ttaubert] from comment #4) > > + let sessionHistory = browser.webNavigation.sessionHistory; > > + let index = sessionHistory.index; > > You don't have access to nsIWebNavigation or nsISHistory from the parent (in > e10s, without using CPOWs), this needs to be implemented in the child if you > decide to go with that solution. It's probably my lack of experience with e10s, but I'm a little confused by this, since existing code in browser.js (into which ReaderParent.jsm is imported) also accesses the session history in this way, e.g. https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#3696 What am I missing?

(In reply to aleth [:aleth] from comment #5) > It's probably my lack of experience with e10s, but I'm a little confused by > this, since existing code in browser.js (into which ReaderParent.jsm is > imported) also accesses the session history in this way, e.g. > https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser. > js#3696 > What am I missing? I was missing the fact that the code I linked (used for the back button context menu) to also generates an "unsafe CPOW usage" warning ;)

(In reply to aleth [:aleth] from comment #5) > It's probably my lack of experience with e10s, but I'm a little confused by > this, since existing code in browser.js (into which ReaderParent.jsm is > imported) also accesses the session history in this way, e.g. > https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser. > js#3696 Yeah, we're transitioning towards e10s, i.e. we're not quite there yet. Which unfortunately means that existing code isn't always the best role model. (In reply to aleth [:aleth] from comment #6) > I was missing the fact that the code I linked (used for the back button > context menu) to also generates an "unsafe CPOW usage" warning ;) Yes, consider that our "deprecation warning" for reaching into the content process. Using a CPOW means that the main/UI process freezes until the child responds and we want to avoid that. The word "unsafe" is in there because that can in some cases also completely lock up Firefox.

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

https://reviewboard.mozilla.org/r/46247/#review42763 I took the patch and move logic to tab-content.js. I did not port the goForward part of the patch. Please tell me if this is alright. I also don't know if this affects the reader mode on mobile as well. If so I wonder if we should address this on the same bug or file a different one.

https://reviewboard.mozilla.org/r/46247/#review42765 Could you tell me which test suites this patch should be tested on on Try also? Thanks.

There is also a question on the user's mental model of session history; why would the "go back" button allow me to go back to the same post but different mode? And worse, why would the go-to-reader-mode button create a new state in history and removed all my forward history. This patch might be a hack on top of the already hacky state. I think we need a UX decision here (which also related to bug 1185918 and friends).

Comment on attachment 8741172 [details] MozReview Request: Bug 1184950 - Use goBack to leave the reader view when possible, r=margaret, r=gijs https://reviewboard.mozilla.org/r/46249/#review43033 ::: browser/base/content/tab-content.js:274 (Diff revision 1) > + if (webNav.canGoBack) { > + let prevEntry = sh.getEntryAtIndex(sh.index - 1, false); > + let prevURL = prevEntry.URI.spec; > + if (prevURL && (prevURL == originalURL || !originalURL)) { > + webNav.goBack(); > + break; This seems reasonable to me, but I'm trying to remember why we didn't do this before... we used to actually have something similar on mobile, and I'm having a tough time remembering why we removed that. Digging through history I did find this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=872965 Which makes me remember that the original implementation for our reader view button was in Java, so it would have been a lot harder to do this work to peek into the session. Let's get a second opinion from Gijs, but I think this is fine to land. You should also update the mobile logic for this in mobile/android/chrome/content/Reader.js and mobile/android/chrome/content/content.js. The logic should be very similar. You can either add a new commit here, which I can review, or file a follow-up bug to address that.

Comment on attachment 8741172 [details] MozReview Request: Bug 1184950 - Use goBack to leave the reader view when possible, r=margaret, r=gijs This looks OK to me, but please could we have an automated test of sorts for this functionality?

https://reviewboard.mozilla.org/r/46249/#review43033 > This seems reasonable to me, but I'm trying to remember why we didn't do this before... we used to actually have something similar on mobile, and I'm having a tough time remembering why we removed that. > > Digging through history I did find this bug: > https://bugzilla.mozilla.org/show_bug.cgi?id=872965 > > Which makes me remember that the original implementation for our reader view button was in Java, so it would have been a lot harder to do this work to peek into the session. > > Let's get a second opinion from Gijs, but I think this is fine to land. > > You should also update the mobile logic for this in mobile/android/chrome/content/Reader.js and mobile/android/chrome/content/content.js. The logic should be very similar. You can either add a new commit here, which I can review, or file a follow-up bug to address that. Let me file another bug to do it; I have not set up the Fennec environment so it would take some time.

Comment on attachment 8741172 [details] MozReview Request: Bug 1184950 - Use goBack to leave the reader view when possible, r=margaret, r=gijs Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46249/diff/1-2/

(In reply to :Gijs Kruitbosch from comment #13) > Comment on attachment 8741172 [details] > MozReview Request: Bug 1184950 - Use goBack to leave the reader view when > possible, r=margaret > > This looks OK to me, but please could we have an automated test of sorts for > this functionality? We have tests on this already; I have modified accordingly to assert the session history.

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

Comment on attachment 8741172 [details] MozReview Request: Bug 1184950 - Use goBack to leave the reader view when possible, r=margaret, r=gijs Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46249/diff/2-3/

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f945e5bb9c0

backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8718970&repo=fx-team

Backout: https://hg.mozilla.org/integration/fx-team/rev/d0c2f09a6285

This is pretty strange because I am sure the changes included in comment 20 are supposedly to address exactly that! OS X passes locally too. Probably a timing issue to be resolved. Let me see what I can do here.

Running the same Try run again and with macosx64 this time: https://treeherder.mozilla.org/#/jobs?repo=try&revision=66e3f6dd50fc

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #24) > Running the same Try run again and with macosx64 this time: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=66e3f6dd50fc Let's try again; Based on the Try result we should not be causing the errors.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #16) > (In reply to :Gijs Kruitbosch from comment #13) > > Comment on attachment 8741172 [details] > > MozReview Request: Bug 1184950 - Use goBack to leave the reader view when > > possible, r=margaret > > > > This looks OK to me, but please could we have an automated test of sorts for > > this functionality? > > We have tests on this already; I have modified accordingly to assert the > session history. You should normally get review for test changes as part of the review for the code changes. I'm not sure what the checkin-needed is for at this stage, but AFAICT this change: https://hg.mozilla.org/integration/fx-team/rev/3c369626af41#l2.13 makes no sense, because promiseTabLoadEvent is here: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/head.js#597-611 and has no third parameter. Same issue with the change in browser_readerMode.js Can you explain and/or update these and get review for these changes?

I didn't realized a need another r+, since I wasn't explicitly told to ask for review after fixing tests. That said, I agree I should have been passing tests locally & try before asking for review at first place. After dxr-reading, I realized the patch failed because I didn't rebase my branch to include bug 1090315, which lands right before! So there *was* a 3rd parameter, it just that there isn't one any more.

Comment on attachment 8741172 [details] MozReview Request: Bug 1184950 - Use goBack to leave the reader view when possible, r=margaret, r=gijs Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46249/diff/3-4/

https://reviewboard.mozilla.org/r/46249/#review43935 promiseWaitForCondition is very very evil. It uses setTimeout loops and is a recipe for intermittent failures. I don't really understand why you can't just use e.g. `yield BrowserTestUtils.browserLoaded(tab.linkedBrowser)` - do we need to wait for something else? What is the something else? I'm fairly sure waiting for `currentURI` to be updated isn't the right thing, either...

https://reviewboard.mozilla.org/r/46249/#review43935 I can't use browserLoaded for the instance because the tab goes one step back in the history (and shows the original article in BFCache), as opposed to (re-)load the original article page. As you can see, I was expecting the `pageshow` event. Could you recommend a better way to do this? Thanks.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #30) > https://reviewboard.mozilla.org/r/46249/#review43935 > > I can't use browserLoaded for the instance because the tab goes one step > back in the history (and shows the original article in BFCache), as opposed > to (re-)load the original article page. As you can see, I was expecting the > `pageshow` event. > > Could you recommend a better way to do this? Thanks. Other code seems to use: yield BrowserTestUtils.waitForContentEvent(tab.linkedBrowser, "pageshow"); Does that work?

https://reviewboard.mozilla.org/r/46249/#review43935 How about implement a `BrowserTestUtils.browserWaitForEvent(browser, evtType)` and listens to that event explicitly in the content process?

Comment on attachment 8741172 [details] MozReview Request: Bug 1184950 - Use goBack to leave the reader view when possible, r=margaret, r=gijs Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46249/diff/4-5/

Comment on attachment 8741172 [details] MozReview Request: Bug 1184950 - Use goBack to leave the reader view when possible, r=margaret, r=gijs https://reviewboard.mozilla.org/r/46249/#review43951 r=me iff try is green with this change.

Try on top of latest m-c: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e8005022c11

Looks like the latest m-c is affected by breakage of bug 1255359. Without furthering abusing Try, let's land the patch...

Hello Everyone! I have tried a ton with the latest Beta 48.0b6 to see the fix of this bug, but for me, I am seeing the same as the Comment 0 build and the latest build when I follow the STR ( It still reloads the page, after disabling the reader moder). Can anyone help me to understand, what should I see as a fix of this bug? Thanks!

(In reply to Hossain Al Ikram [:ikram] (QA Contact) from comment #41) > Hello Everyone! > I have tried a ton with the latest Beta 48.0b6 to see the fix of this bug, > but for me, I am seeing the same as the Comment 0 build and the latest build > when I follow the STR ( It still reloads the page, after disabling the > reader moder). > > Can anyone help me to understand, what should I see as a fix of this bug? > > Thanks! Your bfcache might got evicted in some cases, when that happens, the page has to reload anyway. If you could post your OS/CPU/RAM information that would be great -- also try to reduce # of tabs opened when you try to verify this patch.

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #42) > (In reply to Hossain Al Ikram [:ikram] (QA Contact) from comment #41) > > Hello Everyone! > > I have tried a ton with the latest Beta 48.0b6 to see the fix of this bug, > > but for me, I am seeing the same as the Comment 0 build and the latest build > > when I follow the STR ( It still reloads the page, after disabling the > > reader moder). > > > > Can anyone help me to understand, what should I see as a fix of this bug? > > > > Thanks! > > Your bfcache might got evicted in some cases, when that happens, the page > has to reload anyway. If you could post your OS/CPU/RAM information that > would be great -- also try to reduce # of tabs opened when you try to verify > this patch. I have tried with new profile in every cases. I am using a Windows 7, 64 Bit edition as OS, Core i 7 4th Generation as CPU and a 32 GB Ram enabled PC.

(In reply to Hossain Al Ikram [:ikram] (QA Contact) from comment #43) > I have tried with new profile in every cases. I am using a Windows 7, 64 Bit > edition as OS, Core i 7 4th Generation as CPU and a 32 GB Ram enabled PC. You are right. This bug along does not fix comment 0. I believe bug 1269996 was regressed -- pages did not fall into bfcache at all, so it reloads. Would you mind file a new bug and file it blocks this and bug 1269996? Thanks!

>>> My Info: Win7_64, Nightly 49, 32bit, ID 20160526082509 (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #42) > Your bfcache might got evicted in some cases, when that happens, the page has to reload anyway. The last time I was reading this bug, it sounded like it's expected that some pages couldn't be displayed without reloading (when I exit reader mode). The first I noticed was "bug" with Wikipedia STR_1 (good, AR=ER): 1. Open https://www.wikipedia.org/ 2. Shift+MiddleClick English or Español link to open localized Wikipedia 3. Click reader button in urlbar. Click reader button in urlbar AR: history.go(-1) (i.e. browser navigates to original page in tab history and shows forward button) STR_2 (bad?): 1. Open https://www.wikipedia.org/ 2. Shift+MiddleClick Русский or Polski link to open localized Wikipedia 3. Click reader button in urlbar. Click reader button in urlbar AR: location.href=<new_url> (i.e. browser navigates to new page in tab history) ER: Piñatas? Nobody even mentioned any link or testcase in this bug, come on. [ad] BTW, I already filed bug 1260276 for reader mode messing with tab history

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #44) > I believe bug 1269996 was regressed -- pages did not fall into bfcache at all, so it reloads. > Would you mind file a new bug and file it blocks this and bug 1269996? According to comment 41, comment 0 wasn't fixed on Firefox 48. While bug 1269996 fixed only in 49, therefore 1269996 couldn't be the cause. Furthermore, I also remember that I tested Wikipedia case right after this bug was "fixed", and it had no effect on Russian and Polish Wikipedia. I checked this once again just now: build "2016-04-20" and build "fx-team, 759244c242e1" (first good from comment 38) have no effect on Russian and Polish Wikipedia It's expected that there're more pages like Wikipedia, but please, always provide links/testcases I haven't filed comment 45 is because it was unclear if reader mode was intended to do history.go(-1) on ALL pages, and because my bug 1260276 isn't marked "Resolved" yet (that's my strategy, yes)

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #44) > (In reply to Hossain Al Ikram [:ikram] (QA Contact) from comment #43) > > I have tried with new profile in every cases. I am using a Windows 7, 64 Bit > > edition as OS, Core i 7 4th Generation as CPU and a 32 GB Ram enabled PC. > > You are right. This bug along does not fix comment 0. I believe bug 1269996 > was regressed -- pages did not fall into bfcache at all, so it reloads. > Would you mind file a new bug and file it blocks this and bug 1269996? > Thanks! Tim, can you file this yourself, including enough detail to reliably reproduce / fix this? Thanks!as

(In reply to :Gijs Kruitbosch from comment #48) > Tim, can you file this yourself, including enough detail to reliably > reproduce / fix this? Thanks!as I am sorry, I was completely, utterly confused. Bug 1269996 *enables* the about:reader page to be bfcache-able, and this bug allows re-access of bfcache-able pages through the back button. I was testing a page with bfcache disabled [1], and with a bfcache'd page [2] I can correctly go back to the page without reload. The patch here cannot make non-bfcache'd pages re-render w/o reload. Gijs, is there a room for improvement? If so, that would be the new bug we should be filing. [1] http://www.paulgraham.com/makersschedule.html [2] http://blog.timc.idv.tw/posts/sgml-html-and-xml/