Closed Bug 558882 (desktop-reader) Opened 14 years ago Closed 8 years ago

[meta] Ship Readability in the desktop browser (reader mode)

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: Dolske, Unassigned)

References

(Depends on 10 open bugs, Blocks 1 open bug, )

Details

(Keywords: meta)

Attachments

(2 files, 11 obsolete files)

1.14 MB, image/png
Details
1.15 MB, image/png
Details
Scene: A website appears. It's typography and styling is terrible.

<limi> We should just ship Readability in the browser
<dolske> We do have View -> Page Style...

This would be neat. Limi also points out that on mobile, a clean text-only view of the page would be compelling too.

Not sure what the licensing is on Readability, but it probably won't be hard to cleanbox something similar to it, if needed.
Assignee: nobody → dolske
My wife *loves* Readability. At first she thought it was a new Firefox feature, and I was sad to have to tell her it wasn't.
Assignee: dolske → nobody
Depends on: 792286
Depends on: 793917
Depends on: 793919
Depends on: 793920
Attached patch Reader Mode works! (obsolete) — Splinter Review
Spacing is off with aboutreader.js and a few other files.  Files will be moved around in the next patch as we have put them in directories that are not accessible by the mobile version.  Formatting still needs to be applied to the UI.  Hiding the button is only partially working right now.
Attachment #676183 - Flags: review?(lucasr.at.mozilla)
Attachment #676183 - Flags: review?(jaws)
Comment on attachment 676183 [details] [diff] [review]
Reader Mode works!

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

I only looked at browser.js, since most of the other code is either trivial or copied from mobile. The copied files will need to live in Toolkit so they can be shared by mobile and desktop though.

