990066 - Update generator function declarations in HomeProvider.jsm

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: Data Providers, defect)

Description

[:Margaret Leibovic]

Instead of returning Task.spawn all over the place, we could use Task.async. See this post for more details:

http://mykzilla.blogspot.com/2014/03/simplify-asynchronous-method.html

Comment 1

[zliu.ur]

Hi Margaret:

Can I try to work on this? This task seems feasible for a newb like me :)

(In reply to :Margaret Leibovic from comment #0)
> Instead of returning Task.spawn all over the place, we could use Task.async.
> See this post for more details:
> 
> http://mykzilla.blogspot.com/2014/03/simplify-asynchronous-method.html

Comment 2

[:Margaret Leibovic]

(In reply to zliu.ur from comment #1)
> Hi Margaret:
> 
> Can I try to work on this? This task seems feasible for a newb like me :)
> 
> (In reply to :Margaret Leibovic from comment #0)
> > Instead of returning Task.spawn all over the place, we could use Task.async.
> > See this post for more details:
> > 
> > http://mykzilla.blogspot.com/2014/03/simplify-asynchronous-method.html

Sure thing! I haven't used Task.async myself, so you're venturing into uncharted territory, but I think that blog post does a good job outlining how to use it.

Here are some other docs that would also help you get familiar with the code in HomeProvider.jsm:
https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/HomeProvider.jsm
https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Task.jsm
https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Promise.jsm

This code's asynchronous nature can be confusing, so let me know if you need help!

Comment 3

[zliu.ur]

Hi Margaret:

I am new to javascript and did a little bit study on the bug. So far I have 3 questions.

1. Should I remove all the the Task.spawn() and replace them with the Task.async as in 
http://mykzilla.blogspot.com/2014/03/simplify-asynchronous-method.html?

2. The Task.spawn in the HomeProvider.jsm does not use * as generator function, but they all have yield keyword in the body, should I add * to the function?

3. After I made some modifications, the whole project can pass build and can run on my android tablet. But how can I test these specific functions? How can I map a function to a functionality in the daily use of the app?

Thanks a lot for your help

Regards

Zack

Comment 4

[:Margaret Leibovic]

(In reply to zliu.ur from comment #3)

> 1. Should I remove all the the Task.spawn() and replace them with the
> Task.async as in 
> http://mykzilla.blogspot.com/2014/03/simplify-asynchronous-method.html?

You'll want to look through the Task.spawn calls on a case-by-case basis to see where it would be appropriate to swap this in.

However, I just looked more closely through the code, and I'm not actually sure it makes sense for us to use Task.async, since we would still want to keep these helper methods that current wrap the Task.spawn calls. I'm sorry, I should have looked into this more closely before filing this bug :(

> 2. The Task.spawn in the HomeProvider.jsm does not use * as generator
> function, but they all have yield keyword in the body, should I add * to the
> function?

I hadn't seen this syntax before, but doing a bit of research, I found that it's the new ECMA spec for generators. Although it's currently optional, it's good practice to use to prepare for the day it becomes non-optional. So yes, we should update that. Good find!

Let's morph this bug to just be about addressing that issue.

> 3. After I made some modifications, the whole project can pass build and can
> run on my android tablet. But how can I test these specific functions? How
> can I map a function to a functionality in the daily use of the app?

You should run testHomeProvider, which is designed to exercise these calls:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testHomeProvider.js

This is run through the robocop test harness, which you can learn how to run here:
https://wiki.mozilla.org/Auto-tools/Projects/Robocop#Running_tests

Comment 5

[:Margaret Leibovic]

Hi Zack, I just wanted to check in and see how things are going here. Let me know if you need any help!

Comment 6

[zliu.ur]

(In reply to :Margaret Leibovic from comment #5)
> Hi Zack, I just wanted to check in and see how things are going here. Let me
> know if you need any help!

Hi Margaret:

Sorry for the delay, I was busy with the final exams for the last few weeks. 
I was managed to get the automatic test up and running.

In you previous reply, you said 

"However, I just looked more closely through the code, and I'm not actually sure it makes sense for us to use Task.async, since we would still want to keep these helper methods that current wrap the Task.spawn calls. I'm sorry, I should have looked into this more closely before filing this bug :("

Should I go ahead and modify the generators?

Thanks for your advice.

Zack

Comment 7

[:Margaret Leibovic]

No problem! I hope your exams went well :)

Yes, I think we should just go ahead and modify the generators, so this turned out to be a pretty simple bug.

Once you have a patch ready, you can follow these steps to format it and attach it to the bug:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Then you should set review? to me, and I'll review it for you and help you land it in the tree.

Then I can suggest more bugs for you to work on if you're interested :)

Comment 8

[zliu.ur]

Attachment: add-star-for-generator-functions

Hi margaret:
I did the modifications and that passed the testHomeProvider test in Robocop. Please see the patch in the attachment.

If there are any more bugs that is suitable for me, please feel free to let me know

Regards

Zack

Comment 9

[:Margaret Leibovic]

Comment on attachment 8428860 [details] [diff] [review]
add-star-for-generator-functions

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

::: mobile/android/modules/HomeProvider.jsm
@@ +203,5 @@
>  /**
>   * Creates the database schema.
>   */
>  function createDatabase(db) {
> +  return Task.spawn(function *create_database_task() {

Looking through the rest of the code base, our normal style is to put the asterisk directly at the end of the function keyword. It also seems like we mostly use anonymous generator functions with Task.jsm, so this would be more consistent as:

Task.spawn(function* () { ... });

Could you update the patch to do that instead? Other than that, this patch is looking good!

Comment 10

[:Margaret Leibovic]

(In reply to zliu.ur from comment #8)

> If there are any more bugs that is suitable for me, please feel free to let
> me know

If you're interested in working on more bugs related to these home panels APIs, here's a list of mentor bugs that might interest you:
https://bugzilla.mozilla.org/buglist.cgi?quicksearch=blocked%3A941312%20sw%3Amentor%20%40nobody&list_id=10299494

Comment 11

[zliu.ur]

Attachment: add-star-for-generator-functions-1.

Hi margaret:

I updated the generator functions and upload the patch to the attachment, please help me review it and let me know if you have any feedback

Thanks

Regards

Zack

Comment 12

[:Margaret Leibovic]

Comment on attachment 8429056 [details] [diff] [review]
add-star-for-generator-functions-1.

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

Looks good, thanks!

Comment 13

[:Margaret Leibovic]

I made a try server run to make sure this doesn't cause any test failures:
https://tbpl.mozilla.org/?tree=Try&rev=07ce2a72a0da

If the tests pass, we can set the "checkin-needed" keyword on this bug to have someone land this patch.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/a8aeb5b375fb

Comment 16

[:Margaret Leibovic]

Hey Zack, I had to back out this patch because it caused bug 1017554. I suspect that these generator functions are not doing what we expect and causing us not to call refreshDataset, or causing us to not actually save data properly. Fixing this will require some investigation.

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

Comment 17

[:Margaret Leibovic]

Also, bug 968530 would be another great bug to work on. If that had been fixed, we would have caught this problem in an automated test :)

Comment 18

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.