Bug 211735 - Need to advertise support for WebP in the Accept header
Summary: Need to advertise support for WebP in the Accept header
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
: 185046 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-05-11 11:41 PDT by Said Abou-Hallawa
Modified: 2020-10-21 08:27 PDT (History)
11 users (show)

See Also:


Attachments
Patch (2.11 KB, patch)
2020-05-11 11:54 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (4.66 KB, patch)
2020-05-11 17:59 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (4.59 KB, patch)
2020-05-11 21:40 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2020-05-11 11:41:08 PDT
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).
Comment 1 Said Abou-Hallawa 2020-05-11 11:41:40 PDT
<rdar://problem/63029592>
Comment 2 Said Abou-Hallawa 2020-05-11 11:54:40 PDT
Created attachment 399038 [details]
Patch
Comment 3 Darin Adler 2020-05-11 12:33:55 PDT
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?
Comment 4 Darin Adler 2020-05-11 12:36:12 PDT
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
Comment 5 Said Abou-Hallawa 2020-05-11 17:59:59 PDT
Created attachment 399079 [details]
Patch
Comment 6 Said Abou-Hallawa 2020-05-11 18:16:09 PDT
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.
Comment 7 Darin Adler 2020-05-11 20:34:55 PDT
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.
Comment 8 Darin Adler 2020-05-11 20:35:54 PDT
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.
Comment 9 Said Abou-Hallawa 2020-05-11 21:40:51 PDT
Created attachment 399096 [details]
Patch
Comment 10 EWS 2020-05-12 00:21:01 PDT
Committed r261548: <https://trac.webkit.org/changeset/261548>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 399096 [details].
Comment 11 Darin Adler 2020-05-12 10:28:33 PDT
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?
Comment 12 Don Olmstead 2020-10-21 08:27:18 PDT
*** Bug 185046 has been marked as a duplicate of this bug. ***