1087722 - Convert reader mode API to use promises

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[Brian Nicholson (:bnicholson)]

Our reader mode logic currently suffers from callback hell, and it's pretty difficult to follow the code. We should be able to mitigate this by converting the API to promises.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: WIP reader mode promises

A non-functional WIP progress for converting the API to use promises. A few observations:
* Reduces the reader code by (exactly!) 100 lines
* Reduces the indentation levels in several places
* Cached requests are much easier to manage (simply return the existing promise for a given resource -- no more _runCallbacksAndFinish)
* Each method flows linearly and is easy to read

Comment 2

[:Margaret Leibovic]

Thanks for dropping in your WIP. This is changing quite a bit with my work in bug 1086991 and bug 1085685, but it would be nice to move towards this Promise-based API. I'm just trying to disentangle things one bit at a time! :)

Comment 3

[:Margaret Leibovic]

I started to try to get this to work.

Comment 4

[:Margaret Leibovic]

Attachment: WIP - Convert reader mode API to use promises

This seems to mostly work, except for opening an article from the reading list. It seems like something is wrong with parseDocumentFromURL. Or maybe something is wrong with the async logic in aboutReader.js? I tried using the debugger to investigate this, but I wasn't able to figure out how to follow exactly what's going on.

Comment 5

[:Margaret Leibovic]

Attachment: Convert reader mode API to use promises

I'm sorry this patch has grown to do a bit more refactoring, but I think it's worth it, since I consolidated some of the functions to create a smaller API surface area. I think the refactoring we've been doing in other bugs, as well as the conversion to a cleaner promise-based API really exposed how much redundant logic we have.

I removed the requests callback logic, since it doesn't really seem necessary, given that we won't often be kicking off multiple requests for the same URL in a short time frame, but I can add that back if you think it's still worthwhile. If I were to put that back, I would put it in _downloadAndParseDocument.

I updated the one test we currently have, but I want to add more tests here, especially some tests that parse loaded tabs as well.

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 8517127 [details] [diff] [review]
Convert reader mode API to use promises

Review of attachment 8517127 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine to me, so I hope Brian catches the stuff I missed!

::: mobile/android/chrome/content/Reader.js
@@ +41,1 @@
>        UITelemetry.addEvent("save.1", "pageaction", null, "reader");

You probably want:

  .then(() => UITelemetry.addEvent(…),
        e => Cu.reportError("E…"));

