Summary: | Change Clipboard to use an enum instead of isForDragging = true/false | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Cheng <dcheng> | ||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, commit-queue, dbates, eric, gustavo, levin, tony, webkit-ews, webkit.review.bot, xan.lopez | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | OS X 10.5 | ||||||||||
Attachments: |
|
Description
Daniel Cheng
2010-09-17 15:26:35 PDT
Created attachment 67962 [details]
Patch
Flagging for R? so the EWS bots can try out the patch first.
This is great. I will r+ cq+ as soon as the ews bots go green (I think r+'ing cancels the ews run). I prefer enums over booleans. Is it proper to call these drag-drop and copy-paste instead of drag-and-drop and copy-and-paste? Hence, name the enum value DragAndDrop and CopyAndPaste. (In reply to comment #3) > I prefer enums over booleans. Is it proper to call these drag-drop and copy-paste instead of drag-and-drop and copy-and-paste? Hence, name the enum value DragAndDrop and CopyAndPaste. I like this idea. I'll update the patch once I get back EWS results. Attachment 67962 [details] did not build on gtk: Build output: http://queues.webkit.org/results/4071054 Attachment 67962 [details] did not build on qt: Build output: http://queues.webkit.org/results/4066060 Attachment 67962 [details] did not build on win: Build output: http://queues.webkit.org/results/4034072 Created attachment 68114 [details]
Patch
Fix the compile errors and rename CopyPaste/DragDrop to CopyAndPaste/DragAndDrop.
Comment on attachment 68114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=68114&action=review > WebCore/ChangeLog:8 > + No new tests. (OOPS!) This needs to be replaced with an explanation. Either: 1. Name the test(s), 2. Describe why a test isn't possible, or 3. Indicate that no new functionality is exposed so no new test is needed. > WebCore/dom/Clipboard.h:96 > + Clipboard(ClipboardAccessPolicy, ClipboardType clipboardType); Parameter name "clipboardType" shouldn't be here. > WebCore/platform/chromium/ClipboardChromium.cpp:207 > // If this isn't for a drag, it's for a cut/paste event handler. It looks like this comment isn't needed anymore (but if you remove it, you need to update the next comment line slightly). > WebCore/platform/chromium/ClipboardChromium.cpp:223 > // If this isn't for a drag, it's for a cut/paste event handler. Ditto. > WebCore/platform/mac/ClipboardMac.h:82 > + ClipboardMac(ClipboardType clipboardType, NSPasteboard *, ClipboardAccessPolicy, Frame*); Param name should go away. > WebCore/platform/win/ClipboardWin.h:81 > + ClipboardWin(ClipboardType clipboardType, IDataObject*, ClipboardAccessPolicy, Frame*); > + ClipboardWin(ClipboardType clipboardType, WCDataObject*, ClipboardAccessPolicy, Frame*); Param name should go away. > WebCore/platform/wx/ClipboardWx.h:68 > + ClipboardWx(ClipboardAccessPolicy, ClipboardType clipboardType); Param name should go away. (In reply to comment #9) > (From update of attachment 68114 [details]) Basically consider this an r- (but I want to make sure the ews runs finish.) Created attachment 68194 [details] Patch (In reply to comment #9) > (From update of attachment 68114 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=68114&action=review > > > WebCore/ChangeLog:8 > > + No new tests. (OOPS!) > > This needs to be replaced with an explanation. Either: > 1. Name the test(s), > 2. Describe why a test isn't possible, or > 3. Indicate that no new functionality is exposed so no new test is needed. > Done. > > WebCore/dom/Clipboard.h:96 > > + Clipboard(ClipboardAccessPolicy, ClipboardType clipboardType); > > Parameter name "clipboardType" shouldn't be here. > Done. > > WebCore/platform/chromium/ClipboardChromium.cpp:207 > > // If this isn't for a drag, it's for a cut/paste event handler. > > It looks like this comment isn't needed anymore (but if you remove it, you need to update the next comment line slightly). > > > WebCore/platform/chromium/ClipboardChromium.cpp:223 > > // If this isn't for a drag, it's for a cut/paste event handler. > > Ditto. Done. > > > WebCore/platform/mac/ClipboardMac.h:82 > > + ClipboardMac(ClipboardType clipboardType, NSPasteboard *, ClipboardAccessPolicy, Frame*); > > Param name should go away. > Done. > > WebCore/platform/win/ClipboardWin.h:81 > > + ClipboardWin(ClipboardType clipboardType, IDataObject*, ClipboardAccessPolicy, Frame*); > > + ClipboardWin(ClipboardType clipboardType, WCDataObject*, ClipboardAccessPolicy, Frame*); > > Param name should go away. > Done. > > WebCore/platform/wx/ClipboardWx.h:68 > > + ClipboardWx(ClipboardAccessPolicy, ClipboardType clipboardType); > > Param name should go away. Done. Comment on attachment 68194 [details] Patch Clearing flags on attachment: 68194 Committed r67973: <http://trac.webkit.org/changeset/67973> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/67973 might have broken Qt Windows 32-bit Debug The following changes are on the blame list: http://trac.webkit.org/changeset/67973 http://trac.webkit.org/changeset/67974 http://trac.webkit.org/changeset/67975 http://trac.webkit.org/changeset/67976 http://trac.webkit.org/changeset/67977 http://trac.webkit.org/changeset/67978 http://trac.webkit.org/changeset/67979 |