Make RootView::DispatchGestureEvent() non-virtual

Make RootView::DispatchGestureEvent() non-virtual

In preparation of removing RootView::DispatchGestureEvent(),
replace its two overrides with overrides of
ui::EventProcessor::OnEventFromSource() and then make
the function DispatchGestureEvent() non-virtual.

BUG=353641
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283673

[tdanderson, 2014-07-14 20:58:22]

Sadrul, can you please take a look?

[sadrul, 2014-07-15 18:23:46]

lgtm

https://codereview.chromium.org/395563002/diff/1/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/browser_root_view.cc (right):

https://codereview.chromium.org/395563002/diff/1/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_root_view.cc:165: //                  
once RootView::OnEventFromSource() is removed.
You can actually leave this as it is when you remove
RootView::OnEventFromSource(), and it will do the right thing.

https://codereview.chromium.org/395563002/diff/1/ui/views/controls/menu/menu_...
File ui/views/controls/menu/menu_host_root_view.cc (right):

https://codereview.chromium.org/395563002/diff/1/ui/views/controls/menu/menu_...
ui/views/controls/menu/menu_host_root_view.cc:60: //                   once
RootView::OnEventFromSource() is removed.
ditto

[tdanderson, 2014-07-15 20:25:40]

Hi Scott, can you please take a look at the changes in chrome/ ?

https://codereview.chromium.org/395563002/diff/1/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/browser_root_view.cc (right):

https://codereview.chromium.org/395563002/diff/1/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_root_view.cc:165: //                  
once RootView::OnEventFromSource() is removed.
On 2014/07/15 18:23:46, sadrul wrote:
> You can actually leave this as it is when you remove
> RootView::OnEventFromSource(), and it will do the right thing.

Right, I just thought it would be slightly better for readability if the name
matched the parent class that actually contains the implementation. But now that
I think about it, I should probably leave this as RootView:: anyway for some
possible future scenario where RootView or View overrides OnEventFromSource().

Removed the TODO.

https://codereview.chromium.org/395563002/diff/1/ui/views/controls/menu/menu_...
File ui/views/controls/menu/menu_host_root_view.cc (right):

https://codereview.chromium.org/395563002/diff/1/ui/views/controls/menu/menu_...
ui/views/controls/menu/menu_host_root_view.cc:60: //                   once
RootView::OnEventFromSource() is removed.
On 2014/07/15 18:23:46, sadrul wrote:
> ditto

Done.

[sky, 2014-07-15 20:53:11]

LGTM - but think about the following change to ui::Event as I think it makes
things a bit cleaner.

https://codereview.chromium.org/395563002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_root_view.cc (right):

https://codereview.chromium.org/395563002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_root_view.cc:155: if
(event->IsGestureEvent()) {
I'm not a fan of the static_casts all through out the code. They are easy to get
wrong. WDYT of isolating the static_cast in ui::Event:

ui::GestureEvent* AsGestureEvent() {
  return IsGestureEvent() ? static_cast<GestureEvent*>(this) : NULL;
}
? This code then becomes:

ui::GestureEvent* gesture_event = event->AsGestureEvent();
if (gesture_event) {....

[tdanderson, 2014-07-15 21:31:25]

Hi Scott, please see below:

https://codereview.chromium.org/395563002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_root_view.cc (right):

https://codereview.chromium.org/395563002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_root_view.cc:155: if
(event->IsGestureEvent()) {
On 2014/07/15 20:53:11, sky wrote:
> I'm not a fan of the static_casts all through out the code. They are easy to
get
> wrong. WDYT of isolating the static_cast in ui::Event:
> 
> ui::GestureEvent* AsGestureEvent() {
>   return IsGestureEvent() ? static_cast<GestureEvent*>(this) : NULL;
> }
> ? This code then becomes:
> 
> ui::GestureEvent* gesture_event = event->AsGestureEvent();
> if (gesture_event) {....

Yes, I agree that isolating the static_casts in this manner would be a lot
nicer. But how about instead making IsGestureEvent() a precondition:

ui::GestureEvent* AsGestureEvent() {
  CHECK(IsGestureEvent());
  return static_cast<GestureEvent*>(this);
}

?

[sky, 2014-07-15 23:42:42]

On 2014/07/15 21:31:25, tdanderson wrote:
> Hi Scott, please see below:
> 
>
https://codereview.chromium.org/395563002/diff/20001/chrome/browser/ui/views/...
> File chrome/browser/ui/views/frame/browser_root_view.cc (right):
> 
>
https://codereview.chromium.org/395563002/diff/20001/chrome/browser/ui/views/...
> chrome/browser/ui/views/frame/browser_root_view.cc:155: if
> (event->IsGestureEvent()) {
> On 2014/07/15 20:53:11, sky wrote:
> > I'm not a fan of the static_casts all through out the code. They are easy to
> get
> > wrong. WDYT of isolating the static_cast in ui::Event:
> > 
> > ui::GestureEvent* AsGestureEvent() {
> >   return IsGestureEvent() ? static_cast<GestureEvent*>(this) : NULL;
> > }
> > ? This code then becomes:
> > 
> > ui::GestureEvent* gesture_event = event->AsGestureEvent();
> > if (gesture_event) {....
> 
> Yes, I agree that isolating the static_casts in this manner would be a lot
> nicer. But how about instead making IsGestureEvent() a precondition:
> 
> ui::GestureEvent* AsGestureEvent() {
>   CHECK(IsGestureEvent());
>   return static_cast<GestureEvent*>(this);
> }
> 
> ?

Sure, that works too.

[tdanderson, 2014-07-16 16:53:57]

Scott, can you please take another look?

[sky, 2014-07-16 19:21:28]

LGTM

https://codereview.chromium.org/395563002/diff/40001/ui/views/widget/root_view.h
File ui/views/widget/root_view.h (right):

https://codereview.chromium.org/395563002/diff/40001/ui/views/widget/root_vie...
ui/views/widget/root_view.h:134: void DispatchGestureEvent(ui::GestureEvent*
event);
nit: move implementation to match new position.

[tdanderson, 2014-07-16 20:50:50]

https://codereview.chromium.org/395563002/diff/40001/ui/views/widget/root_view.h
File ui/views/widget/root_view.h (right):

https://codereview.chromium.org/395563002/diff/40001/ui/views/widget/root_vie...
ui/views/widget/root_view.h:134: void DispatchGestureEvent(ui::GestureEvent*
event);
On 2014/07/16 19:21:28, sky wrote:
> nit: move implementation to match new position.

Done.

[tdanderson, 2014-07-16 20:50:55]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2014-07-16 20:51:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tdanderson@chromium.org/395563002/60001

[commit-bot: I haz the power, 2014-07-17 00:50:06]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-17 06:57:19]

Change committed as 283673