see patch
Created attachment 74960 [details] Patch
Created attachment 74961 [details] Patch
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?
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
Attachment 77466 [details] did not build on win: Build output: http://queues.webkit.org/results/7253194
Created attachment 77467 [details] Alternative Patch Fixed Windows Build
Comment on attachment 74961 [details] Patch r- I think base64Encode() should return a String instead of taking it as an out parameter.
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);
Created attachment 79868 [details] Patch
(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...
(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!
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?
Created attachment 79870 [details] Patch
Comment on attachment 79870 [details] Patch r=me
Comment on attachment 79870 [details] Patch Clearing flags on attachment: 79870 Committed r76472: <http://trac.webkit.org/changeset/76472>
All reviewed patches have been landed. Closing bug.