Enumerate the return value of dispatchEvent so it is clear.

Enumerate the return value of dispatchEvent so it is clear.

Change the boolean result of dispatchEvent to an enum class so that
the return value actually makes sense. Gesture events returned
inverted results. Remove some event mediators that aren't required
now that the result code indicates whether if the default handler was
invoked or not.

BUG=560357, 560355, 543611
Committed: https://crrev.com/c68b1aecf681958457d0f6c09c8e379ed02b6b80
Cr-Commit-Position: refs/heads/master@{#377885}

[dtapuska, 2015-11-30 00:23:34]

Description was changed from

==========
Enumerate the return value of dispatchEvent so it is clear.

Change the boolean result of dispatchEvent to an enum class so that
the return value actually makes sense. Gesture events returned
inverted results. Remove some event mediators that aren't required
now that the result code indicates whether if the default handler was
invoked or not.

BUG=560357, 560355, 543611
==========

to

==========
Enumerate the return value of dispatchEvent so it is clear.

Change the boolean result of dispatchEvent to an enum class so that
the return value actually makes sense. Gesture events returned
inverted results. Remove some event mediators that aren't required
now that the result code indicates whether if the default handler was
invoked or not.

BUG=560357, 560355, 543611
==========

[dtapuska, 2015-11-30 00:23:34]

dtapuska@chromium.org changed reviewers:
+ tkent@chromium.org

[tkent, 2015-12-01 00:42:14]

If we have WebInputEventResult, we don't need to introduce
blink::EventTarget::DispatchEventResult.  We should use WebInputEventResult
everywhere.

I recommend to split out removing mediators to another CL.

[dtapuska, 2015-12-01 01:26:22]

dtapuska@chromium.org changed reviewers:
+ rbyers@chromium.org

[dtapuska, 2015-12-01 01:27:48]

On 2015/12/01 00:42:14, tkent wrote:
> If we have WebInputEventResult, we don't need to introduce
> blink::EventTarget::DispatchEventResult.  We should use WebInputEventResult
> everywhere.
> 
> I recommend to split out removing mediators to another CL.

Rick; you agree with pulling WebInputEventResult into core? Typically there has
been an API separation between web/platform and DOM. But I'm not sure what the
architecture plans are.

[dcheng, 2015-12-01 01:35:29]

On 2015/12/01 at 01:27:48, dtapuska wrote:
> On 2015/12/01 00:42:14, tkent wrote:
> > If we have WebInputEventResult, we don't need to introduce
> > blink::EventTarget::DispatchEventResult.  We should use WebInputEventResult
> > everywhere.
> > 
> > I recommend to split out removing mediators to another CL.
> 
> Rick; you agree with pulling WebInputEventResult into core? Typically there
has been an API separation between web/platform and DOM. But I'm not sure what
the architecture plans are.

Drive-by...

public/platform can be used directly in core; there's plenty of precedence for
it already:
https://code.google.com/p/chromium/codesearch#search/&q=file:Source/core/dom%...

Reusing it is better than duplicating the enum, IMO.

[haraken, 2015-12-01 01:37:40]

On 2015/12/01 01:35:29, dcheng wrote:
> On 2015/12/01 at 01:27:48, dtapuska wrote:
> > On 2015/12/01 00:42:14, tkent wrote:
> > > If we have WebInputEventResult, we don't need to introduce
> > > blink::EventTarget::DispatchEventResult.  We should use
WebInputEventResult
> > > everywhere.
> > > 
> > > I recommend to split out removing mediators to another CL.
> > 
> > Rick; you agree with pulling WebInputEventResult into core? Typically there
> has been an API separation between web/platform and DOM. But I'm not sure what
> the architecture plans are.
> 
> Drive-by...
> 
> public/platform can be used directly in core; there's plenty of precedence for
> it already:
>
https://code.google.com/p/chromium/codesearch#search/&q=file:Source/core/dom%...
> 
> Reusing it is better than duplicating the enum, IMO.

Agreed.

The long-term plan is to move web/ into core/, which may remove the necessity of
WebXXXEvent. But it won't happen in this month.

[Rick Byers, 2015-12-01 17:23:01]

On 2015/12/01 01:37:40, haraken wrote:
> On 2015/12/01 01:35:29, dcheng wrote:
> > On 2015/12/01 at 01:27:48, dtapuska wrote:
> > > On 2015/12/01 00:42:14, tkent wrote:
> > > > If we have WebInputEventResult, we don't need to introduce
> > > > blink::EventTarget::DispatchEventResult.  We should use
> WebInputEventResult
> > > > everywhere.
> > > > 
> > > > I recommend to split out removing mediators to another CL.
> > > 
> > > Rick; you agree with pulling WebInputEventResult into core? Typically
there
> > has been an API separation between web/platform and DOM. But I'm not sure
what
> > the architecture plans are.
> > 
> > Drive-by...
> > 
> > public/platform can be used directly in core; there's plenty of precedence
for
> > it already:
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=file:Source/core/dom%...
> > 
> > Reusing it is better than duplicating the enum, IMO.
> 
> Agreed.
> 
> The long-term plan is to move web/ into core/, which may remove the necessity
of
> WebXXXEvent. But it won't happen in this month.

Yep, the separation here is largely historical (for WebKit which is used by
multiple embedders).  There are other examples where we've avoided adding
duplicate types and just used the public/ types directly in core/.  I should
have thought to explicitly mention this to you before you wrote this code -
sorry!

[dtapuska, 2015-12-02 15:25:33]

On 2015/12/01 17:23:01, Rick Byers wrote:
> On 2015/12/01 01:37:40, haraken wrote:
> > On 2015/12/01 01:35:29, dcheng wrote:
> > > On 2015/12/01 at 01:27:48, dtapuska wrote:
> > > > On 2015/12/01 00:42:14, tkent wrote:
> > > > > If we have WebInputEventResult, we don't need to introduce
> > > > > blink::EventTarget::DispatchEventResult.  We should use
> > WebInputEventResult
> > > > > everywhere.
> > > > > 
> > > > > I recommend to split out removing mediators to another CL.
> > > > 
> > > > Rick; you agree with pulling WebInputEventResult into core? Typically
> there
> > > has been an API separation between web/platform and DOM. But I'm not sure
> what
> > > the architecture plans are.
> > > 
> > > Drive-by...
> > > 
> > > public/platform can be used directly in core; there's plenty of precedence
> for
> > > it already:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=file:Source/core/dom%...
> > > 
> > > Reusing it is better than duplicating the enum, IMO.
> > 
> > Agreed.
> > 
> > The long-term plan is to move web/ into core/, which may remove the
necessity
> of
> > WebXXXEvent. But it won't happen in this month.
> 
> Yep, the separation here is largely historical (for WebKit which is used by
> multiple embedders).  There are other examples where we've avoided adding
> duplicate types and just used the public/ types directly in core/.  I should
> have thought to explicitly mention this to you before you wrote this code -
> sorry!

I've removed the DispatchEventResult and just imported the WebInputEventResult
into EventTarget.

In terms of the Mediators; I'd need to adjust their code and it would be
duplicated code of the base
mediator so it is pointless to write code as opposed to just remove it in this
class. It is only the
Gesture, Keyboard mediators being removed.

[tkent, 2015-12-03 00:36:25]

> In terms of the Mediators; I'd need to adjust their code and it would be
duplicated code of the base
> mediator so it is pointless to write code as opposed to just remove it in this
class. It is only the
> Gesture, Keyboard mediators being removed.

Ok, I see.

https://codereview.chromium.org/1479923002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/WorkerMessagingProxy.cpp (right):

https://codereview.chromium.org/1479923002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerMessagingProxy.cpp:176:
postTaskToWorkerGlobalScope(createCrossThreadTask(&processExceptionOnWorkerGlobalScope,
exceptionId, errorHandled));
Can we change the bool argument of processExceptionOnWorkerGlobalScopt to
WebInputEventResult? Or add a TODO() comment?

https://codereview.chromium.org/1479923002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/1479923002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:117:
observer->didDispatchEvent(eventResult != WebInputEventResult::NotHandled);
Can we change the argument of RespondWithObserver to WebInputEventResult? or add
a TODO comment?

[jsbell, 2015-12-03 00:46:17]

jsbell@chromium.org changed reviewers:
+ jsbell@chromium.org

[jsbell, 2015-12-03 00:46:18]

Drive by...

https://codereview.chromium.org/1479923002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp (left):

https://codereview.chromium.org/1479923002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp:461: bool
dontPreventDefault = IDBEventDispatcher::dispatch(event.get(), targets);
Very happy to see this going away. :)

