Replace the custom functions used to load JSON files with the new shared helper in the system app

RESOLVED FIXED in 2.1 S5 (26sep)

Status

Firefox OS
Gaia::System
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gsvelto, Assigned: Mathieu Rhéaume, Mentored)

Tracking

unspecified
2.1 S5 (26sep)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=js])

Attachments

(1 attachment)

(Reporter)

Description

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

There's a couple of places in the system app where we can use the new shared function to load JSON files:

https://github.com/mozilla-b2g/gaia/blob/d21c76cb3cd31dfaf8745034441d6fe83dca01b9/apps/system/js/icc.js#L68
https://github.com/mozilla-b2g/gaia/blob/1bb0ea3dce70fc05fb626c553cdb21b8fd49a3d9/apps/system/js/eu_roaming_manager.js#L338
(Assignee)

Comment 1

3 years ago
Hi,

I also found JSON Load files in the following :

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/operator_variant/operator_variant.js#L189
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/ftu_ping.js#L349

Should we also convert them to LazyLoader ?

(In reply to Gabriele Svelto [:gsvelto] from comment #0)
> +++ This bug was initially created as a clone of Bug #990047 +++
> 
> There's a couple of places in the system app where we can use the new shared
> function to load JSON files:
> 
> https://github.com/mozilla-b2g/gaia/blob/
> d21c76cb3cd31dfaf8745034441d6fe83dca01b9/apps/system/js/icc.js#L68
> https://github.com/mozilla-b2g/gaia/blob/
> 1bb0ea3dce70fc05fb626c553cdb21b8fd49a3d9/apps/system/js/eu_roaming_manager.
> js#L338
(Reporter)

Comment 2

3 years ago
(In reply to Mathieu Rhéaume from comment #1)
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/
> operator_variant/operator_variant.js#L189

Yes, I think it would be nice to cover that too.

> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/ftu_ping.
> js#L349


This one is a POST operation and for now we're only dealing with 'GET' ones.
(Assignee)

Comment 3

3 years ago
Created attachment 8492745 [details] [review]
Github PR for Bug 1062803 with LazyLoader and updated deps.

Hi Gabriele,

I did 2 things concerning this bug :

1.Converted the system xhr json call to lazy loader.
2.Updated Require so that tests works.

Something weird though in Gaia-Try the Giu failed, but I tried running the test a few times on my desktop and it worked. It seems unrelated as it comes from FTU.

Thanks for taking a look.
Attachment #8492745 - Flags: feedback?(gsvelto)
(Reporter)

Comment 4

3 years ago
Comment on attachment 8492745 [details] [review]
Github PR for Bug 1062803 with LazyLoader and updated deps.

A few changes are needed, I've left the comments on the PR.
Attachment #8492745 - Flags: feedback?(gsvelto) → feedback-
(Assignee)

Comment 5

3 years ago
Comment on attachment 8492745 [details] [review]
Github PR for Bug 1062803 with LazyLoader and updated deps.

I updated the PR with the following changes :

Nit fixs : 
1. Aligned the global to fit styling
2. Readded the spaces that were removed
3. Changed .then(func()) parameter to remove 2 vars that were useless.
4. Realigned some funcs to -2 left spaces.

Thanks for taking a look into it.
Attachment #8492745 - Flags: feedback?(gsvelto)
(Reporter)

Comment 6

3 years ago
Comment on attachment 8492745 [details] [review]
Github PR for Bug 1062803 with LazyLoader and updated deps.

Looking good, now onto the proper review. Use the suggested reviews tool to find an appropriate reviewer.
Attachment #8492745 - Flags: feedback?(gsvelto)
Attachment #8492745 - Flags: feedback-
Attachment #8492745 - Flags: feedback+
(Assignee)

Comment 7

3 years ago
Hi,

I addressed the last nit you posted about a missing whitespace whitespace.

Now adding a System Owner for review.

(In reply to Gabriele Svelto [:gsvelto] from comment #6)
> Comment on attachment 8492745 [details] [review]
> Github PR for Bug 1062803 with LazyLoader and updated deps.
> 
> Looking good, now onto the proper review. Use the suggested reviews tool to
> find an appropriate reviewer.
(Assignee)

Updated

3 years ago
Attachment #8492745 - Flags: review?(alive)
Comment on attachment 8492745 [details] [review]
Github PR for Bug 1062803 with LazyLoader and updated deps.

I think it's fine but want the module author to take a look.
Attachment #8492745 - Flags: review?(alive)
Attachment #8492745 - Flags: review+
Attachment #8492745 - Flags: feedback?(josea.olivera)
Attachment #8492745 - Flags: feedback?(frsela)
Attachment #8492745 - Flags: feedback?(arthur.chen)
Comment on attachment 8492745 [details] [review]
Github PR for Bug 1062803 with LazyLoader and updated deps.

Please check my comments in github, thanks.
Attachment #8492745 - Flags: feedback?(arthur.chen) → feedback-
(Assignee)

Comment 10

3 years ago
Comment on attachment 8492745 [details] [review]
Github PR for Bug 1062803 with LazyLoader and updated deps.

I just updated the PR with the comments alive and arthur made in the PR.

The following were added :

1. Removed the useless import in index.html as per :alive feedback.
2. Added the Return Promise.resolve for eu_roaming_manager.js to make _loadjson .thenable

Thanks again !
Attachment #8492745 - Flags: feedback?(arthur.chen)
Comment on attachment 8492745 [details] [review]
Github PR for Bug 1062803 with LazyLoader and updated deps.

A function is thenable only when it returns a promise, so we should also do `return LazyLoader.getJSON(path).then...`. f=me with it fixed, thanks!
Attachment #8492745 - Flags: feedback?(arthur.chen) → feedback+
(Assignee)

Comment 12

3 years ago
Comment on attachment 8492745 [details] [review]
Github PR for Bug 1062803 with LazyLoader and updated deps.

Done.

Added a return statement in on lazy loader to return the promise.

Thanks again.
Attachment #8492745 - Flags: feedback?(arthur.chen)
Comment on attachment 8492745 [details] [review]
Github PR for Bug 1062803 with LazyLoader and updated deps.

Looks good to me, f=me.
Attachment #8492745 - Flags: feedback?(arthur.chen)
Attachment #8492745 - Flags: feedback-
Comment on attachment 8492745 [details] [review]
Github PR for Bug 1062803 with LazyLoader and updated deps.

LGTM. f=me
Attachment #8492745 - Flags: feedback?(josea.olivera) → feedback+
(Assignee)

Comment 15

3 years ago
I guess we can check-in for the patch then. Or wait for review from frsela ?

Thanks
Flags: needinfo?(alive)
(In reply to Mathieu Rhéaume from comment #15)
> I guess we can check-in for the patch then. Or wait for review from frsela ?
> 
> Thanks

Please go if you are confident enough.
Flags: needinfo?(alive)
Assignee: nobody → mathieu
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
master: https://github.com/mozilla-b2g/gaia/commit/53890df7c7253f3b739542d6cc33ebdd0d46f57e
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S5 (26sep)
Attachment #8492745 - Flags: feedback?(frsela) → feedback+
You need to log in before you can comment on or make changes to this bug.