Return a enumeration of the state of handling of InputEvents.

Return a enumeration of the state of handling of InputEvents.

It was not possible to determine how InputEvents were handled by
scripts or the default handlers.

The determination is required to add uma metrics for determining
the use of passive event listeners.

BUG=543611
Committed: https://crrev.com/5d2e9c3ce9d185794978fee56cc952822a4adf5f
Cr-Commit-Position: refs/heads/master@{#362994}

[dtapuska, 2015-11-23 20:03:41]

Description was changed from

==========
Return a enumeration of the state of handling of InputEvents.

It was not possible to determine how InputEvents were handled by
scripts or the default handlers.

The determination is required to add uma metrics for determining
the use of passive event listeners.

BUG=543611
==========

to

==========
Return a enumeration of the state of handling of InputEvents.

It was not possible to determine how InputEvents were handled by
scripts or the default handlers.

The determination is required to add uma metrics for determining
the use of passive event listeners.

BUG=543611
==========

[dtapuska, 2015-11-23 20:03:42]

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

[Rick Byers, 2015-11-23 21:59:56]

I'm still reviewing this (sadly the 2->3 states takes some thought per site),
but since it's EOD I figured I'd send what I have so far.

[Reviewed all files outside Source/* and Source up to the first half of
EventHandler.cpp)

https://codereview.chromium.org/1463823003/diff/20001/content/renderer/npapi/...
File content/renderer/npapi/webplugin_impl.cc (right):

https://codereview.chromium.org/1463823003/diff/20001/content/renderer/npapi/...
content/renderer/npapi/webplugin_impl.cc:438: return ret ?
WebInputEventResult::HandledDefaultHandler
'true' here can mean two things here (script or default), right?  If you'll need
your second phase to distinguish then please add a TODO to track.

https://codereview.chromium.org/1463823003/diff/20001/content/renderer/render...
File content/renderer/render_widget_fullscreen_pepper.cc (right):

https://codereview.chromium.org/1463823003/diff/20001/content/renderer/render...
content/renderer/render_widget_fullscreen_pepper.cc:212: return result ?
WebInputEventResult::HandledDefaultHandler
Are you sure handled events here are all "default"?  I guess in the general
plugin case (not browser plugin) there's no real distinction between default vs.
script, right?  Browser plugin definitely has that distinction but presumably we
don't hit this code path for webview (wjmaclean@ is the expert).

https://codereview.chromium.org/1463823003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1463823003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:107: WebInputEventResult
mergeEventResult(WebInputEventResult responseA, WebInputEventResult responseB)
I _think_ we should never be "upgrading" a script-handled to a default-handled
state, or calling this with two "default handled" states.  So rather than
"merge" this is really just returning the input, or possibly changing
NotHandled->ScriptHandled.  Maybe the "merge" name is fine but should probably
RELEASE_ASSERT that we're not doing an invalid transition.

https://codereview.chromium.org/1463823003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:112: WebInputEventResult
boolToEventResult(bool value)
Seems like any time you'd need this might be a potential bug confusing handled
vs. default handled, right?  Maybe better to inline (and leave TODOs for
possible outstanding issues)?

https://codereview.chromium.org/1463823003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:128: return
WebInputEventResult::HandledDefaultHandler;
I think technically any handlers that run within the context of dispatchEvent
are not, by definition, "default handlers" (they can be triggered by synthetic
JS).  Although IIRC there are one or two well known exceptions for legacy compat
(click events on checkboxes or something).

https://codereview.chromium.org/1463823003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1026: return
WebInputEventResult::HandledDefaultHandler;
this could be either I think.  Ultimately we probably need to propagate this
from passMousePressEventToSubframe, right?

https://codereview.chromium.org/1463823003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1059: WebInputEventResult
swallowEvent = updatePointerTargetAndDispatchEvents(EventTypeNames::mousedown,
mev.innerNode(), m_clickCount, mouseEvent);
nit: rename to 'eventResult'

https://codereview.chromium.org/1463823003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1347: return
WebInputEventResult::HandledDefaultHandler;
ditto

https://codereview.chromium.org/1463823003/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebInputEventResult.h (right):

https://codereview.chromium.org/1463823003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebInputEventResult.h:12: HandledScript, //
consumed by a script handler (ie. preventDefault called)
I wonder, is "script" too restrictive here?  What if we have a case where we
handle the event in C++ outside the default handler.  I know this violates
Anne's model for DOM events, but I think a couple places may exist and it may be
necessary for some layered-platform scenarios.

So we could call this HandledNonDefault or maybe even just Handled.

Or maybe 'Script' is the best way to describe this concisely and we should just
clarify in the comment that in rare cases this may still mean C++ code.

Or maybe (after looking at your use cases) the real distinction here IS script
vs. internal.  In which case HandledScript vs. HandledInternal may be the right
terminology.

The key question is: for all cases where you're saying HandledDefaultHandler,
are we sure that logic runs only when the event has gone unhandled in JS, and is
never run via dispatching a synthetic event from JS.

[Rick Byers, 2015-11-24 01:47:35]

/cc mustaq since he's worked in a bunch of this code

[dtapuska, 2015-11-24 15:23:02]

https://codereview.chromium.org/1463823003/diff/20001/content/renderer/npapi/...
File content/renderer/npapi/webplugin_impl.cc (right):

https://codereview.chromium.org/1463823003/diff/20001/content/renderer/npapi/...
content/renderer/npapi/webplugin_impl.cc:438: return ret ?
WebInputEventResult::HandledDefaultHandler
On 2015/11/23 21:59:55, Rick Byers (slow until Nov 23) wrote:
> 'true' here can mean two things here (script or default), right?  If you'll
need
> your second phase to distinguish then please add a TODO to track.

see other comment in the fullscreen plugin.

https://codereview.chromium.org/1463823003/diff/20001/content/renderer/render...
File content/renderer/render_widget_fullscreen_pepper.cc (right):

https://codereview.chromium.org/1463823003/diff/20001/content/renderer/render...
content/renderer/render_widget_fullscreen_pepper.cc:212: return result ?
WebInputEventResult::HandledDefaultHandler
On 2015/11/23 21:59:55, Rick Byers (slow until Nov 23) wrote:
> Are you sure handled events here are all "default"?  I guess in the general
> plugin case (not browser plugin) there's no real distinction between default
vs.
> script, right?  Browser plugin definitely has that distinction but presumably
we
> don't hit this code path for webview (wjmaclean@ is the expert).

I didn't want to change the pepper API because I didn't think it made sense to
propagate this any further into the plugin code. And since the plugin got it; I
marked it as handledDefaultHandler

I presumed webview plugins went via
components/plugins/renderer/webview_plugin.cc which are then propagated
correctly.

https://codereview.chromium.org/1463823003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1463823003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:107: WebInputEventResult
mergeEventResult(WebInputEventResult responseA, WebInputEventResult responseB)
On 2015/11/23 21:59:55, Rick Byers (slow until Nov 23) wrote:
> I _think_ we should never be "upgrading" a script-handled to a default-handled
> state, or calling this with two "default handled" states.  So rather than
> "merge" this is really just returning the input, or possibly changing
> NotHandled->ScriptHandled.  Maybe the "merge" name is fine but should probably
> RELEASE_ASSERT that we're not doing an invalid transition.

Look at the handleGestureTap case. It generates a mousedown which could be
HandledScript and a mouseup which could be HandledDefaultHandler (I don't know
if there is a default handler for mouseup specifically). But logically the code
could return it.

And that would have to merge (HandleScript, HandleDefaultHandler) to provide
something that is more definitive; like HandleDefaultHandler.

https://codereview.chromium.org/1463823003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:112: WebInputEventResult
boolToEventResult(bool value)
On 2015/11/23 21:59:55, Rick Byers (slow until Nov 23) wrote:
> Seems like any time you'd need this might be a potential bug confusing handled
> vs. default handled, right?  Maybe better to inline (and leave TODOs for
> possible outstanding issues)?

I didn't want to propagate the WebInputEventResult everywhere it was logical to
return a boolean. For example did I update the drop target? Yes or no. But I can
certainly replace inline; but there may not be TODOs to actually do.

https://codereview.chromium.org/1463823003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:128: return
WebInputEventResult::HandledDefaultHandler;
On 2015/11/23 21:59:55, Rick Byers (slow until Nov 23) wrote:
> I think technically any handlers that run within the context of dispatchEvent
> are not, by definition, "default handlers" (they can be triggered by synthetic
> JS).  Although IIRC there are one or two well known exceptions for legacy
compat
> (click events on checkboxes or something).

I was more or less trying to capture "did the script call preventDefault?" And
everything else was the default handler. But we can discuss this when you got a
minute.

[dtapuska, 2015-11-24 22:09:38]

https://codereview.chromium.org/1463823003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1463823003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1026: return
WebInputEventResult::HandledDefaultHandler;
On 2015/11/23 21:59:55, Rick Byers wrote:
> this could be either I think.  Ultimately we probably need to propagate this
> from passMousePressEventToSubframe, right?

Done.

https://codereview.chromium.org/1463823003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1059: WebInputEventResult
swallowEvent = updatePointerTargetAndDispatchEvents(EventTypeNames::mousedown,
mev.innerNode(), m_clickCount, mouseEvent);
On 2015/11/23 21:59:55, Rick Byers wrote:
> nit: rename to 'eventResult'

Done.

https://codereview.chromium.org/1463823003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1347: return
WebInputEventResult::HandledDefaultHandler;
On 2015/11/23 21:59:55, Rick Byers wrote:
> ditto

Done.

[dtapuska, 2015-11-24 22:10:25]

https://codereview.chromium.org/1463823003/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebInputEventResult.h (right):

https://codereview.chromium.org/1463823003/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebInputEventResult.h:12: NotHandled,
I was going to put NotHandled at the end of this list. But it made the
mergeEventResult code less simple.

[Rick Byers, 2015-11-27 21:31:53]

Looks great, thanks.  Just a couple specific places to discuss a little...

https://codereview.chromium.org/1463823003/diff/80001/content/renderer/npapi/...
File content/renderer/npapi/webplugin_impl.cc (right):

https://codereview.chromium.org/1463823003/diff/80001/content/renderer/npapi/...
content/renderer/npapi/webplugin_impl.cc:438: return ret ?
WebInputEventResult::HandledSystem
A plugin probably counts as "application", don't you think?

https://codereview.chromium.org/1463823003/diff/80001/content/renderer/pepper...
File content/renderer/pepper/pepper_webplugin_impl.cc (right):

https://codereview.chromium.org/1463823003/diff/80001/content/renderer/pepper...
content/renderer/pepper/pepper_webplugin_impl.cc:228: ?
blink::WebInputEventResult::HandledSystem
ditto - application?

https://codereview.chromium.org/1463823003/diff/80001/content/renderer/render...
File content/renderer/render_widget_fullscreen_pepper.cc (right):

https://codereview.chromium.org/1463823003/diff/80001/content/renderer/render...
content/renderer/render_widget_fullscreen_pepper.cc:248: return result ?
WebInputEventResult::HandledSystem
ditto

https://codereview.chromium.org/1463823003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (left):

https://codereview.chromium.org/1463823003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1350:
clickTargetNode->dispatchMouseEvent(mouseEvent,
oh weird - looks like we forgot (or lost) the code that sets swallowClickEvent. 
Any idea what this might impact (i.e. what actually uses the result here)?

https://codereview.chromium.org/1463823003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1463823003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:107: WebInputEventResult
mergeEventResult(WebInputEventResult responseA, WebInputEventResult responseB)
This deserves a comment I think (and probably a warning in the enum definition
that order is significant).  I guess it's something like "we want to know the
first thing in the pipeline (if any) that consumed the event".

https://codereview.chromium.org/1463823003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1013: WebInputEventResult
result = passMousePressEventToSubframe(mev, subframe.get());
this capturing logic needs to be done only when the result indicates it was
handled (IIRC this is your bug about preventDefault causing an odd behavior
change - please add a TODO referring to that bug).

https://codereview.chromium.org/1463823003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right):

https://codereview.chromium.org/1463823003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:826: return
WebInputEventResult::HandledSystem;
since this event never makes it to the application, perhaps it should be
"Suppressed" instead?

[mustaq, 2015-11-30 16:02:50]

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

[mustaq, 2015-11-30 16:02:50]

https://codereview.chromium.org/1463823003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (left):

https://codereview.chromium.org/1463823003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1350:
clickTargetNode->dispatchMouseEvent(mouseEvent,
On 2015/11/27 21:31:53, Rick Byers wrote:
> oh weird - looks like we forgot (or lost) the code that sets
swallowClickEvent. 
> Any idea what this might impact (i.e. what actually uses the result here)?

Yes, swallowClickEvent should have been there, and I accidentally dropped it in
https://codereview.chromium.org/1402133003/, sorry.
InspectorOverlay::handleInputEvent() is the only "user" of the return value of
this function, so it's not surprising that we didn't see a failure.

[dtapuska, 2015-11-30 16:15:43]

https://codereview.chromium.org/1463823003/diff/80001/content/renderer/browse...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/1463823003/diff/80001/content/renderer/browse...
content/renderer/browser_plugin/browser_plugin.cc:527: return
blink::WebInputEventResult::HandledSystem;
Adjusted here

https://codereview.chromium.org/1463823003/diff/80001/content/renderer/npapi/...
File content/renderer/npapi/webplugin_impl.cc (right):

https://codereview.chromium.org/1463823003/diff/80001/content/renderer/npapi/...
content/renderer/npapi/webplugin_impl.cc:438: return ret ?
WebInputEventResult::HandledSystem
On 2015/11/27 21:31:52, Rick Byers wrote:
> A plugin probably counts as "application", don't you think?

Done.

https://codereview.chromium.org/1463823003/diff/80001/content/renderer/pepper...
File content/renderer/pepper/pepper_webplugin_impl.cc (right):

https://codereview.chromium.org/1463823003/diff/80001/content/renderer/pepper...
content/renderer/pepper/pepper_webplugin_impl.cc:228: ?
blink::WebInputEventResult::HandledSystem
On 2015/11/27 21:31:52, Rick Byers wrote:
> ditto - application?

Done.

https://codereview.chromium.org/1463823003/diff/80001/content/renderer/render...
File content/renderer/render_widget_fullscreen_pepper.cc (right):

https://codereview.chromium.org/1463823003/diff/80001/content/renderer/render...
content/renderer/render_widget_fullscreen_pepper.cc:212: return result ?
WebInputEventResult::HandledSystem
Adjusted here

https://codereview.chromium.org/1463823003/diff/80001/content/renderer/render...
content/renderer/render_widget_fullscreen_pepper.cc:248: return result ?
WebInputEventResult::HandledSystem
On 2015/11/27 21:31:53, Rick Byers wrote:
> ditto

Done.

https://codereview.chromium.org/1463823003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (left):

https://codereview.chromium.org/1463823003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1350:
clickTargetNode->dispatchMouseEvent(mouseEvent,
On 2015/11/30 16:02:50, mustaq wrote:
> On 2015/11/27 21:31:53, Rick Byers wrote:
> > oh weird - looks like we forgot (or lost) the code that sets
> swallowClickEvent. 
> > Any idea what this might impact (i.e. what actually uses the result here)?
> 
> Yes, swallowClickEvent should have been there, and I accidentally dropped it
in
> https://codereview.chromium.org/1402133003/, sorry.
> InspectorOverlay::handleInputEvent() is the only "user" of the return value of
> this function, so it's not surprising that we didn't see a failure.

I've fixed it; although I return HandledApplication because I didn't want to do
extra work that will be undone in the DOM dispatch event result change that is
up for review.

https://codereview.chromium.org/1463823003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1463823003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:107: WebInputEventResult
mergeEventResult(WebInputEventResult responseA, WebInputEventResult responseB)
On 2015/11/27 21:31:53, Rick Byers wrote:
> This deserves a comment I think (and probably a warning in the enum definition
> that order is significant).  I guess it's something like "we want to know the
> first thing in the pipeline (if any) that consumed the event".

done; and static asserts added

https://codereview.chromium.org/1463823003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1013: WebInputEventResult
result = passMousePressEventToSubframe(mev, subframe.get());
On 2015/11/27 21:31:53, Rick Byers wrote:
> this capturing logic needs to be done only when the result indicates it was
> handled (IIRC this is your bug about preventDefault causing an odd behavior
> change - please add a TODO referring to that bug).

Done.

https://codereview.chromium.org/1463823003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right):

https://codereview.chromium.org/1463823003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:826: return
WebInputEventResult::HandledSystem;
On 2015/11/27 21:31:53, Rick Byers wrote:
> since this event never makes it to the application, perhaps it should be
> "Suppressed" instead?

I think that would be odd this is UA behaviour; although I'd probably argue
there is a bug here in that Firefox seems to always be dispatching the keypress
event. (opened bug 563507)

[Rick Byers, 2015-12-02 14:52:10]

https://codereview.chromium.org/1463823003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1463823003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1013: WebInputEventResult
result = passMousePressEventToSubframe(mev, subframe.get());
On 2015/11/30 16:15:43, dtapuska wrote:
> On 2015/11/27 21:31:53, Rick Byers wrote:
> > this capturing logic needs to be done only when the result indicates it was
> > handled (IIRC this is your bug about preventDefault causing an odd behavior
> > change - please add a TODO referring to that bug).
> 
> Done.

You're still missing the test.  Previously we'd go down this code path only when
passMousePressEventToSubframe returned true, now we do this WHENEVER a mouse
event occurs over an iframe, changing the behavior - right?

https://codereview.chromium.org/1463823003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right):

https://codereview.chromium.org/1463823003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:826: return
WebInputEventResult::HandledSystem;
On 2015/11/30 16:15:43, dtapuska wrote:
> On 2015/11/27 21:31:53, Rick Byers wrote:
> > since this event never makes it to the application, perhaps it should be
> > "Suppressed" instead?
> 
> I think that would be odd this is UA behaviour; although I'd probably argue
> there is a bug here in that Firefox seems to always be dispatching the
keypress
> event. (opened bug 563507)
> 

Ok, so we think it's a bug that we're suppressing this, it really should be the
default action.  I guess it's OK if you prefer to have the return value indicate
what the code is _supposed_ to be doing (not what it actually did) as long as
you plan to try to fix this one way or another.

[Rick Byers, 2015-12-02 16:09:28]

On 2015/12/02 14:52:10, Rick Byers wrote:
>
https://codereview.chromium.org/1463823003/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/input/EventHandler.cpp (right):
> 
>
https://codereview.chromium.org/1463823003/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/input/EventHandler.cpp:1013:
WebInputEventResult
> result = passMousePressEventToSubframe(mev, subframe.get());
> On 2015/11/30 16:15:43, dtapuska wrote:
> > On 2015/11/27 21:31:53, Rick Byers wrote:
> > > this capturing logic needs to be done only when the result indicates it
was
> > > handled (IIRC this is your bug about preventDefault causing an odd
behavior
> > > change - please add a TODO referring to that bug).
> > 
> > Done.
> 
> You're still missing the test.  Previously we'd go down this code path only
when
> passMousePressEventToSubframe returned true, now we do this WHENEVER a mouse
> event occurs over an iframe, changing the behavior - right?

Dave convinced me that this test wasn't responsible for the behavior I thought
it was.  passMousePressEventToSubframe was always returning true, so this isn't
changing the behavior.
It must be some other code somewhere responsible for the weird "uncanceled
mousedown causes
mouse events to be captured by an iframe" behavior.

So LGTM now.
 
>
https://codereview.chromium.org/1463823003/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right):
> 
>
https://codereview.chromium.org/1463823003/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:826: return
> WebInputEventResult::HandledSystem;
> On 2015/11/30 16:15:43, dtapuska wrote:
> > On 2015/11/27 21:31:53, Rick Byers wrote:
> > > since this event never makes it to the application, perhaps it should be
> > > "Suppressed" instead?
> > 
> > I think that would be odd this is UA behaviour; although I'd probably argue
> > there is a bug here in that Firefox seems to always be dispatching the
> keypress
> > event. (opened bug 563507)
> > 
> 
> Ok, so we think it's a bug that we're suppressing this, it really should be
the
> default action.  I guess it's OK if you prefer to have the return value
indicate
> what the code is _supposed_ to be doing (not what it actually did) as long as
> you plan to try to fix this one way or another.

[dtapuska, 2015-12-02 16:32:21]

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

[dtapuska, 2015-12-02 16:34:12]

On 2015/12/02 16:09:28, Rick Byers wrote:
> On 2015/12/02 14:52:10, Rick Byers wrote:
> >
>
https://codereview.chromium.org/1463823003/diff/80001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/input/EventHandler.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1463823003/diff/80001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/input/EventHandler.cpp:1013:
> WebInputEventResult
> > result = passMousePressEventToSubframe(mev, subframe.get());
> > On 2015/11/30 16:15:43, dtapuska wrote:
> > > On 2015/11/27 21:31:53, Rick Byers wrote:
> > > > this capturing logic needs to be done only when the result indicates it
> was
> > > > handled (IIRC this is your bug about preventDefault causing an odd
> behavior
> > > > change - please add a TODO referring to that bug).
> > > 
> > > Done.
> > 
> > You're still missing the test.  Previously we'd go down this code path only
> when
> > passMousePressEventToSubframe returned true, now we do this WHENEVER a mouse
> > event occurs over an iframe, changing the behavior - right?
> 
> Dave convinced me that this test wasn't responsible for the behavior I thought
> it was.  passMousePressEventToSubframe was always returning true, so this
isn't
> changing the behavior.
> It must be some other code somewhere responsible for the weird "uncanceled
> mousedown causes
> mouse events to be captured by an iframe" behavior.
> 
> So LGTM now.
>  
> >
>
https://codereview.chromium.org/1463823003/diff/80001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1463823003/diff/80001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp:826: return
> > WebInputEventResult::HandledSystem;
> > On 2015/11/30 16:15:43, dtapuska wrote:
> > > On 2015/11/27 21:31:53, Rick Byers wrote:
> > > > since this event never makes it to the application, perhaps it should be
> > > > "Suppressed" instead?
> > > 
> > > I think that would be odd this is UA behaviour; although I'd probably
argue
> > > there is a bug here in that Firefox seems to always be dispatching the
> > keypress
> > > event. (opened bug 563507)
> > > 
> > 
> > Ok, so we think it's a bug that we're suppressing this, it really should be
> the
> > default action.  I guess it's OK if you prefer to have the return value
> indicate
> > what the code is _supposed_ to be doing (not what it actually did) as long
as
> > you plan to try to fix this one way or another.

+jochen for [components|contents]/*

[jochen (gone - plz use gerrit), 2015-12-03 14:56:42]

lgtm

[dtapuska, 2015-12-03 14:59:49]

The CQ bit was checked by dtapuska@chromium.org

[commit-bot: I haz the power, 2015-12-03 15:00:11]

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

[commit-bot: I haz the power, 2015-12-03 16:39:59]

Description was changed from

==========
Return a enumeration of the state of handling of InputEvents.

It was not possible to determine how InputEvents were handled by
scripts or the default handlers.

The determination is required to add uma metrics for determining
the use of passive event listeners.

BUG=543611
==========

to

==========
Return a enumeration of the state of handling of InputEvents.

It was not possible to determine how InputEvents were handled by
scripts or the default handlers.

The determination is required to add uma metrics for determining
the use of passive event listeners.

BUG=543611
==========

[commit-bot: I haz the power, 2015-12-03 16:40:00]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-12-03 16:40:45]

Description was changed from

==========
Return a enumeration of the state of handling of InputEvents.

It was not possible to determine how InputEvents were handled by
scripts or the default handlers.

The determination is required to add uma metrics for determining
the use of passive event listeners.

BUG=543611
==========

to

==========
Return a enumeration of the state of handling of InputEvents.

It was not possible to determine how InputEvents were handled by
scripts or the default handlers.

The determination is required to add uma metrics for determining
the use of passive event listeners.

BUG=543611

Committed: https://crrev.com/5d2e9c3ce9d185794978fee56cc952822a4adf5f
Cr-Commit-Position: refs/heads/master@{#362994}
==========

[commit-bot: I haz the power, 2015-12-03 16:40:46]

Patchset 7 (id:??) landed as
https://crrev.com/5d2e9c3ce9d185794978fee56cc952822a4adf5f
Cr-Commit-Position: refs/heads/master@{#362994}

[kotenkov, 2016-01-12 20:55:00]

Sorry for digging this up, but I have a couple of questions. It seems to me that
there is an incorrect handling of some cases here, dtapuska@, would you please
take a look at my comments and correct me if I'm wrong?

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

https://codereview.chromium.org/1463823003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:1384: if
(clickTargetNode->dispatchMouseEvent(mouseEvent,
There should be a negation here: |EventTarget::dispatchEvent()| returns true if
default was not prevented.
It doesn't affect anything in today's code, because the return value of this
function is ignored.

https://codereview.chromium.org/1463823003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/PageWidgetDelegate.cpp (right):

https://codereview.chromium.org/1463823003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/PageWidgetDelegate.cpp:145: return
WebInputEventResult::HandledSystem;
It seems to me that there are a lot of changes in EventHandler.cpp, that are
simply not used. The call above will lead to
|EventHanlder::handleMouseRelease()| that may return any of the elements of the
enum. But we ignore that value and always return
WebInputEventResult::HandledSystem. The same is true for
WebInputEvent::MouseMove and some others. It seems to me that there is an error
here: either we should pay attention to values returned from EventHandler
functions, or we shouldn't have changed anything there.

[dtapuska, 2016-01-12 21:06:48]

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

https://codereview.chromium.org/1463823003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/EventHandler.cpp:1384: if
(clickTargetNode->dispatchMouseEvent(mouseEvent,
On 2016/01/12 20:55:00, kotenkov wrote:
> There should be a negation here: |EventTarget::dispatchEvent()| returns true
if
> default was not prevented.
> It doesn't affect anything in today's code, because the return value of this
> function is ignored.

Thanks for the find; you are correct. This is in fact fixed in a follow up
change here https://codereview.chromium.org/1479923002/ that I need to get back
to working on.

https://codereview.chromium.org/1463823003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/PageWidgetDelegate.cpp (right):

https://codereview.chromium.org/1463823003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/PageWidgetDelegate.cpp:145: return
WebInputEventResult::HandledSystem;
On 2016/01/12 20:55:00, kotenkov wrote:
> It seems to me that there are a lot of changes in EventHandler.cpp, that are
> simply not used. The call above will lead to
> |EventHanlder::handleMouseRelease()| that may return any of the elements of
the
> enum. But we ignore that value and always return
> WebInputEventResult::HandledSystem. The same is true for
> WebInputEvent::MouseMove and some others. It seems to me that there is an
error
> here: either we should pay attention to values returned from EventHandler
> functions, or we shouldn't have changed anything there.

I agree the return values should be used. I wasn't trying to clean up all the
cruft; and that change was larger than I wanted to make this change. Feel free
to go ahead and fix it if you like.

[kotenkov, 2016-01-15 07:56:15]

On 2016/01/12 21:06:48, dtapuska wrote:
>
https://codereview.chromium.org/1463823003/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/input/EventHandler.cpp (right):
> 
>
https://codereview.chromium.org/1463823003/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/input/EventHandler.cpp:1384: if
> (clickTargetNode->dispatchMouseEvent(mouseEvent,
> On 2016/01/12 20:55:00, kotenkov wrote:
> > There should be a negation here: |EventTarget::dispatchEvent()| returns true
> if
> > default was not prevented.
> > It doesn't affect anything in today's code, because the return value of this
> > function is ignored.
> 
> Thanks for the find; you are correct. This is in fact fixed in a follow up
> change here https://codereview.chromium.org/1479923002/ that I need to get
back
> to working on.
> 
>
https://codereview.chromium.org/1463823003/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/web/PageWidgetDelegate.cpp (right):
> 
>
https://codereview.chromium.org/1463823003/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/web/PageWidgetDelegate.cpp:145: return
> WebInputEventResult::HandledSystem;
> On 2016/01/12 20:55:00, kotenkov wrote:
> > It seems to me that there are a lot of changes in EventHandler.cpp, that are
> > simply not used. The call above will lead to
> > |EventHanlder::handleMouseRelease()| that may return any of the elements of
> the
> > enum. But we ignore that value and always return
> > WebInputEventResult::HandledSystem. The same is true for
> > WebInputEvent::MouseMove and some others. It seems to me that there is an
> error
> > here: either we should pay attention to values returned from EventHandler
> > functions, or we shouldn't have changed anything there.
> 
> I agree the return values should be used. I wasn't trying to clean up all the
> cruft; and that change was larger than I wanted to make this change. Feel free
> to go ahead and fix it if you like.

Thanks for the clarification, i'll try to fix the issue with unused returns.