Closed Bug 1087722 Opened 7 years ago Closed 7 years ago

Convert reader mode API to use promises

Categories

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

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 36

People

(Reporter: bnicholson, Assigned: Margaret)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch WIP reader mode promises (obsolete) — Splinter Review
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
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! :)
Blocks: readerv2
I started to try to get this to work.
Assignee: nobody → margaret.leibovic
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.
Attachment #8509915 - Attachment is obsolete: true
Attachment #8516454 - Flags: feedback?(bnicholson)
Depends on: 1007409
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.
Attachment #8516454 - Attachment is obsolete: true
Attachment #8516454 - Flags: feedback?(bnicholson)
Attachment #8517127 - Flags: review?(rnewman)
Attachment #8517127 - Flags: review?(bnicholson)
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!
Attachment #8517127 - Flags: review?(rnewman) → review+
(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
(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 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.
Attachment #8517127 - Flags: review?(bnicholson) → review+
(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 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.
(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.
Depends on: 1094485
https://hg.mozilla.org/mozilla-central/rev/7364b11dd6a9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Duplicate of this bug: 967324
Depends on: 1096612
Depends on: 1097906
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.