Bug 46004 – Change Clipboard to use an enum instead of isForDragging = true/false

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Daniel Cheng]

The bool parameter can get hard to follow, and leads to mistakes--I fixed one probable error while updating the various platforms to use the enum instead of the bool.

[Daniel Cheng]

Created attachment 67962 [details]
Patch

Flagging for R? so the EWS bots can try out the patch first.

[Tony Chang]

This is great.  I will r+ cq+ as soon as the ews bots go green (I think r+'ing cancels the ews run).

[Daniel Bates]

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.

[Daniel Cheng]

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

[WebKit Review Bot]

Attachment 67962 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4071054

[Early Warning System Bot]

Attachment 67962 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4066060

[WebKit Review Bot]

Attachment 67962 [details] did not build on win:
Build output: http://queues.webkit.org/results/4034072

[Daniel Cheng]

Created attachment 68114 [details]
Patch

Fix the compile errors and rename CopyPaste/DragDrop to CopyAndPaste/DragAndDrop.

[David Levin]

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.

[David Levin]

(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.)

[Daniel Cheng]

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.

[WebKit Commit Bot]

Comment on attachment 68194 [details]
Patch

Clearing flags on attachment: 68194

Committed r67973: <http://trac.webkit.org/changeset/67973>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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