Fix FlatTreeTraversal for a slot in a document tree

Fix FlatTreeTraversal for a slot in a document tree

Fix FlatTreeTraversal so that it considers a slot in a document tree correctly.

Slots in a document tree are not well supported. They need a special treatment.
This CL fixes the crash reported in the bug.

BUG=649576
Committed: https://crrev.com/dce2e40e8f3b783ef18e06b72c955bf59ed122d9
Cr-Commit-Position: refs/heads/master@{#425252}

[hayato, 2016-10-13 08:22:26]

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

[commit-bot: I haz the power, 2016-10-13 08:22:36]

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

[hayato, 2016-10-13 08:25:44]

Description was changed from

==========
Fix FlatTreeTraversal for a slot in a document tree

BUG=649576
==========

to

==========
Fix FlatTreeTraversal for a slot in a document tree

Fix FlatTreeTraversal so that it return the correct first child, *skipping*
a slot in a document tree.

Slots in a document tree is not well supported. They need a special treatment.

BUG=649576
==========

[hayato, 2016-10-13 08:25:56]

Description was changed from

==========
Fix FlatTreeTraversal for a slot in a document tree

Fix FlatTreeTraversal so that it return the correct first child, *skipping*
a slot in a document tree.

Slots in a document tree is not well supported. They need a special treatment.

BUG=649576
==========

to

==========
Fix FlatTreeTraversal for a slot in a document tree

Fix FlatTreeTraversal so that it returns the correct first child, *skipping*
a slot in a document tree.

Slots in a document tree is not well supported. They need a special treatment.

BUG=649576
==========

[hayato, 2016-10-13 08:26:46]

Description was changed from

==========
Fix FlatTreeTraversal for a slot in a document tree

Fix FlatTreeTraversal so that it returns the correct first child, *skipping*
a slot in a document tree.

Slots in a document tree is not well supported. They need a special treatment.

BUG=649576
==========

to

==========
Fix FlatTreeTraversal for a slot in a document tree

Fix FlatTreeTraversal so that it considers a slot in a document tree correctly.

Slots in a document tree are not well supported. They need a special treatment.

BUG=649576
==========

[hayato, 2016-10-13 08:27:24]

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

[hayato, 2016-10-13 08:27:25]

PTAL

[hayato, 2016-10-13 08:28:13]

Description was changed from

==========
Fix FlatTreeTraversal for a slot in a document tree

Fix FlatTreeTraversal so that it considers a slot in a document tree correctly.

Slots in a document tree are not well supported. They need a special treatment.

BUG=649576
==========

to

==========
Fix FlatTreeTraversal for a slot in a document tree

Fix FlatTreeTraversal so that it considers a slot in a document tree correctly.

Slots in a document tree are not well supported. They need a special treatment.
This CL fixes the crash reported in the bug.

BUG=649576
==========

[tkent, 2016-10-13 08:56:49]

lgtm

[hayato, 2016-10-13 08:59:07]

The CQ bit was unchecked by hayato@chromium.org

[hayato, 2016-10-13 08:59:10]

The CQ bit was checked by hayato@chromium.org

[commit-bot: I haz the power, 2016-10-13 08:59:14]

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, 2016-10-13 09:49:16]

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

[commit-bot: I haz the power, 2016-10-13 09:49:17]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[hayato, 2016-10-13 11:00:32]

Looks there is a regression. Let me take a look.

[yosin_UTC9, 2016-10-14 01:44:40]

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

https://codereview.chromium.org/2416833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/shadow/FlatTreeTraversal.cpp:62:
TraversalDirection direction) {
BTW, how about using template function to avoid |TraversalDirection| with
ternary operator in if-condition and for-condition?

https://codereview.chromium.org/2416833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/shadow/FlatTreeTraversal.cpp:72:
const_cast<HTMLSlotElement&>(slot)
Can we integrate collecting distributed nodes into update distribution phase?

https://codereview.chromium.org/2416833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/shadow/FlatTreeTraversal.cpp:73:
.updateDistributedNodesInDocumentTree();
It is better that using |collectDistributionNodesInDocumentTree()| or another
which implies populating |m_distributedNoes| instead of
|updateDistributedNodesInDocumentTree()|, since |updateXXX()| looks like
updating flat tree and term "update" is too generic(?).
Or, |updateDistributedNodeCache()| to make using |const_cast| to feel sane?

[hayato, 2016-10-14 03:02:04]

fix a regression

[hayato, 2016-10-14 03:02:06]

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

[commit-bot: I haz the power, 2016-10-14 03:02:25]

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

[hayato, 2016-10-14 03:03:53]

remove TODO

[hayato, 2016-10-14 03:03:55]

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

[commit-bot: I haz the power, 2016-10-14 03:04:10]

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

[hayato, 2016-10-14 03:04:11]

The CQ bit was unchecked by hayato@chromium.org

[hayato, 2016-10-14 03:04:13]

The CQ bit was checked by hayato@chromium.org

[hayato, 2016-10-14 03:04:14]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/2416833002/#ps40001
(title: "remove TODO")

[commit-bot: I haz the power, 2016-10-14 03:04:44]

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

[hayato, 2016-10-14 03:05:35]

I have fixed the regression.
Some functions were renamed accordingly.

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

https://codereview.chromium.org/2416833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/shadow/FlatTreeTraversal.cpp:62:
TraversalDirection direction) {
On 2016/10/14 at 01:44:40, Yosi_UTC9 wrote:
> BTW, how about using template function to avoid |TraversalDirection| with
ternary operator in if-condition and for-condition?

That could be possible. We should do it in another CL.

https://codereview.chromium.org/2416833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/shadow/FlatTreeTraversal.cpp:72:
const_cast<HTMLSlotElement&>(slot)
On 2016/10/14 at 01:44:40, Yosi_UTC9 wrote:
> Can we integrate collecting distributed nodes into update distribution phase?

Yeah, that is one of TODO items. That could not be done easily.
You can find TODO description in this file. :)

