Do not use Node in FocusController, 3rd

Do not use Node in FocusController, 3rd

This is one of the series of patches. See the bug for the motivation.

- 1st CL: https://codereview.chromium.org/1704333002/
- 2nd CL: https://codereview.chromium.org/1711673002/

BUG=587743
Committed: https://crrev.com/fbc4ecfc0d9e7b70f3a0c4fd79d837ff87fc6faf
Cr-Commit-Position: refs/heads/master@{#376680}

[hayato, 2016-02-18 12:49:19]

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

[commit-bot: I haz the power, 2016-02-18 12:49:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1707373002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1707373002/1

[hayato, 2016-02-18 12:53:29]

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

[hayato, 2016-02-18 12:53:30]

fix typo

[commit-bot: I haz the power, 2016-02-18 12:54:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1707373002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1707373002/20001

[commit-bot: I haz the power, 2016-02-18 14:22:27]

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

[commit-bot: I haz the power, 2016-02-18 14:22:28]

Dry run: This issue passed the CQ dry run.

[hayato, 2016-02-19 04:27:41]

Description was changed from

==========
Do not use Node in FocusController, 3rd

BUG=587743
==========

to

==========
Do not use Node in FocusController, 3rd

See https://codereview.chromium.org/1704333002/ for details.

BUG=587743
==========

[hayato, 2016-02-19 04:28:41]

Description was changed from

==========
Do not use Node in FocusController, 3rd

See https://codereview.chromium.org/1704333002/ for details.

BUG=587743
==========

to

==========
Do not use Node in FocusController, 3rd

See https://codereview.chromium.org/1704333002/ for details.

- 1st: https://codereview.chromium.org/1711673002/
- 2nd: https://codereview.chromium.org/1711673002/

BUG=587743
==========

[hayato, 2016-02-19 04:29:15]

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

[hayato, 2016-02-19 04:29:16]

PTAL

[hayato, 2016-02-19 05:53:39]

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

[hayato, 2016-02-19 06:53:11]

Description was changed from

==========
Do not use Node in FocusController, 3rd

See https://codereview.chromium.org/1704333002/ for details.

- 1st: https://codereview.chromium.org/1711673002/
- 2nd: https://codereview.chromium.org/1711673002/

BUG=587743
==========

to

==========
Do not use Node in FocusController, 3rd

This is one of the series of patches. See the bug for the motivation.

- 1st CL: https://codereview.chromium.org/1711673002/
- 2nd CL: https://codereview.chromium.org/1711673002/

BUG=587743
==========

[hayato, 2016-02-19 06:54:42]

Description was changed from

==========
Do not use Node in FocusController, 3rd

This is one of the series of patches. See the bug for the motivation.

- 1st CL: https://codereview.chromium.org/1711673002/
- 2nd CL: https://codereview.chromium.org/1711673002/

BUG=587743
==========

to

==========
Do not use Node in FocusController, 3rd

This is one of the series of patches. See the bug for the motivation.

- 1st CL: https://codereview.chromium.org/1704333002/
- 2nd CL: https://codereview.chromium.org/1711673002/

BUG=587743
==========

[kochi, 2016-02-19 07:25:35]

is this the last one?

https://codereview.chromium.org/1707373002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/1707373002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FocusController.cpp:103: Node& root =
m_rootTreeScope->rootNode();
nit: |ContainerNode&| ?

https://codereview.chromium.org/1707373002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FocusController.cpp:110: return
last->isElementNode() ? toElement(last) : ElementTraversal::previous(*last);
Can |ElementTraversal::lastWithin(rootNode())| return the same for these 2
lines?

https://codereview.chromium.org/1707373002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FocusController.cpp:116: Node& root =
rootNode();
nit: |ContainerNode&| ?

[hayato, 2016-02-19 07:47:17]

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

[hayato, 2016-02-19 07:47:20]

addressed

[commit-bot: I haz the power, 2016-02-19 07:47:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1707373002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1707373002/40001

[hayato, 2016-02-19 07:47:44]

https://codereview.chromium.org/1707373002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/1707373002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FocusController.cpp:103: Node& root =
m_rootTreeScope->rootNode();
On 2016/02/19 07:25:35, kochi wrote:
> nit: |ContainerNode&| ?

Done.

https://codereview.chromium.org/1707373002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FocusController.cpp:110: return
last->isElementNode() ? toElement(last) : ElementTraversal::previous(*last);
On 2016/02/19 07:25:35, kochi wrote:
> Can |ElementTraversal::lastWithin(rootNode())| return the same for these 2
> lines?

It can not. A node can have the non-element last child, which may have an
element child

https://codereview.chromium.org/1707373002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FocusController.cpp:116: Node& root =
rootNode();
On 2016/02/19 07:25:35, kochi wrote:
> nit: |ContainerNode&| ?

Done.

[hayato, 2016-02-19 07:49:38]

On 2016/02/19 07:25:35, kochi wrote:
> is this the last one?

I do not have a pending CL.

I can the same thing for spacial navigation, but it won not happen soon.

[hayato, 2016-02-19 08:19:03]

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

[hayato, 2016-02-19 08:19:05]

use ElementTraversal::lastWithin

[commit-bot: I haz the power, 2016-02-19 08:19:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1707373002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1707373002/60001

[hayato, 2016-02-19 08:21:28]

https://codereview.chromium.org/1707373002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/1707373002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FocusController.cpp:110: return
last->isElementNode() ? toElement(last) : ElementTraversal::previous(*last);
On 2016/02/19 07:47:44, hayato wrote:
> On 2016/02/19 07:25:35, kochi wrote:
> > Can |ElementTraversal::lastWithin(rootNode())| return the same for these 2
> > lines?
> 
> It can not. A node can have the non-element last child, which may have an
> element child
> 

I'm wrong. Looks ElementTraversal::lastWithin does the same thing as I did.
Done.

[kochi, 2016-02-19 08:21:39]

lgtm

(added note: we discussed offline about difference
between NodeTraversal::lastWithin() and ElementTraversal::lastWithin(), and
agreed that
ElementTraversal version does the expected thing.)

https://codereview.chromium.org/1707373002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/1707373002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FocusController.cpp:103: ContainerNode& root
= m_rootTreeScope->rootNode();
nit: |m_rootTreeScope->| can be omitted.

[hayato, 2016-02-19 08:25:45]

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

[hayato, 2016-02-19 08:25:47]

update

[hayato, 2016-02-19 08:25:55]

https://codereview.chromium.org/1707373002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/1707373002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FocusController.cpp:103: ContainerNode& root
= m_rootTreeScope->rootNode();
On 2016/02/19 08:21:39, kochi wrote:
> nit: |m_rootTreeScope->| can be omitted.

Done.

[hayato, 2016-02-19 08:26:06]

The CQ bit was unchecked by hayato@chromium.org

[hayato, 2016-02-19 08:26:07]

The CQ bit was checked by hayato@chromium.org

[hayato, 2016-02-19 08:26:07]

The patchset sent to the CQ was uploaded after l-g-t-m from kochi@chromium.org
Link to the patchset: https://codereview.chromium.org/1707373002/#ps80001
(title: "update")

[commit-bot: I haz the power, 2016-02-19 08:27:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1707373002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1707373002/80001

[commit-bot: I haz the power, 2016-02-19 12:22:24]

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

[commit-bot: I haz the power, 2016-02-19 12:22:25]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[hayato, 2016-02-19 12:40:21]

The CQ bit was checked by hayato@chromium.org

[commit-bot: I haz the power, 2016-02-19 12:41:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1707373002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1707373002/80001

[commit-bot: I haz the power, 2016-02-19 16:39:52]

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

[commit-bot: I haz the power, 2016-02-19 16:39:53]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[hayato, 2016-02-22 01:53:30]

The CQ bit was checked by hayato@chromium.org

[commit-bot: I haz the power, 2016-02-22 01:53:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1707373002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1707373002/80001

[commit-bot: I haz the power, 2016-02-22 03:00:01]

Description was changed from

==========
Do not use Node in FocusController, 3rd

This is one of the series of patches. See the bug for the motivation.

- 1st CL: https://codereview.chromium.org/1704333002/
- 2nd CL: https://codereview.chromium.org/1711673002/

BUG=587743
==========

to

==========
Do not use Node in FocusController, 3rd

This is one of the series of patches. See the bug for the motivation.

- 1st CL: https://codereview.chromium.org/1704333002/
- 2nd CL: https://codereview.chromium.org/1711673002/

BUG=587743
==========

[commit-bot: I haz the power, 2016-02-22 03:00:01]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-02-22 03:01:19]

Description was changed from

==========
Do not use Node in FocusController, 3rd

This is one of the series of patches. See the bug for the motivation.

- 1st CL: https://codereview.chromium.org/1704333002/
- 2nd CL: https://codereview.chromium.org/1711673002/

BUG=587743
==========

to

==========
Do not use Node in FocusController, 3rd

This is one of the series of patches. See the bug for the motivation.

- 1st CL: https://codereview.chromium.org/1704333002/
- 2nd CL: https://codereview.chromium.org/1711673002/

BUG=587743

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

[commit-bot: I haz the power, 2016-02-22 03:01:20]

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