Forward mouse down node information to WebWidgetClient

Forward mouse down node information to WebWidgetClient

There are some chrome clients, e.g. PageClickTracker, wants
to listen on mouse press and gesture tap event to get the
information of the last clicked node. Currently clients
do an extra call of hit test on the mouse/tap event to get
the click node after the event has been processed in blink.
This is wasteful since the event processing part has already
done the hit test and known the clicked node.

So lets get rid of this extra call of hit testing. At the
end of mouse press/tap event processing, we can pass the
mouse down node information to WebWidgetClient through
onMouseDown(const WebNode&).

BUG=498150
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197400

[Yufeng Shen (Slow to review), 2015-06-17 19:47:42]

miletus@chromium.org changed reviewers:
+ esprehn@chromium.org, rouslan@chromium.org

[Yufeng Shen (Slow to review), 2015-06-17 19:52:31]

On 2015/06/17 19:47:48, Yufeng Shen wrote:

chrome-side change is here
https://codereview.chromium.org/1195473005/

[please use gerrit instead, 2015-06-17 20:07:08]

https://codereview.chromium.org/1176423008/diff/1/Source/core/input/EventHand...
File Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1176423008/diff/1/Source/core/input/EventHand...
Source/core/input/EventHandler.cpp:1001: if (mev.hitTestResult().innerNode() &&
mouseEvent.button() == LeftButton) {
&& mouseEvent.type() == PlatformMouseEvent::MousePressed

https://codereview.chromium.org/1176423008/diff/1/Source/core/input/EventHand...
Source/core/input/EventHandler.cpp:2084: if (currentHitTest.innerNode()) {
&& gestureEvent.type() == PlatformEvent::GestureTap

https://codereview.chromium.org/1176423008/diff/1/Source/core/page/ChromeClie...
File Source/core/page/ChromeClient.h (right):

https://codereview.chromium.org/1176423008/diff/1/Source/core/page/ChromeClie...
Source/core/page/ChromeClient.h:242: virtual void onClickInput(Node*) { }
Should this be called "onClickNode(Node*)" instead? I don't think it's
guaranteed to be an input field.

[Yufeng Shen (Slow to review), 2015-06-17 20:12:57]

https://codereview.chromium.org/1176423008/diff/1/Source/core/input/EventHand...
File Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1176423008/diff/1/Source/core/input/EventHand...
Source/core/input/EventHandler.cpp:1001: if (mev.hitTestResult().innerNode() &&
mouseEvent.button() == LeftButton) {
On 2015/06/17 20:07:07, Rouslan wrote:
> && mouseEvent.type() == PlatformMouseEvent::MousePressed

This is inside handleMousePressEvent(), so it is guaranteed a
MousePressed event.

https://codereview.chromium.org/1176423008/diff/1/Source/core/input/EventHand...
Source/core/input/EventHandler.cpp:2084: if (currentHitTest.innerNode()) {
On 2015/06/17 20:07:07, Rouslan wrote:
> && gestureEvent.type() == PlatformEvent::GestureTap

ditto, this is inside handleGestureTap().

https://codereview.chromium.org/1176423008/diff/1/Source/core/page/ChromeClie...
File Source/core/page/ChromeClient.h (right):

https://codereview.chromium.org/1176423008/diff/1/Source/core/page/ChromeClie...
Source/core/page/ChromeClient.h:242: virtual void onClickInput(Node*) { }
On 2015/06/17 20:07:08, Rouslan wrote:
> Should this be called "onClickNode(Node*)" instead? I don't think it's
> guaranteed to be an input field.

oh, I think the name actually means the click is an "input event", not meaning
the Node is an "input" node. And later on we might want to 
pass more information out other than the Node*.

Does that make sense ?

[please use gerrit instead, 2015-06-17 20:58:06]

https://codereview.chromium.org/1176423008/diff/1/Source/core/input/EventHand...
File Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1176423008/diff/1/Source/core/input/EventHand...
Source/core/input/EventHandler.cpp:1001: if (mev.hitTestResult().innerNode() &&
mouseEvent.button() == LeftButton) {
On 2015/06/17 20:12:57, Yufeng Shen wrote:
> On 2015/06/17 20:07:07, Rouslan wrote:
> > && mouseEvent.type() == PlatformMouseEvent::MousePressed
> 
> This is inside handleMousePressEvent(), so it is guaranteed a
> MousePressed event.

In that case, please "ASSERT(mouseEvent.type() ==
PlatformMouseEvent::MousePressed);" to document this assumption.

https://codereview.chromium.org/1176423008/diff/1/Source/core/input/EventHand...
Source/core/input/EventHandler.cpp:2084: if (currentHitTest.innerNode()) {
On 2015/06/17 20:12:56, Yufeng Shen wrote:
> On 2015/06/17 20:07:07, Rouslan wrote:
> > && gestureEvent.type() == PlatformEvent::GestureTap
> 
> ditto, this is inside handleGestureTap().

Please "ASSERT(gestureEvent.type() == PlatformEvent::GestureTap);" to document
this assumption. No worries about performance, this is compiled out in release
mode.

https://codereview.chromium.org/1176423008/diff/1/Source/core/page/ChromeClie...
File Source/core/page/ChromeClient.h (right):

https://codereview.chromium.org/1176423008/diff/1/Source/core/page/ChromeClie...
Source/core/page/ChromeClient.h:242: virtual void onClickInput(Node*) { }
On 2015/06/17 20:12:57, Yufeng Shen wrote:
> On 2015/06/17 20:07:08, Rouslan wrote:
> > Should this be called "onClickNode(Node*)" instead? I don't think it's
> > guaranteed to be an input field.
> 
> oh, I think the name actually means the click is an "input event", not meaning
> the Node is an "input" node. And later on we might want to 
> pass more information out other than the Node*.
> 
> Does that make sense ?

Sure.

[Yufeng Shen (Slow to review), 2015-06-17 22:25:50]

https://codereview.chromium.org/1176423008/diff/1/Source/core/input/EventHand...
File Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1176423008/diff/1/Source/core/input/EventHand...
Source/core/input/EventHandler.cpp:1001: if (mev.hitTestResult().innerNode() &&
mouseEvent.button() == LeftButton) {
On 2015/06/17 20:58:05, Rouslan wrote:
> On 2015/06/17 20:12:57, Yufeng Shen wrote:
> > On 2015/06/17 20:07:07, Rouslan wrote:
> > > && mouseEvent.type() == PlatformMouseEvent::MousePressed
> > 
> > This is inside handleMousePressEvent(), so it is guaranteed a
> > MousePressed event.
> 
> In that case, please "ASSERT(mouseEvent.type() ==
> PlatformMouseEvent::MousePressed);" to document this assumption.

Done.

https://codereview.chromium.org/1176423008/diff/1/Source/core/input/EventHand...
Source/core/input/EventHandler.cpp:2084: if (currentHitTest.innerNode()) {
On 2015/06/17 20:58:05, Rouslan wrote:
> On 2015/06/17 20:12:56, Yufeng Shen wrote:
> > On 2015/06/17 20:07:07, Rouslan wrote:
> > > && gestureEvent.type() == PlatformEvent::GestureTap
> > 
> > ditto, this is inside handleGestureTap().
> 
> Please "ASSERT(gestureEvent.type() == PlatformEvent::GestureTap);" to document
> this assumption. No worries about performance, this is compiled out in release
> mode.

Done.

[please use gerrit instead, 2015-06-17 22:28:00]

lgtm on my side, but you need reviews from esprehn and/or OWNERs.

[esprehn, 2015-06-18 04:58:54]

Does PageClickTracker actually care about all elements? Can we filter this to a
specific kind of element instead?

lgtm

[Yufeng Shen (Slow to review), 2015-06-18 16:07:28]

miletus@chromium.org changed reviewers:
+ chrishtr@chromium.org

[Yufeng Shen (Slow to review), 2015-06-18 16:12:20]

+ Chris for OWNER of public/web/*


> Does PageClickTracker actually care about all elements? Can we filter this to
a
> specific kind of element instead?
> 

it looks like all PageClickTracer does with the clicked node is setting

 focused_node_was_last_clicked_ = !clicked_node.isNull() &&
                                   clicked_node.focused();

So we might be able to filter this on only focusable node. But that would
make the API too specific just for PageClickTracker's purpose ?

[please use gerrit instead, 2015-06-18 16:13:16]

On 2015/06/18 16:12:20, Yufeng Shen wrote:
> + Chris for OWNER of public/web/*
> 
> 
> > Does PageClickTracker actually care about all elements? Can we filter this
to
> a
> > specific kind of element instead?
> > 
> 
> it looks like all PageClickTracer does with the clicked node is setting
> 
>  focused_node_was_last_clicked_ = !clicked_node.isNull() &&
>                                    clicked_node.focused();
> 
> So we might be able to filter this on only focusable node. But that would
> make the API too specific just for PageClickTracker's purpose ?

If only PageClickTracker is using the API, then that should be OK.

[chrishtr, 2015-06-18 16:42:23]

lgtm

[Yufeng Shen (Slow to review), 2015-06-18 16:57:32]

The CQ bit was checked by miletus@chromium.org

[commit-bot: I haz the power, 2015-06-18 16:57:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1176423008/20001

[Rick Byers, 2015-06-18 17:01:27]

The CQ bit was unchecked by rbyers@chromium.org

[Rick Byers, 2015-06-18 17:02:09]

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

[Rick Byers, 2015-06-18 17:02:10]

Sorry for the delay, I have a couple comments.  I'm leaving for my flight in 2
hours (~3:00 eastern) so don't block on me after that.

https://codereview.chromium.org/1176423008/diff/20001/Source/core/input/Event...
File Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1176423008/diff/20001/Source/core/input/Event...
Source/core/input/EventHandler.cpp:2085: if (currentHitTest.innerNode()) {
If we're going to invoke this after mousedown for mouse input, I think we should
do the same for tap for consistency.  Eg. a page that changes the dom on mouseup
events should behave the same wrt. this callback whether or not the input is
touch of mouse.

https://codereview.chromium.org/1176423008/diff/20001/public/web/WebWidgetCli...
File public/web/WebWidgetClient.h (right):

https://codereview.chromium.org/1176423008/diff/20001/public/web/WebWidgetCli...
public/web/WebWidgetClient.h:170: virtual void onClickInput(const WebNode&
clickedNode) { }
You've implemented this to be at mousedown time (and the node is the one that
received the mosuedown event) which isn't the same as "click" (or
"clickedNode").  Is that the behavior to want to match PageClickTracker?  If so,
please rename this to something like onMouseDown with a comment like: "Called
immediately after a mousedown event is dispatched due to a mouse press or
gesture tap event".

[Yufeng Shen (Slow to review), 2015-06-18 18:29:04]

https://codereview.chromium.org/1176423008/diff/20001/Source/core/input/Event...
File Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/1176423008/diff/20001/Source/core/input/Event...
Source/core/input/EventHandler.cpp:2085: if (currentHitTest.innerNode()) {
On 2015/06/18 17:02:10, Rick Byers (Slow until 6-22) wrote:
> If we're going to invoke this after mousedown for mouse input, I think we
should
> do the same for tap for consistency.  Eg. a page that changes the dom on
mouseup
> events should behave the same wrt. this callback whether or not the input is
> touch of mouse.

done.

Moved the onMouseDown() up to right after handleMousePressEvent() is called.

We fire onMouseDown() regardless of the disposition of the dispatching, which
is to match the current expectation of PageClickTracker. But if in the future
we decide that we only fire onMouseDown if the mouse down is not prevent
default,
we can then merge the onMouseDown for tap/mouse into one place in
handleMousePressEvent().

https://codereview.chromium.org/1176423008/diff/20001/public/web/WebWidgetCli...
File public/web/WebWidgetClient.h (right):

https://codereview.chromium.org/1176423008/diff/20001/public/web/WebWidgetCli...
public/web/WebWidgetClient.h:170: virtual void onClickInput(const WebNode&
clickedNode) { }
On 2015/06/18 17:02:10, Rick Byers (Slow until 6-22) wrote:
> You've implemented this to be at mousedown time (and the node is the one that
> received the mosuedown event) which isn't the same as "click" (or
> "clickedNode").  Is that the behavior to want to match PageClickTracker?  If
so,
> please rename this to something like onMouseDown with a comment like: "Called
> immediately after a mousedown event is dispatched due to a mouse press or
> gesture tap event".

Done.

[Yufeng Shen (Slow to review), 2015-06-18 18:58:32]

The CQ bit was checked by miletus@chromium.org

[Yufeng Shen (Slow to review), 2015-06-18 18:58:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, esprehn@chromium.org, rouslan@chromium.org
Link to the patchset: https://codereview.chromium.org/1176423008/#ps40001
(title: "call onMouseDown for GestureTap righter after MousePress is
dispatched")

[commit-bot: I haz the power, 2015-06-18 18:59:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1176423008/40001

[Rick Byers, 2015-06-18 21:24:28]

Thanks, LGTM

[commit-bot: I haz the power, 2015-06-18 22:06:10]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197400