[webkit-reviews] review denied: [Bug 62112] Context-aware HTML paste for Chromium : [Attachment 109919] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Daniel Cheng <dcheng at chromium.org>'s
request for review:
Bug 62112: Context-aware HTML paste for Chromium
https://bugs.webkit.org/show_bug.cgi?id=62112

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

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


> Source/WebCore/editing/markup.cpp:80
> +    static FrameLoaderClient* dummyFrameLoaderClient = new
EmptyFrameLoaderClient;

new needs to be wrapped by adoptPtr(~).leakPtr(). r- because of this.

> Source/WebCore/editing/markup.cpp:116
> +    loader->activeDocumentLoader()->writer()->setMIMEType("text/html");

Are you sure we can always assume the content is text/html? no application/xml?


> Source/WebCore/editing/markup.cpp:763
> +    taggedMarkup.append("<!--");
> +    taggedMarkup.append(fragmentMarkerTag);
> +    taggedMarkup.append("-->");
> +    taggedMarkup.append(markup.substring(fragmentStart, fragmentEnd -
fragmentStart));
> +    taggedMarkup.append("<!--");
> +    taggedMarkup.append(fragmentMarkerTag);
> +    taggedMarkup.append("-->");
> +    taggedMarkup.append(markup.substring(fragmentEnd));

It'll be really nice if we could share code with
MarkupAccumulator::appendComment.
http://trac.webkit.org/browser/trunk/Source/WebCore/editing/MarkupAccumulator.c
pp#L301

> Source/WebCore/editing/markup.cpp:778
> +	   if (node->nodeType() == Node::COMMENT_NODE) {
> +	       if (static_cast<CharacterData*>(node)->data() ==
fragmentMarkerTag) {

It seems like these conditions can be checked in one if-statement.

> Source/WebCore/editing/markup.cpp:791
> +	   Position(fragmentStartNode, Position::PositionIsAfterAnchor),
> +	   Position(fragmentEndNode, Position::PositionIsBeforeAnchor));

Oops, these lines are actually wrong. These positions aren't range-compliant.
Furthermore, you should make use of positionBeforeNode and positionAfterNode so
do:
positionAfterNode(fragmentStartNode).parentAnchoredEquivalent(),
positionBeforeNode(fragmentEndNode).parentAnchoredEquivalent(),