Summary: | Refactoring: window.btoa() and window.atob() should be implemented on DOMWindow | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hajime Morrita <morrita> | ||||||||||
Component: | DOM | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue, webkit.review.bot, yaar | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Attachments: |
|
Description
Hajime Morrita
2010-03-04 00:05:20 PST
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. |