Conditional hittesting & sending boundary events for captured pointer events

Conditional hittesting and sending boundary events for captured pointer events

This CL adds the support sending boundary events for captured
pointer events if needed when pointerEventV1SpecCapturingEnabled
flag is enabled. Although it might not have captured all edge
cases but it does satisfy the basic needs and normal
use cases which suffices the testing need.
This CL does not change any behavior when the flag is off and
it is only for testing purposes when the flag is on.
Removal of this flag is tracked in crbug.com/642776.

BUG=640700
Committed: https://crrev.com/ebbc77fb19301347fe7d062691ad20d82d5eb865
Cr-Commit-Position: refs/heads/master@{#418039}

[Navid Zolghadr, 2016-09-09 15:54:33]

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

[commit-bot: I haz the power, 2016-09-09 15:54:55]

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

[Navid Zolghadr, 2016-09-09 15:55:48]

nzolghadr@chromium.org changed reviewers:
+ dtapuska@chromium.org, mustaq@chromium.org

[dtapuska, 2016-09-09 16:01:01]

On 2016/09/09 15:55:49, Navid Zolghadr wrote:

do we anticipate to write any tests for this? Or are we not investing in that
right now?

[dtapuska, 2016-09-09 16:01:08]

https://codereview.chromium.org/2326653003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/EventHandler.cpp (left):

https://codereview.chromium.org/2326653003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/EventHandler.cpp:1387: if (EventTarget*
mousePointerCapturingNode = m_pointerEventManager->getMouseCapturingNode()) {
I think it is cleaner to store the mousePointerCapturingNode in a variable
instead of calling it multiple times for ease of reading.

[Navid Zolghadr, 2016-09-09 16:03:38]

Regarding the tests Mustaq and I talked before and agreed not to add the tests.
I believe this flag will be removed two weeks from now. So maybe not worth
adding tests.

https://codereview.chromium.org/2326653003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/EventHandler.cpp (left):

https://codereview.chromium.org/2326653003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/EventHandler.cpp:1387: if (EventTarget*
mousePointerCapturingNode = m_pointerEventManager->getMouseCapturingNode()) {
On 2016/09/09 16:01:07, dtapuska wrote:
> I think it is cleaner to store the mousePointerCapturingNode in a variable
> instead of calling it multiple times for ease of reading.

Then I have to store it outside the scope of the if which makes it defined for
the whole function scope. Is there a way to do it better?

[dtapuska, 2016-09-09 16:07:17]

On 2016/09/09 16:03:38, Navid Zolghadr wrote:
> Regarding the tests Mustaq and I talked before and agreed not to add the
tests.
> I believe this flag will be removed two weeks from now. So maybe not worth
> adding tests.
> 
>
https://codereview.chromium.org/2326653003/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/input/EventHandler.cpp (left):
> 
>
https://codereview.chromium.org/2326653003/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/input/EventHandler.cpp:1387: if (EventTarget*
> mousePointerCapturingNode = m_pointerEventManager->getMouseCapturingNode()) {
> On 2016/09/09 16:01:07, dtapuska wrote:
> > I think it is cleaner to store the mousePointerCapturingNode in a variable
> > instead of calling it multiple times for ease of reading.
> 
> Then I have to store it outside the scope of the if which makes it defined for
> the whole function scope. Is there a way to do it better?

if (EventTarget* mousePointerCapturingNode =
(RuntimeEnabledFeatures::pointerEventV1SpecCapturingEnabled() ? 0 :
m_pointerEventManager->getMouseCapturingNode()))

also works but that is hard to read

[Navid Zolghadr, 2016-09-09 16:12:30]

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

[commit-bot: I haz the power, 2016-09-09 16:12:49]

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

[Navid Zolghadr, 2016-09-09 16:13:57]

ptal.

I defined a function scope variable to increase readability. This change is
temporary anyway and it'll be removed soon. So I don't think having the variable
in the function scope hurts for a long time :)

[dtapuska, 2016-09-09 17:02:36]

