[webkit-reviews] review granted: [Bug 104578] [GTK][WK2] New API to detect display/execution of insecure content : [Attachment 178754] Patch proposal + Unit tests

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Dec 12 02:37:59 PST 2012


Martin Robinson <mrobinson at webkit.org> has granted Mario Sanchez Prada
<mario at webkit.org>'s request for review:
Bug 104578: [GTK][WK2] New API to detect display/execution of insecure content
https://bugs.webkit.org/show_bug.cgi?id=104578

Attachment 178754: Patch proposal + Unit tests
https://bugs.webkit.org/attachment.cgi?id=178754&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=178754&action=review


Looks good, as long as cgarcia is okay with the fact that
WEBKIT_INSECURE_CONTENT_RUN is not called WEBKIT_INSECURE_CONTENT_EVENT_RUN and
WEBKIT_INSECURE_CONTENT_DISPLAYED is not called
WEBKIT_INSECURE_CONTENT_EVENT_DISPLAYED. I don't think they are too long, but
it's not a huge deal.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1245
> +	* This signal is emitted when insecure content has been detected
> +	* in a page loaded through a secure connection. This typically
> +	* means that an external resource from an unstrusted source
> +	* (e.g. non HTTPS) has been run or displayed.
> +	*

You might want to try to more clearly describe the situation as "mixing of
HTTPS and non-HTTPS content"

> Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:92
> +    static void insecure_content_detected_callback(WebKitWebView* webView,
WebKitInsecureContentEvent event, InsecureContentTest* test)

Please use WebKit naming conventions here.

> Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:148
> +	   char* contents;
> +	   gsize length;
> +
> +	   g_file_get_contents(pathToFile.get(), &contents, &length, 0);

Okay. Super nit, but wouldn't it make more sense to "glue" the declarations
with the statement that uses them?

> Source/WebKit2/UIProcess/API/gtk/tests/TestSSL.cpp:157
> +	   gsize length;
> +
> +	   g_file_get_contents(pathToFile.get(), &contents, &length, 0);

Same here.

> Tools/Scripts/webkitpy/style/checker.py:184
> +	 "-readability/enum_casing",
>	 "-whitespace/parens"]),
> +
> +    ([# The GTK+ API use upper case, underscore separated, words in
> +	 # certain types of enums (e.g. signals, properties).
> +	 "Source/WebKit2/UIProcess/API/gtk"],
> +	["-readability/enum_casing"]),

I think maybe this is a bit too broad. We want to detect enum style violations
in these files too. Perhaps just turn webkitWebViewInsecureContentDetected into
webkitWebViewInsecureContentRun and webkitWebViewInsecureContentDisplayed.


More information about the webkit-reviews mailing list