Firing got/lost pointer capture events

Firing got/lost pointer capture events

This CL is taking care of firing gotpointercapture and
lostpointercapture events based on the status of the
pointer capture and whether the element is still in
the document or not.

BUG=592280
Committed: https://crrev.com/3add13bbfb3537a8a243711bb184d12faf3ef096
Cr-Commit-Position: refs/heads/master@{#381061}

[Navid Zolghadr, 2016-03-08 18:17:30]

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

[sof, 2016-03-08 18:31:06]

https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:183:
PassRefPtrWillBeRawPtr<PointerEvent> pointerEvent,
This is yet another instance where you _have to_ bind the PassRefPtr<> to a
local before using it multiple times in the method.

You may want to carefully look over the non-Oilpan failures that
PointerEventFactory CLs have caused over the past couple of weeks and try to
avoid repeating those. One way to be sure you're doing it right is to exercise
the tryserver.blink:link_blink_oilpan_rel trybot. Don't be tricked by the
"oilpan" in its name, it is configured to build with Oilpan off.

[Navid Zolghadr, 2016-03-08 18:34:14]

https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:183:
PassRefPtrWillBeRawPtr<PointerEvent> pointerEvent,
On 2016/03/08 18:31:06, sof wrote:
> This is yet another instance where you _have to_ bind the PassRefPtr<> to a
> local before using it multiple times in the method.
> 
> You may want to carefully look over the non-Oilpan failures that
> PointerEventFactory CLs have caused over the past couple of weeks and try to
> avoid repeating those. One way to be sure you're doing it right is to exercise
> the tryserver.blink:link_blink_oilpan_rel trybot. Don't be tricked by the
> "oilpan" in its name, it is configured to build with Oilpan off.

I was paying attention not to add any more incorrect code. But I wasn't actively
fixing the existing code. I'll take another look over PointerEvent classes and
try to fix all the existing instances in this CL.

[Navid Zolghadr, 2016-03-08 21:50:25]

https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:183:
PassRefPtrWillBeRawPtr<PointerEvent> pointerEvent,
On 2016/03/08 18:34:14, Navid Zolghadr wrote:
> On 2016/03/08 18:31:06, sof wrote:
> > This is yet another instance where you _have to_ bind the PassRefPtr<> to a
> > local before using it multiple times in the method.
> > 
> > You may want to carefully look over the non-Oilpan failures that
> > PointerEventFactory CLs have caused over the past couple of weeks and try to
> > avoid repeating those. One way to be sure you're doing it right is to
exercise
> > the tryserver.blink:link_blink_oilpan_rel trybot. Don't be tricked by the
> > "oilpan" in its name, it is configured to build with Oilpan off.
> 
> I was paying attention not to add any more incorrect code. But I wasn't
actively
> fixing the existing code. I'll take another look over PointerEvent classes and
> try to fix all the existing instances in this CL.

I tried running tryserver.blink:link_blink_oilpan_rel and it seems to pass. Were
you expecting to fail since I have this incorrect pointertype? Or should I look
in the stdio of that bot and look for some specific things? In that case what
that thing would be?

[sof, 2016-03-09 06:22:17]

On 2016/03/08 21:50:25, Navid Zolghadr wrote:
>
https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):
> 
>
https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/events/PointerEventFactory.cpp:183:
> PassRefPtrWillBeRawPtr<PointerEvent> pointerEvent,
> On 2016/03/08 18:34:14, Navid Zolghadr wrote:
> > On 2016/03/08 18:31:06, sof wrote:
> > > This is yet another instance where you _have to_ bind the PassRefPtr<> to
a
> > > local before using it multiple times in the method.
> > > 
> > > You may want to carefully look over the non-Oilpan failures that
> > > PointerEventFactory CLs have caused over the past couple of weeks and try
to
> > > avoid repeating those. One way to be sure you're doing it right is to
> exercise
> > > the tryserver.blink:link_blink_oilpan_rel trybot. Don't be tricked by the
> > > "oilpan" in its name, it is configured to build with Oilpan off.
> > 
> > I was paying attention not to add any more incorrect code. But I wasn't
> actively
> > fixing the existing code. I'll take another look over PointerEvent classes
and
> > try to fix all the existing instances in this CL.
> 
> I tried running tryserver.blink:link_blink_oilpan_rel and it seems to pass.
Were
> you expecting to fail since I have this incorrect pointertype? Or should I
look
> in the stdio of that bot and look for some specific things? In that case what
> that thing would be?

Thanks, that's good. To maintain its green non-Oilpan status, add the line

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_oilpan_rel

to your description.

[mustaq, 2016-03-10 19:28:20]

Looks good, thanks. 
- Need to check if HTMLAttributeNames.in leaks the info w/o the flag.
- Any clue why win_chromium_rel_ng test missing some events?

https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/mouse-pointer-capture.html
(right):

