See master bug #115980
Created attachment 202490 [details] patch Sorry for huge patch, It's difficult to split since splited patch will make some tests failed.
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.
Created attachment 202491 [details] patch
Created attachment 202492 [details] patch
Darin, do you know who can review this patch?
I’ll review this. I’ve just been really busy this week.
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?
Created attachment 202780 [details] patch
(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
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.
Created attachment 202784 [details] patch Fix style check failed.
Created attachment 202785 [details] patch
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.
Created attachment 202787 [details] patch
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!
Created attachment 202948 [details] patch
(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
Comment on attachment 202948 [details] patch Clearing flags on attachment: 202948 Committed r150772: <http://trac.webkit.org/changeset/150772>
All reviewed patches have been landed. Closing bug.