Bug 116258 – [WIN] Move Windows port off legacy clipboard

• Component: Platform
• Status: RESOLVED FIXED

[huangxueqing]

See master bug #115980

[huangxueqing]

Created attachment 202490 [details]
patch

Sorry for huge patch, It's difficult to split since splited patch will make some tests failed.

[WebKit Commit Bot]

Attachment 202490 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/dom/Clipboard.h', u'Source/WebCore/page/win/EventHandlerWin.cpp', u'Source/WebCore/platform/Pasteboard.h', u'Source/WebCore/platform/win/ClipboardWin.cpp', u'Source/WebCore/platform/win/ClipboardWin.h', u'Source/WebCore/platform/win/EditorWin.cpp', u'Source/WebCore/platform/win/PasteboardWin.cpp', u'Source/WebKit/win/ChangeLog', u'Source/WebKit/win/WebCoreSupport/WebDragClient.cpp']" exit_code: 1
Source/WebCore/platform/win/PasteboardWin.cpp:114:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/platform/win/PasteboardWin.cpp:115:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/platform/win/PasteboardWin.cpp:298:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/platform/win/PasteboardWin.cpp:569:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebCore/platform/win/EditorWin.cpp:60:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebCore/platform/Pasteboard.h:110:  The parameter name "dataObject" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/Pasteboard.h:111:  The parameter name "dataObject" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/Pasteboard.h:112:  The parameter name "dataMap" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/Pasteboard.h:169:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/Pasteboard.h:170:  The parameter name "dataObject" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/Pasteboard.h:170:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/Pasteboard.h:171:  The parameter name "element" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/Pasteboard.h:171:  The parameter name "url" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/Pasteboard.h:171:  The parameter name "frame" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/Pasteboard.h:171:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 15 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[huangxueqing]

Created attachment 202491 [details]
patch

[huangxueqing]

Created attachment 202492 [details]
patch

[huangxueqing]

Darin, do you know who can review this patch?

[Darin Adler]

I’ll review this. I’ve just been really busy this week.

[Darin Adler]

Comment on attachment 202492 [details]
patch

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?

[huangxueqing]

Created attachment 202780 [details]
patch

[huangxueqing]

(In reply to comment #7)
> (From update of attachment 202492 [details])
> 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.
> 
Done

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

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

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

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

> > Source/WebCore/platform/win/PasteboardWin.cpp:111
> > +// static
> 
> We don't put in these kinds of comments in WebKit coding style.
> 
Done

> > Source/WebCore/platform/win/PasteboardWin.cpp:130
> > +void Pasteboard::createPasteboardPrivate()
> 
> I’d call this finishCreatingPasteboard.
> 
Done

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

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

[WebKit Commit Bot]

Attachment 202780 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/dom/Clipboard.h', u'Source/WebCore/page/win/EventHandlerWin.cpp', u'Source/WebCore/platform/Pasteboard.h', u'Source/WebCore/platform/win/ClipboardWin.cpp', u'Source/WebCore/platform/win/ClipboardWin.h', u'Source/WebCore/platform/win/EditorWin.cpp', u'Source/WebCore/platform/win/PasteboardWin.cpp', u'Source/WebKit/win/ChangeLog', u'Source/WebKit/win/WebCoreSupport/WebDragClient.cpp']" exit_code: 1
Source/WebKit/win/WebCoreSupport/WebDragClient.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[huangxueqing]

Created attachment 202784 [details]
patch

Fix style check failed.

[huangxueqing]

Created attachment 202785 [details]
patch

[WebKit Commit Bot]

Attachment 202785 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/dom/Clipboard.h', u'Source/WebCore/page/win/EventHandlerWin.cpp', u'Source/WebCore/platform/Pasteboard.h', u'Source/WebCore/platform/win/ClipboardWin.cpp', u'Source/WebCore/platform/win/ClipboardWin.h', u'Source/WebCore/platform/win/EditorWin.cpp', u'Source/WebCore/platform/win/PasteboardWin.cpp', u'Source/WebKit/win/ChangeLog', u'Source/WebKit/win/WebCoreSupport/WebDragClient.cpp']" exit_code: 1
Source/WebKit/win/WebCoreSupport/WebDragClient.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[huangxueqing]

Created attachment 202787 [details]
patch

[Darin Adler]

Comment on attachment 202787 [details]
patch

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!

[huangxueqing]

Created attachment 202948 [details]
patch

[huangxueqing]

(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

[WebKit Commit Bot]

Comment on attachment 202948 [details]
patch

Clearing flags on attachment: 202948

Committed r150772: <http://trac.webkit.org/changeset/150772>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.