@reveman, is there some way to specify the target display when we launch the app with garcon? If so, I can plumb that data through from Chrome.

The primary display is a result of this line:

https://cs.chromium.org/chromium/src/components/exo/shell_surface_base.cc?l=1190

We should probably change that so it ends up using ash::Shell::GetRootWindowForNewWindows() as then I think it will work as expected.

Great, so we don't need to change anything with RPCs.

Yeah, I think that will be enough. Do you want to change this or should I take care of it when I find time?

I got time...I'll go ahead and do it. :)

Crostini apps can also be launched with a command line command from the terminal window. For this use case, do we need to launch the app in the same display as the terminal window? Or is it okay to launch it in the primary display?

GetRootWindowForNewWindows() will return the root window for the active window. 

If the terminal you're typing into is going to be the active window so the new app will as a result launch on the same display. No special logic should be needed for crostini here I think.

For launcher and shelf launches, GetRootWindowForNewWindows() is not returning the display that the launcher or shelf is in. It instead always returns the primary display. I don't know if this worked before but at least for now, this is not working.
I'm working to fix it. However, without passing the display ID to the container, I see that there might be a race condition. If the app takes a while to launch and the UI has already moved to another display, the app might launch in the new display that the UI has moved to.

Isn't the launcher/shelf always on the primary display? It makes sense that applications launch on the display where the launcher is, when using that instead of a terminal to launch apps.

GetRootWindowForNewWindows changing between launch and when the app becomes visible is a valid concern. We should make sure we can handle that correctly. I don't think we should pass anything more to the container than the current startup ID though. The startup ID should be enough to track what display an app window should appear on.

There is a separate set of launcher and shelf on both displays. This is the first time I am using dual monitors and it is a surprise to me but that is the situation.

Ok, we should keep track of that then. The startup ID should still be sufficient.

I've been working on this change for about a week.
The current CL is here https://chromium-review.googlesource.com/c/chromium/src/+/1070711.
I also have an email chain asking for permission to access ash code from the shelf. https://groups.google.com/a/google.com/forum/#!topic/mustash-team/fZ-f6_GohX8.

Now I'm wondering if it makes sense to actually pass the display ID into the container. What is the drawback of doing that?
Initially It didn't seem necessary to do that but with this further look that might be a good approach.

A non-trusted client should not be allowed to control what display they appear on from a security and process isolation perspective. There's no standard wayland interface for this for that reason.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fcdfd39cb6b03b457e90fd8b2b8e75be870ef472

commit fcdfd39cb6b03b457e90fd8b2b8e75be870ef472
Author: Tim Zheng <timzheng@google.com>
Date: Tue Jun 05 18:37:00 2018

Launch Crostini app on the shelf display.

There is a separate copy of the shelf on each display in a multiple
display setup. When the Crostini app is launched by clicking the pinned
shelf item it is launched in the same display as the lauching shelf with
this change.

Further changes are going to be made in subsequent CLs to address
similar problems for app lauching from the launcher and for the special
terminal app.

BUG= chromium:843001 
TEST=Manual test on an eve device.

Change-Id: Ib26c708d3c7875f93a7979f7039ee9d28766e332
Reviewed-on: https://chromium-review.googlesource.com/1070711
Commit-Queue: Tim Zheng <timzheng@chromium.org>
Reviewed-by: David Reveman <reveman@chromium.org>
Reviewed-by: Timothy Loh <timloh@chromium.org>
Reviewed-by: Stefan Kuhne <skuhne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564590}
[add] https://crrev.com/fcdfd39cb6b03b457e90fd8b2b8e75be870ef472/chrome/browser/chromeos/crostini/crostini_app_launch_observer.h
[modify] https://crrev.com/fcdfd39cb6b03b457e90fd8b2b8e75be870ef472/chrome/browser/chromeos/crostini/crostini_util.cc
[modify] https://crrev.com/fcdfd39cb6b03b457e90fd8b2b8e75be870ef472/chrome/browser/chromeos/crostini/crostini_util.h
[modify] https://crrev.com/fcdfd39cb6b03b457e90fd8b2b8e75be870ef472/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/fcdfd39cb6b03b457e90fd8b2b8e75be870ef472/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
[modify] https://crrev.com/fcdfd39cb6b03b457e90fd8b2b8e75be870ef472/chrome/browser/ui/ash/launcher/chrome_launcher_controller.h
[add] https://crrev.com/fcdfd39cb6b03b457e90fd8b2b8e75be870ef472/chrome/browser/ui/ash/launcher/crostini_app_display.cc
[add] https://crrev.com/fcdfd39cb6b03b457e90fd8b2b8e75be870ef472/chrome/browser/ui/ash/launcher/crostini_app_display.h
[modify] https://crrev.com/fcdfd39cb6b03b457e90fd8b2b8e75be870ef472/chrome/browser/ui/ash/launcher/crostini_app_window_shelf_controller.cc
[modify] https://crrev.com/fcdfd39cb6b03b457e90fd8b2b8e75be870ef472/chrome/browser/ui/ash/launcher/crostini_app_window_shelf_controller.h
[modify] https://crrev.com/fcdfd39cb6b03b457e90fd8b2b8e75be870ef472/chrome/browser/ui/ash/launcher/crostini_shelf_context_menu.cc
[modify] https://crrev.com/fcdfd39cb6b03b457e90fd8b2b8e75be870ef472/chrome/browser/ui/ash/launcher/launcher_controller_helper.cc
[modify] https://crrev.com/fcdfd39cb6b03b457e90fd8b2b8e75be870ef472/components/exo/shell_surface_base.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/097ca56d447e164ec247fb2bcef9118a4e197911

