Bug 117533 - [GTK] MiniBrowser to automatically download "non-showable" documents when left click in link
Summary: [GTK] MiniBrowser to automatically download "non-showable" documents when lef...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-06-12 02:08 PDT by Andres Gomez Garcia
Modified: 2013-06-12 06:39 PDT (History)
4 users (show)

See Also:


Attachments
Patch (4.62 KB, patch)
2013-06-12 04:48 PDT, Andres Gomez Garcia
no flags Details | Formatted Diff | Diff
Patch (4.36 KB, patch)
2013-06-12 05:58 PDT, Andres Gomez Garcia
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andres Gomez Garcia 2013-06-12 02:08:11 PDT
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.
Comment 1 Andres Gomez Garcia 2013-06-12 04:48:29 PDT
Created attachment 204425 [details]
Patch
Comment 2 Sergio Villar Senin 2013-06-12 04:58:18 PDT
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
Comment 3 Carlos Garcia Campos 2013-06-12 05:05:08 PDT
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.
Comment 4 Andres Gomez Garcia 2013-06-12 05:58:00 PDT
Created attachment 204428 [details]
Patch
Comment 5 Carlos Garcia Campos 2013-06-12 06:12:24 PDT
Comment on attachment 204428 [details]
Patch

Perfect!
Comment 6 Andres Gomez Garcia 2013-06-12 06:18:25 PDT
(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!
Comment 7 WebKit Commit Bot 2013-06-12 06:39:31 PDT
Comment on attachment 204428 [details]
Patch

Clearing flags on attachment: 204428

Committed r151496: <http://trac.webkit.org/changeset/151496>
Comment 8 WebKit Commit Bot 2013-06-12 06:39:33 PDT
All reviewed patches have been landed.  Closing bug.