[webkit-reviews] review granted: [Bug 52775] WebKit2: add support for drag and drop on Windows : [Attachment 81503] Patch3

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted Enrica Casucci
<enrica at apple.com>'s request for review:
Bug 52775: WebKit2: add support for drag and drop on Windows
https://bugs.webkit.org/show_bug.cgi?id=52775

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

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=81503&action=review

I didn't look at the code that was just moved from WebKit to WebCore very
carefully.

> Source/WebCore/platform/mac/DragImageMac.mm:164
> +	   } else
> +		 imageSize.width = max(labelSize.width + DragLabelBorderX * 2,
urlStringSize.width + DragLabelBorderX * 2);

Funky indentation here.

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:694
> +void setCharData(IDataObject* data, FORMATETC* format, const Vector<String>&
dataStrings)

I think it would be more honest to call this setUTF8Data, even though we're
hoping that treating it as UTF-8 doesn't matter.

>> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:705
>> +	memcpy(buffer, charString.data(), stringLength);
> 
> I'm still curious whether Windows apps expect to get a string encoded with
UTF-8, rather than something encoded with the current code page. Have you tried
dragging a non-ASCII string to another Windows app and seeing what happens?

Enrica explained to me that her new code matches the behavior of the existing
code that was already in this function. If we're dealing with ANSI text
incorrectly, we're dealing with it incorrectly everywhere, not just in this new
code. Clearly it isn't a big issue, since we haven't had bugs filed about it in
all the time the old code has been in use.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:694
> +    HBITMAP hbmp = ::CreateDIBSection(0, &bitmapInfo, DIB_RGB_COLORS, &bits,
0, 0);

It would be slightly better to use an OwnPtr<HBITMAP> here…

> Source/WebKit2/UIProcess/WebPageProxy.cpp:701
> +    sdi.hbmpDragImage = hbmp;

…and then to use hbmp.leakPtr() here. That makes the memory management a little
more explicit.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:723
> +    ::GetCursorPos((LPPOINT)&globalPoint);
> +    POINT localPoint = globalPoint;
> +    ::ScreenToClient(m_pageClient->nativeWindow(), (LPPOINT)&localPoint);

I don't think these casts are needed.

> Source/WebKit2/UIProcess/WebPageProxy.h:311
> +    void startDragDrop(const WebCore::IntPoint& imagePoint, const
WebCore::IntPoint& dragPoint, uint64_t okEffect, HashMap<UINT, Vector<String>>
dataMap, const WebCore::IntSize& dragImageSize, const SharedMemory::Handle&
dragImageHandle, bool isLinkDrag);

You should add a space in between ">>". And dataMap should be a reference to
const, to avoid copying the map.

> Source/WebKit2/UIProcess/WebPageProxy.messages.in:203
> +    StartDragDrop(WebCore::IntPoint imagePoint, WebCore::IntPoint dragPoint,
uint64_t okEffect, HashMap<UINT,Vector<String>> dataMap, WebCore::IntSize
dragImageSize, WebKit::SharedMemory::Handle dragImage, bool linkDrag)

Ditto about ">>".

> Source/WebKit2/WebProcess/WebCoreSupport/win/WebDragClientWin.cpp:69
> +    HDC bitmapDC = CreateCompatibleDC(0);

You should use OwnPtr<HDC> here and remove the call to DeleteDC. Please prefix
with ::, too.

> Source/WebKit2/WebProcess/WebCoreSupport/win/WebDragSource.cpp:84
> +HRESULT WebDragSource::GiveFeedback(DWORD dwEffect)

You should remove the unused parameter name here.

> Source/WebKit2/WebProcess/WebCoreSupport/win/WebDragSource.h:37
> +    virtual HRESULT STDMETHODCALLTYPE QueryInterface(REFIID riid, void**
ppvObject);	   
> +    virtual ULONG STDMETHODCALLTYPE AddRef();
> +    virtual ULONG STDMETHODCALLTYPE Release();

Can these be private, too?

> Source/WebKit2/WebProcess/WebCoreSupport/win/WebDragSource.h:40
> +    static PassRefPtr<WebDragSource> createInstance();
> +private:

Missing a blank line before "private".

> Source/WebKit2/win/WebKit2.vcproj:1450
> +	     <File
> +	      
RelativePath="..\WebProcess\WebCoreSupport\win\WebDragClientWin.cpp"
> +	       >
> +	     </File>
> +	     <File
> +	      
RelativePath="..\WebProcess\WebCoreSupport\win\WebDragSource.cpp"
> +	       >
> +	     </File>
> +	     <File
> +	       RelativePath="..\WebProcess\WebCoreSupport\win\WebDragSource.h"
> +	       >
> +	     </File>

Looks like this is using spaces instead of tabs (which VS prefers).