Closed Bug 907013 (app-window-manager) Opened 11 years ago Closed 11 years ago

[Window Management] Implement open and close transition logic in AppWindow

Categories

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: alive, Assigned: alive)

References

Details

Attachments

(3 files)

The acceptable criteria: * Implement AppWindow.prototype.open * Implement AppWindow.prototype.close And fine tune the transition state would be the followup.
The patch is now too big and still buggy. Though completeness and readability would be lower I am still going to split the patch to different bugs marked as blocking this one.
Bug 908601 touches this part as well. Blocked by that.
Depends on: 908601
Depends on: 927310
No longer depends on: 927310
Component: Gaia::System → Gaia::System::Window Mgmt
No longer blocks: task-manager
WIP I am writing as most tests as I could right now.
Comment on attachment 830151 [details] [review] https://github.com/mozilla-b2g/gaia/pull/13533 Seems lots of tests to write. Let's get some feedback first :) P.S. We could just deprecate WindowManager if bug 933590 is fixed. I will update all the testing work after this feedback in the second commit.
Attachment #830151 - Flags: feedback?(timdream)
Alias: app-window-manager
Depends on: 938007
Comment on attachment 830151 [details] [review] https://github.com/mozilla-b2g/gaia/pull/13533 Sorry for taking too long. This gigantic patch looks alright apparently, but it would be better if we could break it down.
Attachment #830151 - Flags: feedback?(timdream) → feedback+
Newly rebased patch: https://github.com/mozilla-b2g/gaia/pull/13797 Fix lots of regression I've found and add some tests.
Blocks: 933590
Depends on: 937456
Dave, I have difficulty finding out where I broke uitest...I think it's due to some API change. Could you help to diagnose? Patch is here: https://github.com/mozilla-b2g/gaia/pull/13797
Flags: needinfo?(dave.hunt)
(In reply to Dave Hunt (:davehunt) from comment #12) > It would appear that app.frame.firstChild is now undefined. See > https://github.com/mozilla-b2g/gaia/blob/ > 9470d15828db48a5558c63a3d9981df9e72eb624/tests/python/gaia-ui-tests/gaiatest/ > atoms/gaia_apps.js#L192 Hmm, that's because iframe isn't firstChild anymore. I think we could try to change it to app.iframe or app.browser.element in my patch...
Depends on: 942660
https://travis-ci.org/mozilla-b2g/gaia/jobs/14469281 All remaining broken issues now: 3 Integrations tests is because for some reason marionette cannot locate modal dialog + tap ok button correctly. I'd fixed the locating issue by https://github.com/mozilla-b2g/marionette-helper/pull/11/files but I don't know why tap doesn't work. But clicking on ok button does work on real device. 2 Python tests fail: test_add_contact_to_favorites.py test_add_contact_to_favorites.TestAddContactToFavorite.test_add_contact_to_favorite test_sms_add_contact.py test_sms_add_contact.TestSmsAddContact.test_sms_add_contact I'm sure these two tests also fail on master but I don't know why other pull requests are passed.
Blocks: 942660
No longer depends on: 942660
Blocks: 942955
Blocks: 943188
Attached file AppWindowManager v3
Let the party begin! * Disable 3 marionette js tests because marionette seems having trouble on finding out alerts. Will file follow up bugs to re-enable them. * Remove WindowManager and introduce AppWindowManager * Introduce LayoutManager who knows and gethers the sizing info well. * Fix OrientationManager issues. * Abandon TransitionMixin and "Re"write an AppTransitionController for the use case of edge gesture -- let appWindow support multiple transitions. * Abandon AuthenticationDialog and "Re"implement AppAuthenticationDialog * AppWindow: Introduce subcomponents under appWindow * AppWindow: Introduce broadcast for internal events, publish for external events. * AppWindow: Introduce event handling mechanism by registeredEvents. * Fix VisibilityManager issues. * Change the mechanism in python based gaia-apps. * More shared code between AppWindow and HomescreenWindow. * More shared code between AppWindow and ActivityWindow. * Add 100+ unit test functions. * Introduce window.css * Add a new event in KeyboardManager for continuos transition control.
Attachment #8338386 - Flags: review?(zcampbell)
Attachment #8338386 - Flags: review?(timdream)
Attachment #8338386 - Flags: review?(jlal)
Attachment #8338386 - Flags: review?(gchen)
Attachment #8338386 - Flags: review?(etienne)
Alive, I have pushed this onto a Hamachi device using make-reset gaia. However when I (or rather, the automated test suite) restarts b2g I get the following error back from Marionette. I've seen this type of error lots of times before on the startup. It will be listed in the logcat. JavascriptException: NS_ERROR_UNEXPECTED: stacktrace: @app://system.gaiamobile.org, line 48 To be sure it is related to this pull I compared it to the equivalent master head (I have two Hamachis) and it did not exhibit the issue. desktopb2g would not catch this issue.
Depends on: 923692
(In reply to Zac C (:zac) from comment #16) > Alive, I have pushed this onto a Hamachi device using make-reset gaia. > > However when I (or rather, the automated test suite) restarts b2g I get the > following error back from Marionette. I've seen this type of error lots of > times before on the startup. It will be listed in the logcat. > > JavascriptException: NS_ERROR_UNEXPECTED: > stacktrace: @app://system.gaiamobile.org, line 48 > > To be sure it is related to this pull I compared it to the equivalent master > head (I have two Hamachis) and it did not exhibit the issue. desktopb2g > would not catch this issue. I really don't think I have something to do with that https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/storage.js#L48 But I will double check. Does this break anything in testing framework?
(In reply to Alive Kuo [:alive][NEEDINFO] from comment #17) > (In reply to Zac C (:zac) from comment #16) > > Alive, I have pushed this onto a Hamachi device using make-reset gaia. > > > > However when I (or rather, the automated test suite) restarts b2g I get the > > following error back from Marionette. I've seen this type of error lots of > > times before on the startup. It will be listed in the logcat. > > > > JavascriptException: NS_ERROR_UNEXPECTED: > > stacktrace: @app://system.gaiamobile.org, line 48 > > > > To be sure it is related to this pull I compared it to the equivalent master > > head (I have two Hamachis) and it did not exhibit the issue. desktopb2g > > would not catch this issue. > > I really don't think I have something to do with that > https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/storage.js#L48 > > But I will double check. Does this break anything in testing framework? I'd made a patch to fix the ERROR but this is really Out Of Scope of this bug....
(In reply to Alive Kuo [:alive][NEEDINFO] from comment #18) > (In reply to Alive Kuo [:alive][NEEDINFO] from comment #17) > > (In reply to Zac C (:zac) from comment #16) > > > Alive, I have pushed this onto a Hamachi device using make-reset gaia. > > > > > > However when I (or rather, the automated test suite) restarts b2g I get the > > > following error back from Marionette. I've seen this type of error lots of > > > times before on the startup. It will be listed in the logcat. > > > > > > JavascriptException: NS_ERROR_UNEXPECTED: > > > stacktrace: @app://system.gaiamobile.org, line 48 > > > > > > To be sure it is related to this pull I compared it to the equivalent master > > > head (I have two Hamachis) and it did not exhibit the issue. desktopb2g > > > would not catch this issue. > > > > I really don't think I have something to do with that > > https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/storage.js#L48 > > > > But I will double check. Does this break anything in testing framework? > > I'd made a patch to fix the ERROR but this is really Out Of Scope of this > bug.... Maybe I know what happens. I change the timing of unlock event and maybe settings DB is not ready when first unlock event caused by FTU is fired. That sounds a gecko issue. But I still modify the bad pattern in storage.js
Blocks: 943717
Blocks: 943720
Comment on attachment 8338386 [details] [review] AppWindowManager v3 This is impressive! First round of comments on github, I need a nap :)
Attachment #8338386 - Flags: review?(etienne)
Comment on attachment 8338386 [details] [review] AppWindowManager v3 Looks good and I don't think I need to review this :) (unless there is a specific part you want me to look into)
Attachment #8338386 - Flags: review?(timdream) → feedback+
Comment on attachment 8338386 [details] [review] AppWindowManager v3 Second round! * More tests * Add option to enable continuous transition * Still not use cloneNode.
Attachment #8338386 - Flags: review?(zcampbell)
Attachment #8338386 - Flags: review?(jlal)
Attachment #8338386 - Flags: review?(gchen)
Attachment #8338386 - Flags: review?(etienne)
I ran the device UI test suite (gaia-ui-tests) on this today and I can give it a clean bill of health :)
Blocks: 945092
Comment on attachment 8338386 [details] [review] AppWindowManager v3 I'm done for this round :) Since we're probably going to wait for next week before landing this, I'd like to indulge in a last review round on the next version of the patch. It's a pretty big patch to digest :)
Attachment #8338386 - Flags: review?(etienne)
(In reply to Etienne Segonzac (:etienne) from comment #24) > Comment on attachment 8338386 [details] [review] > AppWindowManager v3 > > I'm done for this round :) > > Since we're probably going to wait for next week before landing this, I'd > like to indulge in a last review round on the next version of the patch. > > It's a pretty big patch to digest :) Thanks for taking your time! I'll send review again today or tomorrow.
Comment on attachment 8338386 [details] [review] AppWindowManager v3 Let's start the 3rd run...! In this round I tried to utilize jsdoc and produced: http://alivedise.github.io/gaia-system-jsdoc/wip/ It's not perfect but no begin no reach.
Attachment #8338386 - Flags: review?(etienne)
Depends on: 938312
Comment on attachment 8338386 [details] [review] AppWindowManager v3 Sadly we still have some blocking issues :/ Comments are on github, repeating the key points here: * The missing (launchapp?) event when opening a wrapper * The LockscreenTest causing a unit test failure * The NaN warm launch time debug * The |killed| typo in AppWindow * The slow-transition breakage * The need for a nicer commit message :) Hope it's not too frustrating to go through all those review rounds, I'm confident the next one will be the good one ! (I know I already said that before ;))
Attachment #8338386 - Flags: review?(etienne)
(In reply to Etienne Segonzac (:etienne) from comment #27) > Comment on attachment 8338386 [details] [review] > AppWindowManager v3 > > Sadly we still have some blocking issues :/ > > Comments are on github, repeating the key points here: > > * The missing (launchapp?) event when opening a wrapper > * The LockscreenTest causing a unit test failure > * The NaN warm launch time debug > * The |killed| typo in AppWindow > * The slow-transition breakage > * The need for a nicer commit message :) > > Hope it's not too frustrating to go through all those review rounds, I'm > confident the next one will be the good one ! (I know I already said that > before ;)) Thanks again! I won't be frustrated because I already know this won't be the final run. Help on DSDS Feature Complete now, will be back soon!
Note: Current plan is land this after v1.3 is branched. Three days away!
Depends on: 943236
Comment on attachment 8338386 [details] [review] AppWindowManager v3 https://travis-ci.org/mozilla-b2g/gaia/builds/15202613 The first green I'd seen in this patch!!!!
Attachment #8338386 - Flags: review?(etienne)
Updated to latest patch.
I'd rebased but I don't know why the merge button is disabled :/
(In reply to Alive Kuo [:alive][NEEDINFO] from comment #32) > I'd rebased but I don't know why the merge button is disabled :/ Just landed bug 943236 this morning...
(In reply to Etienne Segonzac (:etienne) from comment #33) > (In reply to Alive Kuo [:alive][NEEDINFO] from comment #32) > > I'd rebased but I don't know why the merge button is disabled :/ > > Just landed bug 943236 this morning... nope looks like somehing related to python tests
Oh I know what it is, Dave just merged a patch that merges the atoms that js and Python tests use: https://bugzilla.mozilla.org/show_bug.cgi?id=909839
Comment on attachment 8338386 [details] [review] AppWindowManager v3 Still a few comments and a few nits, but those are simple enough to address that I fill confident r+ing this! We still need to wait for 1.3 to branch and for a green travis but still: \\o \o\ \o/ /o/ o// It has been a honor reviewing this, and the documentation is looking goooood!
Attachment #8338386 - Flags: review?(etienne) → review+
Blocks: 948771
Conflict before merging and now green again https://travis-ci.org/mozilla-b2g/gaia/builds/15264032
https://github.com/mozilla-b2g/gaia/commit/a6c2af8d32274113e81d2c57e23d0621af118e53 Just one year ago, in the snowy city, Berlin, I orally described this idea to Vivien and initialed this project with the first file (window.js) - Window management system rework. What I didn't knew then was, this work took me the whole year to come to an end. During this year, I failed and restarted the work many many many times. Countless conflicts, regressions, testing failures, old bugs, invisible traps and tears :) Finally this day comes: suddenly, everything is set up. People come to help and/or support me for testing, documentation, reviewing. The future OS wide improvement plan is there awaiting the new system to exploit its full power. Thanks everyone. (Waiting for regressions and prepare to step next...)
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Blocks: 948792
Blocks: 948866
Blocks: 948875
\O/
Blocks: 949356
Blocks: 949459
No longer blocks: 949459
See Also: → 949459
Blocks: 949912
Blocks: 949931
Blocks: 949527
Blocks: 949487
Blocks: 949209
Depends on: 950827
Blocks: 868324
Blocks: 952008
Depends on: 972475
Depends on: 950673
See Also: → 991262
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: