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

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> 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 77467: Alternative Patch
https://bugs.webkit.org/attachment.cgi?id=77467&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
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);