Summary: | Add a runtime flag in WebRuntimeFeatures to enable 'image/webp' accept header | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Urvang Joshi <urvang> | ||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED WONTFIX | ||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, fishd, jamesr, japhet, tkent+wkapi, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Urvang Joshi
2013-03-25 18:32:16 PDT
Created attachment 194972 [details]
Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI. Comment on attachment 194972 [details]
Patch
Hmm, what does this have to do with WebKit? I thought HTTP headers like 'accept' are set by the network stack on the chromium side.
> Hmm, what does this have to do with WebKit? I thought HTTP headers like 'accept' are set by the network stack on the chromium side.
It's not so, it seems.
All the accept headers for cached resources are set by source files in Source/WebCore/loader/cache/*.[h,cpp]. Chromium only puts default headers for HTML page itself, and for some resources where WebKit doesn't put any accept header, AFAIK.
jamesr: Take a look at CachedCSSStyleSheet.cpp, CachedSVGDocument.cpp, and CachedXSLStyleSheet.cpp, which all call setAccept. urvang: It seems like you shouldn't duplicate the code that sets the accept header. A helper method seems appropriate. Created attachment 195009 [details]
Helper method to set accept header
@Darin: Yes, I should have. Pls have a look at the new patch. Comment on attachment 195009 [details] Helper method to set accept header View in context: https://bugs.webkit.org/attachment.cgi?id=195009&action=review Two drive-by comments: > Source/WebCore/loader/cache/CachedImage.h:102 > + void setCustomAccept(); // Helper method to set the Accept header. Would you mind renaming this to something that makes it clear that we're setting the header? I think 'CachedImage::setCustomAcceptHeader' would be clear enough that you could remove the comment. > Source/WebKit/chromium/src/WebRuntimeFeatures.cpp:765 > + UNUSED_PARAM(enable); Copy/paste error: There's no 'enable' in this method. Comment on attachment 195009 [details] Helper method to set accept header View in context: https://bugs.webkit.org/attachment.cgi?id=195009&action=review >> Source/WebCore/loader/cache/CachedImage.h:102 >> + void setCustomAccept(); // Helper method to set the Accept header. > > Would you mind renaming this to something that makes it clear that we're setting the header? I think 'CachedImage::setCustomAcceptHeader' would be clear enough that you could remove the comment. +1 Created attachment 195370 [details]
Patch
Comment on attachment 195009 [details] Helper method to set accept header View in context: https://bugs.webkit.org/attachment.cgi?id=195009&action=review >>> Source/WebCore/loader/cache/CachedImage.h:102 >>> + void setCustomAccept(); // Helper method to set the Accept header. >> >> Would you mind renaming this to something that makes it clear that we're setting the header? I think 'CachedImage::setCustomAcceptHeader' would be clear enough that you could remove the comment. > > +1 Done Comment on attachment 195009 [details] Helper method to set accept header View in context: https://bugs.webkit.org/attachment.cgi?id=195009&action=review >> Source/WebKit/chromium/src/WebRuntimeFeatures.cpp:765 >> + UNUSED_PARAM(enable); > > Copy/paste error: There's no 'enable' in this method. Oops, fixed. Comment on attachment 195370 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195370&action=review > Source/WebCore/ChangeLog:8 > + No new tests (OOPS!). nit: you need to delete this line Created attachment 195408 [details]
Patch
@Reviewers: Gentle reminder to pls have another look. Marking this as WONTFIX as it's Chromium specific, and would be taken care of in Blink repo now. Being tracked here: https://code.google.com/p/chromium/issues/detail?id=169182 |