[dtapuska, 2015-12-03 02:00:53]

PTAL

https://codereview.chromium.org/1479923002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/workers/WorkerMessagingProxy.cpp (right):

https://codereview.chromium.org/1479923002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/workers/WorkerMessagingProxy.cpp:176:
postTaskToWorkerGlobalScope(createCrossThreadTask(&processExceptionOnWorkerGlobalScope,
exceptionId, errorHandled));
On 2015/12/03 00:36:25, tkent wrote:
> Can we change the bool argument of processExceptionOnWorkerGlobalScopt to
> WebInputEventResult? Or add a TODO() comment?

I tried this but:

We can't really change the processExceptionOnWorkerGlobalScope without defining
a template for cross thread task copying of WebInputEventResult...

It is an enum class so I guess we need to define a variant of it in the
CrossThreadCopy which I think is overkill for this. If you feel different please
advise.

https://codereview.chromium.org/1479923002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp (left):

https://codereview.chromium.org/1479923002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/indexeddb/IDBRequest.cpp:461: bool
dontPreventDefault = IDBEventDispatcher::dispatch(event.get(), targets);
On 2015/12/03 00:46:18, jsbell wrote:
> Very happy to see this going away. :)

Thanks for the happiness :-)

https://codereview.chromium.org/1479923002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/1479923002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:117:
observer->didDispatchEvent(eventResult != WebInputEventResult::NotHandled);
On 2015/12/03 00:36:25, tkent wrote:
> Can we change the argument of RespondWithObserver to WebInputEventResult? or
add
> a TODO comment?

Done.

[tkent, 2015-12-03 03:20:10]

lgtm

[dtapuska, 2015-12-04 14:24:18]

The CQ bit was checked by dtapuska@chromium.org

[commit-bot: I haz the power, 2015-12-04 14:24:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1479923002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1479923002/80001

[commit-bot: I haz the power, 2015-12-04 14:28:04]

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

[commit-bot: I haz the power, 2015-12-04 14:28:05]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dtapuska, 2015-12-04 17:14:21]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2015-12-04 17:14:22]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/1479923002/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2015-12-04 17:15:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1479923002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1479923002/100001

[commit-bot: I haz the power, 2015-12-04 18:35:32]

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

[commit-bot: I haz the power, 2015-12-04 18:35:33]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dtapuska, 2015-12-04 19:52:03]

The CQ bit was checked by dtapuska@chromium.org

[commit-bot: I haz the power, 2015-12-04 19:53:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1479923002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1479923002/100001

[commit-bot: I haz the power, 2015-12-04 20:11:39]

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

[commit-bot: I haz the power, 2015-12-04 20:11:40]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dtapuska, 2015-12-04 20:47:05]

The CQ bit was checked by dtapuska@chromium.org

