Open Bug 990066 Opened 6 years ago Updated 6 years ago

Update generator function declarations in HomeProvider.jsm

Categories

(Firefox for Android :: Data Providers, defect)

ARM
Android
defect
Not set

Tracking

()

REOPENED
Firefox 32

People

(Reporter: Margaret, Assigned: zliu.ur, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [lang=js])

Attachments

(1 file, 1 obsolete file)

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
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
(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!
Assignee: nobody → 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
(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
Summary: Use Task.async in HomeProvider.jsm → Update generator function declarations in HomeProvider.jsm
Hi Zack, I just wanted to check in and see how things are going here. Let me know if you need any help!
Flags: needinfo?(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
Flags: needinfo?(zliu.ur)
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 :)
Attached patch add-star-for-generator-functions (obsolete) — Splinter Review
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
Attachment #8428860 - Flags: review?(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!
Attachment #8428860 - Flags: review?(margaret.leibovic) → feedback+
(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
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
Attachment #8429056 - Flags: review?(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!
Attachment #8429056 - Flags: review?(margaret.leibovic) → review+
Attachment #8428860 - Attachment is obsolete: true
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.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/a8aeb5b375fb
Keywords: checkin-needed
Whiteboard: [mentor=margaret][lang=js] → [mentor=margaret][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/a8aeb5b375fb
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=margaret][lang=js][fixed-in-fx-team] → [mentor=margaret][lang=js]
Target Milestone: --- → Firefox 32
Depends on: 1017554
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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 :)
Mentor: margaret.leibovic
Whiteboard: [mentor=margaret][lang=js] → [lang=js]
You need to log in before you can comment on or make changes to this bug.