[webkit-reviews] review granted: [Bug 50122] Add an overload to base64Encode with String output : [Attachment 79868] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Patrick R. Gansterer
<paroga at paroga.com>'s request for review:
Bug 50122: Add an overload to base64Encode with String output
https://bugs.webkit.org/show_bug.cgi?id=50122

Attachment 79868: Patch
https://bugs.webkit.org/attachment.cgi?id=79868&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=79868&action=review

Patch seems OK as is, but I think it could be refined a bit more.

> Source/WebCore/page/DOMWindow.cpp:987
> +    // String::latin1() will return character values in the range 0..255.

This comment is oblique. To some it might be helpful, but to me it seems that
it makes a statement about the behavior of the latin1 function without making
clear why that’s relevant here.

A better comment would be one that points out that since the
isSafeToConvertCharList has already checked for non-Latin-1 characters so we
are guaranteed all that characters are in the Latin-1 range and so we can call
latin1() without losing anything. Or a comment that says something about how
the desired behavior  of the atob function is to base-64-encode only Latin-1
characters as single bytes, and so that’s why we want to call latin1(). Or no
comment at all.

I also think that the isSafeToConvertCharList should be probably be renamed
isAllLatin1. Or we could come up with a new term like singleByteCharacter and
use it in functions like latin1() throughout the WebKit project.

> Source/WebCore/platform/graphics/skia/FontCustomPlatformData.cpp:115
>  static String createUniqueFontName()

It’s not good that we have three identical copies of this function. Not your
fault, though.

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:127
>  static String encodeBasicAuthorization(const String& user, const String&
password)

It’s not good that we have two identical copies of this function. Not your
fault, though.

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:130
>      CString unencodedString = (user + ":" + password).utf8();
> -    Vector<char> unencoded(unencodedString.length());
> -    std::copy(unencodedString.data(), unencodedString.data() +
unencodedString.length(), unencoded.begin());
> -    Vector<char> encoded;
> -    base64Encode(unencoded, encoded);
> -    return String(encoded.data(), encoded.size());
> +    return base64Encode(unencodedString);

No need for a local variable any more. This would read better in a single line.


> Source/WebCore/platform/text/Base64.cpp:64
> +template<typename T>
> +static inline void base64EncodeInternal(const char* data, unsigned len,
Vector<T>& out, bool insertLFs)

I seems a bit awkward to have to compile a whole extra copy of this entire
function just to avoid copying from a char vector into a UChar vector. It does
take less memory allocation at runtime though. I’m ambivalent about it, I
guess.

> Source/WebCore/platform/text/Base64.cpp:125
> +String base64Encode(const char* data, unsigned len, bool insertLFs)

How about using the word “length” instead of the abbreviation “len”?

> Source/WebCore/platform/text/Base64.cpp:127
> +    Vector<UChar> out;

I don’t think “out” is a great name even though it’s used elsewhere in the
file. How about “result”?

> Source/WebCore/platform/text/Base64.cpp:128
> +    base64EncodeInternal<UChar>(data, len, out, insertLFs);

You should not need to specify <UChar> here. Could you try without it?

> Source/WebCore/platform/text/Base64.cpp:132
> +void base64Encode(const char* data, unsigned len, Vector<char>& out, bool
insertLFs)

How about using the word “length” instead of the abbreviation “len”?

> Source/WebCore/platform/text/Base64.cpp:134
> +    base64EncodeInternal<char>(data, len, out, insertLFs);

You should not need to specify <char> here. Could you try without it?

> Source/WebCore/platform/text/Base64.cpp:138
> +}
> +
> +
>  bool base64Decode(const Vector<char>& in, Vector<char>& out,
Base64DecodePolicy policy)

Just one blank line between functions, not two.

> Source/WebCore/platform/text/Base64.h:43
> -void base64Encode(const Vector<char>&, Vector<char>&, bool insertLFs =
false);
>  void base64Encode(const char*, unsigned, Vector<char>&, bool insertLFs =
false);
> +inline void base64Encode(const Vector<char>& in, Vector<char>& out, bool
insertLFs = false) { base64Encode(in.data(), in.size(), out, insertLFs); }
> +inline void base64Encode(const CString& in, Vector<char>& out, bool
insertLFs = false) { base64Encode(in.data(), in.length(), out, insertLFs); }
> +String base64Encode(const char*, unsigned, bool insertLFs = false);
> +inline String base64Encode(const Vector<char>& in, bool insertLFs = false) {
return base64Encode(in.data(), in.size(), insertLFs); }
> +inline String base64Encode(const CString& in, bool insertLFs = false) {
return base64Encode(in.data(), in.length(), insertLFs); }

The insertLFs argument suffers from the same unreadable boolean constant
argument problem we’ve encountered elsewhere. The true or false is
unnecessarily mysterious at call sites. We should probably change it into an
enum, although that’s not directly related to this patch.

The inline function bodies make it a little harder to read the header. The
inlines, extra argument names, and function bodies distract from the list of
functions you can call. I’d slightly prefer putting the inline function
definitions at the end of the file after all the function declarations. Makes
it a little easier to find the function you want to use with fewer
distractions.

What do you think?