Bug 225920 – Update Base64 encoding/decoding to match more modern WebKit conventions

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Sam Weinig]

Update Base64 encoding/decoding to match more modern WebKit conventions (fewer out values, string concatenation adaptor, etc.)

[Sam Weinig]

Created attachment 428991 [details]
Patch

[Sam Weinig]

Created attachment 428995 [details]
Patch

[Sam Weinig]

Created attachment 429004 [details]
Patch

[Sam Weinig]

Created attachment 429007 [details]
Patch

[Sam Weinig]

Created attachment 429008 [details]
Patch

[Sam Weinig]

Created attachment 429014 [details]
Patch

[Sam Weinig]

Created attachment 429020 [details]
Patch

[Sam Weinig]

Created attachment 429026 [details]
Patch

[Sam Weinig]

Created attachment 429029 [details]
Patch

[Sam Weinig]

There is a lot more cleanup that can be done, but this is a good start.

The main takeaway from this is that we need to make our binary data representations consistent. Our inconsistent use of both const char* and const uint8_t* is a nuisance.

We should probably converge on const uint8_t* and just rid the remaining const char* away (although maybe we want to try to use std::byte instead?)

[Sam Weinig]

Created attachment 429046 [details]
Patch

[Chris Dumez]

Comment on attachment 429046 [details]
Patch

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

I did not r+ yet because I dislike all the new makeString(base64encoded()). It makes the code more verbose than it used to be, which is unfortunate.

> Source/WTF/ChangeLog:8
> +        Bring base64 encoding/decoding up to more modern WebKit convetions by:

typo: convetions

> Source/WTF/wtf/text/Base64.cpp:34
> +static constexpr unsigned encodeMapSize = 64;

I believe we don't need the static.

> Source/WTF/wtf/text/Base64.cpp:35
> +static constexpr unsigned decodeMapSize = 128;

ditto.

> Source/WTF/wtf/text/Base64.cpp:118
> +            destinationDataBuffer[didx++] = encodeMap[ (inputDataBuffer[sidx    ] >> 2) & 077];

I thought we had stopped adding spacing for alignment like that in new code? I am personally not a fan as it's a pain to maintain and they eventually get misaligned.

> Source/WTF/wtf/text/Base64.cpp:141
> +        destinationDataBuffer[didx] = '=';

destinationDataBuffer[didx++] = '=';

> Source/WTF/wtf/text/Base64.cpp:142
> +        ++didx;

Then we don't need this line.

Or we could use a for loop:
for (; didx < destinationLength; ++didx)
  destinationDataBuffer[didx] = '=';

> Source/WebCore/Modules/plugins/QuickTimePluginReplacement.mm:249
> +        data64 = makeString("data:", mimeType, ";base64,", base64Encoded([data bytes], [data length]));

Is this why base64Encoded() doesn't return a String. Is is more efficient in this case? I am assuming there is a reason base64Encoded() doesn't simply return a String (which would be much nicer to use) considering most call sites need a String.

[Sam Weinig]

