Do not use Node in FocusController, 2nd

Do not use Node in FocusController, 2nd

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

- 1st CL is here: https://codereview.chromium.org/1704333002/

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

[hayato, 2016-02-18 12:31:28]

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:33:28]

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

[hayato, 2016-02-18 12:38:52]

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

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

rebased

[commit-bot: I haz the power, 2016-02-18 12:40:08]

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

[commit-bot: I haz the power, 2016-02-18 13:43:27]

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

[commit-bot: I haz the power, 2016-02-18 13:43:28]

Dry run: This issue passed the CQ dry run.

[hayato, 2016-02-19 04:26:35]

Description was changed from

==========
Do not use Node in FocusController, 2nd

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

to

==========
Do not use Node in FocusController, 2nd

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

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

[hayato, 2016-02-19 04:26:59]

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

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

PTAL

[kochi, 2016-02-19 06:11:33]

Could you write even one-line description of what this CL
does? (referring to the other CL is okay, of course)

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

https://codereview.chromium.org/1711673002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FocusController.cpp:537: return (type ==
WebFocusTypeForward) ? ElementTraversal::next(*node) :
ElementTraversal::previous(*node);
I think if the |node| is an Element, you should return |toElement(node)|,
instead of traversing.

[hayato, 2016-02-19 06:20:20]

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

[hayato, 2016-02-19 06:20:22]

fix

[commit-bot: I haz the power, 2016-02-19 06:20:52]

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

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

Thank you for the review.

FYI. The 3rd is ready to be reviewed:
https://codereview.chromium.org/1707373002/

Could you review this too?

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

https://codereview.chromium.org/1711673002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FocusController.cpp:537: return (type ==
WebFocusTypeForward) ? ElementTraversal::next(*node) :
ElementTraversal::previous(*node);
On 2016/02/19 06:11:33, kochi wrote:
> I think if the |node| is an Element, you should return |toElement(node)|,
> instead of traversing.

Done.

[kochi, 2016-02-19 06:44:23]

hmm, the subject (the first line in the description)
says the summary and all other details are described
in the referred CL?  

I thought you didn't mention anchorNode() in the
referred CL's description, so the same description
doesn't apply to this CL.  That's why I suggested
adding even a one-line summary.

[hayato, 2016-02-19 06:52:05]

Description was changed from

==========
Do not use Node in FocusController, 2nd

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

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

to

==========
Do not use Node in FocusController, 2nd

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

- 1st CL is here: https://codereview.chromium.org/1704333002/ 

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

[hayato, 2016-02-19 06:56:39]

I've updated the description. I think the referring to the bug is good enough in
this CL.

[kochi, 2016-02-19 07:01:57]

On 2016/02/19 06:56:39, hayato wrote:
> I've updated the description. I think the referring to the bug is good enough
in
> this CL.

Thanks, lgtm.

[hayato, 2016-02-19 07:22:00]

The CQ bit was unchecked by hayato@chromium.org

[hayato, 2016-02-19 07:22:01]

The CQ bit was checked by hayato@chromium.org

[commit-bot: I haz the power, 2016-02-19 07:22:48]

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

[commit-bot: I haz the power, 2016-02-19 07:37:27]

Description was changed from

==========
Do not use Node in FocusController, 2nd

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

- 1st CL is here: https://codereview.chromium.org/1704333002/ 

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

to

==========
Do not use Node in FocusController, 2nd

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

- 1st CL is here: https://codereview.chromium.org/1704333002/ 

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

[commit-bot: I haz the power, 2016-02-19 07:37:28]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-19 07:38:52]

Description was changed from

==========
Do not use Node in FocusController, 2nd

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

- 1st CL is here: https://codereview.chromium.org/1704333002/ 

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

to

==========
Do not use Node in FocusController, 2nd

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

- 1st CL is here: https://codereview.chromium.org/1704333002/

BUG=587743

Committed: https://crrev.com/513d58f7d6622a3c8897d911f3a67f6c57f23fc6
Cr-Commit-Position: refs/heads/master@{#376414}
==========

[commit-bot: I haz the power, 2016-02-19 07:38:53]

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

[tkent, 2016-02-22 06:07:07]

tkent@chromium.org changed reviewers:
+ tkent@chromium.org

[tkent, 2016-02-22 06:07:08]

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

https://codereview.chromium.org/1711673002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FocusController.cpp:539: return (type ==
WebFocusTypeForward) ? ElementTraversal::next(*node) :
ElementTraversal::previous(*node);
If ElementTraversal::next(*node) is focusable, the element is not focused by TAB
key, right?
It's a behavior change.

[hayato, 2016-02-24 03:36:31]

On 2016/02/22 06:07:08, tkent wrote:
>
https://codereview.chromium.org/1711673002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/page/FocusController.cpp (right):
> 
>
https://codereview.chromium.org/1711673002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/page/FocusController.cpp:539: return (type ==
> WebFocusTypeForward) ? ElementTraversal::next(*node) :
> ElementTraversal::previous(*node);
> If ElementTraversal::next(*node) is focusable, the element is not focused by
TAB
> key, right?
> It's a behavior change.

Thank you. Let me take a look.

[hayato, 2016-03-15 07:08:16]

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

https://codereview.chromium.org/1711673002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FocusController.cpp:740:
ASSERT(document->documentElement());
I guess this assertion is not always true and this is the cause of the crash.
See https://bugs.chromium.org/p/chromium/issues/detail?id=594841