Tweaks layoutTests to use eventSender.setTouchPointRadius correctly

Tweaks layoutTests to use eventSender.setTouchPointRadius correctly

A link to the Chrome patch (event_sender.cc) https://codereview.chromium.org/990183003/

BUG=

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

[d.pikalov, 2015-03-09 13:35:13]

d.pikalov@partner.samsung.com changed reviewers:
+ rbyers@chromium.org

[Rick Byers, 2015-03-12 14:24:40]

https://codereview.chromium.org/990193002/diff/1/LayoutTests/fast/events/touc...
File LayoutTests/fast/events/touch/script-tests/basic-single-touch-events.js
(right):

https://codereview.chromium.org/990193002/diff/1/LayoutTests/fast/events/touc...
LayoutTests/fast/events/touch/script-tests/basic-single-touch-events.js:95:
eventSender.addTouchPoint(10, 10, 10, 10);
If we've got setTouchPointRadius, then let's just use that instead of allowing
radius to be supplied via addTouchPoint.  You just need to call
setTouchPointRadius after calling addTouchPoint but before calling touchStart.

All I'm aiming for here is a single pattern for setting the extended properties
like radius and tilt...

[Rick Byers, 2015-03-12 14:28:05]

On 2015/03/12 14:24:40, Rick Byers wrote:
>
https://codereview.chromium.org/990193002/diff/1/LayoutTests/fast/events/touc...
> File LayoutTests/fast/events/touch/script-tests/basic-single-touch-events.js
> (right):
> 
>
https://codereview.chromium.org/990193002/diff/1/LayoutTests/fast/events/touc...
> LayoutTests/fast/events/touch/script-tests/basic-single-touch-events.js:95:
> eventSender.addTouchPoint(10, 10, 10, 10);
> If we've got setTouchPointRadius, then let's just use that instead of allowing
> radius to be supplied via addTouchPoint.  You just need to call
> setTouchPointRadius after calling addTouchPoint but before calling touchStart.
> 
> All I'm aiming for here is a single pattern for setting the extended
properties
> like radius and tilt...

And if we're going to take that pattern, it looks like there's exactly two other
places where calls to addTouchPoint need to be updated:
fast/events/touch/touch-fractional-coordinates.html
fast/events/touch/touch-browser-zoom-scales-radius.html

[d.pikalov, 2015-03-16 16:48:44]

https://codereview.chromium.org/990193002/diff/1/LayoutTests/fast/events/touc...
File LayoutTests/fast/events/touch/script-tests/basic-single-touch-events.js
(right):

https://codereview.chromium.org/990193002/diff/1/LayoutTests/fast/events/touc...
LayoutTests/fast/events/touch/script-tests/basic-single-touch-events.js:95:
eventSender.addTouchPoint(10, 10, 10, 10);
On 2015/03/12 14:24:40, Rick Byers wrote:
> If we've got setTouchPointRadius, then let's just use that instead of allowing
> radius to be supplied via addTouchPoint.  You just need to call
> setTouchPointRadius after calling addTouchPoint but before calling touchStart.
> 
> All I'm aiming for here is a single pattern for setting the extended
properties
> like radius and tilt...


There is problem - setTouchPointRadius() sets event's state to StateMoved,
calling this method before eventSender.touchStart() causes StateMoved before
StatePressed - as a result I get 'unexpected timeout'. I going to add one more
parameter (bool set_state_moved) to setTouchPointRadius()

Btw, I guess there are no reasons to keep eventSender.setTouchPointRadius under
"if", right ?

[Rick Byers, 2015-03-16 18:43:17]

LGTM, thanks!

https://codereview.chromium.org/990193002/diff/1/LayoutTests/fast/events/touc...
File LayoutTests/fast/events/touch/script-tests/basic-single-touch-events.js
(right):

