Bug 113260 – Add a runtime flag in WebRuntimeFeatures to enable 'image/webp' accept header

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Urvang Joshi]

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.

[Urvang Joshi]

Created attachment 194972 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[James Robinson]

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.

[Urvang Joshi]

> 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.

[Darin Fisher (:fishd, Google)]

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.

[Urvang Joshi]

Created attachment 195009 [details]
Helper method to set accept header

[Urvang Joshi]

@Darin: Yes, I should have.
Pls have a look at the new patch.

[Mike West]

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.

[Darin Fisher (:fishd, Google)]

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

[Urvang Joshi]

Created attachment 195370 [details]
Patch

[Urvang Joshi]

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

[Urvang Joshi]

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.

[Darin Fisher (:fishd, Google)]

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

[Urvang Joshi]

Created attachment 195408 [details]
Patch

[Urvang Joshi]

@Reviewers: Gentle reminder to pls have another look.

[Urvang Joshi]

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