Right now aboutReader.js and Reader.js each have their own logic for adding articles to the reader cache, and then they each tell Java to add the item to the reading list. Not only is there duplicate logic, but there's slightly inconsistent duplicate logic. We should just create a single API for adding articles to the reading list.

I added a new addArticleToReadingList API to Reader to deal with this. To reduce some of the layers of callbacks, I decided we should just show the failed/duplicate toast messages in JS as soon as we run into those cases, rather than passing a "Reader:Added" message to Java. I'm concerned that the "Tab:Add" observer in aboutReader.js isn't actually doing what we want (I think it should actually be listening for a notification that happens at the same time as the "Tab:Added" message is sent), but we can address that in a separate patch. Also, we're updating the toggle button immediately in aboutReader.js, but it won't revert if the add fails.

Comment on attachment 8508306 [details] [diff] [review] Consolidate logic to add articles to reading list Review of attachment 8508306 [details] [diff] [review]: ----------------------------------------------------------------- I think I'm one patch behind, which bug breaks the Reader object out of browser? fyi, This whole area is tricky, with messages overlapping adding/removing from Reader internal cache vs. adding/removing from ReadingList. ::: mobile/android/base/ReadingListHelper.java @@ +32,5 @@ > public ReadingListHelper(Context context) { > this.context = context; > > EventDispatcher.getInstance().registerGeckoThreadListener((GeckoEventListener) this, > "Reader:Added", "Reader:FaviconRequest"); In Bug 1078355 I almost changed this message into "Reader:ListAdded" to better distinguish between "Reader:Add" and "Reader:Added" ::: mobile/android/chrome/content/Reader.js @@ +107,4 @@ > this.getArticleFromCache(urlWithoutRef, function (article) { > // If the article is already in reading list, bail > if (article) { > + NativeWindow.toast.show(Strings.browser.GetStringFromName("readingList.duplicate"), "short"); Is this right re: ReadingList duplicate vs. articleCache duplicate? ::: mobile/android/chrome/content/aboutReader.js @@ -333,5 @@ > - UITelemetry.addEvent("save.1", "button", uptime, "reader"); > - } > - > - let json = JSON.stringify({ fromAboutReader: true, url: this._article.url }); > - Services.obs.notifyObservers(null, "Reader:Add", json); iir ... here we actually get creative and loop this message back to ourselves in aboutReader.js to update the in/out of ReadingList button on duplicate URL's openned in multiple Tabs. You might need this (?)

Comment on attachment 8508306 [details] [diff] [review] Consolidate logic to add articles to reading list Review of attachment 8508306 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/chrome/content/Reader.js @@ +147,5 @@ > } > } > }, > > + addArticleToReadingList: function Reader_addArticleToReadingList(article) { No need for the Reader_add... bit. @@ +153,5 @@ > + NativeWindow.toast.show(Strings.browser.GetStringFromName("readingList.failed"), "short"); > + return; > + } > + > + this.storeArticleInCache(article, function(success) { I think we should disentangle this still further. Adding something to my reading list does not depend on successfully caching the content. (Indeed, the Share Overlay doesn't cache the content at all.) And we can prune and re-cache content without affecting our list. So we should probably send the request to Java (which shouldn't be called Reader:Added, but Reader:AddToList), *then* store the article in the cache, and not tie them together. For requests initiated from Java, something like ReadingListHelper.handleReadingListAdded can/should be called directly and immediately, followed by asking Gecko to cache the content. Do you agree? @@ +154,5 @@ > + return; > + } > + > + this.storeArticleInCache(article, function(success) { > + this.log("Reader:Add success=" + success + ", url=" + article.url + ", title=" + article.title + ", excerpt=" + article.excerpt); Remove before landing -- logs URL and content. @@ +168,5 @@ > + url: truncate(article.url, MAX_URI_LENGTH), > + length: article.length, > + excerpt: article.excerpt > + }); > + }.bind(this)); Use (success) => { } instead of function (success) { }.bind(this)

