Closed Bug 973443 Opened 7 years ago Closed 7 years ago

[settings] refactor Display panel with AMD pattern

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gasolin, Assigned: gduan)

References

Details

(Whiteboard: [p=5])

Attachments

(2 files)

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:
Assignee: nobody → gduan
Attached file PR to master
waiting for travis.
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!
Attachment #8428697 - Flags: review?(arthur.chen)
Attachment #8428697 - Flags: feedback?(gasolin)
Attachment #8428697 - Flags: feedback?(ejchen)
Comment on attachment 8428697 [details] [review]
PR to master

looks good to me, some comments in github
Attachment #8428697 - Flags: feedback?(gasolin) → feedback+
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 .
Attachment #8428697 - Flags: feedback?(ejchen) → feedback+
Comment on attachment 8428697 [details] [review]
PR to master

Good work! Please check my comments in github, thanks!
Attachment #8428697 - Flags: review?(arthur.chen) → feedback+
Comment on attachment 8428697 [details] [review]
PR to master

Hi Arthur,
issues addressed, could you take a look again?
Thanks.
Attachment #8428697 - Flags: review?(arthur.chen)
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.
Attachment #8428697 - Flags: review?(arthur.chen)
Comment on attachment 8428697 [details] [review]
PR to master

Hi Arthur,
I've updated my patch. Could you take a look again? Thanks.
Attachment #8428697 - Flags: review?(arthur.chen)
Comment on attachment 8428697 [details] [review]
PR to master

We are almost there! Please check my comments in github.
Attachment #8428697 - Flags: review?(arthur.chen)
Comment on attachment 8428697 [details] [review]
PR to master

Hi Arthur,
issues addressed!
Thanks.
Attachment #8428697 - Flags: review?(arthur.chen)
Comment on attachment 8428697 [details] [review]
PR to master

r=me with one last nit addressed, thanks!
Attachment #8428697 - Flags: review?(arthur.chen) → review+
typo
tbpl pass, not travis.
Attached file 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.
Attachment #8440475 - Flags: review?(arthur.chen)
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.
Attachment #8440475 - Flags: review?(arthur.chen)
Comment on attachment 8440475 [details] [review]
PR to master 2

Hi Arthur,
you're right.
Please kindly check again.
thanks.
Attachment #8440475 - Flags: review?(arthur.chen)
Comment on attachment 8440475 [details] [review]
PR to master 2

r=me with the comment addressed. Thanks.
Attachment #8440475 - Flags: review?(arthur.chen) → review+
Why are we still setTimeout() in mocks? We should remove them with stub() or something deterministic. See bug 974319.
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.
land to master,
travis and tbpl are all green.
https://github.com/mozilla-b2g/gaia/commit/7534236582775baf89051583238b49fd68c5bf3c
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Whiteboard: [p=5]
You need to log in before you can comment on or make changes to this bug.