Draggable region support for frameless app window on CrOS.

Draggable region support for frameless app window on CrOS.

BUG=134169
TEST=new unittest for aura


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152683

[Ben Goodger (Google), 2012-08-17 17:10:24]

http://codereview.chromium.org/10831361/diff/11002/ui/aura/window.h
File ui/aura/window.h (right):

http://codereview.chromium.org/10831361/diff/11002/ui/aura/window.h#newcode270
ui/aura/window.h:270: void SetDraggableRegion(SkRegion* region);
Aura is agnostic of things that relate specifically to a window manager, such as
draggable regions. This type of thing really belongs in Ash.

[jianli, 2012-08-17 23:26:45]

I tried to put the logic in
ash::ToplevelWindowEventFilter::PreHandleMouseEvent, but it did not work
well. When this method is called, the event target
is RenderWidgetHostViewAura window. So I convert target and event
from RenderWidgetHostViewAura based to NativeWidgetAura based such that we
can move the top window. The even location passed here seems not be 100%
reliable for the mouse drag event. So I called
wm::GetRootWindowRelativeToWindow and it worked for most of cases but it is
still flaky.

It seems a bit late to process mouse events for draggable region at child
window level and then convert it all the way back to parent window with all
the work to try to make the location conversion right. This is the reason I
try to do it in the aura window layer in order to figure out the right
window target before delegating the event to it. I understand that we
should try not to put the WM specific logic in aura. What I added to
aura::Window in this patch is more like click-through region that is used
to indicate all clicks occurring in its child windows should go through and
be handled by this window. All the logic to interpret these clicks indeed
happen at ash layer. How about renaming draggable_region stored in Window
to something like clickthrough_region?


On Fri, Aug 17, 2012 at 10:10 AM, <ben@chromium.org> wrote:

>
>
http://codereview.chromium.**org/10831361/diff/11002/ui/**aura/window.h<http:...
> File ui/aura/window.h (right):
>
> http://codereview.chromium.**org/10831361/diff/11002/ui/**
>
aura/window.h#newcode270<http://codereview.chromium.org/10831361/diff/11002/ui/aura/window.h#newcode270>
> ui/aura/window.h:270: void SetDraggableRegion(SkRegion* region);
> Aura is agnostic of things that relate specifically to a window manager,
> such as draggable regions. This type of thing really belongs in Ash.
>
>
http://codereview.chromium.**org/10831361/<http://codereview.chromium.org/108...
>

[Ben Goodger (Google), 2012-08-20 20:34:17]

My understanding of how this stuff works:

The browser process keeps a copy of a draggable region defined by the renderer.
This copy is periodically refreshed. The reason for the copy is that the "can I
move the window by dragging here" query implemented by at least Windows and Ash
is one that must be answered synchronously.

The logical place for this copy to exist is in cross-platform shell-window code.
I say this because I am assuming that the region is the same for apps on all
platforms.

The question then becomes how to wire the synchronous query of frame component
through to that code.

In Views, and in Aura, there is a function called "GetNonClientFrameComponent"
implemented by the NonClientFrameView. I believe there is one of these for
ShellWindows. It hit-tests the provided point against the browser-side region.

With that in mind I am not sure why we need any special handling for ash, given
that ash already implements the "OS" side of this contract - i.e. the WM already
calls GetNonClientFrameComponent and respects the result.

[Ben Goodger (Google), 2012-08-20 20:36:40]

I guess it could be that the overlapping RWHVA is consuming the invents
instead of the frame.


On Mon, Aug 20, 2012 at 1:34 PM, <ben@chromium.org> wrote:

