Use more references in ContainerNode code

Use more references in ContainerNode code

Use more references in ContainerNode code instead of pointers when they cannot
be null. This makes the code look safer and gets rid of unnecessary null
checks.

R=adamk

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161202

[Inactive, 2013-11-01 18:43:17]

https://codereview.chromium.org/54273007/diff/50001/Source/core/dom/Node.h
File Source/core/dom/Node.h (right):

https://codereview.chromium.org/54273007/diff/50001/Source/core/dom/Node.h#ne...
Source/core/dom/Node.h:927: inline bool operator!=(const Node& a, const
PassRefPtr<Node>& b) { return !(a == b); }
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]

https://codereview.chromium.org/54273007/diff/50001/Source/core/dom/Node.h
File Source/core/dom/Node.h (right):

https://codereview.chromium.org/54273007/diff/50001/Source/core/dom/Node.h#ne...
Source/core/dom/Node.h:927: inline bool operator!=(const Node& a, const
PassRefPtr<Node>& b) { return !(a == b); }
On 2013/11/01 18:43:18, Chris Dumez wrote:
> 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

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]

https://codereview.chromium.org/54273007/diff/50001/Source/core/dom/Node.h
File Source/core/dom/Node.h (right):

https://codereview.chromium.org/54273007/diff/50001/Source/core/dom/Node.h#ne...
Source/core/dom/Node.h:927: inline bool operator!=(const Node& a, const
PassRefPtr<Node>& b) { return !(a == b); }
On 2013/11/01 18:56:50, adamk wrote:
> On 2013/11/01 18:43:18, Chris Dumez wrote:
> > 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
> 
> 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?

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]

https://codereview.chromium.org/54273007/diff/50001/Source/core/dom/Node.h
File Source/core/dom/Node.h (right):

https://codereview.chromium.org/54273007/diff/50001/Source/core/dom/Node.h#ne...
Source/core/dom/Node.h:927: inline bool operator!=(const Node& a, const
PassRefPtr<Node>& b) { return !(a == b); }
On 2013/11/01 19:03:57, Chris Dumez wrote:
> On 2013/11/01 18:56:50, adamk wrote:
> > On 2013/11/01 18:43:18, Chris Dumez wrote:
> > > 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
> > 
> > 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?
> 
> Oh, I think we can provide both if you are worried about the cost of ref
> counting. I hesitated doing that honestly.

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]

https://codereview.chromium.org/54273007/diff/50001/Source/core/dom/Node.h
File Source/core/dom/Node.h (right):

https://codereview.chromium.org/54273007/diff/50001/Source/core/dom/Node.h#ne...
Source/core/dom/Node.h:927: inline bool operator!=(const Node& a, const
PassRefPtr<Node>& b) { return !(a == b); }
On 2013/11/01 19:07:09, adamk wrote:
> On 2013/11/01 19:03:57, Chris Dumez wrote:
> > On 2013/11/01 18:56:50, adamk wrote:
> > > On 2013/11/01 18:43:18, Chris Dumez wrote:
> > > > 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
> > > 
> > > 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?
> > 
> > Oh, I think we can provide both if you are worried about the cost of ref
> > counting. I hesitated doing that honestly.
> 
> 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.

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]

lgtm

https://codereview.chromium.org/54273007/diff/50001/Source/core/dom/Node.h
File Source/core/dom/Node.h (right):

https://codereview.chromium.org/54273007/diff/50001/Source/core/dom/Node.h#ne...
Source/core/dom/Node.h:927: inline bool operator!=(const Node& a, const
PassRefPtr<Node>& b) { return !(a == b); }
On 2013/11/01 19:39:01, Chris Dumez wrote:
> On 2013/11/01 19:07:09, adamk wrote:
> > On 2013/11/01 19:03:57, Chris Dumez wrote:
> > > On 2013/11/01 18:56:50, adamk wrote:
> > > > On 2013/11/01 18:43:18, Chris Dumez wrote:
> > > > > 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
> > > > 
> > > > 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?
> > > 
> > > Oh, I think we can provide both if you are worried about the cost of ref
> > > counting. I hesitated doing that honestly.
> > 
> > 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.
> 
> 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 :)

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.

[commit-bot: I haz the power, 2013-11-01 20:21:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/54273007/50001

[commit-bot: I haz the power, 2013-11-01 21:25:40]

Change committed as 161202