[webkit-reviews] review denied: [Bug 117533] [GTK] MiniBrowser to automatically download "non-showable" documents when left click in link : [Attachment 204425] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Andres Gomez Garcia
<agomez at igalia.com>'s request for review:
Bug 117533: [GTK] MiniBrowser to automatically download "non-showable"
documents when left click in link
https://bugs.webkit.org/show_bug.cgi?id=117533

Attachment 204425: Patch
https://bugs.webkit.org/attachment.cgi?id=204425&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=204425&action=review


Looks great, thanks! There are just a few minor nits.

> Tools/MiniBrowser/gtk/BrowserWindow.c:344
> +	   {

This brace should be in the previous line.

> Tools/MiniBrowser/gtk/BrowserWindow.c:347
> +		   ||
webkit_navigation_policy_decision_get_mouse_button(navigationDecision) != 2)

Now that we depend on recent GTK+ we can use GDK_BUTTON_MIDDLE instead of 2 :-)


> Tools/MiniBrowser/gtk/BrowserWindow.c:350
> +	       // Opening a new window if link clicked with the middle button

Nit: Comments should finish with a period.

> Tools/MiniBrowser/gtk/BrowserWindow.c:352
> +	       webkit_web_view_set_settings(newWebView,
webkit_web_view_get_settings(webView));

I think this is no longer needed, since both web views are in the same web view
group (the default one) and then sharing the settings already.

> Tools/MiniBrowser/gtk/BrowserWindow.c:361
> +	   {

This brace should be in the previous line.

> Tools/MiniBrowser/gtk/BrowserWindow.c:367
> +	       WebKitResponsePolicyDecision *responseDecision;
> +	       WebKitURIResponse *response;
> +	       WebKitURIRequest *request;
> +	       WebKitWebResource *mainResource;
> +	       const char *mimeType;
> +	       const char *requestURI;

You can declare the variables when used instead of having this block.

> Tools/MiniBrowser/gtk/BrowserWindow.c:379
> +	       if (g_strcmp0 (webkit_web_resource_get_uri(mainResource),
requestURI))

Extra space between function name and parentheses.