Bug 52775 – WebKit2: add support for drag and drop on Windows

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Enrica Casucci]

The equivalent work for Mac was tracked by 52343.

[Enrica Casucci]

Created attachment 80238 [details]
Patch

[WebKit Review Bot]

Attachment 80238 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7597358

[WebKit Review Bot]

Attachment 80238 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7497343

[Early Warning System Bot]

Attachment 80238 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7598363

[Enrica Casucci]

Created attachment 80260 [details]
Patch2

Fixes build breaks.

[Darin Adler]

Comment on attachment 80260 [details]
Patch2

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?

[Alice Liu]

Comment on attachment 80238 [details]
Patch

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

Not a full review by me, since i'm not an expert in Windows clipboard data.  Also compilation issues.

> Source/WebCore/platform/DragData.cpp:28
> +#include <WTFString.h>

Do you really need this if you didn't make any other changes to this file?

> Source/WebCore/platform/DragData.h:87
> +typedef HashMap<UINT, Vector<String> > DragDataMap;

Should we put this within a platform guard?

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:115
> +{

maybe add an early return for invalid params?

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:418
> +{

same comment about invalid params, especially data.

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:478
> +{

haven't null checked or asserted data

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:488
> +}

it doesn't look like you actually need text if you just return the function call results.

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:506
> +{

haven't null checked or asserted data.  there are more but i'll stop talking about them.

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:595
> +

i think this outer if statement needs braces under our particular style guidelines.

> Source/WebCore/platform/win/ClipboardWin.cpp:469
>      }

it seems like we can have a smaller-looking, not as messy function if we have a local variable for either m_dataObject, or m_dragDataMap, which ever is available.

If you don't do that, do none of these additional calls need to pass success?

> Source/WebCore/platform/win/DragDataWin.cpp:160
> +            : (containsHTML(&m_dragDataMap) || containsFilenames(&m_dragDataMap)))

This is difficult to read.  can you use a local variable dragData = m_platformDragData ? m_platformDragData : &m_dragDataMap;  to make this cleaner?  Or even better, provide a convenience function to provide you this platform drag data if available. you can use it elsewhere too.

> Source/WebCore/platform/win/DragDataWin.cpp:193
> +    return 0;

same comment as above

[Enrica Casucci]

Thanks for the review. I'll address all your feedback. 
> > Source/WebKit2/WebProcess/WebPage/WebPage.messages.in:109
> > +#endif
> > +#if !PLATFORM(WIN)
> 
> Why not use #else here?
The python script doesn't handle #else correctly. I'll file a bug about this.

[Enrica Casucci]

> > Source/WebCore/platform/DragData.cpp:28
> > +#include <WTFString.h>
> 
> Do you really need this if you didn't make any other changes to this file?

Yes, otherwise I get an error building for Windows.

> > Source/WebCore/platform/DragData.h:87
> > +typedef HashMap<UINT, Vector<String> > DragDataMap;
> 
> Should we put this within a platform guard?

I did that already in the patch that fixes the build break.

thanks for the rest of the feedback, I'll address your comments.

[Adam Roben (:aroben)]

Comment on attachment 80260 [details]
Patch2

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

> Source/WebCore/platform/DragData.h:55
> +#include <WTFString.h>

This should be wtf/text/WTFString.h. I don't think it needs to be in the PLATFORM(WIN) block, even though it's only used there.

> Source/WebCore/platform/DragData.h:89
> +typedef HashMap<UINT, Vector<String> > DragDataMap;

Would it be better for this to be DragData::Map instead?

Storing the Vector<String> by value means that whenever the map has to be rehashed it will copy all the Vectors. Better would be to allocate the Vectors on the heap and just store pointers. You might need a wrapper class to handle deleting the Vectors before the map gets destroyed, though.

> Source/WebCore/platform/DragData.h:100
> +    DragData::DragData(const DragDataMap&, const IntPoint& clientPosition, const IntPoint& globalPosition, DragOperation sourceOperationMask, DragApplicationFlags = DragApplicationNone);

The "DragData::" here seems like a mistake.

> Source/WebCore/platform/DragData.h:101
> +    DragDataMap dragDataMap();

This will return a copy of the map. Better would be:

const DragDataMap& dragDataMap() const { return m_dragDataMap; }

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:119
> +    WCHAR filename[MAX_PATH];
> +    WCHAR urlBuffer[INTERNET_MAX_URL_LENGTH];

If you use Vector<UChar> instead for these, you can use String::adopt when you have to convert to Strings, which will avoid a memcpy.

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:124
> +    if (!dataObject->contains(cfHDropFormat()->cfFormat))
> +        return false;
> +
> +    wcscpy(filename, dataObject->get(cfHDropFormat()->cfFormat)[0].characters());

You're doing two hash lookups here when you could use one instead by using HashMap::find.

You need to use charactersWithNullTermination() instead of characters() here. WTF::Strings are not null-terminated by default.

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:125
> +    if (_wcsicmp(PathFindExtensionW(filename), L".url"))

Would String::endsWith be enough? Would it be better or worse for any reason?

If we will fairly commonly bail out here you could avoid copying the filename until after we do this check, and just pass dataObject->get(cfHDropFormat()->cfFormat)[0].charactersWithNullTermination() here. (You would probably want to store the String in a local if you were doing that for readability purposes.) In fact, it looks like you would only need to copy the string just before calling PathRemoveExtension.

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:139
> +    succeeded = true;
> +#endif
> +    return succeeded;

The succeeded variable doesn't seem very helpful. You could do something like:

#if PLATFORM(CF)
...
return true;
#else
return false;
#endif

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:437
> +    if (!stringData.isEmpty() && (PathFileExists(stringData.characters()) || PathIsUNC(stringData.characters()))) {

Can this become an early return instead?

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:438
> +        RetainPtr<CFStringRef> pathAsCFString(AdoptCF, CFStringCreateWithCharacters(kCFAllocatorDefault, (const UniChar *)stringData.characters(), stringData.length() - 1));

Why are you cutting off the last character? You can use String::createCFString to get a CFStringRef from a WTF::String.

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:440
> +        if (urlFromPath(pathAsCFString.get(), url)) {
> +            if (title)

You can use && here instead of a nested if.

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

You could use early returns here instead of elses.

> 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;

The outer if needs braces, since its body is more than a single line.

Normally we use RefPtr for local variables. Then you would return fragment.release().

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:613
> +void getClipboardData(IDataObject *dataObject, UINT format, Vector<String>& dataStrings)

Throughout this function you can use early returns instead of chaining elses.

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:622
> +        char* dataBlob = static_cast<char*>(GlobalLock(store.hGlobal));
> +        SIZE_T dataSize = ::GlobalSize(store.hGlobal);
> +        dataStrings.append(String(UTF8Encoding().decode(dataBlob, dataSize)));

Why put :: on GlobalSize but not the other functions?

How do we know UTF-8 is the right encoding? You can use String::fromUTF8 instead of invoking UTF8Encoding directly.

Why store dataBlob in a local here but not in the other cases?

There's a lot of copy-and-pasted code in this function. We should come up with a way to make it more concise and less repetitious. One thing that would help would be to have a function like:

template <typename Char> bool getString(IDataObject*, FORMATETC*, String&)

That function would do all the GlobalLock/GlobalUnlock/ReleaseStgMedium work. (You would need a separate getUTF8String function to handle the HTML case, and another function to handle the HDROP case.)

Doing that would reduce the amount of code inside each case. Then we could get rid of the long if/else if chain by doing something like:

typedef bool (*GetStringFunction)(IDataObject*, FORMATETC*, String&);
typedef HashMap<UINT, pair<FORMATETC*, GetStringFunction> > FormatMap;
FormatMap formatMap;

(You'd want to use better names here.)

At some point early on we'd initialize formatMap like this:

formatMap.add(htmlFormat()->cfFormat, make_pair(htmlFormat(), getUTF8String));
formatMap.add(texthtmlFormat()->cfFormat, make_pair(texthtmlFormat(), getString<UChar>));
formatMap.add(plainTextFormat()->cfFormat, make_pair(plainTextFormat(), getString<char>));

Then getClipboardData would become:

FormatMap::iterator found = formatMap.find(format);
if (found == formatMap.end())
    return;
FORMATETC* formatETC = found->second.first;
GetStringFunction getString = found->second.second;
String string;
if (!getString(dataObject, formatETC, string))
    return;
dataStrings.append(string);

Sooooooooo much shorter, with many fewer opportunities for typos.

[WebKit Review Bot]

Attachment 80238 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7639124

[Enrica Casucci]

> This should be wtf/text/WTFString.h. I don't think it needs to be in the PLATFORM(WIN) block, even though it's only used there.
Done.

> > +    DragData::DragData(const DragDataMap&, const IntPoint& clientPosition, const IntPoint& globalPosition, DragOperation sourceOperationMask, DragApplicationFlags = DragApplicationNone);
> 
> The "DragData::" here seems like a mistake.

Fixed.

> You're doing two hash lookups here when you could use one instead by using HashMap::find.

If fixed this everywhere and I have a helper function to extract the value.
 
> You need to use charactersWithNullTermination() instead of characters() here. WTF::Strings are not null-terminated by default.

That explains some weird behavior I had seen :-)
 
 
> > Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:139
> > +    succeeded = true;
> > +#endif
> > +    return succeeded;
> 
> The succeeded variable doesn't seem very helpful. You could do something like:

Fixed. 

> > Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:437
> > +    if (!stringData.isEmpty() && (PathFileExists(stringData.characters()) || PathIsUNC(stringData.characters()))) {
> 
> Can this become an early return instead?

Yes, done.
 
> > Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:438
> > +        RetainPtr<CFStringRef> pathAsCFString(AdoptCF, CFStringCreateWithCharacters(kCFAllocatorDefault, (const UniChar *)stringData.characters(), stringData.length() - 1));
> 
> Why are you cutting off the last character? You can use String::createCFString to get a CFStringRef from a WTF::String.

Problems arising from not using charactersWithNull. Fixed.
 
> > Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:440
> > +        if (urlFromPath(pathAsCFString.get(), url)) {
> > +            if (title)
> 
> You can use && here instead of a nested if.
>
Done.

> > Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:487
> > +    if (data->contains(plainTextWFormat()->cfFormat))
> > +        text = data->get(plainTextWFormat()->cfFormat)[0];
> > +    else if (data->contains(plainTextFormat()->cfFormat))
> > +        text = data->get(plainTextFormat()->cfFormat)[0];
> > +    else
> > +        text = getURL(data, DragData::DoNotConvertFilenames);
> > +    return text;
> 
> You could use early returns here instead of elses.
> 

Done
> > 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;
> 
> The outer if needs braces, since its body is more than a single line.

Done.
 
> Normally we use RefPtr for local variables. Then you would return fragment.release().
> 
Fixed also in the function I copied from.

> > Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:613
> > +void getClipboardData(IDataObject *dataObject, UINT format, Vector<String>& dataStrings)
> 
> Throughout this function you can use early returns instead of chaining elses.

Done.
> 
> How do we know UTF-8 is the right encoding? You can use String::fromUTF8 instead of invoking UTF8Encoding directly.
> 
I assumed it was since it was like this in the function that operates on IDataObject.

[WebKit Review Bot]

Attachment 80238 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7552324

[Enrica Casucci]

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

[Jessie Berlin]

(In reply to comment #14)
> http://trac.webkit.org/changeset/76824

I think r76824 broke a couple of editing/pasteboard tests on the Windows 7 Release bot:

https://bugs.webkit.org/show_bug.cgi?id=53304

[Enrica Casucci]

Created attachment 81082 [details]
FinalPatch

[Enrica Casucci]

This was resolved by mistake. The current patch contains the final work.

[Adam Roben (:aroben)]

Comment on attachment 81082 [details]
FinalPatch

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

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:177
> -    HGLOBAL cbData = ::GlobalAlloc(GPTR, size * sizeof(UChar));
> +    HGLOBAL cbData = GlobalAlloc(GPTR, size * sizeof(UChar));

We've actually been going the other direction in WebKit2 code, so I'd just leave this alone.

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:631
> +class ClipboardDataItem {
> +public:
> +    FORMATETC* format;
> +    GetStringFunction getString;
> +    SetStringFunction setString;
> +
> +    ClipboardDataItem(FORMATETC* fmt, GetStringFunction f1, SetStringFunction f2): format(fmt), getString(f1), setString(f2) {};
> +};

We normally list function members before data members, and use struct instead of class when using public data members.

You can use the names "format", "getString", and "setString" for the constructor arguments, even though they match the names of the data members.

You're missing a space between the constructor's braces, and have an unnecessary semicolon.

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:688
> +    medium.hGlobal = createGlobalData(dataStrings[0]);

I tend to prefer .first() to [0].

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:691
> +    if (medium.hGlobal)
> +        data->SetData(format, &medium, FALSE);
> +    GlobalFree(medium.hGlobal);

Is calling GlobalFree(0) OK?

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:699
> +    CString asciiString = dataStrings[0].utf8();

I'd move this closer to where it's first used.

"asciiString" isn't a good name for this; it's UTF-8, not ASCII! Is it OK to use UTF-8 for all the formats that end up calling this function? Maybe this function should be called setUtf8Data to match getUtf8Data (though I wish they were both "UTF8" instead of "Utf8").

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

Do we expect this ever to be called? If not, adding an ASSERT_NOT_REACHED(), or just using a function pointer of 0, would be better.

This should really be named setCFData (and the getter should be named accordingly).

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:739
> +    ClipboardFormatMap formatMap = getClipboardMap();

This copies the map. You should declare formatMap as a const ClipboardFormatMap&.

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:740
>      ClipboardFormatMap::iterator found = formatMap.find(format->cfFormat);

You'll have to use const_iterator here once you make the above change.

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:748
> +    ClipboardFormatMap formatMap = getClipboardMap();

Same comment here about copying the map.

> Source/WebCore/platform/win/WCDataObject.cpp:165
> +HRESULT WCDataObject::createInstance(WCDataObject** result, const DragDataMap* dataMap)

We typically use references instead of pointers when the argument cannot be null, so I'd suggest const DragDataMap& for the second argument.

> Source/WebCore/platform/win/WCDataObject.cpp:169
> +    *result = new WCDataObject();

No need for the parentheses here.

> Source/WebCore/platform/win/WCDataObject.cpp:171
> +    if (!*result)
> +        return E_OUTOFMEMORY;

Our FastMalloc implementation will make operator new crash if it fails to allocate memory, so this code is unreachable and should be removed.

> Source/WebKit2/ChangeLog:20
> +        platforms other than Window and Mac.

Typo: Window

> Source/WebKit2/UIProcess/WebPageProxy.cpp:74
> +#include <shlObj.h>

We typically use all lowercase for headers like shlobj.h.

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

There shouldn't be any "WebCore::" in here.

We normally leave a space in between ">>". MSVC doesn't require it, but GCC does, so it's a good habit to get into.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:666
> +    RefPtr<SharedMemory> memoryBuffer = SharedMemory::create(dragImageHandle, SharedMemory::ReadWrite);

Don't we only need Read access here?

> Source/WebKit2/UIProcess/WebPageProxy.cpp:671
> +    RefPtr<WebDragSource> source;
> +    source = WebDragSource::createInstance(m_pageClient->nativeWindow());

This can be done in one line.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:677
> +    COMPtr<IDragSourceHelper> helper;
> +    if (FAILED(CoCreateInstance(CLSID_DragDropHelper, 0, CLSCTX_INPROC_SERVER, IID_IDragSourceHelper, (LPVOID*)&helper)))
> +        return;

COMPtr has a special constructor that can do this for you:

COMPtr<IDragSourceHelper> helper(Create, CLSID_DragDropHelper);
if (!helper)
    return;

This will only work if __uuidof(IDragSourceHelper) compiles, which it may not depending on how that interface is defined.

You should use a C++ cast instead of (LPVOID*).

> Source/WebKit2/UIProcess/WebPageProxy.cpp:681
> +    void* bits;
> +    HBITMAP hbmp = CreateDIBSection(0, &bitmapInfo, DIB_RGB_COLORS, static_cast<void**>(&bits), 0, 0);

Is the static_cast really necessary here?

Please prefix Win32 API calls in WebKit2 with ::.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:687
> +    sdi.crColorKey = 0xffffffff;

Please use the RGB macro here.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:710
> +    POINTL pt;
> +    ::GetCursorPos((LPPOINT)&pt);
> +    POINTL localpt = pt;
> +    ::ScreenToClient(m_pageClient->nativeWindow(), (LPPOINT)&localpt);

If you really want a POINT, why not just use a POINT?

> Source/WebKit2/UIProcess/WebPageProxy.cpp:712
> +    dragEnded(IntPoint(localpt.x, localpt.y), IntPoint(pt.x, pt.y), operation);

If you use POINT above, I don't think you'll need these IntPoint constructors. You can convert from POINT to IntPoint implicitly.

> Source/WebKit2/WebProcess/WebCoreSupport/win/WebDragClientWin.cpp:1
> +/*

I'd really like to see this code shared with WebKit1. This is a lot of code to copy!

> Source/WebKit2/WebProcess/WebCoreSupport/win/WebDragSource.cpp:52
> +STDMETHODIMP WebDragSource::QueryInterface(REFIID riid, void** ppvObject)

We normally just use the return type instead of any of the STDMETHODIMP* macros.

> Source/WebKit2/WebProcess/WebCoreSupport/win/WebDragSource.cpp:81
> +    if (fEscapePressed || !(grfKeyState & (MK_LBUTTON | MK_RBUTTON))) {
> +        m_dropped = !fEscapePressed;
> +        return fEscapePressed? DRAGDROP_S_CANCEL : DRAGDROP_S_DROP;

Missing space before ?.

I think this would be clearer if we didn't test fEscapePressed three times:

if (fEscapePressed) {
    m_dropped = false;
    return DRAGDROP_S_CANCEL;
}

m_dropped = true;

if (grfKeyState & (MK_LBUTTON | MK_RBUTTON))
    return S_OK;

return DRAGDROP_S_DROP;

grfKeyState is confusing. It has "key" in the name, which makes it sound like it's about the keyboard, but we're using it to check whether the left or right mouse button is pressed. MSDN even says it represents the "current state of the keyboard modifier keys on the keyboard." It sure doesn't seem like the mouse buttons should be included in that! But then it says that MK_LBUTTON/MK_MBUTTON/MK_RBUTTON may be included. Is MSDN just wrong about it only representing the keyboard state?

> Source/WebKit2/WebProcess/WebCoreSupport/win/WebDragSource.h:33
> +class WebDragSource : public IDropSource, public RefCounted<WebDragSource> {

We've tended not to use RefCounted for implementing COM classes, but doing so seems OK.

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

You should make all the IDropSource members private, since we never expect them to be called on a pointer with static type WebDragSource*.

> Source/WebKit2/WebProcess/WebCoreSupport/win/WebDragSource.h:37
> +    virtual ULONG STDMETHODCALLTYPE AddRef(void);
> +    virtual ULONG STDMETHODCALLTYPE Release(void);

Please remove the "void"s.

> Source/WebKit2/WebProcess/WebCoreSupport/win/WebDragSource.h:41
> +    static PassRefPtr<WebDragSource> createInstance(HWND hWnd);

"hWnd" is unnecessary.

> Source/WebKit2/WebProcess/WebCoreSupport/win/WebDragSource.h:43
> +    ~WebDragSource() {};
> +private:

Missing a space in here.

There's no need to declare this destructor at all. The compiler will do the same thing for you automatically.

> Source/WebKit2/WebProcess/WebCoreSupport/win/WebDragSource.h:44
> +    WebDragSource(HWND hWnd);

Unnecessary "hWnd".

> Source/WebKit2/WebProcess/WebCoreSupport/win/WebDragSource.h:46
> +    long m_ref;
> +    bool m_dropped;

These members are unused.

> Source/WebKit2/WebProcess/WebCoreSupport/win/WebDragSource.h:49
> +    HWND m_window;
> +
> +};

Extra space in there.

> Source/WebKit2/win/WebKit2.vcproj:21
> -			InheritedPropertySheets="$(WebKitVSPropsRedirectionDir)..\..\..\WebKitLibraries\win\tools\vsprops\FeatureDefines.vsprops;$(WebKitVSPropsRedirectionDir)..\..\..\WebKitLibraries\win\tools\vsprops\common.vsprops;$(WebKitVSPropsRedirectionDir)..\..\..\WebKitLibraries\win\tools\vsprops\debug.vsprops;.\WebKit2Common.vsprops;.\WebKit2DirectX.vsprops;.\WebKit2Apple.vsprops"
> +			InheritedPropertySheets="..\..\..\WebKitLibraries\win\tools\vsprops\FeatureDefines.vsprops;..\..\..\WebKitLibraries\win\tools\vsprops\common.vsprops;..\..\..\WebKitLibraries\win\tools\vsprops\debug.vsprops;.\WebKit2Common.vsprops;.\WebKit2DirectX.vsprops;.\WebKit2Apple.vsprops"

Please undo these changes.

[Enrica Casucci]

(In reply to comment #18)
> (From update of attachment 81082 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81082&action=review
> 
> > Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:177
> > -    HGLOBAL cbData = ::GlobalAlloc(GPTR, size * sizeof(UChar));
> > +    HGLOBAL cbData = GlobalAlloc(GPTR, size * sizeof(UChar));
> 
> We've actually been going the other direction in WebKit2 code, so I'd just leave this alone.
>

I must have misunderstood a comment you made in a previous patch. I thought you  suggested not using ::.
 
> > Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:631
> > +class ClipboardDataItem {
> > +public:
> > +    FORMATETC* format;
> > +    GetStringFunction getString;
> > +    SetStringFunction setString;
> > +
> > +    ClipboardDataItem(FORMATETC* fmt, GetStringFunction f1, SetStringFunction f2): format(fmt), getString(f1), setString(f2) {};
> > +};
> 
> Is calling GlobalFree(0) OK?

yes, because passing FALSE makes a copy of the data.
 
> > Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:699
> > +    CString asciiString = dataStrings[0].utf8();
> 
> I'd move this closer to where it's first used.
> 
> "asciiString" isn't a good name for this; it's UTF-8, not ASCII! Is it OK to use UTF-8 for all the formats that end up calling this function? Maybe this function should be called setUtf8Data to match getUtf8Data (though I wish they were both "UTF8" instead of "Utf8").
>
Sure.

> > Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:715
> > +void setCfData(IDataObject* data, FORMATETC* format, const Vector<String>& dataStrings)
> > +{
> > +}
> 
> Do we expect this ever to be called? If not, adding an ASSERT_NOT_REACHED(), or just using a function pointer of 0, would be better.

I'll add that.

> This should really be named setCFData (and the getter should be named accordingly).
> 
> > Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:739
> > +    ClipboardFormatMap formatMap = getClipboardMap();
> 
> This copies the map. You should declare formatMap as a const ClipboardFormatMap&.

Overlooked.

> Don't we only need Read access here?

Yes.
 
> > Source/WebKit2/UIProcess/WebPageProxy.cpp:671
> > +    RefPtr<WebDragSource> source;
> > +    source = WebDragSource::createInstance(m_pageClient->nativeWindow());
> 
> This can be done in one line.
> 
> > Source/WebKit2/UIProcess/WebPageProxy.cpp:677
> > +    COMPtr<IDragSourceHelper> helper;
> > +    if (FAILED(CoCreateInstance(CLSID_DragDropHelper, 0, CLSCTX_INPROC_SERVER, IID_IDragSourceHelper, (LPVOID*)&helper)))
> > +        return;
> 
> COMPtr has a special constructor that can do this for you:
> 
> COMPtr<IDragSourceHelper> helper(Create, CLSID_DragDropHelper);
> if (!helper)
>     return;
> 
> This will only work if __uuidof(IDragSourceHelper) compiles, which it may not depending on how that interface is defined.
> 
> If you use POINT above, I don't think you'll need these IntPoint constructors. You can convert from POINT to IntPoint implicitly.
I didn't know.
 
> > Source/WebKit2/WebProcess/WebCoreSupport/win/WebDragClientWin.cpp:1
> > +/*
> 
> I'd really like to see this code shared with WebKit1. This is a lot of code to copy!

Is there a good way to share code with WebKit1? Probably the only reasonable solution is to place it in WebCore.
 
> We've tended not to use RefCounted for implementing COM classes, but doing so seems OK.

Why implementing refcounting when we have a class that does that? I did this on purpose.
The WebKit1 implementation did not.

Even though I agree with most of your comments I honestly don't understand the r-.
I'll make the changes and submit a new patch.

[Adam Roben (:aroben)]

(In reply to comment #19)
> (In reply to comment #18)
> > (From update of attachment 81082 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=81082&action=review
> > 
> > > Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:631
> > > +class ClipboardDataItem {
> > > +public:
> > > +    FORMATETC* format;
> > > +    GetStringFunction getString;
> > > +    SetStringFunction setString;
> > > +
> > > +    ClipboardDataItem(FORMATETC* fmt, GetStringFunction f1, SetStringFunction f2): format(fmt), getString(f1), setString(f2) {};
> > > +};
> > 
> > Is calling GlobalFree(0) OK?
> 
> yes, because passing FALSE makes a copy of the data.

(Looks like the wrong part of the patch got quoted here.)

Why does copying the data or not matter? I'm asking what happens if you pass null to GlobalFree, which your code could do in some cases.

> 
> > > Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:699
> > > +    CString asciiString = dataStrings[0].utf8();
> > 
> > I'd move this closer to where it's first used.
> > 
> > "asciiString" isn't a good name for this; it's UTF-8, not ASCII! Is it OK to use UTF-8 for all the formats that end up calling this function? Maybe this function should be called setUtf8Data to match getUtf8Data (though I wish they were both "UTF8" instead of "Utf8").
> >
> Sure.

…So is it OK to use UTF-8 for all the formats that end up calling this function? Is that what Windows expects?

> > > Source/WebKit2/WebProcess/WebCoreSupport/win/WebDragClientWin.cpp:1
> > > +/*
> > 
> > I'd really like to see this code shared with WebKit1. This is a lot of code to copy!
> 
> Is there a good way to share code with WebKit1? Probably the only reasonable solution is to place it in WebCore.

Moving the code to WebCore seems like the right option. You can just take the code that WebKit1's WebDragClient.cpp uses to draw the images, put it in some new functions in WebCore, and make both WebKit1 and WebKit2 call those new functions. Those seem like pretty small changes to me.

> > We've tended not to use RefCounted for implementing COM classes, but doing so seems OK.
> 
> Why implementing refcounting when we have a class that does that? I did this on purpose.
> The WebKit1 implementation did not.

Right, I was pointing out that this was a change from how we've historically done things. But I think it's probably a good change. :-)

> Even though I agree with most of your comments I honestly don't understand the r-.
> I'll make the changes and submit a new patch.

The r- was because of all the copied code. We don't want to have to maintain two implementations of the same thing, and copying code makes for many opportunities for bugs to creep in.

[Enrica Casucci]

Created attachment 81503 [details]
Patch3

Addressing all the comments and adding refactoring to avoid duplicate code for WebKit 1 and 2.

[WebKit Review Bot]

Attachment 81503 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7705752

[Early Warning System Bot]

Attachment 81503 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7708458

[Adam Roben (:aroben)]

Comment on attachment 81503 [details]
Patch3

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

One way to break this patch up into smaller pieces (which would make it easier to review would be):

1) Move the implementation of WebChromeClient::createDragImageForLink on Windows into the new WebCore::createDragImageForLink function, and make WebChromeClient::createDragImageForLink call it in WebKit1 on Windows.
2) Make WebChromeClient::createDragImagForLink call WebCore::createDragImageForLink in WebKit2 on Windows.
3) Repeat (1) for Mac.
4) Repeat (2) for Mac.
5) Wait for other platforms to do the same…
6) Remove DragClient::createDragImageForLink and change all callers to call WebCore::createDragImageForLink instead.

Doing it this way would not require all platforms to change at once, which would make your life easier, as well as the life of your friendly reviewer. :-)

Presumably this is going to require some non-trivial changes for Qt and Chromium (and probably other ports like GTK, too), so I guess I should wait until that's done. Or you could take the approach I outlined above and avoid all that extra work.

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:93
> -    HDROP hdrop = static_cast<HDROP>(GlobalLock(medium.hGlobal));
> +    HDROP hdrop = static_cast<HDROP>(::GlobalLock(medium.hGlobal));

We generally like to make style-only changes like this separately from substantive code changes. We do like to fix up old code's style as we go, but we don't normally do it for a whole file when we're just touching a couple of functions. It can be easy to get lost in all the style changes and miss the important code changes when reviewing.

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:705
> +    CString charString = dataStrings.first().utf8();
> +    size_t stringLength = charString.length();
> +    medium.hGlobal = ::GlobalAlloc(GPTR, stringLength + 1);
> +    if (!medium.hGlobal)
> +        return;
> +    char* buffer = static_cast<char*>(::GlobalLock(medium.hGlobal));
> +    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?

[Adam Roben (:aroben)]

(In reply to comment #24)
> (From update of attachment 81503 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81503&action=review
> 
> One way to break this patch up into smaller pieces (which would make it easier to review would be):
> 
> 1) Move the implementation of WebChromeClient::createDragImageForLink on Windows into the new WebCore::createDragImageForLink function, and make WebChromeClient::createDragImageForLink call it in WebKit1 on Windows.
> 2) Make WebChromeClient::createDragImagForLink call WebCore::createDragImageForLink in WebKit2 on Windows.
> 3) Repeat (1) for Mac.
> 4) Repeat (2) for Mac.
> 5) Wait for other platforms to do the same…
> 6) Remove DragClient::createDragImageForLink and change all callers to call WebCore::createDragImageForLink instead.
> 
> Doing it this way would not require all platforms to change at once, which would make your life easier, as well as the life of your friendly reviewer. :-)

To be clear, I meant that each step would be a separate patch (except for (5), which of course would require multiple patches, but other people would take care of that).

[Adam Roben (:aroben)]

Comment on attachment 81503 [details]
Patch3

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).

[Enrica Casucci]

Created attachment 81546 [details]
Patch4

This patch fixes the build breaks. I want to verify that before I submit.
I'm requesting another review because I found a problem with the code that has been already reviewed.
I've added the implementation of setCFData in ClipboardUtilitiesWin.cpp.
The rest of the code has been reviewed and r+ already, but I don't want to check in code that has not been reviewed 100%.

[Darin Adler]

Comment on attachment 81546 [details]
Patch4

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

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

What guarantees the dataStrings vector is not empty?

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:717
> +    SIZE_T dropFilesSize = sizeof(DROPFILES) + (sizeof(WCHAR) * (dataStrings.first().length() + 2));

+2? Why not +1?

> Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:726
> +    String filename = dataStrings[0];
> +    wcscpy(reinterpret_cast<LPWSTR>(dropFiles + 1), filename.charactersWithNullTermination());    

No need for the local variable here for dataStrings[0]. Also, other functions call this dataStrings.first() but here you used dataStrings[0].

[Enrica Casucci]

(In reply to comment #28)
> (From update of attachment 81546 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81546&action=review
> > Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:714
> > +void setCFData(IDataObject* data, FORMATETC* format, const Vector<String>& dataStrings)
> What guarantees the dataStrings vector is not empty?
This function is called to recreate an IDataObject from the serialized data and we call it only if there is data to deserialize, that is the vector is not empty.

> > Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:717
> > +    SIZE_T dropFilesSize = sizeof(DROPFILES) + (sizeof(WCHAR) * (dataStrings.first().length() + 2));
> +2? Why not +1?
I took this code from ClipboardWin.cpp (line 240 etc.) and didn't make too much sense to me either. I didn't want to change it because sometimes in Windows when dealing with an array of null terminated strings the double 0 indicates the end of the series. That is the only possible explanation I have.

> > Source/WebCore/platform/win/ClipboardUtilitiesWin.cpp:726
> > +    String filename = dataStrings[0];
> > +    wcscpy(reinterpret_cast<LPWSTR>(dropFiles + 1), filename.charactersWithNullTermination());    
> No need for the local variable here for dataStrings[0]. Also, other functions call this dataStrings.first() but here you used dataStrings[0].
You're right about the use of first.

[Enrica Casucci]

Committed revision 77870.

[WebKit Review Bot]

http://trac.webkit.org/changeset/77870 might have broken Leopard Intel Release (Build)

[Ryosuke Niwa]

Build fix in http://trac.webkit.org/changeset/77888.

I still don't understand why we added webkit_CGCeiling when there's an identical ceilCGFloat in ComplexTestController.cpp