[commit-bot: I haz the power, 2015-12-04 20:48:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1479923002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1479923002/100001

[commit-bot: I haz the power, 2015-12-04 21:48:28]

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

[commit-bot: I haz the power, 2015-12-04 21:48:29]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[philipj_slow, 2015-12-04 22:38:50]

philipj@opera.com changed reviewers:
+ philipj@opera.com

[philipj_slow, 2015-12-04 22:38:51]

Using WebInputEventResult for the very generic dispatchEvent doesn't look good.

Something like enum DispatchEventResult { Canceled, NotCanceled } might help
clarify, but a return type with four values is a bit worrying, won't that let us
communicate more information than in the event model and risk ending up with
behaviors that can't be expressed in terms of the spec's model?

https://codereview.chromium.org/1479923002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/EventTarget.cpp (right):

https://codereview.chromium.org/1479923002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/EventTarget.cpp:254:
exceptionState.throwDOMException(InvalidStateError, "The event provided is
null.");
Why did you add back the null check? If it was while resolving conflicts, make
sure to check if any other conflicts were also resolved incorrectly.

https://codereview.chromium.org/1479923002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/EventTarget.cpp:271: // Return whether the
event was cancelled or not to JS not that it
Does this change observable behavior? I can't understand the "not that it might
have actually been handled" bit, missing words or typo?

[philipj_slow, 2015-12-04 22:43:24]

On 2015/12/04 22:38:51, philipj wrote:
> Using WebInputEventResult for the very generic dispatchEvent doesn't look
good.
> 
> Something like enum DispatchEventResult { Canceled, NotCanceled } might help
> clarify, but a return type with four values is a bit worrying, won't that let
us
> communicate more information than in the event model and risk ending up with
> behaviors that can't be expressed in terms of the spec's model?

I've read the existing comment and see that you started out with
DispatchEventResult. To be clear, my concern isn't the use of a Web* type, but
that it has more enum states than the event model and a input-specific name.

[dtapuska, 2015-12-07 14:25:18]

On 2015/12/04 22:43:24, philipj wrote:
> On 2015/12/04 22:38:51, philipj wrote:
> > Using WebInputEventResult for the very generic dispatchEvent doesn't look
> good.
> > 
> > Something like enum DispatchEventResult { Canceled, NotCanceled } might help
> > clarify, but a return type with four values is a bit worrying, won't that
let
> us
> > communicate more information than in the event model and risk ending up with
> > behaviors that can't be expressed in terms of the spec's model?
> 
> I've read the existing comment and see that you started out with
> DispatchEventResult. To be clear, my concern isn't the use of a Web* type, but
> that it has more enum states than the event model and a input-specific name.


I can rename WebInputEventResult to WebEventResult or WebDispatchEventResult
thoughts?

We need the 4 states indicating how the default event handlers are invoked or
not. In our implementation is that default event handlers are invoked during the
dispatch of the event (as a last phase); so how this is handled is never
reported up to the higher levels and doesn't allow us to capture interesting UMA
metrics like in https://codereview.chromium.org/1474443002

[dtapuska, 2015-12-07 14:25:30]

https://codereview.chromium.org/1479923002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/EventTarget.cpp (right):

https://codereview.chromium.org/1479923002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/EventTarget.cpp:254:
exceptionState.throwDOMException(InvalidStateError, "The event provided is
null.");
On 2015/12/04 22:38:51, philipj wrote:
> Why did you add back the null check? If it was while resolving conflicts, make
> sure to check if any other conflicts were also resolved incorrectly.

Ah yes resolve issue because I needed to hold onto the dispatchedEvent.

https://codereview.chromium.org/1479923002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/EventTarget.cpp:271: // Return whether the
event was cancelled or not to JS not that it
On 2015/12/04 22:38:51, philipj wrote:
> Does this change observable behavior? I can't understand the "not that it
might
> have actually been handled" bit, missing words or typo?

Well it may in some cases; Touch and Mouse event listener mediators that
returned the event was handled by the default event handler may return false now
instead of true. But this in fact is a bug in itself as non-trusted events
should never invoke the default event handler according to the spec. I currently
have a bug in my pool for that (aside from click).

[philipj_slow, 2015-12-07 15:01:45]

On 2015/12/07 14:25:18, dtapuska wrote:
> On 2015/12/04 22:43:24, philipj wrote:
> > On 2015/12/04 22:38:51, philipj wrote:
> > > Using WebInputEventResult for the very generic dispatchEvent doesn't look
> > good.
> > > 
> > > Something like enum DispatchEventResult { Canceled, NotCanceled } might
help
> > > clarify, but a return type with four values is a bit worrying, won't that
> let
> > us
> > > communicate more information than in the event model and risk ending up
with
> > > behaviors that can't be expressed in terms of the spec's model?
> > 
> > I've read the existing comment and see that you started out with
> > DispatchEventResult. To be clear, my concern isn't the use of a Web* type,
but
> > that it has more enum states than the event model and a input-specific name.
> 
> 
> I can rename WebInputEventResult to WebEventResult or WebDispatchEventResult
> thoughts?

WebDispatchEventResult is long, but WFM. I defer to all you event experts on the
best generic name.

> We need the 4 states indicating how the default event handlers are invoked or
> not. In our implementation is that default event handlers are invoked during
the
> dispatch of the event (as a last phase); so how this is handled is never
> reported up to the higher levels and doesn't allow us to capture interesting
UMA
> metrics like in https://codereview.chromium.org/1474443002

I'm not sure I understand, that CL has already landed, is it currently not
measuring the right thing because dispatchEvent returns a bool?

From the enum names and descriptions in WebInputEventResult I couldn't tell that
the two extra states are because of the defaultEventHandler() model. As I'm sure
you know the spec fantasy is that default actions are always invoked by whoever
calls dispatchEvent(), which is a rather different model:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=12230

If we managed to actually fix that in Blink, would there still be a need to have
4 dispatchEvent return values? Would it be possible to have a per-spec internal
implementation, and have the two extra states only somewhere in the outer
layers?

[philipj_slow, 2015-12-07 15:03:56]

https://codereview.chromium.org/1479923002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/EventTarget.cpp (right):

https://codereview.chromium.org/1479923002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/EventTarget.cpp:271: // Return whether the
event was cancelled or not to JS not that it
On 2015/12/07 14:25:30, dtapuska wrote:
> On 2015/12/04 22:38:51, philipj wrote:
> > Does this change observable behavior? I can't understand the "not that it
> might
> > have actually been handled" bit, missing words or typo?
> 
> Well it may in some cases; Touch and Mouse event listener mediators that
> returned the event was handled by the default event handler may return false
now
> instead of true. But this in fact is a bug in itself as non-trusted events
> should never invoke the default event handler according to the spec. I
currently
> have a bug in my pool for that (aside from click). 

It would be good if any observable changes are done separately, or if they
really cannot be avoided, tests are needed.

[philipj_slow, 2015-12-07 15:05:21]

Rick, can you take another deep dive into this? If it seems like I'm just not
understanding the issues, I'm happy to defer to you and save us all some time.

[dtapuska, 2015-12-07 15:20:16]

On 2015/12/07 15:01:45, philipj wrote:
> On 2015/12/07 14:25:18, dtapuska wrote:
> > On 2015/12/04 22:43:24, philipj wrote:
> > > On 2015/12/04 22:38:51, philipj wrote:
> > > > Using WebInputEventResult for the very generic dispatchEvent doesn't
look
> > > good.
> > > > 
> > > > Something like enum DispatchEventResult { Canceled, NotCanceled } might
> help
> > > > clarify, but a return type with four values is a bit worrying, won't
that
> > let
> > > us
> > > > communicate more information than in the event model and risk ending up
> with
> > > > behaviors that can't be expressed in terms of the spec's model?
> > > 
> > > I've read the existing comment and see that you started out with
> > > DispatchEventResult. To be clear, my concern isn't the use of a Web* type,
> but
> > > that it has more enum states than the event model and a input-specific
name.
> > 
> > 
> > I can rename WebInputEventResult to WebEventResult or WebDispatchEventResult
> > thoughts?
> 
> WebDispatchEventResult is long, but WFM. I defer to all you event experts on
the
> best generic name.
> 
> > We need the 4 states indicating how the default event handlers are invoked
or
> > not. In our implementation is that default event handlers are invoked during
> the
> > dispatch of the event (as a last phase); so how this is handled is never
> > reported up to the higher levels and doesn't allow us to capture interesting
> UMA
> > metrics like in https://codereview.chromium.org/1474443002
> 
> I'm not sure I understand, that CL has already landed, is it currently not
> measuring the right thing because dispatchEvent returns a bool?
> 
> From the enum names and descriptions in WebInputEventResult I couldn't tell
that
> the two extra states are because of the defaultEventHandler() model. As I'm
sure
> you know the spec fantasy is that default actions are always invoked by
whoever
> calls dispatchEvent(), which is a rather different model:
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=12230
> 
> If we managed to actually fix that in Blink, would there still be a need to
have
> 4 dispatchEvent return values? Would it be possible to have a per-spec
internal
> implementation, and have the two extra states only somewhere in the outer
> layers?

dispatchEvent returns different bools depending on the scenario. (which actually
feels completely wrong)
1) For Touch/Mouse/Keyboard it is NOT default prevented or default handled.
2) Gesture's are default prevented
3) For others it is NOT default prevented

The defaultEvent handler is invoked inside of the dispatchEvent call:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

If we rearranged the code so that the default event handler was never invoked
inside of dispatchEvent then it might be possible; although there are lots of
situations where
suppressed event paths return false such as disabled form events
(crbug.com/560355).

Granted the result is already propagated up; currently to get the UMA metric we
inspect the Event and look at defaultPrevented/defaultHandled booleans that give
us the correct response code. But as I stated above there are scenarios where
the boolean returned as false it wasn't marked as default prevented or default
handled on the event.

[Rick Byers, 2015-12-07 20:51:48]

On 2015/12/07 15:05:21, philipj wrote:
> Rick, can you take another deep dive into this? If it seems like I'm just not
> understanding the issues, I'm happy to defer to you and save us all some time.

