Bug 119949 - Factor Clipboard into drag and non-drag parts
Summary: Factor Clipboard into drag and non-drag parts
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks: 115980
  Show dependency treegraph
 
Reported: 2013-08-17 08:38 PDT by Darin Adler
Modified: 2013-08-20 14:23 PDT (History)
13 users (show)

See Also:


Attachments
Patch (27.08 KB, patch)
2013-08-17 08:50 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (27.01 KB, patch)
2013-08-17 09:21 PDT, Darin Adler
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2013-08-17 08:38:02 PDT
Factor Clipboard into drag and non-drag parts
Comment 1 Darin Adler 2013-08-17 08:50:57 PDT
Created attachment 208998 [details]
Patch
Comment 2 Early Warning System Bot 2013-08-17 08:58:31 PDT
Comment on attachment 208998 [details]
Patch

Attachment 208998 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1465996
Comment 3 Early Warning System Bot 2013-08-17 08:59:29 PDT
Comment on attachment 208998 [details]
Patch

Attachment 208998 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1493097
Comment 4 EFL EWS Bot 2013-08-17 09:01:07 PDT
Comment on attachment 208998 [details]
Patch

Attachment 208998 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1433573
Comment 5 EFL EWS Bot 2013-08-17 09:09:13 PDT
Comment on attachment 208998 [details]
Patch

Attachment 208998 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/1465995
Comment 6 Build Bot 2013-08-17 09:17:18 PDT
Comment on attachment 208998 [details]
Patch

Attachment 208998 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1498129
Comment 7 kov's GTK+ EWS bot 2013-08-17 09:19:44 PDT
Comment on attachment 208998 [details]
Patch

Attachment 208998 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/1493098
Comment 8 Darin Adler 2013-08-17 09:21:09 PDT
Created attachment 209000 [details]
Patch
Comment 9 Darin Adler 2013-08-18 21:17:22 PDT
Committed r154260: <http://trac.webkit.org/changeset/154260>
Comment 10 Daniel Bates 2013-08-20 12:59:14 PDT
(In reply to comment #9)
> Committed r154260: <http://trac.webkit.org/changeset/154260>

This change breaks all builds that disable drag support, such as the iOS WebKit build (*), because we only declare Clipboard::hasData() when building with drag support enabled, but always define Clipboard::hasData() (since its implementation isn't surrounded by a "ENABLE(DRAG_SUPPORT)" guard).

I landed a build fix that defines Clipboard::hasData() when building with and without drag support in <http://trac.webkit.org/changeset/154350>.

Although Clipboard::hasData() is only used in DragController, I chose this approach as opposed to adding an ENABLE(DRAG_SUPPORT) guard around the implementation of Clipboard::hasData() (and the declaration and definition for the Pasteboard method of the same name) because Clipboard exposes other methods for interacting with the underlying pasteboard data when building without drag support, including Clipboard::{clear, get, set}Data() and it seemed strange to segregate hasData() from these other methods based on the availability of drag support. Let me know if we should segregate hasData() from the other Clipboard::*Data methods or if there is a better approach to fixing the build for ports that don't enable drag support.

(*) For completeness, the following is the error we get when we build the iOS port with this change:

Source/WebCore/dom/Clipboard.cpp:111:17: error: out-of-line definition of 'hasData' does not match any declaration in 'WebCore::Clipboard'
Comment 11 Darin Adler 2013-08-20 13:04:19 PDT
(In reply to comment #10)
> Although Clipboard::hasData() is only used in DragController, I chose this approach as opposed to adding an ENABLE(DRAG_SUPPORT) guard around the implementation of Clipboard::hasData() (and the declaration and definition for the Pasteboard method of the same name) because Clipboard exposes other methods for interacting with the underlying pasteboard data when building without drag support

I would have done it the other way, fixing the .cpp file and putting the hasData function in the section for drag support. I am trying to thin out this class. I don’t want it to have lots of functions for logical completeness; I want it to be as small as possible.
Comment 12 Daniel Bates 2013-08-20 14:23:00 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > Although Clipboard::hasData() is only used in DragController, I chose this approach as opposed to adding an ENABLE(DRAG_SUPPORT) guard around the implementation of Clipboard::hasData() (and the declaration and definition for the Pasteboard method of the same name) because Clipboard exposes other methods for interacting with the underlying pasteboard data when building without drag support
> 
> I would have done it the other way, fixing the .cpp file and putting the hasData function in the section for drag support. I am trying to thin out this class. I don’t want it to have lots of functions for logical completeness; I want it to be as small as possible.

Filed bug #120088 to only declare and define Clipboard::hasData() when building with drag support.