Bug 117533 – [GTK] MiniBrowser to automatically download "non-showable" documents when left click in link

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Andres Gomez Garcia]

Currently only way of saving to disk a link is through the context menu.

Non-showable/navigatable documents behind links lead to no action in MiniBrowser.

We would like to start automatically the download of such documents when left clicking it their link.

[Andres Gomez Garcia]

Created attachment 204425 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 204425 [details]
Patch

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

Overall LGTM

> Tools/ChangeLog:11
> +        itself on link left clicking.

The patch description normally goes just after the Reviewed by paragraph. These lines are normally used to comment about specific implementation stuff in some functions

> Tools/MiniBrowser/gtk/BrowserWindow.c:371
> +            mimeType = webkit_uri_response_get_mime_type(response);

It's normally preferred to initialize the values in the declarations so:

WebKitResponsePolicyDecision *responseDecision = WEBKIT_RESPONSE_POLICY_DECISION(decision);
WebKitURIResponse *response = webkit_response_policy_decision_get_response(responseDecision);

and the like

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

remove the blank after g_strcmp0

[Carlos Garcia Campos]

Comment on attachment 204425 [details]
Patch

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.

[Andres Gomez Garcia]

Created attachment 204428 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 204428 [details]
Patch

Perfect!

[Andres Gomez Garcia]

(In reply to comment #2)
> (From update of attachment 204425 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=204425&action=review
> 
> Overall LGTM

(In reply to comment #5)
> (From update of attachment 204428 [details])
> Perfect!

Thanks for the reviews, guys!

[WebKit Commit Bot]

Comment on attachment 204428 [details]
Patch

Clearing flags on attachment: 204428

Committed r151496: <http://trac.webkit.org/changeset/151496>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.