927124 - Implement Readability for Metro Firefox

Status: RESOLVED WONTFIX

(Tracking Graveyard :: Metro Operations, enhancement, P1)

Description

[Eklavya Mirani]

Meta Bug for implementing Readability for Metro Firefox

Comment 1

[Eklavya Mirani]

Attachment: [WIP]Reader mode for Metro Firefox

Changes:
*Added Readability Code
*Reader Mode toggle Icon comes up in UrlBar only if a page is readability supported
*Added About:Reader pages
*[WIP]Added Reader Toolbar
*Light and Dark Skins are working
*Font Size adjustments Added to Reader Toolbar

I am also trying to move the Reader code completely out of browser.js, and add it to reader.js
However moving "ReaderMode" definition out breaks the code. It loads fine if the definition consists of debug log statements, but breaks with the actual implementation. (Will fix it in the next patch. :) )

Comment 2

[Eklavya Mirani]

Attachment: Reader Mode Screenshots.zip

Comment 3

[Matt Brubeck (:mbrubeck)]

Comment on attachment 817470 [details] [diff] [review]
[WIP]Reader mode for Metro Firefox

Nice!  There's a lot of code here so it will take me a while to give a proper review, but I'll try to post a response in the next day or two.

Comment 4

[Eklavya Mirani]

Attachment: Screenshot (11).png

Here's how the Reader toolbar looks like. What else do we need here? Margin?

Comment 5

[Matt Brubeck (:mbrubeck)]

Comment on attachment 817470 [details] [diff] [review]
[WIP]Reader mode for Metro Firefox

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

This looks good overall.  I noted some issues below about the way we associate events/articles with tabs when there are multiple tabs open.

However, it's hard to fully review this patch because I can't see at a glance which code is new or changed, and which is copied from the Android implementation.  For code that's identical (or could be identical) to what we use in Android, we should "hg mv" the code into /toolkit and update the Android front-end to load the code from toolkit.

(As a temporary step, we could "hg cp" the code from /mobile/android to /toolkit and later write a separate patch to remove the /mobile/android copy.)

For UI code that's similar but not identical, it could also be useful to "hg cp" the Android code and then apply the Metro-specific changes.

Could you produce a patch with the same code you have here, but using the hg cp/mv commands as described above?  That should produce a more readable change history.  Let me know if you'd like me to help or do some of the work to create the patch.

::: browser/metro/base/content/browser.js
@@ +1408,4 @@
>    }
>  };
>  
> +let savedArticle = null;

We shouldn't use a global variable here.  Instead we should store a saved article for each tab, as we do on Android.

