1014442 - [Settings] refactor Find My Device panel with AMD pattern

Status: RESOLVED FIXED

(Firefox OS Graveyard :: FindMyDevice, defect)

Description

[[e-mail]]

Overview Description:

Refactor Find My Device panel with AMD pattern referring to
https://github.com/gasolin/gaia/tree/master/apps/settings

to make it modularize and more easier to maintain


Expected Results:

pass all settings test and act the same as original implementation

Additional Information:

Comment 1

[Erin Lancaster [:elan]]

Fred, is this a blocker?

Comment 2

[Doug Turner (:dougt)]

I can't imagine a refactor being a blocker.

Comment 3

[Erin Lancaster [:elan]]

Sounds good. I've tagged for backlog.

Comment 4

[[e-mail]]

It shall be in engineer task queue but not a blocker

Comment 5

[Michael:mtreese]

[Blocking Requested - why for this release]:

Comment 6

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Removing the nomination, I don't really think optional stuff like refactoring should block until we have more devs on this project

Comment 7

[Tony Chung [:tchung]]

agree, this isnt a 2.1 blocker.   minusing.

Comment 8

[GitHub Autolander Bot]

Attachment: [gaia] scottwu:1014442-refactor-find-my-device-with-amd > mozilla-b2g:master

Comment 9

[Scott Wu [:scottwu]]

Comment on attachment 8662190 [details] [review]
[gaia] scottwu:1014442-refactor-find-my-device-with-amd > mozilla-b2g:master

Made some changes to the structure and unit test. Please let me know what you think. Thank!

Comment 10

[[e-mail]]

Comment on attachment 8662190 [details] [review]
[gaia] scottwu:1014442-refactor-find-my-device-with-amd > mozilla-b2g:master

also add lissyx for functionality check since he is 'Find My Device' peer

Comment 11

[:gerard-majax]

Comment on attachment 8662190 [details] [review]
[gaia] scottwu:1014442-refactor-find-my-device-with-amd > mozilla-b2g:master

Looks good but I fear we are going to leak observers.

Comment 12

[[e-mail]]

Comment on attachment 8662190 [details] [review]
[gaia] scottwu:1014442-refactor-find-my-device-with-amd > mozilla-b2g:master

Thanks for working on the patch. 
Please address the comments on github then set review again.

Comment 13

[Scott Wu [:scottwu]]

Comment on attachment 8662190 [details] [review]
[gaia] scottwu:1014442-refactor-find-my-device-with-amd > mozilla-b2g:master

Sorry for taking this long to fix the code. I believe I've fixed all the suggestions. Please let me know if you catch any other mistake. Thanks!

Comment 14

[[e-mail]]

Comment on attachment 8662190 [details] [review]
[gaia] scottwu:1014442-refactor-find-my-device-with-amd > mozilla-b2g:master

Looks good and works on device. Thanks Scott!

Add @lissyx in review queue. Please land after @lissyx feel its ready

Comment 15

[:gerard-majax]

Comment on attachment 8662190 [details] [review]
[gaia] scottwu:1014442-refactor-find-my-device-with-amd > mozilla-b2g:master

Please be extra cautious and make sure you are not introducing any mozSettings observer leaks in your SettingsListener/SettingsHelper use: I have not saw anything wrong but I might be missing them because of AMD.

So any .observe() should be unobserve()'d. There are silent addObserver() call being made by SettingsListener/SettingsHelper when you create those objects, do you need to make sure you have the proper balance or do not constantly recreate thoes objects.

Comment 16

[Scott Wu [:scottwu]]

Got it. Thanks for the reminder Alexandre. I'll be extra careful when dealing with observers.

I've fixed the line break tissue. Thanks Fred!