1265004 - Allow saving reader-view articles from memory instead of redownloading

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reading List, defect)

Description

[Andrzej Hunt :ahunt]

When caching reader view pages (i.e. when bookmarking about:reader pages) we redownload the article.

The article is/was already in memory. We should just store that.

This used to happen in the past, but was removed as a side-effect of Bug 1150174:
https://hg.mozilla.org/mozilla-central/rev/8ce3581b7247

I've not dug into the changes there in detail yet, but we can probably avoid the performance problems that solved by keeping a reference to the article _only_ when entering reader-mode, and trying to retrieve that in _getArticle if available. (Before that bug it looks like we'd always parse the reader-mode version of an article even when viewing a page in normal mode - which we can avoid with this solution.)

Comment 1

[Andrzej Hunt :ahunt]

This would be nice to have, but probably isn't a priority for 48, given that we already had this behaviour for ~1 year?

One huge advantage is that bugs when downloading from scratch (e.g. Bug 1263628) wouldn't matter since parsing an in-memory website is much more reliable.

Comment 2

[:Margaret Leibovic]

I agree this isn't a top priority for 48, but we could look to uplift if the fix is straightforward.

Comment 3

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #2)
> I agree this isn't a top priority for 48, but we could look to uplift if the
> fix is straightforward.

I've changed my mind, I think this is actually important for 48. We want to guarantee that these articles are available offline.

Comment 4

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1265004 - Try to save in-memory reader view article instead of downloading r?margaret

This should guarantee that any reader view page will also be successfully saved offline.

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

Comment 5

[:Margaret Leibovic]

Comment on attachment 8747981 [details]
MozReview Request: Bug 1265004 - Try to save in-memory reader view article instead of downloading r?margaret

https://reviewboard.mozilla.org/r/50097/#review47067

This seems fine, but I definitely think we should have a follow-up patch to remove the unused code. I also think you should consider my comment about splitting up the `_getArticle` method.

::: mobile/android/base/java/org/mozilla/gecko/reader/ReadingListHelper.java:102
(Diff revision 1)
>          }
>  
>          SavedReaderViewHelper rch = SavedReaderViewHelper.getSavedReaderViewHelper(context);
>  
>          if (!rch.isURLCached(url)) {
> -            GeckoAppShell.notifyObservers("Reader:AddToCache", url);
> +            JSONObject object = new JSONObject();

Nit: final.

::: mobile/android/chrome/content/Reader.js:76
(Diff revision 1)
>    observe: function Reader_observe(aMessage, aTopic, aData) {
>      switch (aTopic) {
>        case "Reader:FetchContent": {
>          let data = JSON.parse(aData);
>          this._fetchContent(data.url, data.id);
>          break;

This got me looking into this code more closely, and there's some unused stuff in here that we can clean up, like this unused "Reader:FetchContent" method and its corresponding logic.

::: mobile/android/chrome/content/Reader.js:88
(Diff revision 1)
>  
>        case "Reader:AddToCache": {
> +        let data = JSON.parse(aData);
> +        let tab = BrowserApp.getTabForId(data.tabID);
> +        if (!tab) {
> +          throw new Error("No tab for tabID = " + tabID);

I would expand this message to say this is about trying to add a page to the reader view cache.

::: mobile/android/chrome/content/Reader.js:92
(Diff revision 1)
> +        if (!tab) {
> +          throw new Error("No tab for tabID = " + tabID);
> +        }
> +
>          // If the article is coming from reader mode, we must have fetched it already.
> -        this._getArticle(aData).then((article) => {
> +        this._getArticle(data.url, tab.browser).then((article) => {

If we expect there to be a browser, and for it to already be in reader view, do we need to call this `_getArticle` method, or should we create a new method explictly to get the article that's already loaded in the browser? I feel like we should just use `_getArticleData` directly here, and then reserve `_getArticle` for when we're actually loading the reader view page.

Comment 6

[Andrzej Hunt :ahunt]

https://reviewboard.mozilla.org/r/50097/#review47067

> This got me looking into this code more closely, and there's some unused stuff in here that we can clean up, like this unused "Reader:FetchContent" method and its corresponding logic.

I've filed this as Bug 1270198. I think we want to uplift this change so it's probably simpler to keep the cleanup separate.

Comment 7

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/2f4723efdf3aa222b2cdcd76daf17b5816c2f406
Bug 1265004 - Try to save in-memory reader view article instead of downloading r=margaret

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2f4723efdf3a

Comment 9

[Andrzej Hunt :ahunt]

Comment on attachment 8747981 [details]
MozReview Request: Bug 1265004 - Try to save in-memory reader view article instead of downloading r?margaret

Approval Request Comment
[Feature/regressing bug #]: Bug 1150174
[User impact if declined]: Saving a reader view article redownloads article, meaning articles can't be saved if device is offline. By redownloading it's possible that a different version of the page will be saved offline than is currently visible on screen.
[Describe test coverage new/current, TreeHerder]: Manual testing, including on nightly.
[Risks and why]: Low risk: reuses codepath that existed in the past, we simply parse in-memory data for saving offline articles (instead of downloading them again).
[String/UUID change made/needed]: none.

Comment 10

[Andrzej Hunt :ahunt]

Note: uplifting this is mainly relevant for 48 (aurora) since that's where the reading-list migration landed, and where we will be promoting saving reader view articles offline more heavily.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8747981 [details]
MozReview Request: Bug 1265004 - Try to save in-memory reader view article instead of downloading r?margaret

Is it possible to include tests for this?

Taking it because it is a better user experience and we will have time to fix potential regressions.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/33e0573ce75f