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)

48 Branch
ARM
Android
defect
Not set
normal

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
Blocks: migrate-RL
There may be problems with the way Medium URLs are formatted. See also bug 1263396.
I couldn't get this one into my reading list:
https://code.facebook.com/posts/998080480282805
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)
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/
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
Component: Reading List → Reader Mode
Product: Firefox for Android → Toolkit
FWIW, I'm happy to review this if that would help, Margaret/Andrzej. Likely my (Europe) tomorrow morning, but even so. :-)
(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 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+
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.
(sorry for the delay, I was waylaid by some other stuff earlier today)
(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.
(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 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)
Attachment #8741056 - Flags: review?(margaret.leibovic)
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)
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)
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 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+
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
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
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.