Use more references in ContainerNode code

Source/core/dom/Node.h

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2001 Dirk Mueller (mueller@kde.org)
  * Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights reserved.
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved. (http://www.torchmobile.com/)
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
@@ skipping 482 matching lines @@
 
     bool isDocumentTypeNode() const { return nodeType() == DOCUMENT_TYPE_NODE; }
     virtual bool childTypeAllowed(NodeType) const { return false; }
     unsigned childNodeCount() const;
     Node* childNode(unsigned index) const;
 
     void checkSetPrefix(const AtomicString& prefix, ExceptionState&);
     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); }
 
     // Used to determine whether range offsets use characters or node indices.
     virtual bool offsetInCharacters() const;
     // Number of DOM 16-bit units contained in node. Note that rendered text length can be different - e.g. because of
     // css-transform:capitalize breaking up precomposed characters and ligatures.
     virtual int maxCharacterOffset() const;
 
@@ skipping 400 matching lines @@
     return change >= Inherit || node->childNeedsStyleRecalc() || node->needsStyleRecalc();
 }
 
 // Allow equality comparisons of Nodes by reference or pointer, interchangeably.
 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 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) \
     template<typename T> inline thisType* to##thisType(const RefPtr<T>& node) { return to##thisType(node.get()); } \
     DEFINE_TYPE_CASTS(thisType, Node, node, node->predicate, node.predicate)
 
 // This requires isClassName(const Node&).
 #define DEFINE_NODE_TYPE_CASTS_WITH_FUNCTION(thisType) \
     template<typename T> inline thisType* to##thisType(const RefPtr<T>& node) { return to##thisType(node.get()); } \
     DEFINE_TYPE_CASTS(thisType, Node, node, is##thisType(*node), is##thisType(node))
 
 } // namespace WebCore
 
 #ifndef NDEBUG
 // Outside the WebCore namespace for ease of invocation from gdb.
 void showTree(const WebCore::Node*);
 void showNodePath(const WebCore::Node*);
 #endif
 
 #endif