Closed Bug 990047 Opened 6 years ago Closed 5 years ago

Add a shared function to load JSON files and use it throughout gaia to replace the existing ad-hoc functions

Categories

(Firefox OS Graveyard :: Gaia, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.1+, b2g-v2.1 fixed, b2g-v2.2 fixed)

RESOLVED FIXED
2.1 S4 (12sep)
blocking-b2g 2.1+
Tracking Status
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: gsvelto, Assigned: ifadey, Mentored)

References

Details

(Whiteboard: [lang=js])

Attachments

(1 file, 1 obsolete file)

46 bytes, text/x-github-pull-request
gsvelto
: review+
Details | Review
We have a whole bunch of different places were we re-implement in various creative ways a function that loads a JSON file, see the following lines:

https://github.com/mozilla-b2g/gaia/blob/530b6671255e50b8f9b45c2ef96c468e3a374422/shared/js/keyboard_helper.js#L328
https://github.com/mozilla-b2g/gaia/blob/530b6671255e50b8f9b45c2ef96c468e3a374422/shared/js/tz_select.js#L9
https://github.com/mozilla-b2g/gaia/blob/530b6671255e50b8f9b45c2ef96c468e3a374422/apps/communications/contacts/js/fb/fb_query.js#L50
https://github.com/mozilla-b2g/gaia/blob/530b6671255e50b8f9b45c2ef96c468e3a374422/apps/communications/ftu/js/language.js#L76
https://github.com/mozilla-b2g/gaia/blob/530b6671255e50b8f9b45c2ef96c468e3a374422/apps/costcontrol/js/config/config_manager.js#L119
https://github.com/mozilla-b2g/gaia/blob/530b6671255e50b8f9b45c2ef96c468e3a374422/apps/ringtones/js/pick.js#L102
https://github.com/mozilla-b2g/gaia/blob/530b6671255e50b8f9b45c2ef96c468e3a374422/apps/settings/js/utils.js#L74
https://github.com/mozilla-b2g/gaia/blob/530b6671255e50b8f9b45c2ef96c468e3a374422/apps/system/js/icc.js#L68
https://github.com/mozilla-b2g/gaia/blob/530b6671255e50b8f9b45c2ef96c468e3a374422/apps/system/js/operator_variant/operator_variant.js#L145
https://github.com/mozilla-b2g/gaia/blob/530b6671255e50b8f9b45c2ef96c468e3a374422/apps/wallpaper/js/pick.js#L21
https://github.com/mozilla-b2g/gaia/blob/530b6671255e50b8f9b45c2ef96c468e3a374422/shared/resources/apn/query.js#L274

It's about time we'd have a generic, shared version for all to use. Bonus points if it's promised based.
Whiteboard: [mentor=:gsvelto][lang=javascript]
This could be implemented within LazyLoader (which is not promise based though).
I would like to take this. We need to decide 2 things:

1) Whether the generic function should use callback or promise.
2) Where should we place it. There's one suggestion from @rik "lazy_loader.js". I will take a look at shared code thoroughly and see where should it go.

Here's what I am thinking about its implementation. It will accept options hash containing request type, response type, success/error (if callbacks are used), etc.
We should definitely use promises nowadays.
@julienw: Thanks for the feedback.

