gasolin@mozilla.com Reporter
	Description • 11 years ago

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.

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 1 • 11 years ago

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!

	Evelyn Hung
	Comment 2 • 11 years ago

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.

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 3 • 11 years ago

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!

	gasolin@mozilla.com Reporter
	Comment 4 • 11 years ago

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.

	Kevin Grandon :kgrandon
	Comment 5 • 11 years ago

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.

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 6 • 11 years ago

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.

	Kevin Grandon :kgrandon
	Comment 7 • 11 years ago

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.

	Fabien Cazenave [:kaze]
	Comment 8 • 11 years ago

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?

	gasolin@mozilla.com Reporter
	Comment 9 • 11 years ago

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.

	Fabien Cazenave [:kaze]
	Comment 10 • 11 years ago

Oh, I should have guessed. Thanks Fred.

	gasolin@mozilla.com Reporter
	Comment 11 • 11 years ago

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

	Evelyn Hung
	Comment 12 • 11 years ago

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. :)

	gasolin@mozilla.com Reporter
	Comment 13 • 11 years ago

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

	gasolin@mozilla.com Reporter
	Comment 14 • 11 years ago

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?

	Kevin Grandon :kgrandon
	Comment 15 • 11 years ago

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

	Rudy Lu [:rudyl] (inactive after 2015/7/3)
	Comment 16 • 11 years ago

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

	gasolin@mozilla.com Reporter
	Comment 17 • 11 years ago

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

Thanks!

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 18 • 11 years ago

Thanks Fred for finding the issues!

bubble-tea: 3ec7faaf584e17b3c4ebb5df4b0f4bd359189c6c

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 19 • 11 years ago

PR for bubble-tea

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 20 • 10 years ago

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

PR for master

	Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)
	Comment 21 • 10 years ago

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

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 22 • 10 years ago

thanks, done!

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 23 • 10 years ago

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

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 24 • 10 years ago

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

	Etienne Segonzac (:etienne)
	Comment 25 • 10 years ago

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 :)

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 26 • 10 years ago

Thanks for the quick review, Etienne!

master: 3dc49ae1222204ad37962d29ecadeca9e31134b8

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 27 • 10 years ago

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

master: 6180c5c733a4399e75d0b3c70e8b45743f20032f

	gasolin@mozilla.com Reporter
	Comment 28 • 10 years ago

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}

	gasolin@mozilla.com Reporter
	Comment 29 • 10 years ago

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

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 30 • 10 years ago

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

	gasolin@mozilla.com Reporter
	Comment 31 • 10 years ago

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

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 32 • 10 years ago

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.

	gasolin@mozilla.com Reporter
	Comment 33 • 10 years ago

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

	Arthur Chen [:arthurcc][inactive after 6/30] Assignee
	Comment 34 • 10 years ago

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

	gasolin@mozilla.com Reporter
	Comment 35 • 10 years ago

Comment on attachment 8395530 [details]
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.