Bug 923086 - "Add to reading list" button does not save it's state, r=lucasr
authorMark Capella <markcapella@twcny.rr.com>
Mon, 14 Oct 2013 09:58:16 -0400
changeset 164538 bc531ef1dfa7789c39257304837e9b97d84ca0ed
parent 164537 a21ab2d667de6be5e82b270848566577b1758dbd
child 164539 1c0baa3cf12ca8bb523ad8a542bc2610a52ac367
push id3066
push userakeybl@mozilla.com
push dateMon, 09 Dec 2013 19:58:46 +0000
treeherdermozilla-beta@a31a0dce83aa [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerslucasr
bugs923086
milestone27.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 923086 - "Add to reading list" button does not save it's state, r=lucasr Part 2 - Actual final bug solution, tie into Part 1
mobile/android/base/BrowserApp.java
mobile/android/base/GeckoApp.java
mobile/android/base/ReaderModeUtils.java
mobile/android/base/Tab.java
mobile/android/base/home/HomeFragment.java
mobile/android/base/home/ReadingListPage.java
mobile/android/chrome/content/aboutReader.js
--- a/mobile/android/base/BrowserApp.java
+++ b/mobile/android/base/BrowserApp.java
@@ -335,16 +335,36 @@ abstract public class BrowserApp extends
             @Override
             public void run() {
                 final int count = BrowserDB.getReadingListCount(getContentResolver());
                 GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Reader:ListCountReturn", Integer.toString(count)));
             }
         });
     }
 
+    void handleReaderListStatusRequest(final String url) {
+        ThreadUtils.postToBackgroundThread(new Runnable() {
+            @Override
+            public void run() {
+                final int inReadingList = BrowserDB.isReadingListItem(getContentResolver(), url) ? 1 : 0;
+
+                final JSONObject json = new JSONObject();
+                try {
+                    json.put("url", url);
+                    json.put("inReadingList", inReadingList);
+                } catch (JSONException e) {
+                    Log.e(LOGTAG, "JSON error - failed to return inReadingList status", e);
+                    return;
+                }
+
+                GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Reader:ListStatusReturn", json.toString()));
+            }
+        });
+    }
+
     void handleReaderAdded(int result, final String title, final String url) {
         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);
             }
 
@@ -1099,16 +1119,18 @@ abstract public class BrowserApp extends
 
             } else if (event.equals("Telemetry:Gather")) {
                 Telemetry.HistogramAdd("PLACES_PAGES_COUNT", BrowserDB.getCount(getContentResolver(), "history"));
                 Telemetry.HistogramAdd("PLACES_BOOKMARKS_COUNT", BrowserDB.getCount(getContentResolver(), "bookmarks"));
                 Telemetry.HistogramAdd("FENNEC_FAVICONS_COUNT", BrowserDB.getCount(getContentResolver(), "favicons"));
                 Telemetry.HistogramAdd("FENNEC_THUMBNAILS_COUNT", BrowserDB.getCount(getContentResolver(), "thumbnails"));
             } else if (event.equals("Reader:ListCountRequest")) {
                 handleReaderListCountRequest();
+            } else if (event.equals("Reader:ListStatusRequest")) {
+                handleReaderListStatusRequest(message.getString("url"));
             } else if (event.equals("Reader:Added")) {
                 final int result = message.getInt("result");
                 final String title = message.getString("title");
                 final String url = message.getString("url");
                 handleReaderAdded(result, title, url);
             } else if (event.equals("Reader:Removed")) {
                 final String url = message.getString("url");
                 handleReaderRemoved(url);
--- a/mobile/android/base/GeckoApp.java
+++ b/mobile/android/base/GeckoApp.java
@@ -1456,16 +1456,17 @@ abstract public class GeckoApp
         }
 
         //app state callbacks
         mAppStateListeners = new LinkedList<GeckoAppShell.AppStateListener>();
 
         //register for events
         registerEventListener("log");
         registerEventListener("Reader:ListCountRequest");
+        registerEventListener("Reader:ListStatusRequest");
         registerEventListener("Reader:Added");
         registerEventListener("Reader:Removed");
         registerEventListener("Reader:Share");
         registerEventListener("Reader:FaviconRequest");
         registerEventListener("onCameraCapture");
         registerEventListener("Menu:Add");
         registerEventListener("Menu:Remove");
         registerEventListener("Menu:Update");
