Closed
Bug 1263628
Opened 8 years ago
Closed 8 years ago
Articles from medium.com are not added to "Reading List" folder from Bookmarks
Categories
(Toolkit :: Reader Mode, defect)
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | verified |
People
(Reporter: TeoVermesan, Assigned: ahunt)
References
Details
Attachments
(2 files)
Tested using: Device: Nexus 6 (Android 6.0) Build: Firefox for Android 48.0a2 (2016-04-11) Steps to reproduce: 1. Go to medium.com 2. Choose an article and enter reader view 3. Open Menu and tap the bookmark icon 4. Go to the Bookmarks panel Actual results: - The article is only added as a bookmark Expected results: - The article should be displayed in the "Reading List" folder
Reporter | ||
Updated•8 years ago
|
Blocks: migrate-RL
Comment 1•8 years ago
|
||
There may be problems with the way Medium URLs are formatted. See also bug 1263396.
Comment 2•8 years ago
|
||
I couldn't get this one into my reading list: https://code.facebook.com/posts/998080480282805
Assignee | ||
Comment 3•8 years ago
|
||
E.g. articles on facebook.com provide a meta-refresh containing "0; URL=/foo/bar?....", and we previously attempted to use just this URL component, instead of constructing it using the current page URL. Review commit: https://reviewboard.mozilla.org/r/46145/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46145/
Attachment #8741055 -
Flags: review?(margaret.leibovic)
Attachment #8741056 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 4•8 years ago
|
||
This seems to be a common issue with medium.com articles, but affects any page which was opened using a fragment in the URL (e.g. foo.com/bar#baz is the same page as foo.com/bar, however our reader view caching code didn't recognise the former as being equivalent to the latter). I don't understand in which cases we need to handle URI conversion failing, however it seems safer to do any optimisations/removel of this cdoe in a separate followup bug, ideally with more testing. Review commit: https://reviewboard.mozilla.org/r/46147/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/46147/
Assignee | ||
Comment 5•8 years ago
|
||
We're actually failing on facebook.com URLs as well, e.g. we couldn't save the following: https://code.facebook.com/posts/998080480282805 These patches should fix both issues.
Assignee: nobody → ahunt
Status: NEW → ASSIGNED
Updated•8 years ago
|
Component: Reading List → Reader Mode
Product: Firefox for Android → Toolkit
Comment 6•8 years ago
|
||
FWIW, I'm happy to review this if that would help, Margaret/Andrzej. Likely my (Europe) tomorrow morning, but even so. :-)
Assignee | ||
Comment 7•8 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #6) > FWIW, I'm happy to review this if that would help, Margaret/Andrzej. Likely > my (Europe) tomorrow morning, but even so. :-) That would be awesome if you're more familiar with this code : )! Some notes for myself / anyone else that's interested: 1) :margaret pointed out that we're redownloading the article when saving it offline. - This isn't necessary, the article is already in memory / on-screen. - Fixing this looks complicated to me, not sure I'd get that ready in time for 48. - We always re-downloaded when saving to the reading-list in the past, whereas this would be new code. - OTOH: this was broken for some sites (i.e. the ones I'm fixing now), and saving the in-memory content would be more reliable, since we already have the article ready for showing. (Note: I briefly tested with the facebook link, and adding it to the reading list while viewing it didn't save it offline: loading the reading list item failed without connectivity. This failure just becomes much more apparent now that we link offline-state to reader-view state.) 2) Regardless of (1) we still want to make sure downloading from scratch works: - Might be needed if url-annotation sync is implemented (we'd probably want to download synced pages in the background) - Will be needed if we add the option to directly add pages to the reading-list via the link context-menu. - Is needed for the current implementation, so we need to fix it regardless (_unless_ (1) is done).
Comment 8•8 years ago
|
||
Comment on attachment 8741055 [details] MozReview Request: Bug 1263628 - meta-refresh can use a relative URL, ensure base URI is included r=gijs https://reviewboard.mozilla.org/r/46145/#review42981 r=me . Don't know what mozreview is going to do because I'm not the requested reviewer, but we'll soon find out. ::: toolkit/components/reader/ReaderMode.jsm:228 (Diff revision 1) > - errorMsg += " Refresh target URI: '" + url + "'."; > + errorMsg += " Refresh target URI: '" + newURL + "'."; > reject(errorMsg); > return; > } > // Otherwise, pass an object indicating our new URL: > - reject({newURL: url}); > + reject({newURL: newURL}); nit: you can just reject({newURL}) instead of repeating newURL (yay ES6!)
Attachment #8741055 -
Flags: review+
Updated•8 years ago
|
Blocks: CVE-2015-4508, 1222792
Comment 9•8 years ago
|
||
https://reviewboard.mozilla.org/r/46147/#review42983 ::: toolkit/components/reader/ReaderMode.jsm:241 (Diff revision 1) > - responseURL = Services.io.newURI(responseURL, null, null).spec; > - } catch (ex) { /* Ignore errors - we'll use what we had before */ } > + responseURI = Services.io.newURI(xhr.responseURL, null, null); > + responseURL = responseURI.spec; > + } catch (ex) { > + // Ignore errors - we'll use what we had before > + } > try { > - givenURL = Services.io.newURI(givenURL, null, null).spec; > + givenURI = Services.io.newURI(url, null, null); Instead of the changes to keep 2 variables per URI and so on, it seems like we could do something simpler: for both newURI calls, instead of grabbing `.spec`, grab `.specIgnoringRef`. That would mean, AIUI, a two line patch, and it would keep the code a little simpler, too. Or am I missing a reason that wouldn't work? ::: toolkit/components/reader/ReaderMode.jsm:253 (Diff revision 1) > - } catch (ex) { /* Ignore errors - we'll use what we had before */ } > + givenURL = givenURI.spec; > + } catch (ex) { > + // Ignore errors - we'll use what we had before > + } > > - if (responseURL != givenURL) { > + if (!((responseURI != null) && (givenURI != null) && responseURI.equalsExceptRef(givenURI)) && If we do need to keep this: prevailing style is to use simple falsy-checks instead of explicit comparisons with `null`: ``` if (!(responseURI && givenURI && responseURI.equalsExceptRef(givenURI) && responseURL != givenURL) ``` Additionally, because of operator precedence, you don't need braces in the simple: (a == b && c == d) case, so I've omitted the inner braces around the `responseURL != givenURL` expression as well.
Comment 10•8 years ago
|
||
(sorry for the delay, I was waylaid by some other stuff earlier today)
Comment 11•8 years ago
|
||
(In reply to Andrzej Hunt :ahunt from comment #7) > - We always re-downloaded when saving to the reading-list in the past, > whereas this would be new code. > - OTOH: this was broken for some sites (i.e. the ones I'm fixing now), and > saving the in-memory content would be more reliable, since we already have > the article ready for showing. FWIW, at least on Firefox-for-Desktop we also need this for e.g. session restore: if you open a page, enter reader mode, close the browser, reopen it, and restore your last session (automatically or manually) that tab goes back into reader mode, and we have no cache or anything for the readerized content, so we just have to go back to the server and ask.
Comment 12•8 years ago
|
||
(In reply to Andrzej Hunt :ahunt from comment #7) > - We always re-downloaded when saving to the reading-list in the past, > whereas this would be new code. This used to not be the case. Perhaps it was only the "Add to reading list" button in the reader view toolbar, but that would definitely save the article that was already in memory.
Comment 13•8 years ago
|
||
Comment on attachment 8741055 [details] MozReview Request: Bug 1263628 - meta-refresh can use a relative URL, ensure base URI is included r=gijs I'll let Gijs handle this. Thanks for your help!
Attachment #8741055 -
Flags: review?(margaret.leibovic)
Updated•8 years ago
|
Attachment #8741056 -
Flags: review?(margaret.leibovic)
Assignee | ||
Comment 14•8 years ago
|
||
https://reviewboard.mozilla.org/r/46147/#review42983 > Instead of the changes to keep 2 variables per URI and so on, it seems like we could do something simpler: for both newURI calls, instead of grabbing `.spec`, grab `.specIgnoringRef`. That would mean, AIUI, a two line patch, and it would keep the code a little simpler, too. Or am I missing a reason that wouldn't work? Ooops. The .specIgnoringRef would've worked (the issue here is we're comparing without the ref, but pass in URLs with refs)
Assignee | ||
Comment 15•8 years ago
|
||
Comment on attachment 8741055 [details] MozReview Request: Bug 1263628 - meta-refresh can use a relative URL, ensure base URI is included r=gijs Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46145/diff/1-2/
Attachment #8741055 -
Attachment description: MozReview Request: Bug 1263628 - meta-refresh can use a relative URL, ensure base URI is included r?margaret → MozReview Request: Bug 1263628 - meta-refresh can use a relative URL, ensure base URI is included r=gijs
Attachment #8741056 -
Attachment description: MozReview Request: Bug 1263628 - Ignore fragments when comparing URLs during reader view caching r?margaret → MozReview Request: Bug 1263628 - Ignore fragments when comparing URLs during reader view caching r?gijs
Attachment #8741056 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 16•8 years ago
|
||
Comment on attachment 8741056 [details] MozReview Request: Bug 1263628 - Ignore fragments when comparing URLs during reader view caching r?gijs Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46147/diff/1-2/
Comment 17•8 years ago
|
||
Comment on attachment 8741056 [details] MozReview Request: Bug 1263628 - Ignore fragments when comparing URLs during reader view caching r?gijs https://reviewboard.mozilla.org/r/46147/#review43447 r=me assuming this fixes the issue with facebook/medium (ie I've not tested this locally). :-)
Attachment #8741056 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 18•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/79ea010e10cd2c07c9a16700c85763d1346364ff Bug 1263628 - meta-refresh can use a relative URL, ensure base URI is included r=gijs https://hg.mozilla.org/integration/fx-team/rev/4992cef658a669d514b3469c4b4449ac5826359d Bug 1263628 - Ignore fragments when comparing URLs during reader view caching r=gijs
Comment 19•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/79ea010e10cd https://hg.mozilla.org/mozilla-central/rev/4992cef658a6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Reporter | ||
Comment 21•8 years ago
|
||
Articles from medium.com are added to the "Reading List" folder from Bookmarks, so: Verified as fixed using: Device: Nexus 6 (Android 6.0) Build: Firefox for Android 48.0a2 (2016-04-28)
You need to log in
before you can comment on or make changes to this bug.
Description
•