[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





--- Comment #17 from huangxueqing <huangxueqing at baidu.com>  2013-05-26 21:27:59 PST ---
(In reply to comment #15)
> (From update of attachment 202787 [details])
> 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.
> 
Done

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

> > 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?
> 
Done, just a copy and paste mistake.

> > 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?
> 
You are right, I had moved these constructors to private.

> > 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.
> 
Me too :-), I think we'd better file a new bug to fix this.

> > 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”.
> 
Done

> > 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()));
> 
Done

> > 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!
> 
Done

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

> > 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!
Done

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