Now the follow after booting to show the homescreen is: Show Initlogo Hide Initlogo when FtuLauncher and HomescreenWindow is ready Show homescreen if system is unlocked in AppWindowManager I hope all of this logic is only controlled by someone, maybe core.js And use the following strategy: Promise.all([FtuLaunche.ready(), HomescreenWindowManager.ready(), LockscreenWindowManager.ready()]).then(function() { InitLogo.stop(); });

The first step is to make FtuLauncher based on BaseModule and launched by AppWindowManager. Tracking in bug 971488

The second step will be appCore - bug 1091375

Progress update: I am having a branch for gathering all works belong to new bootstrap now https://github.com/alivedise/gaia/compare/new-bootstrap-on-32ccf090f03cbec97ea6c397797af1e0ce775b03?expand=1 It is improving the bootup time for ~8sec, see http://i.imgur.com/WpsqXXQ.png The basic mechanism is: * Launcher will take care high priority preference before Core starts * Bootstrap will start core when launcher is ready * BaseModule will support low priority modules loading - after idle timer timeouts and normal priority modules loaded

Hi Marshall, here is a question for you: if we don't need to run ftu, do we still need to run ftuPing? What I want to do is, if we don't need to run FTU, we don't even need to load/start ftuPing, but I am not sure if that will break something.

(In reply to Alive Kuo@Paris~2/17 [:alive][NEEDINFO!] from comment #4) > Hi Marshall, here is a question for you: if we don't need to run ftu, do we > still need to run ftuPing? > What I want to do is, if we don't need to run FTU, we don't even need to > load/start ftuPing, but I am not sure if that will break something. Hrm.. the FTU Ping is also known as the 'Activation' ping -- primarily because it's purpose is to count new device activations. The general idea is that we should run the ping during the device's first bootup, regardless of other things being disabled. I hate to answer a question with a question, but.. why would we want to skip running FTU?

(In reply to Marshall Culpepper [:marshall_law] from comment #5) > (In reply to Alive Kuo@Paris~2/17 [:alive][NEEDINFO!] from comment #4) > > Hi Marshall, here is a question for you: if we don't need to run ftu, do we > > still need to run ftuPing? > > What I want to do is, if we don't need to run FTU, we don't even need to > > load/start ftuPing, but I am not sure if that will break something. > > Hrm.. the FTU Ping is also known as the 'Activation' ping -- primarily > because it's purpose is to count new device activations. The general idea is > that we should run the ping during the device's first bootup, regardless of > other things being disabled. > > I hate to answer a question with a question, but.. why would we want to skip > running FTU? I mean the second time we launch the device. BTW I found it's still necessary to have ftuLauncher even not running first time so ignore my last comment :)

Comment on attachment 8573056 [details] [gaia] alivedise:new-bootstrap-0306 > mozilla-b2g:master OK here is what I am having now. Not perfect yet but time to show you. The new rational is: * Put init logo + l10n + service in top of system app header. They are tier 0 modules. * Only load tier1 modules in bottom of system app body. * Load tier2 modules which blocks ftu/homescreen to load in startup.js * Load side modules whenever the busy loading ends after 10sec idle timeout (tune-able) * For most UI modules only lazy load when needed. [Changed] * FtuLauncher become inactive and managed by the new launcher instead of doing tasks on its own. * VersionHelper is not very smart...all the module who needs it in system app is doing: get os version settings + previous os version settings then compare it. Every time. Also this module is blocking the critical path of the launch sequence. I am using Launcher to improve it. * All submodules under AppWindow and its subclasses are now declared using String because it is not loaded when AppWindow are loaded. [Removed modules] * SettingsHelper is just another library for mozSettings.. we don't need that much libraries so I am using SettingsCore request to replace it. * Shared/FontUtils is only needed by appChrome but the logic is removed in master so it becomes an orphan now. [Newly modules] * Launcher: When investigating startup time I found that we are busy fetching these values: lockscreen.enabled/ftu.enabled/os version/os previous version... separately in individual modules. The speed of these asynchronous operation will be slowed by all other busy launching modules. My idea is to get all of these modules in the very beginning of the system app and then decide the next step to do, without being affected by other modules. The data approves this improves start up time from 7sec to 10sec. * CustomDialogService - to group and load CustomDialog on demand. With this we don't need to specify z-index of the custom dialog each time we show it. * AppCore - load all window management stuffs [Opened question] * KeyboardManager and InputWindowManager is still deeply coupling. We could have an InputMethodCore to manage them, but I am not sure if I should/could do it in this patch. * SettingsListener should be replaced by SettingsCore request, but there're too many places to modify (133) so I am afraid I won't do it right now. [Todo] * Fix lockscreen issues if any * Fix layout manager/orientation manager issues if any * Tests Lemme know if you have questions.

Comment on attachment 8573056 [details] [gaia] alivedise:new-bootstrap-0306 > mozilla-b2g:master Did a brief skim and left a couple of nits and comments on the pull request. I do like the direction, but some of the growth around how we leverage Service is a bit scary to me. That could be addressed outside of this pull request though. Nice work!

Update: fixed half of the unit tests; etienne I need your help on the EdgeSwipeDetector failures because I cannot figure out what's wrong with the replacement and it's a little complex to trace. I will be back to this test once I finished others.

Comment on attachment 8573056 [details] [gaia] alivedise:new-bootstrap-0306 > mozilla-b2g:master Did my first pass, comments on github :) The more I think about it the more I think we should just have modules and sub modules (as they are today), and let modules dynamically add sub modules (in an idleObserver or after receiving an event etc...). Also flagging :fcampo for a first feedback of the FTU part since it changes quite a lot and I don't know it that well :) PS: tons of comments and of course some naming nits, but this is impressive work!

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #8) > [Opened question] > * KeyboardManager and InputWindowManager is still deeply coupling. We could > have an InputMethodCore to manage them, but I am not sure if I should/could > do it in this patch. > * SettingsListener should be replaced by SettingsCore request, but there're > too many places to modify (133) so I am afraid I won't do it right now. Yes those are good follow ups :)

Today Update: I rework the InitLogoHandler to be LogoManager(naming!) because it's not test friendly.. It will provide 'poweroff' service so SleepMenu don't need to call reboot on its own.

Comment on attachment 8573056 [details] [gaia] alivedise:new-bootstrap-0306 > mozilla-b2g:master 7-10 seconds is huge! Will really speed up developers as well as being nice UX. Overall this change is great! I left one suggested on github about abstracting the idleTimer logic into the BaseModule. I also agree with Etienne that the name side modules is not completely clear, and it seems like have some be started automatically and some have to be explicitly started (after an idle timeout) is not ideal. Better to separate these two groups I think.

I apologize about my terrible english is comment 15. To clarify, I think we should change the SIDE_MODULES name (or at least clarify with comments), and also have a complete separate XXX_MODULES that only get started after idle.

(In reply to Michael Henretty [:mhenretty] from comment #16) > I apologize about my terrible english is comment 15. To clarify, I think we > should change the SIDE_MODULES name (or at least clarify with comments), and > also have a complete separate XXX_MODULES that only get started after idle. No worry. I agree the naming might be strange... and maybe we need a general way to load the "non-blocking" modules in BaseModule using idle timer or something like that.

(In reply to Kevin Grandon :kgrandon from comment #9) > Comment on attachment 8573056 [details] > [gaia] alivedise:new-bootstrap-0306 > mozilla-b2g:master > > Did a brief skim and left a couple of nits and comments on the pull request. > I do like the direction, but some of the growth around how we leverage > Service is a bit scary to me. That could be addressed outside of this pull > request though. Nice work! The main issue is window.XXXX.AAAA.BBBBB.isActive() does not seem to work because the loading sequence cannot be guaranteed unless they are parent/child relationship. Also I don't want to put all instances under global object... I am open to other solution.

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #17) > (In reply to Michael Henretty [:mhenretty] from comment #16) > > I apologize about my terrible english is comment 15. To clarify, I think we > > should change the SIDE_MODULES name (or at least clarify with comments), and > > also have a complete separate XXX_MODULES that only get started after idle. > > No worry. I agree the naming might be strange... and maybe we need a general > way to load the "non-blocking" modules in BaseModule using idle timer or > something like that. Update * Implemented BaseModule.prototype.loadWhenIdle([ArrayOfSubModule], timeout); Usage: this._start = function() { this.loadWhenIdle(['AppWindowSuspendingManager'], /* optional: timeout after idle */ 10).then(function() { // do something after load.. }); };

Update: Still fixing unit tests. Worthy to mention: I am trying to use hierarchytopmostwindowchanged to replace all the UI events in Statusbar to make it more clear and less dependent from the window mgmt detail.

Update: Looks like all gip fails due to Traceback (most recent call last): File "/home/tester/git_checkout/ci_venv/local/lib/python2.7/site-packages/marionette_client-0.9-py2.7.egg/marionette/marionette_test.py", line 272, in run self.setUp() File "/home/tester/git_checkout/tests/python/gaia-ui-tests/gaiatest/tests/functional/gallery/test_gallery_flick.py", line 15, in setUp GaiaTestCase.setUp(self) File "/home/tester/git_checkout/tests/python/gaia-ui-tests/gaiatest/gaia_test.py", line 763, in setUp self.device.start_b2g() File "/home/tester/git_checkout/tests/python/gaia-ui-tests/gaiatest/gaia_test.py", line 585, in start_b2g self.wait_for_b2g_ready(timeout) File "/home/tester/git_checkout/tests/python/gaia-ui-tests/gaiatest/gaia_test.py", line 593, in wait_for_b2g_ready By.CSS_SELECTOR, '#homescreen[loading-state=false]')) File "/home/tester/git_checkout/ci_venv/local/lib/python2.7/site-packages/marionette_driver-0.2-py2.7.egg/marionette_driver/wait.py", line 143, in until cause=last_exc) Still not sure what's problem. May it's too quick?

Comment on attachment 8573056 [details] [gaia] alivedise:new-bootstrap-0306 > mozilla-b2g:master For the FTU part it looks fine, but I am not really entitled for checking things on system. I just checked that the flags that were used before are still being checked so FTU can run over the rest of processes undisturbed, as always. But indeed the use of submodules makes the code much cleaner now. Maybe Sam wants to give it a go and propose something, as I think he had some ideas about FTU changes

I've been tracking this bug and the FtuLauncher changes look good to me. Pulling out those settings gets and creating the launcher.STATES is a welcome change. I do have some (not yet well formed) ideas for future FTU changes but I don't think anything fundamental to this bootup sequence changes and in either case this looks like a much better base to build on.

