MacViews: Implement event monitoring for a specific window

MacViews: Implement event monitoring for a specific window

ManagePasswordBubbleView wants to monitor events on a specific window,
currently using aura specific code that won't work for MacViews.
views::EventMonitor is enhanced to support monitoring events on a window,
with both Mac and Aura implementations.

Also includes minor compile fixes in ZoomBubbleView and StatusBubbleView.

BUG=425229
TEST=EventMonitorAuraTest, EventMonitorMacTest

Committed: https://crrev.com/e393fdb1d25ea25693a905aa4af97ed569bb2223
Cr-Commit-Position: refs/heads/master@{#307960}

[Andre, 2014-11-20 21:55:07]

Patchset #4 (id:60001) has been deleted

[Andre, 2014-11-20 21:55:14]

Patchset #3 (id:40001) has been deleted

[Andre, 2014-11-20 21:55:19]

Patchset #1 (id:1) has been deleted

[Andre, 2014-11-20 21:55:25]

Patchset #1 (id:20001) has been deleted

[Andre, 2014-11-20 22:04:22]

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

[Andre, 2014-11-20 22:04:23]

Trent, please take an early look and let me know what you think (this is 
unfinished).
In particular, I'm considering converting EventMonitorMacTest to use
EventGenerator and make it work cross platform.
But, EventMonitorMac won't work with the way
EventGeneratorDelegateMac::EmulateSendEvent is implemented.
Changing it to use [NSApp sendEvent:] works though.

[tapted, 2014-11-20 22:39:02]

https://codereview.chromium.org/730833006/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/730833006/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:779: if
(is_fullscreen) {
nit: no curlies

https://codereview.chromium.org/730833006/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/status_bubble_views.cc (right):

https://codereview.chromium.org/730833006/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/status_bubble_views.cc:591: set_ignored_by_shelf(true);
This one is interesting... I think this path is currently used for desktop
widgets too, which seem like they should ignore ash::wm::GetWindowState

Maybe this needs to be something on views::Widget that does (on Mac) [NSWindow
setExcludedFromWindowsMenu:], and NSWindowCollectionBehaviorIgnoresCycle
NSWindowCollectionBehaviorTransient  (or something with windowLevel). And then
something like that which works for win/linux desktop

But all that's orthogonal - this #ifdef seems OK for now

https://codereview.chromium.org/730833006/diff/80001/ui/views/event_monitor_a...
File ui/views/event_monitor_aura.cc (right):

https://codereview.chromium.org/730833006/diff/80001/ui/views/event_monitor_a...
ui/views/event_monitor_aura.cc:16: return new EventMonitorAura(event_handler,
aura::Env::GetInstance());
Seems almost odd that aura::Env::GetInstance() works here. GetEventTargeter()
just does NOTREACHED. Do we need the one-argument Create at all? Aura should
probably pass a RootWindow if it doesn't have a specific window in mind.

On Mac, we can say "pass nil to monitor all events sent to the application"

https://codereview.chromium.org/730833006/diff/80001/ui/views/event_monitor_m...
File ui/views/event_monitor_mac.mm (right):

https://codereview.chromium.org/730833006/diff/80001/ui/views/event_monitor_m...
ui/views/event_monitor_mac.mm:31: return gfx::Point(mouseLocation.x,
mouseLocation.y);
also orthogonal, but we probably should move this snippet to
ui/gfx/mac/coordinate_conversion.h at some point

https://codereview.chromium.org/730833006/diff/80001/ui/views/event_monitor_m...
ui/views/event_monitor_mac.mm:39: if (!target_window || [event window] ==
target_window) {
Can the EventMonitorMac outlive this NSWindow?

(Maybe this is something to plonk on BridgedNativeWidget, which is kinda like
our aura::Window)

[tapted, 2014-11-20 23:03:02]

On 2014/11/20 22:39:02, tapted wrote:
> (Maybe this is something to plonk on BridgedNativeWidget, which is kinda like
> our aura::Window)

btw - I'm thinking adding a function to NativeWidgetMac which might help for
this

in this CL: http://crbug.com/731053002 which I'm about to split up (reimplement
child windows + support visibility changes). Getting that WidgetTest.Visibility
test to pass turned out to be pretty complicated.

[tapted, 2014-11-20 23:03:47]

On 2014/11/20 23:03:02, tapted wrote:
> in this CL: http://crbug.com/731053002

uhh http://crrev.com/731053002

[Andre, 2014-11-21 00:28:29]

WDYT about EventGeneratorDelegateMac::EmulateSendEvent vs -[NSApp sendEvent]?

https://codereview.chromium.org/730833006/diff/80001/ui/views/event_monitor_a...
File ui/views/event_monitor_aura.cc (right):

https://codereview.chromium.org/730833006/diff/80001/ui/views/event_monitor_a...
ui/views/event_monitor_aura.cc:16: return new EventMonitorAura(event_handler,
aura::Env::GetInstance());
On 2014/11/20 22:39:02, tapted wrote:
> Seems almost odd that aura::Env::GetInstance() works here. GetEventTargeter()
> just does NOTREACHED. Do we need the one-argument Create at all? Aura should
> probably pass a RootWindow if it doesn't have a specific window in mind.
> 
> On Mac, we can say "pass nil to monitor all events sent to the application"

It seems to be the current pattern to use aura::Env to monitor app events.
Will RootWindow have the same effect?

https://codereview.chromium.org/730833006/diff/80001/ui/views/event_monitor_m...
File ui/views/event_monitor_mac.mm (right):

https://codereview.chromium.org/730833006/diff/80001/ui/views/event_monitor_m...
ui/views/event_monitor_mac.mm:31: return gfx::Point(mouseLocation.x,
mouseLocation.y);
On 2014/11/20 22:39:02, tapted wrote:
> also orthogonal, but we probably should move this snippet to
> ui/gfx/mac/coordinate_conversion.h at some point

Yeah, we should add ScreenPointFromNSPoint there.

https://codereview.chromium.org/730833006/diff/80001/ui/views/event_monitor_m...
ui/views/event_monitor_mac.mm:39: if (!target_window || [event window] ==
target_window) {
On 2014/11/20 22:39:02, tapted wrote:
> Can the EventMonitorMac outlive this NSWindow?
> 
> (Maybe this is something to plonk on BridgedNativeWidget, which is kinda like
> our aura::Window)

There is that risk, same with EventMonitorAura as well.
We can use something like NativeWindowTracker, I suppose.
Which part are you thinking to plonk on BridgedNativeWidget?
I see you are adding GetBridgeForNativeWindow, but how will that help is the
window is already destroyed?

[tapted, 2014-11-21 00:57:34]

On 2014/11/21 00:28:29, Andre wrote:
> WDYT about EventGeneratorDelegateMac::EmulateSendEvent vs -[NSApp sendEvent]?

When I tried using -[NSWindow sendEvent], it would block indefinitely on some
events because a UI thread wasn't spinning. [NSApp sendEvent] will probably do
the same, but maybe it's ok for some event types.

However, I'm pretty sure we want to keep EventGeneratorDelegateMac avoiding
-[NSApp sendEvent]. ui::test::EventGenerator shouldn't be used for anything that
requires a an interactive_ui_test.

E.g. I'm pretty sure sending a mouse event through NSApp will alter the
behaviour (e.g. [event window]) depending what random things pop up on screen
while all the unittests are running in parallel.

https://codereview.chromium.org/730833006/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/730833006/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:734:
event_monitor_.reset(views::EventMonitor::Create(this, target_window));
can you make the views::EventMonitor::Create* return a scoped_ptr? I think
that's the way to indicate an ownership transfer. Then here I think you can just
do 

event_monitor_ = views::EventMonitor::Create(..)

https://codereview.chromium.org/730833006/diff/80001/ui/views/event_monitor_a...
File ui/views/event_monitor_aura.cc (right):

https://codereview.chromium.org/730833006/diff/80001/ui/views/event_monitor_a...
ui/views/event_monitor_aura.cc:16: return new EventMonitorAura(event_handler,
aura::Env::GetInstance());
On 2014/11/21 00:28:28, Andre wrote:
> On 2014/11/20 22:39:02, tapted wrote:
> > Seems almost odd that aura::Env::GetInstance() works here.
GetEventTargeter()
> > just does NOTREACHED. Do we need the one-argument Create at all? Aura should
> > probably pass a RootWindow if it doesn't have a specific window in mind.
> > 
> > On Mac, we can say "pass nil to monitor all events sent to the application"
> 
> It seems to be the current pattern to use aura::Env to monitor app events.
> Will RootWindow have the same effect?

Ah, I poked around in http://crrev.com/678823002 . Yeah -
aura::Env::GetInstance() seems like the way it's done. And requiring the second
argument would make encapsulation harder. So if it works ignore me :).

But maybe it could be clearer with something like

EventMonitor::CreateApplicaitonMonitor(ui::EventHandler*)
EventMonitor::CreateWindowMonitor(ui::EventHandler*, gfx::NativeWindow)

WDYT?

https://codereview.chromium.org/730833006/diff/80001/ui/views/event_monitor_m...
File ui/views/event_monitor_mac.mm (right):

https://codereview.chromium.org/730833006/diff/80001/ui/views/event_monitor_m...
ui/views/event_monitor_mac.mm:39: if (!target_window || [event window] ==
target_window) {
On 2014/11/21 00:28:28, Andre wrote:
> On 2014/11/20 22:39:02, tapted wrote:
> > Can the EventMonitorMac outlive this NSWindow?
> > 
> > (Maybe this is something to plonk on BridgedNativeWidget, which is kinda
like
> > our aura::Window)
> 
> There is that risk, same with EventMonitorAura as well.
> We can use something like NativeWindowTracker, I suppose.
> Which part are you thinking to plonk on BridgedNativeWidget?
> I see you are adding GetBridgeForNativeWindow, but how will that help is the
> window is already destroyed?

WidgetObserver might be nicer, in which case these Create functions could take a
Widget* instead.

GetBridgeForNativeWindow would allow you to call a function like
BridgedNativeWidget::AddPreTargetHandler


If you make it clear on the added Create method (i.e. comment) that the
EventMonitor must be destroyed before |target_window|, it might be fine as-is.
No point over-engineering if the callers are already doing this (and getting a
Widget* from webcontents might be tricky)

[Andre, 2014-11-22 01:56:22]

Patchset #3 (id:120001) has been deleted

[Andre, 2014-12-08 21:23:58]

Patchset #7 (id:220001) has been deleted

[Andre, 2014-12-09 18:56:59]

Patchset #7 (id:240001) has been deleted

[Andre, 2014-12-09 18:57:09]

Patchset #6 (id:200001) has been deleted

[Andre, 2014-12-09 23:50:11]

Patchset #3 (id:140001) has been deleted

[Andre, 2014-12-09 23:50:19]

Patchset #3 (id:160001) has been deleted

[Andre, 2014-12-09 23:50:27]

Patchset #3 (id:180001) has been deleted

[Andre, 2014-12-09 23:51:05]

Patchset #2 (id:100001) has been deleted

[Andre, 2014-12-10 00:18:13]

Trent PTAL, this is ready for a more thorough review.
It would be nice if the test code are not duplicated between Mac and Aura.
One idea might be to add a property on EventGenerator to choose between
native and emulated event sending.
Do you have other ideas?

https://codereview.chromium.org/730833006/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/730833006/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:734:
event_monitor_.reset(views::EventMonitor::Create(this, target_window));
On 2014/11/21 00:57:34, tapted wrote:
> can you make the views::EventMonitor::Create* return a scoped_ptr? I think
> that's the way to indicate an ownership transfer. Then here I think you can
just
> do 
> 
> event_monitor_ = views::EventMonitor::Create(..) 

Done.

https://codereview.chromium.org/730833006/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:779: if
(is_fullscreen) {
On 2014/11/20 22:39:02, tapted wrote:
> nit: no curlies

Done.

https://codereview.chromium.org/730833006/diff/80001/ui/views/event_monitor_a...
File ui/views/event_monitor_aura.cc (right):

https://codereview.chromium.org/730833006/diff/80001/ui/views/event_monitor_a...
ui/views/event_monitor_aura.cc:16: return new EventMonitorAura(event_handler,
aura::Env::GetInstance());
On 2014/11/21 00:57:34, tapted wrote:
> On 2014/11/21 00:28:28, Andre wrote:
> > On 2014/11/20 22:39:02, tapted wrote:
> > > Seems almost odd that aura::Env::GetInstance() works here.
> GetEventTargeter()
> > > just does NOTREACHED. Do we need the one-argument Create at all? Aura
should
> > > probably pass a RootWindow if it doesn't have a specific window in mind.
> > > 
> > > On Mac, we can say "pass nil to monitor all events sent to the
application"
> > 
> > It seems to be the current pattern to use aura::Env to monitor app events.
> > Will RootWindow have the same effect?
> 
> Ah, I poked around in http://crrev.com/678823002 . Yeah -
> aura::Env::GetInstance() seems like the way it's done. And requiring the
second
> argument would make encapsulation harder. So if it works ignore me :).
> 
> But maybe it could be clearer with something like
> 
> EventMonitor::CreateApplicaitonMonitor(ui::EventHandler*)
> EventMonitor::CreateWindowMonitor(ui::EventHandler*, gfx::NativeWindow)
> 
> WDYT?

Done.

https://codereview.chromium.org/730833006/diff/80001/ui/views/event_monitor_m...
File ui/views/event_monitor_mac.mm (right):

https://codereview.chromium.org/730833006/diff/80001/ui/views/event_monitor_m...
ui/views/event_monitor_mac.mm:39: if (!target_window || [event window] ==
target_window) {
On 2014/11/21 00:57:34, tapted wrote:
> On 2014/11/21 00:28:28, Andre wrote:
> > On 2014/11/20 22:39:02, tapted wrote:
> > > Can the EventMonitorMac outlive this NSWindow?
> > > 
> > > (Maybe this is something to plonk on BridgedNativeWidget, which is kinda
> like
> > > our aura::Window)
> > 
> > There is that risk, same with EventMonitorAura as well.
> > We can use something like NativeWindowTracker, I suppose.
> > Which part are you thinking to plonk on BridgedNativeWidget?
> > I see you are adding GetBridgeForNativeWindow, but how will that help is the
> > window is already destroyed?
> 
> WidgetObserver might be nicer, in which case these Create functions could take
a
> Widget* instead.
> 
> GetBridgeForNativeWindow would allow you to call a function like
> BridgedNativeWidget::AddPreTargetHandler
> 
> 
> If you make it clear on the added Create method (i.e. comment) that the
> EventMonitor must be destroyed before |target_window|, it might be fine as-is.
> No point over-engineering if the callers are already doing this (and getting a
> Widget* from webcontents might be tricky)

Done.
Added the comment.

[tapted, 2014-12-10 02:58:27]

On 2014/12/10 00:18:13, Andre wrote:
> It would be nice if the test code are not duplicated between Mac and Aura.
> One idea might be to add a property on EventGenerator to choose between
> native and emulated event sending.
> Do you have other ideas?

Yeah - agree the duplication isn't great. I had a poke around, an event
generator property sounds good. Or an EventGeneratorDelegate property
(EventGeneratorDelegateAura doesn't currently hold on to the EventGenerator that
owns it in SetContext, but it could, which would make an EventGenerator property
viable *IF* we decide Aura even needs to obey it.

E.g. for *ASH* (not Aura) it might look like

ui::EventTarget* EventGeneratorDelegateAsh::GetTargetAt(
    const gfx::Point& location) {
  if (owner_->targeting_application())
    return ash::Shell::GetInstance();
  return EventGeneratorDelegateAura::GetTargetAt(location);
}

and on Mac, 

void EventGeneratorDelegateMac::OnMouseEvent(ui::MouseEvent* event) {
  // For mouse drag events, ensure the swizzled methods return the right flags.
  base::AutoReset<ui::test::EventGenerator*> reset(&g_active_generator, owner_);
  NSEvent* event = CreateMouseEventInWindow(window_,
                                            event->type(),
                                            event->location(),
                                            event->changed_button_flags());
  if (owner_->targeting_application())
    [NSApp sendEvent:event];
  else
    EmulateSendEvent(window_, event);
}



But of course things would need to be mixed up a lot if we wanted
`EventGeneratorDelegateAsh`, and there's no point if we don't need it. Perhaps
just a TODO in EventGeneratorDelegateAura::GetTargetAt()

I think sadrul is the events expert :). He might have some insight into whether
EventGeneratorDelegateAsh could also be useful for tests.

https://codereview.chromium.org/730833006/diff/260001/ui/views/event_monitor_...
File ui/views/event_monitor_aura_unittest.cc (right):

https://codereview.chromium.org/730833006/diff/260001/ui/views/event_monitor_...
ui/views/event_monitor_aura_unittest.cc:15: void SetUp() override {
nit: // testing::Test:

https://codereview.chromium.org/730833006/diff/260001/ui/views/event_monitor_...
ui/views/event_monitor_aura_unittest.cc:29: Widget* widget_;
nit: Initialize widget_(nullptr) in a default constructor

https://codereview.chromium.org/730833006/diff/260001/ui/views/event_monitor_...
ui/views/event_monitor_aura_unittest.cc:32: };
nit: private: DISALLOW_COPY_AND_ASSIGN(..)

https://codereview.chromium.org/730833006/diff/260001/ui/views/event_monitor_...
File ui/views/event_monitor_mac.h (right):

https://codereview.chromium.org/730833006/diff/260001/ui/views/event_monitor_...
ui/views/event_monitor_mac.h:11: #import <Cocoa/Cocoa.h>
nit: move this to .cc?

And maybe just #include native_widge_types for iwyu

https://codereview.chromium.org/730833006/diff/260001/ui/views/event_monitor_...
File ui/views/event_monitor_mac.mm (right):

https://codereview.chromium.org/730833006/diff/260001/ui/views/event_monitor_...
ui/views/event_monitor_mac.mm:5: #include "ui/views/event_monitor_mac.h"
nit: import (since it has an `id`)

https://codereview.chromium.org/730833006/diff/260001/ui/views/event_monitor_...
ui/views/event_monitor_mac.mm:42: if (!target_window || [event window] ==
target_window) {
So block formatting was taken out of the official style guide because reasons..
clang-format is a bit rubbish since if the argument name is long, it squashes
everything to the right of it. That's kinda happening here.

I think Chrome's preference is to use a 4-space indent from line that has the
opening curly brace, and line up the close curly brace with the start of that
line. (also: a space before the opening curly brace). Here I'd go for

monitor_ = [NSEvent addLocalMonitorForEventsMatchingMask:NSAnyEventMask
    handler:^NSEvent*(NSEvent* event) {
        if (!target_window || [event window] == target_window) {
          scoped_ptr<ui::Event> ui_event = ui::EventFromNative(event);
          event_handler->OnEvent(ui_event.get());
        }
        return event;
    }];


references: `git grep -A5 ':^' -- '*.mm'` and http://cl/25169862

https://codereview.chromium.org/730833006/diff/260001/ui/views/event_monitor_...
File ui/views/event_monitor_mac_unittest.mm (right):

https://codereview.chromium.org/730833006/diff/260001/ui/views/event_monitor_...
ui/views/event_monitor_mac_unittest.mm:16: TestEventGenerator(gfx::NativeWindow
window) : window_(window) {}
nit: explicit

https://codereview.chromium.org/730833006/diff/260001/ui/views/event_monitor_...
ui/views/event_monitor_mac_unittest.mm:18: void ClickLeftButton() {
Could this just be a member function of EventMonitorMacTest that calls
MouseClickInView([widget_->GetNativeView(), ..)?

https://codereview.chromium.org/730833006/diff/260001/ui/views/event_monitor_...
ui/views/event_monitor_mac_unittest.mm:22: [NSApp sendEvent:events.second];
I'm not totally confident that this is non-flaky. E.g. if another window pops up
on screen while all the tests are being parallelized, does Cocoa still send this
to the window? But I just did almost exactly this in
https://codereview.chromium.org/789763002/diff/60001/ui/views/cocoa/cocoa_mou...
and wasn't able to make the test flake locally, so I'm hoping this will keep
working fine. See also comments at the start of ui/base/test/ui_controls_mac.mm
-- maybe it's only broken for dragging.

https://codereview.chromium.org/730833006/diff/260001/ui/views/event_monitor_...
ui/views/event_monitor_mac_unittest.mm:27: };
nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/730833006/diff/260001/ui/views/event_monitor_...
ui/views/event_monitor_mac_unittest.mm:36: void SetUp() override {
nit: // testing::Test:

https://codereview.chromium.org/730833006/diff/260001/ui/views/event_monitor_...
ui/views/event_monitor_mac_unittest.mm:49: Widget* widget_;
nit: initialize widget_(nullptr)

https://codereview.chromium.org/730833006/diff/260001/ui/views/event_monitor_...
ui/views/event_monitor_mac_unittest.mm:52: };
nit: private: DISALLOW.. + constructor

[Andre, 2014-12-10 22:56:16]

Patchset #3 (id:280001) has been deleted

[Andre, 2014-12-10 22:58:05]

Thanks!
I added a property to EventGenerator, PTAL.

https://codereview.chromium.org/730833006/diff/260001/ui/views/event_monitor_...
File ui/views/event_monitor_aura_unittest.cc (right):

https://codereview.chromium.org/730833006/diff/260001/ui/views/event_monitor_...
ui/views/event_monitor_aura_unittest.cc:15: void SetUp() override {
On 2014/12/10 02:58:27, tapted wrote:
> nit: // testing::Test:

Done.

https://codereview.chromium.org/730833006/diff/260001/ui/views/event_monitor_...
ui/views/event_monitor_aura_unittest.cc:15: void SetUp() override {
On 2014/12/10 02:58:27, tapted wrote:
> nit: // testing::Test:

Done.

https://codereview.chromium.org/730833006/diff/260001/ui/views/event_monitor_...
ui/views/event_monitor_aura_unittest.cc:29: Widget* widget_;
On 2014/12/10 02:58:27, tapted wrote:
> nit: Initialize widget_(nullptr) in a default constructor

Done.

https://codereview.chromium.org/730833006/diff/260001/ui/views/event_monitor_...
ui/views/event_monitor_aura_unittest.cc:32: };
On 2014/12/10 02:58:27, tapted wrote:
> nit: private: DISALLOW_COPY_AND_ASSIGN(..)

Done.

https://codereview.chromium.org/730833006/diff/260001/ui/views/event_monitor_...
File ui/views/event_monitor_mac.h (right):

https://codereview.chromium.org/730833006/diff/260001/ui/views/event_monitor_...
ui/views/event_monitor_mac.h:11: #import <Cocoa/Cocoa.h>
On 2014/12/10 02:58:27, tapted wrote:
> nit: move this to .cc?
> 
> And maybe just #include native_widge_types for iwyu

Done.

https://codereview.chromium.org/730833006/diff/260001/ui/views/event_monitor_...
File ui/views/event_monitor_mac.mm (right):

https://codereview.chromium.org/730833006/diff/260001/ui/views/event_monitor_...
ui/views/event_monitor_mac.mm:5: #include "ui/views/event_monitor_mac.h"
On 2014/12/10 02:58:27, tapted wrote:
> nit: import (since it has an `id`)

Done.

https://codereview.chromium.org/730833006/diff/260001/ui/views/event_monitor_...
ui/views/event_monitor_mac.mm:42: if (!target_window || [event window] ==
target_window) {
On 2014/12/10 02:58:27, tapted wrote:
> So block formatting was taken out of the official style guide because
reasons..
> clang-format is a bit rubbish since if the argument name is long, it squashes
> everything to the right of it. That's kinda happening here.
> 
> I think Chrome's preference is to use a 4-space indent from line that has the
> opening curly brace, and line up the close curly brace with the start of that
> line. (also: a space before the opening curly brace). Here I'd go for
> 
> monitor_ = [NSEvent addLocalMonitorForEventsMatchingMask:NSAnyEventMask
>     handler:^NSEvent*(NSEvent* event) {
>         if (!target_window || [event window] == target_window) {
>           scoped_ptr<ui::Event> ui_event = ui::EventFromNative(event);
>           event_handler->OnEvent(ui_event.get());
>         }
>         return event;
>     }];
> 
> 
> references: `git grep -A5 ':^' -- '*.mm'` and http://cl/25169862

Done.

https://codereview.chromium.org/730833006/diff/260001/ui/views/event_monitor_...
File ui/views/event_monitor_mac_unittest.mm (right):

https://codereview.chromium.org/730833006/diff/260001/ui/views/event_monitor_...
ui/views/event_monitor_mac_unittest.mm:16: TestEventGenerator(gfx::NativeWindow
window) : window_(window) {}
On 2014/12/10 02:58:27, tapted wrote:
> nit: explicit

Deleted, no longer needed.

[Andre, 2014-12-10 22:59:14]

Sadrul, PTAL for ui.
See Trent's comment above regarding EventGeneratorDelegateAsh.

[Andre, 2014-12-10 23:00:40]

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

[Andre, 2014-12-10 23:00:40]

Oops Sadrul is OOO.
sky@ can you take a look for OWNERS?

[tapted, 2014-12-10 23:13:38]

lgtm (yay cross-platform tests \o/)

[sky, 2014-12-11 00:49:43]

https://codereview.chromium.org/730833006/diff/300001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/730833006/diff/300001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:845: if
(web_contents) {
Should this be a DCHECK? Without a webcontents won't this be severly limited?
Seems like the old code assumed a WebContents anyway.

https://codereview.chromium.org/730833006/diff/300001/ui/views/event_monitor.h
File ui/views/event_monitor.h (right):

https://codereview.chromium.org/730833006/diff/300001/ui/views/event_monitor....
ui/views/event_monitor.h:19: // RAII-style class that forwards events posted to
the application to
nit: update description to reflect changes.

[Andre, 2014-12-11 18:09:12]

https://codereview.chromium.org/730833006/diff/300001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right):

https://codereview.chromium.org/730833006/diff/300001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:845: if
(web_contents) {
On 2014/12/11 00:49:43, sky wrote:
> Should this be a DCHECK? Without a webcontents won't this be severly limited?
> Seems like the old code assumed a WebContents anyway.

Done.
I also removed the local variable target_window.

https://codereview.chromium.org/730833006/diff/300001/ui/views/event_monitor.h
File ui/views/event_monitor.h (right):

https://codereview.chromium.org/730833006/diff/300001/ui/views/event_monitor....
ui/views/event_monitor.h:19: // RAII-style class that forwards events posted to
the application to
On 2014/12/11 00:49:43, sky wrote:
> nit: update description to reflect changes.

Done.

[sky, 2014-12-11 19:02:24]

LGTM

[Andre, 2014-12-11 19:07:10]

The CQ bit was checked by andresantoso@chromium.org

[commit-bot: I haz the power, 2014-12-11 19:08:07]

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

[commit-bot: I haz the power, 2014-12-11 20:21:39]

Committed patchset #4 (id:320001)

[commit-bot: I haz the power, 2014-12-11 20:22:20]

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

[jam, 2014-12-11 22:45:20]

btw I reverted this manually because it was causing views_unittests
failures. i.e. see
http://build.chromium.org/p/chromium.win/builders/Win8%20Aura/builds/25083/st...

On Thu, Dec 11, 2014 at 12:22 PM, <commit-bot@chromium.org> wrote:

> Patchset 4 (id:??) landed as
> https://crrev.com/e393fdb1d25ea25693a905aa4af97ed569bb2223
> Cr-Commit-Position: refs/heads/master@{#307960}
>
> https://codereview.chromium.org/730833006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jam, 2014-12-11 22:45:50]

also other builders too, i.e.
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...

On Thu, Dec 11, 2014 at 2:45 PM, John Abd-El-Malek <jam@chromium.org> wrote:

> btw I reverted this manually because it was causing views_unittests
> failures. i.e. see
>
>
http://build.chromium.org/p/chromium.win/builders/Win8%20Aura/builds/25083/st...
>
> On Thu, Dec 11, 2014 at 12:22 PM, <commit-bot@chromium.org> wrote:
>
>> Patchset 4 (id:??) landed as
>> https://crrev.com/e393fdb1d25ea25693a905aa4af97ed569bb2223
>> Cr-Commit-Position: refs/heads/master@{#307960}
>>
>> https://codereview.chromium.org/730833006/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.