I just gone through the shared code and it looks like lazy_loader is suitable place for this. Soon I will push a pull request for review.
(In reply to Julien Wajsberg [:julienw] from comment #3)
> We should definitely use promises nowadays.

+1
(In reply to Fawad Hassan from comment #4)
> I just gone through the shared code and it looks like lazy_loader is
> suitable place for this. Soon I will push a pull request for review.

Since you're working on it I'm assigning the bug to you. For simplifying review and testing I would suggest to split the work in multiple patches: one with the new functionality + unit tests and then one patch for every app that needs to use the new function. This will also make rebasing your branch on top of master simpler, with less conflicts to resolve at the same time.
Assignee: nobody → ifadey
Status: NEW → ASSIGNED
I added the functionality in lazy_loader and wrote the unit tests as well. But I am confused how to run those unit tests.

There's an app called "sharedtest" in which I wrote the unit test but I can't find "Shared Tests" app in the menu when I run the gaia.
https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/Gaia_unit_tests should have all the instructions to run the unit tests. If anything is not clear, ask and we shall update the documentation.
Turns out LazyLoader is probably not a great name for this, but I suppose we can use it for now :) It also probably doesn't make sense to use both promises and the current lazy loader callback style in the same file. My recommendation would be to use the same style we use today, port everything to promises (a huge task), or use a new simple file.
(In reply to Anthony Ricaud (:rik) from comment #8)
> https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/
> Gaia_unit_tests should have all the instructions to run the unit tests. If
> anything is not clear, ask and we shall update the documentation.

Thanks a lot. I will take a look at this.


(In reply to Kevin Grandon :kgrandon from comment #9)
> It also probably doesn't make sense to use both
> promises and the current lazy loader callback style in the same file.

Yes that was the reason I asked whether we should go with callback or promises. I will push the code with promises for now and let's see what others say in code review.
(In reply to Kevin Grandon :kgrandon from comment #9)
> Turns out LazyLoader is probably not a great name for this, but I suppose we
> can use it for now :) It also probably doesn't make sense to use both
> promises and the current lazy loader callback style in the same file. My
> recommendation would be to use the same style we use today, port everything
> to promises (a huge task), or use a new simple file.

My idea was to support both callbacks and promises in the existing functions (should be possible quite easily) so that we don't need to update all the code that uses it. This was not to do in this bug, but this was the direction I wanted to move to, and so using promises for the new function made sense to me.

Now, Kevin, since you're the original author of the file, your opinion counts more :)
Sent a pull request. I added a generic function with which we can perform any http request (GET, POST, PUT, DELETE). Currently I haven't supported Mozilla specific properties. I wrote a unit test for only GET request (specifically for JSON).

I have few questions now.

1) Should I support Mozilla specific flags in the generic function like mozAnon, mozSystem?
2) Should I write unit tests for POST, PUT, DELETE as well? If yes then how can I do it?
(In reply to Fawad Hassan from comment #12)
> Sent a pull request. I added a generic function with which we can perform
> any http request (GET, POST, PUT, DELETE). Currently I haven't supported
> Mozilla specific properties. I wrote a unit test for only GET request
> (specifically for JSON).

I found your pull request on GitHub but you should attach it to this bug: create a new attachment for this bug, chose "Paste text as attachment" and paste the pull request URL then give a name to the attachment. Also for the pull request comment you should use the form "Bug 990047 - <your comment here>". We always do this to keep track of which bug was fixed by which patch.

> I have few questions now.
> 
> 1) Should I support Mozilla specific flags in the generic function like
> mozAnon, mozSystem?
> 2) Should I write unit tests for POST, PUT, DELETE as well? If yes then how
> can I do it?

I've looked at your code and it seems a little too generic. If you look at the various bits of code I pointed to in comment 0 you'll notice that they're all 'GET' requests with the responseType set to 'json'. So for now implementing 'GET' alone should be fine; I was content with a loadJSON() function taking an URL and returning a Promise but I like your more flexible approach. One thing I would do differently is taking out the |href| field from the |options| object and passing it as a separate argument as it's not really optional. Last but not least you should provide a comment describing how your new function works and especially what parameters can be stored in |options|.
To be honest, I don't really see a lot of value is us creating a generic wrapper for XMLHttpRequest like this. I think we would probably be better off if we just generated JS files (instead of JSON) that just set their config on some window object, then we could just use lazy loader as-is.

If we do want to create a small utility for JSON, I would recommend using LazyLoader.json() which is only in charge of getting/parsing JSON.
Thanks for your feedback guys. Yes I understand it will be mostly used for GET json only. Let me push another patch for LazyLoader.json() and remove the generic function.