Update: Fixed some MIP(Marionette in Python) change, but switched to Multi-Display work in early days of this week. Will back to fix the remaining failures tomorrow. Sorry for delaying.

Status update: I found the most crash is due to direct window.wrappedJSObject.xxx (but I'd remove the xxx from the global window).. I think this could fix most of the issues! Will request review next week.

All right, only a few tests are still orange. Before we start the review: Etienne, it was just you want me to put the mock stuff in mock_service.. did I misunderstand what you wanted ? We could change to the way using stub everywhere in the test code when Service.query & Service.request is used - and this could be a large work - but we need to know what's really desired. Kevin, could you hint me why do we need to turn on FTU + lockscreen here at the same time in the test? https://github.com/mozilla-b2g/gaia/blob/master/apps/privacy-panel/test/marionette/rp_features_test.js#L15 It's out of my imagination - ftu should unlock the lockscreen. Honestly I don't really know what is being tested here... any hint? Etienne, another question for you, https://github.com/mozilla-b2g/gaia/blob/master/apps/system/test/unit/edge_swipe_detector_test.js#L574 this item is failing now and I don't know why - it seems the threshold is related to window.devicePixelRatio which is 2 on the test env, but the arguments of swipe function is 3 to 7. It seems we should mock devicePixelRatio but we didn't. I wonder why this worked before..any idea? Thanks for advance and sorry for the late progress on this bug. :)

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #28) > Kevin, could you hint me why do we need to turn on FTU + lockscreen here at > the same time in the test? > https://github.com/mozilla-b2g/gaia/blob/master/apps/privacy-panel/test/ > marionette/rp_features_test.js#L15 I am not sure, I touched this code as part of a much larger patch when changing test defaults. Have you tried setting ftu.manifestURL to null for this test?

Comment on attachment 8573056 [details] [gaia] alivedise:new-bootstrap-0306 > mozilla-b2g:master I've left a few comment on the BaseModule for you regarding the loading sequence. It's minor so just leave it if that is what you've intended. Beyond this bug, I wish we could rethink between the balance of learning curve to learn Gaia System specific module pattern v.s. good abstraction design that make us DRY. The latter is great but it always comes with the cost of former. Thank you again for taking the initiative!

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #28) > All right, only a few tests are still orange. > Before we start the review: > > Etienne, it was just you want me to put the mock stuff in mock_service.. did > I misunderstand what you wanted ? We could change to the way using stub > everywhere in the test code when Service.query & Service.request is used - > and this could be a large work - but we need to know what's really desired. My goal was to make testing Service-dependent things easy. Because so much of the system code is tied to Service uses and we don't want each test file to mock it in a slightly different way. Now, after seeing how things evolve with Service it's pretty clear (imo) that the way to go is: * a really shallow MockService * a helper so that each test can easily do stuff like |mockQueryWith('SoftwareButtonManager.height', 42)|, like I suggested on github I think the misunderstanding came from "not having each test roll it's own custom-way to mock Service" vs. "putting a lot of responsibilities on the MockService object to remove it from tests". > Etienne, another question for you, > https://github.com/mozilla-b2g/gaia/blob/master/apps/system/test/unit/ > edge_swipe_detector_test.js#L574 > this item is failing now and I don't know why - it seems the threshold is > related to window.devicePixelRatio which is 2 on the test env, but the > arguments of swipe function is 3 to 7. > > It seems we should mock devicePixelRatio but we didn't. I wonder why this > worked before..any idea? No. but +1 on the dpi mock :)

(In reply to Etienne Segonzac (:etienne) from comment #31) > (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #28) > > All right, only a few tests are still orange. > > Before we start the review: > > > > Etienne, it was just you want me to put the mock stuff in mock_service.. did > > I misunderstand what you wanted ? We could change to the way using stub > > everywhere in the test code when Service.query & Service.request is used - > > and this could be a large work - but we need to know what's really desired. > > My goal was to make testing Service-dependent things easy. > Because so much of the system code is tied to Service uses and we don't want > each test file to mock it in a slightly different way. > > Now, after seeing how things evolve with Service it's pretty clear (imo) > that the way to go is: > * a really shallow MockService > * a helper so that each test can easily do stuff like > |mockQueryWith('SoftwareButtonManager.height', 42)|, like I suggested on > github > > I think the misunderstanding came from "not having each test roll it's own > custom-way to mock Service" vs. "putting a lot of responsibilities on the > MockService object to remove it from tests". I want to be clear here: How about that we don't put any defined MockService.mXXXX inside mock_service#query(), but when any test wants to mock a query, it just needs to do: MockService.mQueries[TO_BE_QUERIED_STRING] = certain-value; (Then MockService.query(TO_BE_QUERIED_STRING) will return the value you assigned just when you refer it) assert(....); > > > > Etienne, another question for you, > > https://github.com/mozilla-b2g/gaia/blob/master/apps/system/test/unit/ > > edge_swipe_detector_test.js#L574 > > this item is failing now and I don't know why - it seems the threshold is > > related to window.devicePixelRatio which is 2 on the test env, but the > > arguments of swipe function is 3 to 7. > > > > It seems we should mock devicePixelRatio but we didn't. I wonder why this > > worked before..any idea? > > No. but +1 on the dpi mock :) What's interesting is if I mock the dpi, lots of current test fail :) I will spend some time on this before next review round.

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #32) > (In reply to Etienne Segonzac (:etienne) from comment #31) > > (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #28) > > > All right, only a few tests are still orange. > > > Before we start the review: > > > > > > Etienne, it was just you want me to put the mock stuff in mock_service.. did > > > I misunderstand what you wanted ? We could change to the way using stub > > > everywhere in the test code when Service.query & Service.request is used - > > > and this could be a large work - but we need to know what's really desired. > > > > My goal was to make testing Service-dependent things easy. > > Because so much of the system code is tied to Service uses and we don't want > > each test file to mock it in a slightly different way. > > > > Now, after seeing how things evolve with Service it's pretty clear (imo) > > that the way to go is: > > * a really shallow MockService > > * a helper so that each test can easily do stuff like > > |mockQueryWith('SoftwareButtonManager.height', 42)|, like I suggested on > > github > > > > I think the misunderstanding came from "not having each test roll it's own > > custom-way to mock Service" vs. "putting a lot of responsibilities on the > > MockService object to remove it from tests". > > I want to be clear here: > > How about that we don't put any defined MockService.mXXXX inside > mock_service#query(), > but when any test wants to mock a query, it just needs to do: > > MockService.mQueries[TO_BE_QUERIED_STRING] = certain-value; > (Then MockService.query(TO_BE_QUERIED_STRING) will return the value you > assigned just when you refer it) > assert(....); I think we shouldn't event do the mQueries thing but use sinon instead.

Update: something to do before review request 1. Fix the latest failure (only 2~3 now) 2. Fix what Etienne said. 3. Fix the lockscreen dependency - now homescreen is launched even before lockscreen starts. We need to make sure lockscreen will start if lockscreen is enabled before the homescreen. The proposed fix is having a LockScreenLauncher for now, and let Launcher to tell it to launch/standby by the startup configuration. 4. Add unit test.

Fixed nearly all tests. Do the MockService favor now. ETA is next Wed.

Comment on attachment 8594315 [details] [gaia] alivedise:new-bootstrap-0417 > mozilla-b2g:master All right, I dare not to say it is 100% complete but I think it's ready for the first review round. Review candidate v1: * Add system.waitForFullyLoaded() as suggested; it's still undetermined that how shall define 'fullyLoaded' well - to be strict, I am afraid we should have a long list to represent all the module names to load/start to signal this event, but I am not doing that right now. * Add a scheduler to process the 'loadWhenIdle' request; it's open to use ftuloaded + lockscreen-apploaded together and base on the first app to launch to wait for. But to simplify, now I am using only homescreenloaded event to represent the signal to load the de-prioritized modules. The reason of the scheduler is that I found the idle timer is too slow and I don't think it is scalable to tune the 10sec idle time for every devices. The original idle timer way is still kept if we need to change back. * Add a LockScreenLauncher to bridge between Launcher and LockScreenWindowManager. Greg will work on splitting lockscreen modules after this patch. * Remove all internal variables in MockService and do mockQueryWith as Etienne suggested. * Launcher also takes care LogoManager animation now. * Fixed ALL integration tests. * Statusbar change - I changed the redundant logic of xxxopened event maintanence. There might be bugs so Michael don't hesitate to advise. TODO: Fix the lint error and more unit tests if necessary.

Please request a QA sign off / smoke-test run when you are close to landing.

(In reply to Gregor Wagner [:gwagner] from comment #38) > Please request a QA sign off / smoke-test run when you are close to landing. Sure.

I found that when lockscreen is enabled, wallpaper will be the bottleneck because lockscreen itself is loaded faster then homescreen; so I am setting wallpaper in the launcher to block the init log as well.

A review like this, you don't do all at once :) But in the meantime: > * Add a scheduler to process the 'loadWhenIdle' request; it's open to use > ftuloaded + lockscreen-apploaded together and base on the first app to > launch to wait for. But to simplify, now I am using only homescreenloaded > event to represent the signal to load the de-prioritized modules. The reason > of the scheduler is that I found the idle timer is too slow and I don't > think it is scalable to tune the 10sec idle time for every devices. The > original idle timer way is still kept if we need to change back. I think this will work. The Scheduler name may be a little bit too generic though. Maybe LoadScheduler? (as in: the device is under load and/or it's used to load modules)

(In reply to Etienne Segonzac (:etienne) from comment #41) > A review like this, you don't do all at once :) Do you miss "need" or it means I need to make sure everything is well before review? > But in the meantime: > > > * Add a scheduler to process the 'loadWhenIdle' request; it's open to use > > ftuloaded + lockscreen-apploaded together and base on the first app to > > launch to wait for. But to simplify, now I am using only homescreenloaded > > event to represent the signal to load the de-prioritized modules. The reason > > of the scheduler is that I found the idle timer is too slow and I don't > > think it is scalable to tune the 10sec idle time for every devices. The > > original idle timer way is still kept if we need to change back. > > I think this will work. The Scheduler name may be a little bit too generic > though. > Maybe LoadScheduler? (as in: the device is under load and/or it's used to > load modules) Hmm, my original idea is a generic scheduler. I don't mind to make it specific for only being responsible to lazy load modules for now, but I think that we will need other operation than "just load" afterwards..

There is one thing I am uncertain: do we need to make Gij wait for system fully loaded by default? It took me lot of time to figure out who should wait for fully load. Apparently some tests which does not need system's functionality could ignore the fully load event. (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #42) > > Hmm, my original idea is a generic scheduler. I don't mind to make it > specific for only being responsible to lazy load modules for now, but I > think that we will need other operation than "just load" afterwards.. Modify: Agree "Scheduler" the naming is bad, but LoadScheduler sounds too specific if we are not supporting only load but any operations which should not block the app loading. Or the naming "load" means the loading of the app...

Comment on attachment 8594315 [details] [gaia] alivedise:new-bootstrap-0417 > mozilla-b2g:master This is too much file for review and for Github, so I ended up only look at a few important files. The files I looked are core.js, base_module.js, and startup.js. The responsibility of Core, BaseModule, and Startup is already nicely divided, but it can be more tangible if these files contains comments at the start of the file. I've also noticed BaseModule being modified (re-purposed?) heavily -- something that should be reflected on the comment (the other choice would be move the changes elsewhere -- explaining below). # Startup I have problem with the pattern used Startup. Looks like it has the potential to become yet another unmaintainable bootstrap.js. I recommend the Startup script should be kept at the minimum because that's something can't be included in the unit tests. You can see what I did in Keyboard app and it's comment: https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/js/keyboard/bootstrap.js#L5-L7 (Basically dependencies are started by the first "root" instance, the "app", so the only use of bootstrap.js is and should be starting the "app") Obviously System app is way more complex than Keyboard but you get the idea. Maybe we can make Startup testable and revive the Bootstrap as the first script to run? I don't know. # BaseModule BaseModule looks good except it's a bit overloaded. Should objects like SettingMixin live in it's own file instead? Maybe other mixins too... It's comment need to be updated. # Core I like the idea doing API detection while loading scripts. We do not modify System app heavily on build time -- something I would love to do on top of the feature implemented here. If build scripts could understand the dependencies it could strip or concentrate script accordingly. That would allow us to build different System app for different device type (and save disk spaces). This sounds somewhat similar to what r.js could do -- but just to be clear, I am not advocating using r.js, I am simply saying any tooling we ended up choosing should be able to do the same things. I saw the Promise chain being utilized heavily in Core#_start, which is very good. I however did not see everything being chained together. For example, Core#startAPIHandler return a promise that was not chained in Core#startAPIHandlers. https://github.com/alivedise/gaia/blob/7fa095038fce5a8a970be6cdd4b37657ac0c511b/apps/system/js/core.js#L141 What we should do here is collect all the promises in an array and create an new promise with Promise.all(arr) and return it. This will be helpful for caller of the Core#startAPIHandlers to get that promise and chained it as well. You should be very careful when calling or designing a function returning a Promise. The rejection _should always be caught_. It can be either handled in the function itself (effectively make the returned promise never rejects) or always have the caller to catch the rejection. Not keeping the returned promise like the example above hide the JavaScript errors thus making debugging miserable. This is a known Promise design issue which user of the interface should be aware of. All the call sites of LazyLoader.load(...).then(..) need a .catch() too if you are not returning that promise out to the caller and have the caller to caught the error. # Conclusion Not catching the returned promises is the only thing keeping me from r+ this patch. I believe everything else can be addressed in follow-ups (or bug filed on regression if caught). I don't want to further block this patch from landing because it blocks others longer than we expected. If you manage to put every LazyLoader.load(...) call into one master promise chain started from Core, that would solve your problem of knowing when the app is fully loaded as well. Nor you don't need to worry whether or not rejection is left uncaught (since the master chain must be ended with one |.catch((e) => { e && console.error(e); })|. I don't expect that from this patch but that is a good follow-up to work on. Good job!

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #44) > Comment on attachment 8594315 [details] > [gaia] alivedise:new-bootstrap-0417 > mozilla-b2g:master > > This is too much file for review and for Github, so I ended up only look at > a few important files. The files I looked are core.js, base_module.js, and > startup.js. > > The responsibility of Core, BaseModule, and Startup is already nicely > divided, but it can be more tangible if these files contains comments at the > start of the file. I've also noticed BaseModule being modified > (re-purposed?) heavily -- something that should be reflected on the comment > (the other choice would be move the changes elsewhere -- explaining below). > > # Startup > > I have problem with the pattern used Startup. Looks like it has the > potential to become yet another unmaintainable bootstrap.js. I recommend the > Startup script should be kept at the minimum because that's something can't > be included in the unit tests. > > You can see what I did in Keyboard app and it's comment: > https://github.com/mozilla-b2g/gaia/blob/master/apps/keyboard/js/keyboard/ > bootstrap.js#L5-L7 > (Basically dependencies are started by the first "root" instance, the "app", > so the only use of bootstrap.js is and should be starting the "app") > > Obviously System app is way more complex than Keyboard but you get the idea. > Maybe we can make Startup testable and revive the Bootstrap as the first > script to run? I don't know. > I read keyboard app but found a big difference: all keyboard scripts are put in the header, so your KeyboardApp core module does not need to care about who to load/when to load the submodules. I could reduce the preloaded scripts number in startup.js, but the most feasible way to reduce startup size for now is only move them into Core and make Core become bigger.

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #42) > (In reply to Etienne Segonzac (:etienne) from comment #41) > > A review like this, you don't do all at once :) > > Do you miss "need" or it means I need to make sure everything is well before > review? > Oh no! Just wanted to say that I was providing early feedback before my review was finished :)

Had an offline discussion with Tim; he proposed an idea to let BaseModule returns all the promise of the submodule's start function. With this we could gather the big start chain in start.js to know eveything is started and then we could 1. catch all errors once and 2. signal the 'fullyLoaded' event. Trying to see if this works.

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #47) > Had an offline discussion with Tim; he proposed an idea to let BaseModule > returns all the promise of the submodule's start function. With this we > could gather the big start chain in start.js to know eveything is started > and then we could 1. catch all errors once and 2. signal the 'fullyLoaded' > event. > > Trying to see if this works. The attempt was successful. Now we have the visuallyLoaded in 5th sec after boot, and fullyLoaded in 13th sec. The two loaded event timing does not change too much with or without the new start promise chaining. I am writing some tests now. For other people who is still reviewing: * BaseModule is chaining all submodules' start() function together in parent start(). So a modules start() promise will be "then"ed after all submodules' start() was done. * Most preloaded scripts in startup.js is moved into specific modules. No big change in addition to above.

Most change happened in the second commit. * BaseModule supports the startup chaining submodules * Move bootstrap out from startup and rename startup to app The launching path and critical modules are clear now: 1. bootstrap will start app once document loaded 2. app will 2.1. Preload common scripts 2.2. Load/start settingsCore + launcher + core in certain ordering 3. Module should return a promise in the _start if it wants to block the start progress. The only tricky part is the complex promise chain in app.js and launcher.js maybe not test-able..

Comment on attachment 8597944 [details] Start chaining Greg, could you help on WallpaperManager and Lockscreen* related changes? I am trying to understand the promise wraps are of the desired effect.

For the LockScreen part we have had a discussion that we need to 'launch' LockScreen in different phase: 1. If there should be FTU, then it should 'launch' in 'standby' mode that only start LockScreenWindow when powerbutton clicked. 2. If there is no FTU, then it should 'launch' in real launch mode, which would start the window immediately. However, for WallpaperManager, I were not in discussion and have no clue about the change. So if you want me to comment it I need to survey it first...

And I don't know if you would see the comment without NI, so I NI you.

Also, I think if Alive's intention includes to update the current convention in System app, please states it, especially in comments. I don't want to submit a patch that follow the 'current' convention but reviewer rejects it just because we now only accept the new, implicitly adopted convention. This is a part of the comment issues I also have addressed on the page, but obviously no one replies my reply after the original comments of the PR.

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #51) > For the LockScreen part we have had a discussion that we need to 'launch' > LockScreen in different phase: > > 1. If there should be FTU, then it should 'launch' in 'standby' mode that > only start LockScreenWindow when powerbutton clicked. > > 2. If there is no FTU, then it should 'launch' in real launch mode, which > would start the window immediately. > > However, for WallpaperManager, I were not in discussion and have no clue > about the change. So if you want me to comment it I need to survey it > first... The wallpaper change is because we need to paint the background before loading lockscreen, otherwise we will the black lockscreen shows, and then blink with the background image. So launcher will get the wallpaper settings ahead from wallpaperManager - but wallpaperManager is still standalone if we removed the launcher. If launcher gets the settings for the wallpaper manager it does not need to get the settings again and could paint background right away after it is launched. That's why we launch the lockscreen after the wallpaper initialization promise resolved.

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #51) > However, for WallpaperManager, I were not in discussion and have no clue > about the change. So if you want me to comment it I need to survey it > first... I simply like you to read the changes in WallpaperManager and see if the promises are chained correctly. (In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #53) > Also, I think if Alive's intention includes to update the current convention > in System app, please states it, especially in comments. I don't want to > submit a patch that follow the 'current' convention but reviewer rejects it > just because we now only accept the new, implicitly adopted convention. This > is a part of the comment issues I also have addressed on the page, but > obviously no one replies my reply after the original comments of the PR. We certainly need more documentations but I think everyone can help if we have communicated orally, not just leave Alive to do it!

Please see previous comment (Bugzilla prevented me to cancel it previously)

Comment on attachment 8594315 [details] [gaia] alivedise:new-bootstrap-0417 > mozilla-b2g:master Wow, impressive work. I don't know how you managed to do this all in a single patch :) Question - do you think it's possible to split it up into a few smaller pieces to attack? Seems difficult to review otherwise, but we can probably manage. Just to sound like a broken record player - I do think that we should strive to move towards harmony modules at some point, possibly using babel. One concern that I have is that the design decisions behind base module may make harmony module work much more difficult to implement. I would much rather see this landed sooner rather than later though, so I'm happy with whatever you think the best step is for now to achieve that.

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #55) > (In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #51) > > However, for WallpaperManager, I were not in discussion and have no clue > > about the change. So if you want me to comment it I need to survey it > > first... > > I simply like you to read the changes in WallpaperManager and see if the > promises are chained correctly. > > > (In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #53) > > Also, I think if Alive's intention includes to update the current convention > > in System app, please states it, especially in comments. I don't want to > > submit a patch that follow the 'current' convention but reviewer rejects it > > just because we now only accept the new, implicitly adopted convention. This > > is a part of the comment issues I also have addressed on the page, but > > obviously no one replies my reply after the original comments of the PR. > > We certainly need more documentations but I think everyone can help if we > have communicated orally, not just leave Alive to do it! I think as you and others have tried to achieve before, the 'if' should happen on Bugzilla, PR page, and the code. If my opinions are wrong, correct me. If not, keeps it. There are nothing we couldn't discuss in public (and most important, with persistent records), but ignorance is definitely not a good way to push things done, not to mention it would nullify the discussion. Also, I don't think to add comments is the duty of 'everyone', while maybe the Wiki and diagram parts are, especially when you're obviously the first person in this battle to try to land a whole new architecture, which seems also would change the current convention in whole System app. Because even if we're all played the best audiences and listened the ideas again and again, we're still not the guy most familiar to the code, not to mention how to comment it (although I actually tried to do that: see the PR page, just at the no replies' reply). If so, I should leave the major commenting work in new LockScreen state machine to others (like: you), and hope the 'others' could figure out what I'm trying to do with that (and fire follow-up bugs to, weirdly, land the comments). You can argue that Alive's patch is much large than mine, but it's just because it's big, and with so much impacts, to comment it is become more important, and to explain the intention in code by others would become more difficult, and even dangerous. In fact, my major concerns about the comments and convention could be summarized as some questions, that I think the code, comment and discussion gives no clear explanation, although maybe it just because that LockScreen is always the borderland of System app, so everyone else have had already know all of the details that even enough to add such comments just like you said: 1. Would Alive want, no matter how far the goal is, to unify the current (unfortunate) multiple conventions into BaseModule mixin, just like this patch does? 2. What's the reason it deserves no more detailed comments in 'core.js'? It's now without any documenting comments to explain any architecture information about the module itself. 3. And, if it's possible, and if it's proper, to keep the current fashion 'instantiable classes' mixin with 'BaseModule'? Or should we change all 'instantiable classes' into literal object + mixin like those used in this patch? Maybe you're thinking I'm trolling, but I'm NOT. For me, the LockScreen part is fine, although it (again) looks lack of comments in launcher, so you asked the Comment 50 while we indeed have had some offline discussions as Comment 51. Anyway, you real owners could decide whether it's good to land, and who should write the comments.

(In reply to Kevin Grandon :kgrandon from comment #57) > Just to sound like a broken record player - I do think that we should strive > to move towards harmony modules at some point, possibly using babel. One > concern that I have is that the design decisions behind base module may make > harmony module work much more difficult to implement. I would much rather > see this landed sooner rather than later though, so I'm happy with whatever > you think the best step is for now to achieve that. Your opinion is valuable. I am sorry I haven't give you a proper response. So I've spent some time looking at es6 modules and I think it's simply an agreed module interface. It will not and should not define what kind of pattern we choose for the code inside it. For example, CommonJS does not ask people always expose a singleton or a function -- everything can be exposed as long as the reference is ultimately point to module.exports, as explained in [1]. [1] http://bites.goodeggs.com/posts/export-this/ I hope this clear things up and explain that BaseModule pattern does not prevent us from using es6 module. HOWEVER, I do agree with you that the usage of LazyLoader probably does, but it does not a problem of BaseModule but a overall optimization we should try in the future. Usage of LazyLoader currently blocks *any* optimization except minify the individual script. If your proposed es6 module fix can provide dependency-based concatenation and somehow keep the syntax looks sync, that's would be superb even compare to r.js.

(In reply to Greg Weng [:snowmantw][:gweng][:λ] from comment #58) You are over-interpreting what I said. 1) I agree some comments are necessary, as I pointed out on Github. 2) I believe simply questions like "How do I create a new module leveraging BaseModule" can be answered on the wiki by people other than Alive since people have the experiences. 3) Agree with you that Alive should document the overall arch design and rationale. For sure. Are these something you would agree with?

* 1) ... some comments necessary before landing.

Yes, I think both Wiki and comments should be as clear and much as possible. I would especially look forward comments because the external documents may be outdated, and there is no review process to force to update them.

Sounds good to me. Moving forward with this first is probably the best thing to do. Thanks!

I read the opinions around BaseModule in github and I do think it is out of scope about this patch; either supporting BaseModule.create(prototype) or BaseModule.mix(prototype) is something should not happen in this patch. What I am going to do now is: 0. Fix the convention change in lockscreen.js (already done in latest patch) 1. Keep stabilizing the patch - however gaia try is dead now. 2. Make a diagram to explain how this works generally. 3. Cherry pick Chens' patch in bug 1154655 to fix the jshint errors (special thanks to him.) @Kevin, I am not sure I have the answer for how to split it - before I finish the 3 action items above. And if it's stable enough, why not just merge but spend more time to create new commits and make sure every commits rebased to latest master + everything work as expected. The only idea I have now is 1. Have a commit to lazy load scripts which is only necessary for a specific module and remove it from the header of system app. 2. Have a commit to remove all direct access to window.XXXXX and use Service.request instead 3. Have a commit to change BaseModule start function() 4. Have a commit to deal with all window management change starting from AppCore 5. Have one or more commits to deal with other sub system change respectively 6. Migrate bootstrap to app + launcher + scheduler 7. Things I miss So again, I cannot gurantee how much time to split it and make sure every commit is standalone; I could give it a try after my action item is done.

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #59) > I hope this clear things up and explain that BaseModule pattern does not > prevent us from using es6 module. Just to add my two cents here, I spent some time looking into converting BaseModule into an es6 module, and while it is not impossible, there are several difficulties we should be aware of: 1.) es6 does not support static properties, only static functions. So doing things like BaseModule.SUB_MODULES = [] will need to be done outside of the module definition. Conversely we could make SUB_MODULES an instance member defined by the constructor. 2.) Similarly, es6 does not allow you to define properties on the prototype. So something like Module.prototype.name cannot be defined within the class definition. We can work around this by adding these properties in the constructor, but this will be placing the property on the instance rather than the prototype which could have some side-effects. 3.) BaseModule.create obviates the need for class `extends`, which is one of the big benefits of es6 modules IMO (ie. classical inheritance with classical syntax). Since we rely on multiple inheritance for BaseModule, I don't think we can get away from the mixin paradigm. However, if we were to get rid of Basemodule.create and instead leverage BaseModule->constructor (make sure sub classes invoke the appropriate parent constructor), I think we can keep the functionality of BaseModule.create with es6 syntax. In any case, non of these items are show stoppers for es6 in the system app, but it is not a trivial amount of work converting everything. Also, given the above items, I don't think we can convert 1 module at a tim, because I think the es6 version of base module would be incompatible with the current version. I could be wrong though.

This is really useful information.. big thanks Michael. (In reply to Michael Henretty [:mhenretty] from comment #65) > (In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to > queue) from comment #59) > > I hope this clear things up and explain that BaseModule pattern does not > > prevent us from using es6 module. > > Just to add my two cents here, I spent some time looking into converting > BaseModule into an es6 module, and while it is not impossible, there are > several difficulties we should be aware of: > > 1.) es6 does not support static properties, only static functions. So doing > things like BaseModule.SUB_MODULES = [] will need to be done outside of the > module definition. Conversely we could make SUB_MODULES an instance member > defined by the constructor. > > 2.) Similarly, es6 does not allow you to define properties on the prototype. > So something like Module.prototype.name cannot be defined within the class > definition. We can work around this by adding these properties in the > constructor, but this will be placing the property on the instance rather > than the prototype which could have some side-effects. > > 3.) BaseModule.create obviates the need for class `extends`, which is one of > the big benefits of es6 modules IMO (ie. classical inheritance with > classical syntax). Since we rely on multiple inheritance for BaseModule, I > don't think we can get away from the mixin paradigm. However, if we were to > get rid of Basemodule.create and instead leverage BaseModule->constructor > (make sure sub classes invoke the appropriate parent constructor), I think > we can keep the functionality of BaseModule.create with es6 syntax. BaseModule.create is something similar to Object.create which I think it's not far away from "make a class based on baseModule class and extends the class with this object". IMO in the long run all modules in the system app who wants to have the functionality of BaseModule should just extend it. > > In any case, non of these items are show stoppers for es6 in the system app, > but it is not a trivial amount of work converting everything. Also, given > the above items, I don't think we can convert 1 module at a tim, because I > think the es6 version of base module would be incompatible with the current > version. I could be wrong though. Looks like this is the "biggest" problem (if we think a big patch is a problem) when migrating BaseModule into es6 form.

I think this(3rd commit mostly) fixes what you addressed most in the previous commit.

I wrote a parser to traverse the tree starting from core in run time, here is the parser code. I will upload the d3 result page soon. var go4ru = function(object, prefix, level, tree) { if (object.LEVEL) { return; } if (!tree) { tree = {}; } tree.name = object.name; tree.children = []; var index = 0; for (var key in object) { if (object.hasOwnProperty(key) && object[key]) { if (object[key].service || object[key].start || object[key].init) { console.log((prefix ? prefix : '') + object.name + '->' + key); object.LEVEL = level || 0; tree.children.push({}); go2(object[key], (prefix ? prefix : '') + object.name + '->' + key + '->', object.LEVEL++, tree.children[index]); index++; } } } return tree; }

Comment on attachment 8597944 [details] Start chaining For LockScreen part I know what's going on so it's good for me. Let's see if others feel it's hard to have the clue according to the code.

Comment on attachment 8599726 [details] Start chaining fix Left some comments on the Github. I've read through the entire patch(!) so the final review should be only about verifying these comments are fixed, after all tests passes.

For those who are still lost in 0417: There're 3 commits here: 0417 + start chain fix (f+ from Tim) + jshint fix (from Chens). This would be very close to the final patch.

(In reply to Michael Henretty [:mhenretty] from comment #65) > (In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to > queue) from comment #59) > > I hope this clear things up and explain that BaseModule pattern does not > > prevent us from using es6 module. > > Just to add my two cents here, I spent some time looking into converting > BaseModule into an es6 module, and while it is not impossible, there are > several difficulties we should be aware of: > > 1.) es6 does not support static properties, only static functions. So doing > things like BaseModule.SUB_MODULES = [] will need to be done outside of the > module definition. Conversely we could make SUB_MODULES an instance member > defined by the constructor. > > 2.) Similarly, es6 does not allow you to define properties on the prototype. > So something like Module.prototype.name cannot be defined within the class > definition. We can work around this by adding these properties in the > constructor, but this will be placing the property on the instance rather > than the prototype which could have some side-effects. > The fact that ES6 does not support static property & instance property scared me so I did some investigation. Actually someone is considering to take property back to class in es7 https://gist.github.com/jeffmo/054df782c05639da2adb And in babel, people are talking about this feature too https://github.com/babel/babel/issues/619 So I don't think utilizing prototype property and class property is the mistake of BaseModule; it is used everywhere in the world in prototype based modules. As I always think and believe: language is just a tool. Tool will change by days, what you think won't; we should not get lost in the language feature but stick to more conceptual stuff. I did not say es6 is bad; its semantic sugar is nice. I think the real potential issue here is what Tim had addressed: the usage of lazy loader will be harmful when we want to minimize the js files in the future. AFAIK even es6 does not provide a solution for module loading - yes, I know it's proposing "import" but there's no real browser implementation right now.

Comment on attachment 8594315 [details] [gaia] alivedise:new-bootstrap-0417 > mozilla-b2g:master Days before I reviewed the following scripts and they are looking good to me. The only comment regarding loading time of the mime helper I left in github. - apps/system/js/bluetooth.js - apps/system/js/bluetooth_core.js - apps/system/js/bluetooth_transfer.js - apps/system/js/device_storage_watcher.js - apps/system/js/operator_variant_handler.js - apps/system/js/operator_variant_manager.js - apps/system/js/radio.js And I just realized that I missed one file to review, apps/system/js/telephony_settings.js. The functions originally called in `_start` are not handlers for settings changes but should be called when initializing. Please make sure they are only called once.

Comment on attachment 8594315 [details] [gaia] alivedise:new-bootstrap-0417 > mozilla-b2g:master Gave this patch a good spin this morning, marking a r- for now since there are blocking issues. But only a few and they are pretty straightforward :) Dumping here since I'm getting lost in all the PR. ## LogoManager * I thought we were missing the poweron animation, but it's just that the screen isn't turned on while it happens :) We should probably add a |Service.request('turnScreenOn')| somewhere. * Also, since it's a new file I'll let myself nit pick: it's not really managing a logo we should maybe rename it to something like |CoverScreen|. ## Scheduler * We talked about renaming the |callback| parameter to |block| or |request| but I don't think this change got into the last PR. ## FindMyDeviceLauncher * We're missing a LazyLoad of |SettingsHelper| I think, seeing "ReferenceError"s at boot time. ## TaskManager/AppWindowManager When launching the TaskManager all cards are squished on top of each others. They're only properly placed when an interaction begins. * The AppWindowManger has 2 |cardviewclosed| listed in it's EVENTS array, we should fix that and also group the cardview* events in the list. (although this is not causing the issue) * The LazyLoader call added in |task_manager.js| is faulty. It's currently inside the |addCard| method which is only called inside a loop :/ We should put the LazyLoader call outside of the loop. And call the |_placeCards()| method after the loop. Otherwise the cards are not instantiated by the time we call |_placeCards()| hence the squishing :) ## UpdateManager / .init() everythwere The |UpdateManager.init()| method is called twice. Causing duplicates in the updates list. This is a symptom of a wider issues since a fair amount of old modules files finish with a |mozL10n.once(Something.init.bind(Something)| call. *But* in |base_module.js| we also call |init| in |_initialSubModule| (nit: could be renamed |_initializeSubModule| or |_initSubModule| since "initial" means something else). I think this double init issue might affect: * UpdateManager * DeviceStorageWatcher * FxAccountUI * Statusbar (!!) Note: I still haven't got to reviewing the tests, next review round :)

Comment on attachment 8600860 [details] Latest patch with part2 + jshint fix Test are all green.

Comment on attachment 8600860 [details] Latest patch with part2 + jshint fix Cancel the request first as the comments on telephony_settings.js seem not being addressed yet.

Comment on attachment 8600860 [details] Latest patch with part2 + jshint fix I decided not to deprecate SettingsHelper usage in this patch.

Comment on attachment 8600860 [details] Latest patch with part2 + jshint fix r=me for scripts mentioned in comment 75, thanks.

(In reply to Etienne Segonzac (:etienne) from comment #76) > Comment on attachment 8594315 [details] > [gaia] alivedise:new-bootstrap-0417 > mozilla-b2g:master > > Gave this patch a good spin this morning, marking a r- for now since there > are blocking issues. But only a few and they are pretty straightforward :) > > Dumping here since I'm getting lost in all the PR. > > ## LogoManager > * I thought we were missing the poweron animation, but it's just that the > screen isn't turned on while it happens :) > We should probably add a |Service.request('turnScreenOn')| somewhere. Hmm...I think we are in different page. What do you mean by 'poweron animation'? Do you have a custom carrier logo? I don't think we need to turn screen on programmatically.. It is already on when loading system app. At least in my device I am seeing "ThunderXXXX" totally black for near 1 sec. screen becomes lighter init logo shows (mozilla tech) 2secs later, init logo disappears. > > * Also, since it's a new file I'll let myself nit pick: it's not really > managing a logo we should maybe rename it to something like |CoverScreen|. > You do like XXXXScreen naming very much! I admit LogoManager is a bad one tough.

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #81) > (In reply to Etienne Segonzac (:etienne) from comment #76) > > Comment on attachment 8594315 [details] > > [gaia] alivedise:new-bootstrap-0417 > mozilla-b2g:master > > > > Gave this patch a good spin this morning, marking a r- for now since there > > are blocking issues. But only a few and they are pretty straightforward :) > > > > Dumping here since I'm getting lost in all the PR. > > > > ## LogoManager > > * I thought we were missing the poweron animation, but it's just that the > > screen isn't turned on while it happens :) > > We should probably add a |Service.request('turnScreenOn')| somewhere. > > Hmm...I think we are in different page. What do you mean by 'poweron > animation'? Do you have a custom carrier logo? > > I don't think we need to turn screen on programmatically.. It is already on > when loading system app. The evidence is even I remove 'ScreenManager' from Core, the screen is not off after booting.

This should fix you comments except the power-on animation one - as if I know what's really wrong I could fix it. Sorry for another pull request because I don't want to affect others who are reviewing another one, and I want to accelerate the review process. Thanks for reading this big patch in detail, I know it's really time consuming.

(In reply to Etienne Segonzac (:etienne) from comment #76) > > ## UpdateManager / .init() everythwere > The |UpdateManager.init()| method is called twice. Causing duplicates in the > updates list. This is a symptom of a wider issues since a fair amount of old > modules files finish with a |mozL10n.once(Something.init.bind(Something)| > call. *But* in |base_module.js| we also call |init| in |_initialSubModule| > (nit: could be renamed |_initializeSubModule| or |_initSubModule| since > "initial" means something else). I think this double init issue might affect: > > * UpdateManager > > * DeviceStorageWatcher > > * FxAccountUI > > * Statusbar (!!) > > Note: I still haven't got to reviewing the tests, next review round :) Sadly found not only what you listed but also StackManager/SheetTransition/MobileIdManager/... I am only leaving only few modules to start themselves

Comment on attachment 8602531 [details] Address etienne's comment Too many new failures, cancel first until I fix them

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #81) > (In reply to Etienne Segonzac (:etienne) from comment #76) > > Comment on attachment 8594315 [details] > > [gaia] alivedise:new-bootstrap-0417 > mozilla-b2g:master > > > > Gave this patch a good spin this morning, marking a r- for now since there > > are blocking issues. But only a few and they are pretty straightforward :) > > > > Dumping here since I'm getting lost in all the PR. > > > > ## LogoManager > > * I thought we were missing the poweron animation, but it's just that the > > screen isn't turned on while it happens :) > > We should probably add a |Service.request('turnScreenOn')| somewhere. > > Hmm...I think we are in different page. What do you mean by 'poweron > animation'? Do you have a custom carrier logo? I mean the screen with a blue background and "Firefox OS" written on it. > I don't think we need to turn screen on programmatically.. It is already on > when loading system app. At least in my device I am seeing > "ThunderXXXX" > totally black for near 1 sec. > screen becomes lighter > init logo shows (mozilla tech) > 2secs later, init logo disappears. > interesting, on my device when the screen turns on we're already on the lockscreen. (but on gaia master I see the coverscreen and everything) > > > > * Also, since it's a new file I'll let myself nit pick: it's not really > > managing a logo we should maybe rename it to something like |CoverScreen|. > > > > You do like XXXXScreen naming very much! I admit LogoManager is a bad one > tough. :) Open to suggestions!

Comment on attachment 8602531 [details] Address etienne's comment I think it's ready for 2nd round.. Maybe this fix the init logo problem? I am seeing the init logo but not the blue background firefox both in this patch and master.

Comment on attachment 8600860 [details] Latest patch with part2 + jshint fix r+ as my comments are addressed.

Comment on attachment 8594315 [details] [gaia] alivedise:new-bootstrap-0417 > mozilla-b2g:master Let's switch to the new patch

Comment on attachment 8602531 [details] Address etienne's comment Most of my comments were addressed in earlier feedback cycles, but I left some comments on statusbar.js. Overall, I am eager to see this land, since I'm sure it's a nightmare to maintain. That said, we should be alert for regressions that this patch causes, and fix them quickly since backing this out will also prove problematic.

I haven't find any bug from manual exploratory test yet. But I was not able to run the gaia ui test on flame, I got this error for every test case: TEST-START | test_edit_contact.py TestContacts.test_edit_contact TEST-UNEXPECTED-ERROR | test_edit_contact.py TestContacts.test_edit_contact | JavascriptException: JavascriptException: TypeError: window.wrappedJSObject.AppWindowManager is not a constructor stacktrace: execute_script @gaia_test.py, line 138 inline javascript, line 39 src: " new window.wrappedJSObject.AppWindowManager();" Traceback (most recent call last): File "/home/slyu/.virtualenvs/gaiauitest/local/lib/python2.7/site-packages/marionette_client-0.8.6-py2.7.egg/marionette/marionette_test.py", line 250, in run self.setUp() File "/home/slyu/workspace/1094759/gaia/tests/python/gaia-ui-tests/gaiatest/tests/functional/contacts/test_edit_contact.py", line 16, in setUp GaiaTestCase.setUp(self) File "/home/slyu/workspace/gaia/tests/python/gaia-ui-tests/gaiatest/gaia_test.py", line 805, in setUp self.cleanup_gaia(full_reset=True) File "/home/slyu/workspace/gaia/tests/python/gaia-ui-tests/gaiatest/gaia_test.py", line 842, in cleanup_gaia self.apps.kill_all() File "/home/slyu/workspace/gaia/tests/python/gaia-ui-tests/gaiatest/gaia_test.py", line 101, in kill_all for app in self.running_apps(include_system_apps=True): File "/home/slyu/workspace/gaia/tests/python/gaia-ui-tests/gaiatest/gaia_test.py", line 138, in running_apps "return GaiaApps.getRunningApps(%s);" % include_system_apps) File "/home/slyu/.virtualenvs/gaiauitest/local/lib/python2.7/site-packages/marionette_client-0.8.6-py2.7.egg/marionette/marionette.py", line 1278, in execute_script filename=os.path.basename(frame[0])) File "/home/slyu/.virtualenvs/gaiauitest/local/lib/python2.7/site-packages/marionette_client-0.8.6-py2.7.egg/marionette/decorators.py", line 36, in _ return func(*args, **kwargs) File "/home/slyu/.virtualenvs/gaiauitest/local/lib/python2.7/site-packages/marionette_client-0.8.6-py2.7.egg/marionette/marionette.py", line 634, in _send_message self._handle_error(response) File "/home/slyu/.virtualenvs/gaiauitest/local/lib/python2.7/site-packages/marionette_client-0.8.6-py2.7.egg/marionette/marionette.py", line 687, in _handle_error raise errors.JavascriptException(message=message, status=status, stacktrace=stacktrace) TEST-INFO took 5302ms

I guess we also need to modify below three parts. https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/marionette_test.py#477 https://github.com/mozilla-b2g/gaia/blob/master/tests/atoms/gaia_apps.js#L248 https://github.com/mozilla-b2g/gaia/blob/master/tests/atoms/gaia_apps.js#L39

Not sure how this file is used, but now the way to reference AppWindowManager is already changed. Jonathan, could you review this? Thanks.

(In reply to George Duan [:gduan] [:喬智] from comment #92) > I guess we also need to modify below three parts. > https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/ > marionette/marionette_test.py#477 Cool finding > https://github.com/mozilla-b2g/gaia/blob/master/tests/atoms/gaia_apps.js#L248 > https://github.com/mozilla-b2g/gaia/blob/master/tests/atoms/gaia_apps.js#L39 This should already being done in the gaia patch.

Comment on attachment 8602531 [details] Address etienne's comment r=me with the few test related comments addressed: - the missing test in edge_swipe_detector_test.js - the missing test in scheduler_test.js - the failure in statusbar_home_button_test.js I agree with Mike, at this point the best way forward is to land soon and invest in bug fixing instead of investing in maintaining the branch. It's in the hands of QA now :) Took the liberty to add Naoki since we need all the help we can get on this front. FTR, with the branding on (browser has a cool firefox logo) I'm still missing the "blue Firefox OS" poweron screen (because the hardware screen only turns on once we're on the lockscreen).

Patch rebased and CI green again.

Hi Alive, is there any message in the adb log that can help us verify the boot sequence? Or does this boot sequence happens before adb log is ready?

(In reply to Shing Lyu [:slyu] from comment #98) > Hi Alive, is there any message in the adb log that can help us verify the > boot sequence? Or does this boot sequence happens before adb log is ready? I don't think you need that...it's inner behavior of the code. the main purpose is to make sure the all lazy loaded functionalities still works fine. Anyway, you could turn on gaia debug trace ('debug.gaia.enabled') and you will see *lots of* logs.

I found some new problem with passcode lock and SIM PIN lock. Precondition: SIM PIN lock and passcode lock are both ON. I) 1. Unlock the phone immediately after boot up, the passcode lock does not activate. See the recording here: https://youtu.be/ArR4AlAvV20 II) 1. Wait for a few seconds after boot up 2. Unlock the phone, enter the correct passcode lock 3. Wait for the SIM PIN screen to show up 4. The passcode lock layer overlaps the SIM PIN screen, so it's impossible to unlock the SIM PIN 5. Press the home button to skip SIM PIN, the homescreen is shifted up by the height of a status bar. See the recording here: https://youtu.be/1wVFNNNNTt0

(In reply to Shing Lyu [:slyu] from comment #101) > I found some new problem with passcode lock and SIM PIN lock. > > Precondition: SIM PIN lock and passcode lock are both ON. > > I) 1. Unlock the phone immediately after boot up, the passcode lock does not > activate. See the recording here: https://youtu.be/ArR4AlAvV20 > > II) 1. Wait for a few seconds after boot up > 2. Unlock the phone, enter the correct passcode lock > 3. Wait for the SIM PIN screen to show up > 4. The passcode lock layer overlaps the SIM PIN screen, so it's > impossible to unlock the SIM PIN > 5. Press the home button to skip SIM PIN, the homescreen is shifted up > by the height of a status bar. > See the recording here: https://youtu.be/1wVFNNNNTt0 Great. 1st one is confirmed and I am fixing it. I am not able to reproduce the second one though.

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #102) > (In reply to Shing Lyu [:slyu] from comment #101) > > I found some new problem with passcode lock and SIM PIN lock. > > > > Precondition: SIM PIN lock and passcode lock are both ON. > > > > I) 1. Unlock the phone immediately after boot up, the passcode lock does not > > activate. See the recording here: https://youtu.be/ArR4AlAvV20 > > > > II) 1. Wait for a few seconds after boot up > > 2. Unlock the phone, enter the correct passcode lock > > 3. Wait for the SIM PIN screen to show up > > 4. The passcode lock layer overlaps the SIM PIN screen, so it's > > impossible to unlock the SIM PIN > > 5. Press the home button to skip SIM PIN, the homescreen is shifted up > > by the height of a status bar. > > See the recording here: https://youtu.be/1wVFNNNNTt0 > > Great. 1st one is confirmed and I am fixing it. I am not able to reproduce > the second one though. PR updated; I fixed case 1 and I believe case 2 is side effect from case 1. Please test again. Thanks!

Working on it, thanks!

(In reply to Alive Kuo from comment #103) The two issues are verified fixed! Thanks!

This attachment is what I've tested during exploratory testing.

Comment on attachment 8610009 [details] test_charter.md Hi, I think #1 is fixed, and I understand #2 is side effect. Could you confirm Bug#3 is regression? Is there any video or screenshot?

Bug 3 is not related to this patch. I can reproduce it on builds without this patch. I'll file another bug for that.

(In reply to Shing Lyu [:slyu] from comment #109) > Bug 3 is not related to this patch. I can reproduce it on builds without > this patch. I'll file another bug for that. May I know what's the remaining testing plan of the patch? Thanks!

We just need to make sure it can run GIP, then we are ready to land. I applied your marionette patch but got into this error for every GIP test: ============================= TEST-UNEXPECTED-ERROR | test_add_new_contact.py TestContacts.test_add_new_contact | JavascriptException: JavascriptException: TypeError: app is undefined stacktrace: execute_script @gaia_test.py, line 109 inline javascript, line 382 src: " while (app.frontWindow && app.frontWindow.isActive()) {" Traceback (most recent call last): File "/home/slyu/.virtualenvs/boot/local/lib/python2.7/site-packages/marionette_client-0.13-py2.7.egg/marionette/marionette_test.py", line 277, in run self.setUp() File "/home/slyu/workspace/gaia/tests/python/gaia-ui-tests/gaiatest/gaia_test.py", line 920, in setUp self.cleanup_gaia(full_reset=True) File "/home/slyu/workspace/gaia/tests/python/gaia-ui-tests/gaiatest/gaia_test.py", line 953, in cleanup_gaia self.device.turn_screen_off() File "/home/slyu/workspace/gaia/tests/python/gaia-ui-tests/gaiatest/gaia_test.py", line 742, in turn_screen_off apps.switch_to_displayed_app() File "/home/slyu/workspace/gaia/tests/python/gaia-ui-tests/gaiatest/gaia_test.py", line 117, in switch_to_displayed_app self.marionette.switch_to_frame(self.displayed_app.frame) File "/home/slyu/workspace/gaia/tests/python/gaia-ui-tests/gaiatest/gaia_test.py", line 109, in displayed_app result = self.marionette.execute_script('return GaiaApps.getDisplayedApp();') File "/home/slyu/.virtualenvs/boot/local/lib/python2.7/site-packages/marionette_driver-0.7-py2.7.egg/marionette_driver/marionette.py", line 1452, in execute_script filename=os.path.basename(frame[0])) File "/home/slyu/.virtualenvs/boot/local/lib/python2.7/site-packages/marionette_driver-0.7-py2.7.egg/marionette_driver/decorators.py", line 36, in _ return func(*args, **kwargs) File "/home/slyu/.virtualenvs/boot/local/lib/python2.7/site-packages/marionette_driver-0.7-py2.7.egg/marionette_driver/marionette.py", line 711, in _send_message self._handle_error(response) File "/home/slyu/.virtualenvs/boot/local/lib/python2.7/site-packages/marionette_driver-0.7-py2.7.egg/marionette_driver/marionette.py", line 747, in _handle_error raise errors.lookup(status)(message, stacktrace=stacktrace) TEST-INFO took 6170ms ============================== I am using the m-c pvt build + make reset-gaia with your patch applied + marionette-client 0.13 with your patch applied. Any thoughts? Am I using the wrong marionette version?

OK. seems to be working now. Just give me a second to finish the GIP tests.

Sorry, still have the problem in comment 112. Is it related to the last patch? How do I apply it to new-bootstrap-0507 branch? Do I need to rebase to the latest m-c?

(In reply to Shing Lyu [:slyu] from comment #114) > Sorry, still have the problem in comment 112. > > Is it related to the last patch? How do I apply it to new-bootstrap-0507 > branch? Do I need to rebase to the latest m-c? I don't think so...please ignore the latest patch because it's just for compatibility. I think I should find someone who is familiar with marionette to fix your problem. One moment.

Thanks Alive. The GIP is now working. I think this patch is ready to land. However, we might need to send an email to the mailing list so people running GIP on real devices can prepare for transition.

Here is the plan to merge: 1. I will land the one line patch of gaia at first once CI is green https://bugzilla.mozilla.org/attachment.cgi?id=8610975 2. After 1., I will mark checkin-needed for the gecko patch https://bugzilla.mozilla.org/attachment.cgi?id=8604504 3. Once 2. is landed, we will land new-bootstrap-0507 and close this bug.

Hi Alive, will this patch set resolve the emulator start-up problem of bug 1108271?

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #118) > Hi Alive, > > will this patch set resolve the emulator start-up problem of bug 1108271? It's first time I knew this issue. But the answer is positive - we could do something very simple to achieve after this patch landed I believe.

Fantastic! Thank you!

I just landed the backward-compatible patch https://github.com/mozilla-b2g/gaia/commit/cb55b07bd74d2ba5148d663105735c95499c77c5 Now let's get the gecko patch landed; both of them are standalone from the big patch and could be safe no matter the big patch is qa+ or not. Hi Committer, please help to land https://bugzilla.mozilla.org/attachment.cgi?id=8604504 Thanks.

https://github.com/mozilla-b2g/gaia/pull/29932 The pull request could not be applied to the integration branch. Please try again after current integration is complete. You may need to rebase your branch against the target branch.

Autolander is not going to work well in this bug due to the slew of different patches. Please file a new bug for non-gaia patches if you need checkin-needed (you can carry reviews over).

(In reply to Kevin Grandon :kgrandon from comment #123) > Autolander is not going to work well in this bug due to the slew of > different patches. > > Please file a new bug for non-gaia patches if you need checkin-needed (you > can carry reviews over). Ya, thanks :/

Comment on attachment 8602531 [details] Address etienne's comment qa‑approval+ based on Shing's testing https://bugzilla.mozilla.org/attachment.cgi?id=8610009

https://github.com/mozilla-b2g/gaia/commit/97c41d6839e2c5fa6962aaf9b1720adf79f219ba Thanks everyone!

Sorry about this, but had to backout on b-i due to perma failures when landing. https://treeherder.mozilla.org/logviewer.html#?job_id=257738&repo=gaia-master https://github.com/mozilla-b2g/gaia/commit/26650467a184a357186b41db6c9c922a073257e0 This is a huge patch, so I'm going to try to debug this today and see if we can re-land. I'm not sure why the errors only show up on b-i and not on gaia-try, but we should be able to debug using gaia.json and normal try server.

There is a pull request with the patch rebased against latest master. I also saw these two errors in the logs, which may be related: 11:41:15 INFO - JavaScript error: app://system.gaiamobile.org/js/screen_manager.js, line 557: TypeError: window.clearIdleTimeout is not a function 11:41:28 INFO - JavaScript error: app://system.gaiamobile.org/js/lockscreen_window.js, line 141: TypeError: frame is null We should compare that error log against a clean one from master. Additionally I'm checking to see if the old "gaia on try" infrastructure still works. A run is in progress here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0da334406f4d

(In reply to Kevin Grandon :kgrandon from comment #130) > There is a pull request with the patch rebased against latest master. I also > saw these two errors in the logs, which may be related: I've been looking quite a lot at logcat noise lately to reduce the gecko one, so my 2 cents: > 11:41:15 INFO - JavaScript error: > app://system.gaiamobile.org/js/screen_manager.js, line 557: TypeError: > window.clearIdleTimeout is not a function I'm pretty sure I never saw this one. > 11:41:28 INFO - JavaScript error: > app://system.gaiamobile.org/js/lockscreen_window.js, line 141: TypeError: > frame is null Not sure about that one since I test with the lockscreen disabled.

Update: backout has hit b-i, but Gu is still crashing. The two errors in comment 130 are gone though. My previous try run (using gaia.json) broke due to bustage on b-i. This has been solved and we can re-submit. Next steps: figure out why we're crashing now, then look at re-landing with a try run using gaia.json[1]. [1] https://wiki.mozilla.org/ReleaseEngineering/TryServer#Using_a_custom_Gaia

B-i is green and open again, but I've unfortunately run out of time to look at this today. Alive - can you take a look at the errors in comment 130 and try to address them? You'll also run into a conflict with screen_manager.js probably, I think I resolved it appropriately and you might just be able to pull my copy of the file from here: https://raw.githubusercontent.com/KevinGrandon/gaia/reland_bug_1094759/apps/system/js/screen_manager.js (Also, please use gaia.json and tryserver to re-submit before landing if you can. Thanks!)

(In reply to Kevin Grandon :kgrandon from comment #128) > Sorry about this, but had to backout on b-i due to perma failures when > landing. > > https://treeherder.mozilla.org/logviewer.html#?job_id=257738&repo=gaia-master > > https://github.com/mozilla-b2g/gaia/commit/ > 26650467a184a357186b41db6c9c922a073257e0 > > This is a huge patch, so I'm going to try to debug this today and see if we > can re-land. I'm not sure why the errors only show up on b-i and not on > gaia-try, but we should be able to debug using gaia.json and normal try > server. This issue is interesting; it's about to click the preload keyboard "LOL" app in settings app permission panel, and the click() of marionette does nothing. I am sure this is intermittent. :(

I did not set gaia.json correctly, so repush: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ff05c2a722f

Update: I suspect it's a testing framework issue; the JS errors is not related. I fixed the screenManager error and the LockscreenWindow error also occurs on a green test (https://s3-us-west-2.amazonaws.com/taskcluster-public-artifacts/jq1pbBiBQmWMycAf9XyJPQ/2/public/logs/live_backing.log)

Hi Jonathan: I saw you had commented on a similar bug https://bugzilla.mozilla.org/show_bug.cgi?id=1023001 for "Automation Error: mozprocess timed out after 1760 seconds running", do you have any hint about how to debug this? Thanks.

(In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to queue) from comment #138) > I did not set gaia.json correctly, so repush: > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ff05c2a722f You need to use a http:// endpoint I believe, not git://. I think the tests should run successfully after that. (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #139) > Update: I suspect it's a testing framework issue; the JS errors is not > related. I fixed the screenManager error and the LockscreenWindow error also > occurs on a green test It could be, but I don't see these errors being logged on master?

... and again, TryChooser was not updated. The correct syntax should be gaia-unit https://dxr.mozilla.org/mozilla-central/source/testing/taskcluster/tasks/job_flags.yml Repushed again and also switch to https URL for GitHub. https://treeherder.mozilla.org/#/jobs?repo=try&revision=5d60d86d34a4

(In reply to Kevin Grandon :kgrandon from comment #141) > (In reply to Tim Guan-tin Chien [:timdream] (slow response; please ni? to > queue) from comment #138) > > I did not set gaia.json correctly, so repush: > > > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ff05c2a722f > > You need to use a http:// endpoint I believe, not git://. I think the tests > should run successfully after that. > > > (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #139) > > Update: I suspect it's a testing framework issue; the JS errors is not > > related. I fixed the screenManager error and the LockscreenWindow error also > > occurs on a green test > > It could be, but I don't see these errors being logged on master? I fixed the js errors in latest patch, but it's still broken. I am trying to run gaia-unit-test locally to see if I could reproduce it. Otherwise I may need to loan a machine.

Update: running ./bin/gaia-test -d is fine. running gaia-unit-test manually via python is also fine. I hope Jonathan had some idea, otherwise I need a loan machine...the last resort. Again, really suspect it's environment issue.

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #144) > Update: running ./bin/gaia-test -d is fine. > running gaia-unit-test manually via python is also fine. > > I hope Jonathan had some idea, otherwise I need a loan machine...the last > resort. > > Again, really suspect it's environment issue. Or, I need to know how b2g-inbound is running gaia-unit-tests. Maybe I am using the different way as it ..(but this should not happen...)

The failed Gu on try with latest patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c53d9776a901 No JS Error anymore, but no luck.

Kevin, you have more knowledge about testing than me. Any idea what could I miss? Thanks.

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #147) > Kevin, you have more knowledge about testing than me. Any idea what could I > miss? Thanks. I unfortunately don't really know what to do here. I would imagine getting a loander machine would be the next step? Have you also tried to reproduce on a linux or try-like environment? It looks like Jgriffin is already CC'd, but maybe also ask folks on the FxOS automation team?

In a successful run on trunk, I see lines like this in the log: 15:46:11 INFO - --runapp found app: Test Agent, disabling lock screen... 15:46:12 INFO - --runapp launching app: Test Agent I don't see this for this try run; I wonder if --runapp is broken?

(In reply to Jonathan Griffin (:jgriffin) from comment #149) > In a successful run on trunk, I see lines like this in the log: > > 15:46:11 INFO - --runapp found app: Test Agent, disabling lock screen... > 15:46:12 INFO - --runapp launching app: Test Agent > > > I don't see this for this try run; I wonder if --runapp is broken? I reproduced the issue on linux... I don't why this is happening on linux desktop b2g, but it seems we are running the GU with lockscreen enabled = true and turn it off at runtime; this is causing the system to be stuck at init logo. I tried to use a custom gaia profile with lockscreen.enabled=false then everything is fine. So my question is: can we build profile for GU test with lockscreen.enabled=false? If not, why?

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #150) > (In reply to Jonathan Griffin (:jgriffin) from comment #149) > > In a successful run on trunk, I see lines like this in the log: > > > > 15:46:11 INFO - --runapp found app: Test Agent, disabling lock screen... > > 15:46:12 INFO - --runapp launching app: Test Agent > > > > > > I don't see this for this try run; I wonder if --runapp is broken? > > I reproduced the issue on linux... > > I don't why this is happening on linux desktop b2g, but it seems we are > running the GU with lockscreen enabled = true and turn it off at runtime; > this is causing the system to be stuck at init logo. > > I tried to use a custom gaia profile with lockscreen.enabled=false then > everything is fine. > > So my question is: can we build profile for GU test with > lockscreen.enabled=false? If not, why? More information: we are injecting lockscreen DOM elements in build time into system app; while running GU in linux desktop b2g with the debug=1 profile, I found that all the lockscreen DOM elements are missing. It's an existing error. If you run with master you will see lockscreen_window.js error, too. The root cause might be a build issue only happened on linux, but I tend to workaround it here...

My workaround: ignore AppWindow instantiation of LockscreenWindow if there is no lockscreen-frame element injected in the system app DOM; beyond that, if system thinks we should launch lockscreen(lockscreen.enabled=true) but lockscreenLauncher failed to create the lockscreen window, launcher will ignore the passcode broadcasting procedure but asks the cover screen to hide right away. It worked in my ubuntu, now I am pushing it to try. https://treeherder.mozilla.org/#/jobs?repo=try&revision=09393fb8e5d3 And here is another pull request rebased to latest master https://github.com/mozilla-b2g/gaia/pull/30383 Crossing finger...

> I reproduced the issue on linux... > > I don't why this is happening on linux desktop b2g, but it seems we are > running the GU with lockscreen enabled = true and turn it off at runtime; > this is causing the system to be stuck at init logo. > > I tried to use a custom gaia profile with lockscreen.enabled=false then > everything is fine. > > So my question is: can we build profile for GU test with > lockscreen.enabled=false? If not, why? Is there an env var we can use to disable the lockscreen? If so, then yes, we could do that. I think what you're suggesting though, is that we monkey-patch the profile after we create it, to set lockscreen.enabled=false. We could do this too, but it's inherently more fragile. The current code for profile generation is here: http://hg.mozilla.org/build/mozharness/file/3e36eedc67a2/mozharness/mozilla/gaia.py#l314

I tried https://github.com/mozilla-b2g/gaia/pull/30383 and the status bar shows two battery icons and the time is at the wrong side. Any ideas about this?

The boot time improvement is incredible. Nice work! Hermes, can you help out with some exploratory testing of systems-fe features with this patch? I guess alive or tim can point you to the right builds.

(In reply to Gregor Wagner [:gwagner] from comment #154) > I tried https://github.com/mozilla-b2g/gaia/pull/30383 and the status bar > shows two battery icons and the time is at the wrong side. Any ideas about > this? I am checking, should come from the recent rebase of statusbar patches. Thanks.

Rebase regression fixed; https://treeherder.mozilla.org/#/jobs?repo=try&revision=89f2195b8dd9 is proving the workaround of GU works.

gaia try and b-i are both green https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d3461d16ced Shall we wait for the test you mentioned in last comment before landing? It's not communicated before...

Shing has performed some tests for that patch and he also said there is no obvious function broken. I think it is ok to land the patch. After landing, I will perform exploratory testing of systems-fe to make sure there is no regression. (leaving NI for following testing)

Going to merge per comment 159.

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #158) > gaia try and b-i are both green > https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d3461d16ced > > Shall we wait for the test you mentioned in last comment before landing? > It's not communicated before... Don't wait if you think its ready and there are no obvious regressions.

Hi Alive, just saw this patch landed. The on-device python marionette test began failing to start b2g (see Bug 1170175) and I think it is related to this patch. Could you take a look, and if possible back out the patch or make a quick fix? It will affect all of the python device tests. cc'ing Dave on the bug as well.

(In reply to No-Jun Park [:njpark] from comment #163) > Hi Alive, just saw this patch landed. The on-device python marionette test > began failing to start b2g (see Bug 1170175) and I think it is related to > this patch. Could you take a look, and if possible back out the patch or > make a quick fix? It will affect all of the python device tests. > > cc'ing Dave on the bug as well. This should not happen if python marionette tests running on desktop passes.... Could you tell me how do you run the tests? Have you updated gecko?

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #164) > (In reply to No-Jun Park [:njpark] from comment #163) > > Hi Alive, just saw this patch landed. The on-device python marionette test > > began failing to start b2g (see Bug 1170175) and I think it is related to > > this patch. Could you take a look, and if possible back out the patch or > > make a quick fix? It will affect all of the python device tests. > > > > cc'ing Dave on the bug as well. > > This should not happen if python marionette tests running on desktop > passes.... > > Could you tell me how do you run the tests? > Have you updated gecko? Please give me the logcat if possible, also, what's the screen you are seeing? Have you reinstall gaia/tests/python/gaia-ui-tests? Shing, is this the test you had ever run? How did you make it work?

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #165) > (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #164) > > (In reply to No-Jun Park [:njpark] from comment #163) > > > Hi Alive, just saw this patch landed. The on-device python marionette test > > > began failing to start b2g (see Bug 1170175) and I think it is related to > > > this patch. Could you take a look, and if possible back out the patch or > > > make a quick fix? It will affect all of the python device tests. > > > > > > cc'ing Dave on the bug as well. > > > > This should not happen if python marionette tests running on desktop > > passes.... > > > > Could you tell me how do you run the tests? > > Have you updated gecko? > > Please give me the logcat if possible, also, what's the screen you are > seeing? Have you reinstall gaia/tests/python/gaia-ui-tests? > > Shing, is this the test you had ever run? How did you make it work? (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #164) > (In reply to No-Jun Park [:njpark] from comment #163) > > Hi Alive, just saw this patch landed. The on-device python marionette test > > began failing to start b2g (see Bug 1170175) and I think it is related to > > this patch. Could you take a look, and if possible back out the patch or > > make a quick fix? It will affect all of the python device tests. > > > > cc'ing Dave on the bug as well. > > This should not happen if python marionette tests running on desktop > passes.... > > Could you tell me how do you run the tests? > Have you updated gecko? Basically, Jenkins flashes the full image of nightly, and runs the python marionette test. Will provide you with the logcat shortly.

Logcat is attached to Bug 1170175. It is captured during repeated test failures. This is consistently being reproed in Jenkins, and I'll try locally to see whether I can reproduce as well. https://bugzilla.mozilla.org/attachment.cgi?id=8615352

I am seeing Javascript Error in statusbar.js, but it should be fixed by https://github.com/mozilla-b2g/gaia/commit/fb8901e7f1ff74b28e5a5eb3b8adf9a3448414fc3#diff-293e71c28c7a1b418148d8af71d58804 Could you try again? Maybe this fixes it...or not. Also, if you could reproduce locally, are you able to build a debug build? I need gaia.debug.enabled=true settings, thanks.

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #168) > I am seeing Javascript Error in statusbar.js, but it should be fixed by > https://github.com/mozilla-b2g/gaia/commit/ > fb8901e7f1ff74b28e5a5eb3b8adf9a3448414fc3#diff- > 293e71c28c7a1b418148d8af71d58804 That gives Page not Found for me.

I tried this locally, and I could not reproduce it. Moreover, now some of the jenkins job is passing with the same nightly. Perhaps the failures earlier might not be related, and the timing was coincidence. Either that, or the bug is shown intermittently. (hopefully the logcat might give some clues?) Will keep an eye on it, but looks like no immediate action is needed for now. Will keep you updated. And at the moment with the debug build, we can't run marionette as it will cause timeout with any find_element calls when running on device.

(In reply to No-Jun Park [:njpark] from comment #170) > I tried this locally, and I could not reproduce it. Moreover, now some of > the jenkins job is passing with the same nightly. Perhaps the failures > earlier might not be related, and the timing was coincidence. Either that, > or the bug is shown intermittently. (hopefully the logcat might give some > clues?) Will keep an eye on it, but looks like no immediate action is needed > for now. Will keep you updated. > > And at the moment with the debug build, we can't run marionette as it will > cause timeout with any find_element calls when running on device. Good news. If you see something not urgent but strange please file a bug and NI me. Thanks!

(In reply to No-Jun Park [:njpark] from comment #170) > I tried this locally, and I could not reproduce it. Moreover, now some of I can still reproduce this locally, even after a full flash. I haven't seen ready-state=fullyLoaded getting added to the system app body element, at all. Unfortunately, I couldn't check with webIDE, because connecting to the Flame device seems to be broken too in the latest build.

(In reply to Martijn Wargers [:mwargers] (QA) from comment #172) > (In reply to No-Jun Park [:njpark] from comment #170) > > I tried this locally, and I could not reproduce it. Moreover, now some of > > I can still reproduce this locally, even after a full flash. I haven't seen > ready-state=fullyLoaded getting added to the system app body element, at > all. Unfortunately, I couldn't check with webIDE, because connecting to the > Flame device seems to be broken too in the latest build. Have you seen any other errors? Have you seen document.body has 'ready-state' attribute? What's the screen you are seeing?

It's midnight here, so I am un-watching the issue until tomorrow morning. If there is a really serious issue, you could consider to backout. But please leave me the right STR. I really can't help if someone just says "you are breaking something!" but the same thing on CI is fine... I'd appreciate more solid test step.

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #173) > Have you seen any other errors? Have you seen document.body has > 'ready-state' attribute? What's the screen you are seeing? No ready-state attribute for document.body at all. I'm seeing the homescreen.

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #174) > It's midnight here, so I am un-watching the issue until tomorrow morning. If > there is a really serious issue, you could consider to backout. But please > leave me the right STR. I really can't help if someone just says "you are > breaking something!" but the same thing on CI is fine... I'd appreciate more > solid test step. Do you have a Flame device to test with? You need to have to carry out the steps at: https://developer.mozilla.org/en-US/Firefox_OS/Automated_testing/gaia-ui-tests/Gaia_UI_Tests_Run_Tests#Testing_on_Firefox_OS_devices

When I reviewed screenshots for these, I noticed some shady looking stuff on the launcher. There are unfinished icons on each of these areas. Turned out to be https://bugzilla.mozilla.org/show_bug.cgi?id=1157851 -- Gaiatest deleting stuff between runs affects smart collections. That was chalked up to one of the deletions here: https://github.com/mozilla-b2g/gaia/blob/7cfe9eb9ade41043dea02d350680735b815d9c5a/tests/python/gaia-ui-tests/gaiatest/gaia_test.py#L910 There are some error messages surrounding this here: https://bugzilla.mozilla.org/show_bug.cgi?id=1157851#c3 I'll upload the screenshot as an attachment so you can see the icons I mean. Does any of this look like it would have caused some component to block the fullyloaded event per step 8 of comment 0? Maybe something threw an exception and caused an implicit promise rejection?

Suspicious looking screenshot per comment 177

(In reply to Geo Mealer [:geo] from comment #177) > When I reviewed screenshots for these, I noticed some shady looking stuff on > the launcher. There are unfinished icons on each of these areas. > > Turned out to be https://bugzilla.mozilla.org/show_bug.cgi?id=1157851 -- > Gaiatest deleting stuff between runs affects smart collections. > > That was chalked up to one of the deletions here: > > https://github.com/mozilla-b2g/gaia/blob/ > 7cfe9eb9ade41043dea02d350680735b815d9c5a/tests/python/gaia-ui-tests/gaiatest/ > gaia_test.py#L910 > > There are some error messages surrounding this here: > > https://bugzilla.mozilla.org/show_bug.cgi?id=1157851#c3 > > I'll upload the screenshot as an attachment so you can see the icons I mean. > > Does any of this look like it would have caused some component to block the > fullyloaded event per step 8 of comment 0? Maybe something threw an > exception and caused an implicit promise rejection? Yes, the only possible reason I could figure out; maybe this will only happen on certain real device. I am setting up the test right now.

(In reply to Geo Mealer [:geo] from comment #177) > When I reviewed screenshots for these, I noticed some shady looking stuff on > the launcher. There are unfinished icons on each of these areas. > > Turned out to be https://bugzilla.mozilla.org/show_bug.cgi?id=1157851 -- > Gaiatest deleting stuff between runs affects smart collections. > > That was chalked up to one of the deletions here: > > https://github.com/mozilla-b2g/gaia/blob/ > 7cfe9eb9ade41043dea02d350680735b815d9c5a/tests/python/gaia-ui-tests/gaiatest/ > gaia_test.py#L910 > > There are some error messages surrounding this here: > > https://bugzilla.mozilla.org/show_bug.cgi?id=1157851#c3 > I really don't think this is related. I have no what will happen idea deleting these stuff during device is running..

Talked to Martijn on IRC, the errors in bug 1170175 is a false alarm. The root cause is the latest gaia-ui-test lib testing on a little old gaia build. According to what he said, bug 1157851 is also fixed by using the latest pvt build, too.

This is causing a smote test blocker bug 1172027. We will need this landing backed out.

This is too big to be backed out by someone else. Lets wait for Alive to get online and fix it or do the backout himself.

I thought we had a successful smoketest run here? What went wrong?

I am fixing it right now, stay tuned.

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #185) > I am fixing it right now, stay tuned. Fixed, waiting for Tim review(he is online) + try server run.

(In reply to Gregor Wagner [:gwagner] from comment #155) > The boot time improvement is incredible. Nice work! > > Hermes, can you help out with some exploratory testing of systems-fe > features with this patch? I guess alive or tim can point you to the right > builds. I have performed some tests against Homescreen/Rocektbar/Download Manager/Browser/Task Manager. I have not found any regressions for now.