Right now, the various Clipboard implementations contain a lot of platform-specific details. We should create a new platform object (like DragData) to abstract away the interactions with platform data.
Created attachment 58540 [details] Patch
Attachment 58540 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 Webcore/platform/ClipboardData.h:1: One or more unexpected \r (^M) found; better to use only a \n [whitespace/carriage_return] [1] Suppressing further [whitespace/carriage_return] reports for this file. Webcore/platform/ClipboardData.cpp:1: One or more unexpected \r (^M) found; better to use only a \n [whitespace/carriage_return] [1] Suppressing further [whitespace/carriage_return] reports for this file. Total errors found: 145 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 58540 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/3183339
Attachment 58540 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/3198315
Created attachment 58547 [details] Patch
Created attachment 58548 [details] Patch Fix bad merge.
Attachment 58547 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/3200329
Attachment 58548 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/3249326
Created attachment 58549 [details] Patch Fix build break (paths had incorrect case)
Attachment 58547 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/3186342
Darin, do you mind reviewing this patch? https://bugs.webkit.org/show_bug.cgi?id=40455 isn't complete, but it shows how this proposed patch would be used.
Comment on attachment 58549 [details] Patch > +class ClipboardData : public RefCounted<ClipboardData> { This seems fine, but I'm not sure why it's an abstract base class. Generally speaking the platform directory does not use runtime factories that vend objects of abstract base classes. I have seen other libraries that use this approach, but it has runtime costs that we prefer not to pay in WebKit. The interface here looks generally OK in terms of what the functions are and the argument types, but the basic approach does not seem right. An example of how we do this without virtual functions is in the FileChooser.h source file. Not sure that needs to be a reference-counted object, but otherwise it's a reasonable example. DragData.h is another example, although a particularly messy one.
(In reply to comment #12) > (From update of attachment 58549 [details]) > > +class ClipboardData : public RefCounted<ClipboardData> { > > This seems fine, but I'm not sure why it's an abstract base class. Generally speaking the platform directory does not use runtime factories that vend objects of abstract base classes. I have seen other libraries that use this approach, but it has runtime costs that we prefer not to pay in WebKit. > > The interface here looks generally OK in terms of what the functions are and the argument types, but the basic approach does not seem right. > > An example of how we do this without virtual functions is in the FileChooser.h source file. Not sure that needs to be a reference-counted object, but otherwise it's a reasonable example. DragData.h is another example, although a particularly messy one. The interface is virtual because there are two implementations for a given platform: 1. One implementation that lives under platform. This provides a read-only implementation of ClipboardData which wraps accessing native copy/paste and drag/drop data. 2. Another common implementation that lives in a location TBD. It is primarily intended for writing (in the cut/copy/dragstart events), though you can read from it as well. The reason for creating two separate implementations like this is because I found the way the current Clipboard worked to be a little wonky: - When starting drags, platform implementations of Clipboard simply allow data members to be set. At some point, DragClient::startDrag is called and the implementation atomically creates a data object and sets all data that will be in that drag. - For copy/cut operations, Clipboard immediately writes back to the clipboard, so Clipboard contents won't be set atomically. Thus, we end up with Clipboard implementations that look like: if (m_isForDragging) { m_foo = bar; } else { // FIXME: Add Pasteboard support. } This is true for Windows WebKit, Chromium WebKit, and probably a number of other platforms as well. - If writing drag data and copy data are abstracted by one generic object, then Clipboard becomes much simpler. Persisting data to the clipboard might look like this instead: if (clipboard.isDirty()) { Pasteboard::clear(); Pasteboard::writeData(clipboard); } This also has the bonus side effect that preventing a copy or cut no longer clears the Pasteboard contents, which follows IE behavior.
Created attachment 113377 [details] Patch
Darin, do you mind taking another look at this? We'll need this change to fix https://bugs.webkit.org/show_bug.cgi?id=34288, so that simply preventing the default action without setting any data doesn't cause the clipboard to get cleared on copy/cut.
I've completely rewritten this patch, and I think it should address previous review concerns to this patch. There is no more ClipboardData interface, so if you could take another look, that'd be great.
Comment on attachment 113377 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113377&action=review > Source/WebCore/editing/Editor.cpp:743 > + if (noDefaultProcessing && policy == ClipboardWritable) { > + Pasteboard* pasteboard = Pasteboard::generalPasteboard(); > + pasteboard->clear(); > + pasteboard->writeClipboard(clipboard.get()); > + } If the actual pasteboard processing is done in this function, then the name dispatchCPPEvent is inappropriate. Is that something new to this patch, or has it always been true.
(In reply to comment #17) > (From update of attachment 113377 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=113377&action=review > > > Source/WebCore/editing/Editor.cpp:743 > > + if (noDefaultProcessing && policy == ClipboardWritable) { > > + Pasteboard* pasteboard = Pasteboard::generalPasteboard(); > > + pasteboard->clear(); > > + pasteboard->writeClipboard(clipboard.get()); > > + } > > If the actual pasteboard processing is done in this function, then the name dispatchCPPEvent is inappropriate. Is that something new to this patch, or has it always been true. This has always been true, but the pasteboard mutation wasn't as obvious before--it was happening inside Clipboard::setData().
(In reply to comment #17) > (From update of attachment 113377 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=113377&action=review > > > Source/WebCore/editing/Editor.cpp:743 > > + if (noDefaultProcessing && policy == ClipboardWritable) { > > + Pasteboard* pasteboard = Pasteboard::generalPasteboard(); > > + pasteboard->clear(); > > + pasteboard->writeClipboard(clipboard.get()); > > + } > > If the actual pasteboard processing is done in this function, then the name dispatchCPPEvent is inappropriate. Is that something new to this patch, or has it always been true. I thought about ways to split up dispatchCPPEvent to move the pasteboard processing out of dispatchCPPEvent, but I can't think of any good way without duplicating some of the boilerplate to set up the clipboard event and fire it. What makes dispatchCPPEvent an inappropriate name, and do you have suggestions on how this can be addressed?
Comment on attachment 113377 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113377&action=review > Source/WebCore/ChangeLog:8 > + Add a Pasteboard::writeClipboard() method which is roughly analogous to Part of this should be the comment for dispatchCPPEvent. > Source/WebCore/ChangeLog:12 > + Covered by existing tests. It is only covered by existing tests if 1. It is only a refactoring (but that is not the case here as I see new function calls). 2. An existing test is broken and this fixes it, but I don't see a new baseline for a test. So what is testing these new api calls?/Why are they needed? > Source/WebCore/ChangeLog:15 > + (WebCore::Editor::tryDHTMLCopy): Ideally your changelog would have an explanation of what was done in each function. For example, it would say "Move clear call to dispatchCPPEvent" here. Think of this like a breadcrumb trail for the reviewer so they don't need to piece as much together. > Source/WebCore/ChangeLog:16 > + (WebCore::Editor::tryDHTMLCut): Here "Ditto" >>>> Source/WebCore/editing/Editor.cpp:743 >>>> + } >>> >>> If the actual pasteboard processing is done in this function, then the name dispatchCPPEvent is inappropriate. Is that something new to this patch, or has it always been true. >> >> This has always been true, but the pasteboard mutation wasn't as obvious before--it was happening inside Clipboard::setData(). > > I thought about ways to split up dispatchCPPEvent to move the pasteboard processing out of dispatchCPPEvent, but I can't think of any good way without duplicating some of the boilerplate to set up the clipboard event and fire it. What makes dispatchCPPEvent an inappropriate name, and do you have suggestions on how this can be addressed? It dispatches the event on line 737 but then this stuff is extra. Please describe what this function does and then we can find a name. > Source/WebCore/editing/mac/EditorMac.mm:52 > + policy == ClipboardWritable ? [NSPasteboard pasteboardWithUniqueName] : [NSPasteboard generalPasteboard], policy, frame); The changelog should tell me why this change is being done. > Source/WebCore/platform/mac/PasteboardMac.mm:333 > + } Why is this code needed now? (Hint: The ChangeLog should tell me.) > Source/WebCore/platform/qt/PasteboardQt.cpp:179 > + static_cast<ClipboardQt*>(clipboard)->clipboardData()); No need for a carriage return here. What does this fix? (Hint: I should know due to the ChangeLog.) > Source/WebCore/platform/wince/PasteboardWinCE.cpp:39 > +#include "NotImplemented.h" Why include this header and not use notImplemented?
Created attachment 114391 [details] Patch
Created attachment 114592 [details] Patch
Comment on attachment 113377 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113377&action=review >> Source/WebCore/ChangeLog:8 >> + Add a Pasteboard::writeClipboard() method which is roughly analogous to > > Part of this should be the comment for dispatchCPPEvent. Done. >> Source/WebCore/ChangeLog:12 >> + Covered by existing tests. > > It is only covered by existing tests if > 1. It is only a refactoring (but that is not the case here as I see new function calls). > 2. An existing test is broken and this fixes it, but I don't see a new baseline for a test. > > So what is testing these new api calls?/Why are they needed? Done. >> Source/WebCore/ChangeLog:15 >> + (WebCore::Editor::tryDHTMLCopy): > > Ideally your changelog would have an explanation of what was done in each function. > > For example, it would say "Move clear call to dispatchCPPEvent" here. Think of this like a breadcrumb trail for the reviewer so they don't need to piece as much together. Done. >> Source/WebCore/ChangeLog:16 >> + (WebCore::Editor::tryDHTMLCut): > > Here "Ditto" Done. >>>>> Source/WebCore/editing/Editor.cpp:743 >>>> >>>> If the actual pasteboard processing is done in this function, then the name dispatchCPPEvent is inappropriate. Is that something new to this patch, or has it always been true. >>> >>> This has always been true, but the pasteboard mutation wasn't as obvious before--it was happening inside Clipboard::setData(). >> >> I thought about ways to split up dispatchCPPEvent to move the pasteboard processing out of dispatchCPPEvent, but I can't think of any good way without duplicating some of the boilerplate to set up the clipboard event and fire it. What makes dispatchCPPEvent an inappropriate name, and do you have suggestions on how this can be addressed? > > It dispatches the event on line 737 but then this stuff is extra. Please describe what this function does and then we can find a name. Done... I think. >> Source/WebCore/editing/mac/EditorMac.mm:52 >> + policy == ClipboardWritable ? [NSPasteboard pasteboardWithUniqueName] : [NSPasteboard generalPasteboard], policy, frame); > > The changelog should tell me why this change is being done. Done. >> Source/WebCore/platform/mac/PasteboardMac.mm:333 >> + } > > Why is this code needed now? (Hint: The ChangeLog should tell me.) Done. >> Source/WebCore/platform/qt/PasteboardQt.cpp:179 >> + static_cast<ClipboardQt*>(clipboard)->clipboardData()); > > No need for a carriage return here. > > What does this fix? (Hint: I should know due to the ChangeLog.) Done. >> Source/WebCore/platform/wince/PasteboardWinCE.cpp:39 >> +#include "NotImplemented.h" > > Why include this header and not use notImplemented? Done.
Comment on attachment 114592 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=114592&action=review Just one question before I r+. > Source/WebCore/platform/gtk/PasteboardGtk.cpp:138 > + static_cast<ClipboardGtk*>(clipboard)->clipboard()); Why did it check m_clipboard before but now it doesn't?
Comment on attachment 114592 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=114592&action=review >> Source/WebCore/platform/gtk/PasteboardGtk.cpp:138 >> + static_cast<ClipboardGtk*>(clipboard)->clipboard()); > > Why did it check m_clipboard before but now it doesn't? Because m_clipboard was only accessible to ClipboardGtk. I added an accessor for it so we can use it in PasteboardGtk as well.
(In reply to comment #25) > (From update of attachment 114592 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=114592&action=review > > >> Source/WebCore/platform/gtk/PasteboardGtk.cpp:138 > >> + static_cast<ClipboardGtk*>(clipboard)->clipboard()); > > > > Why did it check m_clipboard before but now it doesn't? > > Because m_clipboard was only accessible to ClipboardGtk. I added an accessor for it so we can use it in PasteboardGtk as well. I'll be more verbose. Prior code: if (success && m_clipboard) PasteboardHelper::defaultPasteboardHelper()->writeClipboardContents(m_clipboard); Why was the prior code checking that m_clipboard wasn't NULL but the current code uses the equivalent "static_cast<ClipboardGtk*>(clipboard)->clipboard()" without doing a NULL check: PasteboardHelper::defaultPasteboardHelper()->writeClipboardContents( static_cast<ClipboardGtk*>(clipboard)->clipboard()); If you answered this just now, I didn't understand the answer.
Created attachment 114607 [details] Patch
Comment on attachment 114592 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=114592&action=review >>>> Source/WebCore/platform/gtk/PasteboardGtk.cpp:138 >>>> + static_cast<ClipboardGtk*>(clipboard)->clipboard()); >>> >>> Why did it check m_clipboard before but now it doesn't? >> >> Because m_clipboard was only accessible to ClipboardGtk. I added an accessor for it so we can use it in PasteboardGtk as well. > > I'll be more verbose. > > Prior code: > if (success && m_clipboard) > PasteboardHelper::defaultPasteboardHelper()->writeClipboardContents(m_clipboard); > > Why was the prior code checking that m_clipboard wasn't NULL but the current code uses the equivalent "static_cast<ClipboardGtk*>(clipboard)->clipboard()" without doing a NULL check: > > PasteboardHelper::defaultPasteboardHelper()->writeClipboardContents( > static_cast<ClipboardGtk*>(clipboard)->clipboard()); > > If you answered this just now, I didn't understand the answer. Good point. Fixed.
Committed r99924: <http://trac.webkit.org/changeset/99924>
Why did this test land without any tests? This patch changes the timing at which copy/cut/paste events are dispatched and therefore need to be tested.
(In reply to comment #30) > Why did this patch land without any tests? This patch changes the timing at which copy/cut/paste events are dispatched and therefore need to be tested. On second thought, this might be okay since this change only affects copy and cut both of which can't read clipboard data
(In reply to comment #31) > (In reply to comment #30) > > Why did this patch land without any tests? This patch changes the timing at which copy/cut/paste events are dispatched and therefore need to be tested. > > On second thought, this might be okay since this change only affects copy and cut both of which can't read clipboard data This was a mistake on my part. I thought that pasteboard tests that interacted with the system clipboard would be inherently flaky due to the global nature of the clipboard, but it appears we correctly handle that case today. I am working on followup tests and will be landing them in a separate patch.