I hadn't thought about this beyond input scenarios
(https://codereview.chromium.org/1463823003).  I agree the type name shouldn't
be input-specific if it's being used down in the DOM.

On 2015/12/07 15:20:16, dtapuska wrote:
> On 2015/12/07 15:01:45, philipj wrote:
> > On 2015/12/07 14:25:18, dtapuska wrote:
> > > On 2015/12/04 22:43:24, philipj wrote:
> > > > On 2015/12/04 22:38:51, philipj wrote:
> > > > > Using WebInputEventResult for the very generic dispatchEvent doesn't
> look
> > > > good.
> > > > > 
> > > > > Something like enum DispatchEventResult { Canceled, NotCanceled }
might
> > help
> > > > > clarify, but a return type with four values is a bit worrying, won't
> that
> > > let
> > > > us
> > > > > communicate more information than in the event model and risk ending
up
> > with
> > > > > behaviors that can't be expressed in terms of the spec's model?
> > > > 
> > > > I've read the existing comment and see that you started out with
> > > > DispatchEventResult. To be clear, my concern isn't the use of a Web*
type,
> > but
> > > > that it has more enum states than the event model and a input-specific
> name.
> > > 
> > > 
> > > I can rename WebInputEventResult to WebEventResult or
WebDispatchEventResult
> > > thoughts?
> > 
> > WebDispatchEventResult is long, but WFM. I defer to all you event experts on
> the
> > best generic name.
> > 
> > > We need the 4 states indicating how the default event handlers are invoked
> or
> > > not. In our implementation is that default event handlers are invoked
during
> > the
> > > dispatch of the event (as a last phase); so how this is handled is never
> > > reported up to the higher levels and doesn't allow us to capture
interesting
> > UMA
> > > metrics like in https://codereview.chromium.org/1474443002
> > 
> > I'm not sure I understand, that CL has already landed, is it currently not
> > measuring the right thing because dispatchEvent returns a bool?
> > 
> > From the enum names and descriptions in WebInputEventResult I couldn't tell
> that
> > the two extra states are because of the defaultEventHandler() model. As I'm
> sure
> > you know the spec fantasy is that default actions are always invoked by
> whoever
> > calls dispatchEvent(), which is a rather different model:
> > https://www.w3.org/Bugs/Public/show_bug.cgi?id=12230
> > 
> > If we managed to actually fix that in Blink, would there still be a need to
> have
> > 4 dispatchEvent return values? Would it be possible to have a per-spec
> internal
> > implementation, and have the two extra states only somewhere in the outer
> > layers?
> 
> dispatchEvent returns different bools depending on the scenario. (which
actually
> feels completely wrong)
> 1) For Touch/Mouse/Keyboard it is NOT default prevented or default handled.
> 2) Gesture's are default prevented
> 3) For others it is NOT default prevented
> 
> The defaultEvent handler is invoked inside of the dispatchEvent call:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> If we rearranged the code so that the default event handler was never invoked
> inside of dispatchEvent then it might be possible; although there are lots of
> situations where
> suppressed event paths return false such as disabled form events
> (crbug.com/560355).
> 
> Granted the result is already propagated up; currently to get the UMA metric
we
> inspect the Event and look at defaultPrevented/defaultHandled booleans that
give
> us the correct response code. But as I stated above there are scenarios where
> the boolean returned as false it wasn't marked as default prevented or default
> handled on the event.

Thinking about this more I agree with Philip that ideally we wouldn't need this
additional information at the DOM layer at all (or we should already have all
the information we need).  In particular, say we did the following:

- Fix the inverted case of gesture events - that's clearly just a bug
- Fix the invalid combinations of Event states.  I.e.
  - dispatchEvent returns false IFF Event.defaultPrevented or
Event.defaultHandled is true

Then I think the only remaining questions would be:
 1. Does code running inside dispatchEvent need some way to mark an event as
"suppressed".
      HandledSuppressed should be treated exactly the same as HandledSystem, but
the distinction is useful for our metrics (did JS get a chance first or not). 
I'm not sure preserving this distinction is too important though, it probably
depends on the specific cases.  dtapuska@ WDYT?

 2. Is it clearer to use a boolean or enum return value?
      The communicated information should be identical (when you include
Event.defaultPrevented/defaultHandled).
      For the lower level, the boolean was no end of confusion as to it's
semantics.  For Node::dispatchEvent the spec defines the semantics precisely and
we can't (probably wouldn't want to) change the exposed JS API so perhaps it's
not as big of a deal?


Personally if we can fix the issues at the DOM layer without changing the
dispatchEvent return type, then I'd probably prefer we keep it - just to keep
the internal API as close to the spec as possible.  But I don't have a strong
opinion here, and it sounds like (if the above issues were addressed) Philip
would probably be OK either way (it's just an implementation style question at
this point), right?

[Rick Byers, 2015-12-07 21:20:27]

On 2015/12/07 20:51:48, Rick Byers wrote:
> On 2015/12/07 15:05:21, philipj wrote:
> > Rick, can you take another deep dive into this? If it seems like I'm just
not
> > understanding the issues, I'm happy to defer to you and save us all some
time.
> 
> I hadn't thought about this beyond input scenarios
> (https://codereview.chromium.org/1463823003).  I agree the type name shouldn't
> be input-specific if it's being used down in the DOM.
> 
> On 2015/12/07 15:20:16, dtapuska wrote:
> > On 2015/12/07 15:01:45, philipj wrote:
> > > On 2015/12/07 14:25:18, dtapuska wrote:
> > > > On 2015/12/04 22:43:24, philipj wrote:
> > > > > On 2015/12/04 22:38:51, philipj wrote:
> > > > > > Using WebInputEventResult for the very generic dispatchEvent doesn't
> > look
> > > > > good.
> > > > > > 
> > > > > > Something like enum DispatchEventResult { Canceled, NotCanceled }
> might
> > > help
> > > > > > clarify, but a return type with four values is a bit worrying, won't
> > that
> > > > let
> > > > > us
> > > > > > communicate more information than in the event model and risk ending
> up
> > > with
> > > > > > behaviors that can't be expressed in terms of the spec's model?
> > > > > 
> > > > > I've read the existing comment and see that you started out with
> > > > > DispatchEventResult. To be clear, my concern isn't the use of a Web*
> type,
> > > but
> > > > > that it has more enum states than the event model and a input-specific
> > name.
> > > > 
> > > > 
> > > > I can rename WebInputEventResult to WebEventResult or
> WebDispatchEventResult
> > > > thoughts?
> > > 
> > > WebDispatchEventResult is long, but WFM. I defer to all you event experts
on
> > the
> > > best generic name.
> > > 
> > > > We need the 4 states indicating how the default event handlers are
invoked
> > or
> > > > not. In our implementation is that default event handlers are invoked
> during
> > > the
> > > > dispatch of the event (as a last phase); so how this is handled is never
> > > > reported up to the higher levels and doesn't allow us to capture
> interesting
> > > UMA
> > > > metrics like in https://codereview.chromium.org/1474443002
> > > 
> > > I'm not sure I understand, that CL has already landed, is it currently not
> > > measuring the right thing because dispatchEvent returns a bool?
> > > 
> > > From the enum names and descriptions in WebInputEventResult I couldn't
tell
> > that
> > > the two extra states are because of the defaultEventHandler() model. As
I'm
> > sure
> > > you know the spec fantasy is that default actions are always invoked by
> > whoever
> > > calls dispatchEvent(), which is a rather different model:
> > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=12230
> > > 
> > > If we managed to actually fix that in Blink, would there still be a need
to
> > have
> > > 4 dispatchEvent return values? Would it be possible to have a per-spec
> > internal
> > > implementation, and have the two extra states only somewhere in the outer
> > > layers?
> > 
> > dispatchEvent returns different bools depending on the scenario. (which
> actually
> > feels completely wrong)
> > 1) For Touch/Mouse/Keyboard it is NOT default prevented or default handled.
> > 2) Gesture's are default prevented
> > 3) For others it is NOT default prevented
> > 
> > The defaultEvent handler is invoked inside of the dispatchEvent call:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > If we rearranged the code so that the default event handler was never
invoked
> > inside of dispatchEvent then it might be possible; although there are lots
of
> > situations where
> > suppressed event paths return false such as disabled form events
> > (crbug.com/560355).
> > 
> > Granted the result is already propagated up; currently to get the UMA metric
> we
> > inspect the Event and look at defaultPrevented/defaultHandled booleans that
> give
> > us the correct response code. But as I stated above there are scenarios
where
> > the boolean returned as false it wasn't marked as default prevented or
default
> > handled on the event.
> 
> Thinking about this more I agree with Philip that ideally we wouldn't need
this
> additional information at the DOM layer at all (or we should already have all
> the information we need).  In particular, say we did the following:
> 
> - Fix the inverted case of gesture events - that's clearly just a bug
> - Fix the invalid combinations of Event states.  I.e.
>   - dispatchEvent returns false IFF Event.defaultPrevented or
> Event.defaultHandled is true
> 
> Then I think the only remaining questions would be:
>  1. Does code running inside dispatchEvent need some way to mark an event as
> "suppressed".
>       HandledSuppressed should be treated exactly the same as HandledSystem,
but
> the distinction is useful for our metrics (did JS get a chance first or not). 
> I'm not sure preserving this distinction is too important though, it probably
> depends on the specific cases.  dtapuska@ WDYT?
> 
>  2. Is it clearer to use a boolean or enum return value?
>       The communicated information should be identical (when you include
> Event.defaultPrevented/defaultHandled).
>       For the lower level, the boolean was no end of confusion as to it's
> semantics.  For Node::dispatchEvent the spec defines the semantics precisely
and
> we can't (probably wouldn't want to) change the exposed JS API so perhaps it's
> not as big of a deal?
> 
> 
> Personally if we can fix the issues at the DOM layer without changing the
> dispatchEvent return type, then I'd probably prefer we keep it - just to keep
> the internal API as close to the spec as possible.  But I don't have a strong
> opinion here, and it sounds like (if the above issues were addressed) Philip
> would probably be OK either way (it's just an implementation style question at
> this point), right?

