Used the shared JSON file load helper in the ringtones app

RESOLVED WONTFIX

Status

Firefox OS
Gaia::Ringtones
RESOLVED WONTFIX
4 years ago
2 years ago

People

(Reporter: gsvelto, Assigned: goutamnair, Mentored)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=js])

Attachments

(2 attachments, 2 obsolete attachments)

52 bytes, text/html
gsvelto
: feedback-
Details
47 bytes, text/x-github-pull-request
squib
: review-
Details
+++ 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
(Assignee)

Comment 1

4 years ago
Hey, I would like to work on this. Can this be assigned to me?
(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.
Assignee: nobody → goutamnair7
Status: NEW → ASSIGNED
(Assignee)

Comment 3

4 years ago
Do I have to replace this function with LazyLoader?
(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.
(Assignee)

Comment 5

4 years ago
How does onload() and onerror() work with LazyLoader.getJSON(filename)?
(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
(Assignee)

Comment 7

4 years ago
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?
Goutam, can you please push your code and create a pull request? This will help helping you :)
(Assignee)

Comment 9

4 years ago
Created attachment 8495505 [details] [review]
PR for bug 1062805

Please review the PR
Attachment #8495505 - Flags: review?(gsvelto)
Attachment #8495505 - Flags: review?(felash)
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.
Attachment #8495505 - Flags: review?(felash) → review-
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.
Attachment #8495505 - Flags: review?(gsvelto)
(Assignee)

Comment 12

4 years ago
Created attachment 8496035 [details]
PR for bug 1062805

Please review this PR
Attachment #8496035 - Flags: review?(gsvelto)
(Assignee)

Comment 13

4 years ago
Comment on attachment 8496035 [details]
PR for bug 1062805

>https://github.com/mozilla-b2g/gaia/pull/24479/files
Attachment #8496035 - Attachment mime type: text/plain → text/html
Attachment #8495505 - Attachment is obsolete: true
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
Attachment #8496035 - Flags: review?(gsvelto) → feedback-
(Assignee)

Comment 15

3 years ago
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?
Flags: needinfo?(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.
Flags: needinfo?(gsvelto)
(Assignee)

Comment 17

3 years ago
Created attachment 8498654 [details]
Patch for  Bug 1062805
Attachment #8498654 - Flags: feedback?(gsvelto)
(Assignee)

Comment 18

3 years ago
Comment on attachment 8498654 [details]
Patch for  Bug 1062805

>https://github.com/goutamnair7/gaia/compare/LazyLoader-JSON-ringtones
Attachment #8498654 - Attachment mime type: text/plain → text/html
Comment on attachment 8498654 [details]
Patch for  Bug 1062805

This is generally looking good but there's still a few syntax issues to solve.
Attachment #8498654 - Flags: feedback?(gsvelto) → feedback+

Comment 21

3 years ago
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
Attachment #8498654 - Flags: review+
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.
(Assignee)

Comment 23

3 years ago
Created attachment 8502600 [details]
PR for bug
Attachment #8498654 - Attachment is obsolete: true
Attachment #8502600 - Flags: review?(squibblyflabbetydoo)
(Assignee)

Updated

3 years ago
Attachment #8502600 - Attachment mime type: text/plain → text/html
(Assignee)

Updated

3 years ago
Attachment #8502600 - Attachment mime type: text/html → text/x-github-pull-request

Comment 24

3 years ago
Comment on attachment 8502600 [details]
PR for bug

The indentation still needs a bit of cleanup. See my comments on Github.
Attachment #8502600 - Flags: review?(squibblyflabbetydoo) → review-
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.