(In reply to Mark Capella [:capella] from comment #2) > Comment on attachment 8508306 [details] [diff] [review] > Consolidate logic to add articles to reading list > > Review of attachment 8508306 [details] [diff] [review]: > ----------------------------------------------------------------- > > I think I'm one patch behind, which bug breaks the Reader object out of > browser? Sorry, bug 1084134. > fyi, This whole area is tricky, with messages overlapping adding/removing > from Reader internal cache vs. adding/removing from ReadingList. Yeah, it's a bit of a mess :/ Which is why I'm trying to disentangle it here :) > ::: mobile/android/base/ReadingListHelper.java > @@ +32,5 @@ > > public ReadingListHelper(Context context) { > > this.context = context; > > > > EventDispatcher.getInstance().registerGeckoThreadListener((GeckoEventListener) this, > > "Reader:Added", "Reader:FaviconRequest"); > > In Bug 1078355 I almost changed this message into "Reader:ListAdded" to > better distinguish between "Reader:Add" and "Reader:Added" The way these are currently structured, "Reader:Added" actually means "page added to reader mode cache, so go add it to the list now", which is confusing. If I follow rnewman's suggestion above to separate the caching and adding to the list, this message should go away, which will be nice. > ::: mobile/android/chrome/content/Reader.js > @@ +107,4 @@ > > this.getArticleFromCache(urlWithoutRef, function (article) { > > // If the article is already in reading list, bail > > if (article) { > > + NativeWindow.toast.show(Strings.browser.GetStringFromName("readingList.duplicate"), "short"); > > Is this right re: ReadingList duplicate vs. articleCache duplicate? This is a result of the way this code is structured to expect the items in the cache to be equivalent to the items in the list. This change maintained the current behavior, but if I change this to always try adding the article, regardless of whether or not it's in the list, this duplicate error message should move over to Java. > ::: mobile/android/chrome/content/aboutReader.js > @@ -333,5 @@ > > - UITelemetry.addEvent("save.1", "button", uptime, "reader"); > > - } > > - > > - let json = JSON.stringify({ fromAboutReader: true, url: this._article.url }); > > - Services.obs.notifyObservers(null, "Reader:Add", json); > > iir ... here we actually get creative and loop this message back to > ourselves in aboutReader.js to update the in/out of ReadingList button on > duplicate URL's openned in multiple Tabs. You might need this (?) Yeah, this is what I mentioned when I uploaded the patch. I think what we really want is a message from Java -> JS when an item is actually successfully added to the list, and then update it accordingly.

(In reply to Richard Newman [:rnewman] from comment #3) > ::: mobile/android/chrome/content/Reader.js > @@ +147,5 @@ > > } > > } > > }, > > > > + addArticleToReadingList: function Reader_addArticleToReadingList(article) { > > No need for the Reader_add... bit. I was going for consistency within the file... but maybe I'll just file a [good first bug] to clean that up. > @@ +153,5 @@ > > + NativeWindow.toast.show(Strings.browser.GetStringFromName("readingList.failed"), "short"); > > + return; > > + } > > + > > + this.storeArticleInCache(article, function(success) { > > I think we should disentangle this still further. > > Adding something to my reading list does not depend on successfully caching > the content. (Indeed, the Share Overlay doesn't cache the content at all.) > > And we can prune and re-cache content without affecting our list. > > So we should probably send the request to Java (which shouldn't be called > Reader:Added, but Reader:AddToList), *then* store the article in the cache, > and not tie them together. > > For requests initiated from Java, something like > ReadingListHelper.handleReadingListAdded can/should be called directly and > immediately, followed by asking Gecko to cache the content. > > Do you agree? Yeah, I was trying to limit the scope of this bug, but we should deal with this. I might as well work on that now. > @@ +154,5 @@ > > + return; > > + } > > + > > + this.storeArticleInCache(article, function(success) { > > + this.log("Reader:Add success=" + success + ", url=" + article.url + ", title=" + article.title + ", excerpt=" + article.excerpt); > > Remove before landing -- logs URL and content. This actually only logs if we're in a debug mode (and this logging is currently there).

This turned out to be a lot nicer after the patches for bug 1086991. This will stuff URLs into the reading list no matter what, so I decided to get rid of the failed/duplicate toasts. We shouldn't ever fail to add an item, so I don't think we need that toast anymore, but maybe we should add some logic to the Java side to show a toast if you're adding something that's already in your reading list. For the _addTabToReadingList case, this still tries to download/parse the article before adding it to the reading list, since we need to do that to get the length/excerpt metadata. What I should really do next is figure out how to write some tests for all of this...

Comment on attachment 8509759 [details] [diff] [review] Consolidate logic to add articles to reading list Review of attachment 8509759 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. I kinda want to do an arrows-and-names pass through :D ::: mobile/android/chrome/content/Reader.js @@ +109,5 @@ > + // 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 Trailing comma on last item, plz @@ +127,5 @@ > + type: "Reader:AddToList", > + url: truncate(article.url, MAX_URI_LENGTH), > + title: truncate(article.title || "", MAX_TITLE_LENGTH), > + length: article.length || 0, > + excerpt: article.excerpt || "" Same

(In reply to Richard Newman [:rnewman] from comment #7) > Comment on attachment 8509759 [details] [diff] [review] > Consolidate logic to add articles to reading list > > Review of attachment 8509759 [details] [diff] [review]: > ----------------------------------------------------------------- > > LGTM. I kinda want to do an arrows-and-names pass through :D > > ::: mobile/android/chrome/content/Reader.js > @@ +109,5 @@ > > + // 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 > > Trailing comma on last item, plz I'm a non-trailing-comma kind of person, but I don't really care enough to push back. However, I will say that generally the Fennec codebase doesn't do trailing commas. Should we aim for some consistency here in the future?

(In reply to :Margaret Leibovic from comment #8) > I'm a non-trailing-comma kind of person, but I don't really care enough to > push back. However, I will say that generally the Fennec codebase doesn't do > trailing commas. Should we aim for some consistency here in the future? I think prevailing style in Moz JS code has moved to trailing commas, simply because it makes diffs so much neater, and avoids accidental bugs due to omission-then-addition. (I'm sure we've all experienced an hour of pained debugging, only to discover "syntax error on line so and so" when we added a method and forgot the comma on the previous item.) I'd be very happy to see us converge on what I think of as "modern toolkit JS".