ash: Add RootWindowHostFactory class.

ash: Add RootWindowHostFactory class.

This provides a mechanism for creating a RootWindowHost to use in place
of the default one that aura::RootWindow will create. This is exposed
to consumers of ash through the addition of the CreateRootWindowHostFactory
function to the ShellDelegate interface.

BUG=124444
TEST=ash_unittests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173080

[reveman, 2012-11-01 22:09:44]

Re-factoring that makes it easier to replace the RootWindowHost.

Ben, this is a follow up to your feedback here:
https://codereview.chromium.org/10789018/

[danakj, 2012-12-03 18:26:04]

ping for review please :)

[Ben Goodger (Google), 2012-12-11 01:14:02]

My latency is bad right now for things not on my critical path, please bear with
me. I do want to review the ash/aura touchpoints for this.

https://chromiumcodereview.appspot.com/11360045/diff/8001/ui/aura/root_window.h
File ui/aura/root_window.h (right):

https://chromiumcodereview.appspot.com/11360045/diff/8001/ui/aura/root_window...
ui/aura/root_window.h:75: RootWindowDelegate* delegate;
Why can't you just set host to the value you want?

This breaks desktop aura btw, which sets this field to something else in several
places in the code.

As an aside, I'd really like to see aura not have any of the RootWindowHost
impls. It's turned out that all of them are very implementation specific.