::: browser/base/content/browser.js
@@ +47,5 @@
>  
> + [
> +   ["AboutReader", "chrome://browser/content/aboutReader.js"],
> + ].forEach(function (aScript) {
> +   let [name, script] = aScript;

This is a little confusing to me. I see what it is doing, but I hope there is a simpler way of doing the same thing. Nesting these arrays when there is only element of the nested array seems overly complicated.

@@ +242,5 @@
>      if (event.persisted)
>        gPluginHandler.reshowClickToPlayNotification();
>    }
> +  
> +  Reader.parseDocumentFromTab(gBrowser.contentWindow.location.id, function (article) {

There isn't an |id| property on window's location object. This part needs rethinking.

@@ +249,5 @@
> +        let tabURL = gBrowser.currentURI.specIgnoringRef;
> +        if (article == null || (article.url != tabURL)) {
> +          // Don't clear the article for about:reader pages since we want to
> +          // use the article from the previous page
> +          if (!/^about:reader/i.test(tabURL))

This can be made simpler by using:
if (!tabURL.startsWith("about:reader"))

@@ +261,5 @@
> + 				if (savedArticle != null) {
> + 				      document.getElementById("reader-button").hidden = false;//!gPrefService.getBoolPref("reader.enabled");
> + 				}
> + 				else {
> + 				  document.getElementById("reader-button").hidden = true;//!gPrefService.getBoolPref("reader.enabled");

You should add a getter for "reader-button" so that the getElementById code doesn't need to be duplicated everywhere.

@@ +5633,4 @@
>      gPageStyleMenu.disableStyle();
>  }
>  
> +var newTabBrowser = null;

This variable shouldn't be necessary.

@@ +7710,5 @@
>  }
> +
> +var savedArticle = null;
> +
> +let Reader = {

All of this looks to be copied. We shouldn't have this duplication. Can you get started with moving mobile's files to toolkit?
Attachment #676183 - Flags: review?(jaws)
(In reply to Jared Wein [:jaws] from comment #4)
> > + [
> > +   ["AboutReader", "chrome://browser/content/aboutReader.js"],
> > + ].forEach(function (aScript) {
> > +   let [name, script] = aScript;
> 
> This is a little confusing to me. [...]

I agree. A nice way to write this is:

  {
    let name = "AboutReader", script = "chrome://browser/content/aboutReader.js";
    ...
  }

If it later needs to be turned into an array, that is easily done:

  for (let [name, script] of [
    ["AboutReader", "chrome://browser/content/aboutReader.js"]
  ]) {
    ...
  }
Attached patch Reader mode, with toolbar (obsolete) — Splinter Review
We will be adding the changes that were suggested above in our next patch.  The images and css files still need to be added to winstripe and gnomestripe.  We will also be working on the functionality of the buttons on the toolbar in reader mode (share, reading list, etc.).  We may also try to implement an overlay, however if that doesn't work, we will switch the current functionality to load reader mode in the same tab as the original article.  We also need some help with getting the common files between mobile and desktop set up properly in toolkit. (creating a makefile.in is what we are stuck on right now.)
Attachment #676183 - Attachment is obsolete: true
Attachment #676183 - Flags: review?(lucasr.at.mozilla)
Attachment #680492 - Flags: review?(lucasr.at.mozilla)
Attachment #680492 - Flags: review?(jaws)
Comment on attachment 680492 [details] [diff] [review]
Reader mode, with toolbar

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

::: browser/app/profile/firefox.js
@@ +24,5 @@
> +pref("reader.margin_size", 5);
> +pref("reader.font_size", 12);
> +pref("reader.color_scheme", "light");
> +pref("reader.has_used_toolbar", false);
> +pref("reader.enabled", false);

These prefs should be moved to the bottom of this file. There isn't a technical reason, but that is where most new preferences go :)

::: browser/base/content/aboutReader.html
@@ +1,1 @@
> +<!DOCTYPE html>

This needs a license header, similar to http://mxr.mozilla.org/mozilla-central/source/browser/base/content/abouthome/aboutHome.xhtml

::: browser/base/content/aboutReader.js
@@ +22,5 @@
> +
> +let gStrings = Services.strings.createBundle("chrome://browser/locale/aboutReader.properties");
> +
> +let AboutReader = function(doc, win) {
> +  // dump("Init()");

You'll need to remove this and the other commented out code.

@@ +42,5 @@
> +   this._contentElementRef = Cu.getWeakReference(doc.getElementById("reader-content"));
> +   this._toolbarElementRef = Cu.getWeakReference(doc.getElementById("reader-toolbar"));
> + 
> +   this._toolbarEnabled = true;
> + 

nitpick: leading and trailing whitespace here and other places.

::: browser/base/content/browser.xul
@@ +612,4 @@
>                     class="urlbar-icon"
>                     hidden="true"
>                     onclick="SocialShareButton.onClick(event);"/>
> +						<image id="reader-button"

nitpick: replace these tab characters with spaces.

::: browser/base/jar.mn
@@ +56,5 @@
>          content/browser/pageinfo/permissions.js       (content/pageinfo/permissions.js)
>          content/browser/pageinfo/security.js          (content/pageinfo/security.js)
> +        content/browser/Readability.js                (content/Readability.js)
> +        content/browser/JSDOMParser.js                (content/JSDOMParser.js)
> +        content/browser/readerWorker.js               (content/readerWorker.js)

These three files are mentioned in this jar.mn file, but I don't see them as part of this patch.

::: browser/themes/gnomestripe/browser.css
@@ +1427,5 @@
>  }
>  
> +/* Reader Button */
> +#reader-button {
> +  padding: 1px;

No need for the padding and the /* Reader Button */ comment can be removed.

::: mobile/android/themes/core/aboutReader.css
@@ +416,2 @@
>    bottom: -18px;
> +  background-image: url('chrome://browser/skin/places/reader-dropdown-arrow-hdpi.png');

I'm not sure why the styling of the mobile reader mode had to change, especially changes from mdpi to hdpi images. Can this be un-done?
Attachment #680492 - Flags: review?(jaws)
We cleaned up the code (hopefully we got all the misc. whitespace and extra files out of there).  Still working on make .jsm files in toolkit/components/reader, so that both mobile and desktop can use those files.  We also attempted to change the "lazily loading arrays" code to the mentioned above changes, however we could not get it to work.  We will work on getting this fixed before we submit another patch.    We will also be removing the tags for the buttons on the toolbar within reader mode (share, add to reading list, view reading list).  We were getting some strange errors when we viewed the page with them removed.  We will do our best to have those removed before our next submitted patch.
Attachment #680492 - Attachment is obsolete: true
Attachment #680492 - Flags: review?(lucasr.at.mozilla)
Attachment #682930 - Flags: review?(lucasr.at.mozilla)
Attachment #682930 - Flags: review?(jaws)
Please give us feedback on pieces of code that need to change.  We are still attempting to do the "lazily load"without arrays.  We also tried converting JSDOMParser.js and Readability.js to .jsm, however we were unsuccessful.  These two files are only used in readerWorker.js, which we successfully converted to a .jsm.  Please see notes on previous upload for more changes that were made recently.
Attachment #682930 - Attachment is obsolete: true
Attachment #682930 - Flags: review?(lucasr.at.mozilla)
Attachment #682930 - Flags: review?(jaws)
Attachment #683389 - Flags: review?(lucasr.at.mozilla)
Attachment #683389 - Flags: review?(jaws)
Comment on attachment 683389 [details] [diff] [review]
Cleaned up a couple more things and moved common files to toolkit/components/reader/

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

I'm only reviewing the copied files from mobile here. Needs a new version with all the suggested cleanups. But looks good overall!

::: browser/base/content/aboutReader.html
@@ +1,4 @@
> +<!-- 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/. -->
> +   

Remove trailing spaces here.

@@ +23,5 @@
> +  <div id="reader-content" class="content">
> +  </div>
> +
> +  <ul id="reader-toolbar" class="toolbar">
> +    <li><a id="share-button" class="button share-button" href="#"></a></li>

Remove this as it's not really used on desktop Firefox.

@@ +34,5 @@
> +        <hr></hr>
> +        <div id="margin-size-control" class="step-control"></div>
> +      </li>
> +    </ul>
> +    <li><a id="list-button" class="button list-button" href="#"></a></li>

Same here.

@@ +35,5 @@
> +        <div id="margin-size-control" class="step-control"></div>
> +      </li>
> +    </ul>
> +    <li><a id="list-button" class="button list-button" href="#"></a></li>
> +    <li><a id="toggle-button" class="button toggle-button" href="#"></a></li>

And here.

::: browser/base/content/aboutReader.js
@@ +42,5 @@
> +
> +  this._scrollOffset = win.pageYOffset;
> +
> +  let body = doc.body;
> +  body.addEventListener("touchstart", this, false);

You'll have to replace the touch-based logic with mouse-based events here.

@@ +46,5 @@
> +  body.addEventListener("touchstart", this, false);
> +  body.addEventListener("click", this, false);
> +
> +  win.addEventListener("scroll", this, false);
> +  win.addEventListener("popstate", this, false);

The popstate is used here to handle Android's system "Back" button press as a way to dismiss popups inside the reader UI. This is not necessary on desktop. Replace this logic by simply handling Escape key to dismiss popups.

@@ +50,5 @@
> +  win.addEventListener("popstate", this, false);
> +  win.addEventListener("resize", this, false);
> +
> +  this._setupAllDropdowns();
> +  this._setupButton("toggle-button", this._onReaderToggle.bind(this));

Remove all the code related to reading list toggling for now.

@@ +51,5 @@
> +  win.addEventListener("resize", this, false);
> +
> +  this._setupAllDropdowns();
> +  this._setupButton("toggle-button", this._onReaderToggle.bind(this));
> +  this._setupButton("list-button", this._onList.bind(this));

Remove all code related to list for now.

@@ +52,5 @@
> +
> +  this._setupAllDropdowns();
> +  this._setupButton("toggle-button", this._onReaderToggle.bind(this));
> +  this._setupButton("list-button", this._onList.bind(this));
> +  this._setupButton("share-button", this._onShare.bind(this));

Share button is not really implementable on desktop right now. Remove it.

@@ +78,5 @@
> +
> +  dump("Decoding query arguments");
> +  let queryArgs = this._decodeQueryString(win.location.href);
> +
> +  this._isReadingListItem = (queryArgs.readingList == "1");

Remove all code related to reading list for now.

@@ +169,5 @@
> +      classes.remove("on");
> +    }
> +  },
> +
> +  _onReaderToggle: function Reader_onToggle() {

You can remove this method.

@@ +187,5 @@
> +      }.bind(this));
> +    }
> +  },
> +
> +  _onList: function Reader_onList() {

You can remove this method.

@@ +192,5 @@
> +    if (!this._article)
> +      return;
> +  },
> +
> +  _onShare: function Reader_onShare() {

Same here.

@@ +312,5 @@
> +  },
> +
> +
> +
> +  _updateImageMargins: function Reader_updateImageMargins() {

This logic here will probably have to be re-done for the larger screens on desktop. But let's get the core feature working first before tweaking this.

@@ +553,5 @@
> +      }.bind(this), true);
> +    }
> +  },
> +
> +  _pushDropdownState: function Reader_pushDropdownState() {

You can probably remove this method.

::: mobile/android/themes/core/aboutReader.css
@@ +10,4 @@
>  }
>  
>  body {
> +  font-family: "OpenSansRegular",sans-serif;

Is Open Sans available in desktop at all? I don't think it is. We're in the process of licensing new fonts for Reader Mode so you just use "sans-serif" for now.

@@ +37,4 @@
>  }
>  
>  .header > h1 {
> +  font-family: "OpenSansLight",sans-serif;

Same here.

@@ +496,5 @@
>  
> +/*.toggle-button.on {
> +	background-image: url('chrome://browser/skin/places/reader-toggle-on-icon-port-hdpi.png');
> +	width: 45px;
> +	height: 36px;

Remove the commented code.

@@ +520,3 @@
>  
> +.style-button {
> +  background-image: url('chrome://browser/skin/places/reader-style-icon-port-hdpi.png');

You should probably rename the image names to be consistent with the rest of the assets on desktop Firefox. No need to name it for different densities AFAIK.

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

Do you actually need to export self.onmessage here?
Attachment #683389 - Flags: review?(lucasr.at.mozilla) → review-
Comment on attachment 683389 [details] [diff] [review]
Cleaned up a couple more things and moved common files to toolkit/components/reader/

I'll wait for a new patch to be uploaded before looking it over. Nice work so far!
Attachment #683389 - Flags: review?(jaws) → feedback+
Attached file Complete version of Reader Mode (obsolete) —
We have fixed our bugs and added in previously mentioned changes.  Please try applying the patch to make sure that we will be able to commit the code at the end of the project :)
Attachment #683389 - Attachment is obsolete: true
Attachment #687581 - Flags: review?(lucasr.at.mozilla)
Attachment #687581 - Flags: review?(jaws)
Attached patch Complete version of Reader Mode (obsolete) — Splinter Review
Previous attachment wasn't a patch.  oops.  Please try to apply this patch, so that we know our changes are able to be added to firefox.
Attachment #687581 - Attachment is obsolete: true
Attachment #687581 - Flags: review?(lucasr.at.mozilla)
Attachment #687581 - Flags: review?(jaws)
Attachment #687582 - Flags: review?(lucasr.at.mozilla)
Attachment #687582 - Flags: review?(jaws)
(In reply to Chelsea Carr from comment #13)
> Created attachment 687582 [details] [diff] [review]
> Complete version of Reader Mode
> 
> Previous attachment wasn't a patch.  oops.  Please try to apply this patch,
> so that we know our changes are able to be added to firefox.

This patch applied cleanly and passed all browser-chrome tests for Linux, Windows, and OS X: https://tbpl.mozilla.org/?tree=Try&rev=cf612baa3fef
Comment on attachment 687582 [details] [diff] [review]
Complete version of Reader Mode

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

I didn't finish reviewing this patch yet, but here's some preliminary feedback that can be addressed before continuing. I pushed this patch to try server (and enabled reader mode by default). The results can be found here: https://tbpl.mozilla.org/?tree=Try&rev=21cd067568ee

::: browser/app/profile/firefox.js
@@ +26,1 @@
>  pref("browser.chromeURL","chrome://browser/content/");

nit: these blank lines above here should be removed

::: browser/base/content/browser.js
@@ +46,5 @@
>  
> + [
> +   ["AboutReader", "chrome://browser/content/aboutReader.js"],
> + ].forEach(function (aScript) {
> +   let [name, script] = aScript;

Comment #5 provided a possible way to clean this up. Please make the change that was requested there.

@@ +5677,5 @@
>  }
>  
> +var newTabBrowser = null;
> +var savedArticle = null;
> +var ar = null;

The |ar| variable is unused.

@@ +5680,5 @@
> +var savedArticle = null;
> +var ar = null;
> +
> +var ReaderMode = {
> +  _inited: false,

_inited is never used.

@@ +5695,5 @@
> +  BrowserOpenReaderMode: function(aEvent) {
> +    document.getElementById("reader-button").hidden = true;                            
> +    openUILink("about:reader?url=" + gBrowser.currentURI.specIgnoringRef, {target:gBrowser.selectedTab});
> +    newTabBrowser = gBrowser.getBrowserForTab(gBrowser.selectedTab);
> +    newTabBrowser.addEventListener("DOMContentLoaded", ReaderMode.handleDomContentLoaded, true);    

We shouldn't be storing a reference to a browser in a global variable. This will leak the tab when it is closed.

@@ +5701,5 @@
> +  
> +  PageShowHandler: function() {
> +    ReaderMode.checkReaderMode();
> +    if (savedArticle == null) {
> +      document.getElementById("reader-button").hidden = true;

These lines can be simplified to:
document.getElementById("reader-button").hidden = (savedArticle == null);

@@ +5711,5 @@
> +  
> +  TabSwitchHandler: function(event) {
> +    ReaderMode.checkReaderMode();
> +    if (savedArticle == null) {
> +      document.getElementById("reader-button").hidden = true;

document.getElementById("reader-button").hidden = (savedArticle == null);

@@ +5719,5 @@
> +    }
> +  },
> +  
> +  handleDomContentLoaded: function(event) {
> +    if (/^about:reader/.test(newTabBrowser.currentURI.specIgnoringRef)) {

You can get the current URI by doing:
var location = event.target.ownerDocument.defaultView.top.location;
var currentURI;
try {
  currentURI = makeURI(location);
} catch (e) { }
if (currentURI && /^about:reader/.test(currentURI.specIgnoringRef)) {
  ...
}

@@ +5721,5 @@
> +  
> +  handleDomContentLoaded: function(event) {
> +    if (/^about:reader/.test(newTabBrowser.currentURI.specIgnoringRef)) {
> +		  new AboutReader(newTabBrowser.contentDocument, newTabBrowser.contentWindow);
> +      newTabBrowser = gBrowser.getBrowserForTab(gBrowser.selectedTab);

This could just be gBrowser.selectedBrowser;

@@ +5731,5 @@
> +    savedArticle = null;
> +    if (gPrefService.getBoolPref("reader.enabled")) {
> +    
> +			Reader.parseDocumentFromTab(gBrowser.selectedTab, function (article) {
> +		     // Do nothing if there's no article or the page in this tab has

nit: mixed tabs and spaces here. This file's conventions are to use two-space indentation and no tabs.

@@ +5747,5 @@
> +				if (savedArticle != null) {
> +						 document.getElementById("reader-button").hidden = false;
> +				}
> +				else {
> +					document.getElementById("reader-button").hidden = true;

It would probably be a good idea to add a getter for the reader button so the "document.getElementById..." isn't duplicated everywhere.

You could add the following:
get readerButton () {
  return document.getElementById("reader-button");
}

You can learn more about getters and setters in JavaScript here, https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Operators/get

@@ +7791,5 @@
> +                                  success: success,
> +                                  title: title,
> +                                  url: url,
> +                                  }
> +                                  });*/

This dead code should just be deleted.
Attachment #687582 - Flags: review?(jaws) → feedback+
Attached patch Made some more changes (obsolete) — Splinter Review
The suggestion for handledomcontentloaded doesn't quite work, so we implemented a different fix.  We also aren't sure how to push the patch to tinderbox to test it.  How do we do that?
Attachment #687582 - Attachment is obsolete: true
Attachment #687582 - Flags: review?(lucasr.at.mozilla)
Attachment #688825 - Flags: review?(lucasr.at.mozilla)
Attachment #688825 - Flags: review?(jaws)
I pushed the latest patch to the Try server to get some tests run on it, https://tbpl.mozilla.org/?tree=Try&rev=e3c71c398317

(In reply to Chelsea Carr from comment #16)
> We also aren't sure how to push the patch to
> tinderbox to test it.  How do we do that?

Let's talk on IRC about getting you level 1 check-in permissions so you can push your patches to Try server.
Comment on attachment 688825 [details] [diff] [review]
Made some more changes

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

Looking good overall but still needs some more cleanups. I'm reviewing the reader-specific bits btw. I expect Jared to review the desktop Firefox bits.

::: browser/base/content/aboutReader.html
@@ +1,4 @@
> +<!-- 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/. -->
> +  

nit: remove trailing spaces.

::: browser/base/content/aboutReader.js
@@ +19,5 @@
> +function dump(s) {
> +  Services.console.logStringMessage("AboutReader: " + s);
> +}
> +
> +let gStrings = Services.strings.createBundle("chrome://browser/locale/aboutReader.properties");

The aboutReader.properties file is not included in the patch. It should be.

@@ +26,5 @@
> +  this._docRef = Cu.getWeakReference(doc);
> +  this._winRef = Cu.getWeakReference(win);
> +
> +  this._article = null;
> + 

nit: remove trailing spaces.

@@ +44,5 @@
> +
> +  win.addEventListener("click", this, false);
> +
> +  win.addEventListener("scroll", this, false);
> +  win.addEventListener("popstate", this, false);

popstate is used in mobile to allow us to dismiss the style dialog when the user presses the Android Back button. This doesn't really apply to desktop Firefox. You should probably replace this with something as simple as Escape key press handling.

@@ +141,5 @@
> +          this._scrolled = true;
> +          this._setToolbarVisibility(false);
> +        }
> +        break;
> +      case "popstate":

As I said above, you should probably replace this with Escape key press handling.

@@ +216,5 @@
> +  },
> +
> +  _setToolbarVisibility: function Reader_setToolbarVisibility(visible) {
> +    let win = this._win;
> +    if (win.history.state)

This code only makes sense in conjunction with the popstate events. You can probably remove it.

@@ +264,5 @@
> +        this._showError(gStrings.GetStringFromName("aboutReader.loadError"));
> +    }.bind(this));
> +  },
> +
> +

