Bug 36454 - [GTK] Improve reporting of frame loader callbacks in DRT
Summary: [GTK] Improve reporting of frame loader callbacks in DRT
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-22 11:07 PDT by Sergio Villar Senin
Modified: 2010-05-16 00:44 PDT (History)
4 users (show)

See Also:


Attachments
Print didFinishLoadForFrame outcome in DRT (1.68 KB, patch)
2010-03-22 11:36 PDT, Sergio Villar Senin
eric: review-
Details | Formatted Diff | Diff
Add a CR after printing didFinishDocumentLoadForFram (1.79 KB, patch)
2010-03-22 11:37 PDT, Sergio Villar Senin
eric: review-
Details | Formatted Diff | Diff
Print didStartProvisionalLoadForFrame in the callback of notify::load-status property change notification (2.63 KB, patch)
2010-03-22 11:38 PDT, Sergio Villar Senin
eric: review-
Details | Formatted Diff | Diff
Print didCommitLoadForFrame in the callback of signal::load-committed (2.53 KB, patch)
2010-03-22 11:41 PDT, Sergio Villar Senin
eric: review-
Details | Formatted Diff | Diff
Added "onload-event" signal to WebKitWebView. Fix several loading tests (7.40 KB, patch)
2010-03-22 11:42 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Print didFinishLoadForFrame outcome in DRT (1.75 KB, patch)
2010-03-25 04:08 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Add a CR after printing didFinishDocumentLoadForFrame (1.87 KB, patch)
2010-03-25 04:09 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Print didStartProvisionalLoadForFrame in the callback of notify::load-status property change notification (2.78 KB, patch)
2010-03-25 04:09 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Print didCommitLoadForFrame in the callback of signal::load-committed (2.62 KB, patch)
2010-03-25 04:10 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Added "onload-event" signal to WebKitWebView. Fix several loading tests (7.43 KB, patch)
2010-03-25 04:10 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Print didStartProvisionalLoadForFram in the callback of signal::load-committed (2.53 KB, patch)
2010-03-29 07:57 PDT, Sergio Villar Senin
gustavo: review-
Details | Formatted Diff | Diff
Print didStartProvisionalLoadForFrame in the callback of load-committed signal (4.44 KB, patch)
2010-04-09 00:59 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch for landing (4.11 KB, patch)
2010-05-15 18:06 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergio Villar Senin 2010-03-22 11:07:36 PDT
Some tests under http/test/loading fail due to the lack of reporting in DRT
Comment 1 Sergio Villar Senin 2010-03-22 11:36:35 PDT
Created attachment 51316 [details]
Print didFinishLoadForFrame outcome in DRT
Comment 2 Sergio Villar Senin 2010-03-22 11:37:37 PDT
Created attachment 51317 [details]
Add a CR after printing didFinishDocumentLoadForFram
Comment 3 Sergio Villar Senin 2010-03-22 11:38:27 PDT
Created attachment 51318 [details]
Print didStartProvisionalLoadForFrame in the callback of notify::load-status property change notification
Comment 4 WebKit Review Bot 2010-03-22 11:39:41 PDT
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.
Comment 5 Sergio Villar Senin 2010-03-22 11:41:30 PDT
Created attachment 51319 [details]
Print didCommitLoadForFrame in the callback of signal::load-committed
Comment 6 Sergio Villar Senin 2010-03-22 11:42:21 PDT
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 7 Eric Seidel (no email) 2010-03-25 01:52:40 PDT
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 8 Eric Seidel (no email) 2010-03-25 01:53:33 PDT
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 9 Eric Seidel (no email) 2010-03-25 01:54:17 PDT
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 10 Eric Seidel (no email) 2010-03-25 01:54:36 PDT
Comment on attachment 51319 [details]
Print didCommitLoadForFrame in the callback of signal::load-committed

