967324 - Use promise to wrap db accesses in Reader mode

Status: RESOLVED DUPLICATE of bug 1087722

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

Description

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

After bug 918806 landed, We're able to use DOM Promise.

So I think we can refactor the function which request data (e.g. Reader.getArticleFromCache()).

Comment 1

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch

r?

Comment 2

[:Margaret Leibovic]

Can you explain the benefit of this patch? In general, we don't like to accept patches that refactor things without some clear gain (performance, ease of adding some new logic, etc.).

Comment 3

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to :Margaret Leibovic from comment #2)
> Can you explain the benefit of this patch? In general, we don't like to
> accept patches that refactor things without some clear gain (performance,
> ease of adding some new logic, etc.).

I think we get these by this change.
* Separate handling on success/error obviously. `Promise` represents the abstraction container which will fulfill with a result and provide handling points. This change can move some codes for error handling (e.g. http://hg.mozilla.org/mozilla-central/file/262e73a6b7cd/mobile/android/chrome/content/browser.js#l7691) to a part of obvious error handling.
* Stop to passing callbacks as a function's parameter. Each API gets only parameters which its really needs. However, I agree this might be mindset.

BTW, I found some point that I can polish in the patch. I'll modify it.

Comment 4

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: patch v2

Update to the patch v2.

Comment 5

[:Margaret Leibovic]

Comment on attachment 8372304 [details] [diff] [review]
patch v2

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

I would feel more comfortable with this patch if we had some tests to go with it. I think we should see if we can add some basic unit-style tests for getArticleFromCache/storeArticleInCache/removeArticleFromCache. We can use roboextender to write tests that run chrome JS, so maybe we can use that to call into those Reader methods.

You can look in here for some examples:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/roboextender/
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testHomeBanner.java

::: mobile/android/chrome/content/browser.js
@@ +7682,5 @@
>        callback(null);
>      }
>    },
>  
> +  getArticleFromCache: function Reader_getArticleFromCache(url) {

This would be a good opportunity to add some documentation explaining what this returns.

Comment 6

[:Margaret Leibovic]

Comment on attachment 8372304 [details] [diff] [review]
patch v2

Clearing review until there's at least some really basic test coverage.