> My understanding of how this stuff works:
>
> The browser process keeps a copy of a draggable region defined by the
> renderer.
> This copy is periodically refreshed. The reason for the copy is that the
> "can I
> move the window by dragging here" query implemented by at least Windows
> and Ash
> is one that must be answered synchronously.
>
> The logical place for this copy to exist is in cross-platform shell-window
> code.
> I say this because I am assuming that the region is the same for apps on
> all
> platforms.
>
> The question then becomes how to wire the synchronous query of frame
> component
> through to that code.
>
> In Views, and in Aura, there is a function called
> "GetNonClientFrameComponent"
> implemented by the NonClientFrameView. I believe there is one of these for
> ShellWindows. It hit-tests the provided point against the browser-side
> region.
>
> With that in mind I am not sure why we need any special handling for ash,
> given
> that ash already implements the "OS" side of this contract - i.e. the WM
> already
> calls GetNonClientFrameComponent and respects the result.
>
>
http://codereview.chromium.**org/10831361/<http://codereview.chromium.org/108...
>

[jianli, 2012-08-20 23:17:26]

On 2012/08/20 20:36:40, Ben Goodger (Google) wrote:
> I guess it could be that the overlapping RWHVA is consuming the invents
> instead of the frame.
> 
Yes, this is because aura::Window::GetWindowForPoint returns RWHVA window. So I
try to tell aura::Window about the clickthrough region such that
aura::Window::GetWindowForPoint can return the right window. How do you think
about this approach?

> 
> On Mon, Aug 20, 2012 at 1:34 PM, <mailto:ben@chromium.org> wrote:
> 
> > My understanding of how this stuff works:
> >
> > The browser process keeps a copy of a draggable region defined by the
> > renderer.
> > This copy is periodically refreshed. The reason for the copy is that the
> > "can I
> > move the window by dragging here" query implemented by at least Windows
> > and Ash
> > is one that must be answered synchronously.
> >
> > The logical place for this copy to exist is in cross-platform shell-window
> > code.
> > I say this because I am assuming that the region is the same for apps on
> > all
> > platforms.
> >
> > The question then becomes how to wire the synchronous query of frame
> > component
> > through to that code.
> >
> > In Views, and in Aura, there is a function called
> > "GetNonClientFrameComponent"
> > implemented by the NonClientFrameView. I believe there is one of these for
> > ShellWindows. It hit-tests the provided point against the browser-side
> > region.
> >
> > With that in mind I am not sure why we need any special handling for ash,
> > given
> > that ash already implements the "OS" side of this contract - i.e. the WM
> > already
> > calls GetNonClientFrameComponent and respects the result.
> >
> >
>
http://codereview.chromium.**org/10831361/%3Chttp://codereview.chromium.org/1...>
> >

[Ben Goodger (Google), 2012-08-20 23:35:25]

So, what we need then is a way to have GetWindowForPoint() return the frame
in this case.

There is a method on aura::WindowDelegate already called
ShouldDescendIntoChildForEventHandling, this is called by
Window::GetWindowForPoint(). What is missing is a way for ShellWindowViews
to influence the answer
NativeWidgetAura::ShouldDescendIntoChildForEventHandling provides.

I propose adding a method to views::WidgetDelegate with the same name
(ShouldDescendInto..) and then overriding it in ShellWindowViews like so:

bool ShellWindowViews::ShouldDescendIntoChildForEventHandling(aura::Window*
child, const gfx::Point& location) {
DCHECK_EQ(child, webview_->web_contents()->GetView()->GetNativeView());
return !draggable_region_.contains(location);
}

... you would need to make NWA::ShouldDescendInto.. call this
WidgetDelegate method first and return early if it does.

-Ben


On Mon, Aug 20, 2012 at 4:17 PM, <jianli@chromium.org> wrote:

