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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied 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 202492: patch
https://bugs.webkit.org/attachment.cgi?id=202492&action=review

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


Great patch! Definitely a step in the right direction. A few things that need
fixing, but I think it’s getting close!

> Source/WebCore/page/win/EventHandlerWin.cpp:46
> +#if !USE(LEGACY_STYLE_ABSTRACT_CLIPBOARD_CLASS)

Should not use this #if in this file, since it’s only used for PLATFORM(WIN).
Should just keep the !USE(LEGACY_STYLE_ABSTRACT_CLIPBOARD_CLASS) and delete the
other side.

> Source/WebCore/platform/Pasteboard.h:72
> +#if PLATFORM(WIN)
> +class Element;
> +#endif

Should not put an #if around a forward declaration. Just leave the #if off.

> Source/WebCore/platform/Pasteboard.h:171
> +    void declareAndWriteDragImage(Element*, const KURL&, const String&,
Frame*);

This function does not belong here. Pasteboard should expose what’s needed to
implement this, but this function should stay elsewhere. I think
DragControllerWin is the right file for this or we could leave it in
ClipboardWin for now.

> Source/WebCore/platform/win/EditorWin.cpp:30
> +#if !USE(LEGACY_STYLE_ABSTRACT_CLIPBOARD_CLASS)

Should not use this #if in this file, since it’s only used for PLATFORM(WIN).
Should just keep the !USE(LEGACY_STYLE_ABSTRACT_CLIPBOARD_CLASS) and delete the
other side.

> Source/WebCore/platform/win/EditorWin.cpp:62
> +    Pasteboard* pasteboard =
const_cast<Pasteboard*>(&clipboard->pasteboard());
> +    pasteboard->setExternalDataObject(clipboardData.get());

Ultimately this const_cast should not be needed. There’s some kind of design
mistake that makes it needed for now. But no need for the pointer, this should
be:

   
const_cast<Pasteboard&>(clipboard->pasteboard()).setExternalDataObject(clipboar
dData.get());

> Source/WebCore/platform/win/PasteboardWin.cpp:111
> +// static

We don't put in these kinds of comments in WebKit coding style.

> Source/WebCore/platform/win/PasteboardWin.cpp:130
> +void Pasteboard::createPasteboardPrivate()

I’d call this finishCreatingPasteboard.

> Source/WebKit/win/WebCoreSupport/WebDragClient.cpp:34
> +#if !USE(LEGACY_STYLE_ABSTRACT_CLIPBOARD_CLASS)

Should not use this #if in this file, since it’s only used for PLATFORM(WIN).
Should just keep the !USE(LEGACY_STYLE_ABSTRACT_CLIPBOARD_CLASS) and delete the
other side.

> Source/WebKit/win/WebCoreSupport/WebDragClient.cpp:117
> +    Pasteboard* pasteboard =
const_cast<Pasteboard*>(&clipboard->pasteboard());
> +    COMPtr<IDataObject> dataObject = pasteboard->dataObject();

Doesn’t make sense that you have to const_cast here. Can’t dataObject be a
const member function?