[webkit-reviews] review denied: [Bug 58043] Unify the way we generate HTML for an image in the Clipboard : [Attachment 103398] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 58043: Unify the way we generate HTML for an image in the Clipboard
https://bugs.webkit.org/show_bug.cgi?id=58043

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

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


The patch looks good overall but I'd like to see tests moved to
LayoutTests/editing/pasteboard because drag & drop really is an editing
feature.

> LayoutTests/ChangeLog:10
> +	   Add a new folder drag-and-drop for fast tests only focused on drag
and drop.
> +
> +	   Add 3 tests for dragging an image-like element to an editable area.

Please move these tests to LayoutTests/editing/pasteboard.

> LayoutTests/platform/qt/Skipped:275
> +fast/drag-and-drop

Really?  These tests fail on Qt?

> Source/WebCore/editing/MarkupAccumulator.cpp:129
> +    default:
> +	   break;
> +    }
> +    return urlString;

Please get rid of default and explicitly list all cases.  And please put
ASSERT_NOT_REACHED; after the switch statement.  This way, if we ever add new
resolve* in the future and forget to change this function, the compiler will
catch it.

>> Source/WebCore/editing/MarkupAccumulator.h:69
>> +	MarkupAccumulator(Vector<Node*>* nodes, EAbsoluteURLs
resolveUrlsMethod, const Range* range = 0);
> 
> The parameter name "range" adds no information, so it should be removed. 
[readability/parameter_name] [5]

Style bot is right.  Please get rid of nodes and range.