Add the rest of web platform pointerevent tests

Add the rest of web platform pointerevent tests

Add input injection for the rest of web platform
pointerevent tests.

BUG=612924
Committed: https://crrev.com/23ce92bfe9a1160e552a91d443e788f303f1eb6d
Cr-Commit-Position: refs/heads/master@{#403812}

[Navid Zolghadr, 2016-06-10 18:51:08]

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

[Navid Zolghadr, 2016-06-10 18:51:08]

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

[Navid Zolghadr, 2016-06-10 18:51:09]

This is about half of all PE tests. I wanted to know if you guys are fine with
this method of refactoring the code among them and the way right now everything
is structured.

I guess I should have asked you guys earlier before doing half of it but now is
better than when I'm done with all as it is quite a lot of work to change
something in all the tests. There are a few tests I need to add to skip as for
example touch tests don't work on Mac that I will take care in the next patches.

[Navid Zolghadr, 2016-06-10 18:51:09]

This is about half of all PE tests. I wanted to know if you guys are fine with
this method of refactoring the code among them and the way right now everything
is structured.

I guess I should have asked you guys earlier before doing half of it but now is
better than when I'm done with all as it is quite a lot of work to change
something in all the tests. There are a few tests I need to add to skip as for
example touch tests don't work on Mac that I will take care in the next patches.

[dtapuska, 2016-06-10 19:27:51]

On 2016/06/10 18:51:09, Navid Zolghadr wrote:
> This is about half of all PE tests. I wanted to know if you guys are fine with
> this method of refactoring the code among them and the way right now
everything
> is structured.
> 
> I guess I should have asked you guys earlier before doing half of it but now
is
> better than when I'm done with all as it is quite a lot of work to change
> something in all the tests. There are a few tests I need to add to skip as for
> example touch tests don't work on Mac that I will take care in the next
patches.

seems reasonable to me; glad that worked.

[Rick Byers, 2016-06-10 20:17:37]

Yep the approach seems reasonable to me.  There are some minor things we may
want to iterate on (eg. shouldn't  "wpt_automation" be somewhere other than the
"imported" directory since it's not, in fact, imported from anywhere?)

https://codereview.chromium.org/2056333002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_common_input.js
(right):

https://codereview.chromium.org/2056333002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_common_input.js:9:
eventSender.mouseDown(0);
As you know, we ultimately want to avoid using eventSender because it doesn't
test end-to-end.  But I love that you've abstracted this here - we can change it
once the gpuBenchmarking extension is better.

https://codereview.chromium.org/2056333002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_pointertype_touch-manual-input.js
(right):

https://codereview.chromium.org/2056333002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_pointertype_touch-manual-input.js:1:
// Import the common methods
nit: you could avoid some repetition here, eg. a "importAutomatedInputScript"
function in testhardnessreport.js that does all the work (instead of exporting a
variable and then repeating most of the code that uses that variable).

[Navid Zolghadr, 2016-06-15 14:38:57]

I believe I added all the tests. ptal.

Regarding the folder address, I added Dirk to the original review and he didn't
make any objection to this. However, the tests are written in a way that the
whole folder can easily be moved. Which path do you suggest?

https://codereview.chromium.org/2056333002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_common_input.js
(right):

https://codereview.chromium.org/2056333002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_common_input.js:9:
eventSender.mouseDown(0);
On 2016/06/10 20:17:37, Rick Byers wrote:
> As you know, we ultimately want to avoid using eventSender because it doesn't
> test end-to-end.  But I love that you've abstracted this here - we can change
it
> once the gpuBenchmarking extension is better.

Yes. That is the plan. Although even current touch event generators seem to be
problematic in some of the tests as sometimes they cause crash and sometimes
they cause memory leaks. I filed a separate bug for that.

https://codereview.chromium.org/2056333002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_pointertype_touch-manual-input.js
(right):

https://codereview.chromium.org/2056333002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_pointertype_touch-manual-input.js:1:
// Import the common methods
On 2016/06/10 20:17:37, Rick Byers wrote:
> nit: you could avoid some repetition here, eg. a "importAutomatedInputScript"
> function in testhardnessreport.js that does all the work (instead of exporting
a
> variable and then repeating most of the code that uses that variable).

That would be a lot better. Done.

[Navid Zolghadr, 2016-06-17 17:39:49]

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

[Rick Byers, 2016-06-21 15:43:46]

Looks like a good next step to me - thanks!

On 2016/06/15 14:38:57, Navid Zolghadr(OOO til Jun 29) wrote:
> I believe I added all the tests. ptal.
> 
> Regarding the folder address, I added Dirk to the original review and he
didn't
> make any objection to this. However, the tests are written in a way that the
> whole folder can easily be moved. Which path do you suggest?

Yeah I guess it doesn't really matter - it's nice that this keeps them near the
WPT files even though they're not really "imported".  Maybe just updated the
imported/README file to mention this directory as special (an extension to the
imported WPT tests, not an imported test suite itself).

>
https://codereview.chromium.org/2056333002/diff/60001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_common_input.js
> (right):
> 
>
https://codereview.chromium.org/2056333002/diff/60001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_common_input.js:9:
> eventSender.mouseDown(0);
> On 2016/06/10 20:17:37, Rick Byers wrote:
> > As you know, we ultimately want to avoid using eventSender because it
doesn't
> > test end-to-end.  But I love that you've abstracted this here - we can
change
> it
> > once the gpuBenchmarking extension is better.
> 
> Yes. That is the plan. Although even current touch event generators seem to be
> problematic in some of the tests as sometimes they cause crash and sometimes
> they cause memory leaks. I filed a separate bug for that.

Ugh, ok we'll have to dig into that.

One possible problem I just thought of with how you've written these APIs is
that tests may depend on them being synchronous.  Eg. do a mouse move and expect
those handlers to run before doing whatever the next step is.  Once we make all
of this do full end-to-end input testing it'll have to be async.  That'll
probably mean that we'll want these automation hooks to return Promises that
resolve once the action has finished running.  I think the GPU benchmarking APIs
support this style already via callback arguments.

I probably should have mentioned this sooner (sorry).  So feel free to just add
a TODO saying eventually we'll need to convert all these helper function to be
async.
 
>
https://codereview.chromium.org/2056333002/diff/60001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_pointertype_touch-manual-input.js
> (right):
> 
>
https://codereview.chromium.org/2056333002/diff/60001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_pointertype_touch-manual-input.js:1:
> // Import the common methods
> On 2016/06/10 20:17:37, Rick Byers wrote:
> > nit: you could avoid some repetition here, eg. a
"importAutomatedInputScript"
> > function in testhardnessreport.js that does all the work (instead of
exporting
> a
> > variable and then repeating most of the code that uses that variable).
> 
> That would be a lot better. Done.

https://codereview.chromium.org/2056333002/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2056333002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:1143: crbug.com/471824
imported/wpt/pointerevents/pointerevent_pointerup_pointertype-manual.html  [
Failure Timeout ]
this is a lot of new Timeout expectations to be adding, it may slow down total
test running significantly (if each of these tests actually takes the 5s or
whatever to timeout).  Is it just that these tests time out when pointer events
isn't supported?  Perhaps they should just be Skip instead until they no longer
time out?

We should talk about the testing strategy once PointerEvents is
status=experimental.  Do we still want the virtual test suite?  If not you could
just go to status=test now and eliminate the virtual test suite first to make
this simpler.

https://codereview.chromium.org/2056333002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_common_input.js
(right):

https://codereview.chromium.org/2056333002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_common_input.js:1:
// Mouse actions
nit: please add a comment at the top summarizing what this file is for.

https://codereview.chromium.org/2056333002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_common_input.js:39:
mouseMoveIntoTarget(targetId);
Doesn't this just send a mousemove to the exact same position as above?  For
"mouseDrag" I assumed you'd want to move the mouse a bit (either within the
target or out of the target or something)?

https://codereview.chromium.org/2056333002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_common_input.js:64:
function scrollUpInTarget(targetId) {
nit: I guess these should be named like "touchScrollUp..." to avoid confusion
with mouse scroll?

[mustaq, 2016-06-28 17:49:06]

>
https://codereview.chromium.org/2056333002/diff/60001/third_party/WebKit/Layo...
> > File
> >
>
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_common_input.js
> > (right):
> > 
> >
>
https://codereview.chromium.org/2056333002/diff/60001/third_party/WebKit/Layo...
> >
>
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_common_input.js:9:
> > eventSender.mouseDown(0);
> > On 2016/06/10 20:17:37, Rick Byers wrote:
> > > As you know, we ultimately want to avoid using eventSender because it
> doesn't
> > > test end-to-end.  But I love that you've abstracted this here - we can
> change
> > it
> > > once the gpuBenchmarking extension is better.
> > 
> > Yes. That is the plan. Although even current touch event generators seem to
be
> > problematic in some of the tests as sometimes they cause crash and sometimes
> > they cause memory leaks. I filed a separate bug for that.
> 
> Ugh, ok we'll have to dig into that.
> 
> One possible problem I just thought of with how you've written these APIs is
> that tests may depend on them being synchronous.  Eg. do a mouse move and
expect
> those handlers to run before doing whatever the next step is.  Once we make
all
> of this do full end-to-end input testing it'll have to be async.  That'll
> probably mean that we'll want these automation hooks to return Promises that
> resolve once the action has finished running.  I think the GPU benchmarking
APIs
> support this style already via callback arguments.
> 
> I probably should have mentioned this sooner (sorry).  So feel free to just
add
> a TODO saying eventually we'll need to convert all these helper function to be
> async.
>  

If I am counting the things right, majority of your injected scripts are working
fine. For the cases where js injection doesn't work (crashes, times out etc), it
seems logical to mark the tests as [Skip] & hold off the js files for a future
CL.
It is better to incrementally commit things that work (vs mixing working &
non-working things in a single commit).

[mustaq, 2016-06-28 17:49:19]

https://codereview.chromium.org/2056333002/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2056333002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:1143: crbug.com/471824
imported/wpt/pointerevents/pointerevent_pointerup_pointertype-manual.html  [
Failure Timeout ]
On 2016/06/21 15:43:46, Rick Byers wrote:
> this is a lot of new Timeout expectations to be adding, it may slow down total
> test running significantly (if each of these tests actually takes the 5s or
> whatever to timeout).  Is it just that these tests time out when pointer
events
> isn't supported?  Perhaps they should just be Skip instead until they no
longer
> time out?
> 
> We should talk about the testing strategy once PointerEvents is
> status=experimental.  Do we still want the virtual test suite?  If not you
could
> just go to status=test now and eliminate the virtual test suite first to make
> this simpler.

The virtual/pointerevent suite is gone, please update the expectations
accordingly.

https://codereview.chromium.org/2056333002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_common_input.js
(right):

https://codereview.chromium.org/2056333002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_common_input.js:64:
function scrollUpInTarget(targetId) {
On 2016/06/21 15:43:46, Rick Byers wrote:
> nit: I guess these should be named like "touchScrollUp..." to avoid confusion
> with mouse scroll?

Perhaps all touch-related actions should start with "touch...", to make the
input type obvious, as you are doing for mouse, pen and keyboard.

[Navid Zolghadr, 2016-06-30 17:32:51]

ptal.

https://codereview.chromium.org/2056333002/diff/120001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2056333002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:1143: crbug.com/471824
imported/wpt/pointerevents/pointerevent_pointerup_pointertype-manual.html  [
Failure Timeout ]
On 2016/06/28 17:49:19, mustaq wrote:
> On 2016/06/21 15:43:46, Rick Byers wrote:
> > this is a lot of new Timeout expectations to be adding, it may slow down
total
> > test running significantly (if each of these tests actually takes the 5s or
> > whatever to timeout).  Is it just that these tests time out when pointer
> events
> > isn't supported?  Perhaps they should just be Skip instead until they no
> longer
> > time out?
> > 
> > We should talk about the testing strategy once PointerEvents is
> > status=experimental.  Do we still want the virtual test suite?  If not you
> could
> > just go to status=test now and eliminate the virtual test suite first to
make
> > this simpler.
> 
> The virtual/pointerevent suite is gone, please update the expectations
> accordingly.

Done.

https://codereview.chromium.org/2056333002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_common_input.js
(right):

https://codereview.chromium.org/2056333002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_common_input.js:1:
// Mouse actions
On 2016/06/21 15:43:46, Rick Byers wrote:
> nit: please add a comment at the top summarizing what this file is for.

Done.

https://codereview.chromium.org/2056333002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_common_input.js:39:
mouseMoveIntoTarget(targetId);
On 2016/06/21 15:43:46, Rick Byers wrote:
> Doesn't this just send a mousemove to the exact same position as above?  For
> "mouseDrag" I assumed you'd want to move the mouse a bit (either within the
> target or out of the target or something)?

It does. But for the purpose of testing this causes the test to pass anyway as
we do send the event in Blink. Do you want me to actually send a different
position in the second action to make it more similar to the drag action?

https://codereview.chromium.org/2056333002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_common_input.js:64:
function scrollUpInTarget(targetId) {
On 2016/06/28 17:49:19, mustaq wrote:
> On 2016/06/21 15:43:46, Rick Byers wrote:
> > nit: I guess these should be named like "touchScrollUp..." to avoid
confusion
> > with mouse scroll?
> 
> Perhaps all touch-related actions should start with "touch...", to make the
> input type obvious, as you are doing for mouse, pen and keyboard.

Done.

[mustaq, 2016-06-30 19:13:10]

https://codereview.chromium.org/2056333002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_common_input.js
(right):

https://codereview.chromium.org/2056333002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_common_input.js:64:
function scrollUpInTarget(targetId) {
On 2016/06/30 17:32:51, Navid Zolghadr wrote:
> On 2016/06/28 17:49:19, mustaq wrote:
> > On 2016/06/21 15:43:46, Rick Byers wrote:
> > > nit: I guess these should be named like "touchScrollUp..." to avoid
> confusion
> > > with mouse scroll?
> > 
> > Perhaps all touch-related actions should start with "touch...", to make the
> > input type obvious, as you are doing for mouse, pen and keyboard.
> 
> Done.

Can't we rename tapInTarget too? May be touchInTarget? Or even touchTapInTarget?

[mustaq, 2016-06-30 19:14:31]

On 2016/06/28 17:49:06, mustaq wrote:
> >
>
https://codereview.chromium.org/2056333002/diff/60001/third_party/WebKit/Layo...
> > > File
> > >
> >
>
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_common_input.js
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2056333002/diff/60001/third_party/WebKit/Layo...
> > >
> >
>
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_common_input.js:9:
> > > eventSender.mouseDown(0);
> > > On 2016/06/10 20:17:37, Rick Byers wrote:
> > > > As you know, we ultimately want to avoid using eventSender because it
> > doesn't
> > > > test end-to-end.  But I love that you've abstracted this here - we can
> > change
> > > it
> > > > once the gpuBenchmarking extension is better.
> > > 
> > > Yes. That is the plan. Although even current touch event generators seem
to
> be
> > > problematic in some of the tests as sometimes they cause crash and
sometimes
> > > they cause memory leaks. I filed a separate bug for that.
> > 
> > Ugh, ok we'll have to dig into that.
> > 
> > One possible problem I just thought of with how you've written these APIs is
> > that tests may depend on them being synchronous.  Eg. do a mouse move and
> expect
> > those handlers to run before doing whatever the next step is.  Once we make
> all
> > of this do full end-to-end input testing it'll have to be async.  That'll
> > probably mean that we'll want these automation hooks to return Promises that
> > resolve once the action has finished running.  I think the GPU benchmarking
> APIs
> > support this style already via callback arguments.
> > 
> > I probably should have mentioned this sooner (sorry).  So feel free to just
> add
> > a TODO saying eventually we'll need to convert all these helper function to
be
> > async.
> >  
> 
> If I am counting the things right, majority of your injected scripts are
working
> fine. For the cases where js injection doesn't work (crashes, times out etc),
it
> seems logical to mark the tests as [Skip] & hold off the js files for a future
> CL.
> It is better to incrementally commit things that work (vs mixing working &
> non-working things in a single commit).

Any thoughts on holding off the used scripts? E.g. Since
pointerevent_lostpointercapture_for_disconnected_node
is skipped, why would we commit the automation js?

[mustaq, 2016-06-30 19:14:52]

On 2016/06/30 19:14:31, mustaq wrote:
> On 2016/06/28 17:49:06, mustaq wrote:
> > >
> >
>
https://codereview.chromium.org/2056333002/diff/60001/third_party/WebKit/Layo...
> > > > File
> > > >
> > >
> >
>
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_common_input.js
> > > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2056333002/diff/60001/third_party/WebKit/Layo...
> > > >
> > >
> >
>
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_common_input.js:9:
> > > > eventSender.mouseDown(0);
> > > > On 2016/06/10 20:17:37, Rick Byers wrote:
> > > > > As you know, we ultimately want to avoid using eventSender because it
> > > doesn't
> > > > > test end-to-end.  But I love that you've abstracted this here - we can
> > > change
> > > > it
> > > > > once the gpuBenchmarking extension is better.
> > > > 
> > > > Yes. That is the plan. Although even current touch event generators seem
> to
> > be
> > > > problematic in some of the tests as sometimes they cause crash and
> sometimes
> > > > they cause memory leaks. I filed a separate bug for that.
> > > 
> > > Ugh, ok we'll have to dig into that.
> > > 
> > > One possible problem I just thought of with how you've written these APIs
is
> > > that tests may depend on them being synchronous.  Eg. do a mouse move and
> > expect
> > > those handlers to run before doing whatever the next step is.  Once we
make
> > all
> > > of this do full end-to-end input testing it'll have to be async.  That'll
> > > probably mean that we'll want these automation hooks to return Promises
that
> > > resolve once the action has finished running.  I think the GPU
benchmarking
> > APIs
> > > support this style already via callback arguments.
> > > 
> > > I probably should have mentioned this sooner (sorry).  So feel free to
just
> > add
> > > a TODO saying eventually we'll need to convert all these helper function
to
> be
> > > async.
> > >  
> > 
> > If I am counting the things right, majority of your injected scripts are
> working
> > fine. For the cases where js injection doesn't work (crashes, times out
etc),
> it
> > seems logical to mark the tests as [Skip] & hold off the js files for a
future
> > CL.
> > It is better to incrementally commit things that work (vs mixing working &
> > non-working things in a single commit).
> 
> Any thoughts on holding off the used scripts? E.g. Since
> pointerevent_lostpointercapture_for_disconnected_node
> is skipped, why would we commit the automation js?

s/used/unused/, sorry!

[Navid Zolghadr, 2016-06-30 19:26:44]

I changed the name to touchTapInTarget. ptal.

I kept those js that are skipped because it makes it easier to debug them when
removing those skip rules. I actually debugged some of those and fixed them. For
some of them like the assert hit in some of gpu benchmarking I thought it is
better for example to leave the automation script to be able to just remove the
skip rule and run the test to see what went wrong.

[Navid Zolghadr, 2016-07-05 14:19:01]

On 2016/06/30 19:26:44, Navid Zolghadr wrote:
> I changed the name to touchTapInTarget. ptal.
> 
> I kept those js that are skipped because it makes it easier to debug them when
> removing those skip rules. I actually debugged some of those and fixed them.
For
> some of them like the assert hit in some of gpu benchmarking I thought it is
> better for example to leave the automation script to be able to just remove
the
> skip rule and run the test to see what went wrong.

Can you guys have another look and see if this looks okay?

[Rick Byers, 2016-07-05 15:01:30]

LGTM

https://codereview.chromium.org/2056333002/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/imported/wpt/resources/testharnessreport.js
(right):

https://codereview.chromium.org/2056333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/imported/wpt/resources/testharnessreport.js:68: 
There's not supposed to be a 2nd copy of the 'resources' directory here (I
assume you applied this edit to both copies manually, right?).  I filed
https://crbug.com/625846 to get this fixed.

https://codereview.chromium.org/2056333002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_touch-action-button-test_touch-manual-input.js
(right):

https://codereview.chromium.org/2056333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_touch-action-button-test_touch-manual-input.js:6:
touchTapInTarget('btnComplete');
I was originally surprised that this works.  The gpubenchmarking commands are
asynchronous, so when we execute touchTapInTarget the scroll hasn't occurred
yet, so we're getting the client co-ordinates of the button prior to the scroll.
 But of course in this case we're scrolling a div, not the document, so the
client co-ords aren't invalidated.

Aren't there already cases where we want to scroll the document and then tap
somewhere in the document?  If not, I'm sure there will be at some point.  How
will we handle those?

In general, I believe we're eventually going to hit a case where we really to
write these tests in a fully async style - eg. by chaining promises.  I'm fine
putting this off for now, but we should definitely keep our eye out for when
we'll really need it.

[Navid Zolghadr, 2016-07-05 15:42:21]

https://codereview.chromium.org/2056333002/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/imported/wpt/resources/testharnessreport.js
(right):

https://codereview.chromium.org/2056333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/imported/wpt/resources/testharnessreport.js:68: 
On 2016/07/05 15:01:30, Rick Byers wrote:
> There's not supposed to be a 2nd copy of the 'resources' directory here (I
> assume you applied this edit to both copies manually, right?).  I filed
> https://crbug.com/625846 to get this fixed.

Yes. I did it manually. There is a presubmit check that makes sure these two
files are the same.

https://codereview.chromium.org/2056333002/diff/200001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_touch-action-button-test_touch-manual-input.js
(right):

https://codereview.chromium.org/2056333002/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_touch-action-button-test_touch-manual-input.js:6:
touchTapInTarget('btnComplete');
On 2016/07/05 15:01:30, Rick Byers wrote:
> I was originally surprised that this works.  The gpubenchmarking commands are
> asynchronous, so when we execute touchTapInTarget the scroll hasn't occurred
> yet, so we're getting the client co-ordinates of the button prior to the
scroll.
>  But of course in this case we're scrolling a div, not the document, so the
> client co-ords aren't invalidated.
> 
> Aren't there already cases where we want to scroll the document and then tap
> somewhere in the document?  If not, I'm sure there will be at some point.  How
> will we handle those?
> 
> In general, I believe we're eventually going to hit a case where we really to
> write these tests in a fully async style - eg. by chaining promises.  I'm fine
> putting this off for now, but we should definitely keep our eye out for when
> we'll really need it.

I see. There might be some problems already as I have skipped a few of these
touch tests due to some crashes. However, I don't think the current APIs like
tap returns a promise. Right? But if I'm not mistaken they did have a callback
parameter. So I guess I can use the callback parameter to do that
synchronization. Would that be okay?

[Rick Byers, 2016-07-05 16:38:38]

On 2016/07/05 15:42:21, Navid Zolghadr wrote:
>
https://codereview.chromium.org/2056333002/diff/200001/third_party/WebKit/Lay...
> File
third_party/WebKit/LayoutTests/imported/wpt/resources/testharnessreport.js
> (right):
> 
>
https://codereview.chromium.org/2056333002/diff/200001/third_party/WebKit/Lay...
> third_party/WebKit/LayoutTests/imported/wpt/resources/testharnessreport.js:68:

> On 2016/07/05 15:01:30, Rick Byers wrote:
> > There's not supposed to be a 2nd copy of the 'resources' directory here (I
> > assume you applied this edit to both copies manually, right?).  I filed
> > https://crbug.com/625846 to get this fixed.
> 
> Yes. I did it manually. There is a presubmit check that makes sure these two
> files are the same.

