[webkit-reviews] review granted: [Bug 60819] REGRESSION (WK2): Can't drag and drop a link or image from Safari to Desktop : [Attachment 93534] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Enrica Casucci <enrica at apple.com>'s
request for review:
Bug 60819: REGRESSION (WK2): Can't drag and drop a link or image from Safari to
Desktop
https://bugs.webkit.org/show_bug.cgi?id=60819

Attachment 93534: Patch
https://bugs.webkit.org/attachment.cgi?id=93534&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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?