Bug 27787 – The type information about the resource loading is not forwarded to the network layer

• Component: WebKit API
• Status: REOPENED

[Zsombor]

It would be useful, for the network layer to know, not only the url, but also the type of the resource being requested. For example, for a 'Do not download images' feature, or an advanced adblocker filtering, etc.

[Zsombor]

Created attachment 33682 [details]
patch which resource type information to the ResourceRequestBase class

[Alexey Proskuryakov]

Please set review? flag if this patch is intended for review; see <http://webkit.org/coding/contributing.html>.

[Eric Seidel (no email)]

Comment on attachment 33682 [details]
patch which resource type information to the ResourceRequestBase class

Style:
 361     case CachedResource::ImageResource :

[Zsombor]

?

[Eric Seidel (no email)]

Extra space before the :.  Please run check-webkit-style if you have not already.

[Eric Seidel (no email)]

Comment on attachment 33682 [details]
patch which resource type information to the ResourceRequestBase class

r- for lack of ChangeLog.

Please run check-webkit-style.

I think this should just be an argument to the constructor, but I can see how that would be harry to add given the current design.

Please don't add a default here:
+    default : 
+        m_resourceType = GenericRequest;
+        break;
it defeats the point of using a switch for an enum (which is to catch when something is added to the enum).

Never used, and not needed:
+        void setResourceType(RequestType type) { m_resourceType = type; }

Hum... but Platform can never depend on WebCore. So this is a layering violation.

I suggest that instead you make CachedResource use these constants.  And then CachedResource can cast its own constancts back to these for setting.

[Zsombor]

Created attachment 34437 [details]
new version of the patch

I'm hopefull, that this patch is better suited for incorporation.

[Zsombor]

Created attachment 34879 [details]
new version of the patch, with changelog

[Eric Seidel (no email)]

Comment on attachment 34879 [details]
new version of the patch, with changelog

In general this looks good!

Please remove this:
42     default:
 243          return Other;
and put a "return Other" after the switch.  See my previous comment about default: cases for switches over enums as being bad. :)

We could avoid all this search/replace by adding a typedef ResourceRequestBase::Type Type; into CachedResponse.  I'm not sure I really care one way or the other, but it would definitely make the patch smaller.

Style:
 45     case ResourceRequestBase::ImageResource :
(no space after value before ':')

Otherwise looks good.

[Zsombor]

Created attachment 39527 [details]
patch implementing the base functionality

I don't want to mess too much with the switch-cases - there is already some 'default:' branch in the touched code, so this is way I haven't originaly  modified it. I've removed the qt - specific translation code, because it's unknown when ResourceRequestType enum value is added to QtNetworkRequest.

[Adam Barth]

Comment on attachment 39527 [details]
patch implementing the base functionality

This is a layering violation.  The network layer shouldn't know what an image or a style sheet is.  If you want to implement a "block all images feature," you should use a client callback.

[Zsombor]

Can you explain, what do you mean, when you say 'client callback' ? I would like to have a proper adblocker, which is compatible with the de facto standard firefox extension. For this it needs to discriminate between several type of network request, for example 'image','stylesheet','dtd','xmlhttprequest','object', etc. Please check the documentation at : http://adblockplus.org/en/filters in the 'Specifying filter options' section.

[Eric Seidel (no email)]

The *Client.h files in WebCore.  WebCore asks WebKit to make network requests.

[Eric Seidel (no email)]

Bug 32167 is very similar.