https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/mouse-pointer-capture.html:64:
if (implicitRelease && pointerMoveCount++ > 3) {
Didn't notice it before: the var should be called |explicitRelease|.

https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/PointerEventFactory.h (right):

https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/PointerEventFactory.h:46:
PassRefPtrWillBeRawPtr<PointerEvent> createPointerCapture(
Please rename to "createPointerCaptureEvent", otherwise sounds like this method
is triggering the capture logic.

https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/PointerEventFactory.h:51:
PassRefPtrWillBeRawPtr<PointerEvent> createPointerTransition(
Same as before: rename to ...Event.

https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp (right):

https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp:88: void
PointerEventFactoryTest::cloneAndCheckPointerEvent(
s/cloneAndCheckPointerEvent/cloneAndCheckPointerTransitionEvent/

https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLAttributeNames.in (right):

https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLAttributeNames.in:200:
ongotpointercapture
Where do we need this? This is not behind a flag as in Element.idl, so its
availability could be taken as "supports PointerEvents".

https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):

https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:86: //
TODO(crbug.com/587955): The following if is a hack to work around this bug
nit: remove "if"

https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:90: }
This deserves a comment: why not resetting the bit on pointerout (as you did
above).

[Navid Zolghadr, 2016-03-10 20:27:35]

ptal.

Regarding the HTMLAttributeNames.in, as far as I found it is only being using to
create the other files for the names. Rick, do you know if this any is by anyway
exposed directly?

Regarding the Windows test, I didn't find anything precisely but my guess was on
getting the events as drag instead of normal events but I'm not sure why that
behavior is different in Linux. I fixed it by adding a leap forward and an extra
mouse click to deselect any possible selections.

https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/mouse-pointer-capture.html
(right):

https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/mouse-pointer-capture.html:64:
if (implicitRelease && pointerMoveCount++ > 3) {
On 2016/03/10 19:28:19, mustaq wrote:
> Didn't notice it before: the var should be called |explicitRelease|.

Done.

https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/PointerEventFactory.h (right):

https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/PointerEventFactory.h:46:
PassRefPtrWillBeRawPtr<PointerEvent> createPointerCapture(
On 2016/03/10 19:28:19, mustaq wrote:
> Please rename to "createPointerCaptureEvent", otherwise sounds like this
method
> is triggering the capture logic.

Done.

https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/PointerEventFactory.h:51:
PassRefPtrWillBeRawPtr<PointerEvent> createPointerTransition(
On 2016/03/10 19:28:19, mustaq wrote:
> Same as before: rename to ...Event.

Done.

https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp (right):

https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/events/PointerEventFactoryTest.cpp:88: void
PointerEventFactoryTest::cloneAndCheckPointerEvent(
On 2016/03/10 19:28:19, mustaq wrote:
> s/cloneAndCheckPointerEvent/cloneAndCheckPointerTransitionEvent/

I replaced it with createAndCheckPointerTransitionEvent which seems more
fitting.

https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLAttributeNames.in (right):

https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLAttributeNames.in:200:
ongotpointercapture
On 2016/03/10 19:28:19, mustaq wrote:
> Where do we need this? This is not behind a flag as in Element.idl, so its
> availability could be taken as "supports PointerEvents".

I believe there is this EventTypeNames file generated from this and then we use
that file in for example Element.cpp. So I don't think these names are directly
expose to JS. Am I right?

https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):

https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:86: //
TODO(crbug.com/587955): The following if is a hack to work around this bug
On 2016/03/10 19:28:19, mustaq wrote:
> nit: remove "if"

Sure.

https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:90: }
On 2016/03/10 19:28:19, mustaq wrote:
> This deserves a comment: why not resetting the bit on pointerout (as you did
> above).

Since it was a hack anyways and should be removed soon, I didn't bother fully
add it. Just that current tests didn't hit that case. Because usually when you
send an out you will follow it up with an over on another element and that will
override this. However, I can imagine scenarios that it is not the case. I added
the other scenario as well.

[mustaq, 2016-03-10 21:06:57]

https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):

https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:90: }
On 2016/03/10 20:27:35, Navid Zolghadr wrote:
> On 2016/03/10 19:28:19, mustaq wrote:
> > This deserves a comment: why not resetting the bit on pointerout (as you did
> > above).
> 
> Since it was a hack anyways and should be removed soon, I didn't bother fully
> add it. Just that current tests didn't hit that case. Because usually when you
> send an out you will follow it up with an over on another element and that
will
> override this. However, I can imagine scenarios that it is not the case. I
added
> the other scenario as well.

This is significant enough to call for a test. I understand that a solution for
the separate-entry-state bug could affect the test expectations, but we would
see a before-after diff w/o much effort.

Also need to add a test for implicit release of capture when mouse button was
release off the page. Use eventSender.setMouseButtonState(), e.g. in
mouse-pointer-chorded-buttons.html.

[Navid Zolghadr, 2016-03-11 15:40:03]

On 2016/03/10 21:06:57, mustaq wrote:
>
https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):
> 
>
https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/input/PointerEventManager.cpp:90: }
> On 2016/03/10 20:27:35, Navid Zolghadr wrote:
> > On 2016/03/10 19:28:19, mustaq wrote:
> > > This deserves a comment: why not resetting the bit on pointerout (as you
did
> > > above).
> > 
> > Since it was a hack anyways and should be removed soon, I didn't bother
fully
> > add it. Just that current tests didn't hit that case. Because usually when
you
> > send an out you will follow it up with an over on another element and that
> will
> > override this. However, I can imagine scenarios that it is not the case. I
> added
> > the other scenario as well.
> 
> This is significant enough to call for a test. I understand that a solution
for
> the separate-entry-state bug could affect the test expectations, but we would
> see a before-after diff w/o much effort.
> 
> Also need to add a test for implicit release of capture when mouse button was
> release off the page. Use eventSender.setMouseButtonState(), e.g. in
> mouse-pointer-chorded-buttons.html.

