Bug 48189 – Cleanup createGlobalImageFileDescriptor in ClipboardWin

• Component: Platform
• Status: RESOLVED FIXED

[Patrick R. Gansterer]

see patch

[Patrick R. Gansterer]

Created attachment 71645 [details]
Patch

[Adam Roben (:aroben)]

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.

[Patrick R. Gansterer]

Created attachment 72035 [details]
Patch

[Adam Roben (:aroben)]

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.

[Patrick R. Gansterer]

(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?

[Adam Roben (:aroben)]

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.

[Patrick R. Gansterer]

Comment on attachment 72035 [details]
Patch

Clearing flags on attachment: 72035

Manually committed r70914: <http://trac.webkit.org/changeset/70914>

[Patrick R. Gansterer]

Created attachment 72370 [details]
Patch (part 2)

[Adam Roben (:aroben)]

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.

[Patrick R. Gansterer]

Created attachment 72564 [details]
Patch (part 2)

[WebKit Commit Bot]

Comment on attachment 72564 [details]
Patch (part 2)

Clearing flags on attachment: 72564

Committed r71145: <http://trac.webkit.org/changeset/71145>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.