120088 2013-08-20 14:13:34 -0700 Define Clipboard::hasData() only when building with drag support 2013-08-20 15:05:01 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 dbates dbates commit-queue darin esprehn+autocc kangil.han oldest_to_newest 919225 0 dbates 2013-08-20 14:13:34 -0700 Following up from Darin Adler's remark in comment 11 bug #119949, define Clipboard::hasData() only when building with drag support enabled. 919229 1 209229 dbates 2013-08-20 14:20:21 -0700 Created attachment 209229 Patch 919230 2 dbates 2013-08-20 14:22:06 -0700 Should we also add a ENABLE(DRAG_SUPPORT) guard around Pasteboard::hasData()? 919233 3 dbates 2013-08-20 14:26:44 -0700 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. 919241 4 209229 darin 2013-08-20 14:41:11 -0700 Comment on attachment 209229 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. 919243 5 dbates 2013-08-20 14:48:32 -0700 (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. 919247 6 dbates 2013-08-20 14:51:48 -0700 Committed r154360: <http://trac.webkit.org/changeset/154360> 919249 7 darin 2013-08-20 15:05:01 -0700 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! 209229 2013-08-20 14:20:21 -0700 2013-08-20 14:41:11 -0700 Patch bug-120088-20130820142021.patch text/plain 2291 dbates U3VidmVyc2lvbiBSZXZpc2lvbjogMTU0MzUwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNzEzMTdiZWQ1YmMwNGIw YzRmY2Y3MTY0ODE3NDM1MTJkNjdiMzQzOC4uMzcwMWI5N2JkZmJlNGE2MTE5MDg1OTgxOWQzOTE3 NzNmNDg3Mzc2YyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDE3IEBACiAyMDEzLTA4LTIwICBEYW5p ZWwgQmF0ZXMgIDxkYWJhdGVzQGFwcGxlLmNvbT4KIAorICAgICAgICA8aHR0cHM6Ly93ZWJraXQu b3JnL2IvMTIwMDg4PiBEZWZpbmUgQ2xpcGJvYXJkOjpoYXNEYXRhKCkgb25seSB3aGVuIGJ1aWxk aW5nIHdpdGggZHJhZyBzdXBwb3J0CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BT ISkuCisKKyAgICAgICAgQ2xpcGJvYXJkOjpoYXNEYXRhKCkgaXMgc3BlY2lmaWMgdG8gZHJhZy1h bmQtZHJvcCBzdXBwb3J0LiBXZSBzaG91bGQgb25seSBkZWZpbmUgaXQgd2hlbgorICAgICAgICBz dWNoIHN1cHBvcnQgaXMgZW5hYmxlZC4KKworICAgICAgICAqIGRvbS9DbGlwYm9hcmQuY3BwOiBB ZGQgRU5BQkxFKERSQUdfU1VQUE9SVCkgZ3VhcmQgYXJvdW5kIGhhc0RhdGEoKSBpbXBsZW1lbnRh dGlvbi4KKyAgICAgICAgKiBkb20vQ2xpcGJvYXJkLmg6IE1vdmUgaGFzRGF0YSgpIHRvIGRyYWcg c2VjdGlvbiBvZiBmaWxlLgorCisyMDEzLTA4LTIwICBEYW5pZWwgQmF0ZXMgIDxkYWJhdGVzQGFw cGxlLmNvbT4KKwogICAgICAgICBCdWlsZCBmaXggYWZ0ZXIgPGh0dHA6Ly90cmFjLndlYmtpdC5v cmcvY2hhbmdlc2V0LzE1NDI2MD4gKGh0dHBzOi8vd2Via2l0Lm9yZy9iLzExOTk0OSk7CiAgICAg ICAgIGRlY2xhcmUgQ2xpcGJvYXJkOjpoYXNEYXRhKCkgd2hlbiBidWlsZGluZyB3aXRoIGFuZCB3 aXRob3V0IGRyYWcgc3VwcG9ydAogCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9kb20vQ2xp cGJvYXJkLmNwcCBiL1NvdXJjZS9XZWJDb3JlL2RvbS9DbGlwYm9hcmQuY3BwCmluZGV4IDAwZDlk OGE2ZGNlMDZjZDg4MGQ5MzZlYjYyYjM4NDhkZjgxZjI2OTEuLjlhM2Q1NjQ5NDkwNTliNmZiYTBm OTMzODhiM2Q5YzUzODFkMGNhY2MgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2RvbS9DbGlw Ym9hcmQuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL2RvbS9DbGlwYm9hcmQuY3BwCkBAIC0xMDgs MTAgKzEwOCwxMiBAQCBib29sIENsaXBib2FyZDo6Y2FuV3JpdGVEYXRhKCkgY29uc3QKICAgICBy ZXR1cm4gbV9wb2xpY3kgPT0gQ2xpcGJvYXJkV3JpdGFibGU7CiB9CiAKKyNpZiBFTkFCTEUoRFJB R19TVVBQT1JUKQogYm9vbCBDbGlwYm9hcmQ6Omhhc0RhdGEoKQogewogICAgIHJldHVybiBtX3Bh c3RlYm9hcmQtPmhhc0RhdGEoKTsKIH0KKyNlbmRpZgogCiB2b2lkIENsaXBib2FyZDo6Y2xlYXJE YXRhKGNvbnN0IFN0cmluZyYgdHlwZSkKIHsKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2Rv bS9DbGlwYm9hcmQuaCBiL1NvdXJjZS9XZWJDb3JlL2RvbS9DbGlwYm9hcmQuaAppbmRleCA2OTZk MTgxZmE3ZGI4NGIxMTJhY2FhMDA0NTQ4ZWMyMDdiZGJkNmViLi5mYWI5ODMxYjQ3YThjOTM0ZGQz ZDM0MjM1OTM5MzQ4ODMxMmQ4Y2FjIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9kb20vQ2xp cGJvYXJkLmgKKysrIGIvU291cmNlL1dlYkNvcmUvZG9tL0NsaXBib2FyZC5oCkBAIC01OSw4ICs1 OSw2IEBAIG5hbWVzcGFjZSBXZWJDb3JlIHsKIAogICAgICAgICBQYXNzUmVmUHRyPEZpbGVMaXN0 PiBmaWxlcygpIGNvbnN0OwogCi0gICAgICAgIGJvb2wgaGFzRGF0YSgpOwotCiAgICAgICAgIHZv aWQgY2xlYXJEYXRhKGNvbnN0IFN0cmluZyYgdHlwZSk7CiAgICAgICAgIHZvaWQgY2xlYXJEYXRh KCk7CiAKQEAgLTg0LDYgKzgyLDcgQEAgbmFtZXNwYWNlIFdlYkNvcmUgewogI2lmIEVOQUJMRShE UkFHX1NVUFBPUlQpCiAgICAgICAgIHN0YXRpYyBQYXNzUmVmUHRyPENsaXBib2FyZD4gY3JlYXRl Rm9yRHJhZ0FuZERyb3AoKTsKICAgICAgICAgc3RhdGljIFBhc3NSZWZQdHI8Q2xpcGJvYXJkPiBj cmVhdGVGb3JEcmFnQW5kRHJvcChDbGlwYm9hcmRBY2Nlc3NQb2xpY3ksIGNvbnN0IERyYWdEYXRh Jik7CisgICAgICAgIGJvb2wgaGFzRGF0YSgpOwogCiAgICAgICAgIGJvb2wgZHJvcEVmZmVjdElz VW5pbml0aWFsaXplZCgpIGNvbnN0IHsgcmV0dXJuIG1fZHJvcEVmZmVjdCA9PSAidW5pbml0aWFs aXplZCI7IH0KIAo=