Bug 44962 – deduplicate code from Node::checkReplaceChild and Node::checkAddChild

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ojan Vafai]

deduplicate code from Node::checkReplaceChild and Node::checkAddChild

[Ojan Vafai]

Created attachment 66068 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 66068 [details]
Patch

> WebCore/dom/Node.cpp:1076
> +    if (child->nodeType() != parent->DOCUMENT_FRAGMENT_NODE) {
parent-> isn't needed here.

I would have made the fragment case an early return instead.

It's really hard to tell what this diff is doing.  I know it's small, but maybe it could be smallr?

[Ojan Vafai]

(In reply to comment #2)
> (From update of attachment 66068 [details])
> > WebCore/dom/Node.cpp:1076
> > +    if (child->nodeType() != parent->DOCUMENT_FRAGMENT_NODE) {
> parent-> isn't needed here.
> 
> I would have made the fragment case an early return instead.

I was just copy-pasting the code. But I agree. I'll make it an early return.

> It's really hard to tell what this diff is doing.  I know it's small, but maybe it could be smallr?

Meh. This is really a small change. We just really need side-by-side diffs.

[Ojan Vafai]

Created attachment 66069 [details]
Patch

[Ojan Vafai]

As a side-by-side diff: http://codereview.appspot.com/2046045/

[Tony Chang]

Comment on attachment 66069 [details]
Patch

> diff --git a/WebCore/dom/Node.cpp b/WebCore/dom/Node.cpp
> -bool Node::canReplaceChild(Node* newChild, Node*)
> +static bool isChildTypeAllowed(Node* parent, Node* child)

Nit: Perhaps newParent or newChild to make it clear that child->parent() != parent?

The side by side diff was much easier to read.

[Eric Seidel (no email)]

That link only helps a little. this si still *ridiculously* confusing. :(  The names are all getting changed around. They were similar to begin with, so that didn't help.

[Eric Seidel (no email)]

Comment on attachment 66069 [details]
Patch

> WebCore/dom/Node.cpp:1076
> +    if (child->nodeType() != parent->DOCUMENT_FRAGMENT_NODE) {
You still don't need parent-> here.

[Ojan Vafai]

(In reply to comment #8)
> (From update of attachment 66069 [details])
> > WebCore/dom/Node.cpp:1076
> > +    if (child->nodeType() != parent->DOCUMENT_FRAGMENT_NODE) {
> You still don't need parent-> here.

Really? It didn't compile without it.

[Eric Seidel (no email)]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 66069 [details] [details])
> > > WebCore/dom/Node.cpp:1076
> > > +    if (child->nodeType() != parent->DOCUMENT_FRAGMENT_NODE) {
> > You still don't need parent-> here.
> 
> Really? It didn't compile without it.

Yes, but it's an enum.  You can use Node::

OK.  I've read through the diff again.  I believe Ojan has correctly moved code/not-changed-behavior now.

[Ojan Vafai]

> Yes, but it's an enum.  You can use Node::

Oh, duh. Will do.

[Ojan Vafai]

Committed r66511: <http://trac.webkit.org/changeset/66511>