[Settings] refactor storage.js with AMD pattern

RESOLVED FIXED in 2.0 S5 (4july)

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gasolin, Assigned: gasolin)

Tracking

unspecified
2.0 S5 (4july)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [p=5])

Attachments

(4 attachments)

46 bytes, text/x-github-pull-request
eragonj
: feedback+
Details | Review
PR2
46 bytes, text/x-github-pull-request
Details | Review
PR3
46 bytes, text/x-github-pull-request
eragonj
: feedback+
arthurcc
: feedback+
Details | Review
PR5
46 bytes, text/x-github-pull-request
arthurcc
: review+
Details | Review
`storage.js` is lazy loaded in settings.js > root panel

We could use AMD pattern and mvvm to observe the storage result for root panel.

The result could be reused for  app storage and media storage panel
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Target Milestone: --- → 2.0 S3 (6june)
Blocks: 956210
Depends on: 994511
root panel only
Depends on: 973453
Whiteboard: p=3
feature-b2g: --- → 2.0
No longer blocks: 973452
Depends on: 973452
I think I can't make it done in next week, postpone to s4
Status: ASSIGNED → NEW
Target Milestone: 2.0 S3 (6june) → 2.0 S4 (20june)
feature-b2g: 2.0 → 2.1
feature-b2g: 2.1 → ---
the patch did:

1. moved storage.js into panels/root
2. make it pure AMD with AppStorage observer
3. move UMD dialog from index.html to root.html

I think the full patch could be done after MediaStorage refactor, but it's hard to estimate the MediaStorage refactor done date.
Attachment #8438101 - Flags: feedback?(ejchen)
Attachment #8438101 - Flags: feedback?(arthur.chen)
Comment on attachment 8438101 [details] [review]
pull request redirect to github

The changes look good to me, and I left some comments on github, please check it when you have time, Fred.

THanks ;)
Attachment #8438101 - Flags: feedback?(ejchen) → feedback+
updated. found there's marionette test already, will update it as well
Comment on attachment 8438101 [details] [review]
pull request redirect to github

Thanks for the patch!

As r.js helps on merge scripts for us, I would prefer to have separate modules being responsible for each menu item, especially when the code for these items are pretty isolated.

And if there are handlers updating UI elements, make sure they are un-registered when leaving the panel. We can use a simple enabled property to do that, you may refer to keyboard related modules for details.
Attachment #8438101 - Flags: feedback?(arthur.chen)
Blocks: 969264
No longer blocks: 956210
Comment on attachment 8438101 [details] [review]
pull request redirect to github

comment addressed, add enabled function to register/un-register observer while enter/leaving the panel
Attachment #8438101 - Flags: review?(arthur.chen)
Comment on attachment 8438101 [details] [review]
pull request redirect to github

As comment 6 suggested, please create separated modules for each menu item.
Attachment #8438101 - Flags: review?(arthur.chen)
found the USB storage and media storage have some connection between updateVolumeState.

Seems we have to refactor media storage first to make the panel separation clean.
No longer blocks: 973451
Depends on: 973451
Posted file PR2
per offline discussion with arthur, I separate the app storage menu module, but keep usb and media storage in same module. The media storage module will be separated until bug media storage refactor is done

root panel marionette test passed
Attachment #8439046 - Flags: review?(arthur.chen)
Attachment #8439046 - Flags: feedback?(ejchen)
Comment on attachment 8439046 [details] [review]
PR2

(In reply to Fred Lin [:gasolin] from comment #10)
> Created attachment 8439046 [details] [review]
> PR2
> 
> per offline discussion with arthur, I separate the app storage menu module,
> but keep usb and media storage in same module. The media storage module will
> be separated until bug media storage refactor is done
> 
> root panel marionette test passed

Ok got it !

Just left some feedbacks on Github and please ping me directly if you have any concern about comments, Fred.

Overall they all look good to me and you can set me f? again when you are ready. 

Thanks Fred.
Attachment #8439046 - Flags: feedback?(ejchen)
Comment on attachment 8439046 [details] [review]
PR2

Thanks EJ! I've fixed binding issue and move all private functions prefix with '_'. please kindly check it.
Attachment #8439046 - Flags: feedback?(ejchen)
Comment on attachment 8439046 [details] [review]
PR2

The patch conflicts to gaia master. Could you help rebase it?
Attachment #8439046 - Flags: review?(arthur.chen)
Attachment #8439046 - Flags: feedback?(ejchen)
Posted file PR3
rebased the root panel. please review it again
Attachment #8440554 - Flags: review?(arthur.chen)
Attachment #8440554 - Flags: feedback?(ejchen)
Whiteboard: p=3 → [p=3]
Comment on attachment 8440554 [details] [review]
PR3

Only few nits on the patch, please check it on Github.

Basically, they all look good to me.
Attachment #8440554 - Flags: feedback?(ejchen) → feedback+
Comment on attachment 8440554 [details] [review]
PR3

Overall it looks good to me, please check my comments in github, thanks!
Attachment #8440554 - Flags: review?(arthur.chen) → feedback+
Target Milestone: 2.0 S4 (20june) → 2.0 S5 (4july)
Posted file PR5
fix addressed issues and python UI tests, please kindly review it again, thanks!
Attachment #8448519 - Flags: review?(arthur.chen)
Comment on attachment 8448519 [details] [review]
PR5

Thanks for the patch. Please check the comments regarding the naming and unit tests I left in github.
Attachment #8448519 - Flags: review?(arthur.chen)
Comment on attachment 8448519 [details] [review]
PR5

comment addressed, please kindly review it
Attachment #8448519 - Flags: review?(arthur.chen)
Comment on attachment 8448519 [details] [review]
PR5

Looks good to me, thank you!
Attachment #8448519 - Flags: review?(arthur.chen) → review+
merged to master https://github.com/mozilla-b2g/gaia/commit/8304cac69c74b0b5d25eca638c0e79113870bb39

thanks!
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [p=3] → [p=5]
You need to log in before you can comment on or make changes to this bug.