> On 2012/08/20 20:36:40, Ben Goodger (Google) wrote:
>
>> I guess it could be that the overlapping RWHVA is consuming the invents
>> instead of the frame.
>>
>
> Yes, this is because aura::Window::**GetWindowForPoint returns RWHVA
> window. So I
> try to tell aura::Window about the clickthrough region such that
> aura::Window::**GetWindowForPoint can return the right window. How do you
> think
> about this approach?
>
>
>
>  On Mon, Aug 20, 2012 at 1:34 PM, <mailto:ben@chromium.org> wrote:
>>
>
>  > My understanding of how this stuff works:
>> >
>> > The browser process keeps a copy of a draggable region defined by the
>> > renderer.
>> > This copy is periodically refreshed. The reason for the copy is that the
>> > "can I
>> > move the window by dragging here" query implemented by at least Windows
>> > and Ash
>> > is one that must be answered synchronously.
>> >
>> > The logical place for this copy to exist is in cross-platform
>> shell-window
>> > code.
>> > I say this because I am assuming that the region is the same for apps on
>> > all
>> > platforms.
>> >
>> > The question then becomes how to wire the synchronous query of frame
>> > component
>> > through to that code.
>> >
>> > In Views, and in Aura, there is a function called
>> > "GetNonClientFrameComponent"
>> > implemented by the NonClientFrameView. I believe there is one of these
>> for
>> > ShellWindows. It hit-tests the provided point against the browser-side
>> > region.
>> >
>> > With that in mind I am not sure why we need any special handling for
>> ash,
>> > given
>> > that ash already implements the "OS" side of this contract - i.e. the WM
>> > already
>> > calls GetNonClientFrameComponent and respects the result.
>> >
>> >
>>
>
> http://codereview.chromium.****org/10831361/%3Chttp://coderev**
> iew.chromium.org/10831361/ <http://codereview.chromium.org/10831361/>>
>
>> >
>>
>
>
>
http://codereview.chromium.**org/10831361/<http://codereview.chromium.org/108...
>

[jianli, 2012-08-20 23:47:31]

We also need to pass the event type to the delegate method because we only
want to intercept certain mouse events, like mouse drags and clicks. How do
you think?


On Mon, Aug 20, 2012 at 4:35 PM, Ben Goodger (Google) <ben@chromium.org>wrote:

> So, what we need then is a way to have GetWindowForPoint() return the
> frame in this case.
>
> There is a method on aura::WindowDelegate already called
> ShouldDescendIntoChildForEventHandling, this is called by
> Window::GetWindowForPoint(). What is missing is a way for ShellWindowViews
> to influence the answer
> NativeWidgetAura::ShouldDescendIntoChildForEventHandling provides.
>
> I propose adding a method to views::WidgetDelegate with the same name
> (ShouldDescendInto..) and then overriding it in ShellWindowViews like so:
>
> bool
> ShellWindowViews::ShouldDescendIntoChildForEventHandling(aura::Window*
> child, const gfx::Point& location) {
> DCHECK_EQ(child, webview_->web_contents()->GetView()->GetNativeView());
> return !draggable_region_.contains(location);
> }
>
> ... you would need to make NWA::ShouldDescendInto.. call this
> WidgetDelegate method first and return early if it does.
>
> -Ben
>
>
> On Mon, Aug 20, 2012 at 4:17 PM, <jianli@chromium.org> wrote:
>
>> On 2012/08/20 20:36:40, Ben Goodger (Google) wrote:
>>
>>> I guess it could be that the overlapping RWHVA is consuming the invents
>>> instead of the frame.
>>>
>>
>> Yes, this is because aura::Window::**GetWindowForPoint returns RWHVA
>> window. So I
>> try to tell aura::Window about the clickthrough region such that
>> aura::Window::**GetWindowForPoint can return the right window. How do
>> you think
>> about this approach?
>>
>>
>>
>>  On Mon, Aug 20, 2012 at 1:34 PM, <mailto:ben@chromium.org> wrote:
>>>
>>
>>  > My understanding of how this stuff works:
>>> >
>>> > The browser process keeps a copy of a draggable region defined by the
>>> > renderer.
>>> > This copy is periodically refreshed. The reason for the copy is that
>>> the
>>> > "can I
>>> > move the window by dragging here" query implemented by at least Windows
>>> > and Ash
>>> > is one that must be answered synchronously.
>>> >
>>> > The logical place for this copy to exist is in cross-platform
>>> shell-window
>>> > code.
>>> > I say this because I am assuming that the region is the same for apps
>>> on
>>> > all
>>> > platforms.
>>> >
>>> > The question then becomes how to wire the synchronous query of frame
>>> > component
>>> > through to that code.
>>> >
>>> > In Views, and in Aura, there is a function called
>>> > "GetNonClientFrameComponent"
>>> > implemented by the NonClientFrameView. I believe there is one of these
>>> for
>>> > ShellWindows. It hit-tests the provided point against the browser-side
>>> > region.
>>> >
>>> > With that in mind I am not sure why we need any special handling for
>>> ash,
>>> > given
>>> > that ash already implements the "OS" side of this contract - i.e. the
>>> WM
>>> > already
>>> > calls GetNonClientFrameComponent and respects the result.
>>> >
>>> >
>>>
>>
>> http://codereview.chromium.****org/10831361/%3Chttp://coderev**
>> iew.chromium.org/10831361/ <http://codereview.chromium.org/10831361/>>
>>
>>> >
>>>
>>
>>
>>
http://codereview.chromium.**org/10831361/<http://codereview.chromium.org/108...
>>
>
>

