Handle fallback content case in FlatTreeTraversal::traverseSiblings()

Handle fallback content case in FlatTreeTraversal::traverseSiblings()

FlatTreeTraversal::nextSibling()/previousSibling() didn't handle
fallback content (nodes in <slot> where nothing is distributed).

This CL adds fallback content handling and a unit test case.

BUG=683776
Review-Url: https://codereview.chromium.org/2749033006
Cr-Commit-Position: refs/heads/master@{#457399}
Committed: https://chromium.googlesource.com/chromium/src/+/b1c7a1d78f6c79f18f1686014a81b86abfb38806

[kochi, 2017-03-16 07:31:47]

The CQ bit was checked by kochi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 07:32:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kochi, 2017-03-16 07:33:09]

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

[kochi, 2017-03-16 07:33:09]

PTAL

[commit-bot: I haz the power, 2017-03-16 09:12:51]

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

[commit-bot: I haz the power, 2017-03-16 09:12:52]

Dry run: This issue passed the CQ dry run.

[hayato, 2017-03-16 09:32:56]

Nice catch.

https://codereview.chromium.org/2749033006/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/shadow/FlatTreeTraversal.cpp (right):

https://codereview.chromium.org/2749033006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/shadow/FlatTreeTraversal.cpp:137: if
(node.isInV1ShadowTree()) {
Instead of this early check, could you use slot.suppportDistribution() to judge
the slot is *active* or not?
It is more consistent, and more robust for a future change.

https://codereview.chromium.org/2749033006/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/shadow/FlatTreeTraversalTest.cpp
(right):

https://codereview.chromium.org/2749033006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/shadow/FlatTreeTraversalTest.cpp:597:
TEST_F(FlatTreeTraversalTest, v1FallbackContent) {
Could you add a test for a slot which has both child nodes (which are not used
as fallback) and assigned nodes?

[kochi, 2017-03-16 10:09:54]

Thanks for the review!

PTAL.

https://codereview.chromium.org/2749033006/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/shadow/FlatTreeTraversal.cpp (right):

https://codereview.chromium.org/2749033006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/shadow/FlatTreeTraversal.cpp:137: if
(node.isInV1ShadowTree()) {
On 2017/03/16 09:32:56, hayato wrote:
> Instead of this early check, could you use slot.suppportDistribution() to
judge
> the slot is *active* or not?
> It is more consistent, and more robust for a future change.
> 

Done.
Actually it looks better now.  Thanks for the suggestion.

https://codereview.chromium.org/2749033006/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/shadow/FlatTreeTraversalTest.cpp
(right):

https://codereview.chromium.org/2749033006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/shadow/FlatTreeTraversalTest.cpp:597:
TEST_F(FlatTreeTraversalTest, v1FallbackContent) {
On 2017/03/16 09:32:56, hayato wrote:
> Could you add a test for a slot which has both child nodes (which are not used
> as fallback) and assigned nodes?

Added that case, and one more all-fallback contents case
(traversal from fallback content to fallback content).

[kochi, 2017-03-16 10:10:26]

The CQ bit was checked by kochi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 10:10:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hayato, 2017-03-16 10:24:10]

lgtm

[kochi, 2017-03-16 10:27:35]

The CQ bit was unchecked by kochi@chromium.org

[kochi, 2017-03-16 10:27:35]

The CQ bit was checked by kochi@chromium.org

[commit-bot: I haz the power, 2017-03-16 10:27:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-16 12:03:56]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489660055951140,
"parent_rev": "a2c5d8e30655a3dc0ac43dfda31167246b41a597", "commit_rev":
"b1c7a1d78f6c79f18f1686014a81b86abfb38806"}

[commit-bot: I haz the power, 2017-03-16 12:04:35]

Description was changed from

==========
Handle fallback content case in FlatTreeTraversal::traverseSiblings()

FlatTreeTraversal::nextSibling()/previousSibling() didn't handle
fallback content (nodes in <slot> where nothing is distributed).

This CL adds fallback content handling and a unit test case.

BUG=683776
==========

to

==========
Handle fallback content case in FlatTreeTraversal::traverseSiblings()

FlatTreeTraversal::nextSibling()/previousSibling() didn't handle
fallback content (nodes in <slot> where nothing is distributed).

This CL adds fallback content handling and a unit test case.

BUG=683776

Review-Url: https://codereview.chromium.org/2749033006
Cr-Commit-Position: refs/heads/master@{#457399}
Committed:
https://chromium.googlesource.com/chromium/src/+/b1c7a1d78f6c79f18f1686014a81...
==========

[commit-bot: I haz the power, 2017-03-16 12:04:36]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/b1c7a1d78f6c79f18f1686014a81...