Unified Desktop: hook up ash to allow unified desktop mode

Add unified desktop flag "--ash-enable-unified-desktop"

Enhanced MirrorWindowController to manage multiple mirroring windows.

BUG=365662
Committed: https://crrev.com/55ef921cb46ae2b1a95ddda8a376120db53eee69
Cr-Commit-Position: refs/heads/master@{#327172}

[oshima, 2015-04-25 08:47:58]

oshima@chromium.org changed reviewers:
+ mukai@chromium.org

[oshima, 2015-04-25 09:01:58]

Patchset #2 (id:20001) has been deleted

[oshima, 2015-04-25 09:02:01]

Patchset #1 (id:1) has been deleted

[oshima, 2015-04-25 09:05:45]

I'll add more comments and tests, but the rest is ready for review. PTAL.

[Jun Mukai, 2015-04-26 23:45:08]

https://codereview.chromium.org/1107733006/diff/60001/ash/ash.gyp
File ash/ash.gyp (right):

https://codereview.chromium.org/1107733006/diff/60001/ash/ash.gyp#newcode47
ash/ash.gyp:47:
'nnjnnnncontent/display/screen_orientation_controller_chromeos.h',
looks like a typo.

https://codereview.chromium.org/1107733006/diff/60001/ash/ash_switches.cc
File ash/ash_switches.cc (right):

https://codereview.chromium.org/1107733006/diff/60001/ash/ash_switches.cc#new...
ash/ash_switches.cc:136: const char kAshEnableUnifiedDesktop[] =
"ash-enable-unified-desktop";
Why don't you expose the flag to the header?
It'll come to about:flags anyway soon right?

https://codereview.chromium.org/1107733006/diff/60001/ash/display/display_con...
File ash/display/display_controller.cc (right):

https://codereview.chromium.org/1107733006/diff/60001/ash/display/display_con...
ash/display/display_controller.cc:645: primary_tree_host_for_replace_ = NULL;
nullptr

https://codereview.chromium.org/1107733006/diff/60001/ash/display/display_con...
ash/display/display_controller.cc:650: if (primary_tree_host_for_replace_) {
why not else if?

https://codereview.chromium.org/1107733006/diff/60001/ash/display/display_con...
ash/display/display_controller.cc:656: primary_tree_host_for_replace_ = NULL;
nullptr

https://codereview.chromium.org/1107733006/diff/60001/ash/display/display_man...
File ash/display/display_manager.cc (right):

https://codereview.chromium.org/1107733006/diff/60001/ash/display/display_man...
ash/display/display_manager.cc:1114: } break;
I prefer to write
  break;
}
order.

https://codereview.chromium.org/1107733006/diff/60001/ash/host/ash_window_tre...
File ash/host/ash_window_tree_host_virtual.cc (right):

https://codereview.chromium.org/1107733006/diff/60001/ash/host/ash_window_tre...
ash/host/ash_window_tree_host_virtual.cc:52: struct
AshWindowTreeHostVirtual::PerDisplayData {
I am not sure if PerDisplayData is worthwhile. If it's just a scoped_ptr, I
think we can okay to reveal it.

[oshima, 2015-04-27 17:48:38]

https://codereview.chromium.org/1107733006/diff/60001/ash/ash.gyp
File ash/ash.gyp (right):

https://codereview.chromium.org/1107733006/diff/60001/ash/ash.gyp#newcode47
ash/ash.gyp:47:
'nnjnnnncontent/display/screen_orientation_controller_chromeos.h',
On 2015/04/26 23:45:07, Jun Mukai wrote:
> looks like a typo.

sorry for garbage. done.

https://codereview.chromium.org/1107733006/diff/60001/ash/ash_switches.cc
File ash/ash_switches.cc (right):

https://codereview.chromium.org/1107733006/diff/60001/ash/ash_switches.cc#new...
ash/ash_switches.cc:136: const char kAshEnableUnifiedDesktop[] =
"ash-enable-unified-desktop";
On 2015/04/26 23:45:07, Jun Mukai wrote:
> Why don't you expose the flag to the header?
> It'll come to about:flags anyway soon right?

I'll expose when I add a flag. (I don't want to add a flag yet because it's not
ready)

https://codereview.chromium.org/1107733006/diff/60001/ash/display/display_con...
File ash/display/display_controller.cc (right):

https://codereview.chromium.org/1107733006/diff/60001/ash/display/display_con...
ash/display/display_controller.cc:645: primary_tree_host_for_replace_ = NULL;
On 2015/04/26 23:45:07, Jun Mukai wrote:
> nullptr

Done.

