Bug 59367 – [GTK] DRT missing didRunInsecureContent notification

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Philippe Normand]

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.

[Simon Pena]

Created attachment 133501 [details]
Patch

[Philippe Normand]

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?

[Philippe Normand]

CCing some more r flag power folks.

[Philippe Normand]

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 ?

[Martin Robinson]

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.

[Sergio Villar Senin]

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.

[Simon Pena]

Thanks for your time. I'll address your comments, and will update the patch when ready.

[Simon Pena]

Created attachment 135126 [details]
Patch

[Simon Pena]

Created attachment 135165 [details]
Patch

[Simon Pena]

Hopefully documentation in attachment 135165 [details] will be more clear :-)

[Martin Robinson]

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.

[Philippe Normand]

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.

[Philippe Normand]

Created attachment 135287 [details]
Patch

Patch for landing

[Simon Pena]

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

[WebKit Review Bot]

Comment on attachment 135287 [details]
Patch

Clearing flags on attachment: 135287

Committed r113001: <http://trac.webkit.org/changeset/113001>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.