Chatting with Dave more, I do think the whole problem here comes down to the
mismatch in model between the "spec fantasy" and our implementation.  There is
additional logic that we might run both before true dispatch ("suppression") and
after it ("default handler") which in our implementation is implemeneted inside
of Node::dispatchEvent, but in the spec is considered something outside of
dispatchEvent.  I doubt it'll be practical anytime soon to rewrite our
implementation to match the spec here, but we can move closer to it.

In particular, Dave has already been working to ensure the default handler logic
is never invoked for events with isTrusted=false to ensure our behavior matches
the spec.  We should probably be working towards the same thing for all
"suppressed" cases (eg. if JS dispatches an event at a disabled form control,
should it still be received?).

Given that difference, I actually think it's quite misleading to have OUR
dispatchEvent return a boolean.  There aren't just 2 ways an event can be
handled in our implementation, there are 4.  By returning the enum we can
explicitly document (and some day ASSERT) the fact that only
'HandledApplication' and 'NotHandled' should ever occur for events with
isTrusted=false, and the other states represent logic which conceptually (from a
spec perspective) lives before or after "event dispatching".  By making the
return type an enum and writing some good comments in the enum definition we'll
hopefully encourage developers to more often think about the implications of
this difference in model.

Of course if we have a legitimate plan for actually refactoring our
implementation to match the spec model, then at that point we might want to go
back to a boolean so that we match it exactly.

Philip, WDYT?

Dave, do you want to take a crack at adding a few more comments to your enum
definition to help explain this?  

As for the enum name, WebDispatchEventResult or just WebEventResult SGTM.  You
could probably also get away with DispatchEventResult ("PointerProperties"
already exists in public/platform without a 'Web' prefix - I expect that prefix
not to be very meaningful in the future).Obviously we should agree on the plan
before you go through the pain of renaming the enum type.

[philipj_slow, 2015-12-08 10:24:18]

On 2015/12/07 20:51:48, Rick Byers wrote:
> Thinking about this more I agree with Philip that ideally we wouldn't need
this
> additional information at the DOM layer at all (or we should already have all
> the information we need).  In particular, say we did the following:
> 
> - Fix the inverted case of gesture events - that's clearly just a bug
> - Fix the invalid combinations of Event states.  I.e.
>   - dispatchEvent returns false IFF Event.defaultPrevented or
> Event.defaultHandled is true

To be clear, the return value per spec should just be !event.defaultPrevented.
defaultHandled seems to be part of the support for our "default event handler"
model, the bits that should look like this in the "spec fantasy", in JS syntax:

var event = new MouseEvent('click', ...);
if (dispatchEvent(event))
  followLinkOrSomething();

(In this model the default action is outside of dispatch, so there is no notion
of "default handled" as part of the event.)

> Then I think the only remaining questions would be:
>  1. Does code running inside dispatchEvent need some way to mark an event as
> "suppressed".
>       HandledSuppressed should be treated exactly the same as HandledSystem,
but
> the distinction is useful for our metrics (did JS get a chance first or not). 
> I'm not sure preserving this distinction is too important though, it probably
> depends on the specific cases.  dtapuska@ WDYT?
> 
>  2. Is it clearer to use a boolean or enum return value?
>       The communicated information should be identical (when you include
> Event.defaultPrevented/defaultHandled).
>       For the lower level, the boolean was no end of confusion as to it's
> semantics.  For Node::dispatchEvent the spec defines the semantics precisely
and
> we can't (probably wouldn't want to) change the exposed JS API so perhaps it's
> not as big of a deal?
> 
> 
> Personally if we can fix the issues at the DOM layer without changing the
> dispatchEvent return type, then I'd probably prefer we keep it - just to keep
> the internal API as close to the spec as possible.  But I don't have a strong
> opinion here, and it sounds like (if the above issues were addressed) Philip
> would probably be OK either way (it's just an implementation style question at
> this point), right?

I would have no issue with making dispatchEvent return an enum of two values, if
the interpretation of the bool is a source of confusion and bugs internally. Is
that really the case though, how did "For Touch/Mouse/Keyboard it is NOT default
prevented or default handled" come about?

[philipj_slow, 2015-12-08 10:32:02]

On 2015/12/07 21:20:27, Rick Byers wrote:
> Chatting with Dave more, I do think the whole problem here comes down to the
> mismatch in model between the "spec fantasy" and our implementation.  There is
> additional logic that we might run both before true dispatch ("suppression")
and
> after it ("default handler") which in our implementation is implemeneted
inside
> of Node::dispatchEvent, but in the spec is considered something outside of
> dispatchEvent.  I doubt it'll be practical anytime soon to rewrite our
> implementation to match the spec here, but we can move closer to it.
> 
> In particular, Dave has already been working to ensure the default handler
logic
> is never invoked for events with isTrusted=false to ensure our behavior
matches
> the spec.  We should probably be working towards the same thing for all
> "suppressed" cases (eg. if JS dispatches an event at a disabled form control,
> should it still be received?).
> 
> Given that difference, I actually think it's quite misleading to have OUR
> dispatchEvent return a boolean.  There aren't just 2 ways an event can be
> handled in our implementation, there are 4.  By returning the enum we can
> explicitly document (and some day ASSERT) the fact that only
> 'HandledApplication' and 'NotHandled' should ever occur for events with
> isTrusted=false, and the other states represent logic which conceptually (from
a
> spec perspective) lives before or after "event dispatching".  By making the
> return type an enum and writing some good comments in the enum definition
we'll
> hopefully encourage developers to more often think about the implications of
> this difference in model.
> 
> Of course if we have a legitimate plan for actually refactoring our
> implementation to match the spec model, then at that point we might want to go
> back to a boolean so that we match it exactly.
> 
> Philip, WDYT?

When thought of as a tool to help figure out how our model differs from the spec
and to contain and reduce those differences, it's hard to not get excited! Would
it be possible to have the spec's two return values as two of enum entries, and
for the two extra entries to be documented as cases to be removed where
possible? I guess that NotHandled and HandledSuppressed might match to the spec
semantics, but I can't tell from the existing documentation.

