Draggable region support for frameless app window on Mac.

Draggable region support for frameless app window on Mac.

BUG=134169
TEST=Manual test by dragging frameless window


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

[jeremya, 2012-08-07 00:38:53]

http://codereview.chromium.org/10823195/diff/1/chrome/browser/ui/cocoa/extens...
File chrome/browser/ui/cocoa/extensions/shell_window_cocoa.mm (right):

http://codereview.chromium.org/10823195/diff/1/chrome/browser/ui/cocoa/extens...
chrome/browser/ui/cocoa/extensions/shell_window_cocoa.mm:368: [frameView
addSubview:controlRegion];
We should add the ControlRegionViews as children of the WebContentsView, because
when we go to/from fullscreen we remove and re-add the WebContentsView (see
InstallView/UninstallView).

(Also, though, we don't want the window to be draggable while it's in fullscreen
mode -- perhaps the NSView tree could be something like:

+ WebContentsView
+-+ ControlRegion container (NSView*)
  +- ControlRegionView
  +- ControlRegionView
  +- ...

[jianli, 2012-08-08 21:26:31]

http://codereview.chromium.org/10823195/diff/1/chrome/browser/ui/cocoa/extens...
File chrome/browser/ui/cocoa/extensions/shell_window_cocoa.mm (right):

http://codereview.chromium.org/10823195/diff/1/chrome/browser/ui/cocoa/extens...
chrome/browser/ui/cocoa/extensions/shell_window_cocoa.mm:368: [frameView
addSubview:controlRegion];
On 2012/08/07 00:38:53, jeremya wrote:
> We should add the ControlRegionViews as children of the WebContentsView,
because
> when we go to/from fullscreen we remove and re-add the WebContentsView (see
> InstallView/UninstallView).
> 
> (Also, though, we don't want the window to be draggable while it's in
fullscreen
> mode -- perhaps the NSView tree could be something like:
> 
> + WebContentsView
> +-+ ControlRegion container (NSView*)
>   +- ControlRegionView
>   +- ControlRegionView
>   +- ...

Done.

[jeremya, 2012-08-09 00:16:44]

lgtm. +rsesek for OWNERS and further scrutiny.

http://codereview.chromium.org/10823195/diff/4001/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/shell_window_cocoa.mm (right):

http://codereview.chromium.org/10823195/diff/4001/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/shell_window_cocoa.mm:348: NSView*
contentView = web_contents()->GetView()->GetNativeView();
nit: perhaps call this webView to avoid confusion with [window contentView]?

http://codereview.chromium.org/10823195/diff/4001/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/shell_window_cocoa.mm:358: [subview
setHidden:YES];
curious: why do we need to setHidden here?

[jianli, 2012-08-09 00:29:27]

http://codereview.chromium.org/10823195/diff/4001/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/shell_window_cocoa.mm (right):

http://codereview.chromium.org/10823195/diff/4001/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/shell_window_cocoa.mm:348: NSView*
contentView = web_contents()->GetView()->GetNativeView();
On 2012/08/09 00:16:44, jeremya wrote:
> nit: perhaps call this webView to avoid confusion with [window contentView]?

Done.

http://codereview.chromium.org/10823195/diff/4001/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/shell_window_cocoa.mm:358: [subview
setHidden:YES];
On 2012/08/09 00:16:44, jeremya wrote:
> curious: why do we need to setHidden here?

Some sample code uses it before removing the view. Removed since it is not
really needed.

[Robert Sesek, 2012-08-09 14:52:44]

LGTM

http://codereview.chromium.org/10823195/diff/3002/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/shell_window_cocoa.h (right):

http://codereview.chromium.org/10823195/diff/3002/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/shell_window_cocoa.h:10: #include <vector>
Line 9 should be after this, not before.

http://codereview.chromium.org/10823195/diff/3002/chrome/browser/ui/cocoa/ext...
File chrome/browser/ui/cocoa/extensions/shell_window_cocoa.mm (right):

http://codereview.chromium.org/10823195/diff/3002/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/shell_window_cocoa.mm:349: int webViewHeight
= NSHeight([webView bounds]);
NSHeight returns a CGFloat. If you want to truncate, use NSInteger.

http://codereview.chromium.org/10823195/diff/3002/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/shell_window_cocoa.mm:352: // Note that we
need to copy subviews because [webView subviews] returns
Avoid "we" in comments. Third person is preferred.

http://codereview.chromium.org/10823195/diff/3002/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/shell_window_cocoa.mm:355: NSArray* subviews
= [[webView subviews] copy];
scoped_nsobject<T>

http://codereview.chromium.org/10823195/diff/3002/chrome/browser/ui/cocoa/ext...
chrome/browser/ui/cocoa/extensions/shell_window_cocoa.mm:365: iter !=
draggable_regions_.end(); ++iter) {
GO ahead and move ++iter to its own line, too

[commit-bot: I haz the power, 2012-08-09 17:24:19]

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

[commit-bot: I haz the power, 2012-08-09 19:18:11]

Try job failure for 10823195-6003 (retry) (retry) on mac_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-08-09 20:12:39]

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

[commit-bot: I haz the power, 2012-08-09 21:19:37]

Try job failure for 10823195-6003 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-08-09 21:35:10]

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

[commit-bot: I haz the power, 2012-08-10 00:21:42]

Try job failure for 10823195-6003 (retry) on win_rel for step
"interactive_ui_tests" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-08-10 00:33:48]

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

[commit-bot: I haz the power, 2012-08-10 02:38:29]

Try job failure for 10823195-6003 (retry) on win_rel for step "runhooks".
It's a second try, previously, steps "interactive_ui_tests, browser_tests"
failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-08-10 03:01:55]

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

[commit-bot: I haz the power, 2012-08-10 04:53:40]

Change committed as 150994