Bug 48189

Summary: Cleanup createGlobalImageFileDescriptor in ClipboardWin
Product: WebKit Reporter: Patrick R. Gansterer <paroga>
Component: PlatformAssignee: Patrick R. Gansterer <paroga>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben, commit-queue
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Other   
Bug Depends on:    
Bug Blocks: 48412    
Attachments:
Description Flags
Patch
aroben: review-, aroben: commit-queue-
Patch
none
Patch (part 2)
aroben: review+, aroben: commit-queue-
Patch (part 2) none

Description Patrick R. Gansterer 2010-10-23 10:14:46 PDT
see patch
Comment 1 Patrick R. Gansterer 2010-10-23 10:20:23 PDT
Created attachment 71645 [details]
Patch
Comment 2 Adam Roben (:aroben) 2010-10-27 04:57:13 PDT
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.
Comment 3 Patrick R. Gansterer 2010-10-27 07:58:33 PDT
Created attachment 72035 [details]
Patch
Comment 4 Adam Roben (:aroben) 2010-10-27 08:33:00 PDT
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.
Comment 5 Patrick R. Gansterer 2010-10-27 08:37:33 PDT
(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 6 Adam Roben (:aroben) 2010-10-28 13:28:39 PDT
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 7 Patrick R. Gansterer 2010-10-29 12:15:14 PDT
Comment on attachment 72035 [details]
Patch

Clearing flags on attachment: 72035

Manually committed r70914: <http://trac.webkit.org/changeset/70914>
Comment 8 Patrick R. Gansterer 2010-10-29 12:28:43 PDT
Created attachment 72370 [details]
Patch (part 2)
Comment 9 Adam Roben (:aroben) 2010-11-01 14:14:25 PDT
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.
Comment 10 Patrick R. Gansterer 2010-11-01 14:44:49 PDT
Created attachment 72564 [details]
Patch (part 2)
Comment 11 WebKit Commit Bot 2010-11-02 12:07:51 PDT
Comment on attachment 72564 [details]
Patch (part 2)

Clearing flags on attachment: 72564

Committed r71145: <http://trac.webkit.org/changeset/71145>
Comment 12 WebKit Commit Bot 2010-11-02 12:07:57 PDT
All reviewed patches have been landed.  Closing bug.