Use parsed result when clicking the reader icon

RESOLVED FIXED in Firefox 16

Status

()

Firefox for Android
Reader View
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bnicholson, Assigned: bnicholson)

Tracking

unspecified
Firefox 17
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox16- fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Bug 780519 aims to fix this by storing articles in the cache. For a simpler, shorter term solution, we can attach the article to the tab, so only the most recent article is available. This makes clicking the reader mode button instant for general use (but going back/forward, restoring from OOM, etc. will require re-parsing).
(Assignee)

Comment 1

5 years ago
Created attachment 651869 [details] [diff] [review]
Use parsed result when clicking the reader icon

This patch attaches the saved article to the tab. When the reader icon or "Reading List" in the menu is clicked, we can avoid re-parsing the current page.
Attachment #651869 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 651869 [details] [diff] [review]
Use parsed result when clicking the reader icon

Review of attachment 651869 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good but I'd like it to only cover about:reader only instead of any about pages.

::: mobile/android/chrome/content/browser.js
@@ +2871,5 @@
> +          let tabURL = currentURI.specIgnoringRef;
> +          if (article == null || (article.url != tabURL)) {
> +            // Don't clear the article for about pages since we don't want to
> +            // lose it if we go to about:reader
> +            if (!currentURI.schemeIs("about"))

Any reason we should keep it if we go to, say, about:home? Feels like you should be more specific here to avoid any unexpected behaviour.

@@ +2872,5 @@
> +          if (article == null || (article.url != tabURL)) {
> +            // Don't clear the article for about pages since we don't want to
> +            // lose it if we go to about:reader
> +            if (!currentURI.schemeIs("about"))
> +              this.savedArticle = null;

nit: I'd add an empty line here to make the return more prominent.

@@ +6423,5 @@
>    },
>  
> +  getArticleForTab: function Reader_getArticleForTab(tabId, url, callback) {
> +    let tab = BrowserApp.getTabForId(tabId);
> +    let article = tab.savedArticle;

nit: add empty line here, let me breath :-)
Attachment #651869 - Flags: review?(lucasr.at.mozilla) → review-
(Assignee)

Comment 3

5 years ago
(In reply to Lucas Rocha (:lucasr) from comment #2)
> Comment on attachment 651869 [details] [diff] [review]
> Use parsed result when clicking the reader icon
> 
> Review of attachment 651869 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/chrome/content/browser.js
> @@ +2871,5 @@
> > +          let tabURL = currentURI.specIgnoringRef;
> > +          if (article == null || (article.url != tabURL)) {
> > +            // Don't clear the article for about pages since we don't want to
> > +            // lose it if we go to about:reader
> > +            if (!currentURI.schemeIs("about"))
> 
> Any reason we should keep it if we go to, say, about:home? Feels like you
> should be more specific here to avoid any unexpected behaviour.
> 

I avoided it because there's no easy way to determine which about page we're on using nsIURI, but I can just do a simple regex test. I agree it's probably better to be more explicit.
(Assignee)

Comment 4

5 years ago
Created attachment 651886 [details] [diff] [review]
Use parsed result when clicking the reader icon, v2

Addressed comments.
Attachment #651869 - Attachment is obsolete: true
Attachment #651886 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 651886 [details] [diff] [review]
Use parsed result when clicking the reader icon, v2

Review of attachment 651886 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.
Attachment #651886 - Flags: review?(lucasr.at.mozilla) → review+
tracking-firefox16: ? → -
(Assignee)

Comment 6

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/bb6adf1cdfe4
(Assignee)

Comment 7

5 years ago
Comment on attachment 651886 [details] [diff] [review]
Use parsed result when clicking the reader icon, v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: clicking reader button or "Reading List" in menu item will take longer to finish
Testing completed (on m-c, etc.): just landed m-i
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none
Attachment #651886 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
status-firefox16: --- → affected

Comment 8

5 years ago
https://hg.mozilla.org/mozilla-central/rev/bb6adf1cdfe4
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Attachment #651886 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 9

5 years ago
http://hg.mozilla.org/releases/mozilla-aurora/rev/a5a515cfad7d
status-firefox16: affected → fixed
You need to log in before you can comment on or make changes to this bug.