On 2016/09/09 16:13:57, Navid Zolghadr wrote:
> ptal.
> 
> I defined a function scope variable to increase readability. This change is
> temporary anyway and it'll be removed soon. So I don't think having the
variable
> in the function scope hurts for a long time :)

lgtm

[mustaq, 2016-09-09 17:36:06]

On 2016/09/09 17:02:36, dtapuska wrote:
> On 2016/09/09 16:13:57, Navid Zolghadr wrote:
> > ptal.
> > 
> > I defined a function scope variable to increase readability. This change is
> > temporary anyway and it'll be removed soon. So I don't think having the
> variable
> > in the function scope hurts for a long time :)
> 
> lgtm

LGTM.

Please modify the CL title to say "conditional hittesting...", and mention later
that this is effective thru pointerEventV1SpecCapturingEnabled flag.

[Navid Zolghadr, 2016-09-09 17:39:50]

Description was changed from

==========
Do hit-testing and send boundary events for captured pointer events

This CL supports sending boundary events for captured pointer events
if needed. Although it might not have captured all edge cases but
it does satisfy the basic needs and normal use cases which suffices
the testing need.

BUG=640700
==========

to

==========
Conditional hittesting and sending boundary events for captured pointer events

This CL adds the support sending boundary events for captured
pointer events if needed when pointerEventV1SpecCapturingEnabled flag is
enabled. Although it might not have captured all edge cases but
it does satisfy the basic needs and normal use cases which suffices
the testing need.

BUG=640700
==========

[Navid Zolghadr, 2016-09-09 17:40:31]

Description was changed from

==========
Conditional hittesting and sending boundary events for captured pointer events

This CL adds the support sending boundary events for captured
pointer events if needed when pointerEventV1SpecCapturingEnabled flag is
enabled. Although it might not have captured all edge cases but
it does satisfy the basic needs and normal use cases which suffices
the testing need.

BUG=640700
==========

to

==========
Conditional hittesting and sending boundary events for captured pointer events

This CL adds the support sending boundary events for captured
pointer events if needed when pointerEventV1SpecCapturingEnabled
flag is enabled. Although it might not have captured all edge cases
but it does satisfy the basic needs and normal use cases which 
suffices the testing need.

BUG=640700
==========

[Navid Zolghadr, 2016-09-09 17:40:48]

done.

[commit-bot: I haz the power, 2016-09-09 17:50:51]

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

[commit-bot: I haz the power, 2016-09-09 17:50:52]

Dry run: This issue passed the CQ dry run.

[Navid Zolghadr, 2016-09-09 17:51:25]

The CQ bit was checked by nzolghadr@chromium.org

[commit-bot: I haz the power, 2016-09-09 17:51:59]

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

[dtapuska, 2016-09-09 17:53:37]

On 2016/09/09 17:51:59, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

you still need an owner review.

[Navid Zolghadr, 2016-09-09 17:58:11]

On 2016/09/09 17:53:37, dtapuska wrote:
> On 2016/09/09 17:51:59, commit-bot: I haz the power wrote:
> > CQ is trying da patch. Follow status at
> >  
> >
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> 
> you still need an owner review.

oops! I missed it!

[commit-bot: I haz the power, 2016-09-09 17:58:44]

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

[commit-bot: I haz the power, 2016-09-09 17:58:46]

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

[Navid Zolghadr, 2016-09-09 17:59:04]

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

[Navid Zolghadr, 2016-09-09 17:59:05]

rbyers@chromium.org: Please review changes in 

