Bug 37830 – send-oncancel-event touch test should use js-test-post-function.js

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Ben Murdoch]

When the touch layout tests were moved in http://trac.webkit.org/changeset/54253 send-oncancel-event was changed to not use js-test-post-function.js erroneously. This meant that the test threw an error as it used the isSuccessfullyParsed function. This failure was avoided in http://trac.webkit.org/changeset/55735 which removed the reference to isSuccessfullyParsed. Due to the asynchronous nature of touch events on Android, we need to use js-test-post-function to run an asynchronous test.

Patch to follow to reinsert the call to isSuccesfullyParsed and make the test use js-test-post-function.

[Ben Murdoch]

Created attachment 53737 [details]
Patch.

[Jeremy Orlow]

Comment on attachment 53737 [details]
Patch.

r=me

[WebKit Commit Bot]

Comment on attachment 53737 [details]
Patch.

Clearing flags on attachment: 53737

Committed r58058: <http://trac.webkit.org/changeset/58058>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

Created attachment 54107 [details]
Patch

[James Robinson]

Hey guys.  The first patch broke Chromium because in Chromium the touch stuff is not deferred and so the entire test body was running before hitting the end of send-oncancel-events.js.  This means that js-test-post-function.js was not getting parsed, so isSuccessfullyParsed was not being defined, and the 'var successfullyParsed = true' line was not able to run.  This patch moves the test body to the onload event which should work fine on all platforms.  I don't have any way to verify this on Android.  Thanks

[Ben Murdoch]

Ah, I see. Sorry about that. Alternatively, we could set isSuccessfullyParsed right before calling layoutTestController.notifyDone, like in:

http://trac.webkit.org/browser/trunk/LayoutTests/fast/events/touch/script-tests/basic-single-touch-events.js#L23 ?

Thanks, Ben

[Ben Murdoch]

Sorry, for that to work we'd also need to move the js-test-post-function link inside the send-oncancel-event.html file to above the link to the test sctipt, like it's done in basic-single-touch-events.html.

What do you think? It would be nice to maintain consistency.

[James Robinson]

I really dislike the pattern in basic-single-touch-events.js to be honest.  It's too easy to add code below the touchTargets() call that runs on some platforms (that do touch events synchronously) and does not run on others (which do touch events asynchronously).  To be consistent we should make sure that all platforms have a chance to evaluate the same javascript and then run the test.  The simplest way to do that is to just put the actual test body in the onload handler or later.

[Ben Murdoch]

I can confirm that the window.onload approach works on Android.

Looking through the current touch tests and the TEMPLATE.html for those tests, there seems to be some tidying up needed. I think that future tests should make use of TEMPLATE.html (which incidentally also needs fixing as it was a copy from another directory where asynchronous tests were not needed) rather than relying on remembering to run the test code inside window.onload. We can make TEMPLATE.html link to the scripts in the correct order. I don't mind doing this if you'd like to submit this now as-is.

Cheers, Ben

[Yuzo Fujishima]

*** Bug 38020 has been marked as a duplicate of this bug. ***

[Yuzo Fujishima]

Changed LayoutTests/platform/chromium/test_expectations.txt to mark the test failing.

Please change it again once the patch is landed.

[Ben Murdoch]

With the resolution of https://bugs.webkit.org/show_bug.cgi?id=38084 I think this should now be fixed for Chromium. James, would you mind syncing past r58644 to verify?

Thanks, Ben

[Darin Adler]

Comment on attachment 54107 [details]
Patch

Looks OK. r=me

[WebKit Commit Bot]

Comment on attachment 54107 [details]
Patch

Clearing flags on attachment: 54107

Committed r66695: <http://trac.webkit.org/changeset/66695>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.