Use more const references in HTMLCollection / LiveNodeList

Use more const references in HTMLCollection / LiveNodeList

The code looks safer as it is now obvious the nodes / lists cannot
be null. Also, making them const also makes the code look safer as
we know the nodes are not altered by the methods.

R=esprehn, adamk, eseidel

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

[Inactive, 2014-01-17 21:10:43]

https://codereview.chromium.org/141683004/diff/1/Source/core/html/HTMLLabelEl...
File Source/core/html/HTMLLabelElement.cpp (left):

https://codereview.chromium.org/141683004/diff/1/Source/core/html/HTMLLabelEl...
Source/core/html/HTMLLabelElement.cpp:40: if (!element ||
!element->isHTMLElement())
Got rid of a useless null check.

[adamk, 2014-01-21 21:43:54]

Overall this looks great, happy to see more constness.

https://codereview.chromium.org/141683004/diff/1/Source/core/html/HTMLLabelEl...
File Source/core/html/HTMLLabelElement.cpp (right):

https://codereview.chromium.org/141683004/diff/1/Source/core/html/HTMLLabelEl...
Source/core/html/HTMLLabelElement.cpp:71: Element* element =
const_cast<HTMLLabelElement*>(this);
I'd prefer restructuring the logic and removing this const_cast (likely by using
a do {} while loop). It'll involve an extra call to ElementTraversal::next(),
but that seems worthwhile to me to avoid const_casting in a const method (which
looks quite suspicious).

[esprehn, 2014-01-21 21:50:00]

On 2014/01/21 21:43:54, adamk wrote:
> Overall this looks great, happy to see more constness.
> 
>
https://codereview.chromium.org/141683004/diff/1/Source/core/html/HTMLLabelEl...
> File Source/core/html/HTMLLabelElement.cpp (right):
> 
>
https://codereview.chromium.org/141683004/diff/1/Source/core/html/HTMLLabelEl...
> Source/core/html/HTMLLabelElement.cpp:71: Element* element =
> const_cast<HTMLLabelElement*>(this);
> I'd prefer restructuring the logic and removing this const_cast (likely by
using
> a do {} while loop). It'll involve an extra call to ElementTraversal::next(),
> but that seems worthwhile to me to avoid const_casting in a const method
(which
> looks quite suspicious).

ElementTraversal::next() can be quite expensive jumping around the tree, I guess
<label> doesn't usually have many children, but doing more tree traversing to
avoid a const_cast seems really weird. We should be making things more expensive
to appease the compiler.

[esprehn, 2014-01-21 21:50:29]

On 2014/01/21 21:50:00, esprehn wrote:
> ...
> 
> ElementTraversal::next() can be quite expensive jumping around the tree, I
guess
> <label> doesn't usually have many children, but doing more tree traversing to
> avoid a const_cast seems really weird. We shouldn't be making things more
expensive
> to appease the compiler.

_shouldn't_

[adamk, 2014-01-21 21:56:05]

On 2014/01/21 21:50:29, esprehn wrote:
> On 2014/01/21 21:50:00, esprehn wrote:
> > ...
> > 
> > ElementTraversal::next() can be quite expensive jumping around the tree, I
> guess
> > <label> doesn't usually have many children, but doing more tree traversing
to
> > avoid a const_cast seems really weird. We shouldn't be making things more
> expensive
> > to appease the compiler.
> 
> _shouldn't_

I'm not arguing for more _uses_ of ::next(), just two different calls to it
(it's actually a for loop, not a do/while):

Element* element = ElementTraversal::next(*this);
for (Element* element = ElementTraversal::next(*this); element; element =
ElementTraversal::next(*element)) {
  ...
}

[adamk, 2014-01-21 21:56:38]

On 2014/01/21 21:56:05, adamk wrote:
> On 2014/01/21 21:50:29, esprehn wrote:
> > On 2014/01/21 21:50:00, esprehn wrote:
> > > ...
> > > 
> > > ElementTraversal::next() can be quite expensive jumping around the tree, I
> > guess
> > > <label> doesn't usually have many children, but doing more tree traversing
> to
> > > avoid a const_cast seems really weird. We shouldn't be making things more
> > expensive
> > > to appease the compiler.
> > 
> > _shouldn't_
> 
> I'm not arguing for more _uses_ of ::next(), just two different calls to it
> (it's actually a for loop, not a do/while):
> 
> Element* element = ElementTraversal::next(*this);
> for (Element* element = ElementTraversal::next(*this); element; element =
> ElementTraversal::next(*element)) {
>   ...
> }

Except of course without that first line, just the for loop ought to do it.

[Inactive, 2014-01-21 23:14:03]

https://codereview.chromium.org/141683004/diff/1/Source/core/html/HTMLLabelEl...
File Source/core/html/HTMLLabelElement.cpp (right):

https://codereview.chromium.org/141683004/diff/1/Source/core/html/HTMLLabelEl...
Source/core/html/HTMLLabelElement.cpp:71: Element* element =
const_cast<HTMLLabelElement*>(this);
On 2014/01/21 21:43:55, adamk wrote:
> I'd prefer restructuring the logic and removing this const_cast (likely by
using
> a do {} while loop). It'll involve an extra call to ElementTraversal::next(),
> but that seems worthwhile to me to avoid const_casting in a const method
(which
> looks quite suspicious).

Sure, I will refactor to a for loop and thus avoid the const_cast. I agree it
does not cause any additional traversal and the code would be a bit more
readable.

[Inactive, 2014-01-21 23:40:19]

https://codereview.chromium.org/141683004/diff/1/Source/core/html/HTMLLabelEl...
File Source/core/html/HTMLLabelElement.cpp (right):

https://codereview.chromium.org/141683004/diff/1/Source/core/html/HTMLLabelEl...
Source/core/html/HTMLLabelElement.cpp:71: Element* element =
const_cast<HTMLLabelElement*>(this);
On 2014/01/21 21:43:55, adamk wrote:
> I'd prefer restructuring the logic and removing this const_cast (likely by
using
> a do {} while loop). It'll involve an extra call to ElementTraversal::next(),
> but that seems worthwhile to me to avoid const_casting in a const method
(which
> looks quite suspicious).

Done.

[adamk, 2014-01-21 23:41:38]

lgtm

[esprehn, 2014-01-22 00:03:07]

Lgtm

[commit-bot: I haz the power, 2014-01-22 00:46:04]

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

[commit-bot: I haz the power, 2014-01-22 02:51:57]

Change committed as 165504