(inactive after 6/18) Alive Kuo [:alive] Assignee
	Description • 11 years ago

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

And fine tune the transition state would be the followup.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 1 • 11 years ago

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

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 2 • 11 years ago

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

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 3 • 11 years ago

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.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 4 • 11 years ago

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

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 5 • 11 years ago

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

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 6 • 11 years ago

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

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 7 • 11 years ago

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

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 8 • 11 years ago

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.

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 9 • 11 years ago

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.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 10 • 11 years ago

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

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 11 • 11 years ago

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

	Dave Hunt [:davehunt] [he/him] ⌚BST
	Comment 12 • 11 years ago

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

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 13 • 11 years ago

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

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 14 • 11 years ago

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.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 15 • 11 years ago

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.

	Zac C (:zac)
	Comment 16 • 11 years ago

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.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 17 • 11 years ago

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

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 18 • 11 years ago

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

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 19 • 11 years ago

(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

	Etienne Segonzac (:etienne)
	Comment 20 • 11 years ago

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

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

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 21 • 11 years ago

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)

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 22 • 11 years ago

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

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

	Zac C (:zac)
	Comment 23 • 11 years ago

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

	Etienne Segonzac (:etienne)
	Comment 24 • 11 years ago

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

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 25 • 11 years ago

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

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 26 • 11 years ago

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.

	Etienne Segonzac (:etienne)
	Comment 27 • 11 years ago

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

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 28 • 11 years ago

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

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 29 • 11 years ago

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

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 30 • 11 years ago

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

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 31 • 11 years ago

Updated to latest patch.

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 32 • 11 years ago

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

	Etienne Segonzac (:etienne)
	Comment 33 • 11 years ago

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

	Etienne Segonzac (:etienne)
	Comment 34 • 11 years ago

(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

	Zac C (:zac)
	Comment 35 • 11 years ago

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

	Etienne Segonzac (:etienne)
	Comment 36 • 11 years ago

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!

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 37 • 11 years ago

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

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 38 • 11 years ago

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

	(inactive after 6/18) Alive Kuo [:alive] Assignee
	Comment 39 • 11 years ago

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

	gasolin@mozilla.com
	Comment 40 • 11 years ago

\O/