Introduce NodeTraversal::ancestorsOf() and inclusiveAncestors() for range-based for loop

Introduce NodeTraversal::ancestorsOf() and inclusiveAncestors() for range-based for loop

This patch introduces |NodeTraversal::ancestorsOf()| and |inclusiveAncestors()|
to reduce source code size by common code pattern for ease of writing code
in addition to |NodeTraversal::childrenOf()|[1], |NodeTraversal::descendansOf()|
and so on.

Before this patch, we should write:
// Loop for ancestors
for (Node* runner = node->parentNode(); runner; runner = runner->parentNode()) {
 ...
}

// Loop for ancestors or self
for (Node* runner = node; runner; runner = runner->parentNode()) {
 ...
}

After this patch, you can write:
for (Node& runner : NodeTraversal::ancestorsOf(*node)) {
 ...
}

for (Node& runner : NodeTraversal::inclusiveAncestorsOf(*node)) {
 ...
}

[1] https://codereview.chromium.org/642973003 Introduce typed Node/Element
iterators for range-based for loops of C++11

BUG=n/a
TEST=n/a; no behavior changes

Committed: https://crrev.com/95d6f1792e11fefa919d81d1ed7c0d233deafa76
Cr-Commit-Position: refs/heads/master@{#390895}

[yosin_UTC9, 2016-04-28 07:17:42]

Description was changed from

==========
Introduce NodeTraversal::ancestorsOf() and ancestorsOrSelf() for range-for

BUG=
==========

to

==========
Introduce NodeTraversal::ancestorsOf() and ancestorsOrSelf() for range-based for
loop

BUG=
==========

[yosin_UTC9, 2016-04-28 07:39:20]

Description was changed from

==========
Introduce NodeTraversal::ancestorsOf() and ancestorsOrSelf() for range-based for
loop

BUG=
==========

to

==========
Introduce NodeTraversal::ancestorsOf() and ancestorsOrSelf() for range-based for
loop

This patch introduces |NodeTraversal::ancestorsOf()| and |ancestorsOrSelf()|
to reduce source code size by common code pattern for ease of writing code
in addition to |NodeTraversal::childrenOf()|, |NodeTraversal::descendansOf()|
and so on.

Before this patch, we should write:
// Loop for ancestors
for (Node* runner = node->parentNode(); runner; runner = runner->parentNode()) {
 ...
}

// Loop for ancestors or self
for (Node* runner = node; runner; runner = runner->parentNode()) {
 ...
}

After this patch, you can write:
for (Node& runner : NodeTraversal::ancestorsOf(*node)) {
 ...
}

for (Node& runner : NodeTraversal::ancestorsOrSelfOf(node)) {
 ...
}



BUG=n/a
TEST=n/a; no behavior changes
==========

[yosin_UTC9, 2016-04-28 07:56:49]

Description was changed from

==========
Introduce NodeTraversal::ancestorsOf() and ancestorsOrSelf() for range-based for
loop

This patch introduces |NodeTraversal::ancestorsOf()| and |ancestorsOrSelf()|
to reduce source code size by common code pattern for ease of writing code
in addition to |NodeTraversal::childrenOf()|, |NodeTraversal::descendansOf()|
and so on.

Before this patch, we should write:
// Loop for ancestors
for (Node* runner = node->parentNode(); runner; runner = runner->parentNode()) {
 ...
}

// Loop for ancestors or self
for (Node* runner = node; runner; runner = runner->parentNode()) {
 ...
}

After this patch, you can write:
for (Node& runner : NodeTraversal::ancestorsOf(*node)) {
 ...
}

for (Node& runner : NodeTraversal::ancestorsOrSelfOf(node)) {
 ...
}



BUG=n/a
TEST=n/a; no behavior changes
==========

to

==========
Introduce NodeTraversal::ancestorsOf() and ancestorsOrSelf() for range-based for
loop

This patch introduces |NodeTraversal::ancestorsOf()| and |ancestorsOrSelf()|
to reduce source code size by common code pattern for ease of writing code
in addition to |NodeTraversal::childrenOf()|[1], |NodeTraversal::descendansOf()|
and so on.

Before this patch, we should write:
// Loop for ancestors
for (Node* runner = node->parentNode(); runner; runner = runner->parentNode()) {
 ...
}

// Loop for ancestors or self
for (Node* runner = node; runner; runner = runner->parentNode()) {
 ...
}

After this patch, you can write:
for (Node& runner : NodeTraversal::ancestorsOf(*node)) {
 ...
}

for (Node& runner : NodeTraversal::ancestorsOrSelfOf(node)) {
 ...
}

[1] https://codereview.chromium.org/642973003 Introduce typed Node/Element 
iterators for range-based for loops of C++11

BUG=n/a
TEST=n/a; no behavior changes
==========

[yosin_UTC9, 2016-04-28 07:57:14]

yosin@chromium.org changed reviewers:
+ tkent@chromium.org

[yosin_UTC9, 2016-04-28 07:57:15]

PTAL

[tkent, 2016-04-28 08:08:43]

tkent@chromium.org changed reviewers:
+ hayato@chromium.org

[tkent, 2016-04-28 08:08:44]

Maybe hayato@ is a better reviewer for this.

[fs, 2016-04-28 08:18:52]

https://codereview.chromium.org/1932523003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/NodeTraversal.h (right):

https://codereview.chromium.org/1932523003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/NodeTraversal.h:106: static
TraversalRange<TraversalAncestorsOrSelfIterator<NodeTraversal>>
ancestorsOrSelfOf(const Node*);
Drive-by: ancestorsOrSelfOf is kind of a weird name for something that does not
return either 'self' or ancestors (but rather both).

Maybe inclusiveAncestorsOf? (Similar to inclusiveDescendantsOf/descendantsOf)

[hayato, 2016-04-28 08:38:21]

https://codereview.chromium.org/1932523003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/NodeTraversal.h (right):

https://codereview.chromium.org/1932523003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/NodeTraversal.h:236: inline
TraversalRange<TraversalAncestorsOrSelfIterator<NodeTraversal>>
NodeTraversal::ancestorsOrSelfOf(const Node* node)
It might be better that we do not provide a pointer version. In my experience,
that is likely hide a potential wrong usage in the caller side.
In most cases, it would be better to check null-ness explicitly in the caller.


I know that we already have Node::startsAtstartAt(const Node* start), but we
should prevent spreading the wrong APIs.

[yosin_UTC9, 2016-04-28 08:59:05]

PTAL

https://codereview.chromium.org/1932523003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/NodeTraversal.h (right):

https://codereview.chromium.org/1932523003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/NodeTraversal.h:106: static
TraversalRange<TraversalAncestorsOrSelfIterator<NodeTraversal>>
ancestorsOrSelfOf(const Node*);
On 2016/04/28 at 08:18:52, fs wrote:
> Drive-by: ancestorsOrSelfOf is kind of a weird name for something that does
not return either 'self' or ancestors (but rather both).
> 
> Maybe inclusiveAncestorsOf? (Similar to inclusiveDescendantsOf/descendantsOf)

Done.

https://codereview.chromium.org/1932523003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/NodeTraversal.h:236: inline
TraversalRange<TraversalAncestorsOrSelfIterator<NodeTraversal>>
NodeTraversal::ancestorsOrSelfOf(const Node* node)
On 2016/04/28 at 08:38:20, hayato wrote:
> It might be better that we do not provide a pointer version. In my experience,
that is likely hide a potential wrong usage in the caller side.
> In most cases, it would be better to check null-ness explicitly in the caller.
> 
> 
> I know that we already have Node::startsAtstartAt(const Node* start), but we
should prevent spreading the wrong APIs.

I don't agree, pointer version is useful and safe for nature of
inclusiveAncestorsOf(), renamed from ancestorsOrSelf().

In editing and event handling, we sometimes deal with null node situation,
explicit null checking with inclusiveAncestorsOf() increases nesting and need
more attention. I'm afraid that people simply add |*| to convert pointer to
reference.

Example:

OLD:
for (; node; node = node->parentNode()) {
            if (!node->isElementNode())

inclusiveAncestorsOf():
if (node) {
  for (Node& runner : NodeTraversal::inclusiveAncestorsOf(*node)) {
     if (!node->isElementNode())
}

[yosin_UTC9, 2016-04-28 09:01:29]

https://codereview.chromium.org/1932523003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/NodeTraversal.h (right):

https://codereview.chromium.org/1932523003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/NodeTraversal.h:236: inline
TraversalRange<TraversalAncestorsOrSelfIterator<NodeTraversal>>
NodeTraversal::ancestorsOrSelfOf(const Node* node)
On 2016/04/28 at 08:59:04, Yosi_UTC9 wrote:
> On 2016/04/28 at 08:38:20, hayato wrote:
> > It might be better that we do not provide a pointer version. In my
experience, that is likely hide a potential wrong usage in the caller side.
> > In most cases, it would be better to check null-ness explicitly in the
caller.
> > 
> > 
> > I know that we already have Node::startsAtstartAt(const Node* start), but we
should prevent spreading the wrong APIs.
> 
> I don't agree, pointer version is useful and safe for nature of
inclusiveAncestorsOf(), renamed from ancestorsOrSelf().
> 
> In editing and event handling, we sometimes deal with null node situation,
explicit null checking with inclusiveAncestorsOf() increases nesting and need
more attention. I'm afraid that people simply add |*| to convert pointer to
reference.
> 
> Example:
> 
> OLD:
> for (; node; node = node->parentNode()) {
>             if (!node->isElementNode())
> 
> inclusiveAncestorsOf():
> if (node) {
>   for (Node& runner : NodeTraversal::inclusiveAncestorsOf(*node)) {
>      if (!node->isElementNode())
> }

Actually, I introduced reference version only then started rewriting. I found
some of them requires null check, then I introduced pointer version for ease of
writing code.

[hayato, 2016-04-28 09:15:15]

I am aware that there are some places where we have to check null-ness.
Actually, I would rather let some of them to check null-ness explicitly at the
caller side. That's the motivation of my comment.

I prefer avoiding a potential error which is caused by ignoring nullptr.

[yosin_UTC9, 2016-04-28 09:40:39]

PTAL

- Get rid of pointer version of |inclusiveAncestorsOf()|
- rewrite these call sites and add null check or DCHECK() for arguments.

[hayato, 2016-04-28 09:52:39]

lgtm

[hayato, 2016-04-28 09:53:24]

Please update the description of the CL to reflect |inclusiveAncestorsOf|.

[yosin_UTC9, 2016-04-28 09:57:46]

Description was changed from

==========
Introduce NodeTraversal::ancestorsOf() and ancestorsOrSelf() for range-based for
loop

This patch introduces |NodeTraversal::ancestorsOf()| and |ancestorsOrSelf()|
to reduce source code size by common code pattern for ease of writing code
in addition to |NodeTraversal::childrenOf()|[1], |NodeTraversal::descendansOf()|
and so on.

Before this patch, we should write:
// Loop for ancestors
for (Node* runner = node->parentNode(); runner; runner = runner->parentNode()) {
 ...
}

// Loop for ancestors or self
for (Node* runner = node; runner; runner = runner->parentNode()) {
 ...
}

After this patch, you can write:
for (Node& runner : NodeTraversal::ancestorsOf(*node)) {
 ...
}

for (Node& runner : NodeTraversal::ancestorsOrSelfOf(node)) {
 ...
}

[1] https://codereview.chromium.org/642973003 Introduce typed Node/Element 
iterators for range-based for loops of C++11

BUG=n/a
TEST=n/a; no behavior changes
==========

to

==========
Introduce NodeTraversal::ancestorsOf() and inclusiveAncestors() for range-based
for loop

This patch introduces |NodeTraversal::ancestorsOf()| and |inclusiveAncestors()|
to reduce source code size by common code pattern for ease of writing code
in addition to |NodeTraversal::childrenOf()|[1], |NodeTraversal::descendansOf()|
and so on.

Before this patch, we should write:
// Loop for ancestors
for (Node* runner = node->parentNode(); runner; runner = runner->parentNode()) {
 ...
}

// Loop for ancestors or self
for (Node* runner = node; runner; runner = runner->parentNode()) {
 ...
}

After this patch, you can write:
for (Node& runner : NodeTraversal::ancestorsOf(*node)) {
 ...
}

for (Node& runner : NodeTraversal::inclusiveAncestorsOf(*node)) {
 ...
}

[1] https://codereview.chromium.org/642973003 Introduce typed Node/Element 
iterators for range-based for loops of C++11

BUG=n/a
TEST=n/a; no behavior changes
==========

[yosin_UTC9, 2016-04-28 13:48:28]

The CQ bit was checked by yosin@chromium.org

[commit-bot: I haz the power, 2016-04-28 13:48:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1932523003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1932523003/80001

[yosin_UTC9, 2016-04-28 13:54:11]

Kent-san, please review a file in Source/web in this patch.
There is one file: Source/web/WebLocalFrameImpl.cpp

Thanks!

[commit-bot: I haz the power, 2016-04-28 14:01:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-28 14:01:50]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[tkent, 2016-05-02 04:11:35]

The CQ bit was checked by tkent@chromium.org

[tkent, 2016-05-02 04:11:36]

lgtm

[commit-bot: I haz the power, 2016-05-02 04:11:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1932523003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1932523003/80001

[commit-bot: I haz the power, 2016-05-02 05:26:29]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-02 05:28:13]

Description was changed from

==========
Introduce NodeTraversal::ancestorsOf() and inclusiveAncestors() for range-based
for loop

This patch introduces |NodeTraversal::ancestorsOf()| and |inclusiveAncestors()|
to reduce source code size by common code pattern for ease of writing code
in addition to |NodeTraversal::childrenOf()|[1], |NodeTraversal::descendansOf()|
and so on.

Before this patch, we should write:
// Loop for ancestors
for (Node* runner = node->parentNode(); runner; runner = runner->parentNode()) {
 ...
}

// Loop for ancestors or self
for (Node* runner = node; runner; runner = runner->parentNode()) {
 ...
}

After this patch, you can write:
for (Node& runner : NodeTraversal::ancestorsOf(*node)) {
 ...
}

for (Node& runner : NodeTraversal::inclusiveAncestorsOf(*node)) {
 ...
}

[1] https://codereview.chromium.org/642973003 Introduce typed Node/Element 
iterators for range-based for loops of C++11

BUG=n/a
TEST=n/a; no behavior changes
==========

to

==========
Introduce NodeTraversal::ancestorsOf() and inclusiveAncestors() for range-based
for loop

This patch introduces |NodeTraversal::ancestorsOf()| and |inclusiveAncestors()|
to reduce source code size by common code pattern for ease of writing code
in addition to |NodeTraversal::childrenOf()|[1], |NodeTraversal::descendansOf()|
and so on.

Before this patch, we should write:
// Loop for ancestors
for (Node* runner = node->parentNode(); runner; runner = runner->parentNode()) {
 ...
}

// Loop for ancestors or self
for (Node* runner = node; runner; runner = runner->parentNode()) {
 ...
}

After this patch, you can write:
for (Node& runner : NodeTraversal::ancestorsOf(*node)) {
 ...
}

for (Node& runner : NodeTraversal::inclusiveAncestorsOf(*node)) {
 ...
}

[1] https://codereview.chromium.org/642973003 Introduce typed Node/Element
iterators for range-based for loops of C++11

BUG=n/a
TEST=n/a; no behavior changes

Committed: https://crrev.com/95d6f1792e11fefa919d81d1ed7c0d233deafa76
Cr-Commit-Position: refs/heads/master@{#390895}
==========

[commit-bot: I haz the power, 2016-05-02 05:28:14]

Patchset 5 (id:??) landed as
https://crrev.com/95d6f1792e11fefa919d81d1ed7c0d233deafa76
Cr-Commit-Position: refs/heads/master@{#390895}