[Ben Goodger (Google), 2012-08-20 23:57:39]

Does it really matter?


On Mon, Aug 20, 2012 at 4:47 PM, Jian Li <jianli@chromium.org> wrote:

> We also need to pass the event type to the delegate method because we only
> want to intercept certain mouse events, like mouse drags and clicks. How do
> you think?
>
>
> On Mon, Aug 20, 2012 at 4:35 PM, Ben Goodger (Google)
<ben@chromium.org>wrote:
>
>> So, what we need then is a way to have GetWindowForPoint() return the
>> frame in this case.
>>
>> There is a method on aura::WindowDelegate already called
>> ShouldDescendIntoChildForEventHandling, this is called by
>> Window::GetWindowForPoint(). What is missing is a way for ShellWindowViews
>> to influence the answer
>> NativeWidgetAura::ShouldDescendIntoChildForEventHandling provides.
>>
>> I propose adding a method to views::WidgetDelegate with the same name
>> (ShouldDescendInto..) and then overriding it in ShellWindowViews like so:
>>
>> bool
>> ShellWindowViews::ShouldDescendIntoChildForEventHandling(aura::Window*
>> child, const gfx::Point& location) {
>> DCHECK_EQ(child, webview_->web_contents()->GetView()->GetNativeView());
>> return !draggable_region_.contains(location);
>> }
>>
>> ... you would need to make NWA::ShouldDescendInto.. call this
>> WidgetDelegate method first and return early if it does.
>>
>> -Ben
>>
>>
>> On Mon, Aug 20, 2012 at 4:17 PM, <jianli@chromium.org> wrote:
>>
>>> On 2012/08/20 20:36:40, Ben Goodger (Google) wrote:
>>>
>>>> I guess it could be that the overlapping RWHVA is consuming the invents
>>>> instead of the frame.
>>>>
>>>
>>> Yes, this is because aura::Window::**GetWindowForPoint returns RWHVA
>>> window. So I
>>> try to tell aura::Window about the clickthrough region such that
>>> aura::Window::**GetWindowForPoint can return the right window. How do
>>> you think
>>> about this approach?
>>>
>>>
>>>
>>>  On Mon, Aug 20, 2012 at 1:34 PM, <mailto:ben@chromium.org> wrote:
>>>>
>>>
>>>  > My understanding of how this stuff works:
>>>> >
>>>> > The browser process keeps a copy of a draggable region defined by the
>>>> > renderer.
>>>> > This copy is periodically refreshed. The reason for the copy is that
>>>> the
>>>> > "can I
>>>> > move the window by dragging here" query implemented by at least
>>>> Windows
>>>> > and Ash
>>>> > is one that must be answered synchronously.
>>>> >
>>>> > The logical place for this copy to exist is in cross-platform
>>>> shell-window
>>>> > code.
>>>> > I say this because I am assuming that the region is the same for apps
>>>> on
>>>> > all
>>>> > platforms.
>>>> >
>>>> > The question then becomes how to wire the synchronous query of frame
>>>> > component
>>>> > through to that code.
>>>> >
>>>> > In Views, and in Aura, there is a function called
>>>> > "GetNonClientFrameComponent"
>>>> > implemented by the NonClientFrameView. I believe there is one of
>>>> these for
>>>> > ShellWindows. It hit-tests the provided point against the browser-side
>>>> > region.
>>>> >
>>>> > With that in mind I am not sure why we need any special handling for
>>>> ash,
>>>> > given
>>>> > that ash already implements the "OS" side of this contract - i.e. the
>>>> WM
>>>> > already
>>>> > calls GetNonClientFrameComponent and respects the result.
>>>> >
>>>> >
>>>>
>>>
>>> http://codereview.chromium.****org/10831361/%3Chttp://coderev**
>>> iew.chromium.org/10831361/ <http://codereview.chromium.org/10831361/>>
>>>
>>>> >
>>>>
>>>
>>>
>>>
http://codereview.chromium.**org/10831361/<http://codereview.chromium.org/108...
>>>
>>
>>
>

