Summary: | deduplicate code from Node::checkReplaceChild and Node::checkAddChild | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ojan Vafai <ojan> | ||||||
Component: | New Bugs | Assignee: | 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
Ojan Vafai
2010-08-31 10:18:01 PDT
Created attachment 66068 [details]
Patch
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? (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. Created attachment 66069 [details]
Patch
As a side-by-side diff: http://codereview.appspot.com/2046045/ 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. 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 on attachment 66069 [details] Patch > WebCore/dom/Node.cpp:1076 > + if (child->nodeType() != parent->DOCUMENT_FRAGMENT_NODE) { You still don't need parent-> here. (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. (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. > Yes, but it's an enum. You can use Node::
Oh, duh. Will do.
Committed r66511: <http://trac.webkit.org/changeset/66511> |