Bug 48048 - [GTK] WebKitWebFrame's load-status is not properly notified to the tests
Summary: [GTK] WebKitWebFrame's load-status is not properly notified to the tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P3 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-21 01:57 PDT by Sergio Villar Senin
Modified: 2010-10-28 11:43 PDT (History)
5 users (show)

See Also:


Attachments
New provisional-load-fail status signal. (5.31 KB, patch)
2010-10-21 03:42 PDT, Sergio Villar Senin
mrobinson: review-
Details | Formatted Diff | Diff
DRT fixes and refactorings (7.55 KB, patch)
2010-10-21 03:44 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
DRT fixes and refactorings (7.50 KB, patch)
2010-10-22 12:43 PDT, Sergio Villar Senin
mrobinson: review-
Details | Formatted Diff | Diff
Fix frame-created signal emission (1.94 KB, patch)
2010-10-27 04:41 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
load-status refactoring and fix (5.72 KB, patch)
2010-10-27 04:43 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Fix frame-created issuing. load-status refactoring and fixes (8.85 KB, patch)
2010-10-28 01:05 PDT, Sergio Villar Senin
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-10-21 01:57:49 PDT
There are several tests that do not show the expected results because the load status changes of inner frames are not properly handled in DRT. We're only currently notifying about the load status changes for the WebView, i.e, the main frame.
Comment 1 Sergio Villar Senin 2010-10-21 03:42:51 PDT
Created attachment 71415 [details]
New provisional-load-fail status signal.

Added a new signal to be issued when provisional load fails

"frame-created" signal is also now issued before loading any data. Thus, any client will receive all the load related notifications properly
Comment 2 Sergio Villar Senin 2010-10-21 03:44:21 PDT
Created attachment 71417 [details]
DRT fixes and refactorings

DRT now tracks the load-status signals of all the and not only the main frame.

Load status signal callbacks were also refactored in a single method.
Comment 3 Martin Robinson 2010-10-21 09:27:00 PDT
Comment on attachment 71415 [details]
New provisional-load-fail status signal.

View in context: https://bugs.webkit.org/attachment.cgi?id=71415&action=review

Looks good, though we need another nod from a GTK+ review for the API change.

> WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:596
> +

I think the extra line can be removed.

> WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:1136
> +    notifyStatus(m_frame, WEBKIT_LOAD_PROVISIONAL_FAILED);
> +

Same here.

> WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:1146
> +    if (m_loadingErrorPage)
> +        return;
> +
> +    notifyStatus(m_frame, WEBKIT_LOAD_FAILED);
> +

And here.
Comment 4 Martin Robinson 2010-10-21 09:30:39 PDT
Comment on attachment 71417 [details]
DRT fixes and refactorings

View in context: https://bugs.webkit.org/attachment.cgi?id=71417&action=review

> WebKitTools/DumpRenderTree/gtk/DumpRenderTree.cpp:1083
> +    // frame-created is not issued for main frame. That's why we must do this here
> +    g_signal_connect(mainFrame, "notify::load-status", G_CALLBACK(webFrameLoadStatusNotified), NULL);
>  

Is it possible to do this in createWebView?
Comment 5 Sergio Villar Senin 2010-10-21 09:37:42 PDT
Comment on attachment 71417 [details]
DRT fixes and refactorings

Indeed, we can better do that in the object_connect already present there
Comment 6 Sergio Villar Senin 2010-10-22 12:43:29 PDT
Created attachment 71584 [details]
DRT fixes and refactorings

moved signal connection to createWebView
Comment 7 Martin Robinson 2010-10-26 12:32:24 PDT
Comment on attachment 71584 [details]
DRT fixes and refactorings

Great!
Comment 8 Martin Robinson 2010-10-26 12:33:09 PDT
Comment on attachment 71584 [details]
DRT fixes and refactorings

Clearing commit-queue flag for now.
Comment 9 Sergio Villar Senin 2010-10-27 04:41:47 PDT
Created attachment 72006 [details]
Fix frame-created signal emission

frame-created must be emitted before loading any data on the frame.
Comment 10 Sergio Villar Senin 2010-10-27 04:43:15 PDT
Created attachment 72007 [details]
load-status refactoring and fix

load status changes are now handled in a single callback

With this patch we're also handling load-status signals for every frame created and not only the main one
Comment 11 Martin Robinson 2010-10-27 10:09:29 PDT
Comment on attachment 71415 [details]
New provisional-load-fail status signal.

Flipping the bit on this one, because of the API break.
Comment 12 Martin Robinson 2010-10-27 10:10:42 PDT
Xan pointed out smartly that adding a new load state breaks the API, which is precisely why I think we should move away from this in our API. Every time someone adds a new frame loader delegate method we cannot support it without breaking the API.
Comment 13 Martin Robinson 2010-10-27 11:16:16 PDT
Comment on attachment 72006 [details]
Fix frame-created signal emission

As discussed via Jabber, I believe this change should be combined into one patch.
Comment 14 Sergio Villar Senin 2010-10-28 01:05:25 PDT
Created attachment 72156 [details]
Fix frame-created issuing. load-status refactoring and fixes
Comment 15 Martin Robinson 2010-10-28 09:25:43 PDT
Comment on attachment 72156 [details]
Fix frame-created issuing. load-status refactoring and fixes

Thanks!
Comment 16 WebKit Commit Bot 2010-10-28 10:35:27 PDT
The commit-queue encountered the following flaky tests while processing attachment 72156 [details]:

http/tests/appcache/insert-html-element-with-manifest-2.html

Please file bugs against the tests.  These tests were authored by ap@webkit.org.  The commit-queue is continuing to process your patch.
Comment 17 WebKit Commit Bot 2010-10-28 10:37:34 PDT
Comment on attachment 72156 [details]
Fix frame-created issuing. load-status refactoring and fixes

Clearing flags on attachment: 72156

Committed r70788: <http://trac.webkit.org/changeset/70788>
Comment 18 WebKit Commit Bot 2010-10-28 10:37:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 WebKit Review Bot 2010-10-28 11:43:19 PDT
http://trac.webkit.org/changeset/70788 might have broken GTK Linux 32-bit Release
The following tests are not passing:
inspector/audits-panel-functional.html