Bug 120088 - Define Clipboard::hasData() only when building with drag support
Summary: Define Clipboard::hasData() only when building with drag support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-08-20 14:13 PDT by Daniel Bates
Modified: 2013-08-20 15:05 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.24 KB, patch)
2013-08-20 14:20 PDT, Daniel Bates
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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!