Bug 66243

Summary: [GTK] Change webview API tests to use "load-status" signal instead of "load-progress"
Product: WebKit Reporter: Caio Marcelo de Oliveira Filho <cmarcelo>
Component: New BugsAssignee: Caio Marcelo de Oliveira Filho <cmarcelo>
Status: RESOLVED FIXED    
Severity: Normal CC: mrobinson, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 28851    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch mrobinson: review+

Description Caio Marcelo de Oliveira Filho 2011-08-15 11:37:59 PDT
[GTK] Change webview API tests to use "load-status" signal instead of "load-progress"
Comment 1 Caio Marcelo de Oliveira Filho 2011-08-15 11:43:31 PDT
Created attachment 103935 [details]
Patch
Comment 2 Martin Robinson 2011-08-17 15:11:32 PDT
Comment on attachment 103935 [details]
Patch

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

> Source/WebKit/gtk/ChangeLog:10
> +        The idle_quit_loop_cb() already checks the "load-status" flag, so it should be
> +        enough to call it when the "load-status" change.
> +

I'm not sure I understand why this patch is an improvement. Perhaps you could expand a bit here.  It doesn't seem to remove any code. Does it make things more efficient or more correct somehow?

> Source/WebKit/gtk/tests/testwebview.c:170
> +    g_signal_connect(view, "notify::load-status", G_CALLBACK (idle_quit_loop_cb), NULL);

Do you mind also fixing the style issue here?  G_CALLBACK (idle_quit_loop_cb)  G_CALLBACK(idle_quit_loop_cb)
Comment 3 Caio Marcelo de Oliveira Filho 2011-08-18 07:24:36 PDT
Created attachment 104338 [details]
Patch
Comment 4 Caio Marcelo de Oliveira Filho 2011-08-18 07:31:30 PDT
Thanks for the review Martin.

(In reply to comment #2)
> I'm not sure I understand why this patch is an improvement. Perhaps you could expand a bit here.  It doesn't seem to remove any code. Does it make things more efficient or more correct somehow?

Improved with an explanation. It is related to bug 28851. Since it is using progress to check whether its load-status changed, I think it is also a good change to do even if bug 28851 doesn't land.

> Do you mind also fixing the style issue here?  G_CALLBACK (idle_quit_loop_cb)  G_CALLBACK(idle_quit_loop_cb)

Done.
Comment 5 Martin Robinson 2011-08-18 07:51:01 PDT
Comment on attachment 104338 [details]
Patch

Thank you!
Comment 6 Caio Marcelo de Oliveira Filho 2011-08-18 08:52:01 PDT
Committed r93311: <http://trac.webkit.org/changeset/93311>