Bug 1131457 - Add a button to the URLBar that allows adding the current page to the Reading List. r=markh/adw
authorBlair McBride <bmcbride@mozilla.com>
Tue, 17 Mar 2015 12:49:07 -0700
changeset 234145 ad6136c1a4d05e745c0b23200a7647ec610f4a72
parent 234124 e965a1a534ecb6635975fae33a929dc6c06bcb50
child 234146 2f0f28241140a8500622bb9b0a8273ddc4317a58
push id57048
push userkwierso@gmail.com
push dateWed, 18 Mar 2015 02:06:29 +0000
treeherdermozilla-inbound@e5a94f80f342 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmarkh, adw
bugs1131457
milestone39.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1131457 - Add a button to the URLBar that allows adding the current page to the Reading List. r=markh/adw
browser/base/content/browser-readinglist.js
browser/base/content/browser.js
browser/base/content/browser.xul
browser/base/content/content.js
browser/components/readinglist/ReadingList.jsm
browser/components/readinglist/test/xpcshell/test_ReadingList.js
browser/modules/ReaderParent.jsm
browser/themes/linux/browser.css
browser/themes/linux/jar.mn
browser/themes/osx/browser.css
browser/themes/osx/jar.mn
browser/themes/shared/readinglist.inc.css
browser/themes/shared/readinglist/icons.svg
browser/themes/windows/browser.css
browser/themes/windows/jar.mn
--- a/browser/base/content/browser-readinglist.js
+++ b/browser/base/content/browser-readinglist.js
@@ -5,25 +5,44 @@
 */
 
 XPCOMUtils.defineLazyModuleGetter(this, "ReadingList",
   "resource:///modules/readinglist/ReadingList.jsm");
 
 const READINGLIST_COMMAND_ID = "readingListSidebar";
 
 let ReadingListUI = {
+  /**
+   * Frame-script messages we want to listen to.
+   * @type {[string]}
+   */
   MESSAGES: [
     "ReadingList:GetVisibility",
     "ReadingList:ToggleVisibility",
   ],
 
   /**
+   * Add-to-ReadingList toolbar button in the URLbar.
+   * @type {Element}
+   */
+  toolbarButton: null,
+
+  /**
+   * Whether this object is currently registered as a listener with ReadingList.
+   * Used to avoid inadvertantly loading the ReadLingList.jsm module on startup.
+   * @type {Boolean}
+   */
+  listenerRegistered: false,
+
+  /**
    * Initialize the ReadingList UI.
    */
   init() {
+    this.toolbarButton = document.getElementById("readinglist-addremove-button");
+
     Preferences.observe("browser.readinglist.enabled", this.updateUI, this);
 
     const mm = window.messageManager;
     for (let msg of this.MESSAGES) {
       mm.addMessageListener(msg, this);
     }
 
     this.updateUI();
@@ -58,17 +77,28 @@ let ReadingListUI = {
   },
 
   /**
    * Update the UI status, ensuring the UI is shown or hidden depending on
    * whether the feature is enabled or not.
    */
   updateUI() {
     let enabled = this.enabled;
-    if (!enabled) {
+    if (enabled) {
+      // This is a no-op if we're already registered.
+      ReadingList.addListener(this);
+      this.listenerRegistered = true;
+    } else {
+      if (this.listenerRegistered) {
+        // This is safe to call if we're not currently registered, but we don't
+        // want to forcibly load the normally lazy-loaded module on startup.
+        ReadingList.removeListener(this);
+        this.listenerRegistered = true;
+      }
+
       this.hideSidebar();
     }
 
     document.getElementById(READINGLIST_COMMAND_ID).setAttribute("hidden", !enabled);
   },
 
   /**
    * Show the ReadingList sidebar.
@@ -84,16 +114,21 @@ let ReadingListUI = {
    * Hide the ReadingList sidebar, if it is currently shown.
    */
   hideSidebar() {
     if (this.isSidebarOpen) {
       SidebarUI.hide();
     }
   },
 
+  /**
+   * Re-refresh the ReadingList bookmarks submenu when it opens.
+   *
+   * @param {Element} target - Menu element opening.
+   */
   onReadingListPopupShowing: Task.async(function* (target) {
     if (target.id == "BMB_readingListPopup") {
       // Setting this class in the .xul file messes with the way
       // browser-places.js inserts bookmarks in the menu.
       document.getElementById("BMB_viewReadingListSidebar")
               .classList.add("panel-subview-footer");
     }
 
@@ -179,9 +214,101 @@ let ReadingListUI = {
       }
 
       case "ReadingList:ToggleVisibility": {
         this.toggleSidebar();
         break;
       }
     }
   },
+
+  /**
+   * Handles toolbar button styling based on page proxy state changes.
+   *
+   * @see SetPageProxyState()
+   *
+   * @param {string} state - New state. Either "valid" or "invalid".
+   */
+  onPageProxyStateChanged: Task.async(function* (state) {
+    if (!this.toolbarButton) {
+      // nothing to do if we have no button.
+      return;
+    }
+    if (!this.enabled || state == "invalid") {
+      this.toolbarButton.setAttribute("hidden", true);
+      return;
+    }
+
+    let isInList = yield ReadingList.containsURL(gBrowser.currentURI);
+    this.setToolbarButtonState(isInList);
+  }),
+
+  /**
+   * Set the state of the ReadingList toolbar button in the urlbar.
+   * If the current tab's page is in the ReadingList (active), sets the button
+   * to allow removing the page. Otherwise, sets the button to allow adding the
+   * page (not active).
+   *
+   * @param {boolean} active - True if the button should be active (page is
+   *                           already in the list).
+   */
+  setToolbarButtonState(active) {
+    this.toolbarButton.setAttribute("already-added", active);
+
+    let type = (active ? "remove" : "add");
+    let tooltip = gNavigatorBundle.getString(`readingList.urlbar.${type}`);
+    this.toolbarButton.setAttribute("tooltiptext", tooltip);
+
+    this.toolbarButton.removeAttribute("hidden");
+  },
+
+  /**
+   * Toggle a page (from a browser) in the ReadingList, adding if it's not already added, or
+   * removing otherwise.
+   *
+   * @param {<xul:browser>} browser - Browser with page to toggle.
+   * @returns {Promise} Promise resolved when operation has completed.
+   */
+  togglePageByBrowser: Task.async(function* (browser) {
+    let item = yield ReadingList.getItemForURL(browser.currentURI);
+    if (item) {
+      yield item.delete();
+    } else {
+      yield ReadingList.addItemFromBrowser(browser);
+    }
+  }),
+
+  /**
+   * Checks if a given item matches the current tab in this window.
+   *
+   * @param {ReadingListItem} item - Item to check
+   * @returns True if match, false otherwise.
+   */
+  isItemForCurrentBrowser(item) {
+    let currentURL = gBrowser.currentURI.spec;
+    if (item.url == currentURL || item.resolvedURL == currentURL) {
+      return true;
+    }
+    return false;
+  },
+
+  /**
+   * ReadingList event handler for when an item is added.
+   *
+   * @param {ReadingListItem} item - Item added.
+   */
+  onItemAdded(item) {
+    if (this.isItemForCurrentBrowser(item)) {
+      this.setToolbarButtonState(true);
+    }
+  },
+
+  /**
+   * ReadingList event handler for when an item is deleted.
+   *
+   * @param {ReadingListItem} item - Item deleted.
+   */
+  onItemDeleted(item) {
+    if (this.isItemForCurrentBrowser(item)) {
+      this.setToolbarButtonState(false);
+    }
+  },
 };
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -1017,16 +1017,17 @@ var gBrowserInit = {
       }
       goSetCommandEnabled("cmd_newNavigatorTab", false);
     }
 
     // Misc. inits.
     CombinedStopReload.init();
     gPrivateBrowsingUI.init();
     TabsInTitlebar.init();
+    ReadingListUI.init();
 
 #ifdef XP_WIN
     if (window.matchMedia("(-moz-os-version: windows-win8)").matches &&
         window.matchMedia("(-moz-windows-default-theme)").matches) {
       let windowFrameColor = Cu.import("resource:///modules/Windows8WindowFrameColor.jsm", {})
                                .Windows8WindowFrameColor.get();
 
       // Formula from W3C's WCAG 2.0 spec's color ratio and relative luminance,
@@ -1374,17 +1375,16 @@ var gBrowserInit = {
         return;
       }
 
       // Enable the Restore Last Session command if needed
       RestoreLastSessionObserver.init();
 
       SocialUI.init();
       TabView.init();
-      ReadingListUI.init();
 
       // Telemetry for master-password - we do this after 5 seconds as it
       // can cause IO if NSS/PSM has not already initialized.
       setTimeout(() => {
         if (window.closed) {
           return;
         }
         let secmodDB = Cc["@mozilla.org/security/pkcs11moduledb;1"]
@@ -2416,16 +2416,17 @@ function UpdatePageProxyState()
 {
   if (gURLBar && gURLBar.value != gLastValidURLStr)
     SetPageProxyState("invalid");
 }
 
 function SetPageProxyState(aState)
 {
   BookmarkingUI.onPageProxyStateChanged(aState);
+  ReadingListUI.onPageProxyStateChanged(aState);
 
   if (!gURLBar)
     return;
 
   if (!gProxyFavIcon)
     gProxyFavIcon = document.getElementById("page-proxy-favicon");
 
   gURLBar.setAttribute("pageproxystate", aState);
--- a/browser/base/content/browser.xul
+++ b/browser/base/content/browser.xul
@@ -822,16 +822,20 @@
                 <label class="urlbar-display urlbar-display-switchtab" value="&urlbar.switchToTab.label;"/>
               </box>
               <hbox id="urlbar-icons">
                 <image id="page-report-button"
                        class="urlbar-icon"
                        hidden="true"
                        tooltiptext="&pageReportIcon.tooltip;"
                        onclick="gPopupBlockerObserver.onReportButtonClick(event);"/>
+                <toolbarbutton id="readinglist-addremove-button"
+                               class="tabbable urlbar-icon"
+                               hidden="true"
+                               oncommand="ReadingListUI.togglePageByBrowser(gBrowser.selectedBrowser);"/>
                 <toolbarbutton id="reader-mode-button"
                                class="tabbable"
                                hidden="true"
                                onclick="ReaderParent.handleReaderButtonEvent(event);"
                                onkeypress="ReaderParent.handleReaderButtonEvent(event);"/>
               </hbox>
               <toolbarbutton id="urlbar-go-button"
                              class="chromeclass-toolbar-additional"
--- a/browser/base/content/content.js
+++ b/browser/base/content/content.js
@@ -1012,30 +1012,30 @@ addEventListener("pageshow", function(ev
   if (event.target == content.document) {
     sendAsyncMessage("PageVisibility:Show", {
       persisted: event.persisted,
     });
   }
 });
 
 let PageMetadataMessenger = {
-  init: function() {
+  init() {
     addMessageListener("PageMetadata:GetPageData", this);
     addMessageListener("PageMetadata:GetMicrodata", this);
   },
-  receiveMessage: function(aMessage) {
-    switch(aMessage.name) {
+  receiveMessage(message) {
+    switch(message.name) {
       case "PageMetadata:GetPageData": {
         let result = PageMetadata.getData(content.document);
         sendAsyncMessage("PageMetadata:PageDataResult", result);
         break;
       }
 
       case "PageMetadata:GetMicrodata": {
-        let target = aMessage.objects;
+        let target = message.objects;
         let result = PageMetadata.getMicrodata(content.document, target);
         sendAsyncMessage("PageMetadata:MicrodataResult", result);
         break;
       }
     }
   }
 }
 PageMetadataMessenger.init();
--- a/browser/components/readinglist/ReadingList.jsm
+++ b/browser/components/readinglist/ReadingList.jsm
@@ -125,16 +125,48 @@ ReadingListImpl.prototype = {
    * @return Promise<number> The number of matching items in the list.  Rejected
    *         with an Error on error.
    */
   count: Task.async(function* (...optsList) {
     return (yield this._store.count(...optsList));
   }),
 
   /**
+   * Checks whether a given URL is in the ReadingList already.
+   *
+   * @param {String/nsIURI} url - URL to check.
+   * @returns {Promise} Promise that is fulfilled with a boolean indicating
+   *                    whether the URL is in the list or not.
+   */
+  containsURL: Task.async(function* (url) {
+    url = normalizeURI(url).spec;
+
+    // This is used on every tab switch and page load of the current tab, so we
+    // want it to be quick and avoid a DB query whenever possible.
+
+    // First check if any cached items have a direct match.
+    if (this._itemsByURL.has(url)) {
+      return true;
+    }
+
+    // Then check if any cached items may have a different resolved URL
+    // that matches.
+    for (let itemWeakRef of this._itemsByURL.values()) {
+      let item = itemWeakRef.get();
+      if (item && item.resolvedURL == url) {
+        return true;
+      }
+    }
+
+    // Finally, fall back to the DB.
+    let count = yield this.count({url: url}, {resolvedURL: url});
+    return (count > 0);
+  }),
+
+  /**
    * Enumerates the items in the list that match the given options.
    *
    * @param callback Called for each item in the enumeration.  It's passed a
    *        single object, a ReadingListItem.  It may return a promise; if so,
    *        the callback will not be called for the next item until the promise
    *        is resolved.
    * @param optsList A variable number of options objects that control the
    *        items that are matched.  See Options Objects.
@@ -185,16 +217,17 @@ ReadingListImpl.prototype = {
    * returned promise is rejected in that case.
    *
    * @param obj A simple object representing an item.
    * @return Promise<ReadingListItem> Resolved with the new item when the list
    *         is updated.  Rejected with an Error on error.
    */
   addItem: Task.async(function* (obj) {
     obj = stripNonItemProperties(obj);
+    normalizeReadingListProperties(obj);
     yield this._store.addItem(obj);
     this._invalidateIterators();
     let item = this._itemFromObject(obj);
     this._callListeners("onItemAdded", item);
     let mm = Cc["@mozilla.org/globalmessagemanager;1"].getService(Ci.nsIMessageListenerManager);
     mm.broadcastAsyncMessage("Reader:Added", item);
     return item;
   }),
@@ -243,21 +276,48 @@ ReadingListImpl.prototype = {
 
   /**
    * Find any item that matches a given URL - either the item's URL, or its
    * resolved URL.
    *
    * @param {String/nsIURI} uri - URI to match against. This will be normalized.
    */
   getItemForURL: Task.async(function* (uri) {
-    let url = this._normalizeURI(uri).spec;
+    let url = normalizeURI(uri).spec;
     let [item] = yield this.iterator({url: url}, {resolvedURL: url}).items(1);
     return item;
   }),
 
+   /**
+   * Add to the ReadingList the page that is loaded in a given browser.
+   *
+   * @param {<xul:browser>} browser - Browser element for the document.
+   * @return {Promise} Promise that is fullfilled with the added item.
+   */
+  addItemFromBrowser: Task.async(function* (browser) {
+    let metadata = yield getMetadataFromBrowser(browser);
+    let itemData = {
+      url: browser.currentURI,
+      title: metadata.title,
+      resolvedURL: metadata.url,
+      excerpt: metadata.description,
+    };
+
+    if (metadata.description) {
+      itemData.exerpt = metadata.description;
+    }
+
+    if (metadata.previews.length > 0) {
+      itemData.image = metadata.previews[0];
+    }
+
+    let item = yield ReadingList.addItem(itemData);
+    return item;
+  }),
+
   /**
    * Adds a listener that will be notified when the list changes.  Listeners
    * are objects with the following optional methods:
    *
    *   onItemAdded(item)
    *   onItemUpdated(item)
    *   onItemDeleted(item)
    *
@@ -300,32 +360,16 @@ ReadingListImpl.prototype = {
   // A Set containing nsIWeakReferences that refer to valid iterators produced
   // by the list.
   _iterators: null,
 
   // A Set containing listener objects.
   _listeners: null,
 
   /**
-   * Normalize a URI, stripping away extraneous parts we don't want to store
-   * or compare against.
-   *
-   * @param {nsIURI/String} uri - URI to normalize.
-   * @returns {nsIURI} Cloned and normalized version of the input URI.
-   */
-  _normalizeURI(uri) {
-    if (typeof uri == "string") {
-      uri = Services.io.newURI(uri, "", null);
-    }
-    uri = uri.cloneIgnoringRef();
-    uri.userPass = "";
-    return uri;
-  },
-
-  /**
    * Returns the ReadingListItem represented by the given simple object.  If
    * the item doesn't exist yet, it's created first.
    *
    * @param obj A simple object with item properties.
    * @return The ReadingListItem.
    */
   _itemFromObject(obj) {
     let itemWeakRef = this._itemsByURL.get(obj.url);
@@ -370,22 +414,36 @@ ReadingListImpl.prototype = {
         catch (err) {
           Cu.reportError(err);
         }
       }
     }
   },
 
   _ensureItemBelongsToList(item) {
-    if (item.list != this) {
-      throw new Error("The item does not belong to this list");
+    if (!item || !item._ensureBelongsToList) {
+      throw new Error("The item is not a ReadingListItem");
     }
+    item._ensureBelongsToList();
   },
 };
 
+/*
+ * normalize the properties of a "regular" object that reflects a ReadingListItem
+ */
+function normalizeReadingListProperties(obj) {
+  if (obj.url) {
+    obj.url = normalizeURI(obj.url).spec;
+  }
+  if (obj.resolvedURL) {
+    obj.resolvedURL = normalizeURI(obj.resolvedURL).spec;
+  }
+}
+
+
 let _unserializable = () => {}; // See comments in the ReadingListItem ctor.
 
 /**
  * An item in a reading list.
  *
  * Each item belongs to a list, and it's an error to use an item with a
  * ReadingList that the item doesn't belong to.
  *
@@ -426,65 +484,53 @@ ReadingListItem.prototype = {
    * guarenteed to be set for local items.
    * @type string
    */
   get guid() {
     return this._properties.guid || undefined;
   },
   set guid(val) {
     this._properties.guid = val;
-    if (this.list) {
-      this.commit();
-    }
   },
 
   /**
    * The date the item was last modified.
    * @type Date
    */
   get lastModified() {
     return this._properties.lastModified ?
            new Date(this._properties.lastModified) :
            undefined;
   },
   set lastModified(val) {
     this._properties.lastModified = val.valueOf();
-    if (this.list) {
-      this.commit();
-    }
   },
 
   /**
    * The item's URL.
    * @type string
    */
   get url() {
     return this._properties.url;
   },
   set url(val) {
-    this._properties.url = val;
-    if (this.list) {
-      this.commit();
-    }
+    this._properties.url = normalizeURI(val).spec;
   },
 
   /**
    * The item's URL as an nsIURI.
    * @type nsIURI
    */
   get uri() {
     return this._properties.url ?
            Services.io.newURI(this._properties.url, "", null) :
            undefined;
   },
   set uri(val) {
-    this.url = val.spec;
-    if (this.list) {
-      this.commit();
-    }
+    this.url = normalizeURI(val).spec;
   },
 
   /**
    * Returns the domain (a string) of the item's URL.  If the URL doesn't have a
    * domain, then the URL itself (also a string) is returned.
    */
   get domain() {
     try {
@@ -497,265 +543,211 @@ ReadingListItem.prototype = {
   /**
    * The item's resolved URL.
    * @type string
    */
   get resolvedURL() {
     return this._properties.resolvedURL;
   },
   set resolvedURL(val) {
-    this._properties.resolvedURL = val;
-    if (this.list) {
-      this.commit();
-    }
+    this._properties.resolvedURL = normalizeURI(val).spec;
   },
 
   /**
    * The item's resolved URL as an nsIURI.
    * @type nsIURI
    */
   get resolvedURI() {
     return this._properties.resolvedURL ?
            Services.io.newURI(this._properties.resolvedURL, "", null) :
            undefined;
   },
   set resolvedURI(val) {
     this.resolvedURL = val.spec;
-    if (this.list) {
-      this.commit();
-    }
   },
 
   /**
    * The item's title.
    * @type string
    */
   get title() {
     return this._properties.title;
   },
   set title(val) {
     this._properties.title = val;
-    if (this.list) {
-      this.commit();
-    }
   },
 
   /**
    * The item's resolved title.
    * @type string
    */
   get resolvedTitle() {
     return this._properties.resolvedTitle;
   },
   set resolvedTitle(val) {
     this._properties.resolvedTitle = val;
-    if (this.list) {
-      this.commit();
-    }
   },
 
   /**
    * The item's excerpt.
    * @type string
    */
   get excerpt() {
     return this._properties.excerpt;
   },
   set excerpt(val) {
     this._properties.excerpt = val;
-    if (this.list) {
-      this.commit();
-    }
   },
 
   /**
    * The item's status.
    * @type integer
    */
   get status() {
     return this._properties.status;
   },
   set status(val) {
     this._properties.status = val;
-    if (this.list) {
-      this.commit();
-    }
   },
 
   /**
    * Whether the item is a favorite.
    * @type boolean
    */
   get favorite() {
     return !!this._properties.favorite;
   },
   set favorite(val) {
     this._properties.favorite = !!val;
-    if (this.list) {
-      this.commit();
-    }
   },
 
   /**
    * Whether the item is an article.
    * @type boolean
    */
   get isArticle() {
     return !!this._properties.isArticle;
   },
   set isArticle(val) {
     this._properties.isArticle = !!val;
-    if (this.list) {
-      this.commit();
-    }
   },
 
   /**
    * The item's word count.
    * @type integer
    */
   get wordCount() {
     return this._properties.wordCount;
   },
   set wordCount(val) {
     this._properties.wordCount = val;
-    if (this.list) {
-      this.commit();
-    }
   },
 
   /**
    * Whether the item is unread.
    * @type boolean
    */
   get unread() {
     return !!this._properties.unread;
   },
   set unread(val) {
     this._properties.unread = !!val;
-    if (this.list) {
-      this.commit();
-    }
   },
 
   /**
    * The date the item was added.
    * @type Date
    */
   get addedOn() {
     return this._properties.addedOn ?
            new Date(this._properties.addedOn) :
            undefined;
   },
   set addedOn(val) {
     this._properties.addedOn = val.valueOf();
-    if (this.list) {
-      this.commit();
-    }
   },
 
   /**
    * The date the item was stored.
    * @type Date
    */
   get storedOn() {
     return this._properties.storedOn ?
            new Date(this._properties.storedOn) :
            undefined;
   },
   set storedOn(val) {
     this._properties.storedOn = val.valueOf();
-    if (this.list) {
-      this.commit();
-    }
   },
 
   /**
    * The GUID of the device that marked the item read.
    * @type string
    */
   get markedReadBy() {
     return this._properties.markedReadBy;
   },
   set markedReadBy(val) {
     this._properties.markedReadBy = val;
-    if (this.list) {
-      this.commit();
-    }
   },
 
   /**
    * The date the item marked read.
    * @type Date
    */
   get markedReadOn() {
     return this._properties.markedReadOn ?
            new Date(this._properties.markedReadOn) :
            undefined;
   },
   set markedReadOn(val) {
     this._properties.markedReadOn = val.valueOf();
-    if (this.list) {
-      this.commit();
-    }
   },
 
   /**
    * The item's read position.
    * @param integer
    */
   get readPosition() {
     return this._properties.readPosition;
   },
   set readPosition(val) {
     this._properties.readPosition = val;
-    if (this.list) {
-      this.commit();
-    }
   },
 
   /**
-   * Sets the given properties of the item, optionally calling commit().
+   * Sets the given properties of the item, optionally calling list.updateItem().
    *
    * @param props A simple object containing the properties to set.
-   * @param commit If true, commit() is called.
-   * @return Promise<null> If commit is true, resolved when the commit
+   * @param update If true, updateItem() is called for this item.
+   * @return Promise<null> If update is true, resolved when the update
    *         completes; otherwise resolved immediately.
    */
-  setProperties: Task.async(function* (props, commit=true) {
+  setProperties: Task.async(function* (props, update=true) {
     for (let name in props) {
       this._properties[name] = props[name];
     }
-    if (commit) {
-      yield this.commit();
+    // make sure everything is normalized.
+    normalizeReadingListProperties(this._properties);
+    if (update) {
+      yield this.list.updateItem(this);
     }
   }),
 
   /**
    * Deletes the item from its list.
    *
    * @return Promise<null> Resolved when the list has been updated.
    */
   delete: Task.async(function* () {
     this._ensureBelongsToList();
     yield this.list.deleteItem(this);
     this.delete = () => Promise.reject("The item has already been deleted");
   }),
 
-  /**
-   * Notifies the item's list that the item has changed so that the list can
-   * update itself.
-   *
-   * @return Promise<null> Resolved when the list has been updated.
-   */
-  commit: Task.async(function* () {
-    this._ensureBelongsToList();
-    yield this.list.updateItem(this);
-  }),
-
   toJSON() {
     return this._properties;
   },
 
   _ensureBelongsToList() {
     if (!this.list) {
       throw new Error("The item must belong to a reading list");
     }
@@ -850,16 +842,33 @@ ReadingListItemIterator.prototype = {
 
   _ensureValid() {
     if (this.invalid) {
       throw new Error("The iterator has been invalidated");
     }
   },
 };
 
+/**
+ * Normalize a URI, stripping away extraneous parts we don't want to store
+ * or compare against.
+ *
+ * @param {nsIURI/String} uri - URI to normalize.
+ * @returns {nsIURI} Cloned and normalized version of the input URI.
+ */
+function normalizeURI(uri) {
+  if (typeof uri == "string") {
+    uri = Services.io.newURI(uri, "", null);
+  }
+  uri = uri.cloneIgnoringRef();
+  try {
+    uri.userPass = "";
+  } catch (ex) {} // Not all nsURI impls (eg, nsSimpleURI) support .userPass
+  return uri;
+};
 
 function stripNonItemProperties(item) {
   let obj = {};
   for (let name of ITEM_BASIC_PROPERTY_NAMES) {
     if (name in item) {
       obj[name] = item[name];
     }
   }
@@ -880,16 +889,34 @@ function hash(str) {
     join("");
   return hexStr;
 }
 
 function clone(obj) {
   return Cu.cloneInto(obj, {}, { cloneFunctions: false });
 }
 
+/**
+ * Get page metadata from the content document in a given <xul:browser>.
+ * @see PageMetadata.jsm
+ *
+ * @param {<xul:browser>} browser - Browser element for the document.
+ * @returns {Promise} Promise that is fulfilled with an object describing the metadata.
+ */
+function getMetadataFromBrowser(browser) {
+  let mm = browser.messageManager;
+  return new Promise(resolve => {
+    function handleResult(msg) {
+      mm.removeMessageListener("PageMetadata:PageDataResult", handleResult);
+      resolve(msg.json);
+    }
+    mm.addMessageListener("PageMetadata:PageDataResult", handleResult);
+    mm.sendAsyncMessage("PageMetadata:GetPageData");
+  });
+}
 
 Object.defineProperty(this, "ReadingList", {
   get() {
     if (!this._singleton) {
       let store = new SQLiteStore("reading-list-temp.sqlite");
       this._singleton = new ReadingListImpl(store);
     }
     return this._singleton;
--- a/browser/components/readinglist/test/xpcshell/test_ReadingList.js
+++ b/browser/components/readinglist/test/xpcshell/test_ReadingList.js
@@ -117,20 +117,21 @@ add_task(function* constraints() {
     yield gList.addItem(item);
   }
   catch (e) {
     err = e;
   }
   checkError(err);
 
   // update an item with an existing url
-  item.guid = gItems[1].guid;
+  let rlitem = yield gList.getItemForURL(gItems[0].url);
+  rlitem.guid = gItems[1].guid;
   err = null;
   try {
-    yield gList.updateItem(item);
+    yield gList.updateItem(rlitem);
   }
   catch (e) {
     err = e;
   }
   checkError(err);
 
   // add a new item with an existing resolvedURL
   item = kindOfClone(gItems[0]);
@@ -140,59 +141,58 @@ add_task(function* constraints() {
     yield gList.addItem(item);
   }
   catch (e) {
     err = e;
   }
   checkError(err);
 
   // update an item with an existing resolvedURL
-  item.url = gItems[1].url;
+  rlitem = yield gList.getItemForURL(gItems[0].url);
+  rlitem.url = gItems[1].url;
   err = null;
   try {
-    yield gList.updateItem(item);
+    yield gList.updateItem(rlitem);
   }
   catch (e) {
     err = e;
   }
   checkError(err);
 
   // add a new item with no guid, which is allowed
   item = kindOfClone(gItems[0]);
   delete item.guid;
   err = null;
+  let rlitem1;
   try {
-    yield gList.addItem(item);
+    rlitem1 = yield gList.addItem(item);
   }
   catch (e) {
     err = e;
   }
   Assert.ok(!err, err ? err.message : undefined);
-  let item1 = item;
 
   // add a second item with no guid, which is allowed
   item = kindOfClone(gItems[1]);
   delete item.guid;
   err = null;
+  let rlitem2;
   try {
-    yield gList.addItem(item);
+    rlitem2 = yield gList.addItem(item);
   }
   catch (e) {
     err = e;
   }
   Assert.ok(!err, err ? err.message : undefined);
-  let item2 = item;
 
   // Delete both items since other tests assume the store contains only gItems.
-  item1.list = gList;
-  item2.list = gList;
-  yield gList.deleteItem(item1);
-  yield gList.deleteItem(item2);
+  yield gList.deleteItem(rlitem1);
+  yield gList.deleteItem(rlitem2);
   let items = [];
-  yield gList.forEachItem(i => items.push(i), { url: [item1.url, item2.url] });
+  yield gList.forEachItem(i => items.push(i), { url: [rlitem1.url, rlitem2.url] });
   Assert.equal(items.length, 0);
 
   // add a new item with no url
   item = kindOfClone(gItems[0]);
   delete item.url;
   err = null;
   try {
     yield gList.addItem(item);
@@ -508,25 +508,22 @@ add_task(function* iterator_forEach_prom
 
 add_task(function* updateItem() {
   // get an item
   let items = [];
   yield gList.forEachItem(i => items.push(i), {
     guid: gItems[0].guid,
   });
   Assert.equal(items.length, 1);
-  let item = {
-    _properties: items[0]._properties,
-    list: items[0].list,
-  };
+  let item = items[0];
 
   // update its title
   let newTitle = "updateItem new title";
   Assert.notEqual(item.title, newTitle);
-  item._properties.title = newTitle;
+  item.title = newTitle;
   yield gList.updateItem(item);
 
   // get the item again
   items = [];
   yield gList.forEachItem(i => items.push(i), {
     guid: gItems[0].guid,
   });
   Assert.equal(items.length, 1);
@@ -537,92 +534,98 @@ add_task(function* updateItem() {
 add_task(function* item_setProperties() {
   // get an item
   let iter = gList.iterator({
     sort: "guid",
   });
   let item = (yield iter.items(1))[0];
   Assert.ok(item);
 
-  // item.setProperties(commit=false).  After fetching the item again, its title
+  // item.setProperties(update=false).  After fetching the item again, its title
   // should be the old title.
   let oldTitle = item.title;
   let newTitle = "item_setProperties title 1";
   Assert.notEqual(oldTitle, newTitle);
   item.setProperties({ title: newTitle }, false);
   Assert.equal(item.title, newTitle);
   iter = gList.iterator({
     sort: "guid",
   });
   let sameItem = (yield iter.items(1))[0];
   Assert.ok(item === sameItem);
   Assert.equal(sameItem.title, oldTitle);
 
-  // item.setProperties(commit=true).  After fetching the item again, its title
+  // item.setProperties(update=true).  After fetching the item again, its title
   // should be the new title.
   newTitle = "item_setProperties title 2";
   item.setProperties({ title: newTitle }, true);
   Assert.equal(item.title, newTitle);
   iter = gList.iterator({
     sort: "guid",
   });
   sameItem = (yield iter.items(1))[0];
   Assert.ok(item === sameItem);
   Assert.equal(sameItem.title, newTitle);
 
   // Set item.title directly.  After fetching the item again, its title should
   // be the new title.
   newTitle = "item_setProperties title 3";
   item.title = newTitle;
+  gList.updateItem(item);
   Assert.equal(item.title, newTitle);
   iter = gList.iterator({
     sort: "guid",
   });
   sameItem = (yield iter.items(1))[0];
   Assert.ok(item === sameItem);
   Assert.equal(sameItem.title, newTitle);
 });
 
 add_task(function* listeners() {
+  Assert.equal((yield gList.count()), gItems.length);
   // add an item
   let resolve;
   let listenerPromise = new Promise(r => resolve = r);
   let listener = {
     onItemAdded: resolve,
   };
   gList.addListener(listener);
   let item = kindOfClone(gItems[0]);
   let items = yield Promise.all([listenerPromise, gList.addItem(item)]);
   Assert.ok(items[0]);
   Assert.ok(items[0] === items[1]);
   gList.removeListener(listener);
+  Assert.equal((yield gList.count()), gItems.length + 1);
 
   // update an item
   listenerPromise = new Promise(r => resolve = r);
   listener = {
     onItemUpdated: resolve,
   };
   gList.addListener(listener);
   items[0].title = "listeners new title";
+  gList.updateItem(items[0]);
   let listenerItem = yield listenerPromise;
   Assert.ok(listenerItem);
   Assert.ok(listenerItem === items[0]);
   gList.removeListener(listener);
+  Assert.equal((yield gList.count()), gItems.length + 1);
 
   // delete an item
   listenerPromise = new Promise(r => resolve = r);
   listener = {
     onItemDeleted: resolve,
   };
   gList.addListener(listener);
   items[0].delete();
   listenerItem = yield listenerPromise;
   Assert.ok(listenerItem);
   Assert.ok(listenerItem === items[0]);
   gList.removeListener(listener);
+  Assert.equal((yield gList.count()), gItems.length);
 });
 
 // This test deletes items so it should probably run last.
 add_task(function* deleteItem() {
   // delete first item with item.delete()
   let iter = gList.iterator({
     sort: "guid",
   });
@@ -633,27 +636,27 @@ add_task(function* deleteItem() {
   Assert.equal((yield gList.count()), gItems.length - 1);
   let items = [];
   yield gList.forEachItem(i => items.push(i), {
     sort: "guid",
   });
   checkItems(items, gItems.slice(1));
 
   // delete second item with list.deleteItem()
-  yield gList.deleteItem(gItems[1]);
+  yield gList.deleteItem(items[0]);
   gItems[1].list = null;
   Assert.equal((yield gList.count()), gItems.length - 2);
   items = [];
   yield gList.forEachItem(i => items.push(i), {
     sort: "guid",
   });
   checkItems(items, gItems.slice(2));
 
   // delete third item with list.deleteItem()
-  yield gList.deleteItem(gItems[2]);
+  yield gList.deleteItem(items[0]);
   gItems[2].list = null;
   Assert.equal((yield gList.count()), gItems.length - 3);
   items = [];
   yield gList.forEachItem(i => items.push(i), {
     sort: "guid",
   });
   checkItems(items, gItems.slice(3));
 });
@@ -668,17 +671,17 @@ function checkItems(actualItems, expecte
       }
     }
     Assert.equal(actualItems[i].list, expectedItems[i].list);
   }
 }
 
 function checkError(err) {
   Assert.ok(err);
-  Assert.ok(err instanceof Cu.getGlobalForObject(Sqlite).Error);
+  Assert.ok(err instanceof Cu.getGlobalForObject(Sqlite).Error, err);
 }
 
 function kindOfClone(item) {
   let newItem = {};
   for (let prop in item) {
     newItem[prop] = item[prop];
     if (typeof(newItem[prop]) == "string") {
       newItem[prop] += " -- make this string different";
--- a/browser/modules/ReaderParent.jsm
+++ b/browser/modules/ReaderParent.jsm
@@ -55,22 +55,22 @@ let ReaderParent = {
         });
         break;
 
       case "Reader:FaviconRequest": {
         // XXX: To implement.
         break;
       }
       case "Reader:ListStatusRequest":
-        ReadingList.count(message.data).then(count => {
+        ReadingList.containsURL(message.data.url).then(inList => {
           let mm = message.target.messageManager
           // Make sure the target browser is still alive before trying to send data back.
           if (mm) {
             mm.sendAsyncMessage("Reader:ListStatusData",
-                                { inReadingList: !!count, url: message.data.url });
+                                { inReadingList: inList, url: message.data.url });
           }
         });
         break;
 
       case "Reader:RemoveFromList":
         // We need to get the "real" item to delete it.
         ReadingList.getItemForURL(message.data.url).then(item => {
           ReadingList.deleteItem(item)
--- a/browser/themes/linux/browser.css
+++ b/browser/themes/linux/browser.css
@@ -1624,16 +1624,18 @@ richlistitem[type~="action"][actiontype=
   -moz-image-region: rect(28px, 28px, 42px, 14px);
 }
 
 /* Popup blocker button */
 #page-report-button {
   list-style-image: url("chrome://browser/skin/Info.png");
 }
 
+%include ../shared/readinglist.inc.css
+
 /* Reader mode button */
 
 #reader-mode-button {
   -moz-appearance: none;
   padding: 0;
   list-style-image: url("chrome://browser/skin/reader-mode-16.png");
   -moz-image-region: rect(0, 16px, 16px, 0);
 }
--- a/browser/themes/linux/jar.mn
+++ b/browser/themes/linux/jar.mn
@@ -86,16 +86,17 @@ browser.jar:
   skin/classic/browser/Toolbar-inverted.png
   skin/classic/browser/Toolbar-small.png
   skin/classic/browser/undoCloseTab.png                        (../shared/undoCloseTab.png)
   skin/classic/browser/urlbar-arrow.png
   skin/classic/browser/session-restore.svg                  (../shared/incontent-icons/session-restore.svg)
   skin/classic/browser/tab-crashed.svg                      (../shared/incontent-icons/tab-crashed.svg)
   skin/classic/browser/welcome-back.svg                     (../shared/incontent-icons/welcome-back.svg)
   skin/classic/browser/reader-mode-16.png             (../shared/reader/reader-mode-16.png)
+  skin/classic/browser/readinglist/icons.svg          (../shared/readinglist/icons.svg)
   skin/classic/browser/readinglist/readinglist-icon.svg (../shared/readinglist/readinglist-icon.svg)
   skin/classic/browser/readinglist/sidebar.css        (../shared/readinglist/sidebar.css)
   skin/classic/browser/webRTC-shareDevice-16.png
   skin/classic/browser/webRTC-shareDevice-64.png
   skin/classic/browser/webRTC-sharingDevice-16.png    (../shared/webrtc/webRTC-sharingDevice-16.png)
   skin/classic/browser/webRTC-shareMicrophone-16.png
   skin/classic/browser/webRTC-shareMicrophone-64.png
   skin/classic/browser/webRTC-sharingMicrophone-16.png (../shared/webrtc/webRTC-sharingMicrophone-16.png)
--- a/browser/themes/osx/browser.css
+++ b/browser/themes/osx/browser.css
@@ -2518,16 +2518,18 @@ richlistitem[type~="action"][actiontype=
   }
 
   #page-report-button:hover:active,
   #page-report-button[open="true"] {
     -moz-image-region: rect(0, 64px, 32px, 32px);
   }
 }
 
+%include ../shared/readinglist.inc.css
+
 /* Reader mode button */
 
 #reader-mode-button {
   -moz-appearance: none;
   padding: 0;
   list-style-image: url("chrome://browser/skin/reader-mode-16.png");
   -moz-image-region: rect(0, 16px, 16px, 0);
 }
--- a/browser/themes/osx/jar.mn
+++ b/browser/themes/osx/jar.mn
@@ -137,16 +137,17 @@ browser.jar:
   skin/classic/browser/urlbar-arrow@2x.png
   skin/classic/browser/urlbar-popup-blocked.png
   skin/classic/browser/urlbar-popup-blocked@2x.png
   skin/classic/browser/session-restore.svg            (../shared/incontent-icons/session-restore.svg)
   skin/classic/browser/tab-crashed.svg                (../shared/incontent-icons/tab-crashed.svg)
   skin/classic/browser/welcome-back.svg               (../shared/incontent-icons/welcome-back.svg)
   skin/classic/browser/reader-mode-16.png             (../shared/reader/reader-mode-16.png)
   skin/classic/browser/reader-mode-16@2x.png          (../shared/reader/reader-mode-16@2x.png)
+  skin/classic/browser/readinglist/icons.svg          (../shared/readinglist/icons.svg)
   skin/classic/browser/readinglist/readinglist-icon.svg (../shared/readinglist/readinglist-icon.svg)
   skin/classic/browser/readinglist/sidebar.css        (../shared/readinglist/sidebar.css)
   skin/classic/browser/webRTC-shareDevice-16.png
   skin/classic/browser/webRTC-shareDevice-16@2x.png
   skin/classic/browser/webRTC-shareDevice-64.png
   skin/classic/browser/webRTC-shareDevice-64@2x.png
   skin/classic/browser/webRTC-sharingDevice-16.png    (../shared/webrtc/webRTC-sharingDevice-16.png)
   skin/classic/browser/webRTC-sharingDevice-16@2x.png (../shared/webrtc/webRTC-sharingDevice-16@2x.png)
new file mode 100644
--- /dev/null
+++ b/browser/themes/shared/readinglist.inc.css
@@ -0,0 +1,38 @@
+/* Reading List button */
+
+#readinglist-addremove-button {
+  -moz-appearance: none;
+  border: none;
+  list-style-image: url("chrome://browser/skin/readinglist/icons.svg#addpage");
+  padding: 3px;
+}
+
+#readinglist-addremove-button:hover {
+  border: none;
+}
+
+#readinglist-addremove-button > .toolbarbutton-icon {
+  width: 16px;
+  height: 16px
+}
+
+#readinglist-addremove-button:not([already-added="true"]):hover {
+  list-style-image: url("chrome://browser/skin/readinglist/icons.svg#addpage-hover");
+}
+
+#readinglist-addremove-button:not([already-added="true"]):active {
+  list-style-image: url("chrome://browser/skin/readinglist/icons.svg#addpage-active");
+}
+
+#readinglist-addremove-button[already-added="true"] {
+  list-style-image: url("chrome://browser/skin/readinglist/icons.svg#alreadyadded");
+}
+
+#readinglist-addremove-button[already-added="true"]:hover {
+  list-style-image: url("chrome://browser/skin/readinglist/icons.svg#alreadyadded-hover");
+}
+
+#readinglist-addremove-button[already-added="true"]:active {
+  list-style-image: url("chrome://browser/skin/readinglist/icons.svg#alreadyadded-active");
+}
+
new file mode 100644
--- /dev/null
+++ b/browser/themes/shared/readinglist/icons.svg
@@ -0,0 +1,57 @@
+<?xml version="1.0" encoding="utf-8"?>
+<!-- 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/. -->
+<svg xmlns="http://www.w3.org/2000/svg"
+     xmlns:xlink="http://www.w3.org/1999/xlink"
+     viewBox="0 0 16 16"
+     xml:space="preserve">
+
+  <defs>
+    <style type="text/css">
+      use:not(:target) {
+        display: none;
+      }
+
+      #addpage {
+        fill: #808080;
+      }
+      #addpage-hover {
+        fill: #555555;
+      }
+      #addpage-active {
+        fill: #0095DD;
+      }
+
+      #alreadyadded {
+        fill: #0095DD;
+      }
+      #alreadyadded-hover {
+        fill: #555555;
+      }
+      #alreadyadded-active {
+        fill: #808080;
+      }
+    </style>
+
+    <mask id="plus-mask">
+      <rect width="100%" height="100%" fill="white"/>
+      <rect x="4" y="7.5" width="8" height="1"/>
+      <rect x="7.5" y="4" width="1" height="8"/>
+    </mask>
+
+    <g id="addpage-shape">
+      <circle cx="8" cy="8" r="7" mask="url(#plus-mask)"/>
+    </g>
+
+  </defs>
+
+  <use id="addpage"                 xlink:href="#addpage-shape"/>
+  <use id="addpage-hover"           xlink:href="#addpage-shape"/>
+  <use id="addpage-active"          xlink:href="#addpage-shape"/>
+
+  <use id="alreadyadded"            xlink:href="#addpage-shape"/>
+  <use id="alreadyadded-hover"      xlink:href="#addpage-shape"/>
+  <use id="alreadyadded-active"     xlink:href="#addpage-shape"/>
+
+</svg>
--- a/browser/themes/windows/browser.css
+++ b/browser/themes/windows/browser.css
@@ -1571,16 +1571,18 @@ richlistitem[type~="action"][actiontype=
   -moz-image-region: rect(0, 32px, 16px, 16px);
 }
 
 #page-report-button:hover:active,
 #page-report-button[open="true"] {
   -moz-image-region: rect(0, 48px, 16px, 32px);
 }
 
+%include ../shared/readinglist.inc.css
+
 /* Reader mode button */
 
 #reader-mode-button {
   -moz-appearance: none;
   padding: 0;
   list-style-image: url("chrome://browser/skin/reader-mode-16.png");
   -moz-image-region: rect(0, 16px, 16px, 0);
 }
--- a/browser/themes/windows/jar.mn
+++ b/browser/themes/windows/jar.mn
@@ -105,16 +105,17 @@ browser.jar:
         skin/classic/browser/undoCloseTab@2x.png                     (../shared/undoCloseTab@2x.png)
         skin/classic/browser/urlbar-arrow.png
         skin/classic/browser/urlbar-popup-blocked.png
         skin/classic/browser/urlbar-history-dropmarker.png
         skin/classic/browser/session-restore.svg                     (../shared/incontent-icons/session-restore.svg)
         skin/classic/browser/tab-crashed.svg                         (../shared/incontent-icons/tab-crashed.svg)
         skin/classic/browser/welcome-back.svg                        (../shared/incontent-icons/welcome-back.svg)
         skin/classic/browser/reader-mode-16.png                      (../shared/reader/reader-mode-16.png)
+        skin/classic/browser/readinglist/icons.svg                   (../shared/readinglist/icons.svg)
         skin/classic/browser/readinglist/readinglist-icon.svg        (../shared/readinglist/readinglist-icon.svg)
         skin/classic/browser/readinglist/sidebar.css                 (../shared/readinglist/sidebar.css)
         skin/classic/browser/notification-pluginNormal.png           (../shared/plugins/notification-pluginNormal.png)
         skin/classic/browser/notification-pluginAlert.png            (../shared/plugins/notification-pluginAlert.png)
         skin/classic/browser/notification-pluginBlocked.png          (../shared/plugins/notification-pluginBlocked.png)
         skin/classic/browser/webRTC-shareDevice-16.png
         skin/classic/browser/webRTC-shareDevice-64.png
         skin/classic/browser/webRTC-sharingDevice-16.png             (../shared/webrtc/webRTC-sharingDevice-16.png)
@@ -572,16 +573,17 @@ browser.jar:
         skin/classic/aero/browser/undoCloseTab@2x.png                     (../shared/undoCloseTab@2x.png)
         skin/classic/aero/browser/urlbar-arrow.png
         skin/classic/aero/browser/urlbar-popup-blocked.png
         skin/classic/aero/browser/urlbar-history-dropmarker.png
         skin/classic/aero/browser/session-restore.svg               (../shared/incontent-icons/session-restore.svg)
         skin/classic/aero/browser/tab-crashed.svg                   (../shared/incontent-icons/tab-crashed.svg)
         skin/classic/aero/browser/welcome-back.svg                  (../shared/incontent-icons/welcome-back.svg)
         skin/classic/aero/browser/reader-mode-16.png                (../shared/reader/reader-mode-16.png)
+        skin/classic/aero/browser/readinglist/icons.svg             (../shared/readinglist/icons.svg)
         skin/classic/aero/browser/readinglist/readinglist-icon.svg  (../shared/readinglist/readinglist-icon.svg)
         skin/classic/aero/browser/readinglist/sidebar.css           (../shared/readinglist/sidebar.css)
         skin/classic/aero/browser/notification-pluginNormal.png     (../shared/plugins/notification-pluginNormal.png)
         skin/classic/aero/browser/notification-pluginAlert.png      (../shared/plugins/notification-pluginAlert.png)
         skin/classic/aero/browser/notification-pluginBlocked.png    (../shared/plugins/notification-pluginBlocked.png)
         skin/classic/aero/browser/webRTC-shareDevice-16.png
         skin/classic/aero/browser/webRTC-shareDevice-64.png
         skin/classic/aero/browser/webRTC-sharingDevice-16.png             (../shared/webrtc/webRTC-sharingDevice-16.png)