Draggable region support for frameless app window on Windows.

Draggable region support for frameless app window on Windows.

BUG=134169
TEST=Manual test by dragging frameless window

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

[jeremya, 2012-08-07 01:00:17]

lgtm, but I am not in the OWNERS file

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

http://codereview.chromium.org/10822004/diff/5004/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/shell_window_views.cc:212: if
(close_button_->visible() &&
Unrelated to your change, but we really should check that there exists a close
button before asking if it's visible...

http://codereview.chromium.org/10822004/diff/5004/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/shell_window_views.cc:555: 
nit: only 1 empty line here

http://codereview.chromium.org/10822004/diff/5004/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/10822004/diff/5004/content/common/view_message...
content/common/view_messages.h:2192: std::vector<content::DraggableRegion> /*
regions */)
I talked with jamesr a while ago about the best way to do this, and he suggested
maybe sending the draggable region data along with the UpdateRect message (which
include among other things plugin window move information). I think doing it
that way would guarantee that the draggable regions would always be in sync with
the pixels.

This way is fine for now, but it might be good to think about merging this
message with UpdateRect :)

[jianli, 2012-08-07 03:16:56]

Ben, can you take a look as an owner? Thanks.

On Mon, Aug 6, 2012 at 6:00 PM, <jeremya@chromium.org> wrote:

> lgtm, but I am not in the OWNERS file
>
>
> http://codereview.chromium.**org/10822004/diff/5004/chrome/**
>
browser/ui/views/extensions/**shell_window_views.cc<http://codereview.chromium.org/10822004/diff/5004/chrome/browser/ui/views/extensions/shell_window_views.cc>
> File chrome/browser/ui/views/**extensions/shell_window_views.**cc (right):
>
> http://codereview.chromium.**org/10822004/diff/5004/chrome/**
>
browser/ui/views/extensions/**shell_window_views.cc#**newcode212<http://codereview.chromium.org/10822004/diff/5004/chrome/browser/ui/views/extensions/shell_window_views.cc#newcode212>
> chrome/browser/ui/views/**extensions/shell_window_views.**cc:212: if
> (close_button_->visible() &&
> Unrelated to your change, but we really should check that there exists a
> close button before asking if it's visible...
>
> http://codereview.chromium.**org/10822004/diff/5004/chrome/**
>
browser/ui/views/extensions/**shell_window_views.cc#**newcode555<http://codereview.chromium.org/10822004/diff/5004/chrome/browser/ui/views/extensions/shell_window_views.cc#newcode555>
> chrome/browser/ui/views/**extensions/shell_window_views.**cc:555:
> nit: only 1 empty line here
>
> http://codereview.chromium.**org/10822004/diff/5004/**
>
content/common/view_messages.h<http://codereview.chromium.org/10822004/diff/5004/content/common/view_messages.h>
> File content/common/view_messages.h (right):
>
> http://codereview.chromium.**org/10822004/diff/5004/**
>
content/common/view_messages.**h#newcode2192<http://codereview.chromium.org/10822004/diff/5004/content/common/view_messages.h#newcode2192>
> content/common/view_messages.**h:2192:
> std::vector<content::**DraggableRegion> /* regions */)
> I talked with jamesr a while ago about the best way to do this, and he
> suggested maybe sending the draggable region data along with the
> UpdateRect message (which include among other things plugin window move
> information). I think doing it that way would guarantee that the
> draggable regions would always be in sync with the pixels.
>
> This way is fine for now, but it might be good to think about merging
> this message with UpdateRect :)
>
>
http://codereview.chromium.**org/10822004/<http://codereview.chromium.org/108...
>

[Ben Goodger (Google), 2012-08-07 18:03:51]

The main comment I have is that this feature seems specific to apps, which
implies to me that there shouldn't be anything in content/WCD supporting it,
rather this (and other v2 app-specific stuff) should be done as an apps v2
specific message filter defined by the apps code in chrome, sort of like how
autofill defines its messages.

http://codereview.chromium.org/10822004/diff/5004/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/shell_window_views.h (right):

http://codereview.chromium.org/10822004/diff/5004/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/shell_window_views.h:64: SkRegion*
draggable_region() { return draggable_region_.Get(); }
move these above the interface impls above.

[jianli, 2012-08-07 20:38:06]

Changed to handle via extension specific IPC.

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

http://codereview.chromium.org/10822004/diff/5004/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/shell_window_views.cc:212: if
(close_button_->visible() &&
On 2012/08/07 01:00:17, jeremya wrote:
> Unrelated to your change, but we really should check that there exists a close
> button before asking if it's visible...

Done.

http://codereview.chromium.org/10822004/diff/5004/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/shell_window_views.cc:555: 
On 2012/08/07 01:00:17, jeremya wrote:
> nit: only 1 empty line here

Done.

http://codereview.chromium.org/10822004/diff/5004/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/shell_window_views.h (right):

http://codereview.chromium.org/10822004/diff/5004/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/shell_window_views.h:64: SkRegion*
draggable_region() { return draggable_region_.Get(); }
On 2012/08/07 18:03:52, Ben Goodger (Google) wrote:
> move these above the interface impls above.

Done.

[Ben Goodger (Google), 2012-08-07 20:45:27]

http://codereview.chromium.org/10822004/diff/4018/chrome/browser/ui/extension...
File chrome/browser/ui/extensions/shell_window.h (right):

http://codereview.chromium.org/10822004/diff/4018/chrome/browser/ui/extension...
chrome/browser/ui/extensions/shell_window.h:30: struct DraggableRegion;
Is there an apps namespace that this can go into? Doesn't seem like this would
be relevant to extensions?

[jianli, 2012-08-07 20:48:15]

http://codereview.chromium.org/10822004/diff/4018/chrome/browser/ui/extension...
File chrome/browser/ui/extensions/shell_window.h (right):

http://codereview.chromium.org/10822004/diff/4018/chrome/browser/ui/extension...
chrome/browser/ui/extensions/shell_window.h:30: struct DraggableRegion;
On 2012/08/07 20:45:27, Ben Goodger (Google) wrote:
> Is there an apps namespace that this can go into? Doesn't seem like this would
> be relevant to extensions?

Unfortunately there is not an apps or related namespace for apps v2. For now, I
have to reuse extensions namespace.

[Ben Goodger (Google), 2012-08-07 21:13:39]

OK. LGTM.

[commit-bot: I haz the power, 2012-08-07 22:18:08]

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

[commit-bot: I haz the power, 2012-08-07 22:18:22]

Presubmit check for 10822004-4018 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/public
    content/renderer

Presubmit checks took 2.2s to calculate.

[jianli, 2012-08-07 22:22:07]

John, could you please approve it as an owner of content? Thanks.

[jam, 2012-08-07 23:06:42]

content lgtm

[commit-bot: I haz the power, 2012-08-07 23:07:24]

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

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

Change committed as 150471