Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

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

Created:
4 years, 2 months ago by hayato
Modified:
4 years, 1 month 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?
4 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 ...
4 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 ...
4 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 ...
4 years, 1 month ago (2014-10-16 08:02:03 UTC) #5
dglazkov
lgtm
4 years, 1 month 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) : ...
4 years, 1 month 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 ...
4 years, 1 month 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
4 years, 1 month 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
4 years, 1 month 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
4 years, 1 month ago (2014-10-17 09:57:35 UTC) #16
commit-bot: I haz the power
Committed patchset #5 (id:280001) as 183872
4 years, 1 month 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 ...
4 years, 1 month 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 ...
4 years, 1 month ago (2014-10-22 05:15:19 UTC) #19
hayato
4 years, 1 month 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