thirdparty/WebKit/Source/core/*

[Rick Byers, 2016-09-12 16:57:07]

LGTM
May be worth noting in the CL description that there's no change in behavior
when not using the flag, and the flag is temporary for testing/evaluation
purposes (mention the bug that tracks removing the flag).

[Navid Zolghadr, 2016-09-12 16:59:34]

Description was changed from

==========
Conditional hittesting and sending boundary events for captured pointer events

This CL adds the support sending boundary events for captured
pointer events if needed when pointerEventV1SpecCapturingEnabled
flag is enabled. Although it might not have captured all edge cases
but it does satisfy the basic needs and normal use cases which 
suffices the testing need.

BUG=640700
==========

to

==========
Conditional hittesting and sending boundary events for captured pointer events

This CL adds the support sending boundary events for captured
pointer events if needed when pointerEventV1SpecCapturingEnabled
flag is enabled. Although it might not have captured all edge
cases but it does satisfy the basic needs and normal
use cases which suffices the testing need.
This CL does not change any behavior when the flag is off and
it is only for testing purposes when the flag is on.
Removing of this flag is tracked in crbug.com/642776.

BUG=640700
==========

[Navid Zolghadr, 2016-09-12 16:59:45]

Description was changed from

==========
Conditional hittesting and sending boundary events for captured pointer events

This CL adds the support sending boundary events for captured
pointer events if needed when pointerEventV1SpecCapturingEnabled
flag is enabled. Although it might not have captured all edge
cases but it does satisfy the basic needs and normal
use cases which suffices the testing need.
This CL does not change any behavior when the flag is off and
it is only for testing purposes when the flag is on.
Removing of this flag is tracked in crbug.com/642776.

BUG=640700
==========

to

==========
Conditional hittesting and sending boundary events for captured pointer events

This CL adds the support sending boundary events for captured
pointer events if needed when pointerEventV1SpecCapturingEnabled
flag is enabled. Although it might not have captured all edge
cases but it does satisfy the basic needs and normal
use cases which suffices the testing need.
This CL does not change any behavior when the flag is off and
it is only for testing purposes when the flag is on.
Removal of this flag is tracked in crbug.com/642776.

BUG=640700
==========

[Navid Zolghadr, 2016-09-12 17:00:14]

The CQ bit was checked by nzolghadr@chromium.org

[commit-bot: I haz the power, 2016-09-12 17:00:49]

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

[commit-bot: I haz the power, 2016-09-12 20:58:20]

Description was changed from

==========
Conditional hittesting and sending boundary events for captured pointer events

This CL adds the support sending boundary events for captured
pointer events if needed when pointerEventV1SpecCapturingEnabled
flag is enabled. Although it might not have captured all edge
cases but it does satisfy the basic needs and normal
use cases which suffices the testing need.
This CL does not change any behavior when the flag is off and
it is only for testing purposes when the flag is on.
Removal of this flag is tracked in crbug.com/642776.

BUG=640700
==========

to

==========
Conditional hittesting and sending boundary events for captured pointer events

This CL adds the support sending boundary events for captured
pointer events if needed when pointerEventV1SpecCapturingEnabled
flag is enabled. Although it might not have captured all edge
cases but it does satisfy the basic needs and normal
use cases which suffices the testing need.
This CL does not change any behavior when the flag is off and
it is only for testing purposes when the flag is on.
Removal of this flag is tracked in crbug.com/642776.

BUG=640700
==========

[commit-bot: I haz the power, 2016-09-12 20:58:23]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-12 20:59:34]

Description was changed from

==========
Conditional hittesting and sending boundary events for captured pointer events

This CL adds the support sending boundary events for captured
pointer events if needed when pointerEventV1SpecCapturingEnabled
flag is enabled. Although it might not have captured all edge
cases but it does satisfy the basic needs and normal
use cases which suffices the testing need.
This CL does not change any behavior when the flag is off and
it is only for testing purposes when the flag is on.
Removal of this flag is tracked in crbug.com/642776.

BUG=640700
==========

to

==========
Conditional hittesting and sending boundary events for captured pointer events

This CL adds the support sending boundary events for captured
pointer events if needed when pointerEventV1SpecCapturingEnabled
flag is enabled. Although it might not have captured all edge
cases but it does satisfy the basic needs and normal
use cases which suffices the testing need.
This CL does not change any behavior when the flag is off and
it is only for testing purposes when the flag is on.
Removal of this flag is tracked in crbug.com/642776.

BUG=640700

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

[commit-bot: I haz the power, 2016-09-12 20:59:35]

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