(In reply to Chris Dumez from comment #12)
> Comment on attachment 429046 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429046&action=review
> 
> I did not r+ yet because I dislike all the new makeString(base64encoded()).
> It makes the code more verbose than it used to be, which is unfortunate.

Fair enough. I thought that might be the case for some. The other option here is:

Rename base64Encode() (which returns a vector) to base64EncodeToVector() or base64EncodeToData(), and add base64EncodeToString() (or base64Encode() could just return a String()?)


> 
> > Source/WTF/ChangeLog:8
> > +        Bring base64 encoding/decoding up to more modern WebKit convetions by:
> 
> typo: convetions
> 
> > Source/WTF/wtf/text/Base64.cpp:34
> > +static constexpr unsigned encodeMapSize = 64;
> 
> I believe we don't need the static.

You are right. Will fix.

> > Source/WTF/wtf/text/Base64.cpp:118
> > +            destinationDataBuffer[didx++] = encodeMap[ (inputDataBuffer[sidx    ] >> 2) & 077];
> 
> I thought we had stopped adding spacing for alignment like that in new code?
> I am personally not a fan as it's a pain to maintain and they eventually get
> misaligned.

It made reading and understanding this code so much easier for me. I'm not really sure I understand the argument for hard to maintain.

> 
> > Source/WTF/wtf/text/Base64.cpp:141
> > +        destinationDataBuffer[didx] = '=';
> 
> destinationDataBuffer[didx++] = '=';
> 
> > Source/WTF/wtf/text/Base64.cpp:142
> > +        ++didx;
> 
> Then we don't need this line.

Ok.

> 
> Or we could use a for loop:
> for (; didx < destinationLength; ++didx)
>   destinationDataBuffer[didx] = '=';
> 
> > Source/WebCore/Modules/plugins/QuickTimePluginReplacement.mm:249
> > +        data64 = makeString("data:", mimeType, ";base64,", base64Encoded([data bytes], [data length]));
> 
> Is this why base64Encoded() doesn't return a String. Is is more efficient in
> this case? I am assuming there is a reason base64Encoded() doesn't simply
> return a String (which would be much nicer to use) considering most call
> sites need a String.

Yes, it avoids an extra allocation and copy

[Sam Weinig]

Created attachment 429060 [details]
Patch

[Sam Weinig]

Created attachment 429061 [details]
Patch

[Sam Weinig]

Created attachment 429064 [details]
Patch

[Chris Dumez]

Comment on attachment 429064 [details]
Patch

r=me

[Sam Weinig]

Created attachment 429068 [details]
Patch

[EWS]

Committed r277740 (237912@main): <https://commits.webkit.org/237912@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/78216131>

[Darin Adler]

Comment on attachment 429068 [details]
Patch

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

> Source/WTF/wtf/text/Base64.h:77
> +WTF_EXPORT_PRIVATE Optional<Vector<uint8_t>> base64Decode(const String&, OptionSet<Base64DecodeOptions> = { }, Base64DecodeMap = Base64DecodeMap::Default);
> +WTF_EXPORT_PRIVATE Optional<Vector<uint8_t>> base64Decode(StringView, OptionSet<Base64DecodeOptions> = { }, Base64DecodeMap = Base64DecodeMap::Default);
> +WTF_EXPORT_PRIVATE Optional<Vector<uint8_t>> base64Decode(const Vector<uint8_t>&, OptionSet<Base64DecodeOptions> = { }, Base64DecodeMap = Base64DecodeMap::Default);
> +WTF_EXPORT_PRIVATE Optional<Vector<uint8_t>> base64Decode(const Vector<char>&, OptionSet<Base64DecodeOptions> = { }, Base64DecodeMap = Base64DecodeMap::Default);
> +WTF_EXPORT_PRIVATE Optional<Vector<uint8_t>> base64Decode(const uint8_t*, unsigned, OptionSet<Base64DecodeOptions> = { }, Base64DecodeMap = Base64DecodeMap::Default);
> +WTF_EXPORT_PRIVATE Optional<Vector<uint8_t>> base64Decode(const char*, unsigned, OptionSet<Base64DecodeOptions> = { }, Base64DecodeMap = Base64DecodeMap::Default);

Now I suggest we remove the overloads for const String& and const char* since both of those convert into StringView efficiently. Unless there is some kind of problem with ambiguity at call sites?

[Sam Weinig]

(In reply to Darin Adler from comment #21)
> Comment on attachment 429068 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=429068&action=review
> 
> > Source/WTF/wtf/text/Base64.h:77
> > +WTF_EXPORT_PRIVATE Optional<Vector<uint8_t>> base64Decode(const String&, OptionSet<Base64DecodeOptions> = { }, Base64DecodeMap = Base64DecodeMap::Default);
> > +WTF_EXPORT_PRIVATE Optional<Vector<uint8_t>> base64Decode(StringView, OptionSet<Base64DecodeOptions> = { }, Base64DecodeMap = Base64DecodeMap::Default);
> > +WTF_EXPORT_PRIVATE Optional<Vector<uint8_t>> base64Decode(const Vector<uint8_t>&, OptionSet<Base64DecodeOptions> = { }, Base64DecodeMap = Base64DecodeMap::Default);
> > +WTF_EXPORT_PRIVATE Optional<Vector<uint8_t>> base64Decode(const Vector<char>&, OptionSet<Base64DecodeOptions> = { }, Base64DecodeMap = Base64DecodeMap::Default);
> > +WTF_EXPORT_PRIVATE Optional<Vector<uint8_t>> base64Decode(const uint8_t*, unsigned, OptionSet<Base64DecodeOptions> = { }, Base64DecodeMap = Base64DecodeMap::Default);
> > +WTF_EXPORT_PRIVATE Optional<Vector<uint8_t>> base64Decode(const char*, unsigned, OptionSet<Base64DecodeOptions> = { }, Base64DecodeMap = Base64DecodeMap::Default);
> 
> Now I suggest we remove the overloads for const String& and const char*
> since both of those convert into StringView efficiently. Unless there is
> some kind of problem with ambiguity at call sites?

I doubt there will be an issue, just didn't occur to me. Will do.