916709 - (popup-window) [Window Management] Implement PopupWindow

Status: RESOLVED FIXED

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

Description

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

Need Rework PopupManager.
This blocks v1.3 Browser app to System integration.

Comment 1

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

on it.

Comment 2

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

WIP
https://github.com/alivedise/gaia/compare/mozilla-b2g:bubble-tea...bugzilla%2F916709_bubble-tea%2Fpopup-window?expand=1

Comment 3

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

Attachment: PopupWindow WIP v1

This is WIP patch for PopupWindow:
* Remove PopupManager
* Add PopupWindow
* Layout strategy changed: layout any PopupWindow instance inside it's parent Window. We will do the same thing for ActivityWindow later.
* Enable PopupWindow to have modal dialog/authentication dialog/context menu...
* Enable nested PopupWindow (We could turn this off if we dislike.)

Next to do:
* Use childWindow/parentWindow to represent ActivityWindow, too.(another bug might be.)
* More manual testing.
* Add Unit tests.


Note:
Sorry I haven't finished the testing part but I don't want to frustrate etienne by the length of the patch every time so let's get some feedback at first.

Comment 4

[Etienne Segonzac (:etienne)]

Is it me or is the patch missing |popup_window.js| ? :)

Comment 5

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

Attachment: v2: Added popupwindow.js

I messed up my branch so here's another one.

Comment 6

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

Proposed two-dimension window binary tree architecture
https://docs.google.com/drawings/d/110XVKUOY4IiGons7FDcQ8rarIOmScoIgtxhZcU88VLk/edit?usp=sharing

Comment 7

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

Note: comment 6 is not implemented in v2 yet.
I expected to use a new term for vertical window (including popup and inline activity) and a term for horizontal window (including current child window and window activity).

Comment 8

[Etienne Segonzac (:etienne)]

Comment on attachment 8384494 [details] [review]
v2: Added popupwindow.js

preemptive feedback+ on the parent/child, caller/callee, top/bottom version :)

Comment 9

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

Progress today:
Rename parentWindow/childWindow to previousWindow/nextWindow.
Use rearWindow/frontWindow pair to represent popup.
Enhancing sheet apps to have popup/activity ability to do more tests.

Comment 10

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

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

PopupWindow v3
* Implement PopupWindow
* Remove ActivityWindowFactory and let AppWindowManager to do the launch work
* Add ActivityWindowManager
* Remove some duplicated killing app logic
* Enhance ChildWindowFactory

Note this is based on bubble-tea branch because bug 961800 has not been landed to master (running tests) and I will make a master patch later when it's landed.

I know this won't be last round but lots of things has been changed so I ask for review at first. Sorry for taking so long time because I was stuck in blockers all the day.

Comment 11

[Etienne Segonzac (:etienne)]

Comment on attachment 8394040 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/17284

Made some comments on github.
Wish I spent more time on the patch but I played with the tests apps too much :)

On PTO next week but I'm sure Vivien will like the multidimensional window manager too.

Comment 12

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

Working in progress v4 based on master's 961800 is here:
https://github.com/mozilla-b2g/gaia/pull/17567

Will request a new review round tomorrow.

Comment 13

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

Attachment: PopupWindow v5

PopupWindow v4
* More tests.
* Change orientation logic.

Comment 14

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

Comment on attachment 8397028 [details] [review]
PopupWindow v5

Transfer to vivien.

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8397028 [details] [review]
PopupWindow v5

While I really would like to be able to review it, I already know that I will be hard for me to find the necessary time to review this patch next week. I will likely be focused on the l10n refactor, vertical homescreen, widgets, rild.js, webRTC, and Web Components things :/

As Etienne as already started to look at it, he will likely be much faster than myself trying to review it.

For what it worth your windows schema makes total sense to me.

Do you consider the keyboard to be part of the vertical window in the future too ?

Does the horizontal window term will represent the app boundary that will be used by the edge gesture code to determine which types of transitions to apply ?

I guess the visibility delegated to top most window means also that the screenshot will be the top most window only ?

It scares me a little bit to delegate the orientation to the top most window only. For example if a landscape game opens an activity to a portrait only inline activity, the game may react and resize incorrectly, resulting in visual glitches when you close the activity.

Comment 17

[Etienne Segonzac (:etienne)]

Comment on attachment 8397028 [details] [review]
PopupWindow v5

Comments on github (not much) and needs a post bubble-tea rebase.

I think this can/should land this week :)

Comment 18

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

(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #16)
> Comment on attachment 8397028 [details] [review]
> PopupWindow v4
> 
> While I really would like to be able to review it, I already know that I
> will be hard for me to find the necessary time to review this patch next
> week. I will likely be focused on the l10n refactor, vertical homescreen,
> widgets, rild.js, webRTC, and Web Components things :/
> 
> As Etienne as already started to look at it, he will likely be much faster
> than myself trying to review it.
> 
> For what it worth your windows schema makes total sense to me.
> 
> Do you consider the keyboard to be part of the vertical window in the future
> too ?

Hmmm...interesting proposal, KeyboardWindow should be somewhat different because it doesn't cover the caller but co-exist. So I think keyboard is part of the baseWindow(bottom most window).

> 
> Does the horizontal window term will represent the app boundary that will be
> used by the edge gesture code to determine which types of transitions to
> apply ?

About in-app / app-to-app transition: We could determine by relationship of currentWindow & nextWindow/previousWindow.
> 
> I guess the visibility delegated to top most window means also that the
> screenshot will be the top most window only ?

Yes, only take screenshot of topMostWindow in taskManager. But for edge gesture all window should have its screenshot layer.

> 
> It scares me a little bit to delegate the orientation to the top most window
> only. For example if a landscape game opens an activity to a portrait only
> inline activity, the game may react and resize incorrectly, resulting in
> visual glitches when you close the activity.

Not all. Because resize is also delegated to top most window so the caller won't be resized.
But yes, we have bad orientation control right now because orientation is global but we will see 2 apps during transition and if they have different orientation we will have bad look.....I am still thinking how to overcome it.

Comment 19

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

Comment on attachment 8397028 [details] [review]
PopupWindow v5

Rebased v5.

Comment 20

[Etienne Segonzac (:etienne)]

Comment on attachment 8397028 [details] [review]
PopupWindow v5

Bad news: made a few comments on github that are behavior related so I can't r+ before we fix them.

Good news: I'm getting comfortable around this patch so hopefully we won't have to go to double digit version number ;)

Comment 21

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

Comment on attachment 8397028 [details] [review]
PopupWindow v5

v6, took a long time cause of other blockers.
* Rebased.
* Fix comments in github as most as possible.
* Fix broken integration test.

Comment 22

[Etienne Segonzac (:etienne)]

Comment on attachment 8397028 [details] [review]
PopupWindow v5

Like they said, the sixth time's the charm :)

Some comments on github but nothing that requires another review round.
Congrats!

Comment 23

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

https://github.com/mozilla-b2g/gaia/commit/f5b9126fb1b14066baf49775ffa2f54eee106be7

Hopefully we won't have too much backout ;)