Bug 60819 – REGRESSION (WK2): Can't drag and drop a link or image from Safari to Desktop

• Component: WebKit2
• Status: RESOLVED FIXED

[Enrica Casucci]

* STEPS TO REPRODUCE
1.go to flickr.com
2. Drag any image to Desktop 
3. Draga a link to Desktop

* RESULTS
nothing is saved on the Desktop.

<rdar://problem/9370689>

[Enrica Casucci]

Created attachment 93534 [details]
Patch

[WebKit Review Bot]

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.

[Enrica Casucci]

I've already fixed the style issues.

[Darin Adler]

Comment on attachment 93534 [details]
Patch

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?

[Brian Weinstein]

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
> +#if PLATFORM(WIN)
> +#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.

[Enrica Casucci]

Thanks for the comments. I'll address your feedback.

[Enrica Casucci]

http://trac.webkit.org/changeset/86477