Add window manager component.

Add window manager component.

This adds a new component to chrome that builds with GYP_DEFINES="use_wm=1"
and implements "foreign" window management. A foreign window is a
platform specific window not owned by chromium. Using the
CHROMIUM_texture_from_image extension we can redirect foreign window
output to textures and manage these windows using ash.
    
This initial version has limited functionality and focuses on getting
the testing framework in place.

BUG=124444
TEST=wm_unittests, wm_shell

[reveman, 2012-12-17 18:23:33]

Latest patch adopts the term "foreign window" when referring to platform
specific windows not owned by chromium. 

There's currently a large amount of code duplication in
wm/host/root_window_host_linux.cc. My plan is to reduce this after moving
ui/aura/root_window_host_linux.cc to ash but I don't think that needs to be done
before this lands. I actually prefer that we delay this work until it's more
clear what's needed by the wm component.

Dana, I think this is ready for you to review.

[danakj, 2013-02-21 01:33:15]

My brain fell over somewhere in wm/shell, so some thoughts up to there.

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_test_window.h
File wm/foreign_test_window.h (right):

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_test_window.h#...
wm/foreign_test_window.h:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
2013 for all everything now

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window.cc
File wm/foreign_window.cc (right):

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window.cc#newc...
wm/foreign_window.cc:31: const gfx::Size& a_preferred_size)
by value

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window.cc#newc...
wm/foreign_window.cc:63: AddRef();  // Balanced in DeleteDelegate();
T_T

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window.cc#newc...
wm/foreign_window.cc:67: views::ClientView*
ForeignWindow::CreateClientView(views::Widget* widget) {
can you give |widget| a more descriptive name? is it just the parent_widget? or
is its purpose/meaning something else?

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window.h
File wm/foreign_window.h (right):

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window.h#newco...
wm/foreign_window.h:30: CreateParams(gfx::PluginWindowHandle window_handle,
let's make a new constant instead of reusing the plugin thing.
wm::ForeignWindowHandle?

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window.h#newco...
wm/foreign_window.h:31: const gfx::Size& preferred_size);
by value

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window.h#newco...
wm/foreign_window.h:46: virtual ForeignWindow* AsForeignWindow() OVERRIDE;
Seems like this is a backdoor to more ForeignWindow methods. Why not just
include those methods in the delegate?

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window.h#newco...
wm/foreign_window.h:66: // Set plugin window properties.
These don't change the plugin window right? They are making the view match the
plugin?  This sounds like the opposite though.

This class is basically a proxy between the foreign window (the host) and the
native window (the client view) right?

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window.h#newco...
wm/foreign_window.h:70: // Call when plugin window has been destroyed.
"Called"? Or who should call?

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window_client_...
File wm/foreign_window_client_view.cc (right):

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window_client_...
wm/foreign_window_client_view.cc:124: window_->SetExternalTexture(NULL);
how come this doesn't set texture_ to NULL as well?

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window_client_...
wm/foreign_window_client_view.cc:166: texture_ = NULL;
how come this doesn't call window->SetExternalTexture(NULL)?

should we have a ClearTexture() helper than does both of these?

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window_client_...
File wm/foreign_window_client_view.h (right):

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window_client_...
wm/foreign_window_client_view.h:34: void SetWindowSize(const gfx::Size& size);
there's a lot of windows. can we always prefix with Native or Foreign for
clarity throughout this CL?

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window_unittes...
File wm/foreign_window_unittest.cc (right):

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window_unittes...
wm/foreign_window_unittest.cc:14: namespace test {
why not anonymous for the whole file?

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window_unittes...
wm/foreign_window_unittest.cc:43: EXPECT_EQ(2lu, tracker.windows().size());
is 2u not enough?

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window_unittes...
wm/foreign_window_unittest.cc:54: TEST_F(ForeignWindowTest, IsVisible) {
can you stick a comment on/in each of these tests to say what they are testing?

I think this is testing that the native window is shown/hidden when the foreign
window is shown/hidden?

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window_widget.cc
File wm/foreign_window_widget.cc (right):

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window_widget....
wm/foreign_window_widget.cc:22: params.context =
ash::Shell::GetAllRootWindows()[0];
this is going to break on dual-head isn't it? shall we put a TODO here? or can
we get the root from the ForeignWindow*?

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window_widget....
wm/foreign_window_widget.cc:26: SetPersistsAcrossAllWorkspaces(
curious, what happens if we don't set persists across all?

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window_widget.h
File wm/foreign_window_widget.h (right):

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window_widget....
wm/foreign_window_widget.h:23: scoped_refptr<ForeignWindow> window_;
foreign_window_

https://codereview.chromium.org/11485006/diff/21001/wm/gpu/DEPS
File wm/gpu/DEPS (right):

https://codereview.chromium.org/11485006/diff/21001/wm/gpu/DEPS#newcode3
wm/gpu/DEPS:3: "+third_party/WebKit",
third_party/WebKit/Source/Platform/chromium/WebGraphicsContext3D.h ?

https://codereview.chromium.org/11485006/diff/21001/wm/gpu/foreign_window_tex...
File wm/gpu/foreign_window_texture_factory.cc (right):

https://codereview.chromium.org/11485006/diff/21001/wm/gpu/foreign_window_tex...
wm/gpu/foreign_window_texture_factory.cc:72: int32 image_id) :
ui::Texture(flipped, size, device_scale_factor),
shouldn't : be on next line?

https://codereview.chromium.org/11485006/diff/21001/wm/gpu/foreign_window_tex...
wm/gpu/foreign_window_texture_factory.cc:119:
host_context_->deleteTexture(texture_id_);
is there some benefit to doing this when it's lost anyway?

https://codereview.chromium.org/11485006/diff/21001/wm/gpu/foreign_window_tex...
File wm/gpu/foreign_window_texture_factory.h (right):

https://codereview.chromium.org/11485006/diff/21001/wm/gpu/foreign_window_tex...
wm/gpu/foreign_window_texture_factory.h:66: // This class provides the interface
for creating textures boudn to native
"bound"

bound to a foreign window, no?

https://codereview.chromium.org/11485006/diff/21001/wm/host/foreign_test_wind...
File wm/host/foreign_test_window_host.cc (right):

https://codereview.chromium.org/11485006/diff/21001/wm/host/foreign_test_wind...
wm/host/foreign_test_window_host.cc:22: virtual void Destroy() OVERRIDE {}
Should this "delete this"?

https://codereview.chromium.org/11485006/diff/21001/wm/host/foreign_test_wind...
File wm/host/foreign_test_window_host_linux.cc (right):

https://codereview.chromium.org/11485006/diff/21001/wm/host/foreign_test_wind...
wm/host/foreign_test_window_host_linux.cc:13: gfx::AcceleratedWidget parent) :
display_(NULL),
is this : on the same line thing legit style? never saw that before this cl

https://codereview.chromium.org/11485006/diff/21001/wm/host/foreign_test_wind...
wm/host/foreign_test_window_host_linux.cc:118: gfx::AcceleratedWidget parent) {
Why is an X window sometimes AcceleratedWidget and sometimes
PluginWindowThinger?

https://codereview.chromium.org/11485006/diff/21001/wm/host/foreign_test_wind...
File wm/host/foreign_test_window_host_linux.h (right):

https://codereview.chromium.org/11485006/diff/21001/wm/host/foreign_test_wind...
wm/host/foreign_test_window_host_linux.h:26: // Overridden from
ForeignTestWindowHost:
s/:/./

https://codereview.chromium.org/11485006/diff/21001/wm/host/foreign_test_wind...
wm/host/foreign_test_window_host_linux.h:34: // Overridden from
MessageLoopForIO::Watcher:
s/:$/./

https://codereview.chromium.org/11485006/diff/21001/wm/host/root_window_host_...
File wm/host/root_window_host_linux.cc (right):

https://codereview.chromium.org/11485006/diff/21001/wm/host/root_window_host_...
wm/host/root_window_host_linux.cc:215: gfx::Point
GetTargetOriginInRootWindow(const aura::Window* window) {
GetTargetOffset.. and return a Vector2d? Then GetTargetBounds would be like
return gfx::Rect(size..) + GetTargetOrigin();

I think that more closely matches where the ui interfaces will be going.

https://codereview.chromium.org/11485006/diff/21001/wm/shell/content_client/s...
File wm/shell/content_client/shell_browser_main_parts.h (right):

https://codereview.chromium.org/11485006/diff/21001/wm/shell/content_client/s...
wm/shell/content_client/shell_browser_main_parts.h:48:
scoped_ptr<wm::ForeignTestWindow> foreign_test_window_;
Is this class only used for tests? Should it have Test in its name?

Or can this live in a test/ version of this class?

[reveman, 2013-02-22 01:26:44]

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_test_window.h
File wm/foreign_test_window.h (right):

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_test_window.h#...
wm/foreign_test_window.h:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
On 2013/02/21 01:33:15, danakj wrote:
> 2013 for all everything now

Done.

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window.cc
File wm/foreign_window.cc (right):

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window.cc#newc...
wm/foreign_window.cc:67: views::ClientView*
ForeignWindow::CreateClientView(views::Widget* widget) {
On 2013/02/21 01:33:15, danakj wrote:
> can you give |widget| a more descriptive name? is it just the parent_widget?
or
> is its purpose/meaning something else?

This is the implementation of WidgetDelegate. |widget| is the widget represeting
the foreign window. I prefer to have the name of this argument match the
WidgetDelegate declaration.

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window.h
File wm/foreign_window.h (right):

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window.h#newco...
wm/foreign_window.h:30: CreateParams(gfx::PluginWindowHandle window_handle,
On 2013/02/21 01:33:15, danakj wrote:
> let's make a new constant instead of reusing the plugin thing.
> wm::ForeignWindowHandle?

I made this change but decided to revert it. ForeignWindowHandle has to live in
wm/gpu/foreign_window_texture_factory.h as that code depends on it being a
gfx::PluginWindowHandle and I'm not sure I feel like having all code using this
handle depend on that. Long term I think gfx::PluginWindowHandle should change
name throughout all of chrome. Let me think a bit more about this.

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window.h#newco...
wm/foreign_window.h:31: const gfx::Size& preferred_size);
On 2013/02/21 01:33:15, danakj wrote:
> by value

what's the rational for this?

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window.h#newco...
wm/foreign_window.h:46: virtual ForeignWindow* AsForeignWindow() OVERRIDE;
On 2013/02/21 01:33:15, danakj wrote:
> Seems like this is a backdoor to more ForeignWindow methods. Why not just
> include those methods in the delegate?

Removed it as not used.

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window.h#newco...
wm/foreign_window.h:66: // Set plugin window properties.
On 2013/02/21 01:33:15, danakj wrote:
> These don't change the plugin window right? They are making the view match the
> plugin?  This sounds like the opposite though.
> 
> This class is basically a proxy between the foreign window (the host) and the
> native window (the client view) right?

Yes, these functions are called as a result of the foreign window changing. I've
changed this into OnWindow*() in latest patch. Let me know what you think.

A ForeignWindow instance represents an X11 window and the wm component can
safely control its lifetime. The view system controls the lifetime of the client
view. ForeignWindow will proxy changes to the client view while it's still in
existence. This kind of layering might seem unnecessary but it's very useful for
having a system where having full control of ForeignWindow lifetime is critical.

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window.h#newco...
wm/foreign_window.h:70: // Call when plugin window has been destroyed.
On 2013/02/21 01:33:15, danakj wrote:
> "Called"? Or who should call?

This is similar to the above functions. I changed above function names and kept
the name of this.

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window_client_...
File wm/foreign_window_client_view.cc (right):

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window_client_...
wm/foreign_window_client_view.cc:124: window_->SetExternalTexture(NULL);
On 2013/02/21 01:33:15, danakj wrote:
> how come this doesn't set texture_ to NULL as well?

we could but it's not necessary. not doing this is somewhat useful in respect to
validating the context lost behavior of ForeignWindowTextureFactory. we should
probably add a unit test to verify this.

added a comment to the code.

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window_client_...
wm/foreign_window_client_view.cc:166: texture_ = NULL;
On 2013/02/21 01:33:15, danakj wrote:
> how come this doesn't call window->SetExternalTexture(NULL)?
> 
> should we have a ClearTexture() helper than does both of these?

that would prevent any ash window animations that might be running past X11
window being unmapped from rendering correctly.

added a comment about this.

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window_client_...
File wm/foreign_window_client_view.h (right):

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window_client_...
wm/foreign_window_client_view.h:34: void SetWindowSize(const gfx::Size& size);
On 2013/02/21 01:33:15, danakj wrote:
> there's a lot of windows. can we always prefix with Native or Foreign for
> clarity throughout this CL?

We're always referring to the foreign window in this API. Let me know what you
think about the changes I made to these functions. If a "Foreign" prefix is
needed I can add that too.

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window_unittes...
File wm/foreign_window_unittest.cc (right):

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window_unittes...
wm/foreign_window_unittest.cc:14: namespace test {
On 2013/02/21 01:33:15, danakj wrote:
> why not anonymous for the whole file?

trying to be consistent with ash.

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window_unittes...
wm/foreign_window_unittest.cc:43: EXPECT_EQ(2lu, tracker.windows().size());
On 2013/02/21 01:33:15, danakj wrote:
> is 2u not enough?

Done.

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window_unittes...
wm/foreign_window_unittest.cc:54: TEST_F(ForeignWindowTest, IsVisible) {
On 2013/02/21 01:33:15, danakj wrote:
> can you stick a comment on/in each of these tests to say what they are
testing?
> 
> I think this is testing that the native window is shown/hidden when the
foreign
> window is shown/hidden?

Done.

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window_widget.cc
File wm/foreign_window_widget.cc (right):

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window_widget....
wm/foreign_window_widget.cc:22: params.context =
ash::Shell::GetAllRootWindows()[0];
On 2013/02/21 01:33:15, danakj wrote:
> this is going to break on dual-head isn't it? shall we put a TODO here? or can
> we get the root from the ForeignWindow*?

Yes, last time I touched this code ash::Shell::GetAllRootWindows()[0] was used
elsewhere. Added a TODO for now but we should get this fixed asap.

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window_widget....
wm/foreign_window_widget.cc:26: SetPersistsAcrossAllWorkspaces(
On 2013/02/21 01:33:15, danakj wrote:
> curious, what happens if we don't set persists across all?

you probably get parts of the screen where input doesn't work as it goes to the
foreign window. I think the way we handle input makes it relatively easy to
support this in the future though.

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window_widget.h
File wm/foreign_window_widget.h (right):

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window_widget....
wm/foreign_window_widget.h:23: scoped_refptr<ForeignWindow> window_;
On 2013/02/21 01:33:15, danakj wrote:
> foreign_window_

Done.

https://codereview.chromium.org/11485006/diff/21001/wm/gpu/DEPS
File wm/gpu/DEPS (right):

https://codereview.chromium.org/11485006/diff/21001/wm/gpu/DEPS#newcode3
wm/gpu/DEPS:3: "+third_party/WebKit",
On 2013/02/21 01:33:15, danakj wrote:
> third_party/WebKit/Source/Platform/chromium/WebGraphicsContext3D.h ?

changed to third_party/WebKit/Source/Platform/chromium as I don't see us being
more specific than that anywhere else.

https://codereview.chromium.org/11485006/diff/21001/wm/gpu/foreign_window_tex...
File wm/gpu/foreign_window_texture_factory.cc (right):

https://codereview.chromium.org/11485006/diff/21001/wm/gpu/foreign_window_tex...
wm/gpu/foreign_window_texture_factory.cc:72: int32 image_id) :
ui::Texture(flipped, size, device_scale_factor),
On 2013/02/21 01:33:15, danakj wrote:
> shouldn't : be on next line?

Done.

https://codereview.chromium.org/11485006/diff/21001/wm/gpu/foreign_window_tex...
wm/gpu/foreign_window_texture_factory.cc:119:
host_context_->deleteTexture(texture_id_);
On 2013/02/21 01:33:15, danakj wrote:
> is there some benefit to doing this when it's lost anyway?

I think it's good accounting.

https://codereview.chromium.org/11485006/diff/21001/wm/gpu/foreign_window_tex...
File wm/gpu/foreign_window_texture_factory.h (right):

https://codereview.chromium.org/11485006/diff/21001/wm/gpu/foreign_window_tex...
wm/gpu/foreign_window_texture_factory.h:66: // This class provides the interface
for creating textures boudn to native
On 2013/02/21 01:33:15, danakj wrote:
> "bound"
> 
> bound to a foreign window, no?

Done.

https://codereview.chromium.org/11485006/diff/21001/wm/host/foreign_test_wind...
File wm/host/foreign_test_window_host.cc (right):

https://codereview.chromium.org/11485006/diff/21001/wm/host/foreign_test_wind...
wm/host/foreign_test_window_host.cc:22: virtual void Destroy() OVERRIDE {}
On 2013/02/21 01:33:15, danakj wrote:
> Should this "delete this"?

Done. Nice catch. Should be in Delete() though.

https://codereview.chromium.org/11485006/diff/21001/wm/host/foreign_test_wind...
File wm/host/foreign_test_window_host_linux.cc (right):

https://codereview.chromium.org/11485006/diff/21001/wm/host/foreign_test_wind...
wm/host/foreign_test_window_host_linux.cc:13: gfx::AcceleratedWidget parent) :
display_(NULL),
On 2013/02/21 01:33:15, danakj wrote:
> is this : on the same line thing legit style? never saw that before this cl

I like this when possible as it creates a nice separation between parameters and
the initializer list but maybe not legit so changing..

https://codereview.chromium.org/11485006/diff/21001/wm/host/foreign_test_wind...
wm/host/foreign_test_window_host_linux.cc:118: gfx::AcceleratedWidget parent) {
On 2013/02/21 01:33:15, danakj wrote:
> Why is an X window sometimes AcceleratedWidget and sometimes
> PluginWindowThinger?

RootWindowHost uses AcceleratedWidget and BrowserGpuChannelHostFactory uses
gfx::PluginWindowHandle. Platform specific code is needed to bridge these.

https://codereview.chromium.org/11485006/diff/21001/wm/host/foreign_test_wind...
File wm/host/foreign_test_window_host_linux.h (right):

https://codereview.chromium.org/11485006/diff/21001/wm/host/foreign_test_wind...
wm/host/foreign_test_window_host_linux.h:26: // Overridden from
ForeignTestWindowHost:
On 2013/02/21 01:33:15, danakj wrote:
> s/:/./

":" vs "." usage in combination with Overridden seem split about 50/50 in ash
and significantly in favor of ":" throughout all of chromium (~1100 vs ~400).
I'm making wm component consistently use ":" unless there's a good reason we
should be using "." instead?

https://codereview.chromium.org/11485006/diff/21001/wm/host/root_window_host_...
File wm/host/root_window_host_linux.cc (right):

https://codereview.chromium.org/11485006/diff/21001/wm/host/root_window_host_...
wm/host/root_window_host_linux.cc:215: gfx::Point
GetTargetOriginInRootWindow(const aura::Window* window) {
On 2013/02/21 01:33:15, danakj wrote:
> GetTargetOffset.. and return a Vector2d? Then GetTargetBounds would be like
> return gfx::Rect(size..) + GetTargetOrigin();
> 
> I think that more closely matches where the ui interfaces will be going.

Sg, I'll get this fixed soon or maybe if it's not critical you can change this
after the patch lands? ;-)

https://codereview.chromium.org/11485006/diff/21001/wm/shell/content_client/s...
File wm/shell/content_client/shell_browser_main_parts.h (right):

https://codereview.chromium.org/11485006/diff/21001/wm/shell/content_client/s...
wm/shell/content_client/shell_browser_main_parts.h:48:
scoped_ptr<wm::ForeignTestWindow> foreign_test_window_;
On 2013/02/21 01:33:15, danakj wrote:
> Is this class only used for tests? Should it have Test in its name?
> 
> Or can this live in a test/ version of this class?

Not sure what you're referring to. ShellBrowserMainParts is used for the test
shell. Naming should be consistent with ash. ForeignTestWindow is used for both
unit tests and the test shell.

[danakj, 2013-02-22 01:48:08]

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window.h
File wm/foreign_window.h (right):

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window.h#newco...
wm/foreign_window.h:31: const gfx::Size& preferred_size);
On 2013/02/22 01:26:44, David Reveman wrote:
> On 2013/02/21 01:33:15, danakj wrote:
> > by value
> 
> what's the rational for this?

Classes that consist entirely of <= 4 ints, or <= 2 floats are faster/produce
less code to pass by value. We've started consistently doing this in cc:: for
gfx::Rect/Size/Point/SizeF/PointF now.

https://codereview.chromium.org/11485006/diff/21001/wm/host/foreign_test_wind...
File wm/host/foreign_test_window_host_linux.cc (right):

https://codereview.chromium.org/11485006/diff/21001/wm/host/foreign_test_wind...
wm/host/foreign_test_window_host_linux.cc:118: gfx::AcceleratedWidget parent) {
On 2013/02/22 01:26:44, David Reveman wrote:
> On 2013/02/21 01:33:15, danakj wrote:
> > Why is an X window sometimes AcceleratedWidget and sometimes
> > PluginWindowThinger?
> 
> RootWindowHost uses AcceleratedWidget and BrowserGpuChannelHostFactory uses
> gfx::PluginWindowHandle. Platform specific code is needed to bridge these.

Oh argh. Do you think BGCHF could use AcceleratedWidget in the future, or is it
incompatible on some platforms?

https://codereview.chromium.org/11485006/diff/21001/wm/host/foreign_test_wind...
File wm/host/foreign_test_window_host_linux.h (right):

https://codereview.chromium.org/11485006/diff/21001/wm/host/foreign_test_wind...
wm/host/foreign_test_window_host_linux.h:26: // Overridden from
ForeignTestWindowHost:
On 2013/02/22 01:26:44, David Reveman wrote:
> On 2013/02/21 01:33:15, danakj wrote:
> > s/:/./
> 
> ":" vs "." usage in combination with Overridden seem split about 50/50 in ash
> and significantly in favor of ":" throughout all of chromium (~1100 vs ~400).
> I'm making wm component consistently use ":" unless there's a good reason we
> should be using "." instead?

Oh, I thought I saw you doing "." earlier in the CL which is the only reason I
said anthing. Never mind then!

https://codereview.chromium.org/11485006/diff/21001/wm/host/root_window_host_...
File wm/host/root_window_host_linux.cc (right):

https://codereview.chromium.org/11485006/diff/21001/wm/host/root_window_host_...
wm/host/root_window_host_linux.cc:215: gfx::Point
GetTargetOriginInRootWindow(const aura::Window* window) {
On 2013/02/22 01:26:44, David Reveman wrote:
> On 2013/02/21 01:33:15, danakj wrote:
> > GetTargetOffset.. and return a Vector2d? Then GetTargetBounds would be like
> > return gfx::Rect(size..) + GetTargetOrigin();
> > 
> > I think that more closely matches where the ui interfaces will be going.
> 
> Sg, I'll get this fixed soon or maybe if it's not critical you can change this
> after the patch lands? ;-)

We can do it after, it should be easy to do either way as it's just a static
method here. If not now, then please stick a TODO so we find it again later :)

https://codereview.chromium.org/11485006/diff/21001/wm/shell/content_client/s...
File wm/shell/content_client/shell_browser_main_parts.h (right):

https://codereview.chromium.org/11485006/diff/21001/wm/shell/content_client/s...
wm/shell/content_client/shell_browser_main_parts.h:48:
scoped_ptr<wm::ForeignTestWindow> foreign_test_window_;
On 2013/02/22 01:26:44, David Reveman wrote:
> On 2013/02/21 01:33:15, danakj wrote:
> > Is this class only used for tests? Should it have Test in its name?
> > 
> > Or can this live in a test/ version of this class?
> 
> Not sure what you're referring to. ShellBrowserMainParts is used for the test
> shell. Naming should be consistent with ash. ForeignTestWindow is used for
both
> unit tests and the test shell.

Oh.. right I forgot about test shell.

[reveman, 2013-02-22 06:53:01]

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window.h
File wm/foreign_window.h (right):

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window.h#newco...
wm/foreign_window.h:31: const gfx::Size& preferred_size);
On 2013/02/22 01:48:08, danakj wrote:
> On 2013/02/22 01:26:44, David Reveman wrote:
> > On 2013/02/21 01:33:15, danakj wrote:
> > > by value
> > 
> > what's the rational for this?
> 
> Classes that consist entirely of <= 4 ints, or <= 2 floats are faster/produce
> less code to pass by value. We've started consistently doing this in cc:: for
> gfx::Rect/Size/Point/SizeF/PointF now.

Done.

https://codereview.chromium.org/11485006/diff/21001/wm/host/foreign_test_wind...
File wm/host/foreign_test_window_host_linux.cc (right):

https://codereview.chromium.org/11485006/diff/21001/wm/host/foreign_test_wind...
wm/host/foreign_test_window_host_linux.cc:118: gfx::AcceleratedWidget parent) {
On 2013/02/22 01:48:08, danakj wrote:
> On 2013/02/22 01:26:44, David Reveman wrote:
> > On 2013/02/21 01:33:15, danakj wrote:
> > > Why is an X window sometimes AcceleratedWidget and sometimes
> > > PluginWindowThinger?
> > 
> > RootWindowHost uses AcceleratedWidget and BrowserGpuChannelHostFactory uses
> > gfx::PluginWindowHandle. Platform specific code is needed to bridge these.
> 
> Oh argh. Do you think BGCHF could use AcceleratedWidget in the future, or is
it
> incompatible on some platforms?

I tried that initially when adding CHROMIUM_texture_from_image but it requires
platform specific code on the browser side and is not very pretty. Handling this
in wm::ForeignTestWindowHostLinux and wm::RootWindowHostLinux is much better. 

We could always use aura::RootWindow* here instead of gfx::AcceleratedWidget..

https://codereview.chromium.org/11485006/diff/21001/wm/host/foreign_test_wind...
File wm/host/foreign_test_window_host_linux.h (right):

https://codereview.chromium.org/11485006/diff/21001/wm/host/foreign_test_wind...
wm/host/foreign_test_window_host_linux.h:26: // Overridden from
ForeignTestWindowHost:
On 2013/02/22 01:48:08, danakj wrote:
> On 2013/02/22 01:26:44, David Reveman wrote:
> > On 2013/02/21 01:33:15, danakj wrote:
> > > s/:/./
> > 
> > ":" vs "." usage in combination with Overridden seem split about 50/50 in
ash
> > and significantly in favor of ":" throughout all of chromium (~1100 vs
~400).
> > I'm making wm component consistently use ":" unless there's a good reason we
> > should be using "." instead?
> 
> Oh, I thought I saw you doing "." earlier in the CL which is the only reason I
> said anthing. Never mind then!

yea, think I had a "." somewhere but it should now be consistent.

https://codereview.chromium.org/11485006/diff/21001/wm/host/root_window_host_...
File wm/host/root_window_host_linux.cc (right):

https://codereview.chromium.org/11485006/diff/21001/wm/host/root_window_host_...
wm/host/root_window_host_linux.cc:215: gfx::Point
GetTargetOriginInRootWindow(const aura::Window* window) {
On 2013/02/22 01:48:08, danakj wrote:
> On 2013/02/22 01:26:44, David Reveman wrote:
> > On 2013/02/21 01:33:15, danakj wrote:
> > > GetTargetOffset.. and return a Vector2d? Then GetTargetBounds would be
like
> > > return gfx::Rect(size..) + GetTargetOrigin();
> > > 
> > > I think that more closely matches where the ui interfaces will be going.
> > 
> > Sg, I'll get this fixed soon or maybe if it's not critical you can change
this
> > after the patch lands? ;-)
> 
> We can do it after, it should be easy to do either way as it's just a static
> method here. If not now, then please stick a TODO so we find it again later :)

Got this done. Please verify that my changes to RecursiveConfigure looks right.

[danakj, 2013-02-22 21:42:59]

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window.cc
File wm/foreign_window.cc (right):

https://codereview.chromium.org/11485006/diff/21001/wm/foreign_window.cc#newc...
wm/foreign_window.cc:67: views::ClientView*
ForeignWindow::CreateClientView(views::Widget* widget) {
On 2013/02/22 01:26:44, David Reveman wrote:
> On 2013/02/21 01:33:15, danakj wrote:
> > can you give |widget| a more descriptive name? is it just the parent_widget?
> or
> > is its purpose/meaning something else?
> 
> This is the implementation of WidgetDelegate. |widget| is the widget
represeting
> the foreign window. I prefer to have the name of this argument match the
> WidgetDelegate declaration.  

I see, okay. I saw the AddChild() and was unsure what the relationship here was.

https://codereview.chromium.org/11485006/diff/21001/wm/host/foreign_test_wind...
File wm/host/foreign_test_window_host_linux.cc (right):

https://codereview.chromium.org/11485006/diff/21001/wm/host/foreign_test_wind...
wm/host/foreign_test_window_host_linux.cc:118: gfx::AcceleratedWidget parent) {
On 2013/02/22 06:53:01, David Reveman wrote:
> On 2013/02/22 01:48:08, danakj wrote:
> > On 2013/02/22 01:26:44, David Reveman wrote:
> > > On 2013/02/21 01:33:15, danakj wrote:
> > > > Why is an X window sometimes AcceleratedWidget and sometimes
> > > > PluginWindowThinger?
> > > 
> > > RootWindowHost uses AcceleratedWidget and BrowserGpuChannelHostFactory
uses
> > > gfx::PluginWindowHandle. Platform specific code is needed to bridge these.
> > 
> > Oh argh. Do you think BGCHF could use AcceleratedWidget in the future, or is
> it
> > incompatible on some platforms?
> 
> I tried that initially when adding CHROMIUM_texture_from_image but it requires
> platform specific code on the browser side and is not very pretty. Handling
this
> in wm::ForeignTestWindowHostLinux and wm::RootWindowHostLinux is much better. 
> 
> We could always use aura::RootWindow* here instead of gfx::AcceleratedWidget..

I think I would prefer that. Keeping the AcceleratedWidget stuff as contained as
possible seems positive.

https://codereview.chromium.org/11485006/diff/29003/wm/foreign_window.h
File wm/foreign_window.h (right):

https://codereview.chromium.org/11485006/diff/29003/wm/foreign_window.h#newco...
wm/foreign_window.h:79: void OnWindowSizeChanged(const gfx::Size& size);
Ya this is much better, thanks.

https://codereview.chromium.org/11485006/diff/29003/wm/foreign_window_client_...
File wm/foreign_window_client_view.cc (right):

https://codereview.chromium.org/11485006/diff/29003/wm/foreign_window_client_...
wm/foreign_window_client_view.cc:168: // texture for |window_| as that would
prevent the compositor from
Thanks this is very helpful.

https://codereview.chromium.org/11485006/diff/29003/wm/foreign_window_client_...
File wm/foreign_window_client_view.h (right):

https://codereview.chromium.org/11485006/diff/29003/wm/foreign_window_client_...
wm/foreign_window_client_view.h:34: void OnWindowContentsChanged();
Ok this is better, thanks for clarifying with the comments and API changes.

https://codereview.chromium.org/11485006/diff/29003/wm/host/root_window_host_...
File wm/host/root_window_host_linux.cc (right):

https://codereview.chromium.org/11485006/diff/29003/wm/host/root_window_host_...
wm/host/root_window_host_linux.cc:228: GetTargetOffsetInRootWindow(window);
Yep looks good!

https://codereview.chromium.org/11485006/diff/29003/wm/test/test_shell_delega...
File wm/test/test_shell_delegate.h (left):

https://codereview.chromium.org/11485006/diff/29003/wm/test/test_shell_delega...
wm/test/test_shell_delegate.h:84: bool screen_magnifier_enabled_;
Looks like this file changed a bunch since you made a copy? Since we refresh it?
Can do it after if you like.

(Or did you remove stuff here on purpose? How come?)

https://codereview.chromium.org/11485006/diff/29003/wm/test/wm_test_base.cc
File wm/test/wm_test_base.cc (right):

https://codereview.chromium.org/11485006/diff/29003/wm/test/wm_test_base.cc#n...
wm/test/wm_test_base.cc:68: #if !defined(OS_MACOSX)
Why this?

https://codereview.chromium.org/11485006/diff/29003/wm/test/wm_unittests.cc
File wm/test/wm_unittests.cc (right):

https://codereview.chromium.org/11485006/diff/29003/wm/test/wm_unittests.cc#n...
wm/test/wm_unittests.cc:1: // Copyright (c) 2011 The Chromium Authors. All
rights reserved.
2013

[reveman, 2013-02-23 00:16:28]

https://codereview.chromium.org/11485006/diff/29003/wm/test/test_shell_delega...
File wm/test/test_shell_delegate.h (left):

https://codereview.chromium.org/11485006/diff/29003/wm/test/test_shell_delega...
wm/test/test_shell_delegate.h:84: bool screen_magnifier_enabled_;
On 2013/02/22 21:42:59, danakj wrote:
> Looks like this file changed a bunch since you made a copy? Since we refresh
it?
> Can do it after if you like.
> 
> (Or did you remove stuff here on purpose? How come?)

Yes, none of this is used by current wm unit tests and I figured it makes more
sense to not include it until we need to.

https://codereview.chromium.org/11485006/diff/29003/wm/test/wm_test_base.cc
File wm/test/wm_test_base.cc (right):

https://codereview.chromium.org/11485006/diff/29003/wm/test/wm_test_base.cc#n...
wm/test/wm_test_base.cc:68: #if !defined(OS_MACOSX)
On 2013/02/22 21:42:59, danakj wrote:
> Why this?

It was here to be consistent with ash_test_base.cc but as
ForeignTestWindowHost::RunAllPendingInMessageLoop is platform specific it's much
cleaner to handle it there in our case. Remove the ifdef.

https://codereview.chromium.org/11485006/diff/29003/wm/test/wm_unittests.cc
File wm/test/wm_unittests.cc (right):

https://codereview.chromium.org/11485006/diff/29003/wm/test/wm_unittests.cc#n...
wm/test/wm_unittests.cc:1: // Copyright (c) 2011 The Chromium Authors. All
rights reserved.
On 2013/02/22 21:42:59, danakj wrote:
> 2013

Done.

[danakj, 2013-02-23 00:56:42]

LGTM! But can we stick a TODO on that AcceleratedWidget to make it into a
RootWindow*?

[reveman, 2013-02-25 02:52:57]

On 2013/02/23 00:56:42, danakj wrote:
> LGTM! But can we stick a TODO on that AcceleratedWidget to make it into a
> RootWindow*?

Made it a RootWindow* in latest patch and pushed the AcceleratedWidget usage to
platform specific code.

[reveman, 2013-02-25 03:04:16]

Hey,

this patch adds a window manager component to chromium. This wm component uses
ash to implement platform window management and it essentially allows you to use
ash instead of unity on Ubuntu to manage standard Linux application windows.

While X11 on Linux might be the only platform we'll ever support, the code is
structured in a way that keeps platform specific code clearly separated from the
rest of the logic. All platform specific code lives in 'wm/host/'. This
directory contains a modified implementation of aura::RootWindowHostLinux. The
plan is to later re-factor RWHL so that we can minimize the current code
duplication but I prefer to wait with this until we've implemented enough wm
support to know more precisely what's needed. People can of course modify
aura::RWHL or any part of ash without having to worry about keeping this code
working or in sync.

This initial version of the wm component has minimal functionality and is aimed
at getting the testing framework in place. It consists of a test shell and a set
of unit tests (wm_shell and wm_unittests). The 'use_wm' build flag enables
building of the wm component. No code outside of the top level wm/ directory is
affected by this flag at the moment.

David

[Ben Goodger (Google), 2013-02-25 03:34:23]

I don't think we should host the X11 WM part in Chromium. I've been OK with
refactorings within Aura/Ash that support this where they have made sense
in general, but I don't think we want to add this code that is not part of
any of our shipping products to a codebase that all Chromium developers
have to work with and support.

My suggestion would be to create a separate Google Code project for the X11
WM parts and a DEPS file that pulls the necessary bits from chromium.

-Ben

[google-reveman, 2013-02-25 19:32:49]

Seems reasonable to keep this as a separate project for now if possible.
The current test shell only code should be easy to maintain in a separate
repo but this window management support doesn't really shine until built
into the browser. All needed on top of the current patch for this to work
is a one line change to ChromeShellDelegate. Not sure how to best do this
when the code lives in a separate repo so any advice here would
be appreciated.


On Sun, Feb 24, 2013 at 7:34 PM, Ben Goodger (Google) <ben@chromium.org>wrote:

> I don't think we should host the X11 WM part in Chromium. I've been OK
> with refactorings within Aura/Ash that support this where they have made
> sense in general, but I don't think we want to add this code that is not
> part of any of our shipping products to a codebase that all Chromium
> developers have to work with and support.
>
> My suggestion would be to create a separate Google Code project for the
> X11 WM parts and a DEPS file that pulls the necessary bits from chromium.
>
> -Ben
>

[Ben Goodger (Google), 2013-02-25 20:47:14]

What's the ChromeShellDelegate change needed?

-Ben


On Mon, Feb 25, 2013 at 11:32 AM, David Reveman <reveman@google.com> wrote:

> Seems reasonable to keep this as a separate project for now if possible.
> The current test shell only code should be easy to maintain in a separate
> repo but this window management support doesn't really shine until built
> into the browser. All needed on top of the current patch for this to work
> is a one line change to ChromeShellDelegate. Not sure how to best do this
> when the code lives in a separate repo so any advice here would
> be appreciated.
>
>
> On Sun, Feb 24, 2013 at 7:34 PM, Ben Goodger (Google)
<ben@chromium.org>wrote:
>
>> I don't think we should host the X11 WM part in Chromium. I've been OK
>> with refactorings within Aura/Ash that support this where they have made
>> sense in general, but I don't think we want to add this code that is not
>> part of any of our shipping products to a codebase that all Chromium
>> developers have to work with and support.
>>
>> My suggestion would be to create a separate Google Code project for the
>> X11 WM parts and a DEPS file that pulls the necessary bits from chromium.
>>
>> -Ben
>>
>
>

[google-reveman, 2013-02-25 21:35:18]

The ChromeShellDelegate change would make an ash enabled chrome build
support platform window management if possible. Which makes it possible to
render web-pages on Linux as efficiently as on ChromeOS while still being
able to use native apps.

The ChromeShellDelegate change would make CreateRootWindowHostFactory()
look something like this:

ash::RootWindowHostFactory*
ChromeShellDelegate::CreateRootWindowHostFactory() {
#ifdef USE_WM
  return wm::RootWindowHostFactory::Create();
#else
  return ash::RootWindowHostFactory::Create();
#endif
}


On Mon, Feb 25, 2013 at 12:47 PM, Ben Goodger (Google) <ben@chromium.org>wrote:

> What's the ChromeShellDelegate change needed?
>
> -Ben
>
>
> On Mon, Feb 25, 2013 at 11:32 AM, David Reveman <reveman@google.com>wrote:
>
>> Seems reasonable to keep this as a separate project for now if possible.
>> The current test shell only code should be easy to maintain in a separate
>> repo but this window management support doesn't really shine until built
>> into the browser. All needed on top of the current patch for this to work
>> is a one line change to ChromeShellDelegate. Not sure how to best do this
>> when the code lives in a separate repo so any advice here would
>> be appreciated.
>>
>>
>> On Sun, Feb 24, 2013 at 7:34 PM, Ben Goodger (Google)
<ben@chromium.org>wrote:
>>
>>> I don't think we should host the X11 WM part in Chromium. I've been OK
>>> with refactorings within Aura/Ash that support this where they have made
>>> sense in general, but I don't think we want to add this code that is not
>>> part of any of our shipping products to a codebase that all Chromium
>>> developers have to work with and support.
>>>
>>> My suggestion would be to create a separate Google Code project for the
>>> X11 WM parts and a DEPS file that pulls the necessary bits from chromium.
>>>
>>> -Ben
>>>
>>
>>
>

[Ben Goodger (Google), 2013-02-26 00:13:54]

this seems a pretty trivial patch, seems like you could just keep it in
your tree and apply it as part of the build process? a bit ugly, but have
you heard of megapatch? :-P

-Ben


On Mon, Feb 25, 2013 at 1:35 PM, David Reveman <reveman@google.com> wrote:

> The ChromeShellDelegate change would make an ash enabled chrome build
> support platform window management if possible. Which makes it possible to
> render web-pages on Linux as efficiently as on ChromeOS while still being
> able to use native apps.
>
> The ChromeShellDelegate change would make CreateRootWindowHostFactory()
> look something like this:
>
> ash::RootWindowHostFactory*
> ChromeShellDelegate::CreateRootWindowHostFactory() {
> #ifdef USE_WM
>   return wm::RootWindowHostFactory::Create();
> #else
>   return ash::RootWindowHostFactory::Create();
> #endif
> }
>
>
> On Mon, Feb 25, 2013 at 12:47 PM, Ben Goodger (Google)
<ben@chromium.org>wrote:
>
>> What's the ChromeShellDelegate change needed?
>>
>> -Ben
>>
>>
>> On Mon, Feb 25, 2013 at 11:32 AM, David Reveman <reveman@google.com>wrote:
>>
>>> Seems reasonable to keep this as a separate project for now if possible.
>>> The current test shell only code should be easy to maintain in a separate
>>> repo but this window management support doesn't really shine until built
>>> into the browser. All needed on top of the current patch for this to work
>>> is a one line change to ChromeShellDelegate. Not sure how to best do this
>>> when the code lives in a separate repo so any advice here would
>>> be appreciated.
>>>
>>>
>>> On Sun, Feb 24, 2013 at 7:34 PM, Ben Goodger (Google)
<ben@chromium.org>wrote:
>>>
>>>> I don't think we should host the X11 WM part in Chromium. I've been OK
>>>> with refactorings within Aura/Ash that support this where they have made
>>>> sense in general, but I don't think we want to add this code that is not
>>>> part of any of our shipping products to a codebase that all Chromium
>>>> developers have to work with and support.
>>>>
>>>> My suggestion would be to create a separate Google Code project for the
>>>> X11 WM parts and a DEPS file that pulls the necessary bits from chromium.
>>>>
>>>> -Ben
>>>>
>>>
>>>
>>
>

[google-reveman, 2013-02-26 00:43:09]

yeah, I was just thinking we could do something ugly like that for now.
I'll try to get this project set up later this week.


On Mon, Feb 25, 2013 at 4:13 PM, Ben Goodger (Google) <ben@chromium.org>wrote:

> this seems a pretty trivial patch, seems like you could just keep it in
> your tree and apply it as part of the build process? a bit ugly, but have
> you heard of megapatch? :-P
>
> -Ben
>
>
> On Mon, Feb 25, 2013 at 1:35 PM, David Reveman <reveman@google.com> wrote:
>
>> The ChromeShellDelegate change would make an ash enabled chrome build
>> support platform window management if possible. Which makes it possible to
>> render web-pages on Linux as efficiently as on ChromeOS while still being
>> able to use native apps.
>>
>> The ChromeShellDelegate change would make CreateRootWindowHostFactory()
>> look something like this:
>>
>> ash::RootWindowHostFactory*
>> ChromeShellDelegate::CreateRootWindowHostFactory() {
>> #ifdef USE_WM
>>   return wm::RootWindowHostFactory::Create();
>> #else
>>   return ash::RootWindowHostFactory::Create();
>> #endif
>> }
>>
>>
>> On Mon, Feb 25, 2013 at 12:47 PM, Ben Goodger (Google)
<ben@chromium.org>wrote:
>>
>>> What's the ChromeShellDelegate change needed?
>>>
>>> -Ben
>>>
>>>
>>> On Mon, Feb 25, 2013 at 11:32 AM, David Reveman <reveman@google.com>wrote:
>>>
>>>> Seems reasonable to keep this as a separate project for now if
>>>> possible. The current test shell only code should be easy to maintain in a
>>>> separate repo but this window management support doesn't really shine
until
>>>> built into the browser. All needed on top of the current patch for this to
>>>> work is a one line change to ChromeShellDelegate. Not sure how to best do
>>>> this when the code lives in a separate repo so any advice here would
>>>> be appreciated.
>>>>
>>>>
>>>> On Sun, Feb 24, 2013 at 7:34 PM, Ben Goodger (Google) <ben@chromium.org
>>>> > wrote:
>>>>
>>>>> I don't think we should host the X11 WM part in Chromium. I've been OK
>>>>> with refactorings within Aura/Ash that support this where they have made
>>>>> sense in general, but I don't think we want to add this code that is not
>>>>> part of any of our shipping products to a codebase that all Chromium
>>>>> developers have to work with and support.
>>>>>
>>>>> My suggestion would be to create a separate Google Code project for
>>>>> the X11 WM parts and a DEPS file that pulls the necessary bits from
>>>>> chromium.
>>>>>
>>>>> -Ben
>>>>>
>>>>
>>>>
>>>
>>
>

[google-reveman, 2013-02-28 07:34:29]

this is now up and running at https://code.google.com/p/chromium-wm/

I moved the export directives which are still needed to a separate cl:
https://codereview.chromium.org/12377013/


On Mon, Feb 25, 2013 at 7:43 PM, David Reveman <reveman@google.com> wrote:

> yeah, I was just thinking we could do something ugly like that for now.
> I'll try to get this project set up later this week.
>
>
> On Mon, Feb 25, 2013 at 4:13 PM, Ben Goodger (Google)
<ben@chromium.org>wrote:
>
>> this seems a pretty trivial patch, seems like you could just keep it in
>> your tree and apply it as part of the build process? a bit ugly, but have
>> you heard of megapatch? :-P
>>
>> -Ben
>>
>>
>> On Mon, Feb 25, 2013 at 1:35 PM, David Reveman <reveman@google.com>wrote:
>>
>>> The ChromeShellDelegate change would make an ash enabled chrome build
>>> support platform window management if possible. Which makes it possible to
>>> render web-pages on Linux as efficiently as on ChromeOS while still being
>>> able to use native apps.
>>>
>>> The ChromeShellDelegate change would make CreateRootWindowHostFactory()
>>> look something like this:
>>>
>>> ash::RootWindowHostFactory*
>>> ChromeShellDelegate::CreateRootWindowHostFactory() {
>>> #ifdef USE_WM
>>>   return wm::RootWindowHostFactory::Create();
>>> #else
>>>   return ash::RootWindowHostFactory::Create();
>>> #endif
>>> }
>>>
>>>
>>> On Mon, Feb 25, 2013 at 12:47 PM, Ben Goodger (Google) <ben@chromium.org
>>> > wrote:
>>>
>>>> What's the ChromeShellDelegate change needed?
>>>>
>>>> -Ben
>>>>
>>>>
>>>> On Mon, Feb 25, 2013 at 11:32 AM, David Reveman <reveman@google.com>wrote:
>>>>
>>>>> Seems reasonable to keep this as a separate project for now if
>>>>> possible. The current test shell only code should be easy to maintain in
a
>>>>> separate repo but this window management support doesn't really shine
until
>>>>> built into the browser. All needed on top of the current patch for this
to
>>>>> work is a one line change to ChromeShellDelegate. Not sure how to best do
>>>>> this when the code lives in a separate repo so any advice here would
>>>>> be appreciated.
>>>>>
>>>>>
>>>>> On Sun, Feb 24, 2013 at 7:34 PM, Ben Goodger (Google) <
>>>>> ben@chromium.org> wrote:
>>>>>
>>>>>> I don't think we should host the X11 WM part in Chromium. I've been
>>>>>> OK with refactorings within Aura/Ash that support this where they have
made
>>>>>> sense in general, but I don't think we want to add this code that is not
>>>>>> part of any of our shipping products to a codebase that all Chromium
>>>>>> developers have to work with and support.
>>>>>>
>>>>>> My suggestion would be to create a separate Google Code project for
>>>>>> the X11 WM parts and a DEPS file that pulls the necessary bits from
>>>>>> chromium.
>>>>>>
>>>>>> -Ben
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>

[Ben Goodger (Google), 2013-02-28 17:20:35]

Cool!


On Wed, Feb 27, 2013 at 11:34 PM, David Reveman <reveman@google.com> wrote:

> this is now up and running at https://code.google.com/p/chromium-wm/
>
> I moved the export directives which are still needed to a separate cl:
> https://codereview.chromium.org/12377013/
>
>
> On Mon, Feb 25, 2013 at 7:43 PM, David Reveman <reveman@google.com> wrote:
>
>> yeah, I was just thinking we could do something ugly like that for now.
>> I'll try to get this project set up later this week.
>>
>>
>> On Mon, Feb 25, 2013 at 4:13 PM, Ben Goodger (Google)
<ben@chromium.org>wrote:
>>
>>> this seems a pretty trivial patch, seems like you could just keep it in
>>> your tree and apply it as part of the build process? a bit ugly, but have
>>> you heard of megapatch? :-P
>>>
>>> -Ben
>>>
>>>
>>> On Mon, Feb 25, 2013 at 1:35 PM, David Reveman <reveman@google.com>wrote:
>>>
>>>> The ChromeShellDelegate change would make an ash enabled chrome build
>>>> support platform window management if possible. Which makes it possible to
>>>> render web-pages on Linux as efficiently as on ChromeOS while still being
>>>> able to use native apps.
>>>>
>>>> The ChromeShellDelegate change would make CreateRootWindowHostFactory()
>>>> look something like this:
>>>>
>>>> ash::RootWindowHostFactory*
>>>> ChromeShellDelegate::CreateRootWindowHostFactory() {
>>>> #ifdef USE_WM
>>>>   return wm::RootWindowHostFactory::Create();
>>>> #else
>>>>   return ash::RootWindowHostFactory::Create();
>>>> #endif
>>>> }
>>>>
>>>>
>>>> On Mon, Feb 25, 2013 at 12:47 PM, Ben Goodger (Google) <
>>>> ben@chromium.org> wrote:
>>>>
>>>>> What's the ChromeShellDelegate change needed?
>>>>>
>>>>> -Ben
>>>>>
>>>>>
>>>>> On Mon, Feb 25, 2013 at 11:32 AM, David Reveman
<reveman@google.com>wrote:
>>>>>
>>>>>> Seems reasonable to keep this as a separate project for now if
>>>>>> possible. The current test shell only code should be easy to maintain in
a
>>>>>> separate repo but this window management support doesn't really shine
until
>>>>>> built into the browser. All needed on top of the current patch for this
to
>>>>>> work is a one line change to ChromeShellDelegate. Not sure how to best
do
>>>>>> this when the code lives in a separate repo so any advice here would
>>>>>> be appreciated.
>>>>>>
>>>>>>
>>>>>> On Sun, Feb 24, 2013 at 7:34 PM, Ben Goodger (Google) <
>>>>>> ben@chromium.org> wrote:
>>>>>>
>>>>>>> I don't think we should host the X11 WM part in Chromium. I've been
>>>>>>> OK with refactorings within Aura/Ash that support this where they have
made
>>>>>>> sense in general, but I don't think we want to add this code that is
not
>>>>>>> part of any of our shipping products to a codebase that all Chromium
>>>>>>> developers have to work with and support.
>>>>>>>
>>>>>>> My suggestion would be to create a separate Google Code project for
>>>>>>> the X11 WM parts and a DEPS file that pulls the necessary bits from
>>>>>>> chromium.
>>>>>>>
>>>>>>> -Ben
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

[dshwang, 2016-01-27 09:32:18]

dongseong.hwang@intel.com changed reviewers:
+ dongseong.hwang@intel.com