@gsvelto: I didn't knew about attachment. I will attach it as soon as I push another patch.
In terms of API, I think this function should still be called .get() in case someday we want to get other kind of stuff.
Amended my patch. Everyone's feedback is entertained. Kindly take a look now.
(In reply to Fawad Hassan from comment #18)
> Amended my patch. Everyone's feedback is entertained. Kindly take a look now.

I've left some feedback on GitHub, I think we're on the right track here.
(In reply to Gabriele Svelto [:gsvelto] from comment #19)
> I've left some feedback on GitHub, I think we're on the right track here.

Amended the patch containing your feedback.
Mentor: gsvelto
Whiteboard: [mentor=:gsvelto][lang=javascript] → [lang=javascript]
Whiteboard: [lang=javascript] → [lang=js]
I assume this change is no more needed because no one replied after my last comment.
Secondly some of the team members oppose the idea to adding such function in lazy loader file.
(In reply to Fawad Hassan from comment #21)
> I assume this change is no more needed because no one replied after my last
> comment.
> Secondly some of the team members oppose the idea to adding such function in
> lazy loader file.

Sorry Fawad, since there wasn't a review flag set on your patch it fell through the cracks and I forgot to answer. I've left an additional comment on GitHub but from my POV your patch is good to go.

Kevin proposed a different solution in comment 14 (using JS files directly instead of JSON) but that would require inspecting and modifying all current uses. I know that some of the current uses rely on JSON files that are produced by external tools and we'd need to modify those too (or pre-parse their output from JSON to JS) so I don't think it's a viable solution.

Still I'd like to hear his opinion on this. My take is that we could merge the current promise-based patch (plus the nit I've added) and then proceed to modify existing callers. At the very least this would save us quite a bit of code that's duplicated left and right in our codebase which I think is a good reason for taking this.
Flags: needinfo?(kgrandon)
I think I'm warming up to the idea, so this seems fine to me. I think Gabriele would make a good reviewer here, so please mark him as reviewer. Thanks!
Flags: needinfo?(kgrandon)
I am sorry but I don't know how to flag someone for review?
(In reply to Fawad Hassan from comment #24)
> I am sorry but I don't know how to flag someone for review?

Click on the "Details" button on the right side of the attachment entry in this bug. Then in the details screen look for the "Review" flag, it's the first one of the list. Set it to the question mark (that's asking for review) and select me as the reviewer.

You can find more details in this FAQ, it contains important information on reviews and the development flow: https://developer.mozilla.org/en/docs/Code_Review_FAQ
Attached file Review request (obsolete) —
Attachment #8480825 - Flags: review?(gsvelto)
Comment on attachment 8480825 [details]
Review request

I meant that you should have set the flag on the existing attachment :)

That's attachment 8428298 [details] [review], I'm going to do the review there anyway.
Attachment #8480825 - Attachment is obsolete: true
Attachment #8480825 - Flags: review?(gsvelto)
Comment on attachment 8428298 [details] [review]
Pull request for bug 990047

I've left a couple more review comments in the PR. Once they're addressed ask for review again and I'll r+ the patch. Thanks for your work here!
Attachment #8428298 - Flags: review?(gsvelto)
Comment on attachment 8428298 [details] [review]
Pull request for bug 990047

Looks good to me thanks. I'll merge your PR tomorrow after having opened the bugs for replacing our current uses with your new function.
Attachment #8428298 - Flags: review?(gsvelto) → review+
Thanks a lot Gabriele for your guidance. Next I will start refactoring get json code in different parts. You will create new ticket for this next step or it will be the same?
Blocks: 1062269
Bug 1062269 is the first follow up where to use the patch, I haven't merged it yet because there was a test failure in your last try run and I've re-triggered it. I want to make sure it's unrelated before merging. I'll file more bug as soon as I have a minute.
I looked at the failures. I guess these failures are because of old code. I haven't fetch/rebase it to latest master.
(In reply to Fawad Hassan from comment #32)
> I looked at the failures. I guess these failures are because of old code. I
> haven't fetch/rebase it to latest master.

Yeah, the issue is definitely unrelated. Merged to gaia/master f04a5958b6da501a59dc1edaf580361004258f05

https://github.com/mozilla-b2g/gaia/commit/f04a5958b6da501a59dc1edaf580361004258f05

Thanks!
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Blocks: 1062783
Blocks: 1062787
Blocks: 1062798
Blocks: 1062803
Blocks: 1062805
Blocks: 1062806
Blocks: 1062807
reverted for causing gaia unit test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=47395775&tree=B2g-Inbound
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 1062829
(In reply to Carsten Book [:Tomcat] from comment #34)
> reverted for causing gaia unit test failures like
> https://tbpl.mozilla.org/php/getParsedLog.php?id=47395775&tree=B2g-Inbound

Yuck, my fault. We should have rebased first just in case and we would have caught this. Fawad can you rebase your patch and ensure that the error in comment 34 goes away? Ask me for review again once you're done. I'll keep filing follow up bugs in the meantime.
Flags: needinfo?(ifadey)
(In reply to Gabriele Svelto [:gsvelto] from comment #35)
> 
> Yuck, my fault. We should have rebased first just in case and we would have
> caught this. Fawad can you rebase your patch and ensure that the error in
> comment 34 goes away? Ask me for review again once you're done. I'll keep
> filing follow up bugs in the meantime.

Oops! Let me verify it with latest code.
Yes the test is failing because of "property" method. Let me change the test a bit.
Flags: needinfo?(ifadey)
Gabriele,
Unit tests are passing now. Check the new pull request.
(In reply to Fawad Hassan from comment #39)
> Unit tests are passing now. Check the new pull request.

OK, the Try run is not entirely green yet. I've retriggered the Gij tests, let's see what happens.
(In reply to Gabriele Svelto [:gsvelto] from comment #40)
> OK, the Try run is not entirely green yet. I've retriggered the Gij tests,
> let's see what happens.

All green now :)
(In reply to Fawad Hassan from comment #41)
> All green now :)

The integration tests (Gij) were still red for a different reason and I don't understand why. I'll try to re-run everything locally. Sorry for the delay but I'm a bit nervous at merging without a completely green run.
I don't see any red.

Here's the pull request:
https://github.com/mozilla-b2g/gaia/pull/23725

and here are the results of last run (all green):
https://tbpl.mozilla.org/?rev=efe8f90a2d1ae80c792914f515d4bcfb3190ba80&tree=Gaia-Try
(In reply to Fawad Hassan from comment #43)
> I don't see any red.
> 
> Here's the pull request:
> https://github.com/mozilla-b2g/gaia/pull/23725
> 
> and here are the results of last run (all green):
> https://tbpl.mozilla.org/
> ?rev=efe8f90a2d1ae80c792914f515d4bcfb3190ba80&tree=Gaia-Try

Yes indeed, I may have followed the wrong link :-/ I did a test myself and it turned out green too:

https://tbpl.mozilla.org/?rev=65bf2141721f63d9be3667cb40b7315e95aa4e62&tree=Gaia-Try

Let's merge it.
Merged to gaia/master 1c5c7057a821b863efbcaaa63262661d0251ecf1

https://github.com/mozilla-b2g/gaia/commit/1c5c7057a821b863efbcaaa63262661d0251ecf1

Thanks for your work here.
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
[Blocking Requested - why for this release]:

Required for https://bugzilla.mozilla.org/show_bug.cgi?id=1053261 which has already been upflifted

Low risk
blocking-b2g: --- → 2.1?
Comment on attachment 8428298 [details] [review]
Pull request for bug 990047

This introduced a function which is used in 2.1, but the function itself never got uplifted - https://bugzilla.mozilla.org/show_bug.cgi?id=1053261

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): https://bugzilla.mozilla.org/show_bug.cgi?id=1053261
[User impact] if declined: Browser will never show history
[Testing completed]: Manual testing
[Risk to taking this patch] (and alternatives if risky): Very low risk
[String changes made]:
Attachment #8428298 - Flags: approval-gaia-v2.1?
Blocks: 1053261
Blocking a blocker and function is already used in 2.1
blocking-b2g: 2.1? → 2.1+
Comment on attachment 8428298 [details] [review]
Pull request for bug 990047

Sorry, forgot to add a approver
Attachment #8428298 - Flags: approval-gaia-v2.1? → approval-gaia-v2.1?(fabrice)
(In reply to Dale Harvey (:daleharvey) from comment #49)
> Comment on attachment 8428298 [details] [review]
> Pull request for bug 990047
> 
> Sorry, forgot to add a approver

No need for that, we use queries that just check the flag ;)

Do we have tests to catch the "Browser will never show history" bug?
I was gonna wait since I didnt want the test being uplifted first and making 2.1 red, but yup added a test in review to make sure this doesnt regress - https://bugzilla.mozilla.org/show_bug.cgi?id=1067573 will uplift after this one.
Attachment #8428298 - Flags: approval-gaia-v2.1?(fabrice) → approval-gaia-v2.1+
You need to log in before you can comment on or make changes to this bug.