[webkit-reviews] review granted: [Bug 211735] Need to advertise support for WebP in the Accept header : [Attachment 399038] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 211735: Need to advertise support for WebP in the Accept header
https://bugs.webkit.org/show_bug.cgi?id=211735

Attachment 399038: Patch

https://bugs.webkit.org/attachment.cgi?id=399038&action=review




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 399038
  --> https://bugs.webkit.org/attachment.cgi?id=399038
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=399038&action=review

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:138
> +#if HAVE(WEBP) || USE(WEBP)
> +	   String imageHeader = "image/webp,image/apng,image/svg+xml"_s;
> +#else
> +	   String imageHeader = "image/apng,image/svg+xml"_s;
> +#endif
>	   if (ImageDecoder::supportsMediaType(ImageDecoder::MediaType::Video))
> -	       return
"image/png,image/svg+xml,image/*;q=0.8,video/*;q=0.8,*/*;q=0.5"_s;
> -	   return "image/png,image/svg+xml,image/*;q=0.8,*/*;q=0.5"_s;
> -    case CachedResource::Type::CSSStyleSheet:
> +	       imageHeader.append(",video/*;q=0.8"_s);
> +	   return imageHeader + ",image/*;q=0.8,*/*;q=0.5"_s;

Seems like this can all be done compile-time instead of concatenating strings
at runtime. With macros it would be:

#if HAVE(WEBP) || USE(WEBP)
	#define IMAGE_HEADER_START "image/webp,image/apng,image/svg+xml"
#else
	#define IMAGE_HEADER_START "image/apng,image/svg+xml"
#endif
	#define IMAGE_HEADER_END ",image/*;q=0.8,*/*;q=0.5"
	if (ImageDecoder::supportsMediaType(ImageDecoder::MediaType::Video))
	    return IMAGE_HEADER_START ",video/*;q=0.8" IMAGE_HEADER_END _s;
	return IMAGE_HEADER_START IMAGE_HEADER_END _s;
	#undef IMAGE_HEADER_START
	#undef IMAGE_HEADER_END

There may be more clever ways of doing this, but I hate to see us doing string
concatenation work at runtime unnecessarily.

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:139
> +    } case CachedResource::Type::CSSStyleSheet:

Coding style thought: I don’t think we should put the end brace and case on the
same line; doesn’t seem like the "else" rule should apply. Is this something
we’ve done before?