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

[bugzilla-daemon at webkit.org]

Darin Adler <darin 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 80260: Patch2
https://bugs.webkit.org/attachment.cgi?id=80260&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=80260&action=review

Looks good. I have a few style comments, but most were minor.

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:133
> +	   *title = String((UChar*)filename);

You don’t need the explicit String() here. And I’m surprised you need the
explicit cast to UChar*. If there’s a way to avoid it, I think we should.

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:136
> +    url = String((UChar*)urlBuffer);

Ditto.

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:426
> +    if (data->contains(urlWFormat()->cfFormat))
> +	   return extractURL(data->get(urlWFormat()->cfFormat)[0], title);
> +    if (data->contains(urlFormat()->cfFormat))
> +	   return extractURL(data->get(urlFormat()->cfFormat)[0], title);

It’s not good for performance to do first contains and then get on the same
key; that’s a double hash lookup. One alternative is to use find and then use
iterator->second to get the value from the map.

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:435
> +    if (data->contains(filenameWFormat()->cfFormat))
> +	   stringData = data->get(filenameWFormat()->cfFormat)[0];
> +    else if (data->contains(filenameFormat()->cfFormat))
> +	   stringData = data->get(filenameFormat()->cfFormat)[0];

Same issue as above.

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:484
> +    if (data->contains(plainTextWFormat()->cfFormat))
> +	   text = data->get(plainTextWFormat()->cfFormat)[0];
> +    else if (data->contains(plainTextFormat()->cfFormat))
> +	   text = data->get(plainTextFormat()->cfFormat)[0];

Same again. You could probably make a helper function to factor out the code;
these sequences are similar.

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:587
> +PassRefPtr<DocumentFragment> fragmentFromHTML(Document* doc, const
DragDataMap* data) 

Please use document instead of doc.

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:594
> +    if (data->contains(htmlFormat()->cfFormat))
> +	   if (PassRefPtr<DocumentFragment> fragment = fragmentFromCFHTML(doc,
data->get(htmlFormat()->cfFormat)[0]))
> +	       return fragment;

Need parentheses around the second if since it’s a multi-line if body.

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:625
> +	   ReleaseStgMedium(&store);
> +    } else if (format == texthtmlFormat()->cfFormat) {

I personally would prefer returns rather than elses in this function.

> Source/WebCore/platform/win/ClipboardWin.cpp:461
> +	   return (m_dataObject) ? getPlainText(m_dataObject.get(), success) :
getPlainText(&m_dragDataMap);

We normally don’t put parentheses around the predicate in a ? : like this in
WebKit code.

> Source/WebCore/platform/win/DragDataWin.cpp:179
> +	   if (containsFilenames(m_platformDragData))
> +	       if (PassRefPtr<DocumentFragment> fragment =
fragmentFromFilenames(frame->document(), m_platformDragData))
> +		   return fragment;

Braces needed here.

> Source/WebCore/platform/win/DragDataWin.cpp:183
> +	   if (containsHTML(m_platformDragData))
> +	       if (PassRefPtr<DocumentFragment> fragment =
fragmentFromHTML(frame->document(), m_platformDragData))
> +		   return fragment;

And here.

> Source/WebCore/platform/win/DragDataWin.cpp:187
> +	   if (containsFilenames(&m_dragDataMap))
> +	       if (PassRefPtr<DocumentFragment> fragment =
fragmentFromFilenames(frame->document(), &m_dragDataMap))
> +		   return fragment;

And here.

> Source/WebCore/platform/win/DragDataWin.cpp:191
> +	   if (containsHTML(&m_dragDataMap))
> +	       if (PassRefPtr<DocumentFragment> fragment =
fragmentFromHTML(frame->document(), &m_dragDataMap))
> +		   return fragment;

And here.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:619
> +	   dragData->draggingSourceOperationMask(),
> +	   dragData->dragDataMap(),
> +	   dragData->flags()),
> +	   m_pageID);

These needn’t each be on a separate line.

> Source/WebKit2/WebProcess/WebPage/WebPage.messages.in:109
> +#endif
> +#if !PLATFORM(WIN)

Why not use #else here?