[webkit-reviews] review denied: [Bug 62112] Context-aware HTML paste for Chromium : [Attachment 110971] 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 110971: Patch
https://bugs.webkit.org/attachment.cgi?id=110971&action=review

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


> Source/WebCore/ChangeLog:10
> +	   Covered by existing layout tests.

No rebaselines?

> Source/WebCore/editing/markup.cpp:431
> +bool isSpeciallyStyledBlock(const Node* node)

The word "specially" doesn't add any semantics here. I would merge this
function and isHTMLBlockElement, and make it take an enum that indicates
whether we include table cell (td/th) or not.

Alternatively, we can name this isNonTableCellHTMLBlockElement.

> Source/WebCore/editing/markup.cpp:444
> +	   || node->hasTagName(h6Tag);

Let's not add h6 now. We need a test for this. r- because of this.

> Source/WebCore/editing/markup.cpp:449
> +    return node->hasTagName(tdTag)

You're missing th here.

> Source/WebCore/editing/markup.cpp:450
> +	   || node->hasTagName(trTag)

I don't think we should have tr here.

> Source/WebCore/editing/markup.cpp:451
> +	   || node->hasTagName(blockquoteTag)

blockquote probably belongs to isSpeciallyStyledBlock but let's not make that
change either. We need a test.

> Source/WebCore/editing/markup.cpp:455
> +// FIXME: Do we want to handle mail quotes here instead? It might be
useful...

Please remove "It might be useful..." It doesn't add any semantics.

> Source/WebCore/editing/markup.cpp:456
> +// This is currently handled one level about
ancestorToRetainStructureAndAppearance.

I don't understand this comment.

> Source/WebCore/editing/markup.cpp:481
> +static Node* ancestorToRetainStructureAndAppearance(Node* commonAncestor)
> +{
> +    return
ancestorToRetainStructureAndAppearanceForBlock(enclosingBlock(commonAncestor));

> +}
> +
> +static Node* ancestorToRetainStructureAndAppearanceWithNoRenderer(Node*
commonAncestor)

Please add inline keywords to these functions.

> Source/WebCore/editing/markup.cpp:725
> +    // We move the resulting DOM tree into a Document so we can create a
proper Range.

This comment seems unnecessary.

> Source/WebCore/editing/markup.cpp:747
> +    for (Node* node = taggedDocument->firstChild(); node; node =
node->traverseNextNode()) {
> +	   if (node->nodeType() == Node::COMMENT_NODE &&
static_cast<CharacterData*>(node)->data() == fragmentMarkerTag) {
> +	       if (!nodeBeforeContext)
> +		   nodeBeforeContext = node;
> +	       else {
> +		   nodeAfterContext = node;
> +		   break;
> +	       }
> +	   }
> +    }
> +
> +    if (!(nodeBeforeContext && nodeAfterContext))
> +	   return 0;
> +
> +    RefPtr<Range> range = Range::create(taggedDocument.get(),
> +	  
positionAfterNode(nodeBeforeContext.get()).parentAnchoredEquivalent(),
> +	  
positionBeforeNode(nodeAfterContext.get()).parentAnchoredEquivalent());

We can probably extract this segment as a function.

> Source/WebCore/editing/markup.cpp:754
> +    // Set up the initial range we're interested in.

This comment repeats what the code says. Please remove.

> Source/WebCore/editing/markup.cpp:760
> +	   fragment->appendChild(specialCommonAncestor, ec);
> +	   ASSERT(!ec);
> +    } else
> +	  
fragment->takeAllChildrenFrom(static_cast<ContainerNode*>(commonAncestor));

I would rather see a comment on why we call appendChild for
specialCommonAncestor and takeAllChildrenFrom for commonAncestor.

> Source/WebCore/editing/markup.cpp:779
> +    Node* next;
> +    for (RefPtr<Node> node = fragment->firstChild(); node; node = next) {
> +	   if (nodeBeforeContext->isDescendantOf(node.get())) {
> +	       next = node->traverseNextNode();
> +	       continue;
> +	   }
> +	   next = node->traverseNextSibling();
> +	   node->parentNode()->removeChild(node.get(), ec);
> +	   if (nodeBeforeContext == node)
> +	       break;
> +    }
> +
> +    ASSERT(nodeAfterContext->parentNode());
> +    for (Node* node = nodeAfterContext.get(); node; node = next) {
> +	   next = node->traverseNextSibling();
> +	   node->parentNode()->removeChild(node, ec);
> +	   ASSERT(!ec);
> +    }

I feel like this should be extracted as a helper function.