973445 - [settings] refactor Sound panel with AMD pattern

Status: RESOLVED FIXED

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

Description

[[e-mail]]

Overview Description:

Refactor Sound 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

[[e-mail]]

Attachment: pull request redirect to github

WIP, need more test cases

Comment 2

[[e-mail]]

Comment on attachment 8402539 [details] [review]
pull request redirect to github

Code is passed in local integration and uitest, will fix the last travis uitest issue.

This refactor does not handle ForwardLock part yet.
Please feedback what you thought.

Comment 3

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

Comment on attachment 8402539 [details] [review]
pull request redirect to github

Fred, I left some comments on Github and you can take a look when you have time ! 

thanks !!

Comment 4

[[e-mail]]

Comment on attachment 8402539 [details] [review]
pull request redirect to github

travis passed

Comment 5

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

Comment on attachment 8402539 [details] [review]
pull request redirect to github

Thanks Fred.
I have put some comments there. plz kindly check. Thanks.

Comment 6

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

Comment on attachment 8402539 [details] [review]
pull request redirect to github

Thanks for the patch, Fred. Please check my comments in github.

Comment 7

[[e-mail]]

Comment on attachment 8402539 [details] [review]
pull request redirect to github

arthur, I've fixed addressed issues, please kindly review it again

Comment 8

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

Comment on attachment 8402539 [details] [review]
pull request redirect to github

We are almost there! Please address the last few nits, thanks.

Comment 9

[[e-mail]]

Comment on attachment 8402539 [details] [review]
pull request redirect to github

thanks for review. I've fix all nits and put them in separate commit. please kindly review it again

Comment 10

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

Comment on attachment 8402539 [details] [review]
pull request redirect to github

Thanks for the effort. r=me. Before merging, please remove the calling to `uninit` as we don't need need it.

Comment 11

[[e-mail]]

merged https://github.com/mozilla-b2g/gaia/commit/0ea19c05c99b12dc330d8c232af91232e15f9edd

thanks!

Comment 12

[Carsten Book [:Tomcat]]

sorry had to revert for gaia unit test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=38237585&tree=B2g-Inbound

Comment 13

[[e-mail]]

PR replaced by https://github.com/mozilla-b2g/gaia/pull/18553

Comment 14

[[e-mail]]

The root cause is ForwardLock file has been removed(merge into sound.js) during optimization process.
Set the paths to "empty" in `build/settings.build.jslike` could resolve the test error.

Comment 15

[[e-mail]]

merged https://github.com/mozilla-b2g/gaia/commit/473dd9f3738b950910841bc23567b8c234562a31

Comment 16

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

backed out per https://tbpl.mozilla.org/php/getParsedLog.php?id=38618060&tree=B2g-Inbound

master: 4f282fc9294419c0ddfffd8094446639f3ca1319

Comment 17

[[e-mail]]

New PR https://github.com/mozilla-b2g/gaia/pull/18767

Comment 18

[Kai-Chih Hu [:khu]]

Since this was reopened, Howie, can you help to reconfigure the target milestone? When could we have this done? Thanks.

Comment 19

[howie [:howie]]

Hi Fred, could you re-confirm the target milestone? I suppose this will land in 2.0 sprint 2 according to our status meeting. Thanks.

Comment 20

[[e-mail]]

This issue passed all Travis test but met some bad TBPL-only test fail issue. I'm trying to rework on this issue to find out the root cause. 

This refactor does not introduce new features so it should be pass every existing tests anywhere. 
I've talked with domi and his work will not depends on this issue.

Comment 21

[[e-mail]]

TBPL passed, merged to gaia-master https://github.com/mozilla-b2g/gaia/commit/bc203f87fe03416a4c5cd7aacef65642f8200899

The root cause is about shim for forwardlock (fl). remove the shim make it works.

Comment 22

[[e-mail]]

backout 2ebc297cf1 since there's a glitch around carrier test...

Will fix it and add arthur for feedback

Comment 23

[[e-mail]]

Attachment: PR2

Comment 24

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

Comment on attachment 8427557 [details] [review]
PR2

There are a few nits need to be addressed before merging, please check my comments in github, thanks.

Comment 25

[[e-mail]]

Comment on attachment 8427557 [details] [review]
PR2

fixed, please kindly review it again

Comment 26

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

Comment on attachment 8427557 [details] [review]
PR2

LGTM. Please squash the commits and ensure travis and TBPL are good before merging, thanks.

Comment 27

[[e-mail]]

still suffered from TBPL fail. rework with minimum AMD patch https://github.com/mozilla-b2g/gaia/pull/19601

Comment 28

[howie [:howie]]

move to 2.1

Comment 29

[[e-mail]]

Attachment: PR3

rework on this, WIP

Comment 30

[[e-mail]]

Comment on attachment 8482543 [details] [review]
PR3

TBPL passed, please kindly review the new version

Comment 31

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

Comment on attachment 8482543 [details] [review]
PR3

Good job! Please check my comments in github. I'll review the tests once the comments are addressed. Thanks.

Comment 32

[[e-mail]]

Comment on attachment 8482543 [details] [review]
PR3

Addressed comments in github and extract volume/tone manager methods, TBPL green

please kindly review it again

Comment 33

[[e-mail]]

Attachment: PR4

rebased in clean PR, please review this PR instead

Comment 34

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

Comment on attachment 8487035 [details] [review]
PR4

Looks better now. I left a few comments in github to refactor the functions a little bit further, please check them, thanks!

Comment 35

[[e-mail]]

thanks for feedback!

As comment in https://github.com/mozilla-b2g/gaia/pull/23897#discussion_r17525711, do you think we need separate sliderHandler module to deal with per slider stats?

Comment 36

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

Yes, that makes sense creating a separate module. An instance of the module will be created for each channel type with a slider element.

Comment 37

[[e-mail]]

Comment on attachment 8487035 [details] [review]
PR4

separate sliderHandler module, TBPL green, please kindly review it again

Comment 38

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

Comment on attachment 8487035 [details] [review]
PR4

Please check my comments in github, thanks.

Comment 39

[[e-mail]]

Comment on attachment 8487035 [details] [review]
PR4

Issue addressed, TBPL green, please kindly review it again

Comment 40

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

Comment on attachment 8487035 [details] [review]
PR4

Please check my comments in github, thanks.

Comment 41

[[e-mail]]

Comment on attachment 8487035 [details] [review]
PR4

Issue addressed, TBPL green, please kindly review it again.

Note there are 2 test cases break the TBPL so I commented them in source.

The window.clearInterval stub does not work on both local or TBPL, it might be sinon issue?

Comment 42

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

Comment on attachment 8487035 [details] [review]
PR4

Please check my comments in github, thanks.

Comment 43

[[e-mail]]

Comment on attachment 8487035 [details] [review]
PR4

Issue addressed, TreeHerder green, please kindly review it again.

The l10n related code is moved to _renderTone.

Comment 44

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

Comment on attachment 8487035 [details] [review]
PR4

r=me with the comments addressed, thanks!

Comment 45

[[e-mail]]

tree herder passed except gij. checked with greg that the gij screenlock fail case is an intermittent bug.

merged to master https://github.com/mozilla-b2g/gaia/commit/6b1b17744eed5a5edb27dd4148cb6aee6377145c