When a frame finishes loading the last batch of progress reports and status signals goes like this: ... 80%, 90%, FINISHED, 100%. This is because the code in FrameLoader::checkLoadCompleteForThisFrame will call first progressCompleted() and then dispatchDidFinishLoad(). IMHO this goes against the natural expectation of receiving all the progress reports before the FINISHED notification, so I propose to reverse the order of the function calls. The attached patch does that, and does not seem to regress any layout tests here. This surely should get some tests with it, but I'm not sure of what should I be testing, so suggestions are welcome.
Created attachment 38827 [details] progressbeforefinished.patch
I believe Brady is still our loader expert.
(In reply to comment #0) > This is because the code in FrameLoader::checkLoadCompleteForThisFrame will > call first progressCompleted() and then dispatchDidFinishLoad(). I guess it's obvious, but the current code does exactly the opposite and that is what I'm proposing to do now. Sorry for the confusion :)
Ping?
The concept is fine. If all layout tests continue to pass on all platforms, then the patch is fine. It makes me nervous that we've had this behavior forever and are changing it now. But we need tests. That we've had this bizarre behavior forever and no layout test covered it is something that needs to change. DRT already listens to didFinishLoadForFrame, but can definitely be augmented to listen for the progress notifications also. Sometimes the DRT changes to test a patch are 20 times bigger than the patch itself - This might be one of those times... ;)
Comment on attachment 38827 [details] progressbeforefinished.patch Code change looks good, but we can't land this without layout tests.
(In reply to comment #6) > (From update of attachment 38827 [details]) > Code change looks good, but we can't land this without layout tests. Right, so my point is that I'm not really sure what should I be testing exactly. I suppose ideally you'd identify possible places in the code that *could* be relying on the old behavior and see that they don't break, but that's surely impossible to do correctly or thoroughly? Could you give me any indication about what kind of test would you like to see here?
Created attachment 103537 [details] Patch
This patch still doesn't cover all the platforms, but it's already working on Qt port. I uploaded to get some feedback whether the test is correct, or if we need a different kind of testing. The "unnatural" ordering also surfaces on QtWebKit API, I'm making tests to cover the API as well.
Comment on attachment 103537 [details] Patch I think this is the right approach, but we'll need implementations on all the ports (sadly). I agree with Comment #5.
Created attachment 104251 [details] Patch
(Work in progress) Implemented the necessary things in Chromium and GTK DRT. Chromium and GTK LayoutTests work exactly like without them in my machine. I don't get 100% pass in neither, but the number of failings don't change, some help in testing will be appreciated. API tests for Chromium pass, but I think they don't exercise this change. GTK ones showed a problem, that I think I've fixed in bug 66243, that I'm marking as dependency for this bug.
Comment on attachment 104251 [details] Patch Attachment 104251 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9420263
Created attachment 104336 [details] Patch
Created attachment 104834 [details] Patch
(In reply to comment #15) > Created an attachment (id=104834) [details] > Patch Adding skips for Mac and Win ports with links to bug reports to implement them.
Martin and Xan, since the patch involves a bit of non-trivial changes in GTK implementation of DRT would be very nice to get some review/comments from GTK reviewers. Could one of you take a look at this patch?
Comment on attachment 104834 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=104834&action=review > Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:785 > + // The deprecated "load-finished" signal is triggered by postProgressFinishedNotification(), > + // so we can use it here in the DRT to provide the correct dump. > if (frame != topLoadingFrame) > return; > - > - topLoadingFrame = 0; > - WorkQueue::shared()->setFrozen(true); // first complete load freezes the queue for the rest of this test > - if (gLayoutTestController->waitToDump()) > - return; > - > - if (WorkQueue::shared()->count()) > - g_timeout_add(0, processWork, 0); > - else > - dump(); > + if (gLayoutTestController->dumpProgressFinishedCallback()) > + printf("postProgressFinishedNotification\n"); I'm not sure I understand why you are doing this work at a different time now. > Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1051 > case WEBKIT_LOAD_FINISHED: > - if (frame != topLoadingFrame || !done) > + if (!done) > printf("%s - didFinishLoadForFrame\n", frameName.get()); > break; Why do we now print this notification for non-main-frames?
Comment on attachment 104834 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=104834&action=review Martin, thanks for the review. >> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:785 >> + printf("postProgressFinishedNotification\n"); > > I'm not sure I understand why you are doing this work at a different time now. This work was happening (in FrameLoaderClient terms) after the load has finished, during the progress finished. With the patch those two moments will happen in reverse order. Which means that is too soon for clearing "topLoadingFrame" as well as to call dump() (that will set "done = true"), because we still have an load-status callback to process later. >> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1051 >> break; > > Why do we now print this notification for non-main-frames? I didn't understand your question. Did you mean "for main-frames"? If so, we should print the didFinishLoadForFrame for main frames as well, and this is done even before this patch. The topLoadingFrame was being printed because we were "!done" yet, and after we would get progress finished that would change set done to true and reset topLoadingFrame. My understanding is that in this code, the first half of the condition wasn't helping at all.
Comment on attachment 104834 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=104834&action=review The GTK+ parts look good to me. >>> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:785 >>> + printf("postProgressFinishedNotification\n"); >> >> I'm not sure I understand why you are doing this work at a different time now. > > This work was happening (in FrameLoaderClient terms) after the load has finished, during the progress finished. With the patch those two moments will happen in reverse order. Which means that is too soon for clearing "topLoadingFrame" as well as to call dump() (that will set "done = true"), because we still have an load-status callback to process later. Makes sense. >>> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1051 >>> break; >> >> Why do we now print this notification for non-main-frames? > > I didn't understand your question. Did you mean "for main-frames"? > > If so, we should print the didFinishLoadForFrame for main frames as well, and this is done even before this patch. The topLoadingFrame was being printed because we were "!done" yet, and after we would get progress finished that would change set done to true and reset topLoadingFrame. My understanding is that in this code, the first half of the condition wasn't helping at all. Thanks for clearing that up.
Looks like everyone is happy with this patch.
Comment on attachment 104834 [details] Patch Rejecting attachment 104834 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: rTree/chromium/LayoutTestController.h Hunk #2 succeeded at 415 (offset 2 lines). Hunk #3 succeeded at 549 (offset 2 lines). patching file Tools/DumpRenderTree/chromium/WebViewHost.cpp patching file Tools/DumpRenderTree/gtk/DumpRenderTree.cpp patching file Tools/DumpRenderTree/qt/LayoutTestControllerQt.cpp patching file Tools/DumpRenderTree/qt/LayoutTestControllerQt.h Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Adam Barth', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/9564544
Created attachment 105671 [details] Patch for landing
Comment on attachment 105671 [details] Patch for landing Rejecting attachment 105671 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: tTestController.h patching file Tools/DumpRenderTree/chromium/LayoutTestController.cpp patching file Tools/DumpRenderTree/chromium/LayoutTestController.h patching file Tools/DumpRenderTree/chromium/WebViewHost.cpp patching file Tools/DumpRenderTree/gtk/DumpRenderTree.cpp patching file Tools/DumpRenderTree/qt/LayoutTestControllerQt.cpp patching file Tools/DumpRenderTree/qt/LayoutTestControllerQt.h Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 Full output: http://queues.webkit.org/results/9567518
Created attachment 105684 [details] Patch for landing
Comment on attachment 105684 [details] Patch for landing Clearing flags on attachment: 105684 Committed r94105: <http://trac.webkit.org/changeset/94105>
All reviewed patches have been landed. Closing bug.
Revision r94105 cherry-picked into qtwebkit-2.2 with commit 3f29aef <http://gitorious.org/webkit/qtwebkit/commit/3f29aef>