Bug 1085685 - Consolidate logic to add articles to reading list. r=rnewman
authorMargaret Leibovic <margaret.leibovic@gmail.com>
Mon, 27 Oct 2014 17:59:16 -0700
changeset 212545 0d00ee006544cdd1462f3fce34fb139722908ac0
parent 212544 9eea6cacf02a17e156e9b9bfc013cb6cd9a26e15
child 212546 def2e873550b20acc4b874484785783b96ec8245
push id27720
push usercbook@mozilla.com
push dateTue, 28 Oct 2014 14:51:21 +0000
treeherdermozilla-central@a2d58c6420f4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersrnewman
bugs1085685
milestone36.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 1085685 - Consolidate logic to add articles to reading list. r=rnewman
mobile/android/base/ReadingListHelper.java
mobile/android/base/locales/en-US/android_strings.dtd
mobile/android/base/strings.xml.in
mobile/android/chrome/content/Reader.js
mobile/android/chrome/content/aboutReader.js
--- a/mobile/android/base/ReadingListHelper.java
+++ b/mobile/android/base/ReadingListHelper.java
@@ -22,23 +22,18 @@ import android.content.ContentValues;
 import android.content.Context;
 import android.net.Uri;
 import android.util.Log;
 import android.widget.Toast;
 
 public final class ReadingListHelper implements GeckoEventListener, NativeEventListener {
     private static final String LOGTAG = "ReadingListHelper";
 
-    private static final int READER_ADD_SUCCESS = 0;
-    private static final int READER_ADD_FAILED = 1;
-    private static final int READER_ADD_DUPLICATE = 2;
-
     protected final Context context;
 
-
     public ReadingListHelper(Context context) {
         this.context = context;
 
         EventDispatcher.getInstance().registerGeckoThreadListener((GeckoEventListener) this,
             "Reader:AddToList", "Reader:FaviconRequest");
         EventDispatcher.getInstance().registerGeckoThreadListener((NativeEventListener) this,
             "Reader:ListStatusRequest", "Reader:RemoveFromList");
     }
@@ -81,26 +76,16 @@ public final class ReadingListHelper imp
         }
     }
 
     /**
      * A page can be added to the ReadingList by long-tap of the page-action
      * icon, or by tapping the readinglist-add icon in the ReaderMode banner.
      */
     private void handleAddToList(JSONObject message) {
-        final int result = message.optInt("result", READER_ADD_FAILED);
-        if (result != READER_ADD_SUCCESS) {
-            if (result == READER_ADD_FAILED) {
-                showToast(R.string.reading_list_failed, Toast.LENGTH_SHORT);
-            } else if (result == READER_ADD_DUPLICATE) {
-                showToast(R.string.reading_list_duplicate, Toast.LENGTH_SHORT);
-            }
-            return;
-        }
-
         final ContentValues values = new ContentValues();
         final String url = message.optString("url");
 
         values.put(ReadingListItems.URL, url);
         values.put(ReadingListItems.TITLE, message.optString("title"));
         values.put(ReadingListItems.LENGTH, message.optInt("length"));
         values.put(ReadingListItems.EXCERPT, message.optString("excerpt"));
 
--- a/mobile/android/base/locales/en-US/android_strings.dtd
+++ b/mobile/android/base/locales/en-US/android_strings.dtd
@@ -354,18 +354,16 @@ size. -->
 <!-- Localization note (site_settings_*) : These strings are used in the "Site Settings"
      dialog that appears after selecting the "Edit Site Settings" context menu item. -->
 <!ENTITY site_settings_title3       "Site Settings">
 <!ENTITY site_settings_cancel       "Cancel">
 <!ENTITY site_settings_clear        "Clear">
 <!ENTITY site_settings_no_settings  "There are no settings to clear.">
 
 <!ENTITY reading_list_added "Page added to your Reading List">
-<!ENTITY reading_list_failed "Failed to add page to your Reading List">
-<!ENTITY reading_list_duplicate "Page already in your Reading List">
 
 <!-- Localization note (reading_list_time_minutes) : This string is used in the "Reading List"
      panel on the home page to give the user an estimate of how many minutes it will take to
      read an article. The word "minute" should be abbreviated if possible. -->
 <!ENTITY reading_list_time_minutes "&formatD;min">
 <!ENTITY reading_list_time_over_an_hour "Over an hour">
 
 <!-- Localization note : These strings are used as alternate text for accessibility.
--- a/mobile/android/base/strings.xml.in
+++ b/mobile/android/base/strings.xml.in
@@ -289,18 +289,16 @@
   <string name="edit_mode_cancel">&edit_mode_cancel;</string>
 
   <string name="site_settings_title">&site_settings_title3;</string>
   <string name="site_settings_cancel">&site_settings_cancel;</string>
   <string name="site_settings_clear">&site_settings_clear;</string>
   <string name="site_settings_no_settings">&site_settings_no_settings;</string>
 
   <string name="reading_list_added">&reading_list_added;</string>
-  <string name="reading_list_failed">&reading_list_failed;</string>
-  <string name="reading_list_duplicate">&reading_list_duplicate;</string>
   <string name="reading_list_time_minutes">&reading_list_time_minutes;</string>
   <string name="reading_list_time_over_an_hour">&reading_list_time_over_an_hour;</string>
 
   <string name="page_action_dropmarker_description">&page_action_dropmarker_description;</string>
 
   <string name="contextmenu_open_new_tab">&contextmenu_open_new_tab;</string>
   <string name="contextmenu_open_private_tab">&contextmenu_open_private_tab;</string>
   <string name="contextmenu_remove">&contextmenu_remove;</string>
--- a/mobile/android/chrome/content/Reader.js
+++ b/mobile/android/chrome/content/Reader.js
@@ -5,20 +5,16 @@
 "use strict";
 
 let Reader = {
   // Version of the cache database schema
   DB_VERSION: 1,
 
   DEBUG: 0,
 
-  READER_ADD_SUCCESS: 0,
-  READER_ADD_FAILED: 1,
-  READER_ADD_DUPLICATE: 2,
-
   // Don't try to parse the page if it has too many elements (for memory and
   // performance reasons)
   MAX_ELEMS_TO_PARSE: 3000,
 
   _requests: {},
 
   get isEnabledForParseOnLoad() {
     delete this.isEnabledForParseOnLoad;
@@ -33,17 +29,17 @@ let Reader = {
     readerModeCallback: function(tabID) {
       Messaging.sendRequest({
         type: "Reader:Toggle",
         tabID: tabID
       });
     },
 
     readerModeActiveCallback: function(tabID) {
-      Reader.addTabToReadingList(tabID);
+      Reader._addTabToReadingList(tabID);
       UITelemetry.addEvent("save.1", "pageaction", null, "reader");
     },
   },
 
   updatePageAction: function(tab) {
     if (this.pageAction.id) {
       PageActions.remove(this.pageAction.id);
       delete this.pageAction.id;
@@ -88,60 +84,59 @@ let Reader = {
         if (aData.startsWith("reader.parse-on-load.")) {
           this.isEnabledForParseOnLoad = this.getStateForParseOnLoad();
         }
         break;
       }
     }
   },
 
-  addTabToReadingList: function(tabID) {
+  _addTabToReadingList: function(tabID) {
     let tab = BrowserApp.getTabForId(tabID);
     let currentURI = tab.browser.currentURI;
-    let url = currentURI.spec;
     let urlWithoutRef = currentURI.specIgnoringRef;
 
-    let sendResult = function(result, article) {
-      article = article || {};
-
-      Messaging.sendRequest({
-        type: "Reader:AddToList",
-        result: result,
-        title: truncate(article.title, MAX_TITLE_LENGTH),
-        url: truncate(url, MAX_URI_LENGTH),
-        length: article.length,
-        excerpt: article.excerpt
-      });
-    }.bind(this);
-
-    let handleArticle = function(article) {
-      if (!article) {
-        sendResult(this.READER_ADD_FAILED, null);
+    this.getArticleFromCache(urlWithoutRef, (article) => {
+      // If the article is already in the cache, just use that.
+      if (article) {
+        this.addArticleToReadingList(article);
         return;
       }
 
-      this.storeArticleInCache(article, function(success) {
-        let result = (success ? this.READER_ADD_SUCCESS : this.READER_ADD_FAILED);
-        sendResult(result, article);
-      }.bind(this));
-    }.bind(this);
+      // Otherwise, get the article data from the tab.
+      this.getArticleForTab(tabID, urlWithoutRef, (article) => {
+        if (article) {
+          this.addArticleToReadingList(article);
+        } else {
+          // If there was a problem getting the article, just store the
+          // URL and title from the tab.
+          this.addArticleToReadingList({
+            url: urlWithoutRef,
+            title: tab.browser.contentDocument.title,
+          });
+        }
+      });
+    });
+  },
 
-    this.getArticleFromCache(urlWithoutRef, function (article) {
-      // If the article is already in reading list, bail
-      if (article) {
-        sendResult(this.READER_ADD_DUPLICATE, null);
-        return;
-      }
+  addArticleToReadingList: function(article) {
+    if (!article || !article.url) {
+      Cu.reportError("addArticleToReadingList requires article with valid URL");
+      return;
+    }
 
-      if (tabID != null) {
-        this.getArticleForTab(tabID, urlWithoutRef, handleArticle);
-      } else {
-        this.parseDocumentFromURL(urlWithoutRef, handleArticle);
-      }
-    }.bind(this));
+    Messaging.sendRequest({
+      type: "Reader:AddToList",
+      url: truncate(article.url, MAX_URI_LENGTH),
+      title: truncate(article.title || "", MAX_TITLE_LENGTH),
+      length: article.length || 0,
+      excerpt: article.excerpt || "",
+    });
+
+    this.storeArticleInCache(article);
   },
 
   getStateForParseOnLoad: function Reader_getStateForParseOnLoad() {
     let isEnabled = Services.prefs.getBoolPref("reader.parse-on-load.enabled");
     let isForceEnabled = Services.prefs.getBoolPref("reader.parse-on-load.force-enabled");
     // For low-memory devices, don't allow reader mode since it takes up a lot of memory.
     // See https://bugzilla.mozilla.org/show_bug.cgi?id=792603 for details.
     return isForceEnabled || (isEnabled && !BrowserApp.isOnLowMemoryPlatform);
@@ -256,36 +251,33 @@ let Reader = {
 
       request.onsuccess = function(event) {
         this.log("Got article from the cache DB: " + event.target.result);
         callback(event.target.result);
       }.bind(this);
     }.bind(this));
   },
 
-  storeArticleInCache: function Reader_storeArticleInCache(article, callback) {
+  storeArticleInCache: function Reader_storeArticleInCache(article) {
     this._getCacheDB(function(cacheDB) {
       if (!cacheDB) {
-        callback(false);
         return;
       }
 
       let transaction = cacheDB.transaction(cacheDB.objectStoreNames, "readwrite");
       let articles = transaction.objectStore(cacheDB.objectStoreNames[0]);
 
       let request = articles.add(article);
 
       request.onerror = function(event) {
         this.log("Error storing article in the cache DB: " + article.url);
-        callback(false);
       }.bind(this);
 
       request.onsuccess = function(event) {
         this.log("Stored article in the cache DB: " + article.url);
-        callback(true);
       }.bind(this);
     }.bind(this));
   },
 
   removeArticleFromCache: function Reader_removeArticleFromCache(url) {
     this._getCacheDB(function(cacheDB) {
       if (!cacheDB) {
         return;
--- a/mobile/android/chrome/content/aboutReader.js
+++ b/mobile/android/chrome/content/aboutReader.js
@@ -315,35 +315,19 @@ AboutReader.prototype = {
     });
   },
 
   _onReaderToggle: function Reader_onToggle() {
     if (!this._article)
       return;
 
     if (this._isReadingListItem == 0) {
-      let uptime = UITelemetry.uptimeMillis();
-      gChromeWin.Reader.storeArticleInCache(this._article, function(success) {
-        let result = gChromeWin.Reader.READER_ADD_FAILED;
-        if (success) {
-          result = gChromeWin.Reader.READER_ADD_SUCCESS;
-          UITelemetry.addEvent("save.1", "button", uptime, "reader");
-        }
-
-        let json = JSON.stringify({ fromAboutReader: true, url: this._article.url });
+      gChromeWin.Reader.addArticleToReadingList(this._article);
 
-        Messaging.sendRequest({
-          type: "Reader:AddToList",
-          result: result,
-          title: this._article.title,
-          url: this._article.url,
-          length: this._article.length,
-          excerpt: this._article.excerpt
-        });
-      }.bind(this));
+      UITelemetry.addEvent("save.1", "button", null, "reader");
     } else {
       Messaging.sendRequest({
         type: "Reader:RemoveFromList",
         url: this._article.url
       });
 
       UITelemetry.addEvent("unsave.1", "button", null, "reader");
     }