A very basic RootWindowHost interface should live in aura (much more simple than
what's there now), and nothing more. Ash would create its own kind, and
views/desktop-aura/chrome another.

[reveman, 2012-12-11 07:23:30]

https://chromiumcodereview.appspot.com/11360045/diff/8001/ui/aura/root_window.h
File ui/aura/root_window.h (right):

https://chromiumcodereview.appspot.com/11360045/diff/8001/ui/aura/root_window...
ui/aura/root_window.h:75: RootWindowDelegate* delegate;
On 2012/12/11 01:14:02, Ben Goodger (Google) wrote:
> Why can't you just set host to the value you want?
> 
> This breaks desktop aura btw, which sets this field to something else in
several
> places in the code.
> 
> As an aside, I'd really like to see aura not have any of the RootWindowHost
> impls. It's turned out that all of them are very implementation specific.
> 
> A very basic RootWindowHost interface should live in aura (much more simple
than
> what's there now), and nothing more. Ash would create its own kind, and
> views/desktop-aura/chrome another.

OK, I like this. We don't need RootWindowDelegate. That was really just so we
could implement this cleanly without having an ash specific RootWindowHost.

Knowing that we want to move RootWindowHost impls to ash this patch can be made
simpler.

[reveman, 2012-12-11 08:39:23]

Latest patch doesn't have the RootWindow::CreateParams modification or the
RootWindowDelegate class. It instead adds an ash::RootWindowHost class. It
doesn't actually move any of the aura::RootWindowHost impls to ash but I created
a bug [1] for that which I'm interested in working on once this CL lands. PTAL.

[1] http://code.google.com/p/chromium/issues/detail?id=165409

[Ben Goodger (Google), 2012-12-11 16:37:22]

instead of the delegate wiring... can you actually look into moving the
root_window_host_* impls from ui/aura to ash?

I'd create a src/ash/host directory and look at moving those files into it.
Leave the RootWindowHost interface in aura.

Instead of delegates, you could have a factory method exposed in the ash
namespace: CreateRootWindowHost() that any code in ash could call.

[reveman, 2012-12-11 17:17:34]

On 2012/12/11 16:37:22, Ben Goodger (Google) wrote:
> instead of the delegate wiring... can you actually look into moving the
> root_window_host_* impls from ui/aura to ash?
> 
> I'd create a src/ash/host directory and look at moving those files into it.
> Leave the RootWindowHost interface in aura.
> 
> Instead of delegates, you could have a factory method exposed in the ash
> namespace: CreateRootWindowHost() that any code in ash could call.

I don't mind moving root_window_host_* impls to src/ash/host but the purpose of
the delegate changes are to provide a method for ash::ShellDelegate impls to
provide their own RootWindowHost implementation. This is what we need for the wm
component that uses ash but need to have its own implementation of
RootWindowHost (See https://chromiumcodereview.appspot.com/11485006/).

I don't see how an ash root window host factory system would be able to provide
the same functionality but maybe I'm missing something?

[Ben Goodger (Google), 2012-12-11 17:54:07]

On 2012/12/11 17:17:34, David Reveman wrote:
> I don't see how an ash root window host factory system would be able to
provide
> the same functionality but maybe I'm missing something?

OK I think I'm asking for pretty much the same thing you've done. Can you move
the root_window_host* stuff you have in ash right now to a new host/ subdir, and
rename ash::RootWindowHost to ash::RootWindowHostFactory.

I don't see the need for the DisplayManagerDelegate. Instead I'd have it call
through to the RootWindowHostFactory directly, but make the RWHF something that
can be replaced.

[reveman, 2012-12-12 21:50:58]

Remove DisplayManagerDelegate, added RootWindowHostFactory class and moved code
to src/ash/host/. ShellDelegate::CreateRootWindowHostFactory makes it possible
to replace the RWHF used by ash. PTAL.

[Ben Goodger (Google), 2012-12-12 22:50:15]

Can you run try-jobs against win_aura and linux_aura?

https://codereview.chromium.org/11360045/diff/18001/ash/host/root_window_host...
File ash/host/root_window_host_factory_win.cc (right):

https://codereview.chromium.org/11360045/diff/18001/ash/host/root_window_host...
ash/host/root_window_host_factory_win.cc:19: if (base::win::GetVersion() >=
base::win::VERSION_WIN8) {
nit: no braces

[reveman, 2012-12-13 18:29:19]

win_aura bot doesn't seem very happy at the moment. win_ash and linux_aura
completed successfully and as win_aura makes it passed compile before failing
from what seems like unrelated issues I'm pretty confident that this works as it
should.

https://codereview.chromium.org/11360045/diff/18001/ash/host/root_window_host...
File ash/host/root_window_host_factory_win.cc (right):

https://codereview.chromium.org/11360045/diff/18001/ash/host/root_window_host...
ash/host/root_window_host_factory_win.cc:19: if (base::win::GetVersion() >=
base::win::VERSION_WIN8) {
On 2012/12/12 22:50:15, Ben Goodger (Google) wrote:
> nit: no braces

Done.

[Ben Goodger (Google), 2012-12-13 18:56:22]

Can you ask cpu@ to run your change on win8 to make sure it doesn't regress
his stuff and jam@ to run your change against a known-good copy of the
browser tests?

-Ben


On Thu, Dec 13, 2012 at 10:29 AM, <reveman@chromium.org> wrote:

> win_aura bot doesn't seem very happy at the moment. win_ash and linux_aura
> completed successfully and as win_aura makes it passed compile before
> failing
> from what seems like unrelated issues I'm pretty confident that this works
> as it
> should.
>
>
>
> https://codereview.chromium.**org/11360045/diff/18001/ash/**
>
host/root_window_host_factory_**win.cc<https://codereview.chromium.org/11360045/diff/18001/ash/host/root_window_host_factory_win.cc>
> File ash/host/root_window_host_**factory_win.cc (right):
>
> https://codereview.chromium.**org/11360045/diff/18001/ash/**
>
host/root_window_host_factory_**win.cc#newcode19<https://codereview.chromium.org/11360045/diff/18001/ash/host/root_window_host_factory_win.cc#newcode19>
> ash/host/root_window_host_**factory_win.cc:19: if (base::win::GetVersion()
>
>> = base::win::VERSION_WIN8) {
>>
> On 2012/12/12 22:50:15, Ben Goodger (Google) wrote:
>
>> nit: no braces
>>
>
> Done.
>
>
https://codereview.chromium.**org/11360045/<https://codereview.chromium.org/1...
>

[cpu_(ooo_6.6-7.5), 2012-12-13 22:30:14]

my client is broken but looks ok. I'll fix it on windows if it breaks me.

[Ben Goodger (Google), 2012-12-13 22:32:47]

lgtm

[commit-bot: I haz the power, 2012-12-13 23:23:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/11360045/22001

[commit-bot: I haz the power, 2012-12-14 04:13:31]

Change committed as 173080