Bug 30269 - Waiting for iframe load should use onload, not setTimeout
Summary: Waiting for iframe load should use onload, not setTimeout
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Julie Parent
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-09 18:42 PDT by Julie Parent
Modified: 2009-10-14 15:43 PDT (History)
1 user (show)

See Also:


Attachments
Re-writes some editing tests to use iframe load events rather than setTimeout (7.73 KB, patch)
2009-10-12 14:11 PDT, Julie Parent
darin: review+
Details | Formatted Diff | Diff
Updated to address Ojan's concern of iframe load happening before script is loaded. (10.10 KB, patch)
2009-10-13 16:10 PDT, Julie Parent
adele: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Added rebaselines for editing/selection/5136696. Moving the script tag removed an empty text node from the render tree. (12.79 KB, patch)
2009-10-13 18:00 PDT, Julie Parent
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julie Parent 2009-10-09 18:42:25 PDT
Using setTimeout for iframe load leads to flaky tests, leads to unhappiness!
Comment 1 Julie Parent 2009-10-12 14:11:22 PDT
Created attachment 41058 [details]
Re-writes some editing tests to use iframe load events rather than setTimeout
Comment 2 Darin Adler 2009-10-12 14:23:52 PDT
Comment on attachment 41058 [details]
Re-writes some editing tests to use iframe load events rather than setTimeout

Assuming these tests all still pass, r=me
Comment 3 Ojan Vafai 2009-10-12 15:02:14 PDT
> ===================================================================
> --- LayoutTests/editing/execCommand/paste-1.html	(revision 49408)
> +++ LayoutTests/editing/execCommand/paste-1.html	(working copy)
> @@ -3,7 +3,7 @@ if (window.layoutTestController)
>       layoutTestController.dumpEditingCallbacks();
>  </script>
>  <p>This tests cut/paste inside an editable iframe.  You should see 'foo bar baz' below.</p>
> -<iframe src="../resources/contenteditable-iframe-src.html"></iframe>
> +<iframe src="../resources/contenteditable-iframe-src.html" onload="foo();"></iframe>
>  
>  <script>
>  function foo() {
> @@ -21,5 +21,4 @@ function foo() {
>  }
>  if (window.layoutTestController)
>      window.layoutTestController.waitUntilDone();
> -window.setTimeout(foo, 100);

There's still theoretically a race condition here (and in the other tests), right? If the iframe loads before foo is defined or waitUntilDone is called? It's easy enough to fix by moving the iframe after the script tag right?
Comment 4 Julie Parent 2009-10-13 16:10:20 PDT
Created attachment 41135 [details]
Updated to address Ojan's concern of iframe load happening before script is loaded.
Comment 5 Ojan Vafai 2009-10-13 16:41:07 PDT
Yup. That's what I was asking for. I don't think it was a very likely race condition, but while you're fixing this up, may as well fix it.
Comment 6 WebKit Commit Bot 2009-10-13 17:05:40 PDT
Comment on attachment 41135 [details]
Updated to address Ojan's concern of iframe load happening before script is loaded.

Rejecting patch 41135 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11430 test cases.
editing/selection/5136696.html -> failed

Exiting early after 1 failures. 4127 tests run.
66.95s total testing time

4126 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
1 test case (<1%) had stderr output
Comment 7 Julie Parent 2009-10-13 18:00:50 PDT
Created attachment 41143 [details]
Added rebaselines for editing/selection/5136696.  Moving the script tag removed an empty text node from the render tree.

Failed commit queue because of a whitespace change (removed a script tag with newline, now there is no empty text node in the render tree).  Rebaselined the test to remove the empty text node.
Comment 8 Julie Parent 2009-10-14 15:42:39 PDT
Committed: http://trac.webkit.org/changeset/49592