Closed
Bug 1265004
Opened 8 years ago
Closed 8 years ago
Allow saving reader-view articles from memory instead of redownloading
Categories
(Firefox for Android Graveyard :: Reading List, defect)
Firefox for Android Graveyard
Reading List
Tracking
(firefox48 fixed, firefox49 fixed, fennec48+)
RESOLVED
FIXED
Firefox 49
People
(Reporter: ahunt, Assigned: ahunt)
Details
Attachments
(1 file)
58 bytes,
text/x-review-board-request
|
Margaret
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details |
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.)
Assignee | ||
Comment 1•8 years ago
|
||
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.
Assignee: nobody → ahunt
Comment 2•8 years ago
|
||
I agree this isn't a top priority for 48, but we could look to uplift if the fix is straightforward.
Comment 3•8 years ago
|
||
(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.
tracking-fennec: --- → 48+
Assignee | ||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•8 years ago
|
||
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/
Attachment #8747981 -
Flags: review?(margaret.leibovic)
Comment 5•8 years ago
|
||
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.
Attachment #8747981 -
Flags: review?(margaret.leibovic) → review+
Assignee | ||
Comment 6•8 years ago
|
||
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.
Assignee | ||
Comment 7•8 years ago
|
||
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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2f4723efdf3a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Assignee | ||
Comment 9•8 years ago
|
||
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.
Attachment #8747981 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 10•8 years ago
|
||
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•8 years ago
|
||
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.
Attachment #8747981 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 12•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/33e0573ce75f
Updated•6 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•