@@ -1991,16 +1992,17 @@ abstract public class GeckoApp
         super.onRestart();
     }
 
     @Override
     public void onDestroy()
     {
         unregisterEventListener("log");
         unregisterEventListener("Reader:ListCountRequest");
+        unregisterEventListener("Reader:ListStatusRequest");
         unregisterEventListener("Reader:Added");
         unregisterEventListener("Reader:Removed");
         unregisterEventListener("Reader:Share");
         unregisterEventListener("Reader:FaviconRequest");
         unregisterEventListener("onCameraCapture");
         unregisterEventListener("Menu:Add");
         unregisterEventListener("Menu:Remove");
         unregisterEventListener("Menu:Update");
--- a/mobile/android/base/ReaderModeUtils.java
+++ b/mobile/android/base/ReaderModeUtils.java
@@ -57,22 +57,21 @@ public class ReaderModeUtils {
 
         String urlFromAboutReader = getUrlFromAboutReader(newUrl);
         if (urlFromAboutReader == null)
             return false;
 
         return urlFromAboutReader.equals(currentUrl);
     }
 
-    public static String getAboutReaderForUrl(String url, boolean inReadingList) {
-        return getAboutReaderForUrl(url, -1, inReadingList);
+    public static String getAboutReaderForUrl(String url) {
+        return getAboutReaderForUrl(url, -1);
     }
 
-    public static String getAboutReaderForUrl(String url, int tabId, boolean inReadingList) {
-        String aboutReaderUrl = "about:reader?url=" + Uri.encode(url) +
-                                "&readingList=" + (inReadingList ? 1 : 0);
+    public static String getAboutReaderForUrl(String url, int tabId) {
+        String aboutReaderUrl = "about:reader?url=" + Uri.encode(url);
 
         if (tabId >= 0)
             aboutReaderUrl += "&tabId=" + tabId;
 
         return aboutReaderUrl;
     }
 }
--- a/mobile/android/base/Tab.java
+++ b/mobile/android/base/Tab.java
@@ -459,17 +459,17 @@ public class Tab {
         GeckoAppShell.sendEventToGecko(e);
     }
 
     public void toggleReaderMode() {
         if (ReaderModeUtils.isAboutReader(mUrl)) {
             Tabs.getInstance().loadUrl(ReaderModeUtils.getUrlFromAboutReader(mUrl));
         } else if (mReaderEnabled) {
             mEnteringReaderMode = true;
-            Tabs.getInstance().loadUrl(ReaderModeUtils.getAboutReaderForUrl(mUrl, mId, mReadingListItem));
+            Tabs.getInstance().loadUrl(ReaderModeUtils.getAboutReaderForUrl(mUrl, mId));
         }
     }
 
     public boolean isEnteringReaderMode() {
         return mEnteringReaderMode;
     }
 
     public void doReload() {
--- a/mobile/android/base/home/HomeFragment.java
+++ b/mobile/android/base/home/HomeFragment.java
@@ -148,30 +148,30 @@ abstract class HomeFragment extends Frag
                 Log.e(LOGTAG, "Can't open in new tab because URL is null");
                 return false;
             }
 
             int flags = Tabs.LOADURL_NEW_TAB | Tabs.LOADURL_BACKGROUND;
             if (item.getItemId() == R.id.home_open_private_tab)
                 flags |= Tabs.LOADURL_PRIVATE;
 
-            final String url = (info.inReadingList ? ReaderModeUtils.getAboutReaderForUrl(info.url, true) : info.url);
+            final String url = (info.inReadingList ? ReaderModeUtils.getAboutReaderForUrl(info.url) : info.url);
             Tabs.getInstance().loadUrl(url, flags);
             Toast.makeText(context, R.string.new_tab_opened, Toast.LENGTH_SHORT).show();
             return true;
         }
 
         if (itemId == R.id.home_edit_bookmark) {
             // UI Dialog associates to the activity context, not the applications'.
             new EditBookmarkDialog(getActivity()).show(info.url);
             return true;
         }
 
         if (itemId == R.id.home_open_in_reader) {
-            final String url = ReaderModeUtils.getAboutReaderForUrl(info.url, true);
+            final String url = ReaderModeUtils.getAboutReaderForUrl(info.url);
             Tabs.getInstance().loadUrl(url, Tabs.LOADURL_NONE);
             return true;
         }
 
         if (itemId == R.id.home_remove) {
             // Prioritize removing a history entry over a bookmark in the case of a combined item.
             final int historyId = info.historyId;
             if (historyId > -1) {
--- a/mobile/android/base/home/ReadingListPage.java
+++ b/mobile/android/base/home/ReadingListPage.java
@@ -96,17 +96,17 @@ public class ReadingListPage extends Hom
             @Override
             public void onItemClick(AdapterView<?> parent, View view, int position, long id) {
                 final Cursor c = mAdapter.getCursor();
                 if (c == null || !c.moveToPosition(position)) {
                     return;
                 }
 
                 String url = c.getString(c.getColumnIndexOrThrow(URLColumns.URL));
-                url = ReaderModeUtils.getAboutReaderForUrl(url, true);
+                url = ReaderModeUtils.getAboutReaderForUrl(url);
 
                 // This item is a TwoLinePageRow, so we allow switch-to-tab.
                 mUrlOpenListener.onUrlOpen(url, EnumSet.of(OnUrlOpenListener.Flags.ALLOW_SWITCH_TO_TAB));
             }
         });
 
         registerForContextMenu(mList);
     }