ChangeLog :(
Comment 11 Sergio Villar Senin 2010-03-25 04:08:37 PDT
Created attachment 51622 [details]
Print didFinishLoadForFrame outcome in DRT

Improved Changelog
Comment 12 Sergio Villar Senin 2010-03-25 04:09:08 PDT
Created attachment 51623 [details]
Add a CR after printing didFinishDocumentLoadForFrame
Comment 13 Sergio Villar Senin 2010-03-25 04:09:37 PDT
Created attachment 51624 [details]
Print didStartProvisionalLoadForFrame in the callback of notify::load-status property change notification
Comment 14 Sergio Villar Senin 2010-03-25 04:10:16 PDT
Created attachment 51625 [details]
Print didCommitLoadForFrame in the callback of signal::load-committed
Comment 15 Sergio Villar Senin 2010-03-25 04:10:42 PDT
Created attachment 51626 [details]
Added "onload-event" signal to WebKitWebView. Fix several loading tests
Comment 16 Sergio Villar Senin 2010-03-25 04:11:58 PDT
I sent small patches to make them easier to review. If committing small patches is a problem I can create a single one.
Comment 17 Eric Seidel (no email) 2010-03-26 13:29:42 PDT
Comment on attachment 51622 [details]
Print didFinishLoadForFrame outcome in DRT

Yay!  Looking forward to the un-skipping!
Comment 18 Eric Seidel (no email) 2010-03-26 13:29:57 PDT
Comment on attachment 51623 [details]
Add a CR after printing didFinishDocumentLoadForFrame

Yay!  Looking forward to the un-skipping!
Comment 19 Eric Seidel (no email) 2010-03-26 13:30:38 PDT
Comment on attachment 51624 [details]
Print didStartProvisionalLoadForFrame in the callback of notify::load-status property change notification

Excellent.
Comment 20 Eric Seidel (no email) 2010-03-26 13:31:00 PDT
Comment on attachment 51625 [details]
Print didCommitLoadForFrame in the callback of signal::load-committed

OK.
Comment 21 Eric Seidel (no email) 2010-03-26 13:31:46 PDT
Comment on attachment 51626 [details]
Added "onload-event" signal to WebKitWebView. Fix several loading tests

OK.
Comment 22 WebKit Commit Bot 2010-03-26 15:16:18 PDT
Comment on attachment 51622 [details]
Print didFinishLoadForFrame outcome in DRT

Clearing flags on attachment: 51622

Committed r56645: <http://trac.webkit.org/changeset/56645>
Comment 23 Eric Seidel (no email) 2010-03-26 15:39:29 PDT
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?
Comment 24 Sergio Villar Senin 2010-03-27 03:26:25 PDT
(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 25 WebKit Commit Bot 2010-03-27 18:07:38 PDT
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 26 WebKit Commit Bot 2010-03-27 22:34:37 PDT
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 27 WebKit Commit Bot 2010-03-27 22:52:03 PDT
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 28 WebKit Commit Bot 2010-03-27 23:09:32 PDT
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>
Comment 29 WebKit Commit Bot 2010-03-27 23:09:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 30 Eric Seidel (no email) 2010-03-28 09:14:10 PDT
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.
Comment 31 Eric Seidel (no email) 2010-03-28 09:15:05 PDT
My apologies for not spotting the error in my review.
Comment 32 Gustavo Noronha (kov) 2010-03-28 15:31:24 PDT
I am rolling out. Let's please test patches that alter DRT with all tests before putting up for review, and landing.
Comment 33 Eric Seidel (no email) 2010-03-28 15:50:20 PDT
Thank you Gustavo.  Sorry for the trouble.
Comment 34 Sergio Villar Senin 2010-03-29 01:33:39 PDT
(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
Comment 35 Sergio Villar Senin 2010-03-29 07:57:34 PDT
Created attachment 51909 [details]
Print didStartProvisionalLoadForFram in the callback of signal::load-committed

This new version only prints the output when needed
Comment 36 Eric Seidel (no email) 2010-04-01 17:40:22 PDT
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 37 Gustavo Noronha (kov) 2010-04-08 06:24:08 PDT
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.
Comment 38 Sergio Villar Senin 2010-04-08 06:44:14 PDT
(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.
Comment 39 Sergio Villar Senin 2010-04-09 00:59:09 PDT
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 40 Gustavo Noronha (kov) 2010-04-09 05:43:45 PDT
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 41 Adam Barth 2010-05-14 23:55:23 PDT
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 42 WebKit Commit Bot 2010-05-15 10:12:48 PDT
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
Comment 43 Adam Barth 2010-05-15 18:06:27 PDT
Created attachment 56169 [details]
Patch for landing
Comment 44 WebKit Commit Bot 2010-05-16 00:44:45 PDT
Comment on attachment 56169 [details]
Patch for landing

Clearing flags on attachment: 56169

Committed r59568: <http://trac.webkit.org/changeset/59568>
Comment 45 WebKit Commit Bot 2010-05-16 00:44:54 PDT
All reviewed patches have been landed.  Closing bug.