[MacViews] Enable dragging a window by its caption/draggable areas.

[MacViews] Enable dragging a window by its caption/draggable areas.

This works by making all subviews of [NSWindow contentView] draggable.
I.e. they return YES for mouseDownCanMoveWindow. The content view
itself is initially non-draggable. BridgedNativeWidget watches for any
mouse-downs and tests whether they should drag the window. If so, it
makes the content view draggable and reposts the event.

The event is reposted at the kCGSessionEventTap level because window
dragging seems to be handled there before the event is sent to the
application.

When BridgedNativeWidget sees the reposted mouse-down, window dragging
will have started, so the content view can be made non-draggable again.

BUG=485394, 459877
Committed: https://crrev.com/dc783c7be84a2a8c5564736f4b17a4fbd03897d6
Cr-Commit-Position: refs/heads/master@{#333410}

[jackhou1, 2015-05-20 05:50:37]

Here's some working code. It's not very polished yet, please ignore debugging
and other nits.

I have some questions inline. Any other suggestions are welcome.

I initially left the CGEventTap implementation in so you can see how it
compares, but removed it in Patch Set 3. Ultimately we'll probably use the
NSEvent local monitor implementation since I haven't found any issues with it.
And as discussed, it avoids having to listen to all mouse-downs, and the code is
simpler.

https://codereview.chromium.org/1146873002/diff/20001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget.h (right):

https://codereview.chromium.org/1146873002/diff/20001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.h:269: // Whether the window's content view
is currently draggable. Initially true
This should initially be false, but we'd need to add the
CocoaNonClientDragMaskView after the webcontents has been added to the content
view. Another way to do this would be to set/unset mouseDownCanMoveWindow on the
content view, which eliminates the CocoaNonClientDragMaskView. I will look into
that.

This is done in Patch Set 3.

https://codereview.chromium.org/1146873002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/chrome_native_app_window_views_mac.mm (right):

https://codereview.chromium.org/1146873002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views_mac.mm:72: void
ChromeNativeAppWindowViewsMac::UpdateDraggableRegions(
This is only needed as a "If we know the web contents might be draggable, set
mouseDownCanMoveWindow". Is there a way to set this whenever the window is a
BridgedNativeWidget? In fact we'd want to set it on every subview that might
have draggable regions.

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/cocoa_no...
File ui/views/cocoa/cocoa_non_client_drag.h (right):

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/cocoa_no...
ui/views/cocoa/cocoa_non_client_drag.h:34: class VIEWS_EXPORT CocoaNonClientDrag
{
It feels kind of roundabout that a class instantiates CocoaNonClientDrag to
indicate that it wants this behavior, but needs to handle
willReceiveLeftMouseDown on the NSWindow. I'm not sure of a more direct way to
do this.

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/cocoa_no...
File ui/views/cocoa/cocoa_non_client_drag.mm (right):

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/cocoa_no...
ui/views/cocoa/cocoa_non_client_drag.mm:17: int g_ref_count = 0;
Do we already have something that implements this behavior? I.e. singleton, but
reference counted so it is disabled when nobody needs the behavior? Or maybe we
could get away with having it always enabled.

[jackhou1, 2015-05-20 05:51:10]

jackhou@chromium.org changed reviewers:
+ tapted@chromium.org

[jackhou1, 2015-05-20 05:51:11]

Bah, forgot to add reviewers when I last published comments.

[tapted, 2015-05-20 07:18:18]

Awesome work Jack!

You may have told me this already, but does it work fine if Chrome isn't the
active application?


and general comment, if we just go with a leaky lazy singleton, maybe all this
will become simple enough to put it all in BridgedNativeWidget.

i.e.

BridgedNativeWidget::BridgedNativeWidget(NativeWidgetMac* parent)
  ... {
  ...
  static id drag_monitor =
      [NSEvent addLocalMonitorForEventsMatchingMask:NSLeftMouseDownMask
                                              handler:^NSEvent*(NSEvent*
ns_event) {
          return DoubleRepostEventIfHandledByWindow(ns_event) ? nil : ns_event;
      }];
}

https://codereview.chromium.org/1146873002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/native_app_window_frame_view_mac.mm (right):

https://codereview.chromium.org/1146873002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/native_app_window_frame_view_mac.mm:45: return
HTCLIENT;
AppWindowFrameView::NonClientHitTest defaults to HTCAPTION -- should the
ClientView be responsible for deciding to return HTCLIENT?

https://codereview.chromium.org/1146873002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/native_app_window_frame_view_mac.mm:46: }
no you need to return HTNOWHERE ever? (otherwise DCHECK that |point| is
contained within the window bounds?)

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget.h (right):

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.h:138: // reposted.
Perhaps say why this exists here.

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.h:139: bool
OnWindowWillReceiveLeftMouseDown(NSPoint location_in_window);
perhaps ShouldRepostPendingLeftMouseDown ?

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:97: non_client_drag_(new
CocoaNonClientDrag()),
My suggestion: ditch the reference counting and in the constructor body just do

  // Ensure the event tap is installed that allows windows to be dragged around
  // using their content area.
  CR_DEFINE_STATIC_LOCAL(CocoaNonClientDrag, non_client_drag, ());
  ALLOW_UNUSED_LOCAL(non_client_drag);

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:530: BridgedContentView* content_view =
[window_ contentView];
nit: can just use bridged_view_. I think this will also need an

if (!bridged_view)
  return false;

since events may be received between NativeWidgetMac::Close() (which clears it)
and ~BridgedNativeWidget (which destroys CocoaNonClientDrag to kill the event
tap).

An alternative could be to make non_client_drag a scoped_ptr, and [re]set it in
SetRootView(..).

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:533: NSHeight([window_ frame]) -
location_in_window.y);
is this the right thing to flip in? Normally Widget functions expect coordinates
in the content area -- i.e. 

gfx::Point point = [bridged_view_ flipNSRectToRect:location];

might be what you want

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:534: bool draggable =
native_widget_mac()->GetWidget()->GetNonClientComponent(
nit: maybe draggable -> should_move_window (I think it reads better below)

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:540: if (was_draggable) {
nit: move to the top of the function? i.e.

if (!bridged_view_)
  return false;

if ([bridged_view_ mouseDownCanMoveWindow]) {
  SetDraggable(false);
  return false;
}

gfx::Point point..
bool should_move_window..
if (!should_move_window)
  return false;

etc.

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:541: // This is a re-post, the movement
has already completed, so we can make the
completed -> started?

(i.e. I think we are still dragging the window after this)

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:545: // something changes.
is there a private method we could poke to trigger this?

Otherwise I would make this a private method like

BridgedNativeWidget::SetDraggable(bool)

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/cocoa_no...
File ui/views/cocoa/cocoa_non_client_drag.h (right):

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/cocoa_no...
ui/views/cocoa/cocoa_non_client_drag.h:12: @interface NativeWidgetMacNSWindow
(CocoaNonClientDrag)
I don't think this is needed (see later comments)

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/cocoa_no...
File ui/views/cocoa/cocoa_non_client_drag.mm (right):

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/cocoa_no...
ui/views/cocoa/cocoa_non_client_drag.mm:17: int g_ref_count = 0;
On 2015/05/20 05:50:37, jackhou1 wrote:
> Do we already have something that implements this behavior? I.e. singleton,
but
> reference counted so it is disabled when nobody needs the behavior? Or maybe
we
> could get away with having it always enabled.

Yeah I would just create it lazily, once, and not worry about discarding it.
(see comment in BridgedNativeWidget constructor)

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/cocoa_no...
ui/views/cocoa/cocoa_non_client_drag.mm:19: bool is_first_repost = false;
make this a local static of DoubleRepostEventIfHandledByWindow() ?

Also, there may be some racy stuff here... events can sometimes get queued (e.g.
try putting a sleep(5) and sending a bunch of mouse events).

I actually don't know if CGEventPost(kCGSessionEventTap, event) puts the event
at the start or the end of the queue. If it's the start, I don't think there
will be problems.

If it's at the end, maybe you need to track something like kCGMouseEventNumber
to ensure it's tracking the correct mouse event. (but then I have no idea what
to do with ones that don't match..

Ideal would be if we could annotate the CGEventRef with some user data like
'repost count' but I don't think that's a thing

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/cocoa_no...
ui/views/cocoa/cocoa_non_client_drag.mm:31: if ([window
respondsToSelector:@selector(willReceiveLeftMouseDown:)] &&
maybe just check if [[window delegate]
respondsToSelector:@selector(onWindowWillReceiveLeftMouseDown:) and send
straight to the delegate?

i.e. no need for the NativeWidgetMacNSWindow (CocoaNonClientDrag) category

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/cocoa_no...
ui/views/cocoa/cocoa_non_client_drag.mm:52: if
(DoubleRepostEventIfHandledByWindow(
just pass ns_event?

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/cocoa_no...
ui/views/cocoa/cocoa_non_client_drag.mm:59: g_event_monitor =
I guess this can be a static local too.

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/cocoa_no...
ui/views/cocoa/cocoa_non_client_drag.mm:67: --g_ref_count;
I guess this would just become a NOTREACHED() with all the other advice.

[jackhou1, 2015-05-22 02:49:17]

> You may have told me this already, but does it work fine if Chrome isn't the
> active application?

Yup.

https://codereview.chromium.org/1146873002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/native_app_window_frame_view_mac.mm (right):

https://codereview.chromium.org/1146873002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/native_app_window_frame_view_mac.mm:45: return
HTCLIENT;
On 2015/05/20 07:18:15, tapted wrote:
> AppWindowFrameView::NonClientHitTest defaults to HTCAPTION -- should the
> ClientView be responsible for deciding to return HTCLIENT?

I think (at least on Mac) that app windows can only have caption areas with
draggable_region.

https://codereview.chromium.org/1146873002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/native_app_window_frame_view_mac.mm:46: }
On 2015/05/20 07:18:15, tapted wrote:
> no you need to return HTNOWHERE ever? (otherwise DCHECK that |point| is
> contained within the window bounds?)

Done.

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget.h (right):

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.h:138: // reposted.
On 2015/05/20 07:18:15, tapted wrote:
> Perhaps say why this exists here.

Done.

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.h:139: bool
OnWindowWillReceiveLeftMouseDown(NSPoint location_in_window);
On 2015/05/20 07:18:15, tapted wrote:
> perhaps ShouldRepostPendingLeftMouseDown ?

Done.

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:97: non_client_drag_(new
CocoaNonClientDrag()),
On 2015/05/20 07:18:15, tapted wrote:
> My suggestion: ditch the reference counting and in the constructor body just
do
> 
>   // Ensure the event tap is installed that allows windows to be dragged
around
>   // using their content area.
>   CR_DEFINE_STATIC_LOCAL(CocoaNonClientDrag, non_client_drag, ());
>   ALLOW_UNUSED_LOCAL(non_client_drag);

Moved all code to bridged_native_widget.mm.

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:530: BridgedContentView* content_view =
[window_ contentView];
On 2015/05/20 07:18:15, tapted wrote:
> nit: can just use bridged_view_. I think this will also need an
> 
> if (!bridged_view)
>   return false;
> 
> since events may be received between NativeWidgetMac::Close() (which clears
it)
> and ~BridgedNativeWidget (which destroys CocoaNonClientDrag to kill the event
> tap).
> 
> An alternative could be to make non_client_drag a scoped_ptr, and [re]set it
in
> SetRootView(..).

Done.

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:533: NSHeight([window_ frame]) -
location_in_window.y);
On 2015/05/20 07:18:17, tapted wrote:
> is this the right thing to flip in? Normally Widget functions expect
coordinates
> in the content area -- i.e. 
> 
> gfx::Point point = [bridged_view_ flipNSRectToRect:location];
> 
> might be what you want

We want to first flip this to a gfx::Point in the windows coordinates. 

NativeWidgetDelegate::GetNonClientComponent says it's in "client" coordinates,
but aura::WindowDelegate::GetNonClientComponent says it's in window coordinates
and directly calls the former. So I think it's okay to be in window coordinates.

Also BridgedContentView doesn't have flipNSRectToRect because it's not a
BaseView. Should it be?

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:534: bool draggable =
native_widget_mac()->GetWidget()->GetNonClientComponent(
On 2015/05/20 07:18:15, tapted wrote:
> nit: maybe draggable -> should_move_window (I think it reads better below)

Done.

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:540: if (was_draggable) {
On 2015/05/20 07:18:15, tapted wrote:
> nit: move to the top of the function? i.e.
> 
> if (!bridged_view_)
>   return false;
> 
> if ([bridged_view_ mouseDownCanMoveWindow]) {
>   SetDraggable(false);
>   return false;
> }
> 
> gfx::Point point..
> bool should_move_window..
> if (!should_move_window)
>   return false;
> 
> etc.

Done.

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:541: // This is a re-post, the movement
has already completed, so we can make the
On 2015/05/20 07:18:16, tapted wrote:
> completed -> started?
> 
> (i.e. I think we are still dragging the window after this)

Done.

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:545: // something changes.
On 2015/05/20 07:18:17, tapted wrote:
> is there a private method we could poke to trigger this?
> 
> Otherwise I would make this a private method like
> 
> BridgedNativeWidget::SetDraggable(bool)

[NSThemeFrame _resetDragMargins] does something but it comes out buggier, maybe
it only sometimes updates.

Done.

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/cocoa_no...
File ui/views/cocoa/cocoa_non_client_drag.mm (right):

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/cocoa_no...
ui/views/cocoa/cocoa_non_client_drag.mm:17: int g_ref_count = 0;
On 2015/05/20 07:18:17, tapted wrote:
> On 2015/05/20 05:50:37, jackhou1 wrote:
> > Do we already have something that implements this behavior? I.e. singleton,
> but
> > reference counted so it is disabled when nobody needs the behavior? Or maybe
> we
> > could get away with having it always enabled.
> 
> Yeah I would just create it lazily, once, and not worry about discarding it.
> (see comment in BridgedNativeWidget constructor)

Done.

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/cocoa_no...
ui/views/cocoa/cocoa_non_client_drag.mm:19: bool is_first_repost = false;
On 2015/05/20 07:18:18, tapted wrote:
> make this a local static of DoubleRepostEventIfHandledByWindow() ?
> 
> Also, there may be some racy stuff here... events can sometimes get queued
(e.g.
> try putting a sleep(5) and sending a bunch of mouse events).
> 
> I actually don't know if CGEventPost(kCGSessionEventTap, event) puts the event
> at the start or the end of the queue. If it's the start, I don't think there
> will be problems.
> 
> If it's at the end, maybe you need to track something like kCGMouseEventNumber
> to ensure it's tracking the correct mouse event. (but then I have no idea what
> to do with ones that don't match..
>
> Ideal would be if we could annotate the CGEventRef with some user data like
> 'repost count' but I don't think that's a thing

Fortunately, the eventNumber does remain unchanged, so we can use it to track
reposted events.
I've improved the logic to identify intermediate mouse-downs and cancel reposts.

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/cocoa_no...
ui/views/cocoa/cocoa_non_client_drag.mm:31: if ([window
respondsToSelector:@selector(willReceiveLeftMouseDown:)] &&
On 2015/05/20 07:18:17, tapted wrote:
> maybe just check if [[window delegate]
> respondsToSelector:@selector(onWindowWillReceiveLeftMouseDown:) and send
> straight to the delegate?
> 
> i.e. no need for the NativeWidgetMacNSWindow (CocoaNonClientDrag) category

Done.

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/cocoa_no...
ui/views/cocoa/cocoa_non_client_drag.mm:52: if
(DoubleRepostEventIfHandledByWindow(
On 2015/05/20 07:18:18, tapted wrote:
> just pass ns_event?

Done.

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/cocoa_no...
ui/views/cocoa/cocoa_non_client_drag.mm:59: g_event_monitor =
On 2015/05/20 07:18:18, tapted wrote:
> I guess this can be a static local too.

Done.

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/cocoa_no...
ui/views/cocoa/cocoa_non_client_drag.mm:67: --g_ref_count;
On 2015/05/20 07:18:18, tapted wrote:
> I guess this would just become a NOTREACHED() with all the other advice.

Done.

[tapted, 2015-05-22 04:03:12]

niiiice

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1146873002/diff/40001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:533: NSHeight([window_ frame]) -
location_in_window.y);
On 2015/05/22 02:49:16, jackhou1 wrote:
> On 2015/05/20 07:18:17, tapted wrote:
> > is this the right thing to flip in? Normally Widget functions expect
> coordinates
> > in the content area -- i.e. 
> > 
> > gfx::Point point = [bridged_view_ flipNSRectToRect:location];
> > 
> > might be what you want
> 
> We want to first flip this to a gfx::Point in the windows coordinates. 
> 
> NativeWidgetDelegate::GetNonClientComponent says it's in "client" coordinates,
> but aura::WindowDelegate::GetNonClientComponent says it's in window
coordinates
> and directly calls the former. So I think it's okay to be in window
coordinates.

I guess so long as it's not out by the size of the titlebar it should be ok.

> Also BridgedContentView doesn't have flipNSRectToRect because it's not a
> BaseView. Should it be?

'tis now :)

https://codereview.chromium.org/1146873002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/chrome_native_app_window_views_mac.mm (right):

https://codereview.chromium.org/1146873002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views_mac.mm:15: -
(void)setMouseDownCanMoveWindow:(BOOL)can_move;
I *think* this should be declared in @protocol RenderWidgetHostViewMacBase of
content/public/browser/render_widget_host_view_mac_base.h

it's content/public so should be OK to #include here.

Then you can DCHECK(...
conformsToProtocol:@protocol(RenderWidgetHostViewMacBase)) on the
web_contents_view

the existing copy in native_app_window_cocoa.mm should do the same

https://codereview.chromium.org/1146873002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/native_app_window_frame_view_mac.h (right):

https://codereview.chromium.org/1146873002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/native_app_window_frame_view_mac.h:30:
extensions::NativeAppWindow* native_app_window_;
nit: ownership comment

https://codereview.chromium.org/1146873002/diff/60001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_content_view.h (right):

https://codereview.chromium.org/1146873002/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_content_view.h:24: : NSView<NSTextInputClient,
NSUserInterfaceValidations> {
looks like you need to rebase - this is a ToolTipBaseView now :)

https://codereview.chromium.org/1146873002/diff/60001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget.h (right):

https://codereview.chromium.org/1146873002/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.h:135: // Called when the application
receives a mouse-down, but before the event
nit: start with "Called by the NSWindowDelegate". Maybe this should be some kind
of privateAPI that only the delegate can see... but ObjC doesn't play nice with
`friend` declarations.

https://codereview.chromium.org/1146873002/diff/60001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1146873002/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:119: }
else { NOTREACHED(); }? (i.e. we should never see that event number again.
Although perhaps `reposted_event_number = -1` at the same time we set the repost
count to 0 would be more robust to hypothetical wraparaound

https://codereview.chromium.org/1146873002/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:121: } else {
nit: no else after return

https://codereview.chromium.org/1146873002/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:130: } else {
no else after return

https://codereview.chromium.org/1146873002/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:155: static id g_event_monitor = nil;
nit: no `g_` for local statics. I'd maybe just call it monitor.

https://codereview.chromium.org/1146873002/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:159: NSEvent*
(^monitor_callback)(NSEvent* ns_event);
nit: move this into the function call (just to hide the block type syntax). See
e.g. event_monitor_mac.mm

https://codereview.chromium.org/1146873002/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:977: // AppKit will not update it's
cache of mouseDownCanMoveWindow unless
nit: it's -> its

[jackhou1, 2015-05-25 04:50:19]

https://codereview.chromium.org/1146873002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/chrome_native_app_window_views_mac.mm (right):

https://codereview.chromium.org/1146873002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views_mac.mm:15: -
(void)setMouseDownCanMoveWindow:(BOOL)can_move;
On 2015/05/22 04:03:11, tapted wrote:
> I *think* this should be declared in @protocol RenderWidgetHostViewMacBase of
> content/public/browser/render_widget_host_view_mac_base.h
> 
> it's content/public so should be OK to #include here.
> 
> Then you can DCHECK(...
> conformsToProtocol:@protocol(RenderWidgetHostViewMacBase)) on the
> web_contents_view
> 
> the existing copy in native_app_window_cocoa.mm should do the same

Looks like the RenderWidgetHostViewCocoa is a subview of WebContentsViewCocoa.

https://codereview.chromium.org/1146873002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/native_app_window_frame_view_mac.h (right):

https://codereview.chromium.org/1146873002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/native_app_window_frame_view_mac.h:30:
extensions::NativeAppWindow* native_app_window_;
On 2015/05/22 04:03:12, tapted wrote:
> nit: ownership comment

Done.

https://codereview.chromium.org/1146873002/diff/60001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_content_view.h (right):

https://codereview.chromium.org/1146873002/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_content_view.h:24: : NSView<NSTextInputClient,
NSUserInterfaceValidations> {
On 2015/05/22 04:03:12, tapted wrote:
> looks like you need to rebase - this is a ToolTipBaseView now :)

Done.

https://codereview.chromium.org/1146873002/diff/60001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget.h (right):

https://codereview.chromium.org/1146873002/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.h:135: // Called when the application
receives a mouse-down, but before the event
On 2015/05/22 04:03:12, tapted wrote:
> nit: start with "Called by the NSWindowDelegate". Maybe this should be some
kind
> of privateAPI that only the delegate can see... but ObjC doesn't play nice
with
> `friend` declarations.

Done.

https://codereview.chromium.org/1146873002/diff/60001/ui/views/cocoa/bridged_...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1146873002/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:119: }
On 2015/05/22 04:03:12, tapted wrote:
> else { NOTREACHED(); }? (i.e. we should never see that event number again.
> Although perhaps `reposted_event_number = -1` at the same time we set the
repost
> count to 0 would be more robust to hypothetical wraparaound

In the if (expected_repost_count > 0) block below, we set expected_repost_count
= 0; but reposted_event_number is unchanged. In that case, we want to do nothing
and just return nil. Made that more explicit.

https://codereview.chromium.org/1146873002/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:121: } else {
On 2015/05/22 04:03:12, tapted wrote:
> nit: no else after return

Done.

https://codereview.chromium.org/1146873002/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:130: } else {
On 2015/05/22 04:03:12, tapted wrote:
> no else after return

Done.

https://codereview.chromium.org/1146873002/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:155: static id g_event_monitor = nil;
On 2015/05/22 04:03:12, tapted wrote:
> nit: no `g_` for local statics. I'd maybe just call it monitor.

Done.

https://codereview.chromium.org/1146873002/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:159: NSEvent*
(^monitor_callback)(NSEvent* ns_event);
On 2015/05/22 04:03:12, tapted wrote:
> nit: move this into the function call (just to hide the block type syntax).
See
> e.g. event_monitor_mac.mm

Done.

https://codereview.chromium.org/1146873002/diff/60001/ui/views/cocoa/bridged_...
ui/views/cocoa/bridged_native_widget.mm:977: // AppKit will not update it's
cache of mouseDownCanMoveWindow unless
On 2015/05/22 04:03:12, tapted wrote:
> nit: it's -> its

Done.

[tapted, 2015-05-25 05:24:06]

lgtm % 1 nit below

https://codereview.chromium.org/1146873002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/apps/chrome_native_app_window_views_mac.mm (right):

https://codereview.chromium.org/1146873002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/apps/chrome_native_app_window_views_mac.mm:15: -
(void)setMouseDownCanMoveWindow:(BOOL)can_move;
On 2015/05/25 04:50:18, jackhou1 wrote:
> On 2015/05/22 04:03:11, tapted wrote:
> > I *think* this should be declared in @protocol RenderWidgetHostViewMacBase
of
> > content/public/browser/render_widget_host_view_mac_base.h
> > 
> > it's content/public so should be OK to #include here.
> > 
> > Then you can DCHECK(...
> > conformsToProtocol:@protocol(RenderWidgetHostViewMacBase)) on the
> > web_contents_view
> > 
> > the existing copy in native_app_window_cocoa.mm should do the same
> 
> Looks like the RenderWidgetHostViewCocoa is a subview of WebContentsViewCocoa.

Drat. I don't like this re-declaration, but I guess I can live with it :/

https://codereview.chromium.org/1146873002/diff/100001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget.h (right):

https://codereview.chromium.org/1146873002/diff/100001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.h:135: // Called by ViewsNSWindowDelegate
when the application receives a mouse-down,
nit: "Called by the NSWindowDelegate" for consistency with the above

https://codereview.chromium.org/1146873002/diff/100001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1146873002/diff/100001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:170: handler:^NSEvent*(NSEvent*
ns_event) {
I guess this looks alright :p. I don't think clang-format handles blocks well.

go/objcguide?showone=Method_Invocations#Method_Invocations says "Invocations
containing multiple inlined blocks may have their parameter names left-aligned
at a four space indent".

since the first argument name is so long, this inline block probably looks
better with `handler` left-aligned rather than colon-aligned, even though there
are not "multiple" inline blocks. Meh.

[tapted, 2015-05-25 05:24:31]

oh, also "it's" -> its in the CL description & summary

[jackhou1, 2015-05-25 06:26:39]

https://codereview.chromium.org/1146873002/diff/100001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget.h (right):

https://codereview.chromium.org/1146873002/diff/100001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.h:135: // Called by ViewsNSWindowDelegate
when the application receives a mouse-down,
On 2015/05/25 05:24:05, tapted wrote:
> nit: "Called by the NSWindowDelegate" for consistency with the above

Done.

https://codereview.chromium.org/1146873002/diff/100001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1146873002/diff/100001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:170: handler:^NSEvent*(NSEvent*
ns_event) {
On 2015/05/25 05:24:05, tapted wrote:
> I guess this looks alright :p. I don't think clang-format handles blocks well.
> 
> go/objcguide?showone=Method_Invocations#Method_Invocations says "Invocations
> containing multiple inlined blocks may have their parameter names left-aligned
> at a four space indent".
> 
> since the first argument name is so long, this inline block probably looks
> better with `handler` left-aligned rather than colon-aligned, even though
there
> are not "multiple" inline blocks. Meh.

Done.

[jackhou1, 2015-05-26 03:19:17]

tapted, PTAL at:

c/b/ui/views/frame/*

[jackhou1, 2015-06-01 04:15:39]

FTR, patch set 8 didn't seem to work on 10.9.

Patch set 9 adds a basic test. The test passes on 10.10 with sdks [6, 9, 10].

[jackhou1, 2015-06-02 06:33:09]

tapted, PTAL

I have something that is much simpler and seems to work with all cases: [browser
window, native app window, macviews interactive test] building with sdks [6, 9].
But I've only tested running this on 10.10 so far.

https://codereview.chromium.org/1146873002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/native_app_window_frame_view_mac.mm (right):

https://codereview.chromium.org/1146873002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/native_app_window_frame_view_mac.mm:36:
DCHECK(bounds().Contains(point));
Ah, I think this still needs to be fixed. It should test the point and return
HTNOWHERE.

https://codereview.chromium.org/1146873002/diff/180001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1146873002/diff/180001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:994: [window_
setMovableByWindowBackground:NO];
I'm not sure exactly why it wasn't working for browser windows, but this seems
to make it work with only one repost.

https://codereview.chromium.org/1146873002/diff/180001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget_interactive_uitest.mm (right):

https://codereview.chromium.org/1146873002/diff/180001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:18: @interface
WindowedNSNotificationObserver : NSObject {
This can probably be merged with the one in app_shim_interactive_uitest_mac.mm.
That one uses the shared workspace instead of the default notification center,
so there would need to be some logic to distinguish them. WDYT? Is there a good
place for this?

https://codereview.chromium.org/1146873002/diff/180001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:314:
RunPendingMessages();
I suspect these are not actually waiting long enough for the events to be
reposted. Is there a neat way to wait for a mouse event to be received?

[tapted, 2015-06-03 00:28:37]

awesome!

I think one problem remains - the close/minimize/maximize buttons can't be
clicked on a browser window, but possible solution might be commented below.

(and still looking at the test body)

https://codereview.chromium.org/1146873002/diff/120001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_content_view.h (right):

https://codereview.chromium.org/1146873002/diff/120001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_content_view.h:53: @property(assign, nonatomic) BOOL
mouseDownCanMoveWindow;
why this change?

https://codereview.chromium.org/1146873002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_mac.mm (right):

https://codereview.chromium.org/1146873002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view_mac.mm:72: int
component = frame()->client_view()->NonClientHitTest(point);
maybe we need to encode the close/minimize button positions into the
NativeFrameView...

https://codereview.chromium.org/1146873002/diff/180001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1146873002/diff/180001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:97: static NSInteger
reposted_event_number = -1;
did we want to try this out with 0 instead of -1? (maybe kNoEventNumber since
it's reused below). Or perhaps we need a separate static bool for
`reposting_cancelled`. Or enum
RepostState {
  NONE,  // Nothing reposted: hit-test new mousedowns to see if they need to be
ignored and reposted after changing draggability.
  EXPECTING_REPOST,  // Expecting the next event to be the reposted event: let
it go through.
  REPOST_CANCELLED,  // If, while reposting, another mousedown was received:
when the reposted event is seen, ignore it.
};


That might make the logic easier to follow, and make it neater for the most
common case of `NONE` to be dealt with first. Then the flow in the function is
more "linear" (i.e. reading from top-to-bottom is then the order that things
happen in.)

https://codereview.chromium.org/1146873002/diff/180001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:105: // - BridgedNativeWidget's
draggability getting out of sync (e.g. it's
nit: it's -> its

https://codereview.chromium.org/1146873002/diff/180001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:994: [window_
setMovableByWindowBackground:NO];
On 2015/06/02 06:33:09, jackhou1 wrote:
> I'm not sure exactly why it wasn't working for browser windows, but this seems
> to make it work with only one repost.

Nice!

https://codereview.chromium.org/1146873002/diff/180001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget_interactive_uitest.mm (right):

https://codereview.chromium.org/1146873002/diff/180001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:18: @interface
WindowedNSNotificationObserver : NSObject {
On 2015/06/02 06:33:09, jackhou1 wrote:
> This can probably be merged with the one in
app_shim_interactive_uitest_mac.mm.
> That one uses the shared workspace instead of the default notification center,
> so there would need to be some logic to distinguish them. WDYT? Is there a
good
> place for this?

are they both called WindowedNSNotificationObserver .. (shouldn't there be a
symbol conflict if they are both linked into interactive_ui_tests ?)

but! there's also one in bridged_native_widget_interactive_uitest already, so we
should definitely merge them all.

There's also NSWindowFullscreenNotificationWaiter in ui/base/test but it's
slightly different. Not sure if there's a neat way to merge that as well, but
these other 3 should be merged into something going into the same folder.

https://codereview.chromium.org/1146873002/diff/180001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:58: namespace views {
add namespace test { after this (i.e. whole file should be views::test:: rather
than just views::

https://codereview.chromium.org/1146873002/diff/180001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:314:
RunPendingMessages();
On 2015/06/02 06:33:09, jackhou1 wrote:
> I suspect these are not actually waiting long enough for the events to be
> reposted. Is there a neat way to wait for a mouse event to be received?

hm.... for these kind of events, maybe not without another event tap AFAIK. You
could use EventMonitorMac and inherit from ui::EventHandler.

Or, for another crazy idea, I just landed `ScopedSwizzleWaiter` in
native_widget_mac_unittest.mm -- you can give it basically any objective C
method on any class and spin a runloop until it gets called. E.g. you could do
this on [NSWindow setMovableByBackgroundWindow], and just wait for that call to
happen.

Then you probably wouldn't even have to make `ShouldRepostPendingLeftMouseDown`
a virtual method.

https://codereview.chromium.org/1146873002/diff/180001/ui/views/widget/native...
File ui/views/widget/native_widget_mac.h (right):

https://codereview.chromium.org/1146873002/diff/180001/ui/views/widget/native...
ui/views/widget/native_widget_mac.h:132: friend class HitTestNativeWidgetMac;
(this should end up in test::)

[jackhou1, 2015-06-03 04:04:44]

https://codereview.chromium.org/1146873002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/apps/native_app_window_frame_view_mac.mm (right):

https://codereview.chromium.org/1146873002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/apps/native_app_window_frame_view_mac.mm:36:
DCHECK(bounds().Contains(point));
On 2015/06/02 06:33:09, jackhou1 wrote:
> Ah, I think this still needs to be fixed. It should test the point and return
> HTNOWHERE.

Done.

https://codereview.chromium.org/1146873002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_mac.mm (right):

https://codereview.chromium.org/1146873002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_non_client_frame_view_mac.mm:72: int
component = frame()->client_view()->NonClientHitTest(point);
On 2015/06/03 00:28:37, tapted wrote:
> maybe we need to encode the close/minimize button positions into the
> NativeFrameView...

I was thinking we could do our own NSView hit-test. I.e. if the views hit-test
returns HTCAPTION, check if the point overlaps any NSView other than the content
view. See patch set 11 (BridgedNativeWidget::ShouldRepostPendingLeftMouseDown).

https://codereview.chromium.org/1146873002/diff/180001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1146873002/diff/180001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:97: static NSInteger
reposted_event_number = -1;
On 2015/06/03 00:28:37, tapted wrote:
> did we want to try this out with 0 instead of -1? (maybe kNoEventNumber since
> it's reused below). Or perhaps we need a separate static bool for
> `reposting_cancelled`. Or enum
> RepostState {
>   NONE,  // Nothing reposted: hit-test new mousedowns to see if they need to
be
> ignored and reposted after changing draggability.
>   EXPECTING_REPOST,  // Expecting the next event to be the reposted event: let
> it go through.
>   REPOST_CANCELLED,  // If, while reposting, another mousedown was received:
> when the reposted event is seen, ignore it.
> };
> 
> 
> That might make the logic easier to follow, and make it neater for the most
> common case of `NONE` to be dealt with first. Then the flow in the function is
> more "linear" (i.e. reading from top-to-bottom is then the order that things
> happen in.)

Done.

https://codereview.chromium.org/1146873002/diff/180001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:105: // - BridgedNativeWidget's
draggability getting out of sync (e.g. it's
On 2015/06/03 00:28:37, tapted wrote:
> nit: it's -> its

This is actually an it's. Changed to "if it is".

https://codereview.chromium.org/1146873002/diff/180001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget_interactive_uitest.mm (right):

https://codereview.chromium.org/1146873002/diff/180001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:18: @interface
WindowedNSNotificationObserver : NSObject {
On 2015/06/03 00:28:37, tapted wrote:
> On 2015/06/02 06:33:09, jackhou1 wrote:
> > This can probably be merged with the one in
> app_shim_interactive_uitest_mac.mm.
> > That one uses the shared workspace instead of the default notification
center,
> > so there would need to be some logic to distinguish them. WDYT? Is there a
> good
> > place for this?
> 
> are they both called WindowedNSNotificationObserver .. (shouldn't there be a
> symbol conflict if they are both linked into interactive_ui_tests ?)

Nope, this one is in macviews_interactive_ui_tests.

> but! there's also one in bridged_native_widget_interactive_uitest already, so
we
> should definitely merge them all.

That's this file, did you mean somewhere else?
 
> There's also NSWindowFullscreenNotificationWaiter in ui/base/test but it's
> slightly different. Not sure if there's a neat way to merge that as well, but
> these other 3 should be merged into something going into the same folder.
> 

Done.

https://codereview.chromium.org/1146873002/diff/180001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:58: namespace views {
On 2015/06/03 00:28:37, tapted wrote:
> add namespace test { after this (i.e. whole file should be views::test::
rather
> than just views::

Done.

https://codereview.chromium.org/1146873002/diff/180001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:314:
RunPendingMessages();
On 2015/06/03 00:28:37, tapted wrote:
> On 2015/06/02 06:33:09, jackhou1 wrote:
> > I suspect these are not actually waiting long enough for the events to be
> > reposted. Is there a neat way to wait for a mouse event to be received?
> 
> hm.... for these kind of events, maybe not without another event tap AFAIK.
You
> could use EventMonitorMac and inherit from ui::EventHandler.
> 
> Or, for another crazy idea, I just landed `ScopedSwizzleWaiter` in
> native_widget_mac_unittest.mm -- you can give it basically any objective C
> method on any class and spin a runloop until it gets called. E.g. you could do
> this on [NSWindow setMovableByBackgroundWindow], and just wait for that call
to
> happen.
> 
> Then you probably wouldn't even have to make
`ShouldRepostPendingLeftMouseDown`
> a virtual method.

Just ended up using a local event monitor.

I had a look at ScopedSwizzleWaiter, but it seems a lot more complicated than
just overriding ShouldRepostPendingLeftMouseDown.

https://codereview.chromium.org/1146873002/diff/180001/ui/views/widget/native...
File ui/views/widget/native_widget_mac.h (right):

https://codereview.chromium.org/1146873002/diff/180001/ui/views/widget/native...
ui/views/widget/native_widget_mac.h:132: friend class HitTestNativeWidgetMac;
On 2015/06/03 00:28:37, tapted wrote:
> (this should end up in test::)

Done.

[tapted, 2015-06-03 07:04:50]

https://codereview.chromium.org/1146873002/diff/180001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1146873002/diff/180001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:105: // - BridgedNativeWidget's
draggability getting out of sync (e.g. it's
On 2015/06/03 04:04:44, jackhou1 wrote:
> On 2015/06/03 00:28:37, tapted wrote:
> > nit: it's -> its
> 
> This is actually an it's. Changed to "if it is".

whoops! I must have read as "its draggability outside.." :p.

https://codereview.chromium.org/1146873002/diff/180001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget_interactive_uitest.mm (right):

https://codereview.chromium.org/1146873002/diff/180001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:18: @interface
WindowedNSNotificationObserver : NSObject {
On 2015/06/03 04:04:44, jackhou1 wrote:
> On 2015/06/03 00:28:37, tapted wrote:
> > but! there's also one in bridged_native_widget_interactive_uitest already,
so
> we
> > should definitely merge them all.
> 
> That's this file, did you mean somewhere else?

oh whoops - nope. #wirescrossed

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
File ui/base/test/windowed_nsnotification_observer.h (right):

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.h:5: #ifndef
UI_GFX_TEST_WINDOWED_NSNOTIFICATION_OBSERVER_H_
UI_GFX_TEST -> UI_BASE_TEST

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.h:8: #import <Cocoa/Cocoa.h>
This should be <Foundation/Foundation.h> since AppKit isn't being used. All you
"need" is NSObject (even NSString could be forward-declared, but Foundation will
give it to you anyway, and you only #import NSObject.h directly in special
cases.

(but you'll probably need AppKit.h or Cocoa.h in the .mm for NSWorkspace and
things

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.h:11: #include
"base/memory/scoped_ptr.h"
nit: not needed (see below)

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.h:12: #include "base/run_loop.h"
nit: forward-declare

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.h:14: // Watches for
NSNotifications.
something more verbose? (i.e. "sell" this class - describe the situations where
someone would use it). Should say stuff like ~immediately starts listening once
created

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.h:15: @interface
WindowedNSNotificationObserver : NSObject {
Do you know the history behind the "Windowed" prefix? I don't think it's right.

Maybe "CocoaNotificationMessageWaiter" ?

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.h:19: scoped_ptr<base::RunLoop>
runLoop_;
This can just be a raw pointer to base::RunLoop (see below)

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.h:22: // Watch for a notification
on the default notification center.
nit: "a notification" -> "an NSNotification"

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.h:24: // Watch for a notification
on the shared workspace notification center for the
nit: blank line before

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.h:26: -
(id)initForNotification:(NSString*)name
maybe `initForWorkspaceNotification`?

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.h:27:
andBundleId:(NSString*)bundleId;
andBundleId -> bundleId  (or maybe `forBundleId` but I don't think I ever see
`and` on arguments)

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.h:28: - (void)wait;
needs a comment. Needs to say stuff like ~immediately stops observing, so users
don't think they can call wait multiple times.

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.h:32: 
nit: remove extra blank line

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
File ui/base/test/windowed_nsnotification_observer.mm (right):

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.mm:7: #import
"base/mac/sdk_forward_declarations.h"
is this needed?

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.mm:10: -
(void)observe:(NSNotification*)notification;
maybe observe -> onNotificationMessage? or onNotification? or
notificationReceived?

(observe sounds too much like it's doing "AddObserver"

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.mm:16: if (self = [super init]) {
double brackets around assignment in an `if` .. usually there's a warning for
this :/

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.mm:27: if (self = [super init]) {
if ((self = ..

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.mm:28: bundleId_.reset([[bundleId
copy] retain]);
I don't think the retain is needed -- it's causing a refcount leak. (NSCopying
says "The returned object is implicitly retained by the sender")

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.mm:59: runLoop_.reset(new
base::RunLoop);
The run loop can go on the stack here. i.e. these lines become

base::RunLoop runLoop;
runLoop_ = &runLoop;
runLoop.Run();
runLoop_ = nullptr;

https://codereview.chromium.org/1146873002/diff/200001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1146873002/diff/200001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:51: enum RepostState {
Move this to within the RepostEventIfHandledByWindow() function?

https://codereview.chromium.org/1146873002/diff/200001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:121: // We're not in the middle of
reposting, process this new event.
nit: this comment kinda repeats the enum comment now - can probably drop it.

https://codereview.chromium.org/1146873002/diff/200001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:124: reposted_event_number = [ns_event
eventNumber];
= event_number?

https://codereview.chromium.org/1146873002/diff/200001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:159: NOTREACHED();
instead of this, maybe

DCHECK_EQ(REPOST_CANCELLED, repost_state); in place of the `if`

https://codereview.chromium.org/1146873002/diff/200001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:661: for (NSView* subview :
[[bridged_view_ superview] subviews]) {
does something like this work:

bool should_move_window = false;
NSView* hit_view = [[bridged_view_ superview] hitTest:location_in_window];
if (hit_view == bridged_view_) {
  gfx::Point point(..)
  should_move_window = GetNonClientComponent(point) == 
}
if (!should_move_window)
  return false;


(I'm not sure we need the [subview mouseDownCanMoveWindow] handling, since that
should be handled fine without reposting?

https://codereview.chromium.org/1146873002/diff/200001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget_interactive_uitest.mm (right):

https://codereview.chromium.org/1146873002/diff/200001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:200: // Pointer
because the handler block captures local variables by copying.
what about

base::RunLoop run_loop;
base::RunLoop* run_loop_ref = &run_loop;

https://codereview.chromium.org/1146873002/diff/200001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:238: };
DISALLOW_COPY_AND_ASSIGN(..)

https://codereview.chromium.org/1146873002/diff/200001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:253:
RunPendingMessages();
is this needed?

https://codereview.chromium.org/1146873002/diff/220001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_content_view.h (right):

https://codereview.chromium.org/1146873002/diff/220001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_content_view.h:53: @property(assign) BOOL
mouseDownCanMoveWindow;
nonatomic?

https://codereview.chromium.org/1146873002/diff/220001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget_interactive_uitest.mm (right):

https://codereview.chromium.org/1146873002/diff/220001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:14: #include
"ui/events/test/cocoa_test_event_utils.h"
nit: import

https://codereview.chromium.org/1146873002/diff/220001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:158:
HitTestBridgedNativeWidget(NativeWidgetMac* widget)
nit:explicit

https://codereview.chromium.org/1146873002/diff/220001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:161: bool
ShouldRepostPendingLeftMouseDown(NSPoint location_in_window) override {
nit: // BridgedNativeWidget:

https://codereview.chromium.org/1146873002/diff/220001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:173: void
WaitForRepost() {
maybe WaitForIsDraggableChange?

https://codereview.chromium.org/1146873002/diff/220001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:174:
run_loop_.reset(new base::RunLoop);
Same trick with a RunLoop on the stack?

https://codereview.chromium.org/1146873002/diff/220001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:180: };
nit: DISALLOW(..

https://codereview.chromium.org/1146873002/diff/220001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:185:
HitTestNonClientFrameView(Widget* widget)
nit: explciit

https://codereview.chromium.org/1146873002/diff/220001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:197: };
nit: DISALLOW_COPY_..

https://codereview.chromium.org/1146873002/diff/220001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:238: };
nit: DISALLOW_COPY..  (one of these comments is reduandant, but the comment's on
an older patchset :p)

https://codereview.chromium.org/1146873002/diff/220001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:299:
[widget.GetNativeWindow() deminiaturize:nil];
before this, EXPECT_TRUE([widget.GetNativeWindow() isMiniaturized])?

(and maybe an EXPECT_FALSE before the CGEventPost)

[jackhou1, 2015-06-03 08:13:54]

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
File ui/base/test/windowed_nsnotification_observer.h (right):

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.h:5: #ifndef
UI_GFX_TEST_WINDOWED_NSNOTIFICATION_OBSERVER_H_
On 2015/06/03 07:04:48, tapted wrote:
> UI_GFX_TEST -> UI_BASE_TEST

Done.

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.h:8: #import <Cocoa/Cocoa.h>
On 2015/06/03 07:04:48, tapted wrote:
> This should be <Foundation/Foundation.h> since AppKit isn't being used. All
you
> "need" is NSObject (even NSString could be forward-declared, but Foundation
will
> give it to you anyway, and you only #import NSObject.h directly in special
> cases.
> 
> (but you'll probably need AppKit.h or Cocoa.h in the .mm for NSWorkspace and
> things

Done.

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.h:11: #include
"base/memory/scoped_ptr.h"
On 2015/06/03 07:04:48, tapted wrote:
> nit: not needed (see below)

Done.

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.h:12: #include "base/run_loop.h"
On 2015/06/03 07:04:48, tapted wrote:
> nit: forward-declare

Done.

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.h:14: // Watches for
NSNotifications.
On 2015/06/03 07:04:49, tapted wrote:
> something more verbose? (i.e. "sell" this class - describe the situations
where
> someone would use it). Should say stuff like ~immediately starts listening
once
> created

Done.

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.h:15: @interface
WindowedNSNotificationObserver : NSObject {
On 2015/06/03 07:04:48, tapted wrote:
> Do you know the history behind the "Windowed" prefix? I don't think it's
right.
> 
> Maybe "CocoaNotificationMessageWaiter" ?

This is the same as WindowedNotificationObserver in
content/public/test/test_utils.h.

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.h:19: scoped_ptr<base::RunLoop>
runLoop_;
On 2015/06/03 07:04:48, tapted wrote:
> This can just be a raw pointer to base::RunLoop (see below)

Done.

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.h:22: // Watch for a notification
on the default notification center.
On 2015/06/03 07:04:48, tapted wrote:
> nit: "a notification" -> "an NSNotification"

Done.

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.h:24: // Watch for a notification
on the shared workspace notification center for the
On 2015/06/03 07:04:48, tapted wrote:
> nit: blank line before

Done.

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.h:26: -
(id)initForNotification:(NSString*)name
On 2015/06/03 07:04:48, tapted wrote:
> maybe `initForWorkspaceNotification`?

Done.

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.h:27:
andBundleId:(NSString*)bundleId;
On 2015/06/03 07:04:48, tapted wrote:
> andBundleId -> bundleId  (or maybe `forBundleId` but I don't think I ever see
> `and` on arguments)

Done.

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.h:28: - (void)wait;
On 2015/06/03 07:04:48, tapted wrote:
> needs a comment. Needs to say stuff like ~immediately stops observing, so
users
> don't think they can call wait multiple times.

Done.

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.h:32: 
On 2015/06/03 07:04:48, tapted wrote:
> nit: remove extra blank line

Done.

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
File ui/base/test/windowed_nsnotification_observer.mm (right):

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.mm:7: #import
"base/mac/sdk_forward_declarations.h"
On 2015/06/03 07:04:49, tapted wrote:
> is this needed?

Done.

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.mm:10: -
(void)observe:(NSNotification*)notification;
On 2015/06/03 07:04:49, tapted wrote:
> maybe observe -> onNotificationMessage? or onNotification? or
> notificationReceived?
> 
> (observe sounds too much like it's doing "AddObserver"

Done.

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.mm:16: if (self = [super init]) {
On 2015/06/03 07:04:49, tapted wrote:
> double brackets around assignment in an `if` .. usually there's a warning for
> this :/

Done.

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.mm:27: if (self = [super init]) {
On 2015/06/03 07:04:49, tapted wrote:
> if ((self = ..

Done.

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.mm:28: bundleId_.reset([[bundleId
copy] retain]);
On 2015/06/03 07:04:49, tapted wrote:
> I don't think the retain is needed -- it's causing a refcount leak. (NSCopying
> says "The returned object is implicitly retained by the sender")

Done.

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.mm:59: runLoop_.reset(new
base::RunLoop);
On 2015/06/03 07:04:49, tapted wrote:
> The run loop can go on the stack here. i.e. these lines become
> 
> base::RunLoop runLoop;
> runLoop_ = &runLoop;
> runLoop.Run();
> runLoop_ = nullptr;

Done.

https://codereview.chromium.org/1146873002/diff/200001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1146873002/diff/200001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:51: enum RepostState {
On 2015/06/03 07:04:49, tapted wrote:
> Move this to within the RepostEventIfHandledByWindow() function?

Done.

https://codereview.chromium.org/1146873002/diff/200001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:121: // We're not in the middle of
reposting, process this new event.
On 2015/06/03 07:04:49, tapted wrote:
> nit: this comment kinda repeats the enum comment now - can probably drop it.

Done.

https://codereview.chromium.org/1146873002/diff/200001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:124: reposted_event_number = [ns_event
eventNumber];
On 2015/06/03 07:04:49, tapted wrote:
> = event_number?

Done.

https://codereview.chromium.org/1146873002/diff/200001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:159: NOTREACHED();
On 2015/06/03 07:04:49, tapted wrote:
> instead of this, maybe
> 
> DCHECK_EQ(REPOST_CANCELLED, repost_state); in place of the `if`

Done.

https://codereview.chromium.org/1146873002/diff/200001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:661: for (NSView* subview :
[[bridged_view_ superview] subviews]) {
On 2015/06/03 07:04:49, tapted wrote:
> does something like this work:
> 
> bool should_move_window = false;
> NSView* hit_view = [[bridged_view_ superview] hitTest:location_in_window];
> if (hit_view == bridged_view_) {
>   gfx::Point point(..)
>   should_move_window = GetNonClientComponent(point) == 
> }
> if (!should_move_window)
>   return false;
> 
> 
> (I'm not sure we need the [subview mouseDownCanMoveWindow] handling, since
that
> should be handled fine without reposting?

The window controls are sometimes below the bridged_view_, so hitTest would
return bridged_view_.

https://codereview.chromium.org/1146873002/diff/200001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget_interactive_uitest.mm (right):

https://codereview.chromium.org/1146873002/diff/200001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:200: // Pointer
because the handler block captures local variables by copying.
On 2015/06/03 07:04:49, tapted wrote:
> what about
> 
> base::RunLoop run_loop;
> base::RunLoop* run_loop_ref = &run_loop;

Done.

https://codereview.chromium.org/1146873002/diff/200001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:238: };
On 2015/06/03 07:04:49, tapted wrote:
> DISALLOW_COPY_AND_ASSIGN(..)

Done.

https://codereview.chromium.org/1146873002/diff/200001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:253:
RunPendingMessages();
On 2015/06/03 07:04:49, tapted wrote:
> is this needed?

Done.

https://codereview.chromium.org/1146873002/diff/220001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_content_view.h (right):

https://codereview.chromium.org/1146873002/diff/220001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_content_view.h:53: @property(assign) BOOL
mouseDownCanMoveWindow;
On 2015/06/03 07:04:50, tapted wrote:
> nonatomic?

Done.

I removed it because it wouldn't build on 10.10 sdk with nonatomic, but it
should be fine for now.

https://codereview.chromium.org/1146873002/diff/220001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget_interactive_uitest.mm (right):

https://codereview.chromium.org/1146873002/diff/220001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:14: #include
"ui/events/test/cocoa_test_event_utils.h"
On 2015/06/03 07:04:50, tapted wrote:
> nit: import

Done.

https://codereview.chromium.org/1146873002/diff/220001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:158:
HitTestBridgedNativeWidget(NativeWidgetMac* widget)
On 2015/06/03 07:04:50, tapted wrote:
> nit:explicit

Done.

https://codereview.chromium.org/1146873002/diff/220001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:161: bool
ShouldRepostPendingLeftMouseDown(NSPoint location_in_window) override {
On 2015/06/03 07:04:50, tapted wrote:
> nit: // BridgedNativeWidget:

Done.

https://codereview.chromium.org/1146873002/diff/220001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:173: void
WaitForRepost() {
On 2015/06/03 07:04:50, tapted wrote:
> maybe WaitForIsDraggableChange?

Done.

https://codereview.chromium.org/1146873002/diff/220001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:174:
run_loop_.reset(new base::RunLoop);
On 2015/06/03 07:04:50, tapted wrote:
> Same trick with a RunLoop on the stack?

Done.

https://codereview.chromium.org/1146873002/diff/220001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:180: };
On 2015/06/03 07:04:50, tapted wrote:
> nit: DISALLOW(..

Done.

https://codereview.chromium.org/1146873002/diff/220001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:185:
HitTestNonClientFrameView(Widget* widget)
On 2015/06/03 07:04:50, tapted wrote:
> nit: explciit

Done.

https://codereview.chromium.org/1146873002/diff/220001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:197: };
On 2015/06/03 07:04:50, tapted wrote:
> nit: DISALLOW_COPY_..

Done.

https://codereview.chromium.org/1146873002/diff/220001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:238: };
On 2015/06/03 07:04:50, tapted wrote:
> nit: DISALLOW_COPY..  (one of these comments is reduandant, but the comment's
on
> an older patchset :p)

Done.

https://codereview.chromium.org/1146873002/diff/220001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget_interactive_uitest.mm:299:
[widget.GetNativeWindow() deminiaturize:nil];
On 2015/06/03 07:04:50, tapted wrote:
> before this, EXPECT_TRUE([widget.GetNativeWindow() isMiniaturized])?
> 
> (and maybe an EXPECT_FALSE before the CGEventPost)

Done.

[tapted, 2015-06-03 09:12:57]

lgtm!

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
File ui/base/test/windowed_nsnotification_observer.h (right):

https://codereview.chromium.org/1146873002/diff/200001/ui/base/test/windowed_...
ui/base/test/windowed_nsnotification_observer.h:15: @interface
WindowedNSNotificationObserver : NSObject {
On 2015/06/03 08:13:51, jackhou1 wrote:
> On 2015/06/03 07:04:48, tapted wrote:
> > Do you know the history behind the "Windowed" prefix? I don't think it's
> right.
> > 
> > Maybe "CocoaNotificationMessageWaiter" ?
> 
> This is the same as WindowedNotificationObserver in
> content/public/test/test_utils.h.

aah, sounds good.

https://codereview.chromium.org/1146873002/diff/200001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_native_widget.mm (right):

https://codereview.chromium.org/1146873002/diff/200001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_native_widget.mm:661: for (NSView* subview :
[[bridged_view_ superview] subviews]) {
On 2015/06/03 08:13:52, jackhou1 wrote:
> On 2015/06/03 07:04:49, tapted wrote:
> > does something like this work:
> > 
> > bool should_move_window = false;
> > NSView* hit_view = [[bridged_view_ superview] hitTest:location_in_window];
> > if (hit_view == bridged_view_) {
> >   gfx::Point point(..)
> >   should_move_window = GetNonClientComponent(point) == 
> > }
> > if (!should_move_window)
> >   return false;
> > 
> > 
> > (I'm not sure we need the [subview mouseDownCanMoveWindow] handling, since
> that
> > should be handled fine without reposting?
> 
> The window controls are sometimes below the bridged_view_, so hitTest would
> return bridged_view_.

Acknowledged.

https://codereview.chromium.org/1146873002/diff/220001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_content_view.h (right):

https://codereview.chromium.org/1146873002/diff/220001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_content_view.h:53: @property(assign) BOOL
mouseDownCanMoveWindow;
On 2015/06/03 08:13:52, jackhou1 wrote:
> On 2015/06/03 07:04:50, tapted wrote:
> > nonatomic?
> 
> Done.
> 
> I removed it because it wouldn't build on 10.10 sdk with nonatomic, but it
> should be fine for now.

Ah. Interesting. I think we are trying to keep 10.10 happy. E.g. there is a
10.10 bot which I think is currently green - just fired off a job.

What was the compile error?

I see in 10.9 it's a function and in 10.10 it's a readonly property (an atomic
readonly property, which I guess you can't override with nonatomic properties).

Perhaps leave it atomic, but add a comment like

// Extends an atomic, readonly property on NSView to make it assignable.

[jackhou1, 2015-06-03 09:32:48]

https://codereview.chromium.org/1146873002/diff/220001/ui/views/cocoa/bridged...
File ui/views/cocoa/bridged_content_view.h (right):

https://codereview.chromium.org/1146873002/diff/220001/ui/views/cocoa/bridged...
ui/views/cocoa/bridged_content_view.h:53: @property(assign) BOOL
mouseDownCanMoveWindow;
On 2015/06/03 09:12:57, tapted wrote:
> On 2015/06/03 08:13:52, jackhou1 wrote:
> > On 2015/06/03 07:04:50, tapted wrote:
> > > nonatomic?
> > 
> > Done.
> > 
> > I removed it because it wouldn't build on 10.10 sdk with nonatomic, but it
> > should be fine for now.
> 
> Ah. Interesting. I think we are trying to keep 10.10 happy. E.g. there is a
> 10.10 bot which I think is currently green - just fired off a job.
> 
> What was the compile error?
> 
> I see in 10.9 it's a function and in 10.10 it's a readonly property (an atomic
> readonly property, which I guess you can't override with nonatomic
properties).
> 
> Perhaps leave it atomic, but add a comment like
> 
> // Extends an atomic, readonly property on NSView to make it assignable.

Done.

[jackhou1, 2015-06-03 09:37:38]

jackhou@chromium.org changed reviewers:
+ sky@chromium.org

[jackhou1, 2015-06-03 09:37:39]

thakis, please review for OWNERS in:

chrome/browser/ui/views/frame/
ui/base/

[jackhou1, 2015-06-03 09:39:07]

jackhou@chromium.org changed reviewers:
+ thakis@chromium.org

[jackhou1, 2015-06-03 09:39:08]

Oops, that should be
sky for 
chrome/browser/ui/views/frame/

thakis, for 
ui/base/

[sky, 2015-06-03 16:00:36]

LGTM

[jackhou1, 2015-06-05 01:25:55]

Hi Nico, could you take a quick look at ui/base/?

It's based on WindowedNotificationObserver here:
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes...

[Nico, 2015-06-08 23:56:31]

lgtm

Sorry for the delay.

(tapted, feel free to add yourself to any cocoa-y owners files that i'm in)

[jackhou1, 2015-06-08 23:56:56]

The CQ bit was checked by jackhou@chromium.org

[jackhou1, 2015-06-08 23:56:56]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1146873002/#ps280001
(title: "Sync and rebase")

[commit-bot: I haz the power, 2015-06-08 23:57:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1146873002/280001

[commit-bot: I haz the power, 2015-06-09 00:57:08]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-06-09 00:58:11]

Patchset 15 (id:??) landed as
https://crrev.com/dc783c7be84a2a8c5564736f4b17a4fbd03897d6
Cr-Commit-Position: refs/heads/master@{#333410}