[webkit-reviews] review granted: [Bug 48189] Cleanup createGlobalImageFileDescriptor in ClipboardWin : [Attachment 72035] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted  review:
Bug 48189: Cleanup createGlobalImageFileDescriptor in ClipboardWin
https://bugs.webkit.org/show_bug.cgi?id=48189

Attachment 72035: Patch
https://bugs.webkit.org/attachment.cgi?id=72035&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=72035&action=review

> WebCore/platform/win/ClipboardWin.cpp:725
>      int estimatedSize = 0;

Looks like you can get rid of this variable now.

>>> WebCore/platform/win/ClipboardWin.cpp:727
>>> +	 CString content = makeString("[InternetShortcut]\r\nURL=", url,
"\r\n").ascii();
>> 
>> This doesn't do the same thing that the old code did. The old code converted
from UTF-16 to a multi-byte string using the current code page, while this
converts from UTF-16 to ASCII. That seems wrong if the URL contains non-ASCII
characters.
> 
> Isn't the url.string() percent encoded? So i should contain only ASCII by
default?

Yes, it looks like it is. Is there an assertion we can add that says that url
is all-ASCII? (Seems like that should be pretty easy.) It would be good to add
a comment at least.