Bug 44962

Summary: deduplicate code from Node::checkReplaceChild and Node::checkAddChild
Product: WebKit Reporter: Ojan Vafai <ojan>
Component: New BugsAssignee: Ojan Vafai <ojan>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, darin, eric, tony
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch tony: review+

Description Ojan Vafai 2010-08-31 10:18:01 PDT
deduplicate code from Node::checkReplaceChild and Node::checkAddChild
Comment 1 Ojan Vafai 2010-08-31 10:20:12 PDT
Created attachment 66068 [details]
Patch
Comment 2 Eric Seidel (no email) 2010-08-31 10:29:36 PDT
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?
Comment 3 Ojan Vafai 2010-08-31 10:31:55 PDT
(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.
Comment 4 Ojan Vafai 2010-08-31 10:33:57 PDT
Created attachment 66069 [details]
Patch
Comment 5 Ojan Vafai 2010-08-31 10:36:28 PDT
As a side-by-side diff: http://codereview.appspot.com/2046045/
Comment 6 Tony Chang 2010-08-31 10:43:08 PDT
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.
Comment 7 Eric Seidel (no email) 2010-08-31 10:43:38 PDT
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.
Comment 8 Eric Seidel (no email) 2010-08-31 10:44:54 PDT
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.
Comment 9 Ojan Vafai 2010-08-31 10:48:36 PDT
(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.
Comment 10 Eric Seidel (no email) 2010-08-31 10:54:28 PDT
(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.
Comment 11 Ojan Vafai 2010-08-31 11:03:51 PDT
> Yes, but it's an enum.  You can use Node::

Oh, duh. Will do.
Comment 12 Ojan Vafai 2010-08-31 11:22:39 PDT
Committed r66511: <http://trac.webkit.org/changeset/66511>