Support adding content from URLs that redirect to the reader mode cache

RESOLVED FIXED in Firefox 37

Status

()

Firefox for Android
Reader View
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Margaret, Assigned: Margaret)

Tracking

35 Branch
Firefox 37
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

4 years ago
To clarify, this bug is about updating _downloadAndParseDocument [1] to handling downloading any URL that redirects any number of ways. Currently we haven't really found this to be much of a problem in practice because we only ever go through _downloadAndParseDocument as a fallback if a tab with an article has never been loaded. The only way to do this in the UI is to add an article from the share overlay, then try to load it from your reading list. However, that's a way we want to start supporting with bug 1093172.

Currently, we use a browser to download this document, but we should also investigate whether or not we can just do with with an XHR. I made a patch with this change in bug 793920, but we decided to split that off, since it felt like scope creep.

I need to sit down to do some more investigation about how we're failing, but there are problems with our current approach, as well as the XHR approach.

My insidious redirect testcase is the pocket hits feed, which has URLs that do two different kinds of redirects before resolving to the final URL.

http://pocket.co/sBYec
http://getpocket.com/s/BYec
http://well.blogs.nytimes.com/2014/12/03/run-to-stay-young

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/Reader.js#200
Blocks: 917884
(Assignee)

Updated

4 years ago
Blocks: 1113454
(Assignee)

Comment 2

3 years ago
Created attachment 8542659 [details]
MozReview Request: bz://1107588/margaret
Attachment #8542659 - Flags: review?(rnewman)
Attachment #8542659 - Flags: review?(mark.finkle)
(Assignee)

Comment 3

3 years ago
/r/1809 - Bug 1107588 - Use an xhr to download reader mode content instead of creating new browser elements

Pull down this commit:

hg pull review -r 9a78e3c7f8aa2bd4dd182d3ec29091157b6a0136
(Assignee)

Comment 4

3 years ago
I can find no reason why an xhr causes a regression as opposed to using a <browser>. I very much prefer the xhr approach because it is simpler, and we can do it in a window-less jsm context, which will help to share this code with desktop.

In this patch, I added a bit of a hacky approach to deal with meta refresh redirects, but we don't even support those properly with the current <browser> approach.

I tested with these URLs, let me know if you can think of other redirect testcases:

http://pocket.co/snupv
http://getpocket.com/s/nupv
https://t.co/wV1ymMoEB1
Attachment #8542659 - Flags: review?(mark.finkle) → review+
(Assignee)

Updated

3 years ago
Attachment #8542659 - Flags: review?(rnewman)
https://hg.mozilla.org/mozilla-central/rev/8a9910d1105b
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
(Assignee)

Comment 8

3 years ago
Comment on attachment 8542659 [details]
MozReview Request: bz://1107588/margaret
Attachment #8542659 - Attachment is obsolete: true
Attachment #8618792 - Flags: review+
(Assignee)

Comment 9

3 years ago
Created attachment 8618792 [details]
MozReview Request: Bug 1107588 - Use an xhr to download reader mode content instead of creating new browser elements
You need to log in before you can comment on or make changes to this bug.