https://codereview.chromium.org/1107733006/diff/60001/ash/display/display_con...
ash/display/display_controller.cc:650: if (primary_tree_host_for_replace_) {
On 2015/04/26 23:45:07, Jun Mukai wrote:
> why not else if?

Done.

https://codereview.chromium.org/1107733006/diff/60001/ash/display/display_con...
ash/display/display_controller.cc:656: primary_tree_host_for_replace_ = NULL;
On 2015/04/26 23:45:07, Jun Mukai wrote:
> nullptr

Done.

https://codereview.chromium.org/1107733006/diff/60001/ash/display/display_man...
File ash/display/display_manager.cc (right):

https://codereview.chromium.org/1107733006/diff/60001/ash/display/display_man...
ash/display/display_manager.cc:1114: } break;
On 2015/04/26 23:45:08, Jun Mukai wrote:
> I prefer to write
>   break;
> }
> order.

Done.

https://codereview.chromium.org/1107733006/diff/60001/ash/display/display_man...
ash/display/display_manager.cc:1157: #endif
oops, I forgot to remove this. Removed.

https://codereview.chromium.org/1107733006/diff/60001/ash/host/ash_window_tre...
File ash/host/ash_window_tree_host_virtual.cc (right):

https://codereview.chromium.org/1107733006/diff/60001/ash/host/ash_window_tre...
ash/host/ash_window_tree_host_virtual.cc:52: struct
AshWindowTreeHostVirtual::PerDisplayData {
On 2015/04/26 23:45:08, Jun Mukai wrote:
> I am not sure if PerDisplayData is worthwhile. If it's just a scoped_ptr, I
> think we can okay to reveal it.

Forgot to remove it. thank you for the catch.

[Jun Mukai, 2015-04-27 18:23:40]

https://codereview.chromium.org/1107733006/diff/80001/ash/display/display_man...
File ash/display/display_manager_unittest.cc (right):

https://codereview.chromium.org/1107733006/diff/80001/ash/display/display_man...
ash/display/display_manager_unittest.cc:636: LOG(ERROR) << "============";
Is this necessary?

https://codereview.chromium.org/1107733006/diff/80001/ash/display/display_man...
ash/display/display_manager_unittest.cc:1466: #if !defined(OS_WIN) &&
defined(USE_X11)
Is this OS_CHROMEOS?
Do we want to test on linux desktop as well?

Also why X11?

https://codereview.chromium.org/1107733006/diff/80001/ash/display/mirror_wind...
File ash/display/mirror_window_controller.cc (right):

https://codereview.chromium.org/1107733006/diff/80001/ash/display/mirror_wind...
ash/display/mirror_window_controller.cc:171: } break;
style nit: break; } ?

https://codereview.chromium.org/1107733006/diff/100001/ash/display/mirror_win...
File ash/display/mirror_window_controller.cc (right):

https://codereview.chromium.org/1107733006/diff/100001/ash/display/mirror_win...
ash/display/mirror_window_controller.cc:249: mirroring_host_info_map_.size() ==
0)
map_.empty())

https://codereview.chromium.org/1107733006/diff/100001/ash/display/mirror_win...
ash/display/mirror_window_controller.cc:250: return nullptr;
curly braces.

https://codereview.chromium.org/1107733006/diff/100001/ash/display/mirror_win...
ash/display/mirror_window_controller.cc:251: return
mirroring_host_info_map_.begin()
DCHECK_EQ(1, mirroring_host_info_map_.size())?

https://codereview.chromium.org/1107733006/diff/100001/ash/display/root_windo...
File ash/display/root_window_transformers.cc (right):

https://codereview.chromium.org/1107733006/diff/100001/ash/display/root_windo...
ash/display/root_window_transformers.cc:287:
-SkIntToMScalar(display.bounds().y()));
Looks like it doesn't scale if the display height/width is different. Is it
intended?  Or do you think it's a TODO?

https://codereview.chromium.org/1107733006/diff/100001/ash/host/ash_window_tr...
File ash/host/ash_window_tree_host_virtual.cc (right):

https://codereview.chromium.org/1107733006/diff/100001/ash/host/ash_window_tr...
ash/host/ash_window_tree_host_virtual.cc:17: bool IsLocatedEvent(const
ui::Event& event) {
ui::Event has IsLocatedEvent() method.

https://codereview.chromium.org/1107733006/diff/100001/ash/host/ash_window_tr...
File ash/host/ash_window_tree_host_virtual.h (right):

https://codereview.chromium.org/1107733006/diff/100001/ash/host/ash_window_tr...
ash/host/ash_window_tree_host_virtual.h:26: class AshWindowTreeHostVirtual :
public AshWindowTreeHost,
I put the word "Virtual" for random reason, could be renamed if there is a
better name.
"Offscreen" could be better since we use this word in the init flag.

[oshima, 2015-04-27 20:30:56]

https://codereview.chromium.org/1107733006/diff/80001/ash/display/display_man...
File ash/display/display_manager_unittest.cc (right):

https://codereview.chromium.org/1107733006/diff/80001/ash/display/display_man...
ash/display/display_manager_unittest.cc:1466: #if !defined(OS_WIN) &&
defined(USE_X11)
On 2015/04/27 18:23:39, Jun Mukai wrote:
> Is this OS_CHROMEOS?
> Do we want to test on linux desktop as well?
> 
> Also why X11?

Because this doesn't work on ozone yet. I'll remove USE_X11 when compositor side
change is in.

https://codereview.chromium.org/1107733006/diff/80001/ash/display/mirror_wind...
File ash/display/mirror_window_controller.cc (right):

https://codereview.chromium.org/1107733006/diff/80001/ash/display/mirror_wind...
ash/display/mirror_window_controller.cc:171: } break;
On 2015/04/27 18:23:39, Jun Mukai wrote:
> style nit: break; } ?

