Add Traversal<*Element>::firstAncestor() API

Add Traversal<*Element>::firstAncestor() API

Add Traversal<*Element>::firstAncestor() API to traverse elements of a specific
type. It is a common pattern in our code base to traverse the ancestors of a
Node until we find an Element of a specific type. This new firstAncestor() API
makes the code much simpler. This CL already makes use of the new API in
several places.

R=esprehn, eseidel
BUG=346733
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170211

[Inactive, 2014-03-15 23:50:18]

https://codereview.chromium.org/201293002/diff/20001/Source/core/dom/ElementT...
File Source/core/dom/ElementTraversal.h (right):

https://codereview.chromium.org/201293002/diff/20001/Source/core/dom/ElementT...
Source/core/dom/ElementTraversal.h:175: ContainerNode* ancestor =
current.parentNode();
One idea that came to mind, we could potentially overload firstAncestor() to
take an Element in argument and call parentOrShadowHostNode() instead of
parentNode() to avoid unnecessary calls to isShadowRoot(). (likely we would add
a more optimal Element::parentNode() instead that calls parentOrShadowHostNode()
so that the rest of the code can benefit from this optimization as well).

I need to get measurements to see if there would be any noticeable impact on
performance.

Let me know if you have any thoughts about this.

[Inactive, 2014-03-19 13:49:34]

ping review

[esprehn, 2014-03-21 07:58:33]

https://codereview.chromium.org/201293002/diff/20001/Source/core/accessibilit...
File Source/core/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/201293002/diff/20001/Source/core/accessibilit...
Source/core/accessibility/AXNodeObject.cpp:1484: return
isHTMLLabelElement(*node()) ? toHTMLLabelElement(node()) :
Traversal<HTMLLabelElement>::firstAncestor(*node());
Can we use if statements here? I don't think all these ternaries are helping
with the code readability.

https://codereview.chromium.org/201293002/diff/20001/Source/core/dom/ElementT...
File Source/core/dom/ElementTraversal.h (right):

https://codereview.chromium.org/201293002/diff/20001/Source/core/dom/ElementT...
Source/core/dom/ElementTraversal.h:43: // First ElementType ancestor of the
node.
The comment doesn't add anything.

https://codereview.chromium.org/201293002/diff/20001/Source/core/editing/Fram...
File Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/201293002/diff/20001/Source/core/editing/Fram...
Source/core/editing/FrameSelection.cpp:1754: HTMLElement* element = !start ||
start->isHTMLElement() ? toHTMLElement(start) :
Traversal<HTMLElement>::firstAncestor(*start);
This seems like it makes the code less readable.

https://codereview.chromium.org/201293002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/201293002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:2962: n =
ElementTraversal::firstAncestor(*n);
This just makes this code slower and more complicated. You only ever need to go
one level to get from a Node to an Element.

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

PTAL

https://codereview.chromium.org/201293002/diff/20001/Source/core/accessibilit...
File Source/core/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/201293002/diff/20001/Source/core/accessibilit...
Source/core/accessibility/AXNodeObject.cpp:1484: return
isHTMLLabelElement(*node()) ? toHTMLLabelElement(node()) :
Traversal<HTMLLabelElement>::firstAncestor(*node());
On 2014/03/21 07:58:33, esprehn wrote:
> Can we use if statements here? I don't think all these ternaries are helping
> with the code readability.

Done.

https://codereview.chromium.org/201293002/diff/20001/Source/core/dom/ElementT...
File Source/core/dom/ElementTraversal.h (right):

https://codereview.chromium.org/201293002/diff/20001/Source/core/dom/ElementT...
Source/core/dom/ElementTraversal.h:43: // First ElementType ancestor of the
node.
On 2014/03/21 07:58:33, esprehn wrote:
> The comment doesn't add anything.

Each set of ElementTraversal functions is grouped with a comment. If I don't add
a comment, then this new function will be part of an existing group but this one
brings a new functionality.

https://codereview.chromium.org/201293002/diff/20001/Source/core/editing/Fram...
File Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/201293002/diff/20001/Source/core/editing/Fram...
Source/core/editing/FrameSelection.cpp:1754: HTMLElement* element = !start ||
start->isHTMLElement() ? toHTMLElement(start) :
Traversal<HTMLElement>::firstAncestor(*start);
On 2014/03/21 07:58:33, esprehn wrote:
> This seems like it makes the code less readable.

Fair enough, I gave it another shot.

https://codereview.chromium.org/201293002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/201293002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderObject.cpp:2962: n =
ElementTraversal::firstAncestor(*n);
On 2014/03/21 07:58:33, esprehn wrote:
> This just makes this code slower and more complicated. You only ever need to
go
> one level to get from a Node to an Element.

Kept this change out.

[Inactive, 2014-03-25 16:33:17]

esprehn@, is this any better?

[esprehn, 2014-03-27 13:28:16]

lgtm, but I think you want firstAncestorOrSelf() or something, you duplicate the
self check so much.

https://codereview.chromium.org/201293002/diff/90002/Source/core/editing/Fram...
File Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/201293002/diff/90002/Source/core/editing/Fram...
Source/core/editing/FrameSelection.cpp:1767: for (HTMLElement* element =
Traversal<HTMLElement>::firstAncestor(*start); element; element =
Traversal<HTMLElement>::firstAncestor(*element)) {
Couldn't we just add ::firstAncestorOrSelf(Node*) ?

https://codereview.chromium.org/201293002/diff/90002/Source/core/html/HTMLFor...
File Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/201293002/diff/90002/Source/core/html/HTMLFor...
Source/core/html/HTMLFormControlElement.cpp:484: return
Traversal<HTMLFormControlElement>::firstAncestor(*node);
This also could have been firstAncestorOrSelf(node)

https://codereview.chromium.org/201293002/diff/90002/Source/core/html/parser/...
File Source/core/html/parser/HTMLTreeBuilder.cpp (right):

https://codereview.chromium.org/201293002/diff/90002/Source/core/html/parser/...
Source/core/html/parser/HTMLTreeBuilder.cpp:142: return
Traversal<HTMLFormElement>::firstAncestor(element);
Again this is firstAncestorOrSelf()

[Inactive, 2014-03-27 13:32:05]

https://codereview.chromium.org/201293002/diff/90002/Source/core/editing/Fram...
File Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/201293002/diff/90002/Source/core/editing/Fram...
Source/core/editing/FrameSelection.cpp:1767: for (HTMLElement* element =
Traversal<HTMLElement>::firstAncestor(*start); element; element =
Traversal<HTMLElement>::firstAncestor(*element)) {
On 2014/03/27 13:28:16, esprehn wrote:
> Couldn't we just add ::firstAncestorOrSelf(Node*) ?

Excellent idea. I thought about it but couldn't come up with a good name for it.
I like firstAncestorOrSelf() naming though. I will add it and simplify the code.

[Inactive, 2014-03-27 15:42:37]

I introduced an firstAncestorOrSelf() function, as advised. It does look much
better.
Please take another look.

[esprehn, 2014-03-27 20:06:59]

lgtm

[Inactive, 2014-03-27 20:14:57]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-03-27 20:15:04]

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

[commit-bot: I haz the power, 2014-03-27 22:32:52]

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

[commit-bot: I haz the power, 2014-03-27 22:32:52]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[esprehn, 2014-03-27 22:36:55]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-03-27 22:38:10]

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

[commit-bot: I haz the power, 2014-03-27 22:39:13]

Change committed as 170211