https://codereview.chromium.org/990193002/diff/1/LayoutTests/fast/events/touc...
LayoutTests/fast/events/touch/script-tests/basic-single-touch-events.js:95:
eventSender.addTouchPoint(10, 10, 10, 10);
On 2015/03/16 16:48:44, d.pikalov wrote:
> On 2015/03/12 14:24:40, Rick Byers wrote:
> > If we've got setTouchPointRadius, then let's just use that instead of
allowing
> > radius to be supplied via addTouchPoint.  You just need to call
> > setTouchPointRadius after calling addTouchPoint but before calling
touchStart.
> > 
> > All I'm aiming for here is a single pattern for setting the extended
> properties
> > like radius and tilt...
> 
> 
> There is problem - setTouchPointRadius() sets event's state to StateMoved,
> calling this method before eventSender.touchStart() causes StateMoved before
> StatePressed - as a result I get 'unexpected timeout'. I going to add one more
> parameter (bool set_state_moved) to setTouchPointRadius()

Ah, sorry I didn't appreciate that this was more complex.  I was thinking it was
trivial.  What you've done seems fine.

> Btw, I guess there are no reasons to keep eventSender.setTouchPointRadius
under
> "if", right ?

Right, I'd like to get rid of it (it adds uncertainty for the reader of the
test).  But there's a staging problem - you can't (at least for a few more
weeks) land chromium and blink changes atomically.  So you probably need to land
this CL first, then lend the change to event_sender, then submit a 3rd CL to
remove the 'if(eventSender.setTouchPointRadius)' checks.


> 
>

[Rick Byers, 2015-03-16 18:45:11]

Oh please also clarify subject / description to make it clear you're just
changing tests.  Eg. "Tweaks layoutTests to use eventSender.setTouchPointRadius
correctly".

And add a link in the CL description to your event_sender.cc change.

[d.pikalov, 2015-03-17 09:50:21]

Patchset #3 (id:40001) has been deleted

[Rick Byers, 2015-03-17 14:43:45]

On 2015/03/17 09:54:28, d.pikalov wrote:

Looks good. But are these tests going to fail now because the expected output
doesn't match (since you're no longer checking the radius)?  I kicked off a
couple try jobs to confirm.

[d.pikalov, 2015-03-18 11:38:41]

The CQ bit was checked by d.pikalov@partner.samsung.com

[d.pikalov, 2015-03-18 11:38:41]

The patchset sent to the CQ was uploaded after l-g-t-m from rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/990193002/#ps80001 (title:
"LayoutTests fixed (expected results)")

[d.pikalov, 2015-03-18 11:38:45]

The CQ bit was unchecked by d.pikalov@partner.samsung.com

[d.pikalov, 2015-03-18 11:39:22]

The CQ bit was checked by d.pikalov@partner.samsung.com

[commit-bot: I haz the power, 2015-03-18 11:39:36]

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

[d.pikalov, 2015-03-18 11:40:48]

The CQ bit was unchecked by d.pikalov@partner.samsung.com

[d.pikalov, 2015-03-18 11:43:32]

On 2015/03/17 14:43:45, Rick Byers wrote:
> On 2015/03/17 09:54:28, d.pikalov wrote:
> 
> Looks good. But are these tests going to fail now because the expected output
> doesn't match (since you're no longer checking the radius)?  I kicked off a
> couple try jobs to confirm.

I updated "*-expected.txt" files

[d.pikalov, 2015-03-18 11:47:13]

The CQ bit was checked by d.pikalov@partner.samsung.com

[commit-bot: I haz the power, 2015-03-18 11:47:20]

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

[commit-bot: I haz the power, 2015-03-18 13:02:35]

Committed patchset #4 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192088

[Fabrice (no longer in Chrome), 2015-03-19 11:09:49]

A revert of this CL (patchset #4 id:80001) has been created in
https://codereview.chromium.org/1023713002/ by fdegans@chromium.org.

The reason for reverting is: Layout tests failures, see crbug/468704.

[d.pikalov, 2015-03-24 14:54:03]

Patchset #5 (id:100001) has been deleted