Done.

https://codereview.chromium.org/1107733006/diff/100001/ash/display/mirror_win...
File ash/display/mirror_window_controller.cc (right):

https://codereview.chromium.org/1107733006/diff/100001/ash/display/mirror_win...
ash/display/mirror_window_controller.cc:249: mirroring_host_info_map_.size() ==
0)
On 2015/04/27 18:23:40, Jun Mukai wrote:
> map_.empty())

Done.

https://codereview.chromium.org/1107733006/diff/100001/ash/display/mirror_win...
ash/display/mirror_window_controller.cc:250: return nullptr;
On 2015/04/27 18:23:40, Jun Mukai wrote:
> curly braces.

Done.

https://codereview.chromium.org/1107733006/diff/100001/ash/display/mirror_win...
ash/display/mirror_window_controller.cc:251: return
mirroring_host_info_map_.begin()
On 2015/04/27 18:23:40, Jun Mukai wrote:
> DCHECK_EQ(1, mirroring_host_info_map_.size())?

Done.

https://codereview.chromium.org/1107733006/diff/100001/ash/display/root_windo...
File ash/display/root_window_transformers.cc (right):

https://codereview.chromium.org/1107733006/diff/100001/ash/display/root_windo...
ash/display/root_window_transformers.cc:287:
-SkIntToMScalar(display.bounds().y()));
On 2015/04/27 18:23:40, Jun Mukai wrote:
> Looks like it doesn't scale if the display height/width is different. Is it
> intended?  Or do you think it's a TODO?

Width can be different, but different height does not work.
I'm planning to add this support after all necessary changes are
in with testing. I added todo in display_manager code where it computes the
unified bounds.

https://codereview.chromium.org/1107733006/diff/100001/ash/host/ash_window_tr...
File ash/host/ash_window_tree_host_virtual.cc (right):

https://codereview.chromium.org/1107733006/diff/100001/ash/host/ash_window_tr...
ash/host/ash_window_tree_host_virtual.cc:17: bool IsLocatedEvent(const
ui::Event& event) {
On 2015/04/27 18:23:40, Jun Mukai wrote:
> ui::Event has IsLocatedEvent() method.

thanks, done.

https://codereview.chromium.org/1107733006/diff/100001/ash/host/ash_window_tr...
File ash/host/ash_window_tree_host_virtual.h (right):

https://codereview.chromium.org/1107733006/diff/100001/ash/host/ash_window_tr...
ash/host/ash_window_tree_host_virtual.h:26: class AshWindowTreeHostVirtual :
public AshWindowTreeHost,
On 2015/04/27 18:23:40, Jun Mukai wrote:
> I put the word "Virtual" for random reason, could be renamed if there is a
> better name.
> "Offscreen" could be better since we use this word in the init flag.

I agree that we need better name for this, but I haven't decided yet.
I personally prefer "Unified" in ash level, while keeping offscreen at
gfx level. May I change this in a separate CL?

[oshima, 2015-04-27 22:43:34]

renamed to AshWindowTreeHostUnified as discussed offline. PTAL.

[Jun Mukai, 2015-04-27 23:08:48]

lgtm with a nit

https://codereview.chromium.org/1107733006/diff/120001/ash/host/ash_window_tr...
File ash/host/ash_window_tree_host_virtual.cc (right):

https://codereview.chromium.org/1107733006/diff/120001/ash/host/ash_window_tr...
ash/host/ash_window_tree_host_virtual.cc:17: bool IsLocatedEvent(const
ui::Event& event) {
You can remove this function itself.

[oshima, 2015-04-27 23:12:03]

https://codereview.chromium.org/1107733006/diff/120001/ash/host/ash_window_tr...
File ash/host/ash_window_tree_host_virtual.cc (right):

https://codereview.chromium.org/1107733006/diff/120001/ash/host/ash_window_tr...
ash/host/ash_window_tree_host_virtual.cc:17: bool IsLocatedEvent(const
ui::Event& event) {
On 2015/04/27 23:08:48, Jun Mukai wrote:
> You can remove this function itself.

it's been removed in the latest patch. thanks.

[oshima, 2015-04-27 23:12:22]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2015-04-27 23:13:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1107733006/140001

[commit-bot: I haz the power, 2015-04-27 23:27:25]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2015-04-27 23:28:21]

Patchset 6 (id:??) landed as
https://crrev.com/55ef921cb46ae2b1a95ddda8a376120db53eee69
Cr-Commit-Position: refs/heads/master@{#327172}