@@ +97,3 @@
>      let tab = BrowserApp.getTabForId(tabID);
>      if (!tab) {
> +      throw("Can't add tab to reading list because no tab found for ID: " + tabID);

Are you trying to call the `throw` method of the generator?

Or do you mean

  throw new Error("Can't add tab…");

?

::: mobile/android/chrome/content/browser.js
@@ -894,5 @@
>      HealthReportStatusListener.uninit();
>      CharacterEncoding.uninit();
>      SearchEngines.uninit();
>      RemoteDebugger.uninit();
> -    Reader.uninit();

The Reader module still registers for an observer notification. That implies that we should still have an uninit() here, unless you're making the broader assertion that Fennec modules never clean up their observers.

Explain, plz!

Comment 7

[:Margaret Leibovic]

(In reply to Richard Newman [:rnewman] from comment #6)

> @@ +97,3 @@
> >      let tab = BrowserApp.getTabForId(tabID);
> >      if (!tab) {
> > +      throw("Can't add tab to reading list because no tab found for ID: " + tabID);
> 
> Are you trying to call the `throw` method of the generator?
> 
> Or do you mean
> 
>   throw new Error("Can't add tab…");
> 
> ?

Sorry, this is probably leftover from a different version when this wasn't using Task.async.

> ::: mobile/android/chrome/content/browser.js
> @@ -894,5 @@
> >      HealthReportStatusListener.uninit();
> >      CharacterEncoding.uninit();
> >      SearchEngines.uninit();
> >      RemoteDebugger.uninit();
> > -    Reader.uninit();
> 
> The Reader module still registers for an observer notification. That implies
> that we should still have an uninit() here, unless you're making the broader
> assertion that Fennec modules never clean up their observers.
> 
> Explain, plz!

I went a little too refactor-heavy here, I should just remove the requests part of uninit and leave the observer removals in place for now.

However, we should have a bigger conversation about the value of this shutdown implementation, because given the way the Android lifecycle works, we aren't guaranteed to call this, and if we do, we don't have as much time as we want to clean things up. 

In fact, I see us calling BrowserApp.startup in the window onload [1], but I don't even see us calling BrowserApp.shutdown anywhere. Perhaps we can file a bug to just kill all of this.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.xul#9

Comment 8

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #7)

> However, we should have a bigger conversation about the value of this
> shutdown implementation, because given the way the Android lifecycle works,
> we aren't guaranteed to call this, and if we do, we don't have as much time
> as we want to clean things up. 
> 
> In fact, I see us calling BrowserApp.startup in the window onload [1], but I
> don't even see us calling BrowserApp.shutdown anywhere. Perhaps we can file
> a bug to just kill all of this.
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.xul#9

I filed bug 1094234.

Comment 9

[Brian Nicholson (:bnicholson)]

Comment on attachment 8517127 [details] [diff] [review]
Convert reader mode API to use promises

Review of attachment 8517127 [details] [diff] [review]:
-----------------------------------------------------------------

Yay promises!

::: mobile/android/base/tests/testReadingListCache.js
@@ +45,5 @@
>    let article = yield Reader.getArticleFromCache(ARTICLE_URI);
>    do_check_eq(article, null);
>  });
>  
> +add_task(function test_parse_article() {

function -> function*

::: mobile/android/chrome/content/Reader.js
@@ +143,5 @@
> +   *
> +   * @param url The article URL.
> +   * @param tabId (optional) The id of the tab where we can look for a saved article.
> +   * @return Promise
> +   * @resolve JS object representing the article, or null if no article is found.

I'm not familiar with this doc style, but the vast majority of existing code in the tree uses the following format:

@return {Promise}
@resolves ...
@rejects ...

So you may want to use that instead.

Comment 10

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #9)

> ::: mobile/android/chrome/content/Reader.js
> @@ +143,5 @@
> > +   *
> > +   * @param url The article URL.
> > +   * @param tabId (optional) The id of the tab where we can look for a saved article.
> > +   * @return Promise
> > +   * @resolve JS object representing the article, or null if no article is found.
> 
> I'm not familiar with this doc style, but the vast majority of existing code
> in the tree uses the following format:
> 
> @return {Promise}
> @resolves ...
> @rejects ...
> 
> So you may want to use that instead.

I remember searching through the tree to look for examples, but maybe I still managed to copy/paste the wrong thing. I'll update for consistency.

Comment 11

[Brian Nicholson (:bnicholson)]

Comment on attachment 8517127 [details] [diff] [review]
Convert reader mode API to use promises

Review of attachment 8517127 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/content/Reader.js
@@ +356,5 @@
> +
> +    try {
> +      this.log("Parsing response with Readability");
> +      let uri = Services.io.newURI(url, null, null);
> +      let article = yield this._readerParse(uri, doc);

While here, it might be worth taking a look at https://bugzilla.mozilla.org/show_bug.cgi?id=1091256#c4. What happens if the worker throws? Will the failure end up making it back to _readerParse to be rejected, or is it just silently ignored? If the latter, that means we're leaking a <browser> with a page worth of content every time Readability/JSDOMParser throws, which would be terrible. 

If this is actually happening, simply wrapping readerWorker.js in a try/catch should be enough to fix it.

Comment 12

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #11)
> Comment on attachment 8517127 [details] [diff] [review]
> Convert reader mode API to use promises
> 
> Review of attachment 8517127 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/chrome/content/Reader.js
> @@ +356,5 @@
> > +
> > +    try {
> > +      this.log("Parsing response with Readability");
> > +      let uri = Services.io.newURI(url, null, null);
> > +      let article = yield this._readerParse(uri, doc);
> 
> While here, it might be worth taking a look at
> https://bugzilla.mozilla.org/show_bug.cgi?id=1091256#c4. What happens if the
> worker throws? Will the failure end up making it back to _readerParse to be
> rejected, or is it just silently ignored? If the latter, that means we're
> leaking a <browser> with a page worth of content every time
> Readability/JSDOMParser throws, which would be terrible. 
> 
> If this is actually happening, simply wrapping readerWorker.js in a
> try/catch should be enough to fix it.

Yup, if there's an exception in readerWorker we never hear back from it in Reader.js. We should definitely add that try/catch. I can file a follow-up bug to fix that.

Looking at this logic more closely, I wonder if _readerParse even be throwing. I feel like it should just log any problems and return null if it wasn't able to get an article, since we use a null article to represent the "no valid article data" case.

I'll file a bug about better error handling here, where we can address both of these issues.

Comment 13

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/7364b11dd6a9

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7364b11dd6a9