Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(11)

Issue 642973003: Introduce typed Node/Element iterators for range-based for loops of C++11. (Closed)

Created:
6 years, 2 months ago by hayato
Modified:
6 years, 2 months ago
Reviewers:
esprehn, dglazkov, eseidel
CC:
blink-reviews, blink-reviews-dom_chromium.org, dglazkov+blink, eae+blinkwatch, rwlbuis, sof
Project:
blink
Visibility:
Public.

Description

Introduce typed Node/Element iterators for range-based for loops of C++11. Intend to replace the following typical for loop: for (Node* child = node.firstChild(); child; child = child->nextSibling()) { .... } with range-based for loop of C++11: for (Node& child : NodeTraversal::childrenOf(node)) { .... } Less verbose and improves readability, especially if we could close our eyes on NodeTraversal prefix. The following iterators are introduced to address common use cases in our code base: 1. NodeTraversal::childrenOf(const Node&) This could replace: for (Node* child = node.firstChild(); child; child = child->nextSibling()) { ... } with: for (Node& child : NodeTraversal::childrenOf(node)) { ... } 2. NodeTraversal::descendantsOf(const Node&) This could replace: for (Node* node = root.firstChild(); node; node = NodeTraversal::next(*node, &root) { ... } with: for (Node& node : NodeTraversal::descendantsOf(root)) { ... } 3. NodeTraversal::inclusiveDescendantsOf(const Node&) This could replace: for (Node* node = &root; node; node = NodeTraversal::next(*node, &root) { ... } with: for (Node& node : NodeTraversal::inclusiveDescendantsOf(root)) { ... } 4. NodeTraversal::fromNext(const Node&) This could replace: for (Node* node = NodeTraversal::next(start); node; node = NodeTraversal::next(*node) { ... } with: for (Node& node : NodeTraversal::fromNext(start)) { ... } There are also typed template versions for other element types: 5. Traversal<ElementType>::childrenOf(const Node&) This could replace: for (ElementType* child = Traversal<ElementType>::firstChild(node); child; child = Traversal<ElementType>::nextSibling(*child)) { ... } with: for (ElementType& child : Traversal<ElementType>::childrenOf(node)) { ... } 6. Traversal<ElementType>::descendantsOf(const Node&) This could replace: for (ElementType* node = Traversal<ElementType>::firstWithin(root); node; node = Traversal<ElementType>::next(*node, &root)) { ... } with: for (ElementType& node : Traversal<ElementType>::descendantsOf(root)) { ... } 7. Traversal<ElementType>::inclusiveDescendantsOf(const ElementType&) This could replace: for (ElementType* node = &root; node; node = Traversal<ElementType>::next(*node, &root)) { ... } with: for (ElementType& node : Traversal<ElementType>::inclusiveDescendantsOf(root)) { ... } 8. Traversal<ElementType>::fromNext(const Node&) This could replace: for (ElementType* node = Traversal<ElementType>::next(start); node; node = Traversal<ElementType>::next(*node) { ... } with: for (ElementType& node : Traversal<ElementType>::fromNext(start)) { ... } New iterators should be zero overhead because compilers can make them all inlined in a release build. As the first showcase, I let some files use this range-based for loops. This is not comprehensive so that this patch wouldn't get larger. Other changes will be done in another patch. We might want to have similar things for other classes, such as RenderObject, which can also be done in another patch. BUG=none TEST=no layout test failure Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183872

Patch Set 1 #

Patch Set 2 : Rename SiblingIterator to SiblingRange #

Patch Set 3 : Experimental iterators for range-based for loop #

Total comments: 1

Patch Set 4 : Clean up #

Patch Set 5 : Rename `from` to `fromNext`. Make some parameters const references. #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+211 lines, -72 lines) Patch
M Source/core/dom/ContainerNode.cpp View 1 2 2 chunks +9 lines, -9 lines 0 comments Download
M Source/core/dom/Document.cpp View 1 2 3 4 6 chunks +16 lines, -16 lines 0 comments Download
M Source/core/dom/DocumentOrderedMap.cpp View 1 2 3 4 1 chunk +4 lines, -4 lines 0 comments Download
M Source/core/dom/ElementTraversal.h View 1 2 3 4 3 chunks +30 lines, -0 lines 2 comments Download
M Source/core/dom/Node.cpp View 1 2 2 chunks +11 lines, -11 lines 0 comments Download
M Source/core/dom/NodeTraversal.h View 1 2 3 4 4 chunks +109 lines, -0 lines 0 comments Download
M Source/core/dom/TreeScope.cpp View 1 2 1 chunk +4 lines, -4 lines 0 comments Download
M Source/core/dom/TreeScopeAdopter.cpp View 1 2 1 chunk +10 lines, -10 lines 0 comments Download
M Source/core/html/HTMLFormElement.cpp View 1 2 3 4 2 chunks +8 lines, -8 lines 0 comments Download
M Source/core/html/HTMLTextFormControlElement.cpp View 1 2 1 chunk +7 lines, -7 lines 0 comments Download
M Source/core/xml/XPathStep.cpp View 1 2 3 4 1 chunk +3 lines, -3 lines 0 comments Download

Messages

Total messages: 20 (6 generated)
hayato
PTAL. WDYT about this change?
6 years, 2 months ago (2014-10-10 11:51:48 UTC) #2
esprehn
We should convert all of ElementTraversal and NodeTraversal to this pattern. I'm not sure we ...
6 years, 2 months ago (2014-10-10 14:50:09 UTC) #3
hayato
Yeah, I agree that we should prefer a free function instead of a method in ...
6 years, 2 months ago (2014-10-10 15:09:29 UTC) #4
hayato
Hi Elliott and other reviewers, I've designed and implemented new iterators from scratch, utilizing existing ...
6 years, 2 months ago (2014-10-16 08:02:03 UTC) #5
dglazkov
lgtm
6 years, 2 months ago (2014-10-16 17:30:49 UTC) #6
esprehn
Your issue description needs updating. https://codereview.chromium.org/642973003/diff/240001/Source/core/dom/NodeTraversal.h File Source/core/dom/NodeTraversal.h (right): https://codereview.chromium.org/642973003/diff/240001/Source/core/dom/NodeTraversal.h#newcode149 Source/core/dom/NodeTraversal.h:149: TraversalDescendantIterator(const Node& start) : ...
6 years, 2 months ago (2014-10-16 17:43:25 UTC) #7
hayato
Thank you for the review. I'll update the issue description and address the comment. Since ...
6 years, 2 months ago (2014-10-17 03:28:45 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/642973003/280001
6 years, 2 months ago (2014-10-17 09:43:22 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/642973003/280001
6 years, 2 months ago (2014-10-17 09:46:16 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/642973003/280001
6 years, 2 months ago (2014-10-17 09:57:35 UTC) #16
commit-bot: I haz the power
Committed patchset #5 (id:280001) as 183872
6 years, 2 months ago (2014-10-17 09:59:05 UTC) #17
dglazkov
lgtm https://codereview.chromium.org/642973003/diff/280001/Source/core/dom/ElementTraversal.h File Source/core/dom/ElementTraversal.h (right): https://codereview.chromium.org/642973003/diff/280001/Source/core/dom/ElementTraversal.h#newcode141 Source/core/dom/ElementTraversal.h:141: inline TraversalRange<TraversalNextIterator<Traversal<ElementType>>> Traversal<ElementType>::fromNext(const Node& start) This name had ...
6 years, 2 months ago (2014-10-21 16:03:12 UTC) #18
hayato
https://codereview.chromium.org/642973003/diff/280001/Source/core/dom/ElementTraversal.h File Source/core/dom/ElementTraversal.h (right): https://codereview.chromium.org/642973003/diff/280001/Source/core/dom/ElementTraversal.h#newcode141 Source/core/dom/ElementTraversal.h:141: inline TraversalRange<TraversalNextIterator<Traversal<ElementType>>> Traversal<ElementType>::fromNext(const Node& start) On 2014/10/21 16:03:11, dglazkov ...
6 years, 2 months ago (2014-10-22 05:15:19 UTC) #19
hayato
6 years, 2 months ago (2014-10-22 07:16:25 UTC) #20
Message was sent while issue was closed.
Here is the patch for the review: https://codereview.chromium.org/667403002/

Powered by Google App Engine
This is Rietveld 408576698