[webkit-reviews] review denied: [Bug 40515] Atomically update the system clipboard for copy/cut events : [Attachment 113377] Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Daniel Cheng
<dcheng at chromium.org>'s request for review:
Bug 40515: Atomically update the system clipboard for copy/cut events
https://bugs.webkit.org/show_bug.cgi?id=40515

Attachment 113377: Patch
https://bugs.webkit.org/attachment.cgi?id=113377&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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?