Currently window.btoa() and window.atob() are separetely implemented in both JSC and V8 binding. They should share code as much as possible and the code should be on DOMWindow.
Created attachment 49997 [details] patch v0
Attachment 49997 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/page/DOMWindow.cpp:832: Missing spaces around = [whitespace/operators] [4] WebCore/page/DOMWindow.cpp:832: Missing spaces around < [whitespace/operators] [3] Total errors found: 2 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 49998 [details] v1; fix style violations
Comment on attachment 49998 [details] v1; fix style violations > - return jsString(exec, String(out.data(), out.size())); > + return jsString(exec, String(result.characters(), result.length())); This is wrong. It makes an extra copy of the entire string! It should just be jsString(exec, result).
Created attachment 50075 [details] v2; follow the feedback, cleanup a little
Thank you for reviewing quickly! (In reply to comment #4) > This is wrong. It makes an extra copy of the entire string! It should just be > jsString(exec, result). Fixed. And removed unnecessary code that last patch has.
Comment on attachment 50075 [details] v2; follow the feedback, cleanup a little > +static bool hasMultibyteCharacters(const String& string) > +{ > + for (unsigned i = 0; i < string.length(); i++) { > + if (string[i] > 0xFF) > + return true; > + } > + > + return false; > +} This is not a great name for this function. I don’t really think of U+0040 as a “single byte” character nor do I think of U+0100 as “multi-byte”. Patch otherwise seems OK. r=me
Created attachment 50083 [details] v3; rename hasMultibyteCharacters to isSafeToConvertCharList
Thank you for reviewing! > This is not a great name for this function. I don’t really think of U+0040 as a > “single byte” character nor do I think of U+0100 as “multi-byte”. I agree that "multi-byte character" is not right. But I'm not sure if there is some good concept to represent this case. So I chose the name intend to be valid locally in this context.
Comment on attachment 50083 [details] v3; rename hasMultibyteCharacters to isSafeToConvertCharList > +static bool isSafeToConvertCharList(const String& string) This is probably better than the old name, but I would have used a name like containsOnlyEightBitCharacters. Sorry I didn't suggest a name last time. r=me either with your name or with my suggested name.
Comment on attachment 50083 [details] v3; rename hasMultibyteCharacters to isSafeToConvertCharList Clearing flags on attachment: 50083 Committed r55619: <http://trac.webkit.org/changeset/55619>
All reviewed patches have been landed. Closing bug.