Ah, thanks - I misunderstood the history here.  Still would be nice to unify
some day - updated the bug.

>
https://codereview.chromium.org/2056333002/diff/200001/third_party/WebKit/Lay...
> File
>
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_touch-action-button-test_touch-manual-input.js
> (right):
> 
>
https://codereview.chromium.org/2056333002/diff/200001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_touch-action-button-test_touch-manual-input.js:6:
> touchTapInTarget('btnComplete');
> On 2016/07/05 15:01:30, Rick Byers wrote:
> > I was originally surprised that this works.  The gpubenchmarking commands
are
> > asynchronous, so when we execute touchTapInTarget the scroll hasn't occurred
> > yet, so we're getting the client co-ordinates of the button prior to the
> scroll.
> >  But of course in this case we're scrolling a div, not the document, so the
> > client co-ords aren't invalidated.
> > 
> > Aren't there already cases where we want to scroll the document and then tap
> > somewhere in the document?  If not, I'm sure there will be at some point. 
How
> > will we handle those?
> > 
> > In general, I believe we're eventually going to hit a case where we really
to
> > write these tests in a fully async style - eg. by chaining promises.  I'm
fine
> > putting this off for now, but we should definitely keep our eye out for when
> > we'll really need it.
> 
> I see. There might be some problems already as I have skipped a few of these
> touch tests due to some crashes. However, I don't think the current APIs like
> tap returns a promise. Right? But if I'm not mistaken they did have a callback
> parameter. So I guess I can use the callback parameter to do that
> synchronization. Would that be okay?