commit 097ca56d447e164ec247fb2bcef9118a4e197911
Author: Tim Zheng <timzheng@google.com>
Date: Wed Jun 06 00:40:11 2018

Set window bounds once when moving display.

This change avoids set window bounds twice.

BUG= chromium:843001 
TEST=Manual test on an eve device.

Change-Id: If08f8e4fd7b8e91db43f265fa8fdc46c3da6eb86
Reviewed-on: https://chromium-review.googlesource.com/1087844
Reviewed-by: Stefan Kuhne <skuhne@chromium.org>
Commit-Queue: Tim Zheng <timzheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564749}
[modify] https://crrev.com/097ca56d447e164ec247fb2bcef9118a4e197911/chrome/browser/ui/ash/launcher/crostini_app_window_shelf_controller.cc

The Crostini terminal is an extension app and its starting display choice behavior is the same as all other extension apps. That behavior is supposed to be fixed https://bugs.chromium.org/p/chromium/issues/detail?id=586091. However, it looks that has regressed and is not working.

I tested on a Caroline in dev channel. The behavior is inconsistent for gmail app between the shelf and launcher. It works correctly from the shelf but not the launcher.
https://bugs.chromium.org/p/chromium/issues/detail?id=850335

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/73c3c21ad25ae9cb9d8f68a96fd4458b80f41a22

commit 73c3c21ad25ae9cb9d8f68a96fd4458b80f41a22
Author: Tim Zheng <timzheng@google.com>
Date: Thu Jun 07 20:38:05 2018

Launch Crostini app on the launcher display.

When a Crostini app is started by clicking on the launcher item, this
change makes the app windows start in the same display as the launcher.

BUG= chromium:843001 
TEST=Manual test on an eve device.

Change-Id: Id3b65d71148d1e4107d8f9bddd31b2a27df6a58b
Reviewed-on: https://chromium-review.googlesource.com/1089880
Reviewed-by: calamity <calamity@chromium.org>
Reviewed-by: Stefan Kuhne <skuhne@chromium.org>
Commit-Queue: Tim Zheng <timzheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565391}
[modify] https://crrev.com/73c3c21ad25ae9cb9d8f68a96fd4458b80f41a22/chrome/browser/ui/app_list/app_list_controller_impl.cc
[modify] https://crrev.com/73c3c21ad25ae9cb9d8f68a96fd4458b80f41a22/chrome/browser/ui/app_list/crostini/crostini_app_item.cc
[modify] https://crrev.com/73c3c21ad25ae9cb9d8f68a96fd4458b80f41a22/chrome/browser/ui/app_list/search/crostini_app_result.cc
[modify] https://crrev.com/73c3c21ad25ae9cb9d8f68a96fd4458b80f41a22/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
[modify] https://crrev.com/73c3c21ad25ae9cb9d8f68a96fd4458b80f41a22/chrome/browser/ui/ash/launcher/chrome_launcher_controller.h
[modify] https://crrev.com/73c3c21ad25ae9cb9d8f68a96fd4458b80f41a22/chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2eeb5462bee20dbbba8ec71a16f51afe916e1447

commit 2eeb5462bee20dbbba8ec71a16f51afe916e1447
Author: Tim Zheng <timzheng@google.com>
Date: Wed Jun 20 20:22:50 2018

Start Crostini terminal in the right display.

This change start up the Crostini terminal in the same display as the
shelf/launcher used to launch the terminal app.

BUG= chromium:843001 
TEST=Manual test on an eve device.