If Dave (or someone else) make it such that the extra two states can never occur
for the web-exposed dispatchEvent and can ASSERT that, that would be progress
for sure.

[Rick Byers, 2015-12-11 22:36:19]

On 2015/12/08 10:32:02, philipj wrote:
> On 2015/12/07 21:20:27, Rick Byers wrote:
> > Chatting with Dave more, I do think the whole problem here comes down to the
> > mismatch in model between the "spec fantasy" and our implementation.  There
is
> > additional logic that we might run both before true dispatch ("suppression")
> and
> > after it ("default handler") which in our implementation is implemeneted
> inside
> > of Node::dispatchEvent, but in the spec is considered something outside of
> > dispatchEvent.  I doubt it'll be practical anytime soon to rewrite our
> > implementation to match the spec here, but we can move closer to it.
> > 
> > In particular, Dave has already been working to ensure the default handler
> logic
> > is never invoked for events with isTrusted=false to ensure our behavior
> matches
> > the spec.  We should probably be working towards the same thing for all
> > "suppressed" cases (eg. if JS dispatches an event at a disabled form
control,
> > should it still be received?).
> > 
> > Given that difference, I actually think it's quite misleading to have OUR
> > dispatchEvent return a boolean.  There aren't just 2 ways an event can be
> > handled in our implementation, there are 4.  By returning the enum we can
> > explicitly document (and some day ASSERT) the fact that only
> > 'HandledApplication' and 'NotHandled' should ever occur for events with
> > isTrusted=false, and the other states represent logic which conceptually
(from
> a
> > spec perspective) lives before or after "event dispatching".  By making the
> > return type an enum and writing some good comments in the enum definition
> we'll
> > hopefully encourage developers to more often think about the implications of
> > this difference in model.
> > 
> > Of course if we have a legitimate plan for actually refactoring our
> > implementation to match the spec model, then at that point we might want to
go
> > back to a boolean so that we match it exactly.
> > 
> > Philip, WDYT?
> 
> When thought of as a tool to help figure out how our model differs from the
spec
> and to contain and reduce those differences, it's hard to not get excited!
Would
> it be possible to have the spec's two return values as two of enum entries,
and
> for the two extra entries to be documented as cases to be removed where
> possible? I guess that NotHandled and HandledSuppressed might match to the
spec
> semantics, but I can't tell from the existing documentation.

Absolutely, that's my suggestion.  NotHandled and HandledApplication are the two
cases in the spec today (if that's not already clear by the comments, they
should be updated so it is).  The other two should ideally be moved in our
implementation to be set only outside of dispatchEvent to match the spec
semantics.  

> If Dave (or someone else) make it such that the extra two states can never
occur
> for the web-exposed dispatchEvent and can ASSERT that, that would be progress
> for sure.

This may be a good reason for using different enums.  Better than ASSERT, we
should probably work long-term to remove the extra two values from the enum used
by dispatchEvent entirely.  That's potentially a lot of refactoring though, not
sure it's something anyone wants to pick up anytime soon...

[dtapuska, 2016-02-19 22:06:56]

On 2015/12/11 22:36:19, Rick Byers wrote:
> On 2015/12/08 10:32:02, philipj wrote:
> > On 2015/12/07 21:20:27, Rick Byers wrote:
> > > Chatting with Dave more, I do think the whole problem here comes down to
the
> > > mismatch in model between the "spec fantasy" and our implementation. 
There
> is
> > > additional logic that we might run both before true dispatch
("suppression")
> > and
> > > after it ("default handler") which in our implementation is implemeneted
> > inside
> > > of Node::dispatchEvent, but in the spec is considered something outside of
> > > dispatchEvent.  I doubt it'll be practical anytime soon to rewrite our
> > > implementation to match the spec here, but we can move closer to it.
> > > 
> > > In particular, Dave has already been working to ensure the default handler
> > logic
> > > is never invoked for events with isTrusted=false to ensure our behavior
> > matches
> > > the spec.  We should probably be working towards the same thing for all
> > > "suppressed" cases (eg. if JS dispatches an event at a disabled form
> control,
> > > should it still be received?).
> > > 
> > > Given that difference, I actually think it's quite misleading to have OUR
> > > dispatchEvent return a boolean.  There aren't just 2 ways an event can be
> > > handled in our implementation, there are 4.  By returning the enum we can
> > > explicitly document (and some day ASSERT) the fact that only
> > > 'HandledApplication' and 'NotHandled' should ever occur for events with
> > > isTrusted=false, and the other states represent logic which conceptually
> (from
> > a
> > > spec perspective) lives before or after "event dispatching".  By making
the
> > > return type an enum and writing some good comments in the enum definition
> > we'll
> > > hopefully encourage developers to more often think about the implications
of
> > > this difference in model.
> > > 
> > > Of course if we have a legitimate plan for actually refactoring our
> > > implementation to match the spec model, then at that point we might want
to
> go
> > > back to a boolean so that we match it exactly.
> > > 
> > > Philip, WDYT?
> > 
> > When thought of as a tool to help figure out how our model differs from the
> spec
> > and to contain and reduce those differences, it's hard to not get excited!
> Would
> > it be possible to have the spec's two return values as two of enum entries,
> and
> > for the two extra entries to be documented as cases to be removed where
> > possible? I guess that NotHandled and HandledSuppressed might match to the
> spec
> > semantics, but I can't tell from the existing documentation.
> 
> Absolutely, that's my suggestion.  NotHandled and HandledApplication are the
two
> cases in the spec today (if that's not already clear by the comments, they
> should be updated so it is).  The other two should ideally be moved in our
> implementation to be set only outside of dispatchEvent to match the spec
> semantics.  
> 
> > If Dave (or someone else) make it such that the extra two states can never
> occur
> > for the web-exposed dispatchEvent and can ASSERT that, that would be
progress
> > for sure.
> 
> This may be a good reason for using different enums.  Better than ASSERT, we
> should probably work long-term to remove the extra two values from the enum
used
> by dispatchEvent entirely.  That's potentially a lot of refactoring though,
not
> sure it's something anyone wants to pick up anytime soon...

PTAL; I've resurrected the earlier version and rebased it against ToT.

[tkent, 2016-02-22 01:40:22]

https://codereview.chromium.org/1479923002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/EventTarget.cpp (right):

https://codereview.chromium.org/1479923002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/EventTarget.cpp:255: if (!event) {
This check is unnecessary.  It's done in binding layer.

https://codereview.chromium.org/1479923002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/EventTarget.h (right):

https://codereview.chromium.org/1479923002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/EventTarget.h:117: enum class
DispatchEventResult {
nit: You may move this out from EventTarget for code simplicity.

https://codereview.chromium.org/1479923002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/MouseEvent.cpp (right):

https://codereview.chromium.org/1479923002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/MouseEvent.cpp:306: if (result !=
EventTarget::DispatchEventResult::NotHandled)
The logic looks different from the original code.

Original code:
  If |doubleClickEvent| was handled, returns false. Otherwise, returns the
result of |mouseEvent|.
This CL:
  If |mouseEvent| was handled, returns the result of |mouseEvent|.  Otherwise,
returns the result of |doubleClickEvent|.

https://codereview.chromium.org/1479923002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.h (right):

https://codereview.chromium.org/1479923002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.h:182: static
WebInputEventResult
eventDispatchResultToWebInputEventResult(EventTarget::DispatchEventResult);
nit: In the current convention, this function can be named as
|toWebInputEventResult|.