ptal.

As per our discussion offline, I only added the test for releasing the mouse
outside
of the page and will add the test for the other scenario later with the legacy
mouse
bug we have.

[mustaq, 2016-03-14 15:05:12]

On 2016/03/11 15:40:03, Navid Zolghadr wrote:
> On 2016/03/10 21:06:57, mustaq wrote:
> >
>
https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1774913004/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/input/PointerEventManager.cpp:90: }
> > On 2016/03/10 20:27:35, Navid Zolghadr wrote:
> > > On 2016/03/10 19:28:19, mustaq wrote:
> > > > This deserves a comment: why not resetting the bit on pointerout (as you
> did
> > > > above).
> > > 
> > > Since it was a hack anyways and should be removed soon, I didn't bother
> fully
> > > add it. Just that current tests didn't hit that case. Because usually when
> you
> > > send an out you will follow it up with an over on another element and that
> > will
> > > override this. However, I can imagine scenarios that it is not the case. I
> > added
> > > the other scenario as well.
> > 
> > This is significant enough to call for a test. I understand that a solution
> for
> > the separate-entry-state bug could affect the test expectations, but we
would
> > see a before-after diff w/o much effort.
> > 
> > Also need to add a test for implicit release of capture when mouse button
was
> > release off the page. Use eventSender.setMouseButtonState(), e.g. in
> > mouse-pointer-chorded-buttons.html.
> 
> ptal.
> 
> As per our discussion offline, I only added the test for releasing the mouse
> outside
> of the page and will add the test for the other scenario later with the legacy
> mouse
> bug we have.

Thanks Navid, LGTM.

After a cursory look again, I didn't find any possible leaks through
HTMLAttributeNames.in as I feared. Looks like there are non-standard things in
the file that don't appear in the global-interface-listing-expected.txt pointed
out by Rick (e.g. onautocompleteerror) because the idl definition is behind a
flag.

[Rick Byers, 2016-03-14 19:19:15]

RS LGTM with nit

https://codereview.chromium.org/1774913004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):

https://codereview.chromium.org/1774913004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:73: const
AtomicString& pointerType = pointerEvent->type();
nit: maybe call this 'eventType'?  'pointerType' suggests mouse/touch etc.

[Navid Zolghadr, 2016-03-14 19:31:20]

https://codereview.chromium.org/1774913004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):

https://codereview.chromium.org/1774913004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:73: const
AtomicString& pointerType = pointerEvent->type();
On 2016/03/14 19:19:15, Rick Byers wrote:
> nit: maybe call this 'eventType'?  'pointerType' suggests mouse/touch etc.

Done.

[Navid Zolghadr, 2016-03-14 19:31:35]

The CQ bit was checked by nzolghadr@chromium.org

[Navid Zolghadr, 2016-03-14 19:31:35]

The patchset sent to the CQ was uploaded after l-g-t-m from mustaq@chromium.org,
rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/1774913004/#ps60001
(title: "Variable rename")

[commit-bot: I haz the power, 2016-03-14 19:32:02]

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

[commit-bot: I haz the power, 2016-03-14 20:37:42]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-14 20:40:01]

Description was changed from

==========
Firing got/lost pointer capture events

This CL is taking care of firing gotpointercapture and
lostpointercapture events based on the status of the
pointer capture and whether the element is still in
the document or not.

BUG=592280
==========

to

==========
Firing got/lost pointer capture events

This CL is taking care of firing gotpointercapture and
lostpointercapture events based on the status of the
pointer capture and whether the element is still in
the document or not.

BUG=592280

Committed: https://crrev.com/3add13bbfb3537a8a243711bb184d12faf3ef096
Cr-Commit-Position: refs/heads/master@{#381061}
==========

[commit-bot: I haz the power, 2016-03-14 20:40:02]

Patchset 4 (id:??) landed as
https://crrev.com/3add13bbfb3537a8a243711bb184d12faf3ef096
Cr-Commit-Position: refs/heads/master@{#381061}