[webkit-reviews] review denied: [Bug 74775] WebKit editing throws exception when monochrome color dragged onto text : [Attachment 120196] Patch for bug fix and patch for manual test case

Wed Dec 21 11:25:27 PST 2011

Ryosuke Niwa <rniwa at webkit.org> has denied Daniel Jalkut
<jalkut at red-sweater.com>'s request for review:
Bug 74775: WebKit editing throws exception when monochrome color dragged onto
text
https://bugs.webkit.org/show_bug.cgi?id=74775

Attachment 120196: Patch for bug fix and patch for manual test case
https://bugs.webkit.org/attachment.cgi?id=120196&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=120196&action=review

r- due to various nits.

> ChangeLog:3
> +	   Add manual test covering failure to accept grayscale color drags to
contentEditable regions.

This line should repeat what the bug summary.

> ManualTests/drag-color-to-contenteditable.html:3
> +<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN"
> +	   "http://www.w3.org/TR/html4/strict.dtd">
> +<html lang="en">

Can we use HTML5 style DOCTYPE?
<!DOCTYPE html>
<html>

> ManualTests/drag-color-to-contenteditable.html:5
> +<head>
> +</head>

We don't need head.

> ManualTests/drag-color-to-contenteditable.html:11
> +<li>Open a color panel in some host app that facilitates doing so, such as
TextEdit.app.</li>

I don't understand what you mean by "that facilitates doing so". I think it's
better to say "Open a color panel in some app such as TextEdit.app."

> Source/WebCore/ChangeLog:4
> +	   Handle non-RGB colorspace colors in the Mac platform drag manager.
Fixes NSException thrown 
> +	   when dragging monochrome colors to contentEditable regions.

This line should repeat what the bug summary. You can add a long description
below "Reviewed by" line followed by a blank line.

> Source/WebCore/platform/mac/DragDataMac.mm:121
> +    if ([[color colorSpace] colorSpaceModel] != NSRGBColorSpaceModel) {
> +	   color = [color colorUsingColorSpaceName:NSCalibratedRGBColorSpace];
> +    }

WebKit style is not to wrap single statement with { and }.