[jianli, 2012-08-21 00:03:04]

Yes because we still want the RenderWidgetHostViewAura window to handle the
mouse move events since the underlying page might do different things based
on if the mouse is over it or not.

On Mon, Aug 20, 2012 at 4:57 PM, Ben Goodger (Google) <ben@chromium.org>wrote:

> Does it really matter?
>
>
> On Mon, Aug 20, 2012 at 4:47 PM, Jian Li <jianli@chromium.org> wrote:
>
>> We also need to pass the event type to the delegate method because we
>> only want to intercept certain mouse events, like mouse drags and clicks.
>> How do you think?
>>
>>
>> On Mon, Aug 20, 2012 at 4:35 PM, Ben Goodger (Google)
<ben@chromium.org>wrote:
>>
>>> So, what we need then is a way to have GetWindowForPoint() return the
>>> frame in this case.
>>>
>>> There is a method on aura::WindowDelegate already called
>>> ShouldDescendIntoChildForEventHandling, this is called by
>>> Window::GetWindowForPoint(). What is missing is a way for ShellWindowViews
>>> to influence the answer
>>> NativeWidgetAura::ShouldDescendIntoChildForEventHandling provides.
>>>
>>> I propose adding a method to views::WidgetDelegate with the same name
>>> (ShouldDescendInto..) and then overriding it in ShellWindowViews like so:
>>>
>>> bool
>>> ShellWindowViews::ShouldDescendIntoChildForEventHandling(aura::Window*
>>> child, const gfx::Point& location) {
>>> DCHECK_EQ(child, webview_->web_contents()->GetView()->GetNativeView());
>>> return !draggable_region_.contains(location);
>>> }
>>>
>>> ... you would need to make NWA::ShouldDescendInto.. call this
>>> WidgetDelegate method first and return early if it does.
>>>
>>> -Ben
>>>
>>>
>>> On Mon, Aug 20, 2012 at 4:17 PM, <jianli@chromium.org> wrote:
>>>
>>>> On 2012/08/20 20:36:40, Ben Goodger (Google) wrote:
>>>>
>>>>> I guess it could be that the overlapping RWHVA is consuming the invents
>>>>> instead of the frame.
>>>>>
>>>>
>>>> Yes, this is because aura::Window::**GetWindowForPoint returns RWHVA
>>>> window. So I
>>>> try to tell aura::Window about the clickthrough region such that
>>>> aura::Window::**GetWindowForPoint can return the right window. How do
>>>> you think
>>>> about this approach?
>>>>
>>>>
>>>>
>>>>  On Mon, Aug 20, 2012 at 1:34 PM, <mailto:ben@chromium.org> wrote:
>>>>>
>>>>
>>>>  > My understanding of how this stuff works:
>>>>> >
>>>>> > The browser process keeps a copy of a draggable region defined by the
>>>>> > renderer.
>>>>> > This copy is periodically refreshed. The reason for the copy is that
>>>>> the
>>>>> > "can I
>>>>> > move the window by dragging here" query implemented by at least
>>>>> Windows
>>>>> > and Ash
>>>>> > is one that must be answered synchronously.
>>>>> >
>>>>> > The logical place for this copy to exist is in cross-platform
>>>>> shell-window
>>>>> > code.
>>>>> > I say this because I am assuming that the region is the same for
>>>>> apps on
>>>>> > all
>>>>> > platforms.
>>>>> >
>>>>> > The question then becomes how to wire the synchronous query of frame
>>>>> > component
>>>>> > through to that code.
>>>>> >
>>>>> > In Views, and in Aura, there is a function called
>>>>> > "GetNonClientFrameComponent"
>>>>> > implemented by the NonClientFrameView. I believe there is one of
>>>>> these for
>>>>> > ShellWindows. It hit-tests the provided point against the
>>>>> browser-side
>>>>> > region.
>>>>> >
>>>>> > With that in mind I am not sure why we need any special handling for
>>>>> ash,
>>>>> > given
>>>>> > that ash already implements the "OS" side of this contract - i.e.
>>>>> the WM
>>>>> > already
>>>>> > calls GetNonClientFrameComponent and respects the result.
>>>>> >
>>>>> >
>>>>>
>>>>
>>>> http://codereview.chromium.****org/10831361/%3Chttp://coderev**
>>>> iew.chromium.org/10831361/ <http://codereview.chromium.org/10831361/>>
>>>>
>>>>> >
>>>>>
>>>>
>>>>
>>>>
http://codereview.chromium.**org/10831361/<http://codereview.chromium.org/108...
>>>>
>>>
>>>
>>
>

