[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 #202787|review?                     |review+
               Flag|                            |




--- Comment #15 from Darin Adler <darin at apple.com>  2013-05-26 18:01:50 PST ---
(From update of attachment 202787)
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(clipboardData.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!

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