Change-Id: Ie4f748eda46151ecba3b3597513a7b22753dfca6
Reviewed-on: https://chromium-review.googlesource.com/1103577
Reviewed-by: Ben Wells <benwells@chromium.org>
Reviewed-by: Stefan Kuhne <skuhne@chromium.org>
Commit-Queue: Tim Zheng <timzheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568977}
[modify] https://crrev.com/2eeb5462bee20dbbba8ec71a16f51afe916e1447/chrome/browser/chromeos/crostini/crostini_manager.cc
[modify] https://crrev.com/2eeb5462bee20dbbba8ec71a16f51afe916e1447/chrome/browser/chromeos/crostini/crostini_manager.h
[modify] https://crrev.com/2eeb5462bee20dbbba8ec71a16f51afe916e1447/chrome/browser/chromeos/crostini/crostini_util.cc
[modify] https://crrev.com/2eeb5462bee20dbbba8ec71a16f51afe916e1447/chrome/browser/ui/ash/launcher/crostini_app_window_shelf_controller.cc
[modify] https://crrev.com/2eeb5462bee20dbbba8ec71a16f51afe916e1447/chrome/browser/ui/ash/launcher/crostini_app_window_shelf_controller.h
[modify] https://crrev.com/2eeb5462bee20dbbba8ec71a16f51afe916e1447/chrome/browser/ui/extensions/application_launch.cc
[modify] https://crrev.com/2eeb5462bee20dbbba8ec71a16f51afe916e1447/chrome/browser/ui/extensions/application_launch.h
[modify] https://crrev.com/2eeb5462bee20dbbba8ec71a16f51afe916e1447/chrome/browser/ui/views/crostini/crostini_installer_view_browsertest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3f8166d2daf2154d95ef04abab7bd508ded45201

commit 3f8166d2daf2154d95ef04abab7bd508ded45201
Author: Tim Zheng <timzheng@google.com>
Date: Wed Jun 20 20:31:07 2018

Try resize window in proportion to display size.

When moving Crostini app window between displays, try to resize the
window in proportion to the relative size of the displays.

BUG= chromium:843001 
TEST=Manual test on an eve device.

Change-Id: Ic72ae35dcc6b1e07da3877aba51d225f96a4fa4d
Reviewed-on: https://chromium-review.googlesource.com/1103609
Reviewed-by: Stefan Kuhne <skuhne@chromium.org>
Commit-Queue: Tim Zheng <timzheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568990}
[modify] https://crrev.com/3f8166d2daf2154d95ef04abab7bd508ded45201/chrome/browser/ui/ash/launcher/crostini_app_window_shelf_controller.cc

While investigate  crbug.com/854911 , I found out that this is not working for the Crostini apps that don't implement startup notification.
Potential fix for this is related to the fix for bug  crbug.com/854911 .

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c4aebd6adb028eaee92c5d68ed5887d5234ae5a8

commit c4aebd6adb028eaee92c5d68ed5887d5234ae5a8
Author: Tim Zheng <timzheng@google.com>
Date: Sat Jul 07 06:45:05 2018

Use app ID instead of startup ID for display.

This change switch to use Crostini app ID instead of the startup ID to
track the display that the apps are started from.
There are Crostini apps that don't implement the startup notification
protocol but can be linked with their application ID. This change will
make those apps start in the correct display.

BUG= chromium:843001 
TEST=Manually tested on an eve device.

Change-Id: I6033f9fdc187dede9138c20be7935c6e9a802f8c
Reviewed-on: https://chromium-review.googlesource.com/1125515
Commit-Queue: Nicholas Verne <nverne@chromium.org>
Reviewed-by: Nicholas Verne <nverne@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573156}
[modify] https://crrev.com/c4aebd6adb028eaee92c5d68ed5887d5234ae5a8/chrome/browser/chromeos/crostini/crostini_app_launch_observer.h
[modify] https://crrev.com/c4aebd6adb028eaee92c5d68ed5887d5234ae5a8/chrome/browser/chromeos/crostini/crostini_util.cc
[modify] https://crrev.com/c4aebd6adb028eaee92c5d68ed5887d5234ae5a8/chrome/browser/ui/ash/launcher/crostini_app_display.cc
[modify] https://crrev.com/c4aebd6adb028eaee92c5d68ed5887d5234ae5a8/chrome/browser/ui/ash/launcher/crostini_app_display.h
[modify] https://crrev.com/c4aebd6adb028eaee92c5d68ed5887d5234ae5a8/chrome/browser/ui/ash/launcher/crostini_app_window_shelf_controller.cc
[modify] https://crrev.com/c4aebd6adb028eaee92c5d68ed5887d5234ae5a8/chrome/browser/ui/ash/launcher/crostini_app_window_shelf_controller.h