Bug 36454 – [GTK] Improve reporting of frame loader callbacks in DRT

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Sergio Villar Senin]

Some tests under http/test/loading fail due to the lack of reporting in DRT

[Sergio Villar Senin]

Created attachment 51316 [details]
Print didFinishLoadForFrame outcome in DRT

[Sergio Villar Senin]

Created attachment 51317 [details]
Add a CR after printing didFinishDocumentLoadForFram

[Sergio Villar Senin]

Created attachment 51318 [details]
Print didStartProvisionalLoadForFrame in the callback of notify::load-status property change notification

[WebKit Review Bot]

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.

[Sergio Villar Senin]

Created attachment 51319 [details]
Print didCommitLoadForFrame in the callback of signal::load-committed

[Sergio Villar Senin]

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

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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.)

[Eric Seidel (no email)]

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?

[Eric Seidel (no email)]

Comment on attachment 51319 [details]
Print didCommitLoadForFrame in the callback of signal::load-committed

ChangeLog :(

[Sergio Villar Senin]

Created attachment 51622 [details]
Print didFinishLoadForFrame outcome in DRT

Improved Changelog

[Sergio Villar Senin]

Created attachment 51623 [details]
Add a CR after printing didFinishDocumentLoadForFrame

[Sergio Villar Senin]

Created attachment 51624 [details]
Print didStartProvisionalLoadForFrame in the callback of notify::load-status property change notification

[Sergio Villar Senin]

Created attachment 51625 [details]
Print didCommitLoadForFrame in the callback of signal::load-committed

[Sergio Villar Senin]

Created attachment 51626 [details]
Added "onload-event" signal to WebKitWebView. Fix several loading tests

[Sergio Villar Senin]

I sent small patches to make them easier to review. If committing small patches is a problem I can create a single one.

[Eric Seidel (no email)]

Comment on attachment 51622 [details]
Print didFinishLoadForFrame outcome in DRT

Yay!  Looking forward to the un-skipping!

[Eric Seidel (no email)]

Comment on attachment 51623 [details]
Add a CR after printing didFinishDocumentLoadForFrame

Yay!  Looking forward to the un-skipping!

[Eric Seidel (no email)]

Comment on attachment 51624 [details]
Print didStartProvisionalLoadForFrame in the callback of notify::load-status property change notification

Excellent.

[Eric Seidel (no email)]

Comment on attachment 51625 [details]
Print didCommitLoadForFrame in the callback of signal::load-committed

OK.

[Eric Seidel (no email)]

Comment on attachment 51626 [details]
Added "onload-event" signal to WebKitWebView. Fix several loading tests

OK.

[WebKit Commit Bot]

Comment on attachment 51622 [details]
Print didFinishLoadForFrame outcome in DRT

Clearing flags on attachment: 51622

Committed r56645: <http://trac.webkit.org/changeset/56645>

[Eric Seidel (no email)]

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?

[Sergio Villar Senin]

(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

[WebKit Commit Bot]

Comment on attachment 51623 [details]
Add a CR after printing didFinishDocumentLoadForFrame

Clearing flags on attachment: 51623

Committed r56674: <http://trac.webkit.org/changeset/56674>

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

My apologies for not spotting the error in my review.

[Gustavo Noronha (kov)]

I am rolling out. Let's please test patches that alter DRT with all tests before putting up for review, and landing.

[Eric Seidel (no email)]

Thank you Gustavo.  Sorry for the trouble.

[Sergio Villar Senin]

(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

[Sergio Villar Senin]

Created attachment 51909 [details]
Print didStartProvisionalLoadForFram in the callback of signal::load-committed

This new version only prints the output when needed

[Eric Seidel (no email)]

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()) {

[Gustavo Noronha (kov)]

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.

[Sergio Villar Senin]

(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.

[Sergio Villar Senin]

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.

[Gustavo Noronha (kov)]

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? =)

[Adam Barth]

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.

[WebKit Commit Bot]

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

[Adam Barth]

Created attachment 56169 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 56169 [details]
Patch for landing

Clearing flags on attachment: 56169

Committed r59568: <http://trac.webkit.org/changeset/59568>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.