nit: remove these extra empty lines here.

@@ +369,5 @@
> +
> +  _setupStepControl: function Reader_setupStepControl(id, name, callback) {
> +    let doc = this._doc;
> +    let stepControl = doc.getElementById(id);
> +    

nit: remove trailing spaces here.

@@ +373,5 @@
> +    
> +    while (stepControl.hasChildNodes()) {
> +      stepControl.removeChild(stepControl.lastChild);
> +    }
> +    

nit: remove trailing spaces here.

@@ +413,5 @@
> +          if (segmentedButton.lastChild.tagName == "LI") {
> +            segmentedButton.removeChild(segmentedButton.lastChild);
> +          }
> +        }
> +    }

nit: add empty line here.

@@ +512,5 @@
> +
> +        let dropdownClasses = dropdown.classList;
> +
> +        if (dropdownClasses.contains("open")) {
> +          win.history.back();

You can probably replace this with dropdownClasses.remove("open") on desktop.

@@ +516,5 @@
> +          win.history.back();
> +        } else {
> +          updatePopupPosition();
> +          if (!this._closeAllDropdowns())
> +            this._pushDropdownState();

You can probably remove this pushDropdownState() method on desktop.

@@ +524,5 @@
> +      }.bind(this), true);
> +    }
> +  },
> +
> +  _pushDropdownState: function Reader_pushDropdownState() {

You can remove this method together with the popstate handling.

::: mobile/android/themes/core/aboutReader.css
@@ -7,1 @@
>  }