[jianli, 2012-08-21 00:59:18]

Changed as suggested. Ben, can you review again? Thanks.

[Ben Goodger (Google), 2012-08-21 02:17:39]

http://codereview.chromium.org/10831361/diff/2008/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/shell_window_views.cc (right):

http://codereview.chromium.org/10831361/diff/2008/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/shell_window_views.cc:343: DCHECK_EQ(child,
web_view_->web_contents()->GetView()->GetNativeView());
the body of this function needs to be #ifdef USE_AURA.

http://codereview.chromium.org/10831361/diff/2008/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/shell_window_views.cc:348: (event_type !=
ui::ET_MOUSE_PRESSED &&
I don't understand why the need to filter on event type. It seems like any event
to an area designated part of the frame should trigger handling by the top
window.

http://codereview.chromium.org/10831361/diff/2008/ui/views/widget/widget_dele...
File ui/views/widget/widget_delegate.h (right):

http://codereview.chromium.org/10831361/diff/2008/ui/views/widget/widget_dele...
ui/views/widget/widget_delegate.h:154: aura::Window* child,
This won't compile on non-aura builds. You have to pass gfx::NativeView instead.

[jianli, 2012-08-21 05:33:15]

On Mon, Aug 20, 2012 at 7:17 PM, <ben@chromium.org> wrote:

