Some tests under http/test/loading fail due to the lack of reporting in DRT
Created attachment 51316 [details] Print didFinishLoadForFrame outcome in DRT
Created attachment 51317 [details] Add a CR after printing didFinishDocumentLoadForFram
Created attachment 51318 [details] Print didStartProvisionalLoadForFrame in the callback of notify::load-status property change notification
Attachment 51318 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKitTools/DumpRenderTree/gtk/DumpRenderTree.cpp:810: Extra space before ( in function call [whitespace/parens] [4] WebKitTools/DumpRenderTree/gtk/DumpRenderTree.cpp:812: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 2 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 51319 [details] Print didCommitLoadForFrame in the callback of signal::load-committed
Created attachment 51320 [details] Added "onload-event" signal to WebKitWebView. Fix several loading tests This patch does: * Added "onload-event" signal to WebKitWebView * FrameLoader emits onload-event when handling dispatchDidHandleOnloadEvents * Removed 5 tests from Skipped file
Comment on attachment 51316 [details] Print didFinishLoadForFrame outcome in DRT Change looks OK, but you need to fill in the ChangeLog with information about the change. See http://webkit.org/coding/contributing.html
Comment on attachment 51317 [details] Add a CR after printing didFinishDocumentLoadForFram Change again looks OK, but ChangeLog needs work. see http://webkit.org/coding/contributing.html. ChangeLogs are important both for the review of patches, and for when other people wish to understand what your code does (during svn annotate, etc.)
Comment on attachment 51318 [details] Print didStartProvisionalLoadForFrame in the callback of notify::load-status property change notification Again looks fine. Again needs ChangeLog. Shouldn't tests be un-skipped after these changes?
Comment on attachment 51319 [details] Print didCommitLoadForFrame in the callback of signal::load-committed ChangeLog :(
Created attachment 51622 [details] Print didFinishLoadForFrame outcome in DRT Improved Changelog
Created attachment 51623 [details] Add a CR after printing didFinishDocumentLoadForFrame
Created attachment 51624 [details] Print didStartProvisionalLoadForFrame in the callback of notify::load-status property change notification
Created attachment 51625 [details] Print didCommitLoadForFrame in the callback of signal::load-committed
Created attachment 51626 [details] Added "onload-event" signal to WebKitWebView. Fix several loading tests
I sent small patches to make them easier to review. If committing small patches is a problem I can create a single one.
Comment on attachment 51622 [details] Print didFinishLoadForFrame outcome in DRT Yay! Looking forward to the un-skipping!
Comment on attachment 51623 [details] Add a CR after printing didFinishDocumentLoadForFrame Yay! Looking forward to the un-skipping!
Comment on attachment 51624 [details] Print didStartProvisionalLoadForFrame in the callback of notify::load-status property change notification Excellent.
Comment on attachment 51625 [details] Print didCommitLoadForFrame in the callback of signal::load-committed OK.
Comment on attachment 51626 [details] Added "onload-event" signal to WebKitWebView. Fix several loading tests OK.
Comment on attachment 51622 [details] Print didFinishLoadForFrame outcome in DRT Clearing flags on attachment: 51622 Committed r56645: <http://trac.webkit.org/changeset/56645>
Looks like the first checkin caused a test failure: http://build.webkit.org/results/GTK%20Linux%2064-bit%20Release/r56645%20(1381)/http/tests/loading/redirect-with-no-location-crash-pretty-diff.html Maybe that will stop failign once they're all checked in?
(In reply to comment #23) > Looks like the first checkin caused a test failure: > http://build.webkit.org/results/GTK%20Linux%2064-bit%20Release/r56645%20(1381)/http/tests/loading/redirect-with-no-location-crash-pretty-diff.html > > Maybe that will stop failign once they're all checked in? Looks like that's the case. Should be fixed by https://bugs.webkit.org/attachment.cgi?id=51623 as it needs a CR after the didFinishDocumentLoadForFrame
Comment on attachment 51623 [details] Add a CR after printing didFinishDocumentLoadForFrame Clearing flags on attachment: 51623 Committed r56674: <http://trac.webkit.org/changeset/56674>
Comment on attachment 51624 [details] Print didStartProvisionalLoadForFrame in the callback of notify::load-status property change notification Clearing flags on attachment: 51624 Committed r56679: <http://trac.webkit.org/changeset/56679>
Comment on attachment 51625 [details] Print didCommitLoadForFrame in the callback of signal::load-committed Clearing flags on attachment: 51625 Committed r56680: <http://trac.webkit.org/changeset/56680>
Comment on attachment 51626 [details] Added "onload-event" signal to WebKitWebView. Fix several loading tests Clearing flags on attachment: 51626 Committed r56681: <http://trac.webkit.org/changeset/56681>
All reviewed patches have been landed. Closing bug.
These shoudl only be dumping when "dump loader callbacks" is enabled. This is causing many non-loader tests to fail: http://build.webkit.org/results/GTK%20Linux%2032-bit%20Release/r56693%20(10481)/accessibility/aria-activedescendant-crash-pretty-diff.html And should be rolled out unless someone with a Gtk build is willing to make a fix.
My apologies for not spotting the error in my review.
I am rolling out. Let's please test patches that alter DRT with all tests before putting up for review, and landing.
Thank you Gustavo. Sorry for the trouble.
(In reply to comment #32) > I am rolling out. Let's please test patches that alter DRT with all tests > before putting up for review, and landing. Ups, my fault. I'll upload new versions of the patches
Created attachment 51909 [details] Print didStartProvisionalLoadForFram in the callback of signal::load-committed This new version only prints the output when needed
Comment on attachment 51909 [details] Print didStartProvisionalLoadForFram in the callback of signal::load-committed Should this be keyed on database callbacks? 849 if (gLayoutTestController->dumpDatabaseCallbacks()) {
Comment on attachment 51909 [details] Print didStartProvisionalLoadForFram in the callback of signal::load-committed 849 if (gLayoutTestController->dumpDatabaseCallbacks()) { This is wrong, given we are talking about frame loader, not about databases here. And you should also unskip the tests that had to be skipped because of missing this functionality.
(In reply to comment #37) > (From update of attachment 51909 [details]) > 849 if (gLayoutTestController->dumpDatabaseCallbacks()) { > > This is wrong, given we are talking about frame loader, not about databases > here. And you should also unskip the tests that had to be skipped because of > missing this functionality. What a stupid mistake, should be dumpFrameLoaderCallbacks(). I'll upload the new version of the patch, and sorry for the noise.
Created attachment 52947 [details] Print didStartProvisionalLoadForFrame in the callback of load-committed signal One of the tests associated to this bug in Skipped file was not really related to this fix, so I just moved it to where the other loading tests are.
Comment on attachment 52947 [details] Print didStartProvisionalLoadForFrame in the callback of load-committed signal 849 if (gLayoutTestController->dumpFrameLoadCallbacks()) { 850 if (loadStatus == WEBKIT_LOAD_PROVISIONAL) { Doing early returns here would be more usual. I hope you have run all the tests with this change in? =)
Comment on attachment 52947 [details] Print didStartProvisionalLoadForFrame in the callback of load-committed signal This patch has been sitting in pending-commit for over a month. Hopefully it still works.
Comment on attachment 52947 [details] Print didStartProvisionalLoadForFrame in the callback of load-committed signal Rejecting patch 52947 from commit-queue. Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Gustavo Noronha Silva', u'--force']" exit_code: 1 Last 500 characters of output: ). patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/platform/gtk/Skipped Hunk #1 succeeded at 3313 (offset -103 lines). Hunk #2 FAILED at 5771. 1 out of 2 hunks FAILED -- saving rejects to file LayoutTests/platform/gtk/Skipped.rej patching file WebKitTools/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file WebKitTools/DumpRenderTree/gtk/DumpRenderTree.cpp Hunk #1 succeeded at 843 (offset 1 line). Hunk #2 succeeded at 884 (offset 1 line). Full output: http://webkit-commit-queue.appspot.com/results/2307110
Created attachment 56169 [details] Patch for landing
Comment on attachment 56169 [details] Patch for landing Clearing flags on attachment: 56169 Committed r59568: <http://trac.webkit.org/changeset/59568>