[philipj_slow, 2016-02-22 07:30:18]

https://codereview.chromium.org/1479923002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/EventTarget.h (right):

https://codereview.chromium.org/1479923002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/EventTarget.h:119: NotHandled,
In order to bring this closer to the
https://dom.spec.whatwg.org/#concept-event-dispatch model, can you put the two
cases corresponding to the true/false return values of dispatchEvent at the top,
and give the other two long and scary names as well as comments that they should
not be used?

Maybe NotCanceled, Canceled(ByEventHandler), CanceledByDefaultEventHandler and
CanceledBeforeDispatch, if that's accurate?

[Rick Byers, 2016-02-22 15:16:14]

https://codereview.chromium.org/1479923002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/EventTarget.h (right):

https://codereview.chromium.org/1479923002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/EventTarget.h:119: NotHandled,
On 2016/02/22 07:30:18, philipj_UTC7 wrote:
> In order to bring this closer to the
> https://dom.spec.whatwg.org/#concept-event-dispatch model, can you put the two
> cases corresponding to the true/false return values of dispatchEvent at the
top,
> and give the other two long and scary names as well as comments that they
should
> not be used?
> 
> Maybe NotCanceled, Canceled(ByEventHandler), CanceledByDefaultEventHandler and
> CanceledBeforeDispatch, if that's accurate?

+1.  This is the place to describe how we want the usage of these to evolve over
time to better match the spec.

https://codereview.chromium.org/1479923002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1479923002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:1931:
EventTarget::DispatchEventResult gestureDomEventResult =
eventTarget->dispatchEvent(gestureDomEvent);
There used to be an ASSERT in the GestureEventDispatchMediator that enforced
that preventDefault wasn't supported for gesture events.  Can you preserve that
coverage by adding the ASSERT here (and any below)?

[dtapuska, 2016-02-23 19:14:41]

https://codereview.chromium.org/1479923002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/EventTarget.cpp (right):

https://codereview.chromium.org/1479923002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/EventTarget.cpp:255: if (!event) {
On 2016/02/22 01:40:22, tkent wrote:
> This check is unnecessary.  It's done in binding layer.

Done.

https://codereview.chromium.org/1479923002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/EventTarget.h (right):

https://codereview.chromium.org/1479923002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/EventTarget.h:117: enum class
DispatchEventResult {
On 2016/02/22 01:40:22, tkent wrote:
> nit: You may move this out from EventTarget for code simplicity.

Done.

https://codereview.chromium.org/1479923002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/EventTarget.h:119: NotHandled,
On 2016/02/22 15:16:14, Rick Byers wrote:
> On 2016/02/22 07:30:18, philipj_UTC7 wrote:
> > In order to bring this closer to the
> > https://dom.spec.whatwg.org/#concept-event-dispatch model, can you put the
two
> > cases corresponding to the true/false return values of dispatchEvent at the
> top,
> > and give the other two long and scary names as well as comments that they
> should
> > not be used?
> > 
> > Maybe NotCanceled, Canceled(ByEventHandler), CanceledByDefaultEventHandler
and
> > CanceledBeforeDispatch, if that's accurate?
> 
> +1.  This is the place to describe how we want the usage of these to evolve
over
> time to better match the spec.

Done.

https://codereview.chromium.org/1479923002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/EventTarget.h:119: NotHandled,
On 2016/02/22 07:30:18, philipj_UTC7 wrote:
> In order to bring this closer to the
> https://dom.spec.whatwg.org/#concept-event-dispatch model, can you put the two
> cases corresponding to the true/false return values of dispatchEvent at the
top,
> and give the other two long and scary names as well as comments that they
should
> not be used?
> 
> Maybe NotCanceled, Canceled(ByEventHandler), CanceledByDefaultEventHandler and
> CanceledBeforeDispatch, if that's accurate?

Done.

https://codereview.chromium.org/1479923002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/MouseEvent.cpp (right):

https://codereview.chromium.org/1479923002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/MouseEvent.cpp:306: if (result !=
EventTarget::DispatchEventResult::NotHandled)
On 2016/02/22 01:40:22, tkent wrote:
> The logic looks different from the original code.
> 
> Original code:
>   If |doubleClickEvent| was handled, returns false. Otherwise, returns the
> result of |mouseEvent|.
> This CL:
>   If |mouseEvent| was handled, returns the result of |mouseEvent|.  Otherwise,
> returns the result of |doubleClickEvent|.

Done.

https://codereview.chromium.org/1479923002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1479923002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:1931:
EventTarget::DispatchEventResult gestureDomEventResult =
eventTarget->dispatchEvent(gestureDomEvent);
On 2016/02/22 15:16:14, Rick Byers wrote:
> There used to be an ASSERT in the GestureEventDispatchMediator that enforced
> that preventDefault wasn't supported for gesture events.  Can you preserve
that
> coverage by adding the ASSERT here (and any below)?

Done; I rolled this into a method.

https://codereview.chromium.org/1479923002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/EventHandler.h (right):

https://codereview.chromium.org/1479923002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.h:182: static
WebInputEventResult
eventDispatchResultToWebInputEventResult(EventTarget::DispatchEventResult);
On 2016/02/22 01:40:22, tkent wrote:
> nit: In the current convention, this function can be named as
> |toWebInputEventResult|.

Done.

[tkent, 2016-02-23 23:50:26]

looks ok.  Please address test failures.

[philipj_slow, 2016-02-24 10:19:51]

https://codereview.chromium.org/1479923002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/EventTarget.h (right):

https://codereview.chromium.org/1479923002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/EventTarget.h:83: // Event was not
consumed by application or system.
s/consumed/canceled/, s/application/event handler/ and s/system/default event
handler/ would be more familiar terminology.

https://codereview.chromium.org/1479923002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/EventTarget.h:88:
CanceledByDefaultEventHandler,
Add stern warnings that these two should not be used?

[dtapuska, 2016-02-24 22:14:16]

https://codereview.chromium.org/1479923002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/EventTarget.h (right):

https://codereview.chromium.org/1479923002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/EventTarget.h:83: // Event was not
consumed by application or system.
On 2016/02/24 10:19:51, philipj_UTC7 wrote:
> s/consumed/canceled/, s/application/event handler/ and s/system/default event
> handler/ would be more familiar terminology.

Done.

https://codereview.chromium.org/1479923002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/EventTarget.h:88:
CanceledByDefaultEventHandler,
On 2016/02/24 10:19:51, philipj_UTC7 wrote:
> Add stern warnings that these two should not be used?

Done.

[philipj_slow, 2016-02-25 04:21:27]

New descriptions look good, I haven't looked at anything else though.

https://codereview.chromium.org/1479923002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/EventTarget.h (right):

https://codereview.chromium.org/1479923002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/EventTarget.h:85: // Event was canceled by
event handler; ie. a script handler calling preventDefault.
i.e. is a much more common spelling, in Source/core at least.

https://codereview.chromium.org/1479923002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/EventTarget.h:92: // This result should be
used sparingly; and likely its usage likely indicates
two likely

[dtapuska, 2016-02-25 14:20:24]

PTAL

https://codereview.chromium.org/1479923002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/EventTarget.h (right):

https://codereview.chromium.org/1479923002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/EventTarget.h:85: // Event was canceled by
event handler; ie. a script handler calling preventDefault.
On 2016/02/25 04:21:27, philipj_UTC7 wrote:
> i.e. is a much more common spelling, in Source/core at least.

Done.

https://codereview.chromium.org/1479923002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/EventTarget.h:92: // This result should be
used sparingly; and likely its usage likely indicates
On 2016/02/25 04:21:27, philipj_UTC7 wrote:
> two likely

