Consolidate message observer API for win and aura (and touch).

Consolidate message observer API for MessagePumpX and MessagePumWin

This is an attempt to simplify the code around message pump observer.

BUG=none
TEST=none

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

[oshima, 2011-09-23 18:33:56]

Ben, sadrul, this is an attempt to consolidate MessagePumpObjserver and I'd like
to hear your opinion.

Once we fully migrate, we can eliminate Gtk and can just
use WillProcessEevent(const NativeEvent&) DidProcessEvent(const NativeEvent&)

this has two NativeEvent definition (base and views) because
touch has NativeEvent as GdkEvent, but this can also be consolidated  one
migration is done.

[sadrul, 2011-09-23 18:44:06]

+msw

There is also going to be ui::NativeEvent
(http://codereview.chromium.org/7942004/). views::NativeEvent will probably
become XEvent* unconditionally too (the same CL). So that may make things a
little easier. However, having NativeEvent in base::, ui:: and views:: ... I
suspect this is going to become way too confusing. I do like the idea of
consolidating the MLObserver API, and I don't have a better suggestion (perhaps
always use base::NativeEvent everywhere, and deprecate views::NativeEvent and
not introduce ui::NativeEvent)?

[oshima, 2011-09-23 18:49:10]

On 2011/09/23 18:44:06, sadrul wrote:
> +msw
> 
> There is also going to be ui::NativeEvent
> (http://codereview.chromium.org/7942004/). views::NativeEvent will probably
> become XEvent* unconditionally too (the same CL). So that may make things a
> little easier. However, having NativeEvent in base::, ui:: and views:: ... I
> suspect this is going to become way too confusing.

Agree. This was just a temporary solution, and was necessary because Touch has
two different NativeEvents.

> I do like the idea of
> consolidating the MLObserver API, and I don't have a better suggestion
(perhaps
> always use base::NativeEvent everywhere, and deprecate views::NativeEvent and
> not introduce ui::NativeEvent)?

Given that this is the root component that needs to access Event type,
this may be a good place for NativeEvent. (just NativeEvent, not others).
mike, what do you think?

[msw, 2011-09-23 19:07:23]

On 2011/09/23 18:49:10, oshima wrote:
> On 2011/09/23 18:44:06, sadrul wrote:
> > +msw
> > 
> > There is also going to be ui::NativeEvent
> > (http://codereview.chromium.org/7942004/). views::NativeEvent will probably
> > become XEvent* unconditionally too (the same CL). So that may make things a
> > little easier. However, having NativeEvent in base::, ui:: and views:: ... I
> > suspect this is going to become way too confusing.
> 
> Agree. This was just a temporary solution, and was necessary because Touch has
> two different NativeEvents.
> 
> > I do like the idea of
> > consolidating the MLObserver API, and I don't have a better suggestion
> (perhaps
> > always use base::NativeEvent everywhere, and deprecate views::NativeEvent
and
> > not introduce ui::NativeEvent)?
> 
> Given that this is the root component that needs to access Event type,
> this may be a good place for NativeEvent. (just NativeEvent, not others).
> mike, what do you think?

If we can define base::NativeEvent as MSG/XEvent/WaylandEvent, then we can do
away with the ui::NativeEvent that I'm adding in:
http://codereview.chromium.org/7942004/diff/8006/ui/base/events.h
Afaict, we'll still need views::NativeEvent for the aura::Event* case:
http://codereview.chromium.org/7942004/diff/34049/views/events/event.h

(I'm pretty close to removing NativeEvent2 and just using GdkEvent where needed,
this was a good exercise in auditing and marking the GdkEvent locations for
later removal).

[msw, 2011-09-23 19:10:14]

On 2011/09/23 19:07:23, msw wrote:
> On 2011/09/23 18:49:10, oshima wrote:
> > On 2011/09/23 18:44:06, sadrul wrote:
> > > +msw
> > > 
> > > There is also going to be ui::NativeEvent
> > > (http://codereview.chromium.org/7942004/). views::NativeEvent will
probably
> > > become XEvent* unconditionally too (the same CL). So that may make things
a
> > > little easier. However, having NativeEvent in base::, ui:: and views:: ...
I
> > > suspect this is going to become way too confusing.
> > 
> > Agree. This was just a temporary solution, and was necessary because Touch
has
> > two different NativeEvents.
> > 
> > > I do like the idea of
> > > consolidating the MLObserver API, and I don't have a better suggestion
> > (perhaps
> > > always use base::NativeEvent everywhere, and deprecate views::NativeEvent
> and
> > > not introduce ui::NativeEvent)?
> > 
> > Given that this is the root component that needs to access Event type,
> > this may be a good place for NativeEvent. (just NativeEvent, not others).
> > mike, what do you think?
> 
> If we can define base::NativeEvent as MSG/XEvent/WaylandEvent, then we can do
> away with the ui::NativeEvent that I'm adding in:
> http://codereview.chromium.org/7942004/diff/8006/ui/base/events.h
> Afaict, we'll still need views::NativeEvent for the aura::Event* case:
> http://codereview.chromium.org/7942004/diff/34049/views/events/event.h
> 
> (I'm pretty close to removing NativeEvent2 and just using GdkEvent where
needed,
> this was a good exercise in auditing and marking the GdkEvent locations for
> later removal).

If we're unable to accommodate Wayland as a base::NativeEvent, we may still need
ui::NativeEvent for the event cracking necessary of aura and views. But I could
be wrong...

[Ben Goodger (Google), 2011-09-23 20:41:52]

You should seek input primarily from the base/ OWNERS responsible for
MessageLoop.

[oshima, 2011-09-23 20:50:46]

+darin, evan 

Darin, Evan, I'd like to consolidate MessagePumpObserver interface between Win
and Aura+Linux, so that we do have to
use if/defs. This requires defining NativeEvent in base, but this doesn't add
any new dependency to base because it's just typdef to types that are already
used in base.

I talked to msw and sky and they said this is fine.

Please let me know what you think and if I can procceed.

[darin (slow to review), 2011-09-23 20:58:45]

http://codereview.chromium.org/8021009/diff/1/base/message_pump_x.h
File base/message_pump_x.h (right):

http://codereview.chromium.org/8021009/diff/1/base/message_pump_x.h#newcode32
base/message_pump_x.h:32: class BASE_EXPORT MessagePumpObserver {
it seems like you should factor this out into a separate header file that can be
shared by both windows and X11 ports.

[oshima, 2011-09-23 21:12:59]

http://codereview.chromium.org/8021009/diff/1/base/message_pump_x.h
File base/message_pump_x.h (right):

http://codereview.chromium.org/8021009/diff/1/base/message_pump_x.h#newcode32
base/message_pump_x.h:32: class BASE_EXPORT MessagePumpObserver {
On 2011/09/23 20:58:46, darin wrote:
> it seems like you should factor this out into a separate header file that can
be
> shared by both windows and X11 ports.

Yes, will do. So are you ok to have NativeEvent in base?

[Evan Martin, 2011-09-23 21:37:35]

On 2011/09/23 21:12:59, oshima wrote:
> http://codereview.chromium.org/8021009/diff/1/base/message_pump_x.h
> File base/message_pump_x.h (right):
> 
> http://codereview.chromium.org/8021009/diff/1/base/message_pump_x.h#newcode32
> base/message_pump_x.h:32: class BASE_EXPORT MessagePumpObserver {
> On 2011/09/23 20:58:46, darin wrote:
> > it seems like you should factor this out into a separate header file that
can
> be
> > shared by both windows and X11 ports.
> 
> Yes, will do. So are you ok to have NativeEvent in base?

It seems it'd be only used for MessagePumpObserver, right?
In that case it's probably OK with me.  I would be more reluctant if we started
sprinkling NativeEvent all over the message loop files, but I think they can
mostly remain using the fundamental types.

[oshima, 2011-09-23 21:48:07]

On 2011/09/23 21:37:35, Evan Martin wrote:
> On 2011/09/23 21:12:59, oshima wrote:
> > http://codereview.chromium.org/8021009/diff/1/base/message_pump_x.h
> > File base/message_pump_x.h (right):
> > 
> >
http://codereview.chromium.org/8021009/diff/1/base/message_pump_x.h#newcode32
> > base/message_pump_x.h:32: class BASE_EXPORT MessagePumpObserver {
> > On 2011/09/23 20:58:46, darin wrote:
> > > it seems like you should factor this out into a separate header file that
> can
> > be
> > > shared by both windows and X11 ports.
> > 
> > Yes, will do. So are you ok to have NativeEvent in base?
> 
> It seems it'd be only used for MessagePumpObserver, right?
> In that case it's probably OK with me.  I would be more reluctant if we
started
> sprinkling NativeEvent all over the message loop files, but I think they can
> mostly remain using the fundamental types.

Yes, this is only in MessagePumpObserver on Windows and Aura+Linux. Mac and
Linux/Gtk shouldn't be affacted.

[oshima, 2011-09-26 18:48:19]

This is ready for review. (I'll fix clang error in follow up CLs)

I created new message_pump_observer_views.h for views environment. This is not
used on views/gkt environment, which is planned to be removed.

Please take a look.

[darin (slow to review), 2011-09-26 19:31:00]

I don't think we want the concept of views in base/.

On Mon, Sep 26, 2011 at 11:48 AM, <oshima@chromium.org> wrote:

> This is ready for review. (I'll fix clang error in follow up CLs)
>
> I created new message_pump_observer_views.h for views environment. This is
> not
> used on views/gkt environment, which is planned to be removed.
>
> Please take a look.
>
>
>
>
>
>
>
http://codereview.chromium.**org/8021009/<http://codereview.chromium.org/8021...
>

[oshima, 2011-09-26 19:53:06]

It has no dependency to views code, so it can be any name. How about just
calling message_pump_observer.h instead,
with more comment to emphasize that this is for aura/views environment?

- oshima

On Mon, Sep 26, 2011 at 12:30 PM, Darin Fisher <darin@chromium.org> wrote:

> I don't think we want the concept of views in base/.
>
> On Mon, Sep 26, 2011 at 11:48 AM, <oshima@chromium.org> wrote:
>
>> This is ready for review. (I'll fix clang error in follow up CLs)
>>
>> I created new message_pump_observer_views.h for views environment. This is
>> not
>> used on views/gkt environment, which is planned to be removed.
>>
>> Please take a look.
>>
>>
>>
>>
>>
>>
>>
http://codereview.chromium.**org/8021009/<http://codereview.chromium.org/8021...
>>
>
>

[darin (slow to review), 2011-09-26 20:08:30]

+1 to just calling it message_pump_observer.h

I don't know why there needs to be such a comment.  Adding a comment about
aura/views is just as problematic.  You are leaking concepts into
lower-level APIs.  Two problems with that:

1-  People hacking on base/ shouldn't have to know about aura/views.
2-  People hacking on aura/views shouldn't have to maintain comments in
base/ that refer to aura/views.

The problem with #1 is that it increases cognitive load.  The problem with
#2 is that it leads to stale comments.  No one thinks of doing a comment
search to fixup such details.

It seems like it just comes down to what MessageLoop flavors/ports use the
interface.

-Darin


On Mon, Sep 26, 2011 at 12:52 PM, oshima <oshima@chromium.org> wrote:

> It has no dependency to views code, so it can be any name. How about just
> calling message_pump_observer.h instead,
> with more comment to emphasize that this is for aura/views environment?
>
> - oshima
>
>
> On Mon, Sep 26, 2011 at 12:30 PM, Darin Fisher <darin@chromium.org> wrote:
>
>> I don't think we want the concept of views in base/.
>>
>> On Mon, Sep 26, 2011 at 11:48 AM, <oshima@chromium.org> wrote:
>>
>>> This is ready for review. (I'll fix clang error in follow up CLs)
>>>
>>> I created new message_pump_observer_views.h for views environment. This
>>> is not
>>> used on views/gkt environment, which is planned to be removed.
>>>
>>> Please take a look.
>>>
>>>
>>>
>>>
>>>
>>>
>>>
http://codereview.chromium.**org/8021009/<http://codereview.chromium.org/8021...
>>>
>>
>>
>

[oshima, 2011-09-26 20:22:27]

On Mon, Sep 26, 2011 at 1:08 PM, Darin Fisher <darin@chromium.org> wrote:

> +1 to just calling it message_pump_observer.h
>
> I don't know why there needs to be such a comment.  Adding a comment about
> aura/views is just as problematic.  You are leaking concepts into
> lower-level APIs.  Two problems with that:
>
> 1-  People hacking on base/ shouldn't have to know about aura/views.
> 2-  People hacking on aura/views shouldn't have to maintain comments in
> base/ that refer to aura/views.
>
> The problem with #1 is that it increases cognitive load.  The problem with
> #2 is that it leads to stale comments.  No one thinks of doing a comment
> search to fixup such details.
>
> It seems like it just comes down to what MessageLoop flavors/ports use the
> interface.
>

Done. renamed file and updated the comment.

- oshima


>
> -Darin
>
>
> On Mon, Sep 26, 2011 at 12:52 PM, oshima <oshima@chromium.org> wrote:
>
>> It has no dependency to views code, so it can be any name. How about just
>> calling message_pump_observer.h instead,
>> with more comment to emphasize that this is for aura/views environment?
>>
>> - oshima
>>
>>
>> On Mon, Sep 26, 2011 at 12:30 PM, Darin Fisher <darin@chromium.org>wrote:
>>
>>> I don't think we want the concept of views in base/.
>>>
>>> On Mon, Sep 26, 2011 at 11:48 AM, <oshima@chromium.org> wrote:
>>>
>>>> This is ready for review. (I'll fix clang error in follow up CLs)
>>>>
>>>> I created new message_pump_observer_views.h for views environment. This
>>>> is not
>>>> used on views/gkt environment, which is planned to be removed.
>>>>
>>>> Please take a look.
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
http://codereview.chromium.**org/8021009/<http://codereview.chromium.org/8021...
>>>>
>>>
>>>
>>
>

[oshima, 2011-09-27 00:23:39]

Re-sync'ed and resolved conflict with TOT. PTAL.

[msw, 2011-09-27 01:16:46]

I just have some typing/naming consistency nits and opportunities to potentially
share some signatures and code between platforms. The different configurations
of USE_X11, TOOLKIT_USES_GTK, TOUCH_UI, and #elses are a little tough to follow,
but that may just be me.

Did you plan to eliminate the new ui::NativeEvent typedefs here or later?

http://codereview.chromium.org/8021009/diff/19029/base/message_pump_observer.h
File base/message_pump_observer.h (right):

http://codereview.chromium.org/8021009/diff/19029/base/message_pump_observer....
base/message_pump_observer.h:42: virtual EventStatus WillProcessEvent(const
NativeEvent& xevent) = 0;
Should this param be named xevent if it's cross-platform?

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system_key_event_listener.cc (right):

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system_key_event_listener.cc:156: const NativeEvent
xevent) {
ref and naming consistency.

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system_key_event_listener.h (right):

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system_key_event_listener.h:56: virtual EventStatus
WillProcessEvent(const NativeEvent xevent) OVERRIDE;
Pass-by-ref and naming (if the declaration isn't left as xevent): "const
NativeEvent& event". ditto below.

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/jankometer.cc
File chrome/browser/jankometer.cc (right):

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/jankometer.c...
chrome/browser/jankometer.cc:307: virtual base::EventStatus
WillProcessEvent(const MSG& msg) OVERRIDE {
You could make this signature cross platform, and if you tweaked the #ifs you
might be able to share the signatures.

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/notification...
File chrome/browser/notifications/balloon_collection_win.cc (right):

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/notification...
chrome/browser/notifications/balloon_collection_win.cc:36: base::EventStatus
BalloonCollectionImpl::WillProcessEvent(const MSG& event) {
naming consistency between event here and msg below.

optional: type consistency with declaration in this platform-specific instance.

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_frame_view.cc (right):

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_frame_view.cc:172: void
PanelBrowserFrameView::MouseWatcher::DidProcessEvent(const MSG& msg) {
type/name, although it looks tough to merge signatures/impls with touch/aura
here (because of the gtk impl).

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/ui/views/htm...
File chrome/browser/ui/views/html_dialog_view_browsertest.cc (right):

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/ui/views/htm...
chrome/browser/ui/views/html_dialog_view_browsertest.cc:70: #if defined(OS_WIN)
It looks like you could share the class decl, ctor, and GetInstance between all
these blocks, maybe some impl between win and touch/aura.

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/ui/views/htm...
chrome/browser/ui/views/html_dialog_view_browsertest.cc:81: const MSG& msg)
OVERRIDE {
name/type (here and below), this should fit on a single line.

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/dragged_tab_controller.cc (right):

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/dragged_tab_controller.cc:508: base::EventStatus
DraggedTabController::WillProcessEvent(const MSG& msg) {
type/name here and below; potential code sharing.

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/dragged_tab_controller.cc:522: const
base::NativeEvent& xevent) {
xevent/event naming consistency here and below.

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/dragged_tab_controller.h (right):

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/dragged_tab_controller.h:158: const
base::NativeEvent& msg) OVERRIDE;
name consistency.

http://codereview.chromium.org/8021009/diff/19029/views/mouse_watcher.cc
File views/mouse_watcher.cc (right):

http://codereview.chromium.org/8021009/diff/19029/views/mouse_watcher.cc#newc...
views/mouse_watcher.cc:38: base::EventStatus WillProcessEvent(const MSG& msg)
OVERRIDE {
type/name here and below. potential code sharing.

http://codereview.chromium.org/8021009/diff/19029/views/widget/native_widget_...
File views/widget/native_widget_win.cc (right):

http://codereview.chromium.org/8021009/diff/19029/views/widget/native_widget_...
views/widget/native_widget_win.cc:1124: base::EventStatus
NativeWidgetWin::WillProcessEvent(const MSG& msg) {
name/type

http://codereview.chromium.org/8021009/diff/19029/views/widget/native_widget_...
File views/widget/native_widget_win.h (right):

http://codereview.chromium.org/8021009/diff/19029/views/widget/native_widget_...
views/widget/native_widget_win.h:282: base::EventStatus WillProcessEvent(const
MSG& msg) OVERRIDE;
name/type

http://codereview.chromium.org/8021009/diff/19029/views/widget/tooltip_manage...
File views/widget/tooltip_manager_views.cc (right):

http://codereview.chromium.org/8021009/diff/19029/views/widget/tooltip_manage...
views/widget/tooltip_manager_views.cc:118: const base::NativeEvent& xevent) {
xevent/event name

http://codereview.chromium.org/8021009/diff/19029/views/widget/tooltip_manage...
views/widget/tooltip_manager_views.cc:127: void
TooltipManagerViews::DidProcessEvent(const base::NativeEvent& msg) {
"msg" name in the USE_X11 block?

http://codereview.chromium.org/8021009/diff/19029/views/widget/tooltip_manage...
views/widget/tooltip_manager_views.cc:131: base::EventStatus
TooltipManagerViews::WillProcessEvent(const MSG& msg) {
type/name

[oshima, 2011-09-27 02:12:11]

This CL is already big (in terms of # of lines), so let me do ui::NativeEvent ->
base::NativeEvent conversion in separate CL. 

I'll also address code sharing in follow up CL. I believe we can share more
event handling code by converting native event to Views event.

http://codereview.chromium.org/8021009/diff/19029/base/message_pump_observer.h
File base/message_pump_observer.h (right):

http://codereview.chromium.org/8021009/diff/19029/base/message_pump_observer....
base/message_pump_observer.h:42: virtual EventStatus WillProcessEvent(const
NativeEvent& xevent) = 0;
On 2011/09/27 01:16:46, msw wrote:
> Should this param be named xevent if it's cross-platform?

Done.

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system_key_event_listener.cc (right):

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system_key_event_listener.cc:156: const NativeEvent
xevent) {
On 2011/09/27 01:16:46, msw wrote:
> ref and naming consistency.

Done.

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system_key_event_listener.h (right):

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system_key_event_listener.h:56: virtual EventStatus
WillProcessEvent(const NativeEvent xevent) OVERRIDE;
On 2011/09/27 01:16:46, msw wrote:
> Pass-by-ref and naming (if the declaration isn't left as xevent): "const
> NativeEvent& event". ditto below.

Done.

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/jankometer.cc
File chrome/browser/jankometer.cc (right):

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/jankometer.c...
chrome/browser/jankometer.cc:307: virtual base::EventStatus
WillProcessEvent(const MSG& msg) OVERRIDE {
On 2011/09/27 01:16:46, msw wrote:
> You could make this signature cross platform, and if you tweaked the #ifs you
> might be able to share the signatures.
Update signature. Sharing signature made the code harder to follow, so I kept
this way.

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/notification...
File chrome/browser/notifications/balloon_collection_win.cc (right):

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/notification...
chrome/browser/notifications/balloon_collection_win.cc:36: base::EventStatus
BalloonCollectionImpl::WillProcessEvent(const MSG& event) {
On 2011/09/27 01:16:46, msw wrote:
> naming consistency between event here and msg below.
> 
> optional: type consistency with declaration in this platform-specific
instance.

Done.

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_frame_view.cc (right):

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_frame_view.cc:172: void
PanelBrowserFrameView::MouseWatcher::DidProcessEvent(const MSG& msg) {
On 2011/09/27 01:16:46, msw wrote:
> type/name, although it looks tough to merge signatures/impls with touch/aura
> here (because of the gtk impl).

Done.

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/ui/views/htm...
File chrome/browser/ui/views/html_dialog_view_browsertest.cc (right):

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/ui/views/htm...
chrome/browser/ui/views/html_dialog_view_browsertest.cc:70: #if defined(OS_WIN)
On 2011/09/27 01:16:46, msw wrote:
> It looks like you could share the class decl, ctor, and GetInstance between
all
> these blocks, maybe some impl between win and touch/aura.

Done for class decl etc. I didn't share impl as it makes it harder to follow.

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/ui/views/htm...
chrome/browser/ui/views/html_dialog_view_browsertest.cc:81: const MSG& msg)
OVERRIDE {
On 2011/09/27 01:16:46, msw wrote:
> name/type (here and below), this should fit on a single line.

Done.

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/dragged_tab_controller.cc (right):

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/dragged_tab_controller.cc:508: base::EventStatus
DraggedTabController::WillProcessEvent(const MSG& msg) {
On 2011/09/27 01:16:46, msw wrote:
> type/name here and below; potential code sharing.

Done.

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/dragged_tab_controller.cc:522: const
base::NativeEvent& xevent) {
On 2011/09/27 01:16:46, msw wrote:
> xevent/event naming consistency here and below.

Done.

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/dragged_tab_controller.h (right):

http://codereview.chromium.org/8021009/diff/19029/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/dragged_tab_controller.h:158: const
base::NativeEvent& msg) OVERRIDE;
On 2011/09/27 01:16:46, msw wrote:
> name consistency.

Done.

http://codereview.chromium.org/8021009/diff/19029/views/mouse_watcher.cc
File views/mouse_watcher.cc (right):

http://codereview.chromium.org/8021009/diff/19029/views/mouse_watcher.cc#newc...
views/mouse_watcher.cc:38: base::EventStatus WillProcessEvent(const MSG& msg)
OVERRIDE {
On 2011/09/27 01:16:46, msw wrote:
> type/name here and below. potential code sharing.

Done.

http://codereview.chromium.org/8021009/diff/19029/views/widget/native_widget_...
File views/widget/native_widget_win.cc (right):

http://codereview.chromium.org/8021009/diff/19029/views/widget/native_widget_...
views/widget/native_widget_win.cc:1124: base::EventStatus
NativeWidgetWin::WillProcessEvent(const MSG& msg) {
On 2011/09/27 01:16:46, msw wrote:
> name/type

Done.

http://codereview.chromium.org/8021009/diff/19029/views/widget/native_widget_...
File views/widget/native_widget_win.h (right):

http://codereview.chromium.org/8021009/diff/19029/views/widget/native_widget_...
views/widget/native_widget_win.h:282: base::EventStatus WillProcessEvent(const
MSG& msg) OVERRIDE;
On 2011/09/27 01:16:46, msw wrote:
> name/type

Done.

http://codereview.chromium.org/8021009/diff/19029/views/widget/tooltip_manage...
File views/widget/tooltip_manager_views.cc (right):

http://codereview.chromium.org/8021009/diff/19029/views/widget/tooltip_manage...
views/widget/tooltip_manager_views.cc:118: const base::NativeEvent& xevent) {
On 2011/09/27 01:16:46, msw wrote:
> xevent/event name

Done.

http://codereview.chromium.org/8021009/diff/19029/views/widget/tooltip_manage...
views/widget/tooltip_manager_views.cc:127: void
TooltipManagerViews::DidProcessEvent(const base::NativeEvent& msg) {
On 2011/09/27 01:16:46, msw wrote:
> "msg" name in the USE_X11 block?

Done.

http://codereview.chromium.org/8021009/diff/19029/views/widget/tooltip_manage...
views/widget/tooltip_manager_views.cc:131: base::EventStatus
TooltipManagerViews::WillProcessEvent(const MSG& msg) {
On 2011/09/27 01:16:46, msw wrote:
> type/name

Done.

[oshima, 2011-09-27 02:13:11]

> This CL is already big (in terms of # of lines)

s/# of lines/#of files/

[msw, 2011-09-27 03:51:42]

Naming and white-space nits and human-powered compiler errors.

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/automation/u...
File chrome/browser/automation/ui_controls_gtk.cc (right):

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/automation/u...
chrome/browser/automation/ui_controls_gtk.cc:50: virtual void
DidProcessEvent(const base::NativeEvent& event) {
Add a blank line between these functions.

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system_key_event_listener.cc (right):

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system_key_event_listener.cc:156: const NativeEvent&
event) {
Doesn't this NativeEvent need a namespace qualifier? I'm surprised this doesn't
give an error, is something using base?

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system_key_event_listener.cc:159: void
SystemKeyEventListener::DidProcessEvent(const NativeEvent& event) {
Add a blank line between these functions.

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/jankometer.cc
File chrome/browser/jankometer.cc (right):

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/jankometer.c...
chrome/browser/jankometer.cc:329: virtual void DidProcessEvent(const
ui::NativeEvent& event) OVERRIDE {
You probably meant base::NativeEvent here.

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/jankometer.c...
chrome/browser/jankometer.cc:337: virtual void DidProcessEvent(const
base::NativeEvent& event) OVERRIDE {
Add a blank line between these functions.

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/notification...
File chrome/browser/notifications/balloon_collection_win.cc (right):

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/notification...
chrome/browser/notifications/balloon_collection_win.cc:42: switch (msg.message)
{
This should be event.message... I'd expect you'd see an error on Win build.

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/ui/views/htm...
File chrome/browser/ui/views/html_dialog_view_browsertest.cc (right):

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/ui/views/htm...
chrome/browser/ui/views/html_dialog_view_browsertest.cc:91: if (msg.message ==
WM_PAINT || msg.message == WM_TIMER)
event.message

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/dragged_tab_controller.cc (right):

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/dragged_tab_controller.cc:518: if (msg.message ==
WM_KEYDOWN && msg.wParam == VK_ESCAPE)
event.message, etc.

http://codereview.chromium.org/8021009/diff/26002/views/mouse_watcher.cc
File views/mouse_watcher.cc (right):

http://codereview.chromium.org/8021009/diff/26002/views/mouse_watcher.cc#newc...
views/mouse_watcher.cc:55: switch (msg.message) {
event.message

http://codereview.chromium.org/8021009/diff/26002/views/widget/tooltip_manage...
File views/widget/tooltip_manager_views.cc (right):

http://codereview.chromium.org/8021009/diff/26002/views/widget/tooltip_manage...
views/widget/tooltip_manager_views.cc:127: void
TooltipManagerViews::DidProcessEvent(const base::NativeEvent& event) {
Add a blank line between these functions.

[sadrul, 2011-09-27 04:13:22]

I did manage to find two more nits ;)

Other than the nits: LGTM

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/ui/views/htm...
File chrome/browser/ui/views/html_dialog_view_browsertest.cc (right):

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/ui/views/htm...
chrome/browser/ui/views/html_dialog_view_browsertest.cc:156: 
-blank line

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/dragged_tab_controller.cc (right):

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/dragged_tab_controller.cc:526: void
DraggedTabController::DidProcessEvent(const base::NativeEvent& event) {
+blank line

[oshima, 2011-09-27 16:39:38]

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/automation/u...
File chrome/browser/automation/ui_controls_gtk.cc (right):

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/automation/u...
chrome/browser/automation/ui_controls_gtk.cc:50: virtual void
DidProcessEvent(const base::NativeEvent& event) {
On 2011/09/27 03:51:42, msw wrote:
> Add a blank line between these functions.

Done.

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system_key_event_listener.cc (right):

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system_key_event_listener.cc:156: const NativeEvent&
event) {
On 2011/09/27 03:51:42, msw wrote:
> Doesn't this NativeEvent need a namespace qualifier? I'm surprised this
doesn't
> give an error, is something using base?

Done.

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system_key_event_listener.cc:159: void
SystemKeyEventListener::DidProcessEvent(const NativeEvent& event) {
On 2011/09/27 03:51:42, msw wrote:
> Add a blank line between these functions.

Done.

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/jankometer.cc
File chrome/browser/jankometer.cc (right):

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/jankometer.c...
chrome/browser/jankometer.cc:329: virtual void DidProcessEvent(const
ui::NativeEvent& event) OVERRIDE {
On 2011/09/27 03:51:42, msw wrote:
> You probably meant base::NativeEvent here.

Done.

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/jankometer.c...
chrome/browser/jankometer.cc:337: virtual void DidProcessEvent(const
base::NativeEvent& event) OVERRIDE {
On 2011/09/27 03:51:42, msw wrote:
> Add a blank line between these functions.

Done.

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/notification...
File chrome/browser/notifications/balloon_collection_win.cc (right):

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/notification...
chrome/browser/notifications/balloon_collection_win.cc:42: switch (msg.message)
{
On 2011/09/27 03:51:42, msw wrote:
> This should be event.message... I'd expect you'd see an error on Win build.

Done.

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/ui/views/htm...
File chrome/browser/ui/views/html_dialog_view_browsertest.cc (right):

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/ui/views/htm...
chrome/browser/ui/views/html_dialog_view_browsertest.cc:91: if (msg.message ==
WM_PAINT || msg.message == WM_TIMER)
On 2011/09/27 03:51:42, msw wrote:
> event.message

Done.

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/ui/views/htm...
chrome/browser/ui/views/html_dialog_view_browsertest.cc:156: 
On 2011/09/27 04:13:23, sadrul wrote:
> -blank line

Done.

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/ui/views/tab...
File chrome/browser/ui/views/tabs/dragged_tab_controller.cc (right):

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/dragged_tab_controller.cc:518: if (msg.message ==
WM_KEYDOWN && msg.wParam == VK_ESCAPE)
On 2011/09/27 03:51:42, msw wrote:
> event.message, etc.

Done.

http://codereview.chromium.org/8021009/diff/26002/chrome/browser/ui/views/tab...
chrome/browser/ui/views/tabs/dragged_tab_controller.cc:526: void
DraggedTabController::DidProcessEvent(const base::NativeEvent& event) {
On 2011/09/27 04:13:23, sadrul wrote:
> +blank line

Done.

http://codereview.chromium.org/8021009/diff/26002/views/mouse_watcher.cc
File views/mouse_watcher.cc (right):

http://codereview.chromium.org/8021009/diff/26002/views/mouse_watcher.cc#newc...
views/mouse_watcher.cc:55: switch (msg.message) {
On 2011/09/27 03:51:42, msw wrote:
> event.message

Done.

http://codereview.chromium.org/8021009/diff/26002/views/widget/tooltip_manage...
File views/widget/tooltip_manager_views.cc (right):

http://codereview.chromium.org/8021009/diff/26002/views/widget/tooltip_manage...
views/widget/tooltip_manager_views.cc:127: void
TooltipManagerViews::DidProcessEvent(const base::NativeEvent& event) {
On 2011/09/27 03:51:42, msw wrote:
> Add a blank line between these functions.

Done.

[oshima, 2011-09-28 15:17:23]

friendly ping. I need approval from owners for following directory.

chrome/browser/ui -> ben
base -> darin

Please let me know if I should send this to another owner.

Thanks.

[oshima, 2011-09-30 20:00:53]

Brett, can you review changes under base? Thanks.

[oshima, 2011-09-30 20:24:36]

Mark, Will, can one of you review base change?

Both Brett and Darin are OOO.

[Mark Mentovai, 2011-09-30 20:28:55]

base LGTM. Looks mostly like you’re just reorganizing in base.

Darin may still want to have a look when he gets back. I trust you’ll address
any of his comments in a follow-up if he has.

I only looked at the files inside base for OWNERS approval.

[oshima, 2011-09-30 20:34:43]

On 2011/09/30 20:28:55, Mark Mentovai wrote:
> base LGTM. Looks mostly like you’re just reorganizing in base.
> 
> Darin may still want to have a look when he gets back. I trust you’ll address
> any of his comments in a follow-up if he has.

Yes, I'll do for sure. Thanks!

> I only looked at the files inside base for OWNERS approval.

[Ben Goodger (Google), 2011-09-30 20:50:50]

http://codereview.chromium.org/8021009/diff/29001/views/widget/native_widget_...
File views/widget/native_widget_gtk.cc (right):

http://codereview.chromium.org/8021009/diff/29001/views/widget/native_widget_...
views/widget/native_widget_gtk.cc:272: virtual void WillProcessEvent(GdkEvent*
event) {
Why are these two branches incompatible? Does GdkEvent* never map to
base::NativeEvent?

[oshima, 2011-09-30 21:17:22]

http://codereview.chromium.org/8021009/diff/29001/views/widget/native_widget_...
File views/widget/native_widget_gtk.cc (right):

http://codereview.chromium.org/8021009/diff/29001/views/widget/native_widget_...
views/widget/native_widget_gtk.cc:272: virtual void WillProcessEvent(GdkEvent*
event) {
On 2011/09/30 20:50:51, Ben Goodger (Google) wrote:
> Why are these two branches incompatible? Does GdkEvent* never map to
> base::NativeEvent?

aura/touch has different signature (returns base::EventStatus) and NativeEvent
is XEvent* on X11/Liinux.
Mapping Gdk to NativeEvent would be tricky because TOUCH build needs NativeEvent
to be XEvent too, which
has TOOLKIT_USES_GTK=1. Once we migrate to aura, this code will be all gone.

[msw, 2011-09-30 21:32:36]

On 2011/09/30 21:17:22, oshima wrote:
>
http://codereview.chromium.org/8021009/diff/29001/views/widget/native_widget_...
> File views/widget/native_widget_gtk.cc (right):
> 
>
http://codereview.chromium.org/8021009/diff/29001/views/widget/native_widget_...
> views/widget/native_widget_gtk.cc:272: virtual void WillProcessEvent(GdkEvent*
> event) {
> On 2011/09/30 20:50:51, Ben Goodger (Google) wrote:
> > Why are these two branches incompatible? Does GdkEvent* never map to
> > base::NativeEvent?
> 
> aura/touch has different signature (returns base::EventStatus) and NativeEvent
> is XEvent* on X11/Liinux.
> Mapping Gdk to NativeEvent would be tricky because TOUCH build needs
NativeEvent
> to be XEvent too, which
> has TOOLKIT_USES_GTK=1. Once we migrate to aura, this code will be all gone.

We deprecated GdkEvent as a NativeEvent with crrev.com/102815; XEvent is a
NativeEvent and NativeEvent2 is gone. Explicit GdkEvent code should be easier to
deprecate when possible. I hope that's still a desirable change; and this seems
to follow that same pattern.

[oshima, 2011-10-01 03:43:23]

On 2011/09/30 21:32:36, msw wrote:
> On 2011/09/30 21:17:22, oshima wrote:
> >
>
http://codereview.chromium.org/8021009/diff/29001/views/widget/native_widget_...
> > File views/widget/native_widget_gtk.cc (right):
> > 
> >
>
http://codereview.chromium.org/8021009/diff/29001/views/widget/native_widget_...
> > views/widget/native_widget_gtk.cc:272: virtual void
WillProcessEvent(GdkEvent*
> > event) {
> > On 2011/09/30 20:50:51, Ben Goodger (Google) wrote:
> > > Why are these two branches incompatible? Does GdkEvent* never map to
> > > base::NativeEvent?
> > 
> > aura/touch has different signature (returns base::EventStatus) and
NativeEvent
> > is XEvent* on X11/Liinux.
> > Mapping Gdk to NativeEvent would be tricky because TOUCH build needs
> NativeEvent
> > to be XEvent too, which
> > has TOOLKIT_USES_GTK=1. Once we migrate to aura, this code will be all gone.
> 
> We deprecated GdkEvent as a NativeEvent with crrev.com/102815; XEvent is a
> NativeEvent and NativeEvent2 is gone. Explicit GdkEvent code should be easier
to
> deprecate when possible. I hope that's still a desirable change; and this
seems
> to follow that same pattern.

I should have also mentioned that after this CL, I'm going to change so that
ui::NativeEvent becomes base::NativeEvent, so they needs to be consistent.

[Ben Goodger (Google), 2011-10-03 23:30:00]

LGTM

[sadrul, 2011-10-04 02:19:48]

This seems to have broken touchui chromeos builds.

[oshima, 2011-10-04 02:35:28]

On 2011/10/04 02:19:48, sadrul wrote:
> This seems to have broken touchui chromeos builds.

Working on fix now