events: Start changing EventHandler interface to not return a value.

events: Start changing EventHandler interface to not return a value.

The EventHandler should explicitly stop the propagation of an event
(Event::StopPropagation()), or mark it has having been handled
(Event::PreventDefault()), by calling a function on the event
object, instead of returning a value from the event-handler functions.

This patch takes the first step by converting EventHandler::OnEvent. Subsequent
patches will conver the event-specific callbacks (OnKeyEvent etc.)

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

[sadrul, 2012-12-03 17:21:46]

One concern I have is that the event-handlers may perform some action, and then
forget to mark the event as being processed. So between this, and the
event-handlers returning incorrect event-result value (in the current code), I
am not really sure which is worse.

[Ben Goodger (Google), 2012-12-03 17:36:44]

I believe "unhandled" is probably right in 99% of cases, so I'm not too
concerned.

[Ben Goodger (Google), 2012-12-03 17:36:53]

lgtm

https://codereview.chromium.org/11308322/diff/1/ui/base/events/event.cc
File ui/base/events/event.cc (right):

https://codereview.chromium.org/11308322/diff/1/ui/base/events/event.cc#newco...
ui/base/events/event.cc:81: CHECK(phase_ != EP_PREDISPATCH && phase_ !=
EP_POSTDISPATCH);
i would also like the ability to prevent this (and probably stoppropagation?)
via a flag on the dispatcher api to event. e.g. focus change events should not
be cancelable.

can be done in a followon cl.

https://codereview.chromium.org/11308322/diff/1/ui/base/events/event.h
File ui/base/events/event.h (right):

https://codereview.chromium.org/11308322/diff/1/ui/base/events/event.h#newcod...
ui/base/events/event.h:166: // be in the list will not receive the event after
this is called.
One thing to note. Reading the W3C spec for DOMEvents, stop propagation only
takes effect after the current layer, e.g. for pre-target handlers all handlers
at the current level will be fired and then propagation stopped. Not sure we
care here, at least not yet.

https://codereview.chromium.org/11308322/diff/1/ui/base/events/event.h#newcod...
ui/base/events/event.h:171: bool default_prevented() const { return result_ !=
ui::ER_UNHANDLED; }
This reads a little awkwardly. I'm not sure I have any more precise suggestions
though. DOM's "canceled" is simpler but maybe vaguer?

[sadrul, 2012-12-03 17:45:20]

https://codereview.chromium.org/11308322/diff/1/ui/base/events/event.cc
File ui/base/events/event.cc (right):

https://codereview.chromium.org/11308322/diff/1/ui/base/events/event.cc#newco...
ui/base/events/event.cc:81: CHECK(phase_ != EP_PREDISPATCH && phase_ !=
EP_POSTDISPATCH);
On 2012/12/03 17:36:54, Ben Goodger (Google) wrote:
> i would also like the ability to prevent this (and probably stoppropagation?)
> via a flag on the dispatcher api to event. e.g. focus change events should not
> be cancelable.
> 
> can be done in a followon cl.

Indeed. I have that in mind :) [ crbug.com/163617 ]

https://codereview.chromium.org/11308322/diff/1/ui/base/events/event.h
File ui/base/events/event.h (right):

https://codereview.chromium.org/11308322/diff/1/ui/base/events/event.h#newcod...
ui/base/events/event.h:166: // be in the list will not receive the event after
this is called.
On 2012/12/03 17:36:54, Ben Goodger (Google) wrote:
> One thing to note. Reading the W3C spec for DOMEvents, stop propagation only
> takes effect after the current layer, e.g. for pre-target handlers all
handlers
> at the current level will be fired and then propagation stopped. Not sure we
> care here, at least not yet.

One of the use-cases for StopPropagation is when the target is destroyed by an
event-handler. In such cases, continuing to dispatch the event to other handlers
can cause crashes etc.

https://codereview.chromium.org/11308322/diff/1/ui/base/events/event.h#newcod...
ui/base/events/event.h:171: bool default_prevented() const { return result_ !=
ui::ER_UNHANDLED; }
On 2012/12/03 17:36:54, Ben Goodger (Google) wrote:
> This reads a little awkwardly. I'm not sure I have any more precise
suggestions
> though. DOM's "canceled" is simpler but maybe vaguer?

How about 'handled()' or 'processed()' (and 'MarkHandled'/'MarkProcessed')?

[sadrul, 2012-12-03 18:04:48]

Changed to SetHandled() and handled()

[Ben Goodger (Google), 2012-12-03 18:45:36]

slgtm

[Ben Goodger (Google), 2012-12-04 08:16:28]

I like handled()/SetHandled().

-Ben


On Mon, Dec 3, 2012 at 9:45 AM, <sadrul@chromium.org> wrote:

>
> https://codereview.chromium.**org/11308322/diff/1/ui/base/**
>
events/event.cc<https://codereview.chromium.org/11308322/diff/1/ui/base/events/event.cc>
> File ui/base/events/event.cc (right):
>
> https://codereview.chromium.**org/11308322/diff/1/ui/base/**
>
events/event.cc#newcode81<https://codereview.chromium.org/11308322/diff/1/ui/base/events/event.cc#newcode81>
> ui/base/events/event.cc:81: CHECK(phase_ != EP_PREDISPATCH && phase_ !=
> EP_POSTDISPATCH);
> On 2012/12/03 17:36:54, Ben Goodger (Google) wrote:
>
>> i would also like the ability to prevent this (and probably
>>
> stoppropagation?)
>
>> via a flag on the dispatcher api to event. e.g. focus change events
>>
> should not
>
>> be cancelable.
>>
>
>  can be done in a followon cl.
>>
>
> Indeed. I have that in mind :) [ crbug.com/163617 ]
>
>
>
https://codereview.chromium.**org/11308322/diff/1/ui/base/**events/event.h<ht...
> File ui/base/events/event.h (right):
>
> https://codereview.chromium.**org/11308322/diff/1/ui/base/**
>
events/event.h#newcode166<https://codereview.chromium.org/11308322/diff/1/ui/base/events/event.h#newcode166>
> ui/base/events/event.h:166: // be in the list will not receive the event
> after this is called.
> On 2012/12/03 17:36:54, Ben Goodger (Google) wrote:
>
>> One thing to note. Reading the W3C spec for DOMEvents, stop
>>
> propagation only
>
>> takes effect after the current layer, e.g. for pre-target handlers all
>>
> handlers
>
>> at the current level will be fired and then propagation stopped. Not
>>
> sure we
>
>> care here, at least not yet.
>>
>
> One of the use-cases for StopPropagation is when the target is destroyed
> by an event-handler. In such cases, continuing to dispatch the event to
> other handlers can cause crashes etc.
>
>
> https://codereview.chromium.**org/11308322/diff/1/ui/base/**
>
events/event.h#newcode171<https://codereview.chromium.org/11308322/diff/1/ui/base/events/event.h#newcode171>
> ui/base/events/event.h:171: bool default_prevented() const { return
> result_ != ui::ER_UNHANDLED; }
> On 2012/12/03 17:36:54, Ben Goodger (Google) wrote:
>
>> This reads a little awkwardly. I'm not sure I have any more precise
>>
> suggestions
>
>> though. DOM's "canceled" is simpler but maybe vaguer?
>>
>
> How about 'handled()' or 'processed()' (and
> 'MarkHandled'/'MarkProcessed')**?
>
>
https://codereview.chromium.**org/11308322/<https://codereview.chromium.org/1...
>