Closed Bug 973445 Opened 6 years ago Closed 5 years ago

[settings] refactor Sound panel with AMD pattern

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S5 (26sep)

People

(Reporter: gasolin, Assigned: gasolin)

References

Details

(Whiteboard: [p=13])

Attachments

(4 files)

46 bytes, text/x-github-pull-request
arthurcc
: review+
Details | Review
PR2
46 bytes, text/x-github-pull-request
arthurcc
: review+
Details | Review
PR3
46 bytes, text/x-github-pull-request
Details | Review
PR4
46 bytes, text/x-github-pull-request
arthurcc
: review+
Details | Review
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:
Assignee: nobody → gasolin
Blocks: 956210
Target Milestone: --- → 1.4 S5 (11apr)
Status: NEW → ASSIGNED
WIP, need more test cases
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.
Attachment #8402539 - Flags: feedback?(gduan)
Attachment #8402539 - Flags: feedback?(ejchen)
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 !!
Attachment #8402539 - Flags: feedback?(ejchen)
Comment on attachment 8402539 [details] [review]
pull request redirect to github

travis passed
Attachment #8402539 - Flags: review?(arthur.chen)
Comment on attachment 8402539 [details] [review]
pull request redirect to github

Thanks Fred.
I have put some comments there. plz kindly check. Thanks.
Attachment #8402539 - Flags: feedback?(gduan)
Target Milestone: 1.4 S5 (11apr) → 1.4 S6 (25apr)
Comment on attachment 8402539 [details] [review]
pull request redirect to github

Thanks for the patch, Fred. Please check my comments in github.
Attachment #8402539 - Flags: review?(arthur.chen)
Comment on attachment 8402539 [details] [review]
pull request redirect to github

arthur, I've fixed addressed issues, please kindly review it again
Attachment #8402539 - Flags: review?(arthur.chen)
Comment on attachment 8402539 [details] [review]
pull request redirect to github

We are almost there! Please address the last few nits, thanks.
Attachment #8402539 - Flags: review?(arthur.chen)
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
Attachment #8402539 - Flags: review?(arthur.chen)
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.
Attachment #8402539 - Flags: review?(arthur.chen) → review+
merged https://github.com/mozilla-b2g/gaia/commit/0ea19c05c99b12dc330d8c232af91232e15f9edd

thanks!
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
sorry had to revert for gaia unit test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=38237585&tree=B2g-Inbound
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Target Milestone: 1.4 S6 (25apr) → 2.0 S1 (9may)
merged https://github.com/mozilla-b2g/gaia/commit/473dd9f3738b950910841bc23567b8c234562a31
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [p=1]
backed out per https://tbpl.mozilla.org/php/getParsedLog.php?id=38618060&tree=B2g-Inbound

master: 4f282fc9294419c0ddfffd8094446639f3ca1319
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 982949
Blocks: 982951
Whiteboard: [p=1] → [p=3]
feature-b2g: --- → 2.0
Since this was reopened, Howie, can you help to reconfigure the target milestone? When could we have this done? Thanks.
Flags: needinfo?(hochang)
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.
Flags: needinfo?(hochang) → needinfo?(gasolin)
Target Milestone: 2.0 S1 (9may) → 2.0 S2 (23may)
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.
Flags: needinfo?(gasolin)
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.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
backout 2ebc297cf1 since there's a glitch around carrier test...

Will fix it and add arthur for feedback
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached file PR2
Attachment #8427557 - Flags: review?(arthur.chen)
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.
Attachment #8427557 - Flags: review?(arthur.chen)
Comment on attachment 8427557 [details] [review]
PR2

fixed, please kindly review it again
Attachment #8427557 - Flags: review?(arthur.chen)
Comment on attachment 8427557 [details] [review]
PR2

LGTM. Please squash the commits and ensure travis and TBPL are good before merging, thanks.
Attachment #8427557 - Flags: review?(arthur.chen) → review+
still suffered from TBPL fail. rework with minimum AMD patch https://github.com/mozilla-b2g/gaia/pull/19601
Target Milestone: 2.0 S2 (23may) → 2.0 S4 (20june)
move to 2.1
feature-b2g: 2.0 → 2.1
No longer blocks: 982951
No longer blocks: 982949
Whiteboard: [p=3] → [p=8]
feature-b2g: 2.1 → ---
Blocks: 969264
No longer blocks: 956210
Target Milestone: 2.0 S4 (20june) → 2.0 S5 (4july)
Attached file PR3
rework on this, WIP
Comment on attachment 8482543 [details] [review]
PR3

TBPL passed, please kindly review the new version
Attachment #8482543 - Flags: review?(arthur.chen)
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.
Attachment #8482543 - Flags: review?(arthur.chen)
Comment on attachment 8482543 [details] [review]
PR3

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

please kindly review it again
Attachment #8482543 - Flags: review?(arthur.chen)
Attached file PR4
rebased in clean PR, please review this PR instead
Attachment #8487035 - Flags: review?(arthur.chen)
Attachment #8482543 - Flags: review?(arthur.chen)
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!
Attachment #8487035 - Flags: review?(arthur.chen)
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?
Flags: needinfo?(arthur.chen)
Yes, that makes sense creating a separate module. An instance of the module will be created for each channel type with a slider element.
Flags: needinfo?(arthur.chen)
Whiteboard: [p=8] → [p=13]
Target Milestone: 2.0 S5 (4july) → 2.1 S5 (26sep)
Comment on attachment 8487035 [details] [review]
PR4

separate sliderHandler module, TBPL green, please kindly review it again
Attachment #8487035 - Flags: review?(arthur.chen)
Comment on attachment 8487035 [details] [review]
PR4

Please check my comments in github, thanks.
Attachment #8487035 - Flags: review?(arthur.chen)
Comment on attachment 8487035 [details] [review]
PR4

Issue addressed, TBPL green, please kindly review it again
Attachment #8487035 - Flags: review?(arthur.chen)
Comment on attachment 8487035 [details] [review]
PR4

Please check my comments in github, thanks.
Attachment #8487035 - Flags: review?(arthur.chen)
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?
Attachment #8487035 - Flags: review?(arthur.chen)
Comment on attachment 8487035 [details] [review]
PR4

Please check my comments in github, thanks.
Attachment #8487035 - Flags: review?(arthur.chen)
Comment on attachment 8487035 [details] [review]
PR4

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

The l10n related code is moved to _renderTone.
Attachment #8487035 - Flags: review?(arthur.chen)
Comment on attachment 8487035 [details] [review]
PR4

r=me with the comments addressed, thanks!
Attachment #8487035 - Flags: review?(arthur.chen) → review+
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
Status: REOPENED → RESOLVED
Closed: 6 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.