Bug 50122 – Add an overload to base64Encode with String output

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Patrick R. Gansterer]

see patch

[Patrick R. Gansterer]

Created attachment 74960 [details]
Patch

[Patrick R. Gansterer]

Created attachment 74961 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 74961 [details]
Patch

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

Yeah.  Seems we just want to return the String.  All the callsites want that. :)

> WebCore/platform/text/Base64.h:40
> +void base64Encode(const char*, unsigned, String&, bool insertLFs = false);

Why not just return the String?

[Patrick R. Gansterer]

Created attachment 77466 [details]
Alternative Patch

(In reply to comment #3)
> (From update of attachment 74961 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=74961&action=review
> 
> Yeah.  Seems we just want to return the String.  All the callsites want that. :)
> 
> > WebCore/platform/text/Base64.h:40
> > +void base64Encode(const char*, unsigned, String&, bool insertLFs = false);
> 
> Why not just return the String?
I wanted to keep all the callsites consistent (pass the output argument instead of returning), but it makes sense to return String directly

[Build Bot]

Attachment 77466 [details] did not build on win:
Build output: http://queues.webkit.org/results/7253194

[Patrick R. Gansterer]

Created attachment 77467 [details]
Alternative Patch

Fixed Windows Build

[David Kilzer (:ddkilzer)]

Comment on attachment 74961 [details]
Patch

r-

I think base64Encode() should return a String instead of taking it as an out parameter.

[David Kilzer (:ddkilzer)]

Comment on attachment 77467 [details]
Alternative Patch

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

r=me with the add base64Encode(const String&) method and fixing the DOMWindow.cpp changes.

> WebCore/page/DOMWindow.cpp:978
> +    CString in = stringToEncode.latin1();
> +    return base64Encode(in.data(), in.length());

Converting the string to latin1() here doesn't seem correct.  Why don't you do this instead?

    return base64Encode(stringToEncode.data(), stringToEncode.length());

To be honest, there seems to be a few cases where you just want to encode another String, too, so I think you should add an inline convenience method for these cases (to make the code more readable):

    String base64Encode(const String& in, bool insertLFs = false) { return base64Encode(in.data(), in.length(), insertLFs); }

Then the above just becomes:

    return base64Encode(stringToEncode);

> WebCore/platform/network/cf/ResourceHandleCFNet.cpp:128
> +    return base64Encode(unencodedString.data(), unencodedString.length());

return base64Encode(unencodedString);

> WebCore/platform/network/mac/ResourceHandleMac.mm:166
> +    return base64Encode(unencodedString.data(), unencodedString.length());

return base64Encode(unencodedString);

[Patrick R. Gansterer]

Created attachment 79868 [details]
Patch

[Patrick R. Gansterer]

(In reply to comment #8)
> r=me with the add base64Encode(const String&) method and fixing the DOMWindow.cpp changes.
I don't think that your proposed change is correct, so can you pleas have an other look at this patch.

> > WebCore/page/DOMWindow.cpp:978
> > +    CString in = stringToEncode.latin1();
> > +    return base64Encode(in.data(), in.length());
> 
> Converting the string to latin1() here doesn't seem correct.  Why don't you do this instead?
> 
>     return base64Encode(stringToEncode.data(), stringToEncode.length());
String::data() returns UChar instead of char ;-)
String::latin1() returns char. The valid-check is done in the code lines before.

> To be honest, there seems to be a few cases where you just want to encode another String, too, so I think you should add an inline convenience method for these cases (to make the code more readable):
I also inlined one of the decode functions...

[David Kilzer (:ddkilzer)]

(In reply to comment #10)
> (In reply to comment #8)
> > r=me with the add base64Encode(const String&) method and fixing the DOMWindow.cpp changes.
> I don't think that your proposed change is correct, so can you pleas have an other look at this patch.

Thank you!

> > > WebCore/page/DOMWindow.cpp:978
> > > +    CString in = stringToEncode.latin1();
> > > +    return base64Encode(in.data(), in.length());
> > 
> > Converting the string to latin1() here doesn't seem correct.  Why don't you do this instead?
> > 
> >     return base64Encode(stringToEncode.data(), stringToEncode.length());
> String::data() returns UChar instead of char ;-)
> String::latin1() returns char. The valid-check is done in the code lines before.

Thanks, didn't see that when I looked at the source code before.

> > To be honest, there seems to be a few cases where you just want to encode another String, too, so I think you should add an inline convenience method for these cases (to make the code more readable):
> I also inlined one of the decode functions...

Will do!

[Darin Adler]

Comment on attachment 79868 [details]
Patch

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?

[Patrick R. Gansterer]

Created attachment 79870 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 79870 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 79870 [details]
Patch

Clearing flags on attachment: 79870

Committed r76472: <http://trac.webkit.org/changeset/76472>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.