Last Comment Bug 782421 - Use parsed result when clicking the reader icon
: Use parsed result when clicking the reader icon
Status: RESOLVED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: Reader View (show other bugs)
: unspecified
: ARM Android
: -- normal (vote)
: Firefox 17
Assigned To: Brian Nicholson (:bnicholson)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-13 14:28 PDT by Brian Nicholson (:bnicholson)
Modified: 2012-08-23 11:40 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
fixed


Attachments
Use parsed result when clicking the reader icon (6.64 KB, patch)
2012-08-14 13:49 PDT, Brian Nicholson (:bnicholson)
lucasr.at.mozilla: review-
Details | Diff | Splinter Review
Use parsed result when clicking the reader icon, v2 (6.52 KB, patch)
2012-08-14 14:19 PDT, Brian Nicholson (:bnicholson)
lucasr.at.mozilla: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Brian Nicholson (:bnicholson) 2012-08-13 14:28:40 PDT
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) 2012-08-14 13:49:04 PDT
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.
Comment 2 Lucas Rocha (:lucasr) 2012-08-14 14:05:31 PDT
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) 2012-08-14 14:08:06 PDT
(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) 2012-08-14 14:19:03 PDT
Created attachment 651886 [details] [diff] [review]
Use parsed result when clicking the reader icon, v2

Addressed comments.
Comment 5 Lucas Rocha (:lucasr) 2012-08-14 14:22:09 PDT
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) 2012-08-20 10:25:15 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/bb6adf1cdfe4
Comment 7 Brian Nicholson (:bnicholson) 2012-08-20 10:26:41 PDT
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] 2012-08-21 06:31:40 PDT
https://hg.mozilla.org/mozilla-central/rev/bb6adf1cdfe4
Comment 9 Brian Nicholson (:bnicholson) 2012-08-23 11:40:48 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/a5a515cfad7d

Note You need to log in before you can comment on or make changes to this bug.