1062805 - Used the shared JSON file load helper in the ringtones app

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::Ringtones, defect)

Description

[Gabriele Svelto [:gsvelto]]

+++ This bug was initially created as a clone of Bug #990047 +++

We can use the new shared helper to replace the custom code used to load a JSON file:

https://github.com/mozilla-b2g/gaia/blob/550644f70968fb07e4d3167f605d790460157c03/apps/ringtones/js/built_in_ringtones.js#L203

Comment 1

[Goutam(:goutamnair)]

Hey, I would like to work on this. Can this be assigned to me?

Comment 2

[Gabriele Svelto [:gsvelto]]

(In reply to Goutam from comment #1)
> Hey, I would like to work on this. Can this be assigned to me?

Yes, of course. I'm am the mentor of this bug so feel free to ask me if you need help.

Comment 3

[Goutam(:goutamnair)]

Do I have to replace this function with LazyLoader?

Comment 4

[Gabriele Svelto [:gsvelto]]

(In reply to Goutam from comment #3)
> Do I have to replace this function with LazyLoader?

Yes, this bug is about replacing the custom code used to load the JSON file with the LazyLoader.loadJSON() function.

Comment 5

[Goutam(:goutamnair)]

How does onload() and onerror() work with LazyLoader.getJSON(filename)?

Comment 6

[Gabriele Svelto [:gsvelto]]

(In reply to Goutam from comment #5)
> How does onload() and onerror() work with LazyLoader.getJSON(filename)?

When onload() is called we invoke the resolve() method of the promise returned by LazyLoader.getJSON() and when onerror() is called we invoke reject(). See the relevant code:

https://github.com/mozilla-b2g/gaia/blob/2308520617e46fbad7b34644a66fa3bd1e0ca38d/shared/js/lazy_loader.js#L80

Comment 7

[Goutam(:goutamnair)]

Hey, I have changed the ringtone file, but the code seems to fail in two tests of /common/vendor/mocha/mocha.js that give a timeout error. Could you help me out with this?

Comment 8

[Julien Wajsberg [:julienw]]

Goutam, can you please push your code and create a pull request? This will help helping you :)

Comment 9

[Goutam(:goutamnair)]

Attachment: PR for bug 1062805

Please review the PR

Comment 10

[Julien Wajsberg [:julienw]]

Comment on attachment 8495505 [details] [review]
PR for bug 1062805

I'm not the right reviewer for this but I think the unit tests timeout simply because the function returns a promise that's never resolved.

Please take care to have the function work the same than before your patch, and this should work fine.

Comment 11

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8495505 [details] [review]
PR for bug 1062805

Clearing my review flag. Goutam, you can ask me and Julien for feedback (and me in particular since I'm this bug's mentor ;) ) but we can't provide a review for your patch. Once the patch is working and you've got our positive feedback you will have to ask to one of the application peers or the owner. You can find them here:

https://wiki.mozilla.org/Modules/All#Ringtones

Or use the 'suggested reviewers' option.

Comment 12

[Goutam(:goutamnair)]

Attachment: PR for bug 1062805

Please review this PR

Comment 13

[Goutam(:goutamnair)]

Comment on attachment 8496035 [details]
PR for bug 1062805

>https://github.com/mozilla-b2g/gaia/pull/24479/files

Comment 14

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8496035 [details]
PR for bug 1062805

There's quite a few issues to address, ask for feedback again when you've addressed those. There's one important thing though, you could have caught a lot of these errors before review if you had used the linter on your code. Before asking for feedback again make sure that your code is linter-clean. See this post for more information on using it:

https://groups.google.com/d/msg/mozilla.dev.gaia/IvlyCwemvLk/grE6l35EVXYJ

Comment 15

[Goutam(:goutamnair)]

Do I have to defer lazy_loader.js in pick.html and manage.html instead of require the same file in built_in_ringtones.js?

Comment 16

[Gabriele Svelto [:gsvelto]]

(In reply to Goutam from comment #15)
> Do I have to defer lazy_loader.js in pick.html and manage.html instead of
> require the same file in built_in_ringtones.js?

Yes, you have to include it both in pick.html and manage.html because it will be used in both.

Comment 17

[Goutam(:goutamnair)]

Attachment: Patch for Bug 1062805

Comment 18

[Goutam(:goutamnair)]

Comment on attachment 8498654 [details]
Patch for  Bug 1062805

>https://github.com/goutamnair7/gaia/compare/LazyLoader-JSON-ringtones

Comment 19

[Goutam(:goutamnair)]

Comment on attachment 8498654 [details]
Patch for  Bug 1062805

>https://github.com/mozilla-b2g/gaia/pull/24786/files

Comment 20

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8498654 [details]
Patch for  Bug 1062805

This is generally looking good but there's still a few syntax issues to solve.

Comment 21

[Jim Porter (:squib)]

Comment on attachment 8498654 [details]
Patch for  Bug 1062805

The linter is currently failing, so r=me with that fixed and my comments on Github[1] addressed.

[1] https://github.com/mozilla-b2g/gaia/pull/24786

Comment 22

[Sudheesh Singanamalla (:ShellHacker)]

Hi Goutam,

The Linter is still failing after your latest commit. I've put up a comment on github to help you out, you could make the changes and rebase the commit and push back.

Comment 23

[Goutam(:goutamnair)]

Attachment: PR for bug

Comment 24

[Jim Porter (:squib)]

Comment on attachment 8502600 [details]
PR for bug

The indentation still needs a bit of cleanup. See my comments on Github.