I was banging my head for awhile in bug 965453 trying to figure out why sending a Reader:Remove to remove an item from the reading list wasn't working, and it turns out Reader:Remove is pretty funky. * Reader:Removed is fired only from aboutReader.js's removeArticleFromCache callback; it should be sent as a response to Reader:Remove (like Reader:Add->Reader:Added). * We try to do removeArticleFromCache twice when removing in aboutReader.js (we call it directly at [1], fire Reader:Remove at [2], which calls removeArticleFromCache at [3]). * We're duplicating the logic at [4] that should really just be handled as a response at [5] [1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutReader.js#352 [2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutReader.js#355 [3] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#7560 [4] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/HomeFragment.java#248 [5] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#406

Fixes the above issues by firing Reader:Removed in Reader:Remove's handler.

Comment on attachment 8374581 [details] [diff] [review] Send Reader:Removed as response to Reader:Remove Review of attachment 8374581 [details] [diff] [review]: ----------------------------------------------------------------- Need some questions answered before reviewing. ::: mobile/android/base/home/HomeFragment.java @@ -246,5 @@ > GeckoAppShell.sendEventToGecko(e); > - > - int count = BrowserDB.getReadingListCount(cr); > - e = GeckoEvent.createBroadcastEvent("Reader:ListCountUpdated", Integer.toString(count)); > - GeckoAppShell.sendEventToGecko(e); Why is this not needed anymore? How is the reading list button enabled state going to be updated? @@ +253,5 @@ > + // The remove from reading list toast is handled in Reader:Removed, > + // so handle only the bookmark removed toast here. > + if (!mInReadingList) { > + Toast.makeText(mContext, R.string.bookmark_removed, Toast.LENGTH_SHORT).show(); > + } This is going to cause a small conflict with what sola is working in bug 959290. Please let him know. ::: mobile/android/chrome/content/browser.js @@ +7740,5 @@ > this._getCacheDB(function(cacheDB) { > + sendMessageToJava({ > + type: "Reader:Removed", > + url: url > + }); Why sending the message so early? Shouldn't you only do so in onsuccess()?

(In reply to Lucas Rocha (:lucasr) from comment #2) > ::: mobile/android/base/home/HomeFragment.java > @@ -246,5 @@ > > GeckoAppShell.sendEventToGecko(e); > > - > > - int count = BrowserDB.getReadingListCount(cr); > > - e = GeckoEvent.createBroadcastEvent("Reader:ListCountUpdated", Integer.toString(count)); > > - GeckoAppShell.sendEventToGecko(e); > > Why is this not needed anymore? How is the reading list button enabled state > going to be updated? The same reason we don't need to make the toast here anymore -- this is already handled in Reader:Added (http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#409). > @@ +253,5 @@ > > + // The remove from reading list toast is handled in Reader:Removed, > > + // so handle only the bookmark removed toast here. > > + if (!mInReadingList) { > > + Toast.makeText(mContext, R.string.bookmark_removed, Toast.LENGTH_SHORT).show(); > > + } > > This is going to cause a small conflict with what sola is working in bug > 959290. Please let him know. Sure. > ::: mobile/android/chrome/content/browser.js > @@ +7740,5 @@ > > this._getCacheDB(function(cacheDB) { > > + sendMessageToJava({ > > + type: "Reader:Removed", > > + url: url > > + }); > > Why sending the message so early? Shouldn't you only do so in onsuccess()? This was based off of what we're already doing; i.e., sending Reader:Removed without looking at success: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutReader.js#357. We should probably change this though -- updated.

(In reply to Brian Nicholson (:bnicholson) from comment #3) > Created attachment 8374969 [details] [diff] [review] > Send Reader:Removed as response to Reader:Remove, v2 > > (In reply to Lucas Rocha (:lucasr) from comment #2) > > ::: mobile/android/base/home/HomeFragment.java > > @@ -246,5 @@ > > > GeckoAppShell.sendEventToGecko(e); > > > - > > > - int count = BrowserDB.getReadingListCount(cr); > > > - e = GeckoEvent.createBroadcastEvent("Reader:ListCountUpdated", Integer.toString(count)); > > > - GeckoAppShell.sendEventToGecko(e); > > > > Why is this not needed anymore? How is the reading list button enabled state > > going to be updated? > > The same reason we don't need to make the toast here anymore -- this is > already handled in Reader:Added > (http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/ > BrowserApp.java#409). I assume you actually meant that we already show the toast in Reader:Removed here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#415

Comment on attachment 8374969 [details] [diff] [review] Send Reader:Removed as response to Reader:Remove, v2 Review of attachment 8374969 [details] [diff] [review]: ----------------------------------------------------------------- Got it. ::: mobile/android/chrome/content/aboutReader.js @@ +347,5 @@ > title: this._article.title, > url: this._article.url, > }); > }.bind(this)); > } else { For clarity, add a comment here explaining that a Reader:Remove here will both cause the article to be removed in browser.js (Reader) and the toggle button to be updated in aboutReader.js.