>
> http://codereview.chromium.**org/10831361/diff/2008/chrome/**
>
browser/ui/views/extensions/**shell_window_views.cc<http://codereview.chromium.org/10831361/diff/2008/chrome/browser/ui/views/extensions/shell_window_views.cc>
> File chrome/browser/ui/views/**extensions/shell_window_views.**cc (right):
>
> http://codereview.chromium.**org/10831361/diff/2008/chrome/**
>
browser/ui/views/extensions/**shell_window_views.cc#**newcode343<http://codereview.chromium.org/10831361/diff/2008/chrome/browser/ui/views/extensions/shell_window_views.cc#newcode343>
> chrome/browser/ui/views/**extensions/shell_window_views.**cc:343:
> DCHECK_EQ(child, web_view_->web_contents()->**GetView()->GetNativeView());
> the body of this function needs to be #ifdef USE_AURA.
>
> http://codereview.chromium.**org/10831361/diff/2008/chrome/**
>
browser/ui/views/extensions/**shell_window_views.cc#**newcode348<http://codereview.chromium.org/10831361/diff/2008/chrome/browser/ui/views/extensions/shell_window_views.cc#newcode348>
> chrome/browser/ui/views/**extensions/shell_window_views.**cc:348:
> (event_type != ui::ET_MOUSE_PRESSED &&
> I don't understand why the need to filter on event type. It seems like
> any event to an area designated part of the frame should trigger
> handling by the top window.
>
> The draggable region of the frameless app window is only intended to be
used for window dragging purpose. Other mouse events like mouse hover
should still be handled by the underlying web page. For example, a
frameless app window that has marked the top titlebar area as the draggable
region would like to render the titlebar area in different color when the
mouse moves over it (see bug
http://code.google.com/p/chromium/issues/detail?id=142275). So we need to
return the different window for the event handling depending on the type of
the event.



> http://codereview.chromium.**org/10831361/diff/2008/ui/**
>
views/widget/widget_delegate.h<http://codereview.chromium.org/10831361/diff/2008/ui/views/widget/widget_delegate.h>
> File ui/views/widget/widget_**delegate.h (right):
>
> http://codereview.chromium.**org/10831361/diff/2008/ui/**
>
views/widget/widget_delegate.**h#newcode154<http://codereview.chromium.org/10831361/diff/2008/ui/views/widget/widget_delegate.h#newcode154>
> ui/views/widget/widget_**delegate.h:154: aura::Window* child,
> This won't compile on non-aura builds. You have to pass gfx::NativeView
> instead.
>
>
http://codereview.chromium.**org/10831361/<http://codereview.chromium.org/108...
>

[jianli, 2012-08-21 17:22:00]

http://codereview.chromium.org/10831361/diff/2008/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/shell_window_views.cc (right):

http://codereview.chromium.org/10831361/diff/2008/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/shell_window_views.cc:343: DCHECK_EQ(child,
web_view_->web_contents()->GetView()->GetNativeView());
On 2012/08/21 02:17:40, Ben Goodger (Google) wrote:
> the body of this function needs to be #ifdef USE_AURA.

Done.

http://codereview.chromium.org/10831361/diff/2008/ui/views/widget/widget_dele...
File ui/views/widget/widget_delegate.h (right):

http://codereview.chromium.org/10831361/diff/2008/ui/views/widget/widget_dele...
ui/views/widget/widget_delegate.h:154: aura::Window* child,
On 2012/08/21 02:17:40, Ben Goodger (Google) wrote:
> This won't compile on non-aura builds. You have to pass gfx::NativeView
instead.

Done.

[Ben Goodger (Google), 2012-08-21 18:18:09]

On 2012/08/21 05:33:15, jianli wrote:
> > The draggable region of the frameless app window is only intended to be
> used for window dragging purpose. Other mouse events like mouse hover
> should still be handled by the underlying web page. For example, a
> frameless app window that has marked the top titlebar area as the draggable
> region would like to render the titlebar area in different color when the
> mouse moves over it (see bug
> http://code.google.com/p/chromium/issues/detail?id=142275). So we need to
> return the different window for the event handling depending on the type of
> the event.

What happens if the app puts a clickable element overlapping the titlebar area?

[jianli, 2012-08-21 18:24:45]

On Tue, Aug 21, 2012 at 11:18 AM, <ben@chromium.org> wrote:

> On 2012/08/21 05:33:15, jianli wrote:
>
>> > The draggable region of the frameless app window is only intended to be
>> used for window dragging purpose. Other mouse events like mouse hover
>> should still be handled by the underlying web page. For example, a
>> frameless app window that has marked the top titlebar area as the
>> draggable
>> region would like to render the titlebar area in different color when the
>> mouse moves over it (see bug
>>
http://code.google.com/p/**chromium/issues/detail?id=**142275<http://code.goo...).
>> So we need to
>> return the different window for the event handling depending on the type
>> of
>> the event.
>>
>
> What happens if the app puts a clickable element overlapping the titlebar
> area?
>

If the app chooses to do that, the clickable element will not receive the
mouse click events, but we still want to honor the mouse move events. The
draggable titlebar area is meant to be non-clickable.


>
>
http://codereview.chromium.**org/10831361/<http://codereview.chromium.org/108...
>

[jianli, 2012-08-21 22:01:43]

I've removed the event type filtering logic. Ben, can you take a look again?
Thanks.

[Ben Goodger (Google), 2012-08-21 22:03:54]

lgtm

[commit-bot: I haz the power, 2012-08-21 22:36:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jianli@chromium.org/10831361/15048

[commit-bot: I haz the power, 2012-08-22 00:14:08]

Change committed as 152683