[webkit-reviews] review granted: [Bug 116258] [WIN] Move Windows port off legacy clipboard : [Attachment 202787] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted huangxueqing
<huangxueqing at baidu.com>'s request for review:
Bug 116258: [WIN] Move Windows port off legacy clipboard
https://bugs.webkit.org/show_bug.cgi?id=116258

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

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


> Source/WebCore/ChangeLog:8
> +	   Tests:

I’d leave this line out.

> Source/WebCore/ChangeLog:9
> +	   Clipboard refactor, no new tests.

It’s OK to have a line like this explaining why we have no new tests.

> Source/WebCore/ChangeLog:16
> +	   (WebCore):
> +	   (Pasteboard):

Unpleasant lines like these should be removed from the change log. They are
just signs of bugs in the prepare-ChangeLog script.

> Source/WebCore/ChangeLog:26
> +	   (WebCore):

Ditto.

> Source/WebCore/platform/Pasteboard.h:42
> +#include <WebCore/COMPtr.h>
> +#include <WebCore/WCDataObject.h>

Includes inside WebCore from files inside WebCore are normally done like this:

    #include "COMPtr.h"
    #include "WCDataObject.h"

Is there some reason we need to do <WebCore/xxx.h> here?

> Source/WebCore/platform/Pasteboard.h:111
> +#if PLATFORM(WIN)
> +    explicit Pasteboard(IDataObject*);
> +    explicit Pasteboard(WCDataObject*);
> +    explicit Pasteboard(const DragDataMap&);
> +#endif

Do these constructors need to be public? Can we keep them private instead since
they are called only by other Pasteboard functions?

> Source/WebCore/platform/win/EditorWin.cpp:52
> +   
const_cast<Pasteboard&>(clipboard->pasteboard()).setExternalDataObject(clipboar
dData.get());

I think the OldGetClipboard code should go inside
Pasteboard::createForCopyAndPaste() and not here. Eventually if not in this
patch.

> Source/WebCore/platform/win/PasteboardWin.cpp:106
> +    // Windows has not "Private pasteboard" concept.

The grammar here is not right. Probably should replace the word “not” here with
the word “no”.

> Source/WebCore/platform/win/PasteboardWin.cpp:123
> +    // We could not call DragData::dragDataMap via |const DragData&|.

This comment seems confusing to me. The issue is that DragData::dragDataMap
needs a const version that returns a const& result. We should fix that.

> Source/WebCore/platform/win/PasteboardWin.cpp:125
> +    const DragDataMap& dragDataMap =
const_cast<DragData*>((&dragData))->dragDataMap();
> +    return adoptPtr(new Pasteboard(dragDataMap));

If we need the const cast here, it can still be written simpler in one line
without all the parentheses and uses of pointers:

    // FIXME: Should add a const overload of dragDataMap so we don’t need a
const_cast here.
    return adoptPtr(new
Pasteboard(const_cast<DragData&>(dragData).dragDataMap()));

> Source/WebCore/platform/win/PasteboardWin.cpp:187
> +    String qType = type.stripWhiteSpace().lower();

The name “qType” sure seems strange here, but I suppose best to not change
this.

> Source/WebCore/platform/win/PasteboardWin.cpp:396
> +    if (winType == ClipboardDataTypeURL)
> +	   return WebCore::writeURL(m_writableDataObject.get(),
KURL(ParsedURLString, data), String(), false, true);

This is not a correct nor safe use of ParsedURLString. It should be
KURL(KURL(), data) instead. Just a bug in the code we are moving, but still not
good!

> Source/WebCore/platform/win/PasteboardWin.cpp:415
> +void Pasteboard::setDragImage(DragImageRef, const IntPoint& hotSpot)

Should omit the argument name hotSpot to possibly avoid unused argument
warnings in future versions of the compiler.

> Source/WebCore/platform/win/PasteboardWin.cpp:565
> +	   KURL kurl(ParsedURLString, url);

This is not a correct nor safe use of ParsedURLString. It should be KURL
kurl(KURL(), url) instead. Just a bug in the code we are moving, but still not
good!