Closed Bug 1130206 Opened 9 years ago Closed 9 years ago

Clicking reader mode button fetches article using XHR, instead of using stored article

Categories

(Firefox :: General, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 39
Tracking Status
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(1 file, 1 obsolete file)

I thought this was just an issue with e10s, but I see this happening in non-e10s tabs as well. Something is going wrong with our logic to fetch the article stored with the <browser>.

I'm not sure if this is happening in Fennec as well. It's hard to notice this without logging/debugging, although it does make the reader mode article noticeably slower to appear.
Assignee: nobody → margaret.leibovic
/r/4121 - Bug 1130206 - Don't blow away saved reader mode article until another non-about:reader page is shown. r=bnicholson

Pull down this commit:

hg pull review -r 83243fba4520cff5a480ae44fc9cc8fa83e0ea1a
Attachment #8567402 - Flags: review?(bnicholson)
I found the problem here is that we're blowing away the saved article on pagehide.

On Android, we don't have this issue because we get rid of the saved article on pageshow:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/content.js?force=1#65

The reason this logic isn't the same on desktop is that on desktop, we need to hide the reader mode button immediately on pagehide, since it isn't replaced by the stop button the way it is on desktop.

To be consistent, we should probably consider just hiding the pageaction in a pagehide handler on mobile as well, although that's not strictly necessary.
Note: This still doesn't work with e10s enabled, since about:reader is still loaded in the parent process (bug 1129106), so we end up blowing away our content.js context when we replace the <browser> to load about:reader.

However, e10s support is not necessary for Fx38, and hopefully this will just be resolved as part of bug 1129106.
Confirmed this also fixes my <br> example bug  :-D
Comment on attachment 8567402 [details]
MozReview Request: bz://1130206/margaret

https://reviewboard.mozilla.org/r/4119/#review3321
Attachment #8567402 - Flags: review?(bnicholson) → review+
[Tracking Requested - why for this release]: This bug affects reader view, which we're trying to ship in 38.

https://hg.mozilla.org/integration/fx-team/rev/4036e2e8e152
https://hg.mozilla.org/mozilla-central/rev/4036e2e8e152
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Attachment #8567402 - Attachment is obsolete: true
Attachment #8619375 - Flags: review+
You need to log in before you can comment on or make changes to this bug.