Bug 59367 - [GTK] DRT missing didRunInsecureContent notification
Summary: [GTK] DRT missing didRunInsecureContent notification
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-25 15:52 PDT by Philippe Normand
Modified: 2012-04-03 02:21 PDT (History)
6 users (show)

See Also:


Attachments
Patch (6.58 KB, patch)
2012-03-23 09:23 PDT, Simon Pena
no flags Details | Formatted Diff | Diff
Patch (10.30 KB, patch)
2012-04-02 10:28 PDT, Simon Pena
no flags Details | Formatted Diff | Diff
Patch (10.37 KB, patch)
2012-04-02 13:23 PDT, Simon Pena
no flags Details | Formatted Diff | Diff
Patch (10.33 KB, patch)
2012-04-03 01:04 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2011-04-25 15:52:36 PDT
Updated in r84739 http/tests/security/mixedContent/insecure-css-in-main-frame.html now fails in GTK:

--- /home/slave/webkitgtk/gtk-linux-64-debug/build/layout-test-results/http/tests/security/mixedContent/insecure-css-in-iframe-expected.txt	2011-04-25 15:09:25.717866125 -0700
+++ /home/slave/webkitgtk/gtk-linux-64-debug/build/layout-test-results/http/tests/security/mixedContent/insecure-css-in-iframe-actual.txt	2011-04-25 15:09:25.713911539 -0700
@@ -3,7 +3,6 @@
 frame "<!--framePath //<!--frame0-->-->" - didCommitLoadForFrame
 CONSOLE MESSAGE: line 1: The page at https://127.0.0.1:8443/security/mixedContent/resources/frame-with-insecure-css.html ran insecure content from http://127.0.0.1:8080/security/mixedContent/resources/style.css.
 
-didRunInsecureContent
 frame "<!--framePath //<!--frame0-->-->" - didFinishDocumentLoadForFrame
 frame "<!--framePath //<!--frame0-->-->" - didHandleOnloadEventsForFrame
 main frame - didHandleOnloadEventsForFrame

Will skip for now.
Comment 1 Simon Pena 2012-03-23 09:23:32 PDT
Created attachment 133501 [details]
Patch
Comment 2 Philippe Normand 2012-03-23 09:30:55 PDT
Comment on attachment 133501 [details]
Patch

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

Thanks! Looks good to me but we need +1 from another reviewer for the added signal.

> LayoutTests/platform/gtk/Skipped:616
> -http/tests/security/mixedContent/insecure-css-in-main-frame.html
>  http/tests/security/mixedContent/insecure-iframe-in-main-frame.html
>  http/tests/security/mixedContent/insecure-image-in-main-frame.html
>  http/tests/security/mixedContent/insecure-script-in-iframe.html

Maybe some more tests can be unskipped?
Comment 3 Philippe Normand 2012-03-23 09:32:25 PDT
CCing some more r flag power folks.
Comment 4 Philippe Normand 2012-03-23 09:33:16 PDT
Comment on attachment 133501 [details]
Patch

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

> Source/WebKit/gtk/webkit/webkitwebframe.cpp:443
> +     * Invoked when insecure content is run

Hum maybe a Since: tag is missing ?
Comment 5 Martin Robinson 2012-03-23 09:33:46 PDT
Comment on attachment 133501 [details]
Patch

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

Thanks for the contribution. I think the documentation could use some love and perhaps you could figure out the other insecure content tests aren't passing (if they aren't) and add a comment.

> Source/WebKit/gtk/webkit/webkitwebframe.cpp:443
> +     * Invoked when insecure content is run

This documentation could be improved quite a bit. For instance you could give examples of what type of insecure content trigger this signal.
Comment 6 Sergio Villar Senin 2012-03-23 09:47:24 PDT
Comment on attachment 133501 [details]
Patch

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

Great stuff! Hope you keep contributing :)

> Source/WebKit/gtk/ChangeLog:7
> +

You need a brief comment here describing the patch.

> Source/WebKit/gtk/webkit/webkitwebframe.cpp:116
> +    // Insecure content

don't think that comment is needed.

> Source/WebKit/gtk/webkit/webkitwebframe.cpp:450
> +            0,

IMO it's better just to write this as 0, 0, 0 on the same line

> Tools/ChangeLog:7
> +

Again you need a brief description of the change.

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1266
> +static void didRunInsecureContent(WebKitWebFrame* webFrame, WebKitSecurityOrigin* origin, CString url)

I guess you can omit the first two parameter names here.

> LayoutTests/ChangeLog:7
> +

Missing comment here too.
Comment 7 Simon Pena 2012-03-23 10:09:47 PDT
Thanks for your time. I'll address your comments, and will update the patch when ready.
Comment 8 Simon Pena 2012-04-02 10:28:14 PDT
Created attachment 135126 [details]
Patch
Comment 9 Simon Pena 2012-04-02 13:23:24 PDT
Created attachment 135165 [details]
Patch
Comment 10 Simon Pena 2012-04-02 13:24:31 PDT
Hopefully documentation in attachment 135165 [details] will be more clear :-)
Comment 11 Martin Robinson 2012-04-02 13:30:34 PDT
Comment on attachment 135165 [details]
Patch

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

This looks great to me! Since this adds new API we need the approval of one more GTK+ reviewer. I've CC'd some. One of them should be able to give the official r+.

I have just a couple nits below, but I think a committer could probably fix this up before landing without a problem.

> Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:661
> +    WebKitSecurityOrigin* securityOrigin = kit(coreOrigin);
> +    g_signal_emit_by_name(m_frame, "insecure-content-run", securityOrigin, url.string().utf8().data());

This could be one line.

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:102
> +static void didRunInsecureContent(WebKitWebFrame*, WebKitSecurityOrigin*, const char*);

You probably want to include the "url" name on the last parameter, const char* is a generic type.
Comment 12 Philippe Normand 2012-04-03 00:00:04 PDT
Comment on attachment 135165 [details]
Patch

+1 from me as well. I'll fixup the nits and land the patch, if it's ok with you, Simón.
Comment 13 Philippe Normand 2012-04-03 01:04:34 PDT
Created attachment 135287 [details]
Patch

Patch for landing
Comment 14 Simon Pena 2012-04-03 01:15:04 PDT
(In reply to comment #12)
> (From update of attachment 135165 [details])
> +1 from me as well. I'll fixup the nits and land the patch, if it's ok with you, Simón.

Sure! That's perfect :-)
Comment 15 WebKit Review Bot 2012-04-03 02:21:07 PDT
Comment on attachment 135287 [details]
Patch

Clearing flags on attachment: 135287

Committed r113001: <http://trac.webkit.org/changeset/113001>
Comment 16 WebKit Review Bot 2012-04-03 02:21:12 PDT
All reviewed patches have been landed.  Closing bug.