[Webkit-unassigned] [Bug 116258] [WIN] Move Windows port off legacy clipboard

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #202492|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #7 from Darin Adler <darin at apple.com>  2013-05-23 19:09:41 PST ---
(From update of attachment 202492)
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(clipboardData.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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.