Refactored static_cast'ing of events to use the AsXXXEvent() methods.

Use the CHECK'd AsXXXEvent() methods on Event rather than the unsafe static_cast's.

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

[kylix_rd, 2016-09-29 17:30:09]

The CQ bit was checked by kylixrd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 17:30:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kylix_rd, 2016-09-29 17:43:59]

Description was changed from

==========
Refactored static_cast'ing of events to use the AsXXXEvent() methods

BUG=
==========

to

==========
Refactored static_cast'ing of events to use the AsXXXEvent() methods
==========

[kylix_rd, 2016-09-29 17:43:59]

kylixrd@chromium.org changed reviewers:
+ rjkroege@chromium.org, sadrul@chromium.org

[kylix_rd, 2016-09-29 17:48:33]

Description was changed from

==========
Refactored static_cast'ing of events to use the AsXXXEvent() methods
==========

to

==========
Use the CHECK'd AsXXXEvent() methods on Event rather than the unsafe
static_cast's.
==========

[kylix_rd, 2016-09-29 18:12:19]

https://codereview.chromium.org/2374213003/diff/1/ui/events/event_handler.cc
File ui/events/event_handler.cc (left):

https://codereview.chromium.org/2374213003/diff/1/ui/events/event_handler.cc#...
ui/events/event_handler.cc:25: //                   event types.
I noticed this comment and had a couple of cycles to do some refactoring to use
the AsXXXEvent() methods instead of static_casts everywhere.

https://codereview.chromium.org/2374213003/diff/1/ui/events/event_handler.cc
File ui/events/event_handler.cc (right):

https://codereview.chromium.org/2374213003/diff/1/ui/events/event_handler.cc#...
ui/events/event_handler.cc:35:
OnCancelMode(static_cast<CancelModeEvent*>(event));
Should there be an AsCancelMode() method on Event? My guess is that it isn't
used enough to warrant it.

[commit-bot: I haz the power, 2016-09-29 18:17:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 18:17:36]

Dry run: This issue passed the CQ dry run.

[rjkroege, 2016-10-03 17:42:45]

My preference would be if you added the AsCancelEvent. The rest is a nice
cleanup.

Aside: sadrul@: why can't the As* methods be inline? And maybe be DCHECK?

https://codereview.chromium.org/2374213003/diff/1/ui/events/event_handler.cc
File ui/events/event_handler.cc (right):

https://codereview.chromium.org/2374213003/diff/1/ui/events/event_handler.cc#...
ui/events/event_handler.cc:35:
OnCancelMode(static_cast<CancelModeEvent*>(event));
On 2016/09/29 18:12:19, kylix_rd wrote:
> Should there be an AsCancelMode() method on Event? My guess is that it isn't
> used enough to warrant it.

my sense of implementation "rightness" is that there probably should be an
AsCancelEvent

[kylix_rd, 2016-10-03 18:29:04]

Added xxCancelModeEvent() methods to Event and refactored code in
event_handler.cc to use it.

https://codereview.chromium.org/2374213003/diff/1/ui/events/event_handler.cc
File ui/events/event_handler.cc (right):

https://codereview.chromium.org/2374213003/diff/1/ui/events/event_handler.cc#...
ui/events/event_handler.cc:35:
OnCancelMode(static_cast<CancelModeEvent*>(event));
On 2016/10/03 17:42:45, rjkroege wrote:
> On 2016/09/29 18:12:19, kylix_rd wrote:
> > Should there be an AsCancelMode() method on Event? My guess is that it isn't
> > used enough to warrant it.
> 
> my sense of implementation "rightness" is that there probably should be an
> AsCancelEvent

Done.

[rjkroege, 2016-10-03 22:58:40]

lgtm

