This is the actual reader mode implementation. It takes a url as query argument and loads the corresponding readable version of the page. Just filing this bug to be able to land the fundamental pieces of the reader mode.

Here a few observations: - The original readability.js script has been originally written to be run in a fire-and-forget manner that doesn't really fit our uses cases so I had to refactor it quite heavily (keeping the core parsing algorithm, of course). - For now, I have disabled the multi-page article parsing support as it needs extra plumbing to actually work. I have filed bug 760554 to track this. - The article cache will actually be used in my patches for bug 750702. - I added the bare minimum HTML code for the about:reader page. The actual styling and toolbar should be implemented as part of bug 750686 and bug 750681.

Comment on attachment 630611 [details] [diff] [review] Add about:reader special url for reader mode pages >diff --git a/mobile/android/chrome/content/Readability.js b/mobile/android/chrome/content/Readability.js I'm doing a simple scan of this file, mainly looking for issues about being used in an about page >+function dump(s) { >+ msg = "Reader: (Readability) " + s; >+ Cc["@mozilla.org/consoleservice;1"].getService(Ci.nsIConsoleService).logStringMessage(msg); Cu.import("resource://gre/modules/Services.jsm"); function dump(s) { msg = "Reader: (Readability) " + s; Services.console.logStringMessage(msg); >+var Readability = function(uri, doc) { >+ this._uri = uri; nit: I have been trying to use url when the object is a string and uri when the object is a nsIURI. We have not been too consistent though. If this is from the upstream code, definitely leave it as is. >diff --git a/mobile/android/chrome/content/aboutReader.html b/mobile/android/chrome/content/aboutReader.html >+ <title></title> Just a question. Do we set a title somewhere? It would be nicer than showing the URL >+</html> >\ No newline at end of file Add a newline >diff --git a/mobile/android/chrome/content/aboutReader.js b/mobile/android/chrome/content/aboutReader.js >+function dump(msg) { >+ Cc["@mozilla.org/consoleservice;1"].getService(Ci.nsIConsoleService).logStringMessage("Reader: " + msg); Services.console.logStringMessage("Reader: " + msg); >+ _loadFromURL: function Reader_loadFromURL(url) { >+ if (article) { >+ this._showContent(article); >+ } else { >+ this._showError(gStrings.GetStringFromName("aboutReader.loadError")); >+ } {} not needed for one-liners >+ _loadFromTab: function Reader_loadFromTab(tabId) { function body indent is 4 spaces instead of 2 >+ gChromeWin.Reader.parseDocumentFromTab(tabId, function(article) { >+ if (article) { >+ this._showContent(article); >+ } else { >+ this._showError(gStrings.GetStringFromName("aboutReader.loadError")); >+ } {} not needed for one-liners >+ _showError: function Reader_showError(error) { >+ document.title= error; space before '=' >+ _showContent: function Reader_showContent(article) { >+ document.title = article.title; OK, we do show the title. This should cause the URL bar to display the title >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js >+ parseDocumentFromURL: function Reader_parseDocumentFromURL(url, callback) { This function could use a few comments in it to help figure out the flow >+ if (url in this._requests) { >+ let request = this._requests[url]; >+ request.callbacks.push(callback); What exactly is _requests? It's not the cache, iiui. Reading more of the patch, I see it's a way to manage resources (notably a browser) created while loading a URL. So in this case you checking to see if we already loaded (or in process of loading) a URL. Hmm, maybe only "in process of loading" since we free the browser when we are finished loading. Do we frequently run into the situation? Where we attempt a request while a previous request is already loading? >+ getArticleFromCache: function Reader_getArticleFromCache(url, callback) { >+ this.log("Error getting article from the cache e4DB: " + url); "... cache e4DB:" -> "... cache DB" ? >+ _dowloadDocument: function Reader_downloadDocument(url, callback) { >+ // We want to parse those arbitrary pages safely, outside the privileged >+ // context of chrome. We create a hidden browser element to fetch the >+ // loaded page's document object then discard the browser element. >+ >+ let browser = document.createElement("browser"); >+ browser.setAttribute("type", "content"); >+ browser.setAttribute("collapsed", "true"); >+ >+ document.documentElement.appendChild(browser); This will start a load of "about:blank". We are kinda assuming that the load will finish before we start the loadURLWithFlags. Add a call to browser.stop(); after you append to help make sure we don't race the initial load. We do that here too: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1538 >+ browser.addEventListener("DOMContentLoaded", function (event) { >+ if (doc.location.href == "about:blank" || doc.defaultView.frameElement) { >+ callback(null); >+ return; This seems like the "error" state. Why not kill the browser here too? >diff --git a/mobile/android/themes/core/aboutReader.css b/mobile/android/themes/core/aboutReader.css >\ No newline at end of file Add newline r- just to see the cleanup and get some details

