In order to prototype the "haidification" of apps further [1], we'll need to be able to spawn new "sheets" from an app. Currently, if an app calls window.open it goes through the PopupManager [2] and opens a modal window with chrome (it's used for facebook auth...). One exception is the AttentionWindow [3] which uses a special feature keyword to open a window on top of everything else (like the dialer's call screen). We probably want to implement something similar the the AttentionWindowFactory submodule to handle new sheets. ie. * Listen for |mozbrowseropenwindow| event on the app iframe directly * Intercept (prevent default) requests for windows _on the same origin_ than the app (so opening a facebook URL for auth would still spawn a window with some chrome) * Publish an open-sheet event -> The AppWindowManager will listen for the open-sheet events, take over, instantiate the AppWindow (which will be properly inserted in the StackManager), transition the new sheet from the right... PS: Not sure how to handle the case where an app window.open on the exact same URL it's currently displaying, pretty sure some stuff will break, we might want to prevent this. [1] see bug 946750 [2] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/popup_manager.js#L33 [3] see bug 927862

feedback? on the proposition in the Description.

Hey Gregor, not sure how SystemFE bugs get prioritized, but apparently this is a MWC blocker.

Etienne, IMO this should be done in PopupWindow rewrite. Attention is too strong to use and the animation is not the same. We could do this in PopupWindowFactory: 1. Open a remote url: append browser chrome to PopupWindow/BrowserWindow and use popup animation 2. Open the same origin: do not append browser chrome to PopupWindow/BrowserWindow and use haida animation

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #3) > Etienne, IMO this should be done in PopupWindow rewrite. Attention is too > strong to use and the animation is not the same. Small misunderstanding, I meant doing it in a similar submodule, not as part of the AttentionWindows. > We could do this in PopupWindowFactory: > 1. Open a remote url: append browser chrome to PopupWindow/BrowserWindow and > use popup animation > 2. Open the same origin: do not append browser chrome to > PopupWindow/BrowserWindow and use haida animation But don't we want the new sheet the be an AppWindow? It should be part of the navigation stack, the cardview and should be able to open a new sheet too.

(In reply to Etienne Segonzac (:etienne) from comment #4) > (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #3) > > Etienne, IMO this should be done in PopupWindow rewrite. Attention is too > > strong to use and the animation is not the same. > > Small misunderstanding, I meant doing it in a similar submodule, not as part > of the AttentionWindows. > > > We could do this in PopupWindowFactory: > > 1. Open a remote url: append browser chrome to PopupWindow/BrowserWindow and > > use popup animation > > 2. Open the same origin: do not append browser chrome to > > PopupWindow/BrowserWindow and use haida animation > > But don't we want the new sheet the be an AppWindow? It should be part of > the navigation stack, the cardview and should be able to open a new sheet > too. Cardview should be another story :/ A submodule to help window.open features does make sense, and thank you for reading my code before I set review. But I think I know what you mean now, we need a XXXXWindowFactory for app wants to open a new sheet. So my question is what's the features keyword for app to open sheets in the long run?

(In reply to Alive Kuo [:alive][NEEDINFO!][:艾莉芙] from comment #5) > So my question is what's the features keyword for app to open sheets in the > long run? I _think_ all window.open on the app origin (minus the attention screens) could be new sheets by default. But if we have a rationale to use another special feature instead I'm completely fine with that too. Vivien, any thought?

(In reply to Etienne Segonzac (:etienne) from comment #6) > (In reply to Alive Kuo [:alive][NEEDINFO!][:艾莉芙] from comment #5) > > So my question is what's the features keyword for app to open sheets in the > > long run? > > I _think_ all window.open on the app origin (minus the attention screens) > could be new sheets by default. > But if we have a rationale to use another special feature instead I'm > completely fine with that too. > > Vivien, any thought? By comment 6 we would have * submodule: PopupWindowFactory to open PopupWindow(has no browser chrome) from a non-app Window. * submodule: AttentionWindowFactory to open AttentionWindow from an app window instance with features=attention * submodule: SheetWindowFactory? to open AppWindow from an app window instance without features. But I think this doesn't make sense if an installed app tries to open |http://google.com| but we make an AppWindow for it thus google.com as it looks like part of the app as a sheet?

(In reply to Alive Kuo [:alive][NEEDINFO!][:艾莉芙] from comment #7) > (In reply to Etienne Segonzac (:etienne) from comment #6) > > (In reply to Alive Kuo [:alive][NEEDINFO!][:艾莉芙] from comment #5) > > > So my question is what's the features keyword for app to open sheets in the > > > long run? > > > > I _think_ all window.open on the app origin (minus the attention screens) > > could be new sheets by default. > > But if we have a rationale to use another special feature instead I'm > > completely fine with that too. > > > > Vivien, any thought? > > By comment 6 we would have > * submodule: PopupWindowFactory to open PopupWindow(has no browser chrome) > from a non-app Window. > * submodule: AttentionWindowFactory to open AttentionWindow from an app > window instance with features=attention > * submodule: SheetWindowFactory? to open AppWindow from an app window > instance without features. > > But I think this doesn't make sense if an installed app tries to open > |http://google.com| but we make an AppWindow for it thus google.com as it > looks like part of the app as a sheet? I agree that an installed app that pops up a new AppWindow but we still want a sheet, pointing for example to |http://google.com| should not create an AppWindow but a Popup/BrowserWindow. For example if there is an installed Twitter app and the user click on a link inside it, then it should popup as a new independent sheet. Part of my idea was that if the window is opened with the |dialog| keyword then the window will popup covering the current window/sheet by coming from the bottom and sliding to the top. So this one sounds like a DialogWindow to me (if we want to split PopupWindow into multiples submodules), that should be dependent on the state of the sheet. So if the sheet is killed/closed then it should be removed. If the window is not opened with this keyword, then it will pop up as a new sheet, either a new AppWindow or a BrowserWindow, sliding from the left to the right. Hope it makes sense.

On it right now. My first bug during this ww :) Proposal: Implement submodule of AppWindow, ChildWindowFactory to deal with all window.open request. I will update AttentionWindowFactory later if I have some time to finish that at this week.

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #8) > (In reply to Alive Kuo [:alive][NEEDINFO!][:艾莉芙] from comment #7) > > (In reply to Etienne Segonzac (:etienne) from comment #6) > > > (In reply to Alive Kuo [:alive][NEEDINFO!][:艾莉芙] from comment #5) > > > > So my question is what's the features keyword for app to open sheets in the > > > > long run? > > > > > > I _think_ all window.open on the app origin (minus the attention screens) > > > could be new sheets by default. > > > But if we have a rationale to use another special feature instead I'm > > > completely fine with that too. > > > > > > Vivien, any thought? > > > > By comment 6 we would have > > * submodule: PopupWindowFactory to open PopupWindow(has no browser chrome) > > from a non-app Window. > > * submodule: AttentionWindowFactory to open AttentionWindow from an app > > window instance with features=attention > > * submodule: SheetWindowFactory? to open AppWindow from an app window > > instance without features. > > > > But I think this doesn't make sense if an installed app tries to open > > |http://google.com| but we make an AppWindow for it thus google.com as it > > looks like part of the app as a sheet? > > I agree that an installed app that pops up a new AppWindow but we still want > a sheet, pointing for example to |http://google.com| should not create an > AppWindow but a Popup/BrowserWindow. > For example if there is an installed Twitter app and the user click on a > link inside it, then it should popup as a new independent sheet. > > Part of my idea was that if the window is opened with the |dialog| keyword > then the window will popup covering the current window/sheet by coming from > the bottom and sliding to the top. > > So this one sounds like a DialogWindow to me (if we want to split > PopupWindow into multiples submodules), that should be dependent on the > state of the sheet. So if the sheet is killed/closed then it should be > removed. > > If the window is not opened with this keyword, then it will pop up as a new > sheet, either a new AppWindow or a BrowserWindow, sliding from the left to > the right. > > Hope it makes sense. Let's clarify again: ### Without any features ### * appWindow(installed twitter) uses window.open: an new AppWindow as child of current AppWindow. No dependency. (But if it's opening the same origin page, they will be in same process and OOM killer would make us kill both.) (So what do you mean by 'dependent'?) * browserWindow(web page) uses window.open: an new BrowserWindow as child of current BrowserWindow. No dependency. ### With attention features ### * Open AttentionWindow if it has permission, otherwise opens Whatever window its parent is. ### With dialog features ### * Open DialogWindow, no permission check here. Dependency.

WIP https://github.com/alivedise/gaia/compare/bugzilla;961800;child-window-factory?expand=1 This patch is 'just' working. Issue revealed: 1. 2 or more sheets of same app in cardview. Idea: Patch cardview to show leaf window only if appWindow has 'childWindow' property. 2. Sheet manager is not smart enough. Idea: Count relationship in 3. AppWindowManager display root window but not leaf window when the app is opened again. 4. We may have some trouble if popup/activity/attetnion could open new sheet.

WIP today https://github.com/alivedise/gaia/compare/bugzilla;961800;child-window-factory?expand=1 function works, need to fix and add unit tests.

I wonder we should make window.open always open OOP mozbrowser iframe otherwise we would change current behavior.

One more problem is we may at the same enable 'multiple same origin' app window instances. But AppWindowManager still store instances by origin now...that's bad.

1. AppWindowManager fix: discard origin reference as best as I could! 2. Change StackManager to adopt in app sheets 3. AppWindow API change 4. window.open changes 5. Implement ChildWindowFactory.

I think I'd fixed almost the broken tests. Still watching..

Comment on attachment 8370863 [details] https://github.com/mozilla-b2g/gaia/pull/16045 Use a new pull request :)

Comment on attachment 8370863 [details] https://github.com/mozilla-b2g/gaia/pull/16045 Very impressive patch! Very cool to have the test apps to play with it :) I did a solid first pass, mainly about testing. PS: we should create a new attachment with the correct PR

Second run!

Zac, could you guide me what's wrong with the GaiaApps modification? Seeing 'JavascriptException: JavascriptException: TypeError: appWindow is null stacktrace: execute_async_script @gaia_test.py, line 82 inline javascript, line 295 src: " let origin = appWindow.origin;"' But not sure how to debug it...

(In reply to Alive Kuo [:alive][NEEDINFO!][:艾莉芙] from comment #20) > Zac, could you guide me what's wrong with the GaiaApps modification? > Seeing 'JavascriptException: JavascriptException: TypeError: appWindow is > null > stacktrace: > execute_async_script @gaia_test.py, line 82 > inline javascript, line 295 > src: " let origin = appWindow.origin;"' > But not sure how to debug it... I am not hugely familiar with this part of the code (Dave and Jonathan did most of it) but it looks like the line prior to this one in gaia_apps.js is not returning anything, it is: let appWindow = GaiaApps.getAppByName(appName); The appName the test passes in as per the manifest file. Why don't you put an exception if the loop in getAppByName finishes without finding the app? To actually run the tests locally: https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/gaia-ui-tests/Gaia_UI_Tests_Run_Tests#Running_tests_using_the_Travis_scripts_%28for_Gecko_and_Gaia_developers%29 You can also ping Askeing for help!

Comment on attachment 8374735 [details] ChildWindowFactory v4 Second round...done :) Mainly the missing ChildWindowFactory coverage and the broken python test, the third one should be the charm :)

(In reply to Etienne Segonzac (:etienne) from comment #22) > Comment on attachment 8374735 [details] > ChildWindowFactory v2 > > Second round...done :) > > Mainly the missing ChildWindowFactory coverage and the broken python test, > the third one should be the charm :) OH I just noticed I forget to git add child_window_factory_test....zombie ate my brain...

Dave could you help to debug gaia_apps change?

I'm not sure what I can add, other than I tend to use console.log for debugging the atoms.

I am getting this message on local run: test_cold_launch (test_cold_launch.TestColdLaunch) ... ERROR ====================================================================== ERROR: None ---------------------------------------------------------------------- Traceback (most recent call last): File "/Library/Python/2.6/site-packages/marionette_client-0.7.3-py2.6.egg/marionette/marionette_test.py", line 143, in run testMethod() File "/Users/alive/Projects/gaia/tests/python/gaia-ui-tests/gaiatest/tests/unit/test_cold_launch.py", line 11, in test_cold_launch app = self.apps.launch('Clock') File "/Users/alive/Projects/gaia/tests/python/gaia-ui-tests/gaiatest/gaia_test.py", line 91, in launch self.switch_to_frame(app.frame_id, url) File "/Users/alive/Projects/gaia/tests/python/gaia-ui-tests/gaiatest/gaia_test.py", line 166, in switch_to_frame if check(self.marionette.get_url()): File "/Library/Python/2.6/site-packages/marionette_client-0.7.3-py2.6.egg/marionette/marionette.py", line 901, in get_url response = self._send_message("getCurrentUrl", "value") File "/Library/Python/2.6/site-packages/marionette_client-0.7.3-py2.6.egg/marionette/marionette.py", line 605, in _send_message self._handle_error(response) File "/Library/Python/2.6/site-packages/marionette_client-0.7.3-py2.6.egg/marionette/marionette.py", line 667, in _handle_error raise MarionetteException(message=response, status=500) MarionetteException: MarionetteException: {u'message': u'Marionette does not recognize the packet type "getCurrentUrl"', u'error': u'unrecognizedPacketType'} TEST-UNEXPECTED-FAIL | test_cold_launch.py test_cold_launch.TestColdLaunch.test_cold_launch | ----------------------------------------------------------------------

Comment on attachment 8374735 [details] ChildWindowFactory v4 Only one python test is failing now so I think it's proper to set review now.

(In reply to Alive Kuo [:alive][NEEDINFO!][:艾莉芙] from comment #26) > I am getting this message on local run: > > test_cold_launch (test_cold_launch.TestColdLaunch) ... ERROR > > ====================================================================== > ERROR: None > ---------------------------------------------------------------------- > Traceback (most recent call last): > File > "/Library/Python/2.6/site-packages/marionette_client-0.7.3-py2.6.egg/ > marionette/marionette_test.py", line 143, in run > testMethod() > File > "/Users/alive/Projects/gaia/tests/python/gaia-ui-tests/gaiatest/tests/unit/ > test_cold_launch.py", line 11, in test_cold_launch > app = self.apps.launch('Clock') > File > "/Users/alive/Projects/gaia/tests/python/gaia-ui-tests/gaiatest/gaia_test. > py", line 91, in launch > self.switch_to_frame(app.frame_id, url) > File > "/Users/alive/Projects/gaia/tests/python/gaia-ui-tests/gaiatest/gaia_test. > py", line 166, in switch_to_frame > if check(self.marionette.get_url()): > File > "/Library/Python/2.6/site-packages/marionette_client-0.7.3-py2.6.egg/ > marionette/marionette.py", line 901, in get_url > response = self._send_message("getCurrentUrl", "value") > File > "/Library/Python/2.6/site-packages/marionette_client-0.7.3-py2.6.egg/ > marionette/marionette.py", line 605, in _send_message > self._handle_error(response) > File > "/Library/Python/2.6/site-packages/marionette_client-0.7.3-py2.6.egg/ > marionette/marionette.py", line 667, in _handle_error > raise MarionetteException(message=response, status=500) > MarionetteException: MarionetteException: {u'message': u'Marionette does not > recognize the packet type "getCurrentUrl"', u'error': > u'unrecognizedPacketType'} > TEST-UNEXPECTED-FAIL | test_cold_launch.py > test_cold_launch.TestColdLaunch.test_cold_launch | > ---------------------------------------------------------------------- This is likely related to bug 941136. You will need to make sure you have a matching Marionette client/server versions.

Comment on attachment 8374735 [details] ChildWindowFactory v4 And... third round. Let's do a last round once the comments are addressed and travis is green with a contacts app, email app and a homescreen app peer in the loop.

Comment on attachment 8374735 [details] ChildWindowFactory v4 V4

Comment on attachment 8374735 [details] ChildWindowFactory v4 LGTM, just a question on github, thx

Are we documenting the various window.open features we have? (attention, dialog...) Should we? (sorry if this isn't the correct use of dev-doc-needed)

Comment on attachment 8374735 [details] ChildWindowFactory v4 r=me with comments addressed, we should probably wait for the 1.4 branch to be created before landing this. Huge patch, huge feature, huge potential :)

Comment on attachment 8374735 [details] ChildWindowFactory v4 Great job, I've been trying the contacts and ftu modifications, working well, also the tricky activity cicle between contacts, sms and contacts again working well. Thanks Alive!

bubble tea https://github.com/mozilla-b2g/gaia/commit/bb50bacf3d1c60dc39dfe0dfcf635f6c397f5e3e

in master https://github.com/gasolin/gaia/commit/439b23a04c6dba788293f577b1efe8c5d8843b78

Reverted in https://github.com/mozilla-b2g/gaia/commit/e11b3cd5c35a564075fe21a610604418975f0c00 because something in this or one of the six other patches that the b2g bumper bot included in a single push to b2g-inbound broke gaia-ui tests on TBPL: bumper bot's push's contents: https://hg.mozilla.org/integration/b2g-inbound/rev/0c2938cd910b The gaia-ui failure: https://tbpl.mozilla.org/php/getParsedLog.php?id=36421229&tree=B2g-Inbound In the future, can we please not merge seven patches in the course of 5 minutes? It would make tracking down regressions much simpler if they were spaced several minutes apart each.

https://travis-ci.org/mozilla-b2g/gaia/builds/21163364 green reland https://github.com/mozilla-b2g/gaia/commit/a1b8093857a64538b7e6758d129dd4958fde003f

hi Alive, sorry had to revert this changeset again, seems we run into a gaia ui test bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=36439850&tree=B2g-Inbound which seems the same like in comment #39 again

All green, https://tbpl.mozilla.org/?tree=Try&rev=866b6c52b90a https://travis-ci.org/mozilla-b2g/gaia/builds/21400719 Landing.

See https://bugzilla.mozilla.org/show_bug.cgi?id=987014#c1. Patch landings are not allowed to blindly shut off tests to get a feature to land. This is a critical automated test that cannot be shut off in the build, so this needs to be backed out.

This will require a release of gaiatest and a dependency bump for all packages that depend on it, such as b2gperf, in order to restore the ability to test against master.

I'd fixed the problem by some tricks. Will backout and reland soon.

50b956066751019d4adcb5e8c36eeda2d82de826 reverted

https://github.com/mozilla-b2g/gaia/pull/17514 Awaiting tbpl and travis.