https://codereview.chromium.org/2374213003/diff/20001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/2374213003/diff/20001/ui/events/event.cc#newc...
ui/events/event.cc:291: CancelModeEvent* Event::AsCancelModeEvent() {
I know that you're (rightly) following the pattern of the other As* methods but
I'm unhappy that these aren't inlined. A function call is a lot slower than a
compile time cast. Can these all of this methods be converted to DCHECK and be
sure to be inlined in a future CL?

[sadrul, 2016-10-04 00:35:04]

lgtm

https://codereview.chromium.org/2374213003/diff/20001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/2374213003/diff/20001/ui/events/event.cc#newc...
ui/events/event.cc:291: CancelModeEvent* Event::AsCancelModeEvent() {
On 2016/10/03 22:58:40, rjkroege wrote:
> I know that you're (rightly) following the pattern of the other As* methods
but
> I'm unhappy that these aren't inlined. A function call is a lot slower than a
> compile time cast. Can these all of this methods be converted to DCHECK and be
> sure to be inlined in a future CL?

+1

[kylix_rd, 2016-10-04 14:10:17]

The CQ bit was checked by kylixrd@chromium.org

[commit-bot: I haz the power, 2016-10-04 14:10:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kylix_rd, 2016-10-04 14:12:21]

https://codereview.chromium.org/2374213003/diff/20001/ui/events/event.cc
File ui/events/event.cc (right):

https://codereview.chromium.org/2374213003/diff/20001/ui/events/event.cc#newc...
ui/events/event.cc:291: CancelModeEvent* Event::AsCancelModeEvent() {
On 2016/10/03 22:58:40, rjkroege wrote:
> I know that you're (rightly) following the pattern of the other As* methods
but
> I'm unhappy that these aren't inlined. A function call is a lot slower than a
> compile time cast. Can these all of this methods be converted to DCHECK and be
> sure to be inlined in a future CL?

Certainly. I can simply move the method bodies to the header and the compiler
should use that as a hint to inline, right? I can also add 'inline' to the decl
to be sure.

[commit-bot: I haz the power, 2016-10-04 15:30:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-04 15:30:39]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[kylix_rd, 2016-10-04 16:17:59]

The CQ bit was checked by kylixrd@chromium.org

[commit-bot: I haz the power, 2016-10-04 16:18:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-04 17:00:12]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-04 17:03:33]

Description was changed from

==========
Use the CHECK'd AsXXXEvent() methods on Event rather than the unsafe
static_cast's.
==========

to

==========
Use the CHECK'd AsXXXEvent() methods on Event rather than the unsafe
static_cast's.

Committed: https://crrev.com/d47244cedff9630c4a81e31a285b56cc3dcf263b
Cr-Commit-Position: refs/heads/master@{#422830}
==========

[commit-bot: I haz the power, 2016-10-04 17:03:36]

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

[kylix_rd, 2016-10-04 20:35:44]

On 2016/10/04 14:12:21, kylix_rd wrote:
> https://codereview.chromium.org/2374213003/diff/20001/ui/events/event.cc
> File ui/events/event.cc (right):
> 
>
https://codereview.chromium.org/2374213003/diff/20001/ui/events/event.cc#newc...
> ui/events/event.cc:291: CancelModeEvent* Event::AsCancelModeEvent() {
> On 2016/10/03 22:58:40, rjkroege wrote:
> > I know that you're (rightly) following the pattern of the other As* methods
> but
> > I'm unhappy that these aren't inlined. A function call is a lot slower than
a
> > compile time cast. Can these all of this methods be converted to DCHECK and
be
> > sure to be inlined in a future CL?
> 
> Certainly. I can simply move the method bodies to the header and the compiler
> should use that as a hint to inline, right? I can also add 'inline' to the
decl
> to be sure.

I looked into this and it seems that the reason the methods aren't inlined or
declared in the header is due to the static_cast rule where the compiler checks
object pointer lineage. The compiler cannot perform the downcast because the
descendant classes are only forward declared and the actual definition hasn't
yet been seen.

C:\builds\chromium\src\ui\events\event.h(233,12):  error: static_cast from
'ui::Event *' to 'ui::CancelModeEvent *', which are not related by inheritance,
is not allowed
    return static_cast<CancelModeEvent*>(this);
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
C:\builds\chromium\src\ui\events\event.h(33,7):  note: 'CancelModeEvent' is
incomplete
class CancelModeEvent;
      ^

An alternative would be to add a static inline method on each descendant class
that does the downcast. While not universal, there is a similar static method on
LocatedEvent that tries to do just that:

  static const ui::LocatedEvent* FromIfValid(const ui::Event* event) {
    return event && event->IsLocatedEvent() ? event->AsLocatedEvent() : nullptr;
  }

Instead of calling AsXXXXEvent() on the base Event class, each descendant could
provide a method such as this:

  static const ui::LocatedEvent* Cast(const ui::Event* event) {
    DCHECK(event->IsLocatedEvent());
    return static_cast<const LocatedEvent*>(event);
  }