973443 - [settings] refactor Display panel with AMD pattern

Status: RESOLVED FIXED

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

Description

[[e-mail]]

Overview Description:

Refactor Display panel with AMD pattern referring to
https://github.com/crh0716/gaia/tree/settings2_iterative

to make it modularize and more easier to maintain

Steps to Reproduce:
1) run make test-perf APP=settings
2) run make test-integration APP=settings


Expected Results:

pass all settings test and act the same as original implementation

Additional Information:

Comment 1

[(Inactive after June) George Duan [:gduan] [:喬智]]

Attachment: PR to master

waiting for travis.

Comment 2

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 8428697 [details] [review]
PR to master

Hi Arthur, EJ, Fred,

I've refactored this panel, could you kindly take a look at it?
Thanks!

Comment 3

[[e-mail]]

Comment on attachment 8428697 [details] [review]
PR to master

looks good to me, some comments in github

Comment 4

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8428697 [details] [review]
PR to master

Hi George,

I already left some comments on your patch and please check it later !

Overall it is good to me ! Thanks .

Comment 5

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

Comment on attachment 8428697 [details] [review]
PR to master

Good work! Please check my comments in github, thanks!

Comment 6

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 8428697 [details] [review]
PR to master

Hi Arthur,
issues addressed, could you take a look again?
Thanks.

Comment 7

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

Comment on attachment 8428697 [details] [review]
PR to master

Well done, George! There are some minor issues and they shouldn't be difficult to fix. Please check them in github. And as the patch also fix the jshint errors in fl.js, let's also request a review from the owner.

Comment 8

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 8428697 [details] [review]
PR to master

Hi Arthur,
I've updated my patch. Could you take a look again? Thanks.

Comment 9

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

Comment on attachment 8428697 [details] [review]
PR to master

We are almost there! Please check my comments in github.

Comment 10

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 8428697 [details] [review]
PR to master

Hi Arthur,
issues addressed!
Thanks.

Comment 11

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

Comment on attachment 8428697 [details] [review]
PR to master

r=me with one last nit addressed, thanks!

Comment 12

[(Inactive after June) George Duan [:gduan] [:喬智]]

travis pass
https://tbpl.mozilla.org/?tree=Gaia-Try&rev=fa819ef061f4

merge to master
https://github.com/mozilla-b2g/gaia/commit/f38eb30aaef8186f30391dbca164ac0cf28ed42e

Comment 13

[(Inactive after June) George Duan [:gduan] [:喬智]]

typo
tbpl pass, not travis.

Comment 14

[Ed Morley [:emorley]]

Unfortunately I've had to revert this due to intermittent test failures, eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=41591983&tree=B2g-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=41588938&tree=B2g-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=41584317&tree=B2g-Inbound

https://github.com/mozilla-b2g/gaia/commit/080027d50696ad4e003d44f899257f3b9f7aed98

Comment 15

[(Inactive after June) George Duan [:gduan] [:喬智]]

Attachment: PR to master 2

Hi Arthur,
I think the setTimeout in mock_moz_activity call the onsuccess too early before I set the value of successResult.
Please kindly check 2nd commit of my patch.
Thanks.

Comment 16

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

Comment on attachment 8440475 [details] [review]
PR to master 2

Based on the log, the test failed due to that "onsuccess" was called outside the test again and resulted in a javascript error. We should use a fake timer and wait for the trigger of "onsuccess" instead of triggering it ourselves.

Comment 17

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 8440475 [details] [review]
PR to master 2

Hi Arthur,
you're right.
Please kindly check again.
thanks.

Comment 18

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

Comment on attachment 8440475 [details] [review]
PR to master 2

r=me with the comment addressed. Thanks.

Comment 19

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Why are we still setTimeout() in mocks? We should remove them with stub() or something deterministic. See bug 974319.

Comment 20

[(Inactive after June) George Duan [:gduan] [:喬智]]

bug 1025763 , remove setTimeout from mock_moz_activity.js
(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #19)
> Why are we still setTimeout() in mocks? We should remove them with stub() or
> something deterministic. See bug 974319.

Comment 21

[(Inactive after June) George Duan [:gduan] [:喬智]]

land to master,
travis and tbpl are all green.
https://github.com/mozilla-b2g/gaia/commit/7534236582775baf89051583238b49fd68c5bf3c