782421 - Use parsed result when clicking the reader icon

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[Brian Nicholson (:bnicholson)]

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).

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: 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.

Comment 2

[Lucas Rocha (:lucasr)]

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 :-)

Comment 3

[Brian Nicholson (:bnicholson)]

(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.

Comment 4

[Brian Nicholson (:bnicholson)]

Attachment: Use parsed result when clicking the reader icon, v2

Addressed comments.

Comment 5

[Lucas Rocha (:lucasr)]

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

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

Nice.

Comment 6

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/bb6adf1cdfe4

Comment 7

[Brian Nicholson (:bnicholson)]

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

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bb6adf1cdfe4

Comment 9

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/a5a515cfad7d