| Summary: | send-oncancel-event touch test should use js-test-post-function.js | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ben Murdoch <benm> | ||||||
| Component: | Tools / Tests | Assignee: | Ben Murdoch <benm> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| 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: | |||||||||
| Attachments: |
|
||||||||
|
Description
Ben Murdoch
2010-04-19 16:04:35 PDT
Created attachment 53737 [details]
Patch.
Comment on attachment 53737 [details]
Patch.
r=me
Comment on attachment 53737 [details] Patch. Clearing flags on attachment: 53737 Committed r58058: <http://trac.webkit.org/changeset/58058> All reviewed patches have been landed. Closing bug. Created attachment 54107 [details]
Patch
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 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 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. 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. 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 *** Bug 38020 has been marked as a duplicate of this bug. *** Changed LayoutTests/platform/chromium/test_expectations.txt to mark the test failing. Please change it again once the patch is landed. 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 on attachment 54107 [details]
Patch
Looks OK. r=me
Comment on attachment 54107 [details] Patch Clearing flags on attachment: 54107 Committed r66695: <http://trac.webkit.org/changeset/66695> All reviewed patches have been landed. Closing bug. |