Bug 37830

Summary: send-oncancel-event touch test should use js-test-post-function.js
Product: WebKit Reporter: Ben Murdoch <benm>
Component: Tools / TestsAssignee: Ben Murdoch <benm>
Severity: Normal CC: android-webkit-unforking, commit-queue, gdk, hausmann, jamesr, kim.1.gronholm, yuzo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Android   
OS: Android   
Bug Depends on: 38084    
Bug Blocks:    
Description Flags
Patch none

Description Ben Murdoch 2010-04-19 16:04:35 PDT
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.
Comment 1 Ben Murdoch 2010-04-19 16:42:17 PDT
Created attachment 53737 [details]
Comment 2 Jeremy Orlow 2010-04-21 18:08:13 PDT
Comment on attachment 53737 [details]

Comment 3 WebKit Commit Bot 2010-04-21 23:02:14 PDT
Comment on attachment 53737 [details]

Clearing flags on attachment: 53737

Committed r58058: <http://trac.webkit.org/changeset/58058>
Comment 4 WebKit Commit Bot 2010-04-21 23:02:19 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 James Robinson 2010-04-22 16:34:18 PDT
Created attachment 54107 [details]
Comment 6 James Robinson 2010-04-22 16:36:13 PDT
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
Comment 7 Ben Murdoch 2010-04-22 16:51:51 PDT
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
Comment 8 Ben Murdoch 2010-04-22 16:59:58 PDT
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.
Comment 9 James Robinson 2010-04-22 17:02:58 PDT
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.
Comment 10 Ben Murdoch 2010-04-22 17:31:00 PDT
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
Comment 11 Yuzo Fujishima 2010-04-22 18:34:11 PDT
*** Bug 38020 has been marked as a duplicate of this bug. ***
Comment 12 Yuzo Fujishima 2010-04-22 18:35:51 PDT
Changed LayoutTests/platform/chromium/test_expectations.txt to mark the test failing.

Please change it again once the patch is landed.
Comment 13 Ben Murdoch 2010-05-04 09:06:04 PDT
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
Comment 14 Darin Adler 2010-06-12 18:52:29 PDT
Comment on attachment 54107 [details]

Looks OK. r=me
Comment 15 WebKit Commit Bot 2010-09-02 17:01:52 PDT
Comment on attachment 54107 [details]

Clearing flags on attachment: 54107

Committed r66695: <http://trac.webkit.org/changeset/66695>
Comment 16 WebKit Commit Bot 2010-09-02 17:01:59 PDT
All reviewed patches have been landed.  Closing bug.