This flag would be off by default. The main idea is to enable Website owners to test their site with these accept header change, before we turn it on by default (later). See the context here: https://code.google.com/p/chromium/issues/detail?id=169182 Also, this flag is meaningful only when the browser supports WebP (denoted by 'USE(WEBP)') -- only Chromium for now. Note: This change is from Webkit side. One more change from chromium side would actually enable/disable this based on command-line options.
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