Done.

[philipj_slow, 2016-02-25 15:23:46]

Checked the whole CL, LGTM % nits.

https://codereview.chromium.org/1479923002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/RejectedPromises.cpp (right):

https://codereview.chromium.org/1479923002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/RejectedPromises.cpp:73: // Log to
console if event was not handled.
s/handled/canceled/, but with the new naming the comment is far less helpful.
Doesn't hurt, though.

https://codereview.chromium.org/1479923002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/SelectionEditor.h (right):

https://codereview.chromium.org/1479923002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionEditor.h:30: #include
"core/events/EventTarget.h"
Put DispatchEventResult in its own header to other headers don't need to pull in
EventTarget.h? It includes a bunch of things in turn and could make a difference
in build time.

https://codereview.chromium.org/1479923002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/Event.h (right):

https://codereview.chromium.org/1479923002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/Event.h:204: DispatchEventResult
eventResult() const
dispatchEventResult or dispatchResult? The result is not of the event, but of a
specific dispatch. If you could make this a free function
dispatchEventResult(const Event& event) in EventTarget.h that would also assert
that the event has already been dispatched, that might be nice. (Or not, you be
the judge.)

https://codereview.chromium.org/1479923002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/EventTarget.cpp (right):

https://codereview.chromium.org/1479923002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/EventTarget.cpp:254:
RefPtrWillBeRawPtr<Event> event = dispatchEvent;
If this is to protect the event for its use in the return clause, it can be
reverted if that is changed.

https://codereview.chromium.org/1479923002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/EventTarget.cpp:273: return
!event->defaultPrevented();
Will this not be equivalent to dispatchEventInternal(event) !=
DispatchEventResult::CanceledByEventHandler? It would be worrying if there were
any difference.

https://codereview.chromium.org/1479923002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/1479923002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLFormControlElement.cpp:542:
DispatchEventResult eventResult =
dispatchEvent(Event::createCancelable(EventTypeNames::invalid));
I think dispatchResult would be a better short name in places like this. Or
dispatchEventResult for maximum line length.

https://codereview.chromium.org/1479923002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):

https://codereview.chromium.org/1479923002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:70:
DispatchEventResult dispatchResult = target->dispatchEvent(pointerEvent);
Like this :)

[dtapuska, 2016-02-25 22:06:10]

https://codereview.chromium.org/1479923002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/RejectedPromises.cpp (right):

https://codereview.chromium.org/1479923002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/RejectedPromises.cpp:73: // Log to
console if event was not handled.
On 2016/02/25 15:23:46, philipj_UTC7 wrote:
> s/handled/canceled/, but with the new naming the comment is far less helpful.
> Doesn't hurt, though.

Done.

https://codereview.chromium.org/1479923002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/editing/SelectionEditor.h (right):

https://codereview.chromium.org/1479923002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/editing/SelectionEditor.h:30: #include
"core/events/EventTarget.h"
On 2016/02/25 15:23:46, philipj_UTC7 wrote:
> Put DispatchEventResult in its own header to other headers don't need to pull
in
> EventTarget.h? It includes a bunch of things in turn and could make a
difference
> in build time.

Done.

https://codereview.chromium.org/1479923002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/Event.h (right):

https://codereview.chromium.org/1479923002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/Event.h:204: DispatchEventResult
eventResult() const
On 2016/02/25 15:23:46, philipj_UTC7 wrote:
> dispatchEventResult or dispatchResult? The result is not of the event, but of
a
> specific dispatch. If you could make this a free function
> dispatchEventResult(const Event& event) in EventTarget.h that would also
assert
> that the event has already been dispatched, that might be nice. (Or not, you
be
> the judge.)

Done.

https://codereview.chromium.org/1479923002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/EventTarget.cpp (right):

https://codereview.chromium.org/1479923002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/EventTarget.cpp:254:
RefPtrWillBeRawPtr<Event> event = dispatchEvent;
On 2016/02/25 15:23:46, philipj_UTC7 wrote:
> If this is to protect the event for its use in the return clause, it can be
> reverted if that is changed.

Done.

https://codereview.chromium.org/1479923002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/EventTarget.cpp:273: return
!event->defaultPrevented();
On 2016/02/25 15:23:46, philipj_UTC7 wrote:
> Will this not be equivalent to dispatchEventInternal(event) !=
> DispatchEventResult::CanceledByEventHandler? It would be worrying if there
were
> any difference.

Done.

https://codereview.chromium.org/1479923002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/1479923002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLFormControlElement.cpp:542:
DispatchEventResult eventResult =
dispatchEvent(Event::createCancelable(EventTypeNames::invalid));
On 2016/02/25 15:23:46, philipj_UTC7 wrote:
> I think dispatchResult would be a better short name in places like this. Or
> dispatchEventResult for maximum line length.

Done.

https://codereview.chromium.org/1479923002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):

https://codereview.chromium.org/1479923002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:70:
DispatchEventResult dispatchResult = target->dispatchEvent(pointerEvent);
On 2016/02/25 15:23:46, philipj_UTC7 wrote:
> Like this :)

Acknowledged.

[tkent, 2016-02-25 23:16:20]

lgtm

[philipj_slow, 2016-02-26 04:11:53]

Looks fabulous!

https://codereview.chromium.org/1479923002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/core.gypi (right):

https://codereview.chromium.org/1479923002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/core.gypi:2646: 'events/EventDispatcResult.h',
I think this typo has upset the bots.

[dtapuska, 2016-02-26 14:48:06]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2016-02-26 14:48:06]

The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com,
tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/1479923002/#ps220001
(title: "Fix typo")

[commit-bot: I haz the power, 2016-02-26 14:48:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1479923002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1479923002/220001

[commit-bot: I haz the power, 2016-02-26 14:57:51]

Description was changed from

==========
Enumerate the return value of dispatchEvent so it is clear.

Change the boolean result of dispatchEvent to an enum class so that
the return value actually makes sense. Gesture events returned
inverted results. Remove some event mediators that aren't required
now that the result code indicates whether if the default handler was
invoked or not.

BUG=560357, 560355, 543611
==========

to

==========
Enumerate the return value of dispatchEvent so it is clear.

Change the boolean result of dispatchEvent to an enum class so that
the return value actually makes sense. Gesture events returned
inverted results. Remove some event mediators that aren't required
now that the result code indicates whether if the default handler was
invoked or not.

BUG=560357, 560355, 543611
==========

[commit-bot: I haz the power, 2016-02-26 14:57:54]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-02-26 14:59:26]

Description was changed from

==========
Enumerate the return value of dispatchEvent so it is clear.

Change the boolean result of dispatchEvent to an enum class so that
the return value actually makes sense. Gesture events returned
inverted results. Remove some event mediators that aren't required
now that the result code indicates whether if the default handler was
invoked or not.

BUG=560357, 560355, 543611
==========

to

==========
Enumerate the return value of dispatchEvent so it is clear.

Change the boolean result of dispatchEvent to an enum class so that
the return value actually makes sense. Gesture events returned
inverted results. Remove some event mediators that aren't required
now that the result code indicates whether if the default handler was
invoked or not.

BUG=560357, 560355, 543611

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

[commit-bot: I haz the power, 2016-02-26 14:59:28]

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

[Rick Byers, 2016-02-26 15:06:42]

I didn't (re)review the whole CL (tkent@ and philip@ are experts here - I'm
unlikely to add anything).  I just wanted to say that I really like the clear
enum definition - thank you!