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

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


The patch looks much nicer!

> Source/WebCore/editing/markup.cpp:460
> +static Node* ancestorToRetainStructureAndAppearance(Node* commonAncestor)

Please add inline keyword here.

> Source/WebCore/editing/markup.cpp:465
> +static Node* ancestorToRetainStructureAndAppearanceHACK(Node*
commonAncestor)

We definitely don't want the name HACK here. r- because of this.

> Source/WebCore/editing/markup.cpp:474
> +    if (commonAncestor->isTextNode())
> +	   return 0;
> +
> +    // FIXME: Do we want to handle mail quotes correctly? It might be
useful...
> +    // We cheat here since we don't have a render tree.
> +    if (commonAncestor->hasTagName(tdTag)
> +	   || commonAncestor->hasTagName(liTag))
> +	   commonAncestor = commonAncestor->parentNode();

My suggestion is to extract a function out of
ancestorToRetainStructureAndAppearanceForBlock (lines 444-454) and add td, tr,
table, blockquote to the list.
And call enclosingNodeOfType with that.

> Source/WebCore/editing/markup.cpp:736
> +    if (!startNode || !endNode)

You should also check that startNode and endNode have parents.

> Source/WebCore/editing/markup.cpp:757
> +    for (Node* n = fragment->firstChild(); n; n = next) {

Please don't use one letter abbreviation like n.

> Source/WebCore/editing/markup.cpp:759
> +	       next = n->traverseNextNode();

You probably want to call traverseNextSibling here.

> Source/WebCore/editing/markup.cpp:764
> +	   next = n->traverseNextNode();

Ditto.

> Source/WebCore/editing/markup.cpp:766
> +	   // FIXME: We're comparing a deleted pointer. It works, but it seems
a little dangerous.

You should make n ref-counted.

> Source/WebCore/editing/markup.cpp:770
> +    for (Node* n = endNode; n; n = next) {

Ditto about one letter variable.

> Source/WebCore/editing/markup.cpp:773
> +	   if (n->childNodeCount())
> +	       static_cast<ContainerNode*>(n)->removeAllChildren();
> +	   next = n->traverseNextNode();

No need to remove all children if you just call traverseNextSibling.