Yes, if updating the GPU benchmarking extension to return a promise is
non-trivial then you can just have your utility functions defined in terms of
promises but implemented on top of GPU benchmarking using callbacks (that way
most of the test code can still get the convenience of chaining async code using
modern promise-style).

Note that this is a key difference between eventSender and gpuBenchmarking. 
eventSender is synchronous, and so tests using it may be taking a dependency on
that.  We can clean this up later as you move away from eventSender (though
there's some risk more tests will accumulate which copy the existing pattern and
come to depend on the synchronous semantics).

[Navid Zolghadr, 2016-07-05 16:41:04]

On 2016/07/05 16:38:38, Rick Byers wrote:
> On 2016/07/05 15:42:21, Navid Zolghadr wrote:
> >
>
https://codereview.chromium.org/2056333002/diff/200001/third_party/WebKit/Lay...
> > File
> third_party/WebKit/LayoutTests/imported/wpt/resources/testharnessreport.js
> > (right):
> > 
> >
>
https://codereview.chromium.org/2056333002/diff/200001/third_party/WebKit/Lay...
> >
third_party/WebKit/LayoutTests/imported/wpt/resources/testharnessreport.js:68:
> 
> > On 2016/07/05 15:01:30, Rick Byers wrote:
> > > There's not supposed to be a 2nd copy of the 'resources' directory here (I
> > > assume you applied this edit to both copies manually, right?).  I filed
> > > https://crbug.com/625846 to get this fixed.
> > 
> > Yes. I did it manually. There is a presubmit check that makes sure these two
> > files are the same.
> 
> Ah, thanks - I misunderstood the history here.  Still would be nice to unify
> some day - updated the bug.
> 
> >
>
https://codereview.chromium.org/2056333002/diff/200001/third_party/WebKit/Lay...
> > File
> >
>
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_touch-action-button-test_touch-manual-input.js
> > (right):
> > 
> >
>
https://codereview.chromium.org/2056333002/diff/200001/third_party/WebKit/Lay...
> >
>
third_party/WebKit/LayoutTests/imported/wpt_automation/pointerevents/pointerevent_touch-action-button-test_touch-manual-input.js:6:
> > touchTapInTarget('btnComplete');
> > On 2016/07/05 15:01:30, Rick Byers wrote:
> > > I was originally surprised that this works.  The gpubenchmarking commands
> are
> > > asynchronous, so when we execute touchTapInTarget the scroll hasn't
occurred
> > > yet, so we're getting the client co-ordinates of the button prior to the
> > scroll.
> > >  But of course in this case we're scrolling a div, not the document, so
the
> > > client co-ords aren't invalidated.
> > > 
> > > Aren't there already cases where we want to scroll the document and then
tap
> > > somewhere in the document?  If not, I'm sure there will be at some point. 
> How
> > > will we handle those?
> > > 
> > > In general, I believe we're eventually going to hit a case where we really
> to
> > > write these tests in a fully async style - eg. by chaining promises.  I'm
> fine
> > > putting this off for now, but we should definitely keep our eye out for
when
> > > we'll really need it.
> > 
> > I see. There might be some problems already as I have skipped a few of these
> > touch tests due to some crashes. However, I don't think the current APIs
like
> > tap returns a promise. Right? But if I'm not mistaken they did have a
callback
> > parameter. So I guess I can use the callback parameter to do that
> > synchronization. Would that be okay?
> 
> Yes, if updating the GPU benchmarking extension to return a promise is
> non-trivial then you can just have your utility functions defined in terms of
> promises but implemented on top of GPU benchmarking using callbacks (that way
> most of the test code can still get the convenience of chaining async code
using
> modern promise-style).
> 
> Note that this is a key difference between eventSender and gpuBenchmarking. 
> eventSender is synchronous, and so tests using it may be taking a dependency
on
> that.  We can clean this up later as you move away from eventSender (though
> there's some risk more tests will accumulate which copy the existing pattern
and
> come to depend on the synchronous semantics).

