make menus, bubbles, etc. top level windows on aura

make menus, bubbles, etc. top level windows on aura

Makes these windows top-level so they interact with the system better (and also maintain focus better in some cases)

Two additional delegations:
- One to notify when the animation is complete because when closing a window the root window close needs to be deferred until after the animation is complete.
- One to add extra space to the root window used during animations.

R=sky@chromium.org
BUG=175016, 175833, 179786, 180071, 181175
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190446

[sky, 2013-03-07 23:00:36]

Before I review this can you run through all the tests to make sure there
aren't any problems?


On Thu, Mar 7, 2013 at 2:57 PM, <scottmg@chromium.org> wrote:

> Reviewers: sky,
>
> Description:
> make menus, bubbles, etc. top level windows on aura
>
> Makes these windows top-level so they interact with the system better (and
> also
> maintain focus better in some cases)
>
> Two additional delegations:
> - One to notify when the animation is complete because when closing a
> window the
> root window close needs to be deferred until after the animation is
> complete.
> - One to add extra space to the root window used during animations.
>
> R=sky@chromium.org
> BUG=175833,179786,180071
>
> Please review this at
https://codereview.chromium.**org/12342028/<https://codereview.chromium.org/1...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>   M chrome/browser/ui/views/**chrome_views_delegate.cc
>   M content/browser/renderer_host/**render_widget_host_view_aura.h
>   M content/browser/renderer_host/**render_widget_host_view_aura.**cc
>   M content/browser/web_contents/**web_contents_view_aura.h
>   M content/browser/web_contents/**web_contents_view_aura.cc
>   M ui/aura/remote_root_window_**host_win.h
>   M ui/aura/remote_root_window_**host_win.cc
>   M ui/aura/root_window_host.h
>   M ui/aura/root_window_host_win.h
>   M ui/aura/root_window_host_win.**cc
>   M ui/aura/window_delegate.h
>   M ui/views/corewm/window_**animations.cc
>   M ui/views/widget/desktop_aura/**desktop_native_widget_aura.h
>   M ui/views/widget/desktop_aura/**desktop_native_widget_aura.cc
>   M ui/views/widget/desktop_aura/**desktop_root_window_host_win.h
>   M ui/views/widget/desktop_aura/**desktop_root_window_host_win.**cc
>   M ui/views/widget/native_widget_**aura.h
>   M ui/views/widget/native_widget_**aura.cc
>   M ui/views/widget/widget.cc
>
>
>

[scottmg, 2013-03-14 18:07:16]

Hey Scott, the tests are clean on this now. Could you please take a look?

[sky, 2013-03-14 20:06:37]

https://codereview.chromium.org/12342028/diff/84001/chrome/browser/ui/views/c...
File chrome/browser/ui/views/chrome_views_delegate.cc (right):

https://codereview.chromium.org/12342028/diff/84001/chrome/browser/ui/views/c...
chrome/browser/ui/views/chrome_views_delegate.cc:217: #if defined(OS_WIN) &&
defined(USE_AURA)
Why is this windows specific, don't we want it for aura non-chromeos?

Also, we shouldn't need this for HOST_DESKTOP_TYPE_ASH right?

Lastly, add a comment in the body describing why we have this logic, that is,
what the empty else does and why we want it.

https://codereview.chromium.org/12342028/diff/84001/ui/aura/root_window_host.h
File ui/aura/root_window_host.h (right):

https://codereview.chromium.org/12342028/diff/84001/ui/aura/root_window_host....
ui/aura/root_window_host.h:126: virtual void SetHostWindowExpansion(const
gfx::Rect& extra) = 0;
Add a description.

https://codereview.chromium.org/12342028/diff/84001/ui/aura/root_window_host_...
File ui/aura/root_window_host_linux.cc (right):

https://codereview.chromium.org/12342028/diff/84001/ui/aura/root_window_host_...
ui/aura/root_window_host_linux.cc:869: }
NOTIMPLEMENTED?

https://codereview.chromium.org/12342028/diff/84001/ui/views/corewm/window_an...
File ui/views/corewm/window_animations.cc (right):

https://codereview.chromium.org/12342028/diff/84001/ui/views/corewm/window_an...
ui/views/corewm/window_animations.cc:137:
window_->delegate()->OnWindowHidingAnimationCompleted();
Add a test for coverage of this.

https://codereview.chromium.org/12342028/diff/84001/ui/views/corewm/window_an...
ui/views/corewm/window_animations.cc:188: const Layer* p = layer;
This use Layer::GetTargetTransformRelativeTo (that would require making it
public).

https://codereview.chromium.org/12342028/diff/84001/ui/views/corewm/window_an...
ui/views/corewm/window_animations.cc:200: gfx::Rect
GetLayerWorldBoundsAfterTransform(ui::Layer* layer,
nit: newline between 199/200.

https://codereview.chromium.org/12342028/diff/84001/ui/views/corewm/window_an...
ui/views/corewm/window_animations.cc:239: AugmentWindowSize(window,
start_transform, end_transform);
Don't we only need to do this for some windows? For example, chromeos and
HOST_DESKTOP_TYPE_ASH never need this, right?

https://codereview.chromium.org/12342028/diff/84001/ui/views/widget/desktop_a...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.h (right):

https://codereview.chromium.org/12342028/diff/84001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_native_widget_aura.h:241: bool
pending_close_;
Description?

https://codereview.chromium.org/12342028/diff/84001/ui/views/widget/desktop_a...
File ui/views/widget/desktop_aura/desktop_root_window_host_linux.cc (right):

https://codereview.chromium.org/12342028/diff/84001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_root_window_host_linux.cc:886: }
NOTIMPLEMENTED?

https://codereview.chromium.org/12342028/diff/84001/ui/views/widget/desktop_a...
File ui/views/widget/desktop_aura/desktop_root_window_host_win.cc (right):

https://codereview.chromium.org/12342028/diff/84001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_root_window_host_win.cc:59:
window_expansion_(0, 0, 0, 0) {
nit: this is the default.

https://codereview.chromium.org/12342028/diff/84001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_root_window_host_win.cc:382: return
WidgetSizeIsClientSize() ?
What about this? Doesn't it need to change too?

https://codereview.chromium.org/12342028/diff/84001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_root_window_host_win.cc:471: 
nit: remove one of these newlines.

https://codereview.chromium.org/12342028/diff/84001/ui/views/widget/desktop_a...
File ui/views/widget/desktop_aura/desktop_root_window_host_win.h (right):

https://codereview.chromium.org/12342028/diff/84001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_root_window_host_win.h:238: gfx::Rect
window_expansion_;
Description?

[scottmg, 2013-03-14 22:51:53]

Thanks!

(I apologize, I accidentally rebased. If you're looking at the 22->23 diff, I
noted the locations that are not part of this CL)

https://codereview.chromium.org/12342028/diff/84001/chrome/browser/ui/views/c...
File chrome/browser/ui/views/chrome_views_delegate.cc (right):

https://codereview.chromium.org/12342028/diff/84001/chrome/browser/ui/views/c...
chrome/browser/ui/views/chrome_views_delegate.cc:217: #if defined(OS_WIN) &&
defined(USE_AURA)
On 2013/03/14 20:06:37, sky wrote:
> Why is this windows specific, don't we want it for aura non-chromeos?

I'm not clear on what's possible on Linux. I don't know if we can blend other
top-level windows with the main window, so I was focusing on Windows for now.

> 
> Also, we shouldn't need this for HOST_DESKTOP_TYPE_ASH right?

Done.

> 
> Lastly, add a comment in the body describing why we have this logic, that is,
> what the empty else does and why we want it.

Done.

https://codereview.chromium.org/12342028/diff/84001/ui/aura/root_window_host.h
File ui/aura/root_window_host.h (right):

https://codereview.chromium.org/12342028/diff/84001/ui/aura/root_window_host....
ui/aura/root_window_host.h:126: virtual void SetHostWindowExpansion(const
gfx::Rect& extra) = 0;
On 2013/03/14 20:06:37, sky wrote:
> Add a description.

Done.

https://codereview.chromium.org/12342028/diff/84001/ui/aura/root_window_host_...
File ui/aura/root_window_host_linux.cc (right):

https://codereview.chromium.org/12342028/diff/84001/ui/aura/root_window_host_...
ui/aura/root_window_host_linux.cc:869: }
On 2013/03/14 20:06:37, sky wrote:
> NOTIMPLEMENTED?

Done.

https://codereview.chromium.org/12342028/diff/84001/ui/views/corewm/window_an...
File ui/views/corewm/window_animations.cc (right):

https://codereview.chromium.org/12342028/diff/84001/ui/views/corewm/window_an...
ui/views/corewm/window_animations.cc:137:
window_->delegate()->OnWindowHidingAnimationCompleted();
On 2013/03/14 20:06:37, sky wrote:
> Add a test for coverage of this.

ui/views/corewm/window_animations_unittest.cc wasn't being built. :( I added it
to views_unittests where it looks like it belonged, removed the tests that
didn't compile (using ash internals) and added a test there for this delegate
notification.

https://codereview.chromium.org/12342028/diff/84001/ui/views/corewm/window_an...
ui/views/corewm/window_animations.cc:188: const Layer* p = layer;
On 2013/03/14 20:06:37, sky wrote:
> This use Layer::GetTargetTransformRelativeTo (that would require making it
> public).

Done.

https://codereview.chromium.org/12342028/diff/84001/ui/views/corewm/window_an...
ui/views/corewm/window_animations.cc:200: gfx::Rect
GetLayerWorldBoundsAfterTransform(ui::Layer* layer,
On 2013/03/14 20:06:37, sky wrote:
> nit: newline between 199/200.

Done.

https://codereview.chromium.org/12342028/diff/84001/ui/views/corewm/window_an...
ui/views/corewm/window_animations.cc:239: AugmentWindowSize(window,
start_transform, end_transform);
On 2013/03/14 20:06:37, sky wrote:
> Don't we only need to do this for some windows? For example, chromeos and
> HOST_DESKTOP_TYPE_ASH never need this, right?

That's right. It's ignored by the delegate in those cases since ui/views doesn't
know about desktop types. Maybe there's a better way to do this here though.

https://codereview.chromium.org/12342028/diff/84001/ui/views/widget/desktop_a...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.h (right):

https://codereview.chromium.org/12342028/diff/84001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_native_widget_aura.h:241: bool
pending_close_;
On 2013/03/14 20:06:37, sky wrote:
> Description?

Done.

https://codereview.chromium.org/12342028/diff/84001/ui/views/widget/desktop_a...
File ui/views/widget/desktop_aura/desktop_root_window_host_linux.cc (right):

https://codereview.chromium.org/12342028/diff/84001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_root_window_host_linux.cc:886: }
On 2013/03/14 20:06:37, sky wrote:
> NOTIMPLEMENTED?

Done.

https://codereview.chromium.org/12342028/diff/84001/ui/views/widget/desktop_a...
File ui/views/widget/desktop_aura/desktop_root_window_host_win.cc (right):

https://codereview.chromium.org/12342028/diff/84001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_root_window_host_win.cc:59:
window_expansion_(0, 0, 0, 0) {
On 2013/03/14 20:06:37, sky wrote:
> nit: this is the default.

Done.

https://codereview.chromium.org/12342028/diff/84001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_root_window_host_win.cc:382: return
WidgetSizeIsClientSize() ?
On 2013/03/14 20:06:37, sky wrote:
> What about this? Doesn't it need to change too?

Done. (I'm a bit unclear on why it didn't matter -- I guess because it's not
generally called during animation).

https://codereview.chromium.org/12342028/diff/84001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_root_window_host_win.cc:471: 
On 2013/03/14 20:06:37, sky wrote:
> nit: remove one of these newlines.

Done.

https://codereview.chromium.org/12342028/diff/84001/ui/views/widget/desktop_a...
File ui/views/widget/desktop_aura/desktop_root_window_host_win.h (right):

https://codereview.chromium.org/12342028/diff/84001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_root_window_host_win.h:238: gfx::Rect
window_expansion_;
On 2013/03/14 20:06:37, sky wrote:
> Description?

Done.

https://codereview.chromium.org/12342028/diff/92001/content/browser/web_conte...
File content/browser/web_contents/web_contents_view_aura.cc (right):

https://codereview.chromium.org/12342028/diff/92001/content/browser/web_conte...
content/browser/web_contents/web_contents_view_aura.cc:1202: return
window_->GetBoundsInScreen();
(due to rebase, not this cl)

https://codereview.chromium.org/12342028/diff/92001/ui/views/widget/desktop_a...
File ui/views/widget/desktop_aura/desktop_root_window_host_linux.cc (right):

https://codereview.chromium.org/12342028/diff/92001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_root_window_host_linux.cc:1128:
widget->GetRootView()->Layout();
(due to rebase, not this CL)

https://codereview.chromium.org/12342028/diff/92001/ui/views/widget/desktop_a...
File ui/views/widget/desktop_aura/desktop_root_window_host_win.h (right):

https://codereview.chromium.org/12342028/diff/92001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_root_window_host_win.h:156: virtual bool
ShouldHandleSystemCommands() const OVERRIDE;
(due to rebase, not this CL)

https://codereview.chromium.org/12342028/diff/92001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/12342028/diff/92001/ui/views/widget/widget.cc...
ui/views/widget/widget.cc:468: }
(due to rebase, not this CL)

[sky, 2013-03-15 03:15:05]

I'm adding Ben to the review too as this is going to result in a lot of things
subtling changing that I fear are going to be a source of countless bugs.

https://codereview.chromium.org/12342028/diff/92001/ui/aura/root_window_host.h
File ui/aura/root_window_host.h (right):

https://codereview.chromium.org/12342028/diff/92001/ui/aura/root_window_host....
ui/aura/root_window_host.h:127: // contain layer animation in excess of the
regular size.
Also indicate what effects this has on the layers and bounds of the rootwindow.

https://codereview.chromium.org/12342028/diff/92001/ui/aura/window_delegate.h
File ui/aura/window_delegate.h (right):

https://codereview.chromium.org/12342028/diff/92001/ui/aura/window_delegate.h...
ui/aura/window_delegate.h:44: virtual void SetHostTransitionBounds(const
gfx::Rect& bounds) = 0;
This and OnWindowHidingAnimationCompleted seem so specific that I would rather
they exist in their own interface. The animation code could look for a specific
property set on the window to see it it should invoke these.
DesktopNativeWidgetAura or maybe just RootWindowHostWin would implement the
interface.

https://codereview.chromium.org/12342028/diff/92001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/12342028/diff/92001/ui/compositor/layer.cc#ne...
ui/compositor/layer.cc:673: bool Layer::GetTargetTransformRelativeTo(const
Layer* ancestor,
Move implementation to match new position in header.

https://codereview.chromium.org/12342028/diff/92001/ui/views/widget/desktop_a...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/12342028/diff/92001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:253:
desktop_root_window_host_->InitFocus(window_);
What is the focus needed for?

https://codereview.chromium.org/12342028/diff/92001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:438: if
(!pending_close_)
Ugh, this also means if you Hide() a window then ask if its visible it will
return true.

https://codereview.chromium.org/12342028/diff/92001/ui/views/widget/desktop_a...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.h (right):

https://codereview.chromium.org/12342028/diff/92001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_native_widget_aura.h:245: bool
pending_close_;
This worries me. It results in delaying close quite a bit more than we do now
and I suspect could be the source of some subtle bugs. Is it possible to do this
another way? Probably not, but its worth thinking about.

https://codereview.chromium.org/12342028/diff/92001/ui/views/widget/desktop_a...
File ui/views/widget/desktop_aura/desktop_root_window_host_linux.cc (right):

https://codereview.chromium.org/12342028/diff/92001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_root_window_host_linux.cc:333:
NOTIMPLEMENTED();
Should this be focus_client_->FocusWindow(window); ?

[scottmg, 2013-03-15 22:23:46]

Thanks

> I'm adding Ben to the review too as this is going to result in a lot of things
> subtling changing that I fear are going to be a source of countless bugs.

I agree, there's sure to be some fallout from this change.

On the flip side, there's already a lot of bugs (some linked in BUG=) caused by
not having top level windows for these things, due to feature-level code
assumptions about focus when opening/closing HWNDs. Whether it'll be a net win
for number of bugs, who knows.

https://codereview.chromium.org/12342028/diff/92001/ui/aura/root_window_host.h
File ui/aura/root_window_host.h (right):

https://codereview.chromium.org/12342028/diff/92001/ui/aura/root_window_host....
ui/aura/root_window_host.h:127: // contain layer animation in excess of the
regular size.
On 2013/03/15 03:15:05, sky wrote:
> Also indicate what effects this has on the layers and bounds of the
rootwindow.

Done.

https://codereview.chromium.org/12342028/diff/92001/ui/aura/window_delegate.h
File ui/aura/window_delegate.h (right):

https://codereview.chromium.org/12342028/diff/92001/ui/aura/window_delegate.h...
ui/aura/window_delegate.h:44: virtual void SetHostTransitionBounds(const
gfx::Rect& bounds) = 0;
On 2013/03/15 03:15:05, sky wrote:
> This and OnWindowHidingAnimationCompleted seem so specific that I would rather
> they exist in their own interface. The animation code could look for a
specific
> property set on the window to see it it should invoke these.
> DesktopNativeWidgetAura or maybe just RootWindowHostWin would implement the
> interface.

Done, seems much better.

https://codereview.chromium.org/12342028/diff/92001/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/12342028/diff/92001/ui/compositor/layer.cc#ne...
ui/compositor/layer.cc:673: bool Layer::GetTargetTransformRelativeTo(const
Layer* ancestor,
On 2013/03/15 03:15:05, sky wrote:
> Move implementation to match new position in header.

Done.

https://codereview.chromium.org/12342028/diff/92001/ui/views/widget/desktop_a...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/12342028/diff/92001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:253:
desktop_root_window_host_->InitFocus(window_);
On 2013/03/15 03:15:05, sky wrote:
> What is the focus needed for?

The window_->Show has to be deferred until after the VisibilityController is
created, so that it fades in. Initial focus setting is currently rolled into
DRWH::Init, but because the window's not visible it's not focusable. So, this is
just to split the focus setting after VisibilityController creation and the
window is shown.

The VisibilityController can't be created earlier because it's implemented as a
root window property.

https://codereview.chromium.org/12342028/diff/92001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:438: if
(!pending_close_)
On 2013/03/15 03:15:05, sky wrote:
> Ugh, this also means if you Hide() a window then ask if its visible it will
> return true.

I believe Hide() still works as before (or that's the intention at least). This
just affects the Host, nothing at the aura::Window level.

The change in behaviour is that Close() is not immediate, and there's an
intermediate period where the Hide is in progress before the Host is actually
closed.

https://codereview.chromium.org/12342028/diff/92001/ui/views/widget/desktop_a...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.h (right):

https://codereview.chromium.org/12342028/diff/92001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_native_widget_aura.h:245: bool
pending_close_;
On 2013/03/15 03:15:05, sky wrote:
> This worries me. It results in delaying close quite a bit more than we do now
> and I suspect could be the source of some subtle bugs. Is it possible to do
this
> another way? Probably not, but its worth thinking about.

Yeah, I'm a bit concerned too. I can't see any cleaner way though.

One thing we could do to shake out potential bugs might be to add a flag to make
transitions very long (rather than the short 200ms default) to make the bugs
easier to see and reproduce.

https://codereview.chromium.org/12342028/diff/92001/ui/views/widget/desktop_a...
File ui/views/widget/desktop_aura/desktop_root_window_host_linux.cc (right):

https://codereview.chromium.org/12342028/diff/92001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_root_window_host_linux.cc:333:
NOTIMPLEMENTED();
On 2013/03/15 03:15:05, sky wrote:
> Should this be focus_client_->FocusWindow(window); ?

I don't know, I haven't tested on Linux beyond making sure tests pass. I'd
rather defer Linux changes for now.

Overall, this patch tries not to change Linux behaviour at all.
chrome_views_delegate.cc doesn't change the native widget or window hosts that
Linux uses because I don't know if we can do the top level window blending on
Linux or not.

[Ben Goodger (Google), 2013-03-15 22:31:47]

looks like you skipped adding animation_host* please add them.

in general I like the look of this change and it is remarkably cleanly
integrated.

[scottmg, 2013-03-15 22:33:53]

On 2013/03/15 22:31:47, Ben Goodger (Google) wrote:
> looks like you skipped adding animation_host* please add them.

oops, done.

[sky, 2013-03-18 15:49:42]

https://codereview.chromium.org/12342028/diff/83018/ui/views/corewm/window_an...
File ui/views/corewm/window_animations.cc (right):

https://codereview.chromium.org/12342028/diff/83018/ui/views/corewm/window_an...
ui/views/corewm/window_animations.cc:213: gfx::Rect world_at_start =
Can you early out here if animation_host is NULL?

https://codereview.chromium.org/12342028/diff/83018/ui/views/widget/desktop_a...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/12342028/diff/83018/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:423: pending_close_ =
true;
Do you end up in this code path on linux?

https://codereview.chromium.org/12342028/diff/83018/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:661: void
DesktopNativeWidgetAura::SetHostTransitionBounds(const gfx::Rect& bounds) {
If this is only applicable to windows, should we implement this in
DesktopRootWindowHostWin?

[scottmg, 2013-03-18 21:47:48]

https://codereview.chromium.org/12342028/diff/83018/ui/views/corewm/window_an...
File ui/views/corewm/window_animations.cc (right):

https://codereview.chromium.org/12342028/diff/83018/ui/views/corewm/window_an...
ui/views/corewm/window_animations.cc:213: gfx::Rect world_at_start =
On 2013/03/18 15:49:42, sky wrote:
> Can you early out here if animation_host is NULL?

Done.

https://codereview.chromium.org/12342028/diff/83018/ui/views/widget/desktop_a...
File ui/views/widget/desktop_aura/desktop_native_widget_aura.cc (right):

https://codereview.chromium.org/12342028/diff/83018/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_native_widget_aura.cc:661: void
DesktopNativeWidgetAura::SetHostTransitionBounds(const gfx::Rect& bounds) {
On 2013/03/18 15:49:42, sky wrote:
> If this is only applicable to windows, should we implement this in
> DesktopRootWindowHostWin?

Ah, that seems better, removes all the empty/NOTIMPLEMENTED functions. Done.

[sky, 2013-03-18 22:16:00]

LGTM

[Ben Goodger (Google), 2013-03-18 22:32:05]

verrah nice, lgtm

https://codereview.chromium.org/12342028/diff/132001/ui/aura/client/animation...
File ui/aura/client/animation_host.h (right):

https://codereview.chromium.org/12342028/diff/132001/ui/aura/client/animation...
ui/aura/client/animation_host.h:19: class AURA_EXPORT AnimationHost {
document purpose.

https://codereview.chromium.org/12342028/diff/132001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_root_window_host_win.h (right):

https://codereview.chromium.org/12342028/diff/132001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_root_window_host_win.h:131: // Overridden
from aura::WindowAnimationHost
nit: aura::client::AnimationHost:

[scottmg, 2013-03-19 04:12:35]

https://codereview.chromium.org/12342028/diff/132001/ui/aura/client/animation...
File ui/aura/client/animation_host.h (right):

https://codereview.chromium.org/12342028/diff/132001/ui/aura/client/animation...
ui/aura/client/animation_host.h:19: class AURA_EXPORT AnimationHost {
On 2013/03/18 22:32:06, Ben Goodger (Google) wrote:
> document purpose.

Done.

https://codereview.chromium.org/12342028/diff/132001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_root_window_host_win.h (right):

https://codereview.chromium.org/12342028/diff/132001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_root_window_host_win.h:131: // Overridden
from aura::WindowAnimationHost
On 2013/03/18 22:32:06, Ben Goodger (Google) wrote:
> nit: aura::client::AnimationHost:

Done.

[commit-bot: I haz the power, 2013-03-19 04:12:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/12342028/139001

[commit-bot: I haz the power, 2013-03-19 04:22:49]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-03-19 16:11:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/12342028/139001

[commit-bot: I haz the power, 2013-03-19 19:06:46]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-03-19 21:02:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/12342028/158001

[commit-bot: I haz the power, 2013-03-19 22:33:08]

Retried try job too often on linux_aura for step(s) views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-03-20 00:47:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/12342028/170001

[commit-bot: I haz the power, 2013-03-20 02:44:43]

Retried try job too often on linux_aura for step(s) views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-03-25 16:11:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scottmg@chromium.org/12342028/174003

[commit-bot: I haz the power, 2013-03-25 18:39:04]

Change committed as 190446