Bug 235461 – Improve FourCC to use more inlining, fix incorrect mix of WEBCORE_EXPORT on entire class with inline functions

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Darin Adler]

Improve FourCC to use more inlining, fix incorrect mix of WEBCORE_EXPORT on entire class with inline functions

[Darin Adler]

Created attachment 449712 [details]
Patch

[Darin Adler]

Created attachment 449741 [details]
Patch

[Darin Adler]

Created attachment 449758 [details]
Patch

[Darin Adler]

OK, after two false starts, this one is working and ready for review.

For FourCC itself, this makes the code easier to read, more elegant, reduces reliance on reinterpret_cast and assumptions that all platforms are little-endian, and even makes it a bit more efficient, although mostly in places where efficiency isn’t critical.

On the other hand, for the ISOBox classes, while this fixes the problem where the coding style checker says "never use inline functions in a class marked WEBCORE_EXPORT", it makes the code less elegant. The underlying cause is that because these seemingly-simple structures are instead a class hierarchy that descends from a polymorphic class with a virtual destructor. That pattern isn’t working all that well to keep the code simple, and at some point it might be worth exploring whether it’s really needed, and considering alternatives.

[Darin Adler]

Comment on attachment 449758 [details]
Patch

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

> Source/WebCore/platform/graphics/FourCC.h:34
> +    constexpr FourCC(uint32_t value) : value { value } { }

Would like to remove this so people don’t accidentally use it in cross-platform code instead of string literals, but sadly it’s too valuable for interoperating with Cocoa API that uses unsigned 32-bit ints for this, the type FourCharCode and such, so I was unable to delete it at this time.

> Source/WebCore/platform/graphics/FourCC.h:48
> +    value = data[0] << 24 | data[1] << 16 | data[2] << 8 | data[3];

Should use member construction syntax for this instead of assignment.

[Darin Adler]

Comment on attachment 449758 [details]
Patch

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

> Source/WebCore/platform/graphics/FourCC.cpp:44
> +std::array<char, 5> FourCC::string() const

We could make this inline and put it in the header. Also could make it constexpr.

[Sam Weinig]

Comment on attachment 449758 [details]
Patch

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

> Source/WebCore/platform/graphics/FourCC.cpp:36
> +        static_cast<char>(string[0]),

Are there any other rules worth asserting here? For instance, can a character be '\0'?

[Darin Adler]

Comment on attachment 449758 [details]
Patch

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

>> Source/WebCore/platform/graphics/FourCC.cpp:36
>> +        static_cast<char>(string[0]),
> 
> Are there any other rules worth asserting here? For instance, can a character be '\0'?

In practice, I think these all need to be printables, in the range 0x20-0x7F. But not sure if it’s valuable in practice to check that, though. Making a FourCC successfully with values < 0x20 for some of the characters would not have any ill effect that I am aware of. Mainly the reason this function checks that the string is all ASCII is we don’t want to chop non-8-bit UTF-16 code units and use just the low 8 bits. I think I won’t add more checks.

Related: The '\0' below is just there so we can reuse the constructor that is mainly present for ASCII literals, and it will be collapsed away by inlining.

[Darin Adler]

Created attachment 449771 [details]
Patch for landing

[Darin Adler]

Committed r288426 (246317@trunk): <https://commits.webkit.org/246317@trunk>

[Radar WebKit Bug Importer]

<rdar://problem/87947650>