907013 - (app-window-manager) [Window Management] Implement open and close transition logic in AppWindow

Status: RESOLVED FIXED

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

Description

[(inactive after 6/18) Alive Kuo [:alive]]

The acceptable criteria:
* Implement AppWindow.prototype.open
* Implement AppWindow.prototype.close

And fine tune the transition state would be the followup.

Comment 1

[(inactive after 6/18) Alive Kuo [:alive]]

WIP
https://github.com/mozilla-b2g/gaia/pull/12358

Comment 2

[(inactive after 6/18) Alive Kuo [:alive]]

WIP 0927
https://github.com/alivedise/gaia/compare/bugzilla;907013_master_v4;transition-controller?expand=1

Comment 3

[(inactive after 6/18) Alive Kuo [:alive]]

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.

Comment 4

[(inactive after 6/18) Alive Kuo [:alive]]

Bug 908601 touches this part as well. Blocked by that.

Comment 5

[(inactive after 6/18) Alive Kuo [:alive]]

new WIP
https://github.com/alivedise/gaia/compare/bugzilla;907013_master_new;v2?expand=1

Comment 6

[(inactive after 6/18) Alive Kuo [:alive]]

lastest WIP
https://github.com/mozilla-b2g/gaia/pull/13533

Comment 7

[(inactive after 6/18) Alive Kuo [:alive]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/13533

WIP
I am writing as most tests as I could right now.

Comment 8

[(inactive after 6/18) Alive Kuo [:alive]]

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.

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 10

[(inactive after 6/18) Alive Kuo [:alive]]

Newly rebased patch:
https://github.com/mozilla-b2g/gaia/pull/13797
Fix lots of regression I've found and add some tests.

Comment 11

[(inactive after 6/18) Alive Kuo [:alive]]

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

Comment 12

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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

Comment 13

[(inactive after 6/18) Alive Kuo [:alive]]

(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...

Comment 14

[(inactive after 6/18) Alive Kuo [:alive]]

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.

Comment 15

[(inactive after 6/18) Alive Kuo [:alive]]

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

Comment 16

[Zac C (:zac)]

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.

Comment 17

[(inactive after 6/18) Alive Kuo [:alive]]

(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?

Comment 18

[(inactive after 6/18) Alive Kuo [:alive]]

(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....

Comment 19

[(inactive after 6/18) Alive Kuo [:alive]]

(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

Comment 20

[Etienne Segonzac (:etienne)]

Comment on attachment 8338386 [details] [review]
AppWindowManager v3

This is impressive!
First round of comments on github, I need a nap :)

Comment 21

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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)

Comment 22

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8338386 [details] [review]
AppWindowManager v3

Second round!
* More tests
* Add option to enable continuous transition
* Still not use cloneNode.

Comment 23

[Zac C (:zac)]

I ran the device UI test suite (gaia-ui-tests) on this today and I can give it a clean bill of health :)

Comment 24

[Etienne Segonzac (:etienne)]

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

Comment 25

[(inactive after 6/18) Alive Kuo [:alive]]

(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 26

[(inactive after 6/18) Alive Kuo [:alive]]

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.

Comment 27

[Etienne Segonzac (:etienne)]

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

Comment 28

[(inactive after 6/18) Alive Kuo [:alive]]

(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!

Comment 29

[(inactive after 6/18) Alive Kuo [:alive]]

Note: Current plan is land this after v1.3 is branched. Three days away!

Comment 30

[(inactive after 6/18) Alive Kuo [:alive]]

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!!!!

Comment 31

[(inactive after 6/18) Alive Kuo [:alive]]

Attachment: Updated appWindow documentation

Updated to latest patch.

Comment 32

[(inactive after 6/18) Alive Kuo [:alive]]

I'd rebased but I don't know why the merge button is disabled :/

Comment 33

[Etienne Segonzac (:etienne)]

(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...

Comment 34

[Etienne Segonzac (:etienne)]

(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

Comment 35

[Zac C (:zac)]

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 36

[Etienne Segonzac (:etienne)]

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!

Comment 37

[(inactive after 6/18) Alive Kuo [:alive]]

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

Comment 38

[(inactive after 6/18) Alive Kuo [:alive]]

Conflict before merging and now green again
https://travis-ci.org/mozilla-b2g/gaia/builds/15264032

Comment 39

[(inactive after 6/18) Alive Kuo [:alive]]

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

Comment 40

[[e-mail]]

\O/