964180 - [Settings] Using AMD modules for Module separation

Status: RESOLVED FIXED

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

Description

[[e-mail]]

Currently basic settings services (mozSettings/UI bindings, panel navigation...) used by all panels and root panel specific logic were mixed and defined in a few modules (Settings, Connectivity). 

The goal is to break the panel dependency. This should be done with breaking existing modules into smaller ones, which enables each panel to load only the necessary codes. And new panels should follow the new code structure so we will achieve:

    Module separation
    Panel separation
    Inline activities
    View/logic separation

And we should try to keep compatibility with current structure.

Comment 1

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

Attachment: link to https://github.com/mozilla-b2g/gaia/pull/15546

Evelyn, the patch does two things:
1. Introduce AMD
2. Separate logic in the settings object to different modules

For modules please refer to https://github.com/crh0716/gaia/blob/settings2_iterative/apps/settings/README.md for details. The patch looks huge, however, most of it is simply code movement. Let me know if you encounter any problem. I can explain the patch face to face. Thanks!

Comment 2

[Evelyn Hung]

Comment on attachment 8371252 [details]
link to https://github.com/mozilla-b2g/gaia/pull/15546

set feedback? to Kaze and Kevin to get more thoughts about this patch.

Comment 3

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

Kaze, Kevin, in the end we decided to refactor the existing settings app iteratively considering the refactor process may last for a few months and which introduces huge overheads of rebase. 

The patch introduces AMD and supports new panel definition. It is backward compatible to the existing code. So that we don't need to rewrite all panels to make them fit into the new architecture.

Look forward to your feedback, thanks!

Comment 4

[[e-mail]]

Besides we also did `make-perf` and get the similar average loadtime (219) as current settings.

So it does not impact current loadtime too much. With this refactor, we could enhance the loadtime by moving global lazyLoad js/css to panel modules in future refactoring.

Comment 5

[Kevin Grandon :kgrandon]

Comment on attachment 8371252 [details]
link to https://github.com/mozilla-b2g/gaia/pull/15546

Nice work! I think we could probably do something more aligned with sheets/haida coming - but is a ways away. If this is the approach you guys want to take for now it will work for me.

Comment 6

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

I was thinking to have iframes inside settings app but it seems to be a duplicate work of haida. The second thing is that does loading overhead come along with the haida concept (i.e. loading common scripts across panels)? Maybe the system app will optimize this part when haida is ready, but I am afraid of the potential performance regression if we separate all scripts and styles now.

Comment 7

[Kevin Grandon :kgrandon]

I love the incremental approach though - it should have much less risk. Once this lands I'll see if I can spend some time to help convert panels.

Comment 8

[Fabien Cazenave [:kaze]]

Wow, impressive work! We’re still in 1.3 rush at the moment, please leave me a couple days to look at this properly.

(In reply to Fred Lin [:gasolin] from comment #4)
> Besides we also did `make-perf` and get the similar average loadtime (219)
> as current settings.
> 
> So it does not impact current loadtime too much. With this refactor, we
> could enhance the loadtime by moving global lazyLoad js/css to panel modules
> in future refactoring.

I’m not sure to follow you here. If you’re saying that the current Settings app starts in 219 ms, then we don’t use the same device. ;-) Are we talking of a cold startup on a supported device like Buri or Inari?

My main worry is that we should not have any negative impact on the startup time — neither before the first paint, nor before the UI becomes usable (scrollable). Have you checked that on Buri/Inari? if yes, what difference does it make?

Comment 9

[[e-mail]]

Ooh I ran the make test-perf in mac and get 219(after)/225(before patch)

I'll setup and run in inari for more accurate result.

Comment 10

[Fabien Cazenave [:kaze]]

Oh, I should have guessed. Thanks Fred.

Comment 11

[[e-mail]]

run with b2gperf (what datazilla use) and https://github.com/askeing/b2g-beta-tools
which runs on inari device 30 times, the result are:

* before:          median:1711, mean:1664, std: 126, max:1867, min:1422
* with this patch: median:1645, mean:1658, std: 56, max:1788, min:1530

according to the mean value (1664/1658) the loadtime are basically at the same level

Comment 12

[Evelyn Hung]

Comment on attachment 8371252 [details]
link to https://github.com/mozilla-b2g/gaia/pull/15546

Arthur, thanks, the patch is good as long as you can add more comments in the code under modules/ because they are new structure and will be the backbone of Settings app. It's just the first step of our refactoring plan, and you have shown us a structure of panels and their interactions. The patch also intend to break codes in settings.js and move them to proper modules(panels). I think it's good, we won't hack this file anymore. \O/
Here is my comments overall:
1. considering running on different devices e.g. tablet and phone (and more in the future), is it possible to abstract device types and keep device detection in a module so we don't have to check |isTablet| or |twoColumn| everywhere. Not sure if it's do-able, just throw an idea.

