[webkit-reviews] review denied: [Bug 56330] [chromium] Implement glue between DataTransferItems and the pasteboard. : [Attachment 85715] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Daniel Cheng <dcheng at chromium.org>'s
request for review:
Bug 56330: [chromium] Implement glue between DataTransferItems and the
pasteboard.
https://bugs.webkit.org/show_bug.cgi?id=56330

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=85715&action=review

> Source/WebCore/ChangeLog:5
> +	   Need a short description and bug URL (OOPS!)

This needs to be filled out.

> Source/WebCore/ChangeLog:7
> +	   No new tests. (OOPS!)

We need tests for this change!

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:47
>  PassRefPtr<DataTransferItemChromium>
DataTransferItemChromium::create(PassRefPtr<Clipboard> owner,
>									
ScriptExecutionContext* context,
> +									
FromPasteboardType,
> +									 const
String& type) {

This should all be on one line.

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:49
> +    if (type == mimeTypeTextPlain ||
> +	   type == mimeTypeTextHTML) {

We'd usually just put these on one line.

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:51
> +    }

We don't need these braces.

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:108
> +    if (m_type == mimeTypeTextPlain) {
> +	   callback->scheduleCallback(m_context,
PlatformBridge::clipboardReadPlainText(PasteboardPrivate::StandardBuffer));
> +    } else if (m_type == mimeTypeTextHTML) {

No braces for one-line if bodies.  Also, prefer early return.

> Source/WebCore/platform/chromium/DataTransferItemChromium.cpp:120
> +    if ((m_owner->policy() != ClipboardReadable && m_owner->policy() !=
ClipboardWritable)
> +	   || m_kind != kindFile)
> +	   return 0;
> +    return 0;

Why are we returning zero in both cases?  That seems wrong...

> Source/WebKit/chromium/ChangeLog:5
> +	   Need a short description and bug URL (OOPS!)

This needs to be filled out.