Bug 211735 – Need to advertise support for WebP in the Accept header

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

On Mac and iOS the system has to support WebP: HAVE(WEBP). On other platforms, WebKit has to be able to decode WebP images: USE(WEBP).

[Said Abou-Hallawa]

<rdar://problem/63029592>

[Said Abou-Hallawa]

Created attachment 399038 [details]
Patch

[Darin Adler]

Comment on attachment 399038 [details]
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?

[Darin Adler]

Comment on attachment 399038 [details]
Patch

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

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:131
> +#if HAVE(WEBP) || USE(WEBP)

This seems wrong. Why is it "||"? It should just be USE(WEBP).

>> Source/WebCore/loader/cache/CachedResourceRequest.cpp:138
>> +        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.

Here’s another way:

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

[Said Abou-Hallawa]

Created attachment 399079 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 399038 [details]
Patch

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

>> Source/WebCore/loader/cache/CachedResourceRequest.cpp:131
>> +#if HAVE(WEBP) || USE(WEBP)
> 
> This seems wrong. Why is it "||"? It should just be USE(WEBP).

HAVE(WEBP) is defined in PlatformHave.h as:

#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101600) || (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 140000)
#define HAVE_WEBP 1
#endif

But USE(WEBP) is defined in PlatformUse.h as:

#if PLATFORM(GTK) || PLATFORM(WPE)
#define USE_WEBP 1
#endif

So I think we need both. Or should I convert HAVE(WEPB) on macOS and iOS to be USE(WEBP) also?

>>> Source/WebCore/loader/cache/CachedResourceRequest.cpp:138
>>> +        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.
> 
> Here’s another way:
> 
> #if HAVE(WEBP) || USE(WEBP)
>         #define WEBP_HEADER_PART "image/webp,"
> #else
>         #define WEBP_HEADER_PART ""
> endif
>         if (ImageDecoder::supportsMediaType(ImageDecoder::MediaType::Video))
>             return WEBP_HEADER_PART "image/png,image/svg+xml,image/*;q=0.8,video/*;q=0.8,*/*;q=0.5"_s;
>         return WEBP_HEADER_PART "image/png,image/svg+xml,image/*;q=0.8,*/*;q=0.5"_s;
>         #undef WEBP_HEADER_PART

I moved this code to a new constexpr function which returns ASCIILiteral. I did not add the macro because I felt handling WebP and video images are different. I understand checking for supporting the WebP can be done at run time while checking for video images have to be don et run time. Please have a look at the new patch and let me know if I need to put the macros.

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

You right we should not do this way. I searched the code for "} case" and I found only one case in CSSCalculationValue.cpp and I fixed in the current patch.

[Darin Adler]

Comment on attachment 399038 [details]
Patch

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

>>> Source/WebCore/loader/cache/CachedResourceRequest.cpp:131
>>> +#if HAVE(WEBP) || USE(WEBP)
>> 
>> This seems wrong. Why is it "||"? It should just be USE(WEBP).
> 
> HAVE(WEBP) is defined in PlatformHave.h as:
> 
> #if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101600) || (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 140000)
> #define HAVE_WEBP 1
> #endif
> 
> But USE(WEBP) is defined in PlatformUse.h as:
> 
> #if PLATFORM(GTK) || PLATFORM(WPE)
> #define USE_WEBP 1
> #endif
> 
> So I think we need both. Or should I convert HAVE(WEPB) on macOS and iOS to be USE(WEBP) also?

USE(WEBP) needs to be true on macOS and iOS if we plan to use WebP. There may be some difficulty straightening things out; perhaps USE(WEBP) guards an implementation of WebP that we don’t use on macOS and iOS. But in the end, we can’t have these two with this strange relationship.

[Darin Adler]

Comment on attachment 399079 [details]
Patch

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

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:135
> +#if !(HAVE(WEBP) || USE(WEBP))
> +    if (supportsVideoImage)
> +        return "image/webp,image/png,image/svg+xml,image/*;q=0.8,video/*;q=0.8,*/*;q=0.5"_s;
> +    return "image/webp,image/png,image/svg+xml,image/*;q=0.8,*/*;q=0.5"_s;
> +#else
> +    if (supportsVideoImage)
> +        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;
> +#endif

This is OK. I don’t know why you chose not the use the macros, but it’s fine.

[Said Abou-Hallawa]

Created attachment 399096 [details]
Patch

[EWS]

Committed r261548: <https://trac.webkit.org/changeset/261548>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399096 [details].

[Darin Adler]

Comment on attachment 399096 [details]
Patch

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

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:125
> +static inline constexpr ASCIILiteral acceptHeaderValueForImageResource(bool supportsVideoImage)

Never need to write both constexpr and inline. Since constexpr implies inline for functions and static data members (it implies const for objects). Maybe we should teach this to the style checker?

[Don Olmstead]

*** Bug 185046 has been marked as a duplicate of this bug. ***