https://codereview.chromium.org/2416833002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/shadow/FlatTreeTraversal.cpp:73:
.updateDistributedNodesInDocumentTree();
On 2016/10/14 at 01:44:40, Yosi_UTC9 wrote:
> It is better that using |collectDistributionNodesInDocumentTree()| or another
which implies populating |m_distributedNoes| instead of
> |updateDistributedNodesInDocumentTree()|, since |updateXXX()| looks like
updating flat tree and term "update" is too generic(?).
> Or, |updateDistributedNodeCache()| to make using |const_cast| to feel sane?

I think using *update* is consistent within this file.

[commit-bot: I haz the power, 2016-10-14 04:22:21]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-14 04:26:49]

Description was changed from

==========
Fix FlatTreeTraversal for a slot in a document tree

Fix FlatTreeTraversal so that it considers a slot in a document tree correctly.

Slots in a document tree are not well supported. They need a special treatment.
This CL fixes the crash reported in the bug.

BUG=649576
==========

to

==========
Fix FlatTreeTraversal for a slot in a document tree

Fix FlatTreeTraversal so that it considers a slot in a document tree correctly.

Slots in a document tree are not well supported. They need a special treatment.
This CL fixes the crash reported in the bug.

BUG=649576

Committed: https://crrev.com/dce2e40e8f3b783ef18e06b72c955bf59ed122d9
Cr-Commit-Position: refs/heads/master@{#425252}
==========

[commit-bot: I haz the power, 2016-10-14 04:26:51]

Patchset 3 (id:??) landed as
https://crrev.com/dce2e40e8f3b783ef18e06b72c955bf59ed122d9
Cr-Commit-Position: refs/heads/master@{#425252}

[johnme, 2016-10-14 11:21:42]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2420883002/ by johnme@chromium.org.

The reason for reverting is: The following content_browsertests have started
consistently failing on KitKat Tablet Tester, Lollipop Tablet Tester and
Marshmallow Tablet Tester:

FindRequestManagerTests/FindRequestManagerTest.RemoveFrame/0
FindRequestManagerTest.AddFrameAfterNoMatches
FindRequestManagerTests/FindRequestManagerTest.AddFrame/0
FindRequestManagerTest.HiddenFrame
FindRequestManagerTests/FindRequestManagerTest.CharacterByCharacter/0
FindRequestManagerTests/FindRequestManagerTest.Basic/0
FindRequestManagerTest.FindInPage_Issue627799
FindRequestManagerTest.ActivateNearestFindMatch
FindRequestManagerTest.FindMatchRects
FindRequestManagerTests/FindRequestManagerTest.NavigateFrame/0
FindRequestManagerTest.FindInPage_Issue644448
FindRequestManagerTests/FindRequestManagerTest.FindNewMatches/0
FindRequestManagerTests/FindRequestManagerTest.RapidFire/0

e.g.
https://build.chromium.org/p/chromium.android/builders/Marshmallow%20Tablet%2...

And the stack_tool_with_logcat_dump/stack_tool_for_tombstones steps both
prominently feature
findPlainTextInternal<blink::EditingAlgorithm<blink::FlatTreeTraversal>>

(oddly on KitKat Tablet Tester these tests started failing on the build after
this patch landed; presumably the failure is at least partly flaky).

[hayato, 2016-10-17 04:08:36]

This CL is not culprit because the tests still fail after reverting the CL: 
https://build.chromium.org/p/chromium.android/builders/Marshmallow%20Tablet%2...

Let me reland this CL.

[hayato, 2016-10-17 04:21:38]

reland