(In reply to Mark Finkle (:mfinkle) from comment #2) > Comment on attachment 630611 [details] [diff] [review] > Add about:reader special url for reader mode pages > > >diff --git a/mobile/android/chrome/content/Readability.js b/mobile/android/chrome/content/Readability.js > > I'm doing a simple scan of this file, mainly looking for issues about being > used in an about page > > >+function dump(s) { > >+ msg = "Reader: (Readability) " + s; > >+ Cc["@mozilla.org/consoleservice;1"].getService(Ci.nsIConsoleService).logStringMessage(msg); > > Cu.import("resource://gre/modules/Services.jsm"); > > function dump(s) { > msg = "Reader: (Readability) " + s; > Services.console.logStringMessage(msg); Done. > >+var Readability = function(uri, doc) { > >+ this._uri = uri; > > nit: I have been trying to use url when the object is a string and uri when > the object is a nsIURI. We have not been too consistent though. If this is > from the upstream code, definitely leave it as is. This is not from upstream. This is actually an nsIURI instance, so we're good. As I said, I had to change the original script quite heavily to fit our system as there were a bunch of assumptions on the original script that wouldn't work in our environment (use of document and window globals, retained global state after parsing, etc). > >diff --git a/mobile/android/chrome/content/aboutReader.html b/mobile/android/chrome/content/aboutReader.html > > >+ <title></title> > > Just a question. Do we set a title somewhere? It would be nicer than showing > the URL Yes, in aboutReader.js. > >+</html> > >\ No newline at end of file > > Add a newline Done. > >diff --git a/mobile/android/chrome/content/aboutReader.js b/mobile/android/chrome/content/aboutReader.js > > >+function dump(msg) { > >+ Cc["@mozilla.org/consoleservice;1"].getService(Ci.nsIConsoleService).logStringMessage("Reader: " + msg); > > Services.console.logStringMessage("Reader: " + msg); Done. > >+ _loadFromURL: function Reader_loadFromURL(url) { > > >+ if (article) { > >+ this._showContent(article); > >+ } else { > >+ this._showError(gStrings.GetStringFromName("aboutReader.loadError")); > >+ } > > {} not needed for one-liners Done. > >+ _loadFromTab: function Reader_loadFromTab(tabId) { > > function body indent is 4 spaces instead of 2 Fixed. > >+ gChromeWin.Reader.parseDocumentFromTab(tabId, function(article) { > >+ if (article) { > >+ this._showContent(article); > >+ } else { > >+ this._showError(gStrings.GetStringFromName("aboutReader.loadError")); > >+ } > > {} not needed for one-liners Fixed. > >+ _showError: function Reader_showError(error) { > > >+ document.title= error; > > space before '=' Fixed. > >+ _showContent: function Reader_showContent(article) { > > >+ document.title = article.title; > > OK, we do show the title. This should cause the URL bar to display the title Yep. Still wondering about what to do with the favicon in the reader. Need to check with UX team. > >diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js > > >+ parseDocumentFromURL: function Reader_parseDocumentFromURL(url, callback) { > > This function could use a few comments in it to help figure out the flow Added. > >+ if (url in this._requests) { > >+ let request = this._requests[url]; > >+ request.callbacks.push(callback); > > What exactly is _requests? It's not the cache, iiui. Reading more of the > patch, I see it's a way to manage resources (notably a browser) created > while loading a URL. So in this case you checking to see if we already > loaded (or in process of loading) a URL. Hmm, maybe only "in process of > loading" since we free the browser when we are finished loading. _requests tracks all on-going requests in the reader. The entries in _requests are removed once they are finished, see _runCallbacksAndFinish(). The code above is just a little optimization for the case where the same URL is requested while there's already a request for it. In that case, we simply append one more callback and bail. > Do we frequently run into the situation? Where we attempt a request while a > previous request is already loading? I'm just making it future-proof really, it's an unlikely situation. > >+ getArticleFromCache: function Reader_getArticleFromCache(url, callback) { > > >+ this.log("Error getting article from the cache e4DB: " + url); > > "... cache e4DB:" -> "... cache DB" ? Yep, fixed. > >+ _dowloadDocument: function Reader_downloadDocument(url, callback) { > >+ // We want to parse those arbitrary pages safely, outside the privileged > >+ // context of chrome. We create a hidden browser element to fetch the > >+ // loaded page's document object then discard the browser element. > >+ > >+ let browser = document.createElement("browser"); > >+ browser.setAttribute("type", "content"); > >+ browser.setAttribute("collapsed", "true"); > >+ > >+ document.documentElement.appendChild(browser); > > This will start a load of "about:blank". We are kinda assuming that the load > will finish before we start the loadURLWithFlags. > Add a call to browser.stop(); after you append to help make sure we don't > race the initial load. We do that here too: > http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/ > browser.js#1538 Ok, done. > >+ browser.addEventListener("DOMContentLoaded", function (event) { > > >+ if (doc.location.href == "about:blank" || doc.defaultView.frameElement) { > >+ callback(null); > >+ return; > > This seems like the "error" state. Why not kill the browser here too? Good catch, done. > >diff --git a/mobile/android/themes/core/aboutReader.css b/mobile/android/themes/core/aboutReader.css > > >\ No newline at end of file > > Add newline Done.

Pushed: http://hg.mozilla.org/integration/mozilla-inbound/rev/8060c5ce574d

Comment on attachment 631336 [details] [diff] [review] Add about:reader special url for reader mode pages Reader Mode is tentatively planned for Firefox 15. It's still disabled and invisible to users. Need to keep landing it in aurora and central.

As with bug 750698, it's not clear that uplifting Reader Mode has been requested by the product team.

s/uplifting/enabling/

(In reply to Alex Keybl [:akeybl] from comment #7) > As with bug 750698, it's not clear that uplifting Reader Mode has been > requested by the product team. It is on our roadmap for Fx15. Karen is taking ownership of the roadmap and should chime in.

Small nit: aboutReader.loading should have ellipsis and not three dots, is this correct for Android, Axel? (I can open a follow-up bug if so)

(In reply to Guillermo López (:willyaranda) from comment #11) > Small nit: aboutReader.loading should have ellipsis and not three dots, is > this correct for Android, Axel? > > (I can open a follow-up bug if so) Guillermo, feel free to file a follow-up but this UI and strings are probably going to change once we implement the real reader mode UI in bug 750681, bug 750686, and more. This is just a placeholder while I wait for the UI specs.

Ok, so just keeping those bug in my radar for now. Thanks!

Comment on attachment 631336 [details] [diff] [review] Add about:reader special url for reader mode pages [Triage Comment] Reader mode is not slated for FF15 in the current draft of the product roadmap.

Verified fixed on: -build: Firefox for Android 22.0a1 (2013-02-25) -device: Samsung Galaxy Nexus -OS: Android 4.2.2