This patch is applying on the mobile version of aboutReader.css. Is that intended?

@@ -563,5 @@
> -  }
> -
> -  .toolbar {
> -    background-size: 56px;
> -    background-image: url('chrome://browser/skin/images/reader-toolbar-bg-xlarge-mdpi.png');

The image names can probably be simplified on desktop as you don't have to handle this many screen densities.
Attachment #688825 - Flags: review?(lucasr.at.mozilla) → review-
Comment on attachment 688825 [details] [diff] [review]
Made some more changes

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

::: browser/base/content/browser.js
@@ +228,5 @@
> +    if (event.persisted) {
> +      gPluginHandler.reshowClickToPlayNotification();
> +    }
> +    
> +    ReaderMode.PageShowHandler();

The function body here got an extra indention that should be reverted.

@@ +1285,5 @@
> +      if (event.target == content.document)
> +        setTimeout(pageShowEventHandlers, 0, event);
> +    }, true);
> +    
> +    gBrowser.tabContainer.addEventListener("TabSelect", function(evt) { setTimeout(ReaderMode.TabSwitchHandler, 0, evt); }, true);

Change the variable name from evt to event.

@@ +5680,5 @@
> +  // ReaderMode Public Methods
> +  init: function ()
> +  {
> +    //Reader Button disable/enable
> +    document.getElementById("reader-button").hidden = true;//!gPrefService.getBoolPref("reader.enabled");

This function isn't necessary. The "reader-button" should have the hidden="true" attribute in browser.xul.

@@ +5685,5 @@
> +  },
> +
> +  //Reader Mode On-Click
> +  BrowserOpenReaderMode: function(aEvent) {
> +    document.getElementById("reader-button").hidden = true;                            

A good text editor should be able to remove trailing white-space with a single command and convert tabs to 2-space indents.

@@ +5691,5 @@
> +    ReaderMode.selectedReaderTab.addEventListener("DOMContentLoaded", ReaderMode.handleDomContentLoaded, true);    
> +  },
> +  
> +  PageShowHandler: function() {
> +    ReaderMode.checkReaderMode();

You should check if the pref is enabled, and if not then the other functions should return immediately.

@@ +5701,5 @@
> +    ReaderMode.readerButton.hidden = (savedArticle == null);
> +  },
> +  
> +  handleDomContentLoaded: function(event) {
> +		if(/^about:reader/.test(ReaderMode.selectedReaderTab.currentURI.specIgnoringRef)) {

This will break if the location is aBoUt:rEaDeR, so this should be tweaked a little. Should call .toLowerCase() on the specIgnoringRef. See https://bug716464.bugzilla.mozilla.org/attachment.cgi?id=591602 for a similar bug that happened with about:Addons.

::: browser/base/content/browser.xul
@@ +607,5 @@
>                     hidden="true"
>                     onclick="SocialShareButton.onClick(event);"/>
> +            <image id="reader-button"
> +                   class="urlbar-icon"
> +                   onclick="ReaderMode.BrowserOpenReaderMode(event);"/>

Add a hidden="true" here just like the SocialShareButton above.

::: browser/themes/gnomestripe/jar.mn
@@ +79,5 @@
> +  skin/classic/browser/places/readerDropdownArrow@2x.png     (places/readerDropdownArrow@2x.png)
> +  skin/classic/browser/places/readerMinusIcon@2x.png  (places/readerMinusIcon@2x.png)
> +  skin/classic/browser/places/readerPlusIcon@2x.png   (places/readerPlusIcon@2x.png)
> +  skin/classic/browser/places/readerStyleIcon@2x.png  (places/readerStyleIcon@2x.png)
> +  skin/classic/browser/places/readerToolbarBg@2x.png  (places/readerToolbarBg@2x.png)

It doesn't make sense to include HiDPI images for Linux.

::: mobile/android/themes/core/aboutReader.css
@@ +321,4 @@
>  }
>  
>  .toolbar {
> +   font-family: helvetica,arial,clean,sans-serif;

I'm not sure why the Android theme is changing here. It should remain how it was.

::: toolkit/components/reader/readerWorker.jsm
@@ +5,5 @@
> +
> +
> +
> +
> +importScripts("JSDOMParser.js", "Readability.js");

nit: remove these empty lines above. only have one blank line between the license header and the importScripts call.
Attachment #688825 - Flags: review?(jaws) → review-
Has private browsing been considered in the implementation of the reader mode?
(In reply to Ehsan Akhgari [:ehsan] from comment #20)
> Has private browsing been considered in the implementation of the reader
> mode?

I don't think this patch has PB implications. When the reading list feature is added later in a followup, PB will need to be handled.
Forgive me if I'm replying out of embarrassing ignorance, but does Readability store the cleaned up page in a temporary location?
If so is this then cleared out when the page is closed?
Or is this just not accessible in PB mode?
Does readability store anything to the disk?  Does it cache anything in memory which can later be used in other tabs or windows?  If the answer to both of these questions is no, then we're probably fine.
(In reply to Ehsan Akhgari [:ehsan] from comment #23)
> Does readability store anything to the disk?  Does it cache anything in
> memory which can later be used in other tabs or windows?  If the answer to
> both of these questions is no, then we're probably fine.

Reading list handling is not implemented yet so nothing is written to disk (yet). I guess we'll have to disable reading list features while in a private tab. Reader mode attaches the result of the readability parsing to the tab in memory so that I can be quickly fetched once you switch to reader mode. This data is disposed with the tab when the tab is closed (can't really be used by other tabs).
Cool. This sounds good.

It may be another discussion, but we allow users to access their tabs from other computers in PB mode, so I don't see the need to disallow accessing the Reading list.  Just reread, perhaps you weren't suggesting that.
Adding to it though would seem another question.  We allow users to save bookmarks while in PB mode, is adding to the reading list much different?
Please CC me on the bug which wants to implement the reading list on both desktop and mobile, and we can discuss the privacy implications there.  According to comment 24, there's no more concerns with regard to PB mode here.  Thanks for the clarification!
(In reply to Ehsan Akhgari [:ehsan] from comment #26)
> Please CC me on the bug which wants to implement the reading list on both
> desktop and mobile, and we can discuss the privacy implications there.

To be clear (and this isn't the right bug but the discussion is here), reading list exists today in Firefox for Android. Bug 750707 appears to be the bug that added this feature, but Lucas will know more about it.
(In reply to comment #27)
> (In reply to Ehsan Akhgari [:ehsan] from comment #26)
> > Please CC me on the bug which wants to implement the reading list on both
> > desktop and mobile, and we can discuss the privacy implications there.
> 
> To be clear (and this isn't the right bug but the discussion is here), reading
> list exists today in Firefox for Android. Bug 750707 appears to be the bug that
> added this feature, but Lucas will know more about it.

I see.  Then I guess bug 820608 will cover that.  I'd appreciate if people in the know comment there.  Thanks!
(In reply to Mark S. from comment #25)
> Cool. This sounds good.
> 
> It may be another discussion, but we allow users to access their tabs from
> other computers in PB mode, so I don't see the need to disallow accessing
> the Reading list.  Just reread, perhaps you weren't suggesting that.
> Adding to it though would seem another question.  We allow users to save
> bookmarks while in PB mode, is adding to the reading list much different?

The reading list is essentially a separate bookmark root with bookmarks in it. So, that part is exactly like bookmarks. On mobile though, we also persist the content of the reading list item on disk via an IndexedDB owned by the about:reader page. This is necessary on mobile because you don't want to rely on internet connection to be able to read your saved stuff. That might not be as relevant on desktop though.
For the record, IndexedDB is completely disabled in private tabs/windows.
I am having troubles getting the reader mode to work. 

I cloned the parent rev, built it, imported the patch, rebuilt, changed setting in about:config but I dont see the reader mode toolbar.

What am I missing here?

PS: working on https://github.com/Yoric/Mozilla-Student-Projects/issues/57 so I thought this should be the place to start.
(In reply to eklavyamirani from comment #35)
> I am having troubles getting the reader mode to work. 
> 
> I cloned the parent rev, built it, imported the patch, rebuilt, changed
> setting in about:config but I dont see the reader mode toolbar.
> 
> What am I missing here?

You should only see the reader icon in the toolbar if the current page is detected as "readable content" (i.e. article-like pages). Have you tried visiting an article in, say, nytimes.com?
Yeah, I did try a few (How to geek, nytimes, Times of India, etc). Didn't work. I am using Windows Server 2012, btw.

I am trying to debug it, but I can't seem to get anywhere. 

So what should 'about:reader?url=someurl' return?
I get a page with 2 bulleted lists with nothing but '_____________________________'

I'll try to seek some help in the IRC.
(In reply to eklavyamirani from comment #37)
> So what should 'about:reader?url=someurl' return?
> I get a page with 2 bulleted lists with nothing but
> '_____________________________'
> 
> I'll try to seek some help in the IRC.

This probably means the page is not working properly. Have a look at the GeckoConsole log messages in 'adb logcat' and try to spot any errors or warnings related to aboutReader.html, aboutReader.js, etc.

Accessing about:reader?url=someurl should download, parse, and display the target page in reader mode.
Found the problem.

let request = window.indexedDB.open("about:reader", this.DB_VERSION);

throws an unknown exception with the message "The operation failed for reasons unrelated to the database itself and not covered by any other error code.", which I believe has something to do with this: https://bugzilla.mozilla.org/show_bug.cgi?id=643318.

request.onerror is not called, and request object itself becomes null.
(In reply to eklavyamirani from comment #39)
> Found the problem.
> 
> let request = window.indexedDB.open("about:reader", this.DB_VERSION);
> 
> throws an unknown exception with the message "The operation failed for
> reasons unrelated to the database itself and not covered by any other error
> code.", which I believe has something to do with this:
> https://bugzilla.mozilla.org/show_bug.cgi?id=643318.
> 
> request.onerror is not called, and request object itself becomes null.

Ouch. Just so that you don't block on that, I'd probably consider initially disabling all the IndexedDB part of the code initially. The IndexedDB bits are only used for caching reading list items for offline usage. I suggest focusing only on the reader mode functionality (parsing and reformatting pages) first. Leave the reading list handling for follow-up patches.
I also identified a few bugs, which lead to the readability toolbar not showing up, and fixed them.

Mostly, the files added to jar.mn in winstripe required those files to also be listed in the aero section which was not present.

As a result, it was looking for those files in source/browser/obj/chrome/.... . However, I fixed it, and the icon now shows.

Also, as a result, now about:reader?url=someurl shows an empty page with the list-style toolbar not displaying on the page as bulleted lists, but as the reader-toolbar (with font controls, etc) however clicking it does nothing and the page itself is completely blank.
(In reply to eklavyamirani from comment #41)
> I also identified a few bugs, which lead to the readability toolbar not
> showing up, and fixed them.
> 
> Mostly, the files added to jar.mn in winstripe required those files to also
> be listed in the aero section which was not present.
> 
> As a result, it was looking for those files in
> source/browser/obj/chrome/.... . However, I fixed it, and the icon now shows.
> 
> Also, as a result, now about:reader?url=someurl shows an empty page with the
> list-style toolbar not displaying on the page as bulleted lists, but as the
> reader-toolbar (with font controls, etc) however clicking it does nothing
> and the page itself is completely blank.

I would probably go through aboutReader.js, adding some logging, and debug at which point it's failing to run.
Update:
It now works completely on Windows with caching disabled. Tested url http://communities.washingtontimes.com/neighborhood/red-pill-blue-bill/2013/sep/11/two-two-morse-and-giron-both-recalled/

Lots of article based websites are not supported however.
Blocks: 915272
Tested working with the parent revision on Windows Server 2012.
Attachment #804314 - Flags: review?(lucasr.at.mozilla)
Attachment #804314 - Flags: review?(jaws)
Comment on attachment 804314 [details] [diff] [review]
Added missing entries for reader mode icons and aboutreader.css in /browser/themes/winstripe/jar.mn under the aero section

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

jaws and/or mbrubeck are probably more appropriate reviewers here. I can give feedback on the readability-specific bits.
Attachment #804314 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 804314 [details] [diff] [review]
Added missing entries for reader mode icons and aboutreader.css in /browser/themes/winstripe/jar.mn under the aero section

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

Thanks for updating the patches here. Do you think you can rebase them off of mozilla-central tip? We no longer have a winstripe directory, it's been renamed to 'windows'. 'gnomestripe' -> 'linux', and 'pinstripe' -> 'osx', also.
Attachment #804314 - Flags: review?(jaws) → feedback+
Blocks: 920712
Blocks: 927124
Sure thing Jared, happy to help.
Is the current work being done on this bug mean reader mode will be enabled on OS X or is it restricted to just windows?
Summary: Ship Readability in the browser → Ship Readability in the desktop browser (reader mode)
While I am waiting for final review on Bug 927124, I'll start working on this.
Assignee: nobody → eklavyamirani
Status: NEW → ASSIGNED
Blocks: metrobacklog
Whiteboard: [feature] p=0
No longer blocks: metrobacklog, 927124
Whiteboard: [feature] p=0
Follows Bug 927124. Since most of the code remains the same, I copied the files directly from MetroFX after that patch is applied.

Currently implemented only for Windows. Once its complete on Windows, I'll add the files over to linux and osx.
Attachment #804314 - Attachment is obsolete: true
Attachment #8356724 - Flags: review?(jaws)
No longer blocks: 915272, 920712
Depends on: 927124
I know my comment will be totally useless but I'm really looking for this feature.

If I can help...?

Cheer-up!
Attachment #8356724 - Flags: review?(bmcbride)
Attachment #688825 - Attachment is obsolete: true
Wow, you have awesome timing - one of the questions I was about to ask in my review in bug 927124 was what would need changed to help support this on Desktop Firefox :) This is the perfect way to help answer that.
Again, follows patch in Bug 927124.

*Each tab now handles readability support properly.
*Added preference to disable reader mode

:P sorry for posting another one before the review, but I wanted to finish this one off before looking for a new project for this semester!
Attachment #8356724 - Attachment is obsolete: true
Attachment #8356724 - Flags: review?(jaws)
Attachment #8356724 - Flags: review?(bmcbride)
Attachment #8357978 - Flags: review?(mbrubeck)
Attachment #8357978 - Flags: review?(jaws)
Attachment #8357978 - Flags: review?(bmcbride)
Attached file Screenshots of desktop reader mode (obsolete) —
looks good, yuan? :)
Attachment #8357990 - Flags: review?(ywang)
Nice work, Eklavya! I would like to get our visual design Michael to take a final look and see if we need any visual tweak before land this.

Michael, seems like the font option has been changed to Charis and Clear Sans. I think the reader icon and states might need tweaks. Please see the screenshots above for details.
Flags: needinfo?(mmaslaney)
(In reply to Eklavya from comment #53)
> :P sorry for posting another one before the review, but I wanted to finish
> this one off before looking for a new project for this semester!

Heh, no worries. Sorry I'm lagging on the reviews here a bit - I'm fighting the flu this week, and I'm been inundated with dozens of reviews. I'm think I'm most of the way through the previous patch in this bug and the Metro bug. I'll probably post the review for the old patch, and use that to work on the review for the new patch.
Comment on attachment 8357978 [details] [diff] [review]
Implemented Reader Mode for Desktop

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

Thanks for doing this!  I haven't reviewed this thoroughly (and I'm not officially a reviewer for desktop Firefox), but here is some feedback:

::: browser/base/content/global-scripts.inc
@@ +6,4 @@
>  <script type="application/javascript" src="chrome://global/content/printUtils.js"/>
>  <script type="application/javascript" src="chrome://global/content/viewZoomOverlay.js"/>
>  <script type="application/javascript" src="chrome://browser/content/places/browserPlacesViews.js"/>
> +<script type="application/javascript" src="chrome://browser/content/aboutReader.js"/>

Is this needed in the main window, or only in aboutReader.html?  If it's not used in the chrome window, you should remove this line.

::: browser/themes/windows/browser.css
@@ +1246,5 @@
>  }
>  
> +#urlbar > #reader-button {
> +  list-style-image: url("chrome://browser/skin/urlbar-reader.png");
> +  -moz-image-region: rect(0, 14px, 14px, 0px);

We'll need similar changes for the Mac and Linux themes.
Attachment #8357978 - Flags: review?(mbrubeck)
Additional UX work is needed before we ship:

• Reader mode glyphs should be themed and sized for each platform

• Desktop's form factor differs from android, therefore, we will need to adjust the typesetting for each of the three size options

• We need to make sure that the Reader mode UI can scale for future features like "Reading List".
Flags: needinfo?(mmaslaney)
Comment on attachment 8357978 [details] [diff] [review]
Implemented Reader Mode for Desktop

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

Thanks for the patch! I'm clearing review for now, please address the comments below and reupload.

::: browser/base/content/browser.js
@@ +158,5 @@
>    "about:home",
>    "about:privatebrowsing",
>    "about:welcomeback",
> +  "about:sessionrestore",
> +  "about:reader"

why does about:reader need to be added to gInitialPages?

@@ +804,5 @@
>      // setup history swipe animation
>      gHistorySwipeAnimation.init();
> +    Reader.init();
> +    ReaderMode.init();
> +    gBrowser.addEventListener("DOMContentLoaded", function(aEvent) {

Should this event listener be removed at some point in the future?

@@ +806,5 @@
> +    Reader.init();
> +    ReaderMode.init();
> +    gBrowser.addEventListener("DOMContentLoaded", function(aEvent) {
> +      if (ReaderMode.IsPrefEnabled) {
> +        if (aEvent.originalTarget.documentURI.toLowerCase().startsWith("about:reader") && aEvent.originalTarget.defaultView == gBrowser.contentWindow) {

nit, this line is quite long. Can you break it up to separate lines?

@@ +814,5 @@
> +            Services.console.logStringMessage("\nAboutReader : " + ex + "\n");
> +          }
> +        }
> +        let currentTab = gBrowser.selectedTab;
> +        currentTab.isReadabilitySupported = undefined;

It would be better if we don't add this type of data to the Tab element. What would happen if the tab is duplicated (Ctrl+drag) or dragged out of the browser to create a new window?

::: browser/base/content/browser.xul
@@ +647,5 @@
>                               tooltiptext="&goEndCap.tooltip;"/>
> +              <toolbarbutton id="reader-button"
> +                             class="chromeclass-toolbar-additional"
> +                             onclick="ReaderMode.BrowserOpenReaderMode(event)"
> +                             />

nit, you can self close the tag on the line of the last attribute instead of on its own line.

::: browser/base/content/readerMode.js
@@ +29,2 @@
>        this.readerButton.hidden = true;
> +      gBrowser.tabContainer.addEventListener("TabSelect", ReaderMode.TabSelectHandler, false);

nit, you can drop the last argument here.

@@ +47,4 @@
>    // Reader Mode On-Click
>    BrowserOpenReaderMode: function(aEvent) {
>      this.readerButton.hidden = true;
> +    this._openReaderPage("about:reader?url=" + gBrowser.contentWindow.location.href);

_openReaderPage should add the "about:reader?url=" prefix so that all of the callers of the function don't have to supply it.

@@ +56,5 @@
>    },
>  
>    PageShowHandler: function(aEvent) {
> +    let tab = aEvent.target;
> +    if (tab.isReadabilitySupported == undefined) {

Using undefined here causes "isReadabilitySupported" to be a three-state variable, which isn't obvious at first. Can you find a better way to write this code? (Maybe introduce another variable?)

@@ +65,5 @@
>    },
>  
> +  TabSelectHandler: function(aEvent) {
> +    ReaderMode.Log("Tab switched. Calling PageShowHandler");
> +    ReaderMode.readerButton.hidden = true;

We should only show/hide the button once per TabSelect+PageShow so it doesn't blink when switching between about:reader pages.
Attachment #8357978 - Flags: review?(jaws)
Comment on attachment 8356724 [details] [diff] [review]
[WIP]Implementing Readability for Firefox (Desktop)

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

My comments here go hand-in-hand with my comments in 927124, so should be taken together. Haven't looked at the differences between this and the new patch yet, will get to it when I can.

A lot of these files seem to need basically no changes, which is what I was hoping for. So I think we should move the following into /toolkit/ so they can be used by both Desktop and Metro with minimal code duplication:
* aboutReader.html
* aboutReader.js
* aboutReader.css (see my comments in bug 927124).

Generally, most things should be in toolkit. An application should only need a few small-ish things to add support for Reader mode:
* JS code for integration (called readerMode.js in these patches)
* CSS styles for toolbars/dropdowns
* CSS styles for form-factor specific tweaks to the page (making sure the typography is appropriate for a phone vs a desktop screen)
* Any UI in the main window to switch to reader mode
* An about: redirector

::: browser/base/content/aboutReader.html
@@ +5,4 @@
>    <meta content="text/html; charset=UTF-8" http-equiv="content-type">
>    <meta name="viewport" content="width=device-width; user-scalable=0" />
>  
> +  <link rel="stylesheet" href="chrome://browser/skin/aboutReader.css" type="text/css"/>

The downside of moving this to /toolkit/ is that you can't use overlays with HTML documents to make changes specific to Desktop or Metro :\

But there is an alternative:

You can add one <link> to load a stylesheet in /toolkit/ that contains all the page styles - this is the generic styles used by both desktop and metro. Then another <link> to load a separate stylesheet in /toolkit/ that contains all the styles for the toolbars/dropdown on desktop (the default target for toolkit is desktop). Then in Metro, add an "override" directive in a jar.mn file to override the toolbars/dropdown stylesheet with Metro's own stylesheet (same syntax as what you did in bug 927124 with aboutReader.css).

::: browser/base/content/browser.js
@@ +10,4 @@
>  Cu.import("resource://gre/modules/NotificationDB.jsm");
>  Cu.import("resource:///modules/RecentWindow.jsm");
>  Cu.import("resource://gre/modules/WindowsPrefSync.jsm");
> +Cu.import("resource://gre/modules/Reader.jsm");

This should be lazy-loaded.
Attachment #8356724 - Flags: review-
Comment on attachment 8357978 [details] [diff] [review]
Implemented Reader Mode for Desktop

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

I think between my comments for the previous version of this patch, Jared's comments on this version of the patch, and the comments in bug 927124, this patch will already has enough changes scheduled that any further comments on this version won't be much use.

One important thing that did just come out of bug 927124 though: The about redirector needs to change to be *unprivileged* (ie, add URI_SAFE_FOR_UNTRUSTED_CONTENT). Should probably hide it from about:about too.
Attachment #8357978 - Flags: review?(bmcbride) → review-
Depends on: 961994
Perhaps it has been discussed elsewhere (if so please point me) or I missed it here when scanning, but is this designed such that it can be easily enough extended by addons?
I'm sure there will be users who would like to add more options or increase granularity of control for display.  
For me personally, I would love to use this as a one-click way to clean up articles being sent to Evernote.  It's very useful functionality that can be leveraged in multiple ways I think.
(In reply to Mark S. from comment #62)
> Perhaps it has been discussed elsewhere (if so please point me) or I missed
> it here when scanning, but is this designed such that it can be easily
> enough extended by addons?

Unless the addon works with the page uri *only* (for e.g. the Pocket addon), I think readability parsed pages should work with the addons out of the box, since all we are doing is displaying an html document.

(In reply to Jared Wein [:jaws] from comment #59)
> > +        let currentTab = gBrowser.selectedTab;
> > +        currentTab.isReadabilitySupported = undefined;
> 
> It would be better if we don't add this type of data to the Tab element.
> What would happen if the tab is duplicated (Ctrl+drag) or dragged out of the
> browser to create a new window?

I understand that duplicating a tab *should* duplicate this new property too, but as soon as "DOMContentLoaded" is fired, it is set to undefined. Not the cleanest way to do it, but we need a tab/browser specific property that we can use to check if the page is readability supported, instead of making an XHR everytime a tab is switched (even though the page doesn't change.)

Where do you suggest I attach the property?
 
> > +  TabSelectHandler: function(aEvent) {
> > +    ReaderMode.Log("Tab switched. Calling PageShowHandler");
> > +    ReaderMode.readerButton.hidden = true;
> 
> We should only show/hide the button once per TabSelect+PageShow so it
> doesn't blink when switching between about:reader pages.

In that case, the reader icon remains there for a second or so, before being hidden on an unsupported page. And clicking it opens about:reader page for that unsupported page.

Resetting the visibility will cause the reader icon to show up only if the page is readability supported, after the XHR request completes (or the property is read)

Does this sound good? or do you suggest changing it?
Flags: needinfo?(jaws)
(In reply to Eklavya from comment #63)
> (In reply to Jared Wein [:jaws] from comment #59)
> > > +        let currentTab = gBrowser.selectedTab;
> > > +        currentTab.isReadabilitySupported = undefined;
> > 
> > It would be better if we don't add this type of data to the Tab element.
> > What would happen if the tab is duplicated (Ctrl+drag) or dragged out of the
> > browser to create a new window?
> 
> I understand that duplicating a tab *should* duplicate this new property
> too, but as soon as "DOMContentLoaded" is fired, it is set to undefined. Not
> the cleanest way to do it, but we need a tab/browser specific property that
> we can use to check if the page is readability supported, instead of making
> an XHR everytime a tab is switched (even though the page doesn't change.)
> 
> Where do you suggest I attach the property?

You can put it on the browser (gBrowser.selectedBrowser). For example, see things like userTypedClear and mIconURL in /toolkit/content/widgets/browser.xml. But you will need to get OK from a toolkit peer to add this to the toolkit binding.

> > > +  TabSelectHandler: function(aEvent) {
> > > +    ReaderMode.Log("Tab switched. Calling PageShowHandler");
> > > +    ReaderMode.readerButton.hidden = true;
> > 
> > We should only show/hide the button once per TabSelect+PageShow so it
> > doesn't blink when switching between about:reader pages.
> 
> In that case, the reader icon remains there for a second or so, before being
> hidden on an unsupported page. And clicking it opens about:reader page for
> that unsupported page.
> 
> Resetting the visibility will cause the reader icon to show up only if the
> page is readability supported, after the XHR request completes (or the
> property is read)
> 
> Does this sound good? or do you suggest changing it?

Ok, you can leave this unchanged.
Flags: needinfo?(jaws)
Hi !

There is also an other readability service around there, called wallbag (wallbag.org, formerly poche). I'm not part of the team but regular user and beleive that they would be happy (https://twitter.com/wallabagapp/status/431770343943180288) if it were possible to work together to allow a kind of synchronisation with one's wallbag instance (everyone can install it on he's server).

By the way, I personally would love it it were possible to have the text displayed with justification (maybe by an option to chose). I feel it much easier to read.

Still in dreams, is it planned to add e-pub export one day ?

It's very likely that this comment doesn't have its place here. If it's the case, please let me know where I should move it.

Thank a lot for your work. I love it ! (from what I've seen in the screenshots)
Flags: firefox-backlog?
(In reply to dorian.goepp from comment #65)
> Hi !
> 
> There is also an other readability service around there, called wallbag
> (wallbag.org, formerly poche). I'm not part of the team but regular user and
> beleive that they would be happy
> (https://twitter.com/wallabagapp/status/431770343943180288) if it were
> possible to work together to allow a kind of synchronisation with one's
> wallbag instance (everyone can install it on he's server).
> 
> By the way, I personally would love it it were possible to have the text
> displayed with justification (maybe by an option to chose). I feel it much
> easier to read.
> 
> Still in dreams, is it planned to add e-pub export one day ?
> 
> It's very likely that this comment doesn't have its place here. If it's the
> case, please let me know where I should move it.
> 
> Thank a lot for your work. I love it ! (from what I've seen in the
> screenshots)

My bad, the application name is Wallabag and their website is https://www.wallabag.org/.

Sounds good, no ?
Flags: firefox-backlog? → firefox-backlog+
Eklavya, now that Michael posted an awesome spec to work with, would you be comfortable working on this some more?
What are your plans?
Flags: needinfo?(eklavyamirani+bugmail)
Certainly. I am wrapping up the work for reader mode in metro firefox, since it is almost done.
You can find all my changes at https://github.com/eklavyamirani/gecko-dev
Flags: needinfo?(eklavyamirani+bugmail)
(In reply to Eklavya Mirani from comment #69)
> Certainly. I am wrapping up the work for reader mode in metro firefox, since
> it is almost done.
> You can find all my changes at https://github.com/eklavyamirani/gecko-dev

Did you know that Metro Firefox was discontinued?
Yes. But since it's almost done, I really wanted to see it through to the end (atleast v)

Since my semester is about to end, I should be able to actively get back to desktop reader mode by next week.

Is there a specific deadline/milestone that I should keep in mind to finish v1 on desktop? :)
(In reply to Eklavya Mirani from comment #71)
> Yes. But since it's almost done, I really wanted to see it through to the
> end (atleast v)

Alright, good to hear!

> Since my semester is about to end, I should be able to actively get back to
> desktop reader mode by next week.

Even better to hear!!

> Is there a specific deadline/milestone that I should keep in mind to finish
> v1 on desktop? :)

Nope. But as always, the sooner the better. ;)
Attached image ReaderMode_Fira.png
An amendment to the spec.

We will be using Fira Sans instead of Clear sans for the Sans Serif typeface, specifically the Bold weight for the headers and the book weights for the body. The CSS values will remain the same as they appear in the spec.

If needed I can update the official spec.
(In reply to mmaslaney from comment #74)
> Created attachment 8432519 [details]
> ReaderMode_menu.png

I really think we should expose UI allowing users to change the font size.

I'd also float the idea that there is no need to include the font name as there is little to no benefit gained from it.  Reading it may slow down the interaction.
Hi, sorry for not posting updates on this. I am trying to squeeze in time for this, but between my first job and relocating from my house to another part of the country (and waiting for a new laptop to be delivered), I haven't been able to complete this.

Still committed to completing this. :)
Flags: needinfo?(eklavyamirani+bugmail)
Hi,

I am rebasing the patch today. Will post it for review as I am done. (and in case anybody wants to pick it up...)

For some odd reason DOMContentLoaded is not firing the first time the page is being loaded but it is fired when I switch tabs.
Flags: needinfo?(eklavyamirani+bugmail)
(In reply to Eklavya Mirani from comment #80)

> I am rebasing the patch today. Will post it for review as I am done. (and in
> case anybody wants to pick it up...)

That's awesome! I would definitely be interested in helping you land your work. I've been working on reader mode/reading list in Firefox for Android, and I want to move our code into a shared place that desktop can use as well.

It looks like the patch in this bug depends on some other changes that aren't landed in the tree. Could you give a summary of your current work and the bugs at play here?

I would love to get involved and help you out, but I don't want to step on any toes! :)
Flags: needinfo?(eklavyamirani+bugmail)
Thanks Margaret, I will sit tonight and document my work and try to fix as much as I can and share it with you. 

Here's a summary of the bugs that this bug depends upon/used:
Bug 795973 : Adds a pref to enable/disable reader mode from about:config. Implemented and tested. (Not assigned to me though, anyway, it was a part of MetroFX work I did. Bug 941850)

Bug 795981 : Add a reader mode button to the location bar. Implemented and tested on Windows and OS X. Not sure about Linux, but it should work. (Added functionality in the next bug as well.)

Bug 920712 - Add Reader code to metro firefox so the Reader Icon is only displayed if the page is supported for parsing by Readability. So if you change tabs and the page is not readability supported, reader mode icon is hidden, and when you switch back, it shows up. (Similarly works in reader mode desktop as well)

Bug 963657 - UX work - Readability UI improvements
So far in this, I have implemented the UI, and a buggy drag and drop of the reader toolbar. However in my Reader Mode V1, I didn't implement any backend for reader lists due to a bug in indexedDB. This is what I have to complete for reader mode desktop.

Bug 961994 - Move aboutReader.html to /toolkit - This last patch was never reviewed. But I moved about reader.js, about reader.html, readability.js to /toolkit, since both MetroFX and Firefox desktop use the same code without any changes. The plan was for all three of the platforms to use the same codebase.

Bug 962433 - Use Reader Mode for printing articles
Nothing fancy here, but once the reader mode was implemented, we could also use for printing the readability parsed article.

If you want I can share my code with you, the only problem being DOMContentLoaded not firing as the earlier version of firefox that I was using, which was driving the code for checking if the page is readability supported and creating a new AboutReader object in case the uri was a reader page (about:reader?url=...)

Thanks,
Eklavya
Flags: needinfo?(eklavyamirani+bugmail)
Okay, here is what I have found:

1. We are using E10s now. My patch doesn't work on e10s since DOMContentLoaded/other page load events are not firing until I switch the active tab. (I loaded non-e10 window, and everything but the reader mode api works. See #2)

2. Reader Mode Api from Android has changed and requires rebasing the code on the new version of reader.js
Flags: needinfo?(jaws)
Yeah, a lot has changed between then and now. The process of rebasing the patches may not be worth the effort. If you would like to, you could probably work side-by-side with Margaret on reimplementing many of the ideas/efforts that were attempted in the previous approach.
Flags: needinfo?(jaws)
Eklavya, thanks so much for the summary.

I'm currently working in bug 793920 to move our current mobile reader mode code into toolkit, and after that I plan to work on factoring out some of the mobile-specific parts, as well as making the shared code e10s-friendly (I will file new bugs for these parts). After these changes, we should be ready to start implementing the desktop UI components on top of the shared code. I think this bug here should actually turn into a meta bug, and we can use the sub-bugs to track progress on different parts of this effort.

Given the fact that a lot has changed since you last worked on this, I think it would make sense to wait for me to finish these larger pieces of work, and then you can jump in to help out with some of the desktop UI pieces you've already worked on. Once the big pieces land, there should be lots of things we can do in parallel, and I would definitely appreciate your help!
Depends on: 1111142
Depends on: 1113795
Depends on: 1117258
Attachment #8357978 - Attachment is obsolete: true
Attachment #8357990 - Attachment is obsolete: true
Alias: desktop-reader
Depends on: 1120518
Depends on: 1120735
Depends on: 1123471
Depends on: 1123910
Depends on: 1124011
Depends on: 1124271
Depends on: 907079
Depends on: 1127337
Depends on: 1128724
Depends on: 1129029
Depends on: 1129106
Flags: firefox-backlog+
Depends on: 1128757
Depends on: 1130094
Depends on: 1130206
Depends on: 1128916
Depends on: 1130588
Depends on: 1132307
Depends on: 1132547
Depends on: 1132656
Depends on: 1132659
Depends on: 1132665
No longer blocks: 1131303
Depends on: 1131303
Assignee: eklavyamirani+bugmail → nobody
Depends on: 1132674
Depends on: 1134363
Depends on: 1134441
Depends on: 1134932
Depends on: 1134965
Depends on: 1135355
Since the e10s-friendly redesign, with this feature still be accessible to addons?

I can imagine imagine an uber featured addon which allows more custom display or one that uses the output to clean up pages before sending them to Evernote.
(In reply to Caspy7 from comment #86)
> Since the e10s-friendly redesign, with this feature still be accessible to
> addons?
> 
> I can imagine imagine an uber featured addon which allows more custom
> display or one that uses the output to clean up pages before sending them to
> Evernote.

To be honest, I haven't spent much time thinking about how add-ons will want to interact with reader view, but I imagine they could follow the same strategies add-ons use to interact with content in general (such as whatever any page mode add-on would need to do). I think it could be interesting to see add-ons add more capabilities to reader view.

And as for an add-on that just wants to use the basic Readability functionality without modifying the reader view, ReaderMode.jsm provides APIs that would let them directly process content.
Depends on: 1135587
Depends on: 1135589
Depends on: 1135593
Depends on: 1135915
Depends on: 1107097
Depends on: 1136245
Depends on: 1135553
Depends on: 1136265
Depends on: 1136704
Depends on: 1136716
Depends on: 1134443
Depends on: 1136231
Depends on: 1138628
Depends on: 1139026
Depends on: 1137211
No longer depends on: 1128916
Depends on: fix-readability
Depends on: 1139678
Depends on: 1140045
Depends on: 1139967
Depends on: 1140233
Depends on: 1140340
Depends on: 1135351
Depends on: 1140345
No longer depends on: 1139165
Depends on: 1140172
Depends on: 1142499
Depends on: 1142298
Depends on: 1143844
No longer depends on: 1135355
Depends on: 1128916
Depends on: 1144822
Depends on: 1146358
Depends on: 1146373
Depends on: 1147122
Depends on: 1147626
Blocks: 1149138
Depends on: 1150476
Depends on: 1147337
Depends on: 1151415
Depends on: 1151685
Depends on: 1151732
Depends on: 1151734
Depends on: 1151735
Depends on: 1151737
Depends on: 1151793
Depends on: 1151795
Depends on: 1150229
Depends on: 1152121
Depends on: 1153775
Depends on: 1154028
Depends on: 1151150
Depends on: 1156211
Depends on: 1157123
Depends on: 1157682
Depends on: 1159684
Depends on: 1155536
Depends on: 1163519
Depends on: 1163523
Depends on: 1163534
Depends on: 1163526
Depends on: 1162750
Depends on: 1166240
Depends on: 1166648
Depends on: 1166651
Depends on: 1166704
Depends on: 1165670
Blocks: 1184362
What is the current purpose of this bug? (and does it make sense that it's still open?)

The bug-title is "Ship Readability in the desktop browser (reader mode)", and we did that last June in Firefox 38.0.5.

Seems like this bug should either be closed as FIXED (with concerns/issues still tracked via 'depends-on' entries), or its summary should be updated to reflect the reality of whatever work is remaining to be done here (if any).
(In reply to Daniel Holbert [:dholbert] from comment #88)
> What is the current purpose of this bug? (and does it make sense that it's
> still open?)
> 
> The bug-title is "Ship Readability in the desktop browser (reader mode)",
> and we did that last June in Firefox 38.0.5.
> 
> Seems like this bug should either be closed as FIXED (with concerns/issues
> still tracked via 'depends-on' entries), or its summary should be updated to
> reflect the reality of whatever work is remaining to be done here (if any).

At this point this bug is just a dumping ground of issues we found while implementing desktop reader mode.

We could just close this. Someone could triage the list of open bugs, but unless this becomes more of a priority I don't think anyone is going to have the time to work on them.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Depends on: 1480459
Summary: Ship Readability in the desktop browser (reader mode) → [meta] Ship Readability in the desktop browser (reader mode)
You need to log in before you can comment on or make changes to this bug.