2. As we all understand this is just a start, we need a plan on bugs so everyone can expect what will happen next.

Thanks again to you and Fred, you make the plan and push it on the way. :)

Comment 13

[[e-mail]]

will fix buildscript issue and land to bubble-tea branch https://github.com/mozilla-b2g/gaia/pull/16555

Comment 14

[[e-mail]]

After fixing build script issue, we found `KeyboardHelper requests` error in unit-tests-in-firefox ,
and failed 'test_settings_change_keyboard_language.py' in gaia_ui_tests

https://travis-ci.org/mozilla-b2g/gaia/builds/19557225

kevin, can you provide some suggestion to what element might cause this issue?

Comment 15

[Kevin Grandon :kgrandon]

Hmm, I'm not sure without looking into it. This seems keyboard related so moving ni? to rudy.

Comment 16

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Per an offline discussion, Fred and Arthur knows where to look at to tackle the issue mentioned in comment 14. Clear the ni.

Comment 17

[[e-mail]]

Found the root cause is the set _currentPanel timing, which cause keyboard settings act incorrectly.

Thanks!

Comment 18

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

Thanks Fred for finding the issues!

bubble-tea: 3ec7faaf584e17b3c4ebb5df4b0f4bd359189c6c

Comment 19

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

Attachment: Link to https://github.com/mozilla-b2g/gaia/pull/16555

PR for bubble-tea

Comment 20

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

https://github.com/mozilla-b2g/gaia/pull/17281

PR for master

Comment 21

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Arthur, there are two commits in this pull request, I think squash to one will be better.

Comment 22

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

thanks, done!

Comment 23

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

Waiting for TBPL results: https://tbpl.mozilla.org/?tree=Try&rev=8df242f8c3c4

Comment 24

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

Attachment: Link to https://github.com/mozilla-b2g/gaia/pull/17281

Hi Etienne, the whole patch has been reviewed as comment 12. I did some minor modifications on the dialer tests due to the shared mock_navigator_moz_settings changes (setting mSyncRepliesOnly to false when tearing down). Travis and TBPL are all passed. Could you have a look on the following two files? Thanks!

apps/communications/dialer/test/unit/multi_sim_action_button_test.js
apps/communications/dialer/test/unit/phone_action_menu_test.js

Comment 25

[Etienne Segonzac (:etienne)]

Comment on attachment 8393394 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/17281

r=me for the mockmozsettings update in the dialer.

<3 the apps/settings/Readme.md by the way :)

Comment 26

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

Thanks for the quick review, Etienne!

master: 3dc49ae1222204ad37962d29ecadeca9e31134b8

Comment 27

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

Revert as it broke the settings app in the production build.

master: 6180c5c733a4399e75d0b3c70e8b45743f20032f

Comment 28

[[e-mail]]

Reproduce steps:

1. install settings with command `GAIA_OPTIMIZE=1 make install-gaia APP=settings`

2. adb error log shows

[JavaScript Error: "ReferenceError: Settings is not defined" {file: "app://settings.gaiamobile.org/gaia_build_defer_index.js" line: 170}

Comment 29

[[e-mail]]

The error is located in https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/js/firefox_accounts/menu_loader.js#L6 , might be due to race condition

Comment 30

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

And based on the comments in bug 986460, I suspect that the scripts (optimized) were not loaded at all.

Comment 31

[[e-mail]]

Ya... moved menu_loader.js into js/settings.js::this.LazyLoader.load , the error in Comment 28 fixed, 
but still can't navigate.

Comment 32

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

The root cause is that webapp-optimize.js does not take the script specified in the data-main attribute into account. As it does not make sense to make webapp-optimize.js recognize requirejs specific attributes, we may need enable the minify process in the makefile of the settings app.

Comment 33

[[e-mail]]

Attachment: pull request redirect to github

This patch did following things:

1. moved menu_load.js into settings.js lazy_load
2. changed to the same bootstrap process as camera app.

https://github.com/gasolin/gaia/commit/5f9b32d624cb877828d54f5f6ba7a0b2dbb8415b

Comment 34

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

re-land the patch. Fixed the issue by skipping the optimization. Will create a new bug to track the refinement of the build script.

master: 7324a7b921b7b60de1427124e0efcafa91f4be75

Comment 35

[[e-mail]]

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

Per offline discussion with arthur, we'll use optimize-skip to land this patch, then add followup issue to fix build script.