Use more references in ContainerNode code

Source/core/dom/Node.h

Index: Source/core/dom/Node.h
diff --git a/Source/core/dom/Node.h b/Source/core/dom/Node.h
index 44ee6c7d3ec37187e2de39f1588e487deb545d2f..fe25b609320dd08d8d336ef2693d7ed3ce95bc7a 100644
--- a/Source/core/dom/Node.h
+++ b/Source/core/dom/Node.h
@@ -500,7 +500,7 @@ public:
     bool isDescendantOf(const Node*) const;
     bool contains(const Node*) const;
     bool containsIncludingShadowDOM(const Node*) const;
-    bool containsIncludingHostElements(const Node*) const;
+    bool containsIncludingHostElements(const Node&) const;
 
     // FIXME: Remove this when crbug.com/265716 cleans up contains semantics.
     bool bindingsContains(const Node* node) const { return containsIncludingShadowDOM(node); }
@@ -921,10 +921,10 @@ inline bool operator==(const Node* a, const Node& b) { return a == &b; }
 inline bool operator!=(const Node& a, const Node& b) { return !(a == b); }
 inline bool operator!=(const Node& a, const Node* b) { return !(a == b); }
 inline bool operator!=(const Node* a, const Node& b) { return !(a == b); }
-inline bool operator==(const RefPtr<Node>& a, const Node& b) { return a.get() == &b; }
-inline bool operator==(const Node& a, const RefPtr<Node>& b) { return &a == b.get(); }
-inline bool operator!=(const RefPtr<Node>& a, const Node& b) { return !(a == b); }
-inline bool operator!=(const Node& a, const RefPtr<Node>& b) { return !(a == b); }
+inline bool operator==(const PassRefPtr<Node>& a, const Node& b) { return a.get() == &b; }
+inline bool operator==(const Node& a, const PassRefPtr<Node>& b) { return &a == b.get(); }
+inline bool operator!=(const PassRefPtr<Node>& a, const Node& b) { return !(a == b); }
+inline bool operator!=(const Node& a, const PassRefPtr<Node>& b) { return !(a == b); }

[Inactive, 2013/11/01 18:43:18]

This one cost me some time debugging. I was doing an (a == b) comparison with a
being a Node& and b being a PassRefPtr<Node> and the comparison operator would
silently construct a RefPtr from my PassRefPtr.

This might seem harmless but a side effect of this was that my PassRefPtr was
getting nullified by the operator==() :D

[adamk, 2013/11/01 18:56:50]

Yikes! This is why the WebKit style guide always recommended against doing
_anything_ with PassRefPtrs besides putting them in function argument lists.

I think I'd rather remove these overloads than change them to PassRefPtrs, as
that'll cause unnecessary refcount churn.

Any chance we can avoid the pitfall by providing both RefPtr and PassRefPtr
versions? Or does that confuse the compiler?

[Inactive, 2013/11/01 19:03:57]

Oh, I think we can provide both if you are worried about the cost of ref
counting. I hesitated doing that honestly.

[adamk, 2013/11/01 19:07:09]

I admit it seems unlikely for the cost to be noticeable, but my C++ instincts
are to avoid making operator overloads more expensive than the operation they're
trying to implement. Basically, I'd be sad if someone used this operator== in an
inner loop somewhere and got slowed down by the refcount churn.

[Inactive, 2013/11/01 19:39:01]

Sadly it does not build. It is ambiguous according to the compiler. Do you mind
if we keep it this way? I much prefer a RefPtr gets silently converted to a
PassRefPtr than the other way around :)

[adamk, 2013/11/01 20:10:54]

I think I'd actually prefer dropping the PassRefPtr overloads altogether and
just forcing the few callsites to use either .get() == or &ref ==. But I can
take care of that in a followup, I agree that this is safer.

Hopefully we'll at some point get rid of PassRefPtr altogether and then we'll
all be better off.

 
 
 #define DEFINE_NODE_TYPE_CASTS(thisType, predicate) \