Bug 120088

Summary: Define Clipboard::hasData() only when building with drag support
Product: WebKit Reporter: Daniel Bates <dbates>
Component: WebCore Misc.Assignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, esprehn+autocc, kangil.han
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch darin: review+

Description Daniel Bates 2013-08-20 14:13:34 PDT
Following up from Darin Adler's remark in comment 11 bug #119949, define Clipboard::hasData() only when building with drag support enabled.
Comment 1 Daniel Bates 2013-08-20 14:20:21 PDT
Created attachment 209229 [details]
Patch
Comment 2 Daniel Bates 2013-08-20 14:22:06 PDT
Should we also add a ENABLE(DRAG_SUPPORT) guard around Pasteboard::hasData()?
Comment 3 Daniel Bates 2013-08-20 14:26:44 PDT
Would it be better to move the definition of hasData() to another place in the file so as to avoid mixing it with the Clipboard IDL functions and/or to more closely match its position in Clipboard.h? Having said that, I like that hasData() is grouped with logically related functions.
Comment 4 Darin Adler 2013-08-20 14:41:11 PDT
Comment on attachment 209229 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209229&action=review

> Source/WebCore/dom/Clipboard.cpp:116
> +#if ENABLE(DRAG_SUPPORT)
>  bool Clipboard::hasData()
>  {
>      return m_pasteboard->hasData();
>  }
> +#endif

I suggest moving it down to the DRAG_SUPPORT part of the file. I chose that over littering the file with #if statements.

Note that this function does *not* belong in the same family as functions like getData; if it was suitable for exposure to the web as getData is, it would have to check canReadData and return false unconditionally in that case. It’s really a function just for internal use.

> Source/WebCore/dom/Clipboard.h:85
>          static PassRefPtr<Clipboard> createForDragAndDrop();
>          static PassRefPtr<Clipboard> createForDragAndDrop(ClipboardAccessPolicy, const DragData&);
> +        bool hasData();

Should not be grouped with the create functions in a single paragraph. Just include another blank line, please.
Comment 5 Daniel Bates 2013-08-20 14:48:32 PDT
(In reply to comment #4)
> > Source/WebCore/dom/Clipboard.cpp:116
> > +#if ENABLE(DRAG_SUPPORT)
> >  bool Clipboard::hasData()
> >  {
> >      return m_pasteboard->hasData();
> >  }
> > +#endif
> 
> I suggest moving it down to the DRAG_SUPPORT part of the file. I chose that over littering the file with #if statements.
> [...]

Will move definition to the DRAG_SUPPORT part of the file.

> > Source/WebCore/dom/Clipboard.h:85
> >          static PassRefPtr<Clipboard> createForDragAndDrop();
> >          static PassRefPtr<Clipboard> createForDragAndDrop(ClipboardAccessPolicy, const DragData&);
> > +        bool hasData();
> 
> Should not be grouped with the create functions in a single paragraph. Just include another blank line, please.

Will add empty line above hasData() declaration.
Comment 6 Daniel Bates 2013-08-20 14:51:48 PDT
Committed r154360: <http://trac.webkit.org/changeset/154360>
Comment 7 Darin Adler 2013-08-20 15:05:01 PDT
After you landed this, I figured out what I really want to do (and will do soon). Just have the caller get at the pasteboard directly instead of having this forwarding function. So I’ll be removing this function entirely.

Thanks!