Summary: | [GTK] Add API to WebKitResponsePolicyDecision to check if the MIME type can be shown | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||
Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | cdumez, commit-queue, gustavo, gyuyoung.kim, mrobinson, rakuco | ||||
Priority: | P2 | Keywords: | Gtk | ||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Carlos Garcia Campos
2013-11-20 03:44:06 PST
Created attachment 217417 [details]
Patch
I'm not sure about the name though, I'm using webkit_response_policy_decision_is_mime_type_supported() but I'm not totally convinced. Any other suggestion for the function name?
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API Comment on attachment 217417 [details]
Patch
Hrm. Wouldn't webkit_response_policy_decision_can_show_mime_type make sense since we already have webkit_web_view_can_show_mime_type?
(In reply to comment #3) > (From update of attachment 217417 [details]) > Hrm. Wouldn't webkit_response_policy_decision_can_show_mime_type make sense since we already have webkit_web_view_can_show_mime_type? That was my first idea, but sounded a bit weird to me, webkit_web_view_can_show_mime_type makes sense because a WebView can show content, but a policy decision doesn't. What we want to know is whether the web view that triggered this policy decision request can show the mime type of the response. Comment on attachment 217417 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=217417&action=review > Source/WebKit2/UIProcess/API/gtk/WebKitResponsePolicyDecision.cpp:142 > + * webkit_response_policy_decision_is_mime_type_supported: I like the name in principle. If we want to be very accurate about it, though, we could have webkit_response_policy_decision_can_web_view_show_mime_type(). I'm OK with is_mime_type_supported(), though, if you guys don't like that one ;) I agree with Carlos that can_show_mime_type makes a bit less sense here than in WebView. > Source/WebKit2/UIProcess/API/gtk/WebKitResponsePolicyDecision.cpp:147 > + * that triggered this policy decision request. > + * See also webkit_web_view_can_show_mime_type(). Nit: these could be on the same line. (In reply to comment #5) > (From update of attachment 217417 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=217417&action=review > > > Source/WebKit2/UIProcess/API/gtk/WebKitResponsePolicyDecision.cpp:142 > > + * webkit_response_policy_decision_is_mime_type_supported: > > I like the name in principle. If we want to be very accurate about it, though, we could have webkit_response_policy_decision_can_web_view_show_mime_type(). I'm OK with is_mime_type_supported(), though, if you guys don't like that one ;) I agree with Carlos that can_show_mime_type makes a bit less sense here than in WebView. I'm fine with is_mime_type_supported() too, Martin? > > Source/WebKit2/UIProcess/API/gtk/WebKitResponsePolicyDecision.cpp:147 > > + * that triggered this policy decision request. > > + * See also webkit_web_view_can_show_mime_type(). > > Nit: these could be on the same line. Sure! (In reply to comment #6) > I'm fine with is_mime_type_supported() too, Martin? The name seems reasonable given the constraints Carlos mentioned. It's a little sloppy, in that there's a difference between a mime type being supported and showable. What I mean is that we support non-showable mime types by downloading them. I think my point is a nitpick though, so unless anyone can think of a better one, is_mime_type_supported should work just fine. The next best thing I can think of is is_mime_type_showable, but that feels a little bit tortured. so, if we agree on the name, could someone please review the patch? :-) Committed r161256: <http://trac.webkit.org/changeset/161256> |