see patch
Created attachment 71645 [details] Patch
Comment on attachment 71645 [details] Patch I think this patch would be a lot easier to review if you were to clean up createGlobalUrlFileDescriptor and createGlobalURLContent without merging them into writeURL (maybe even with a separate patch for each), and then merge them into writeURL. As it currently stands there's so much changing at once that it's hard to verify that the result is correct.
Created attachment 72035 [details] Patch
Comment on attachment 72035 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=72035&action=review > 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.
(In reply to comment #4) > (From update of attachment 72035 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=72035&action=review > > > 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?
Comment on attachment 72035 [details] Patch 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.
Comment on attachment 72035 [details] Patch Clearing flags on attachment: 72035 Manually committed r70914: <http://trac.webkit.org/changeset/70914>
Created attachment 72370 [details] Patch (part 2)
Comment on attachment 72370 [details] Patch (part 2) View in context: https://bugs.webkit.org/attachment.cgi?id=72370&action=review > WebCore/platform/win/ClipboardWin.cpp:695 > + FILEGROUPDESCRIPTOR* fgd = reinterpret_cast<FILEGROUPDESCRIPTOR*>(GlobalLock(urlFileDescriptor)); static_cast would be better here... > WebCore/platform/win/ClipboardWin.cpp:705 > + char* fileContents = reinterpret_cast<char*>(GlobalLock(urlFileContent)); ...and here.
Created attachment 72564 [details] Patch (part 2)
Comment on attachment 72564 [details] Patch (part 2) Clearing flags on attachment: 72564 Committed r71145: <http://trac.webkit.org/changeset/71145>
All reviewed patches have been landed. Closing bug.