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

Status: RESOLVED FIXED

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

Description

[Gabriele Svelto [:gsvelto]]

+++ 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

Comment 1

[Mathieu Rhéaume]

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

Comment 2

[Gabriele Svelto [:gsvelto]]

(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.

Comment 3

[Mathieu Rhéaume]

Attachment: 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.

Comment 4

[Gabriele Svelto [:gsvelto]]

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.

Comment 5

[Mathieu Rhéaume]

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.

Comment 6

[Gabriele Svelto [:gsvelto]]

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.

Comment 7

[Mathieu Rhéaume]

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.

Comment 8

[(inactive after 6/18) Alive Kuo [: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.

Comment 9

[Arthur Chen [:arthurcc][inactive after 6/30]]

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

Please check my comments in github, thanks.

Comment 10

[Mathieu Rhéaume]

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 !

Comment 11

[Arthur Chen [:arthurcc][inactive after 6/30]]

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!

Comment 12

[Mathieu Rhéaume]

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.

Comment 13

[Arthur Chen [:arthurcc][inactive after 6/30]]

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

Looks good to me, f=me.

Comment 14

[José Antonio Olivera Ortega [:jaoo]]

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

LGTM. f=me

Comment 15

[Mathieu Rhéaume]

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

Thanks

Comment 16

[(inactive after 6/18) Alive Kuo [: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.

Comment 17

[Carsten Book [:Tomcat]]

master: https://github.com/mozilla-b2g/gaia/commit/53890df7c7253f3b739542d6cc33ebdd0d46f57e