--- a/mobile/android/chrome/content/aboutReader.js
+++ b/mobile/android/chrome/content/aboutReader.js
@@ -28,16 +28,17 @@ let AboutReader = function(doc, win) {
   this._docRef = Cu.getWeakReference(doc);
   this._winRef = Cu.getWeakReference(win);
 
   Services.obs.addObserver(this, "Reader:FaviconReturn", false);
   Services.obs.addObserver(this, "Reader:Add", false);
   Services.obs.addObserver(this, "Reader:Remove", false);
   Services.obs.addObserver(this, "Reader:ListCountReturn", false);
   Services.obs.addObserver(this, "Reader:ListCountUpdated", false);
+  Services.obs.addObserver(this, "Reader:ListStatusReturn", false);
 
   this._article = null;
 
   dump("Feching toolbar, header and content notes from about:reader");
   this._headerElementRef = Cu.getWeakReference(doc.getElementById("reader-header"));
   this._domainElementRef = Cu.getWeakReference(doc.getElementById("reader-domain"));
   this._titleElementRef = Cu.getWeakReference(doc.getElementById("reader-title"));
   this._creditsElementRef = Cu.getWeakReference(doc.getElementById("reader-credits"));
@@ -114,17 +115,18 @@ let AboutReader = function(doc, win) {
 
   let fontSize = Services.prefs.getIntPref("reader.font_size");
   this._setupSegmentedButton("font-size-buttons", fontSizeOptions, fontSize, this._setFontSize.bind(this));
   this._setFontSize(fontSize);
 
   dump("Decoding query arguments");
   let queryArgs = this._decodeQueryString(win.location.href);
 
-  this._isReadingListItem = (queryArgs.readingList == "1");
+  // Track status of reader toolbar add/remove toggle button
+  this._isReadingListItem = -1;
   this._updateToggleButton();
 
   // Track status of reader toolbar list button
   this._readingListCount = -1;
   this._updateListButton();
   this._requestReadingListCount();
 
   let url = queryArgs.url;
@@ -187,46 +189,68 @@ AboutReader.prototype = {
         this._loadFavicon(args.url, args.faviconUrl);
         Services.obs.removeObserver(this, "Reader:FaviconReturn");
         break;
       }
 
       case "Reader:Add": {
         let args = JSON.parse(aData);
         if (args.url == this._article.url) {
-          if (!this._isReadingListItem) {
-            this._isReadingListItem = true;
+          if (this._isReadingListItem != 1) {
+            this._isReadingListItem = 1;
             this._updateToggleButton();
           }
         }
         break;
       }
 
       case "Reader:Remove": {
         if (aData == this._article.url) {
-          if (this._isReadingListItem) {
-            this._isReadingListItem = false;
+          if (this._isReadingListItem != 0) {
+            this._isReadingListItem = 0;
             this._updateToggleButton();
           }
         }
         break;
       }
 
       case "Reader:ListCountReturn":
-      case "Reader:ListCountUpdated":  {
+      case "Reader:ListCountUpdated": {
         let count = parseInt(aData);
         if (this._readingListCount != count) {
           let isInitialStateChange = (this._readingListCount == -1);
           this._readingListCount = count;
           this._updateListButton();
 
           // Display the toolbar when all its initial component states are known
           if (isInitialStateChange) {
             this._setToolbarVisibility(true);
           }
+
+          // Initial readinglist count is requested before any page is displayed
+          if (this._article) {
+            this._requestReadingListStatus();
+          }
+        }
+        break;
+      }
+
+      case "Reader:ListStatusReturn": {
+        let args = JSON.parse(aData);
+        if (args.url == this._article.url) {
+          if (this._isReadingListItem != args.inReadingList) {
+            let isInitialStateChange = (this._isReadingListItem == -1);
+            this._isReadingListItem = args.inReadingList;
+            this._updateToggleButton();
+
+            // Display the toolbar when all its initial component states are known
+            if (isInitialStateChange) {
+              this._setToolbarVisibility(true);
+            }
+          }
         }
         break;
       }
     }
   },
 
   handleEvent: function Reader_handleEvent(aEvent) {
     if (!aEvent.isTrusted)
@@ -258,24 +282,25 @@ AboutReader.prototype = {
         this._handleDeviceLight(aEvent.value);
         break;
 
       case "unload":
         Services.obs.removeObserver(this, "Reader:Add");
         Services.obs.removeObserver(this, "Reader:Remove");
         Services.obs.removeObserver(this, "Reader:ListCountReturn");
         Services.obs.removeObserver(this, "Reader:ListCountUpdated");
+        Services.obs.removeObserver(this, "Reader:ListStatusReturn");
         break;
     }
   },
 
   _updateToggleButton: function Reader_updateToggleButton() {
     let classes = this._doc.getElementById("toggle-button").classList;
 
-    if (this._isReadingListItem) {
+    if (this._isReadingListItem == 1) {
       classes.add("on");
     } else {
       classes.remove("on");
     }
   },
 
   _updateListButton: function Reader_updateListButton() {
     let classes = this._doc.getElementById("list-button").classList;
@@ -286,24 +311,31 @@ AboutReader.prototype = {
       classes.remove("on");
     }
   },
 
   _requestReadingListCount: function Reader_requestReadingListCount() {
     gChromeWin.sendMessageToJava({ type: "Reader:ListCountRequest" });
   },
 
+  _requestReadingListStatus: function Reader_requestReadingListStatus() {
+    gChromeWin.sendMessageToJava({
+      type: "Reader:ListStatusRequest",
+      url: this._article.url
+    });
+  },
+
   _onReaderToggle: function Reader_onToggle() {
     if (!this._article)
       return;
 
-    this._isReadingListItem = !this._isReadingListItem;
+    this._isReadingListItem = (this._isReadingListItem == 1) ? 0 : 1;
     this._updateToggleButton();
 
-    if (this._isReadingListItem) {
+    if (this._isReadingListItem == 1) {
       gChromeWin.Reader.storeArticleInCache(this._article, function(success) {
         dump("Reader:Add (in reader) success=" + success);
 
         let result = (success ? gChromeWin.Reader.READER_ADD_SUCCESS :
             gChromeWin.Reader.READER_ADD_FAILED);
 
         let json = JSON.stringify({ fromAboutReader: true, url: this._article.url });
         Services.obs.notifyObservers(null, "Reader:Add", json);
@@ -457,17 +489,17 @@ AboutReader.prototype = {
     let win = this._win;
     if (win.history.state)
       win.history.back();
 
     if (!this._toolbarEnabled)
       return;
 
     // Don't allow visible toolbar until banner state is known
-    if (this._readingListCount == -1)
+    if (this._readingListCount == -1 || this._isReadingListItem == -1)
       return;
 
     if (this._getToolbarVisibility() === visible)
       return;
 
     this._toolbarElement.classList.toggle("toolbar-hidden");
 
     if (!visible && !this._hasUsedToolbar) {
@@ -627,16 +659,17 @@ AboutReader.prototype = {
     let contentFragment = parserUtils.parseFragment(article.content, Ci.nsIParserUtils.SanitizerDropForms,
                                                     false, articleUri, this._contentElement);
     this._contentElement.innerHTML = "";
     this._contentElement.appendChild(contentFragment);
     this._updateImageMargins();
     this._maybeSetTextDirection(article);
 
     this._contentElement.style.display = "block";
+    this._requestReadingListStatus();
 
     this._toolbarEnabled = true;
     this._setToolbarVisibility(true);
 
     this._requestFavicon();
   },
 
   _hideContent: function Reader_hideContent() {