Bug 60819

Summary: REGRESSION (WK2): Can't drag and drop a link or image from Safari to Desktop
Product: WebKit Reporter: Enrica Casucci <enrica>
Component: WebKit2Assignee: Enrica Casucci <enrica>
Severity: Normal CC: bweinstein, webkit.review.bot
Priority: P2 Keywords: InRadar, PlatformOnly, Regression
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows 7   
Description Flags
Patch darin: review+

Description Enrica Casucci 2011-05-13 16:51:15 PDT
1.go to flickr.com
2. Drag any image to Desktop 
3. Draga a link to Desktop

nothing is saved on the Desktop.

Comment 1 Enrica Casucci 2011-05-13 17:05:55 PDT
Created attachment 93534 [details]
Comment 2 WebKit Review Bot 2011-05-13 17:08:12 PDT
Attachment 93534 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/win/ClipboardUtilitiesWin.h:81:  The parameter name "dataObject" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/win/ClipboardUtilitiesWin.h:82:  The parameter name "dataObject" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/win/ClipboardUtilitiesWin.h:83:  The parameter name "dataObject" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/win/ClipboardUtilitiesWin.h:84:  The parameter name "dataObject" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 13 files

If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Enrica Casucci 2011-05-13 17:10:50 PDT
I've already fixed the style issues.
Comment 4 Darin Adler 2011-05-13 17:16:07 PDT
Comment on attachment 93534 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=93534&action=review

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:374
> +    static FORMATETC fileDescriptorFormat = {cf, 0, DVASPECT_CONTENT, -1, TYMED_HGLOBAL};

WebKit coding style puts spaces at the braces.

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:381
> +    static FORMATETC fileContentFormat = {cf, 0, DVASPECT_CONTENT, 0, TYMED_HGLOBAL};

WebKit coding style puts spaces at the braces.

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:392
> +    FILEGROUPDESCRIPTOR* fgd = (FILEGROUPDESCRIPTOR*)::GlobalLock(store.hGlobal);

Could we use a C++ cast here?

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:393
> +    size = fgd->fgd[0].nFileSizeLow;

What if the size is larger than 31 bits?

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:414
> +    STGMEDIUM medium = {0};

The braces thing again.

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:421
> +    FILEGROUPDESCRIPTOR* fgd = (FILEGROUPDESCRIPTOR*)::GlobalLock(medium.hGlobal);

Could we use a C++ cast here?

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:427
> +    wcscpy(fgd->fgd[0].cFileName, pathname.charactersWithNullTermination());

What guarantees that cFileName is big enough to hold pathname without overrunning the buffer?

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:435
> +    STGMEDIUM medium = {0};

Braces again.

>> Source/WebCore/platform/win/ClipboardUtilitiesWin.h:81
>> +void getFileDescriptorData(IDataObject* dataObject, int& size, String& pathname);
> The parameter name "dataObject" adds no information, so it should be removed.  [readability/parameter_name] [5]

I agree with the style queue on this.

> Source/WebCore/platform/win/ClipboardWin.cpp:221
> +        OutputDebugString(L"FILE WRITTEN\n");

Did you mean to land this?

> Source/WebKit2/WebProcess/WebCoreSupport/win/WebDragClientWin.cpp:96
> +    m_page->send(Messages::WebPageProxy::StartDragDrop(imageOrigin, dragPoint, okEffect, dragData.dragDataMap(), (uint64_t)fileSize, pathname, fileContentHandle, IntSize(bitmapInfo.bmiHeader.biWidth, bitmapInfo.bmiHeader.biHeight), handle, isLink), m_page->pageID());

Do we need that typecast to uint64_t?
Comment 5 Brian Weinstein 2011-05-13 17:20:44 PDT
My comments:

View in context: https://bugs.webkit.org/attachment.cgi?id=93534&action=review

> Source/WebCore/ChangeLog:24
> +        (WebCore::getFileDescriptorData):
> +        (WebCore::getFileContentData):
> +        (WebCore::setFileDescriptorData):
> +        (WebCore::setFileContentData):
> +        (WebCore::setCFData):

A brief discussion of the functions here would be nice.

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:392
> +    FILEGROUPDESCRIPTOR* fgd = (FILEGROUPDESCRIPTOR*)::GlobalLock(store.hGlobal);

Can this be a static cast?

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:794
> +    medium.tymed = TYMED_HGLOBAL;

Was this change necessary? A comment in the ChangeLog about this would be nice.

> Source/WebCore/platform/win/ClipboardWin.cpp:221
> +        OutputDebugString(L"FILE WRITTEN\n");

This should be removed.

> Source/WebCore/platform/win/ClipboardWin.cpp:704
> +    CString content = makeString("[InternetShortcut]\r\nURL=", url, "\r\n").latin1();

It this the only change that was needed for the regression from r70914? It would be nice to have this in its own patch, but doesn't seem necessary if the patch is already written.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:75
> +#include <WebCore/ClipboardUtilitiesWin.h>
> +#endif

This should be after the other includes, I think.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:793
> +    }

A blank line after this brace would be nice.

> Source/WebKit2/WebProcess/WebCoreSupport/win/WebDragClientWin.cpp:95
> +    }

I'd like to see a blank line after this brace.
Comment 6 Enrica Casucci 2011-05-13 17:26:36 PDT
Thanks for the comments. I'll address your feedback.
Comment 7 Enrica Casucci 2011-05-13 18:19:29 PDT