Make unload-reparent-sibling-frame.html wait for completion
Created attachment 184819 [details] Patch
Comment on attachment 184819 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184819&action=review This is kind of changing what this test is testing, no? As in, it used to also test that we'd fire the onload events for both frames when they're reparented and the description seems to be that it's explicitly testing that. Adding waituntildone/notifydone makes sense, but I think you need to notifyDone in teh onload handlers. > LayoutTests/fast/frames/unload-reparent-sibling-frame.html:40 > + isSuccessfullyParsed(); Ideally you'd include js-test-post.js. That ensures that the test doesn't appear to pass if you accidentally introduce a JS error. I don't think this actually does what it's intended to if don't include js-test-post.js (i.e. it's supposed to spit out an error if it's not successfully parsed.
Comment on attachment 184819 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184819&action=review > LayoutTests/fast/frames/unload-reparent-sibling-frame.html:37 > + setTimeout(function() { Why the timeout?
(In reply to comment #2) > (From update of attachment 184819 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=184819&action=review > > This is kind of changing what this test is testing, no? As in, it used to also test that we'd fire the onload events for both frames when they're reparented and the description seems to be that it's explicitly testing that. Adding waituntildone/notifydone makes sense, but I think you need to notifyDone in teh onload handlers. We know it fired the onload event if the text in the dump contains the value of the srcdoc. You can't depend on the onload handlers because they may fire synchronously inside appendChild. > > > LayoutTests/fast/frames/unload-reparent-sibling-frame.html:40 > > + isSuccessfullyParsed(); > > Ideally you'd include js-test-post.js. That ensures that the test doesn't appear to pass if you accidentally introduce a JS error. I don't think this actually does what it's intended to if don't include js-test-post.js (i.e. it's supposed to spit out an error if it's not successfully parsed. Nah, js-test-post.js in async mode just waits for you to call finishJSTest() and then calls isSuccessfullyParsed() which is exactly equivalent to this code. If I used post.js I'd need to set jsTestIsAsync = true and then call finishJsTest() where I have isSuccessfullyParsed() right now and the output would be identical.
(In reply to comment #3) > (From update of attachment 184819 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=184819&action=review > > > LayoutTests/fast/frames/unload-reparent-sibling-frame.html:37 > > + setTimeout(function() { > > Why the timeout? We have to wait until the current ChildFrameDisconnector::disconnect() call completes before looking at the loaded state of the documents. The bug this test is for is that we're in the middle of iterating the list of frames to disconnect in ChildFrameDisconnector::disconnectFrameOwners(), and frame at index N moves the frame at index N+k to a different location in the document but with the same parentElement() by moving some ancestor, then when we get to frame N+k we disconnect it even though it's no longer in the subtree we should be disconnecting because the old isValid() method would have returned true. See: https://trac.webkit.org/changeset/140856
Comment on attachment 184819 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184819&action=review >>> LayoutTests/fast/frames/unload-reparent-sibling-frame.html:40 >>> + isSuccessfullyParsed(); >> >> Ideally you'd include js-test-post.js. That ensures that the test doesn't appear to pass if you accidentally introduce a JS error. I don't think this actually does what it's intended to if don't include js-test-post.js (i.e. it's supposed to spit out an error if it's not successfully parsed. > > Nah, js-test-post.js in async mode just waits for you to call finishJSTest() and then calls isSuccessfullyParsed() which is exactly equivalent to this code. If I used post.js I'd need to set jsTestIsAsync = true and then call finishJsTest() where I have isSuccessfullyParsed() right now and the output would be identical. You're right. That said, the more code we have that digs into the internals of the js-test functions, the harder it is to refactor them later (e.g. to get rid of js-test-post).
(In reply to comment #4) > (In reply to comment #2) > > (From update of attachment 184819 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=184819&action=review > > > > This is kind of changing what this test is testing, no? As in, it used to also test that we'd fire the onload events for both frames when they're reparented and the description seems to be that it's explicitly testing that. Adding waituntildone/notifydone makes sense, but I think you need to notifyDone in teh onload handlers. > > We know it fired the onload event if the text in the dump contains the value of the srcdoc. You can't depend on the onload handlers because they may fire synchronously inside appendChild. Isn't this a race condition? If the onload hasn't fired yet, the text dump will have the wrong output, no?
Created attachment 184838 [details] Patch Address review comments
(In reply to comment #7) > ... > Isn't this a race condition? If the onload hasn't fired yet, the text dump will have the wrong output, no? I think you're right, fixed in the new patch.
Comment on attachment 184838 [details] Patch Clearing flags on attachment: 184838 Committed r140885: <http://trac.webkit.org/changeset/140885>
All reviewed patches have been landed. Closing bug.