@@ +1420,5 @@
> +    return Browser.selectedTab;
> +  },
> +  
> +  get selectedReaderBrowser () {
> +    return Browser.selectedBrowser;

I don't think these aliases are necessary; we can just use Browser.selectedBrowser directly when needed.

@@ +1456,5 @@
> +  },
> +  
> +  PageShowHandler: function() {
> +    ReaderMode.checkReaderMode();
> +    ReaderMode.readerButton.hidden = (savedArticle == null);

savedArticle might not be updated at this point, since the callback in checkReaderMode can happen asynchronously.  You should update the button inside that callback, instead.

@@ +1457,5 @@
> +  
> +  PageShowHandler: function() {
> +    ReaderMode.checkReaderMode();
> +    ReaderMode.readerButton.hidden = (savedArticle == null);
> +    ReaderMode.handleDomContentLoaded({target : Browser.selectedBrowser});

The target here is not necessarily the selected browser.

@@ +1467,5 @@
> +  },
> +  
> +  handleDomContentLoaded: function(event) {
> +    try{
> +      ReaderMode.Log(ReaderMode.selectedReaderBrowser.currentURI.specIgnoringRef);

This should check which browser actually generated the event, rather than assuming that it is the selected browser.  (The selected tab might have changed since the event listener was added.)

::: browser/metro/base/content/reader.js
@@ +1,1 @@
> +let Reader = {

It looks like this file is mostly independent of the browser UI, so with some minor changes we could move it to /toolkit and share it with Firefox for desktop (and Android).

@@ +66,5 @@
> +  parseDocumentFromTab: function(tabId, callback) {
> +    try {
> +      this.log("parseDocumentFromTab: " + tabId);
> +  
> +      let url = ReaderMode.selectedReaderBrowser.contentWindow.location.href;

The "tabId" argument is never used.  Perhaps we should just pass the browser into this function instead, and then it will not need to access ReaderMode.selectedReaderBrowser.

::: browser/metro/base/jar.mn
@@ +85,5 @@
>    content/AnimatedZoom.js                      (content/AnimatedZoom.js)
>    content/dbg-metro-actors.js                  (content/dbg-metro-actors.js)
> +  content/aboutReader.js                       (content/aboutReader.js)
> +  content/reader.js                            (content/reader.js)
> +  content/readerMode.js                        (content/readerMode.js)

It looks like "readerMode.js" is not included in this patch and can be removed here.

Comment 6

[Allison Naaktgeboren :ally]

:D

Comment 7

[Eklavya Mirani]

Thanks for the comments Matt. Will do :)

Comment 8

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

Comment on attachment 818098 [details]
Screenshot (11).png

Hi Eklavya,
I cancelled the ui-review because I was not clear about the UI and interactions for readability before.

I just attached some mockups for you. Please take a look and I am happy to discuss the details with you.

Comment 9

[Yuan Wang(:Yuan) – Mobile Firefox Design Lead]

Attachment: ReaderMode_101813.pdf

Reader Mode for v1 mockups, reviewed with Firefox UX team today.

Comment 10

[Eklavya Mirani]

Attachment: [WIP]Moved reader files to toolkit/components/reader , minor UI fixes

Copied files to toolkit/components/reader
Minor Changes in the UI
Removed Reader List and Share code
Removed Auto skin for Reader Mode
Pressing "Esc" button now exits reader mode. (goes back to previous page)

To Do:
Add a close button to the right of the screen. (see UI mockup)
instead of using selectedBrowser/getBrowser() as event targets, use the tab as event target
UI Fix, hiding the reader toolbar does not close the dropdown.

Suffering from exams, I'll start working on it again next week.

Comment 11

[Eklavya Mirani]

Attachment: [WIP]Moved reader files to toolkit/components/reader , minor UI fixes

Fixed the dropdown not closing, mentioned in the previous comment.

Is there someway I could edit my attachments?

Comment 12

[Matt Brubeck (:mbrubeck)]

Thanks again -- as before, it might take a handful of days for me to finish reviewing this.

(In reply to eklavyamirani from comment #11)
> Is there someway I could edit my attachments?

Uploading a new attachment and obsoleting the old one (just as you did) is the correct way to do this.

Comment 13

[Matt Brubeck (:mbrubeck)]

Comment on attachment 824644 [details] [diff] [review]
[WIP]Moved reader files to toolkit/components/reader , minor UI fixes

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

Here's a partial review.  I still need to review the JS files in toolkit/components/reader.

Also requesting feedback from lucasr, since we'll need to think about how and when to re-unify this with the original Android code.

In terms of landing strategy, I'd like to land this in a way that is easily disabled with a pref or a build flag, to minimize the risk of adding new bugs that could delay the first release of Metro Firefox.  If it's not ready to be enabled for the first release, we can enable it right afterward.  (We can also do things like keep it enabled on Nightly for testing/development but disabled on other channels until it's fully tested.)

::: browser/metro/base/content/browser.js
@@ +94,5 @@
>      PopupBlockerObserver.init();
>      APZCObserver.init();
>  
> +    // Init the Reader Mode
> +	  try{

Something's up with the indentation here.  In general, for code in /browser/metro, please use 2-space indents, a single space after keywords (like "try" and "if"), and no trailing whitespace.  Don't worry too much about this, though -- I can always clean up mechanical stuff like this when landing the patch.  At this stage it's just about making the patch easier to read and review.

@@ +1606,5 @@
> +      let i = 0;
> +      if(aEvent.target == content.document){
> +        ReaderMode.PageShowHandler({target : browser});
> +        // To Check how many times this event fires and ReaderMode.PageShowHandler is called
> +        Services.console.logStringMessage("Page Shown " + ++i + "times. \n");

We'll want to remove development logging stuff like this before landing, of course.

::: toolkit/components/reader/Makefile.in
@@ +9,5 @@
> +
> +include $(DEPTH)/config/autoconf.mk
> +
> +
> +include $(topsrcdir)/config/rules.mk
\ No newline at end of file

I think you can remove this Makefile.in since it defines no rules of its own.  (We used to require a Makefile.in for every directory in the build, but this changed recently.)

::: toolkit/components/reader/content/aboutReader.css
@@ +324,4 @@
>  }
>  
>  .toolbar {
> +  font-family: "Droid Sans", helvetica, arial, clean, sans-serif;

Maybe we should add "Segoe UI" to the front of the list here, for Metro devices.

We might want to split this into two stylesheets: A base stylesheet to be shared across apps/platforms, and a second one that each app can override.  (You don't need to do that here; we can do that as a follow-up.)

@@ +329,5 @@
>    transition-duration: 0.7s;
>    visibility: visible;
>    opacity: 1.0;
>    position: fixed;
> +  width: 4%;

We should probably use an absolute length here -- don't want this button becoming tiny on a narrow screen.

::: toolkit/components/reader/content/aboutReader.js
@@ +186,3 @@
>          break;
> +      case "keydown":
> +        if (aEvent.keyCode == 27)

Please use "Ci.nsIDOMKeyEvent.DOM_VK_ESCAPE" instead of "27".

Comment 14

[Lucas Rocha (:lucasr)]

Comment on attachment 824644 [details] [diff] [review]
[WIP]Moved reader files to toolkit/components/reader , minor UI fixes

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

Had a quick look. It seems to me that putting the Reader front-end bits (aboutReader.*) in a shared space doesn't make much sense to me. I'd try to avoid making the UI for Android and Metro depend on each other. Unless this space is for sharing code between Firefox desktop and Metro?

What we do want to share though is the Readability.js/readerWorker.js/JSDOMParser.js/reader.js code. It's not clear from the patch if you're making any changes to these files. I can change Android's reader to use the shared components as soon as this patch lands.

::: toolkit/components/reader/content/aboutReader.js
@@ -353,4 @@
>      Services.prefs.setIntPref("reader.font_size", this._fontSize);
>    },
>  
> -  _handleDeviceLight: function Reader_handleDeviceLight(newLux) {

Isn't the light sensor API available on Window 8?

Comment 15

[Matt Brubeck (:mbrubeck)]

Comment on attachment 824644 [details] [diff] [review]
[WIP]Moved reader files to toolkit/components/reader , minor UI fixes

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

This is looking great!  Thanks again for your patience with the long review delays.

Let me know if you have any questions, or would like any extra help completing this feature.  Once my own review comments are addressed, I'll pass this to someone else to review the toolkit parts.  Then we can file some follow-up bugs for any additional work, like unifying this with Android, and adding a pref to enable/disable the feature.  You are welcome to work on some or all of those follow-ups, and some of us from the Firefox team will be happy to work on them too.

::: browser/metro/base/content/browser.js
@@ +1604,5 @@
>        }
>        self._eventDeferred = null;
> +      let i = 0;
> +      if(aEvent.target == content.document){
> +        ReaderMode.PageShowHandler({target : browser});

"content" doesn't seem to be defined here.  I think you need something like in mobile/android/chrome/content/browser.js:

  if (aEvent.originalTarget.defaultView == this.browser.contentWindow) {

::: toolkit/components/reader/content/aboutReader.css
@@ +324,1 @@
>  }

Lucas is probably right that aboutReader.css and aboutReader.html should go into /browser/metro.  I hoped that we could share these between platforms with just some minor overrides, but the visual design turned out to be pretty different so it's probably best to just have a copy for each app.

::: toolkit/components/reader/reader.js
@@ +1,1 @@
> +let Reader = {

Please add a license header to the top of this file. You can copy the header from https://www.mozilla.org/MPL/headers/

Also, I'd like to enable strict mode in all new JavaScript files.  You can do this by adding this line right after the licence header:

"use strict";

It would be nice to make this file into a JavaScript Module (.jsm) file, so that it has its own scope and can't accidentally depend on any outside global variables:
https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Using

@@ +339,5 @@
> +      
> +  	// Throws a silent error, and does not call the onerror callback. request object itself becomes null.
> +    let request = null; //window.indexedDB.open("about:reader", this.DB_VERSION);
> +      
> +  	if (!request){

nit: fix indentation

::: toolkit/components/reader/readerMode.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

Since this file code directly accesses the Metro UI, it should stay in /browser/metro.  (Sorry if my previous review was confusing about what should go in toolkit and what should go in browser.)

Strict mode would be nice here too.

@@ +43,5 @@
> +  
> +  PageShowHandler: function(aEvent) {
> +    ReaderMode.checkReaderMode(aEvent.target);
> +    ReaderMode.Log("aEvent " + aEvent.target.currentURI.specIgnoringRef);
> +    ReaderMode.handleDomContentLoaded(aEvent);

The Android code that handleDomContentLoaded is based on runs only after the "DOMContentLoaded" event, and it has some logic to check that the document has a "body" property.  Do we need to do the same here?

::: toolkit/locales/jar.mn
@@ +14,4 @@
>    locale/@AB_CD@/global/aboutSupport.properties         (%chrome/global/aboutSupport.properties)
>    locale/@AB_CD@/global/aboutTelemetry.dtd              (%chrome/global/aboutTelemetry.dtd)
>    locale/@AB_CD@/global/aboutTelemetry.properties       (%chrome/global/aboutTelemetry.properties)
> +  locale/@AB_CD@/global/aboutReader.properties          (%chrome/global/aboutReader.properties)

I don't see this file in the patch.  Did you remember to "hg add" it?

If we move aboutReader.js to /browser/metro then the .properties file should move there too.

Comment 16

[Eklavya Mirani]

Hi Matt! Thanks for reviewing. Had a busy week at school, but I should fix these by tomorrow.
Should the next patch be merged with this one(huge patch!! again :P), or should I add a new patch addressing these fixes?

(In reply to Lucas Rocha (:lucasr) from comment #14)
> Comment on attachment 824644 [details] [diff] [review]
> [WIP]Moved reader files to toolkit/components/reader , minor UI fixes

> What we do want to share though is the
> Readability.js/readerWorker.js/JSDOMParser.js/reader.js code. It's not clear
> from the patch if you're making any changes to these files. I can change

No changes in these files. reader.js is a new file. But the other 3 were just copied as is from the android part.
 
> Isn't the light sensor API available on Window 8?

I wasn't sure about this, but since the mockup didn't have it for the V1 release, I removed it. Should I add it back?

Comment 17

[Eklavya Mirani]

Are we planning to share the code with desktop firefox here? 
I did go through the old implementation for the desktop firefox, and the code here requires negligible changes to get it running on desktop firefox. 

If this is the case, should the aboutReader.* files be shared with the desktop counter part and hence stay in the toolkit(as Lucas mentioned)?

Comment 18

[Matt Brubeck (:mbrubeck)]

I'd like to share as much code as possible with desktop Firefox, but I'd also like to make it easy for desktop and Metro to use different visual/UI designs, and to easily make UI changes in one or the other without necessarily changing both.  So I think we should put as much non-UI code as we can into a shared JS "library" in toolkit, but the CSS and HTML (and any JS that directly interfaces with the HTML) should be in the application directories instead.  And it's okay if some of that UI code is duplicated.

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to eklavyamirani from comment #17)
> Are we planning to share the code with desktop firefox here? 
> I did go through the old implementation for the desktop firefox, and the
> code here requires negligible changes to get it running on desktop firefox. 
> 
> If this is the case, should the aboutReader.* files be shared with the
> desktop counter part and hence stay in the toolkit(as Lucas mentioned)?

Yes, we should try to share between Metro and desktop Firefox. The aboutReader.* files won't be shared between Desktop and Metro, but as Matt said the aboutReader.* files should be placed in /browser/metro. I hope that is makes sense, please let me know if it is still not clear.

Comment 20

[Eklavya Mirani]

Attachment: Addressing previous feedback

In Continuation with Attachment 824644 [details] [diff]

Changes:
Moved aboutReader.* files to browser/metro
AboutReader object is now created if the page url starts with about:reader when the pageshow event fires
Reader toolbar button's size is in absolute values. It does not change when the browser is resized.
Added "Segoe UI" as the default font type for reader toolbar UI.
Enabled strict mode for readerMode.js and reader.js

Comment 21

[Lucas Rocha (:lucasr)]

Comment on attachment 8334818 [details] [diff] [review]
Addressing previous feedback

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

All this code seems very Metro-specific. mbrubeck is the more appropriate reviewer here. Let me know if you'd like me to have a look at something specific in this patch.

Comment 22

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8334818 [details] [diff] [review]
Addressing previous feedback

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

This is looking good to me.  Could you fix a few minor issues below, fold the two patches together (see the "hg qfold" command if you are using mercurial queues), update the combined patch to apply against the latest mozilla-central repo, and post the resulting patch below?  It would also be great if you could get rid of trailing whitespace in the lines you've changed or added.  I can help with that if you like.

I also filed bug 941850 to temporarily hide this feature behind a preference.  (This is because we're currently stabilizing Metro Firefox for its first release, and we don't want to add any features until that release is finished.  We can aim at enabling the feature by default in Nightly 29, in about 2.5 weeks.)  Would you like to work on that bug, or shall I?

::: browser/metro/base/content/aboutReader.js
@@ +61,4 @@
>    ];
>  
>    let colorScheme = Services.prefs.getCharPref("reader.color_scheme");
> +  this._colorScheme = colorScheme;

Why is this needed - doesn't _setColorSchemePref do the right thing?

Same question about the _fontType and _fontSize lines below.

::: browser/metro/base/content/browser.js
@@ +1598,2 @@
>      function onPageShowEvent(aEvent) {
>        browser.removeEventListener("pageshow", onPageShowEvent);

Please get delete the "removeEventListener" line above.  It was already failing because it was missing "true" for the third argument.  Instead of fixing that, we should just remove the line, because we now want this listener to run every time.

::: browser/metro/base/content/reader.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

This file should stay in toolkit, since it doesn't depend on the application UI.

::: toolkit/components/reader/moz.build
@@ +11,1 @@
>  EXTRA_JS_MODULES += [

Note: To build these patches with the current code on mozilla-central, you'll need to remove the LIBRARY_NAME and MODULE variables from this file.  (This is because of recent changes to our build system.)

Comment 23

[Eklavya Mirani]

(In reply to Matt Brubeck (:mbrubeck) from comment #22)
> is finished.  We can aim at enabling the feature by default in Nightly 29,
> in about 2.5 weeks.)  Would you like to work on that bug, or shall I?

I'd love to work on it!
 
> ::: browser/metro/base/content/aboutReader.js
> @@ +61,4 @@
> >    ];
> >  
> >    let colorScheme = Services.prefs.getCharPref("reader.color_scheme");
> > +  this._colorScheme = colorScheme;
> 
> Why is this needed - doesn't _setColorSchemePref do the right thing?
> 
> Same question about the _fontType and _fontSize lines below.

Strict mode warnings. Would setting them to null be the right way to avoid warnings about using undefined variables?

Comment 24

[Matt Brubeck (:mbrubeck)]

(In reply to eklavyamirani from comment #23)
> Strict mode warnings. Would setting them to null be the right way to avoid
> warnings about using undefined variables?

Yes -- you could set "this._colorScheme = null;" in the AboutReader constructor, or add "_colorScheme: null," to the prototype (and the same for the other properties).

Comment 25

[Eklavya Mirani]

Attachment: Combined the last two patches. Addressed minor bugs.

Updated the code to work with the latest mozilla-central repository and combined the patches into one.

Comment 26

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8339135 [details] [diff] [review]
Combined the last two patches. Addressed minor bugs.

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

r=mbrubeck for the Metro parts, modulo some whitespace cleanup that I can do when landing.

Adding Unfocused to review the toolkit parts.  For reference, most of this code is copied from this Android code, and we'll be doing follow-up work shortly  to make Android use the Toolkit version:
http://hg.mozilla.org/mozilla-central/file/6ecf0c4dfcbe/mobile/android/chrome/content/browser.js#l7317

::: toolkit/components/reader/jar.mn
@@ +3,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +toolkit.jar:
> +   skin/classic/aero/global/reader/reader-style-icon.png      (images/reader-style-icon.png)
> +   skin/classic/aero/global/reader/reader-dropdown-arrow.png  (images/reader-dropdown-arrow.png)
\ No newline at end of file

Sorry I missed this before, but these should be moved to browser/metro/theme/images and the links to them should be adjusted accordingly.

(No need to upload a new patch to fix this yet; let's wait until any other reviewers have chimed in.)

Comment 27

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8339135 [details] [diff] [review]
Combined the last two patches. Addressed minor bugs.

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

(Only looking at the toolkit parts of the patch.)


Good followup bugs:
* Convert logging to use Log.jsm
* Avoid usage of [].forEach, in favour of the more performant for-of loops
* Adopt usage of arrow functions where appropriate (amongst other benefits, arrow functions benefits makes working with callbacks nicer, and lets you avoid bind)

General comments:
* All the JS files should be using strict-mode. This can help catch a few difficult and subtle classes of bugs, as well as help with performance.
* reader.js should really be Reader.jsm (ie, a JS module)

::: toolkit/components/reader/reader.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

A lot of trailing white space in this file.

@@ +6,5 @@
> + 
> +let Reader = {
> +  // Version of the cache database schema
> +  DB_VERSION: 1,    
> +  DEBUG: 1,  

This should be disabled by default.

@@ +9,5 @@
> +  DB_VERSION: 1,    
> +  DEBUG: 1,  
> +  // Don't try to parse the page if it has too many elements (for memory and
> +  // performance reasons)
> +  // Perhaps we can add a preference here?

This is a new todo comment - if you're adding these, prefix them with XXX or TODO, to make them stand out (and be searchable) from documentation comments.

@@ +25,5 @@
> +      let request = this._requests[url];
> +      request.callbacks.push(callback);
> +      return;
> +    }  
> +    let request = { url: url, callbacks: [callback] };

Ni: Restore the deleted blank line before this line.

@@ +34,5 @@
> +         
> +      // First, try to find a cached parsed article in the DB
> +      this.getArticleFromCache(url, function(article) {
> +        if (article) {
> +          //this.log("Page found in cache, return article immediately");

Why is this commented out? (There's a few cases of log calls being commented out here)

@@ +46,5 @@
> +      }
> +               
> +      // Article hasn't been found in the cache DB, we need to
> +      // download the page and parse the article out of it.
> +        this._downloadAndParseDocument(url, request);

Bad indentation of above comment.

@@ +48,5 @@
> +      // Article hasn't been found in the cache DB, we need to
> +      // download the page and parse the article out of it.
> +        this._downloadAndParseDocument(url, request);
> +      }.bind(this));
> +    }catch (e){

The original spacing in these catch statements is what matches the prevailing JS code style:
  } catch (e) {

@@ +54,5 @@
> +      this._runCallbacksAndFinish(request, null);
> +    }
> +  },
> +      
> +  getArticleForTab: function Reader_getArticleForTab(tabId, url, callback) {

Toolkit code doesn't (and can't) have a concept of tabs, because every application implements it differently (or may not even have tabs).

So for any API originally accepting a tab (or tab ID), it'll need to start accepting a document instead (with an appropriate rename of the method).

@@ +57,5 @@
> +      
> +  getArticleForTab: function Reader_getArticleForTab(tabId, url, callback) {
> +    var article;
> +    try {
> +      article = savedArticle;

This is broken. Seems savedArticle was originally a property of the tab object. Since this code can no longer know about tabs, if this API is still useful then it'll need to use a different caching method for saved articles.

@@ +68,5 @@
> +      this.parseDocumentFromURL(url, callback);
> +    }
> +  },
> +      
> +  parseDocumentFromTab: function(aDoc, callback) {

Will need to rename this method, for example.

@@ +226,5 @@
> +        
> +      // Append URL to the article data. specIgnoringRef will ignore any hash
> +      // in the URL.
> +      if (article)
> +        article.url = uri.specIgnoringRef;

Should still be sanitising article.title
ie, http://hg.mozilla.org/mozilla-central/diff/09334ec10eba/mobile/android/chrome/content/browser.js

@@ +343,5 @@
> +      return;
> +    }
> +      
> +    // Throws a silent error, and does not call the onerror callback. request object itself becomes null.
> +    let request = null; //window.indexedDB.open("about:reader", this.DB_VERSION);

We should either find a solution to this, or decide if it's safe to fix in a followup.

Comment 28

[Eklavya Mirani]

Attachment: Moved Reader images to browser/metro/theme, converted reader.js to Reader.jsm and addressed feedback

Moved images to browser/metro/
Converted reader.js to Reader.jsm
Addressed :Unfocused's feedback
qfolded the code for the pref to disable the reader mode
fixed the bug causing a page in someother tab to change the behaviour of reader icon in the current tab (To reproduce/verify, load a page that supports readability in one tab, and then load another page that does not support readability in another tab, and switch back to the previous tab once the current page loads)

Comment 29

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8346064 [details] [diff] [review]
Moved Reader images to browser/metro/theme, converted reader.js to Reader.jsm and addressed feedback

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

r=mbrubeck for the /browser/metro parts.  A few minor observations below.

I haven't looked in detail at the toolkit parts yet, but I'll do that too as soon as I can.

::: browser/metro/base/content/browser.js
@@ +94,5 @@
> +      ReaderMode.init();
> +      Services.console.logStringMessage("Metro Reader: ReaderMode Init Success");
> +    } catch (e){
> +      Services.console.logStringMessage("Metro Reader: ReaderMode Error: " + e);
> +    }

Please remove the success logging before landing this patch.  (The error logging can stay.)

::: toolkit/components/reader/Reader.jsm
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +var EXPORTED_SYMBOLS = ["Reader"];
> +
> +"use strict";

Note: This has to be before the "var EXPORTED SYMBOLS..." line above.

Comment 31

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8346064 [details] [diff] [review]
Moved Reader images to browser/metro/theme, converted reader.js to Reader.jsm and addressed feedback

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

Followup bugs I'd like to see (ie, not a requirement for this landing):
* Setting the favicon for the article
* Supporting a reading list in the toolkit implementation
* Convert logging to use Log.jsm
* Convert Android's to use Toolkit's implementation. Anything not used on Metro/desktop yet (eg, using a luminosity sensor) can be controlled by a preference set by the application. 
* Modernise some of the JS inherited from Android. eg, move away from [].forEach(), use arrow-functions where appropriate, etc
* Anything else marked via a TODO comment

::: browser/metro/base/content/aboutReader.js
@@ +2,2 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */

This file feels like an odd-duck. On one hand, it's something that would normally be loaded by aboutReader.js itself. On the other hand, it's used as a module and loaded by the chrome window. Except... it's not actually a module. 

On Android it's loaded as a subscript into it's own scope. I guess that has the benefit of not having the memory overhead of a module, and there's only one window there (ditto with Metro). Not so with any other application (such as Desktop Firefox) though - and as seen in your patch in bug 558882, it's easy to mis-use this file (where you're *not* loading it into it's own scope).

Given it's usage, that it'll be in toolkit (see my comments in bug 558882), and that ideally any application should need minimal amount of boilerplate code to hook up reader mode, I'd rather just have this loaded directly by aboutReader.html (which has chrome privileges anyway).

Is there anything that *won't* work if we did this?

@@ -656,5 @@
>      this._headerElement.style.display = "block";
>  
> -    let parserUtils = Cc["@mozilla.org/parserutils;1"].getService(Ci.nsIParserUtils);
> -    let contentFragment = parserUtils.parseFragment(article.content, Ci.nsIParserUtils.SanitizerDropForms,
> -                                                    false, articleUri, this._contentElement);

Hm, why'd you remove this?

::: browser/metro/base/content/browser-ui.js
@@ +5,4 @@
>  "use strict";
>  
>  Cu.import("resource://gre/modules/devtools/dbg-server.jsm")
> +Cu.import("resource://gre/modules/Reader.jsm");

Don't need to import this here - it's already imported in browser.js

::: toolkit/components/reader/Reader.jsm
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Can you name this ReaderMode.jsm, and the exported variable ReaderMode? The reason I suggest that is, when you look at importing this module from resource://gre/modules/, Reader.jsm is very ambiguous.

@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +var EXPORTED_SYMBOLS = ["Reader"];

In JSM files, EXPORTED_SYNMBOLS and any symbols we want to export need to be explicitly set on the global object (ie, |this|). This is due to the way JSMs are handled on FirefoxOS - I know this JSM isn't use there at the moment, but we need to be consistent.

So this line should be:

this.EXPORTED_SYMBOLS = ...

Ditto with the ReaderMode (Reader) variable.

@@ +6,5 @@
> +
> +"use strict";
> +
> +Components.utils.import("resource://gre/modules/Services.jsm")
> +let Ci = Components.interfaces, Cc = Components.classes;

Include 'Cu' here too, and flip these lines around. Also, Ci, Cc, and Cu should be const's.

@@ +12,5 @@
> +let Reader = {
> +
> +  _DEBUG: 0,
> +
> +  _requests: {},

Since this is holding data, with arbitrary keys, it should be a Map.

@@ +20,5 @@
> +  // TODO: Perhaps we can add a preference here?
> +  MAX_ELEMS_TO_PARSE: 3000,
> +
> +  init: function Reader_init() {
> +    this._log("init()");

Do you imagine having some immediate need for an init() method? If not, this should be removed so this whole file can be lazy-loaded.

@@ +23,5 @@
> +  init: function Reader_init() {
> +    this._log("init()");
> +  },
> +
> +  parseDocumentFromURL: function Reader_parseDocumentFromURL(url, document, callback) {

Naming the functions for these methods is inconsistent - sometimes they're named, sometimes they're not. We're don't really need them named these days, as the JS engine will infer named based on the property name. So may we well remove all names, and make it all consistent.

@@ +28,5 @@
> +    // If there's an on-going request for the same URL, simply append one
> +    // more callback to it to be called when the request is done.
> +    if (url in this._requests) {
> +      let request = this._requests[url];
> +      request.callbacks.push(callback);

If you make request.callbacks a Set, you won't need to worry about the possibility of callback already being in request.callbacks.

@@ +46,5 @@
> +      this._runCallbacksAndFinish(request, null);
> +    }
> +  },
> +
> +  parseDocument: function(aDoc, callback) {

Inconsistent parameter naming: s/aDoc/doc/

@@ +50,5 @@
> +  parseDocument: function(aDoc, callback) {
> +    try {
> +
> +      let url = aDoc.contentWindow.location.href;
> +      let uri = Services.io.newURI(url, null, null);

To get the URI of the document, you can just use: doc.documentURIObject

@@ +151,5 @@
> +    var self = this;
> +    request.callbacks.forEach( function(callback) { callback(result) } );
> +  },
> +
> +  _downloadDocument: function Reader_downloadDocument(url, document, callback) {

It just occurred to me that using a <browser> is left-over from before Readability was changed to run in a Worker. We don't need/want the overhead or complexity of that any more, since the document is just re-serialized again. So this should be changed to download the document via a XMLHttpRequest instead. The JSDOMParser has a strict requirement that the markup be valid, so you'll need to set responseType="document" to still have this return a document that can be re-serialized, but it'll cut down a lot of overhead (and code). 

And, it'll also mean you can remove the 'document' parameter from this method and parseDocumentFromURL. Ideally, parseDocumentFromURL shouldn't have a chrome document passed to it anyway, nor should we be operating on one for any of this (from a toolkit perspective), at least not without monitoring if the document if the document's window goes away at any stage. Normally in these cases (and if we couldn't use a XMLHttpRequest), we're use the application's hidden window - but Metro doesn't implement that :\

Comment 32

[Brian Nicholson (:bnicholson)]

(In reply to Blair McBride [:Unfocused] from comment #31)
> Given it's usage, that it'll be in toolkit (see my comments in bug 558882),
> and that ideally any application should need minimal amount of boilerplate
> code to hook up reader mode, I'd rather just have this loaded directly by
> aboutReader.html (which has chrome privileges anyway).

To prevent XSS attacks, aboutReader.html should *not* have chrome privileges -- see bug 778582. aboutReader.js is used to add cherry-picked privileged functionality to aboutReader.html. This allows us to inject innerHTML without worrying about potential exploits.

Comment 33

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Blair McBride [:Unfocused] from comment #31)
> ::: toolkit/components/reader/Reader.jsm
> @@ +1,1 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> 
> Can you name this ReaderMode.jsm, and the exported variable ReaderMode? The
> reason I suggest that is, when you look at importing this module from
> resource://gre/modules/, Reader.jsm is very ambiguous.

Which means it'd be best if we found another name for browser/metro/base/content/readerMode.js - that's the application integration of toolkit's ReaderMode, so maybe ReaderModeIntregration.js?

Comment 34

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Brian Nicholson (:bnicholson) from comment #32)
> (In reply to Blair McBride [:Unfocused] from comment #31)
> > Given it's usage, that it'll be in toolkit (see my comments in bug 558882),
> > and that ideally any application should need minimal amount of boilerplate
> > code to hook up reader mode, I'd rather just have this loaded directly by
> > aboutReader.html (which has chrome privileges anyway).
> 
> To prevent XSS attacks, aboutReader.html should *not* have chrome privileges
> -- see bug 778582. aboutReader.js is used to add cherry-picked privileged
> functionality to aboutReader.html. This allows us to inject innerHTML
> without worrying about potential exploits.

Oh, indeed. I'd wondered about that, but then I had thought I saw Android having it as privileged - but nope, it's explicitly unprivileged.

So:
* aboutReader.js should become ReaderModeContent.jsm
* The about redirector needs to change to be *unprivileged*

Comment 35

[Jim Mathies [:jimm]]

Comment on attachment 8346064 [details] [diff] [review]
Moved Reader images to browser/metro/theme, converted reader.js to Reader.jsm and addressed feedback

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

Curious, will this work with remote tabs, or will we need to make changes when we turn e10s on?

::: browser/metro/base/content/browser.js
@@ +1322,5 @@
>        }
>        self._eventDeferred = null;
> +
> +      if (self._isReaderEnabled) {
> +        if (self.active && !self._isReaderActive && aEvent.originalTarget.defaultView == browser.contentWindow) {

nit - please keep line lengths in here to ~80 chars or so. It's a loose rule. We've been cleaning this up as we go through the old fennec front end code.

Comment 36

[Jim Mathies [:jimm]]

(In reply to Blair McBride [:Unfocused] from comment #31)
> And, it'll also mean you can remove the 'document' parameter from this
> method and parseDocumentFromURL. Ideally, parseDocumentFromURL shouldn't
> have a chrome document passed to it anyway, nor should we be operating on
> one for any of this (from a toolkit perspective), at least not without
> monitoring if the document if the document's window goes away at any stage.
> Normally in these cases (and if we couldn't use a XMLHttpRequest), we're use
> the application's hidden window - but Metro doesn't implement that :\

We support hidden windows, but they don't have a physical desktop windows associated with them -

http://mxr.mozilla.org/mozilla-central/source/widget/windows/winrt/MetroWidget.cpp#235

Comment 37

[Eklavya Mirani]

Thanks for the review, Blair, I am fixing it all up.

(In reply to Jim Mathies [:jimm] from comment #35) 
> Curious, will this work with remote tabs, or will we need to make changes
> when we turn e10s on?

I am not sure, but I will let you know as soon as I have that figured out.

Comment 38

[Eklavya Mirani]

(In reply to Blair McBride [:Unfocused] from comment #31)
> Comment on attachment 8346064 [details] [diff] [review]
> @@ -656,5 @@
> >      this._headerElement.style.display = "block";
> >  
> > -    let parserUtils = Cc["@mozilla.org/parserutils;1"].getService(Ci.nsIParserUtils);
> > -    let contentFragment = parserUtils.parseFragment(article.content, Ci.nsIParserUtils.SanitizerDropForms,
> > -                                                    false, articleUri, this._contentElement);
> 
> Hm, why'd you remove this?

I was assuming it does the same thing as changing the innerHTML of the contentElement. But I just read why we'd use parserUtils.parseFragment() instead of directly assigning the innerHTML.

"The nsIParserUtils.parseFragment() method will convert a string to a document fragment while removing any scripts or other unsafe content in the process."
(https://developer.mozilla.org/en-US/Add-ons/Overlay_Extensions/XUL_School/DOM_Building_and_HTML_Insertion)

Comment 39

[Eklavya Mirani]

(In reply to Jim Mathies [:jimm] from comment #35)
> Curious, will this work with remote tabs, or will we need to make changes
> when we turn e10s on?

No, I don't think it will work. We are accessing DOM objects in the chrome process, which isn't possible with e10.
(http://timtaubert.de/blog/2011/08/firefox-electrolysis-101/)

Comment 40

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Eklavya from comment #39)
> (In reply to Jim Mathies [:jimm] from comment #35)
> > Curious, will this work with remote tabs, or will we need to make changes
> > when we turn e10s on?
> 
> No, I don't think it will work. We are accessing DOM objects in the chrome
> process, which isn't possible with e10.
> (http://timtaubert.de/blog/2011/08/firefox-electrolysis-101/)

Oh, I meant to mention that. I'm not too worried about it - I don't think it will be too much work. We're only touching the DOM once when we finally put all the finished pieces into the page (and it's only three items - article content, article title, site favicon), so it should be relatively easy.

Working with e10s can take some getting used to - we can talk about it on IRC/email if you'd like.

Comment 41

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Blair McBride [:Unfocused] from comment #40)
> (In reply to Eklavya from comment #39)
> Oh, I meant to mention that. I'm not too worried about it - I don't think it
> will be too much work. We're only touching the DOM once when we finally put
> all the finished pieces into the page (and it's only three items - article
> content, article title, site favicon), so it should be relatively easy.

Er, to clarify - I meant aside from ReaderModeContent.jsm (aboutReader.js). Which I think should mostly just run without modification when imported into a content process's frame script (/browser/base/content/content.js on Desktop Firefox, don't think Metro has an equivalent frame script for miscellaneous things), with the required code to hook it up.

And, in theory, once that's all hooked up the same setup should auto-magically work for non-e10s too.

Comment 42

[Jim Mathies [:jimm]]

(In reply to Blair McBride [:Unfocused] from comment #41)
> (In reply to Blair McBride [:Unfocused] from comment #40)
> > (In reply to Eklavya from comment #39)
> > Oh, I meant to mention that. I'm not too worried about it - I don't think it
> > will be too much work. We're only touching the DOM once when we finally put
> > all the finished pieces into the page (and it's only three items - article
> > content, article title, site favicon), so it should be relatively easy.
> 
> Er, to clarify - I meant aside from ReaderModeContent.jsm (aboutReader.js).
> Which I think should mostly just run without modification when imported into
> a content process's frame script (/browser/base/content/content.js on
> Desktop Firefox, don't think Metro has an equivalent frame script for
> miscellaneous things), with the required code to hook it up.
> 
> And, in theory, once that's all hooked up the same setup should
> auto-magically work for non-e10s too.

We have /browser/metro/base/content/contenthandlers/Content.js, although we generally like to put large js objects in their own files to cleanliness. I'll file a follow up and attach it to our e10s bug.

Comment 43

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Eklavya: Sorry I missed you on IRC. To answer your question about which bug to use for moving aboutReader.* to toolkit - my vote would be for a separate bug (or at least, not this bug). It doesn't really matter in terms of this bug or getting review (though it will be needed for bug 55888), I just think a separate bug would make it easier for you (and me as reviewer), and lets this bug land a bit sooner.

Comment 44

[Eklavya Mirani]

Attachment: Addressed toolkit feedback

* Implemented Reader Mode for Metro Firefox
* ReaderMode.jsm now uses XMLHttpRequest to download pages instead of creating a <browser> element and then using it
* Renamed aboutReader.js to ReaderModeContent.jsm
* Moved ReaderModeContent.jsm from /browser/metro/base/content/ to /toolkit/components/reader/
* Renamed readerMode.js to readerModeIntegration.js
* Renamed /toolkit/components/reader/Reader.jsm to ReaderMode.jsm

Comment 45

[Jim Mathies [:jimm]]

moving [story] bugs over to tracking product.

Comment 48

[Eklavya Mirani]

Based on the updated reader mode spec, the toolbar contains Reader list related buttons too. Hide them for the time being, or shall I start implementing it?

Comment 49

[Allison Naaktgeboren :ally]

Eklavya, I might hide them for now and do them in a followup bug. I think I'd really like to see a bare bones reader mode land at this point. 

:mbrubeck, what are your thoughts?

Comment 50

[Eklavya Mirani]

Attachment: Reader Mode Current Implementation.png

Maybe I should move the reader style button to the bottom while we are hiding the other buttons?
if this looks fine, I'll put the patch up for review. :)

Comment 51

[Michael Maslaney [:mmaslaney] ([e-mail])]

Comment on attachment 8398914 [details]
Reader Mode Current Implementation.png

Because we have just one function (at the moments), I would move the type control button to the bottom (same margins as close).

The type seems a little small, please double check the CSS in the spec. Also, the type controls background seems a little dark than the one I had in the spec.

Comment 52

[Allison Naaktgeboren :ally]

I support mmaslaney's feedback.

Comment 53

[Allison Naaktgeboren :ally]

Comment on attachment 8398914 [details]
Reader Mode Current Implementation.png

Almost there! Thanks for keeping at this

Comment 54

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8398914 [details]
Reader Mode Current Implementation.png

Deferring to mmaslaney.

Also, sad news that you may have already heard; we have decided not to ship the Metro app as part of our official Firefox releases.  But we can still accept patches if you want to keep working on the Metro feature (and of course we still want this feature for desktop Firefox too).

Comment 55

[Eklavya Mirani]

Yeah, I'll keep working on the Metro feature too. I should be able to get back to the code at the end of this week, I have some college work that's eating away all my time at the moment.

Comment 56

[Eklavya Mirani]

Attachment: Reader Mode 3-6-14.png

How about now?

Comment 57

[Michael Maslaney [:mmaslaney] ([e-mail])]

Looking good, Eklavya. I would make sure you're using the Fira "Book" weight for body copy and the "Bold" weight for headers. Also, the author's name is looking a bit small and the rule under the URL is spaced out a little to far. Please refer to the spec for exact type specs: http://people.mozilla.org/~mmaslaney/readermode/type_light.html

Comment 58

[Michael Maslaney [:mmaslaney] ([e-mail])]

Comment on attachment 8433183 [details]
Reader Mode 3-6-14.png

Looking good, Eklavya. I would make sure you're using the Fira "Book" weight for body copy and the "Bold" weight for headers. Also, the author's name is looking a bit small and the rule under the URL is spaced out a little to far. Please refer to the spec for exact type specs: http://people.mozilla.org/~mmaslaney/readermode/type_light.html

The button should have a pressed state. I would recommend using the same background color of the type menu.

Comment 59

[Bryan Clark (DevTools PM) [:clarkbw]]

I don't think there's any more work happening here.  I'm going to close this out to match the rest of the metro bugs.  Feel free to reopen if this is a mistake.