I'll need to take a look around to see how easy changing gpubenchmarking is to
return promises. I'll take care of it in the future CLs.

[Navid Zolghadr, 2016-07-05 17:35:37]

The CQ bit was checked by nzolghadr@chromium.org

[commit-bot: I haz the power, 2016-07-05 17:35:51]

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-07-05 18:53:44]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-07-05 18:53:46]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-05 18:55:13]

Description was changed from

==========
Add the rest of web platform pointerevent tests

Add input injection for the rest of web platform
pointerevent tests.

BUG=612924
==========

to

==========
Add the rest of web platform pointerevent tests

Add input injection for the rest of web platform
pointerevent tests.

BUG=612924

Committed: https://crrev.com/23ce92bfe9a1160e552a91d443e788f303f1eb6d
Cr-Commit-Position: refs/heads/master@{#403812}
==========

[commit-bot: I haz the power, 2016-07-05 18:55:15]

Patchset 11 (id:??) landed as
https://crrev.com/23ce92bfe9a1160e552a91d443e788f303f1eb6d
Cr-Commit-Position: refs/heads/master@{#403812}

[phoglund_chromium, 2016-07-06 07:54:20]

A revert of this CL (patchset #11 id:200001) has been created in
https://codereview.chromium.org/2129453002/ by phoglund@chromium.org.

The reason for reverting is: Pointer event tests fail on linux:
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20%28dbg...
and Win:
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Leak/b...
(possibly others)..