Move asserts in FocusNavigationController.

Move asserts in FocusNavigationController.

An assertion in
FocusController::findFocusableElementAcrossFocusScopesForward and
FocusController::findFocusableElementAcrossFocusScopesBackward
assumed that focus navigation happens from a focused
element to another, but with the introduction of
sequential focus navigation starting point,
it is no longer true.

Moving the assertion to where appropriate.

BUG=609012
TEST=FocusControllerTest.DoNotCrash{1,2}

Committed: https://crrev.com/65a82e070e5102bc29de52ddf6071018d703c304
Cr-Commit-Position: refs/heads/master@{#393194}

[kochi, 2016-05-06 10:40:46]

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

[commit-bot: I haz the power, 2016-05-06 10:41:01]

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

[commit-bot: I haz the power, 2016-05-06 11:59:52]

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

[commit-bot: I haz the power, 2016-05-06 11:59:53]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kochi, 2016-05-09 04:04:18]

Description was changed from

==========
*DO NOT COMMIT* Remove false assert in FocusNavigationController.

This adds window.internals.setSequentialFocusNavigationStartingPoint()
and window.internals.sequentialFocusNavigationStartingPoint()

tentative for debug

BUG=609012
==========

to

==========
Remove false assert in FocusNavigationController.

Assertion in FocusController.cpp assumed that focus
navigation happens from a focused element to another,
but with the introduction of sequential focus navigation
starting point, it is no longer true.

This also adds
window.internals.setSequentialFocusNavigationStartingPoint().

BUG=609012
==========

[kochi, 2016-05-09 04:04:18]

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

[kochi, 2016-05-09 04:05:40]

PTAL

[kochi, 2016-05-09 05:08:24]

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

[commit-bot: I haz the power, 2016-05-09 05:08:34]

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

[commit-bot: I haz the power, 2016-05-09 06:01:20]

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

[commit-bot: I haz the power, 2016-05-09 06:01:20]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hayato, 2016-05-10 07:58:15]

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

https://codereview.chromium.org/1954023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FocusController.cpp:649: ASSERT(!current ||
!isNonFocusableShadowHost(*current));
Could you help me to understand how this ASSERT hits in the test?

It is not good to delete this ASSERT due to the feature which was introduced
recently.
Is there any way to preserve this ASSERT? Can we distinguish a special case and
preserve this ASSERT if we are not in a special case?

[kochi, 2016-05-10 09:49:52]

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

https://codereview.chromium.org/1954023002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FocusController.cpp:649: ASSERT(!current ||
!isNonFocusableShadowHost(*current));
On 2016/05/10 07:58:15, hayato wrote:
> Could you help me to understand how this ASSERT hits in the test?
> 
> It is not good to delete this ASSERT due to the feature which was introduced
> recently.
> Is there any way to preserve this ASSERT? Can we distinguish a special case
and
> preserve this ASSERT if we are not in a special case?

This assertion is, if de morgan's law is applied:
ASSERT(!(current && isNonFocusableShadowHost(*current)))

and the condition can be expanded to:
!(current && isShadowHost(current) && !hasCustomFocusLogic(current) &&
!current.isFocusable())

thus, this was intended to catch "current is not null, is a shadow host,
doesn't have custom focus logic, and is not focusable" condition.

All the 4 cases have to be met, but what was significant here was
"if shadow host is passed and it is not focusable".

A shadow host usually forms "scope" (inside its shadow root), and
shadow host should not be directly passed down to this function,
instead a scope with its shadow root as its root should be passed.
The case when a shadow host itself is passed down to this function
is when it had focus.

After sequential focus navigation starting point was introduced,
any node can be the starting point.  But about the same time,
a refactoring to restrict any starting point to Element (not Node)
was done.  Thus sequential focus navigation starting point introduced
a hack to return neighboring element to the exact node, depending on
the direction, e.g. if direction is forward, previous element is returned
as the starting point.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Therefore any Element which did not have focus can be passed as
a starting point, and it can be an element that meets all the
4 conditions above.  On material-design chrome:// pages it
is easy to hit the condition as it uses Polymer with native
Shadow DOM (thus it was reported from internal, not from the wild web).

I think the right solution is to allow any Node (not Element) to
be a starting point and remove the neighboring hack of sequential
focus navigation starting point.

[kochi, 2016-05-10 09:57:48]

On 2016/05/10 09:49:52, kochi wrote:
>
https://codereview.chromium.org/1954023002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/page/FocusController.cpp (left):
> 
>
https://codereview.chromium.org/1954023002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/page/FocusController.cpp:649: ASSERT(!current
||
> !isNonFocusableShadowHost(*current));
> On 2016/05/10 07:58:15, hayato wrote:
> > Could you help me to understand how this ASSERT hits in the test?
> > 
> > It is not good to delete this ASSERT due to the feature which was introduced
> > recently.
> > Is there any way to preserve this ASSERT? Can we distinguish a special case
> and
> > preserve this ASSERT if we are not in a special case?
> 
> This assertion is, if de morgan's law is applied:
> ASSERT(!(current && isNonFocusableShadowHost(*current)))
> 
> and the condition can be expanded to:
> !(current && isShadowHost(current) && !hasCustomFocusLogic(current) &&
> !current.isFocusable())
> 
> thus, this was intended to catch "current is not null, is a shadow host,
> doesn't have custom focus logic, and is not focusable" condition.
> 
> All the 4 cases have to be met, but what was significant here was
> "if shadow host is passed and it is not focusable".
> 
> A shadow host usually forms "scope" (inside its shadow root), and
> shadow host should not be directly passed down to this function,
> instead a scope with its shadow root as its root should be passed.
> The case when a shadow host itself is passed down to this function
> is when it had focus.
> 
> After sequential focus navigation starting point was introduced,
> any node can be the starting point.  But about the same time,
> a refactoring to restrict any starting point to Element (not Node)
> was done.  Thus sequential focus navigation starting point introduced
> a hack to return neighboring element to the exact node, depending on
> the direction, e.g. if direction is forward, previous element is returned
> as the starting point.
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Therefore any Element which did not have focus can be passed as
> a starting point, and it can be an element that meets all the
> 4 conditions above.  On material-design chrome:// pages it
> is easy to hit the condition as it uses Polymer with native
> Shadow DOM (thus it was reported from internal, not from the wild web).
> 
> I think the right solution is to allow any Node (not Element) to
> be a starting point and remove the neighboring hack of sequential
> focus navigation starting point.

I'd like to work on removing the "neighboring element" hack in later CL,
but fixing this assertion is somewhat urgent as it is blocking the
MD chrome:// pages development.

[hayato, 2016-05-10 11:10:13]

Thanks. The point is how we should minimize the scarification.

The current sequential focus navigation code is, implicitly, assuming that both
of pre-condition and post-condition are satisfied by this ASSERT:

A. Pre-condition: isRegularFocusableElement(before)
B. Post-condition: isRegularFocusableElement(after))

As far as I can read, it turned out that only A became invalid, but B is still
valid assumption, right?
If we remove ASESRT, as this CL does, we lost both assumptions from our code
base.
What we have to give up should be only A. We can still assert post-condition.

Ideally, we should keep A somehow with an exceptional case because A is still a
valid assumption in most cases.

i.e.
A': Pre-condition: isRegularFocusableElement(before) ||
isExceptionalStartingPointWhichIsGuessedByRange(before)

But I am not sure that keeping A' is worth doing because we need additional
*flag* or parameter to distinguish these cases, I guess.

[kochi, 2016-05-10 11:16:21]

On 2016/05/10 11:10:13, hayato wrote:
> Thanks. The point is how we should minimize the scarification.
> 
> The current sequential focus navigation code is, implicitly, assuming that
both
> of pre-condition and post-condition are satisfied by this ASSERT:
> 
> A. Pre-condition: isRegularFocusableElement(before)
> B. Post-condition: isRegularFocusableElement(after))
> 
> As far as I can read, it turned out that only A became invalid, but B is still
> valid assumption, right?
> If we remove ASESRT, as this CL does, we lost both assumptions from our code
> base.
> What we have to give up should be only A. We can still assert post-condition.
> 
> Ideally, we should keep A somehow with an exceptional case because A is still
a
> valid assumption in most cases.
> 
> i.e.
> A': Pre-condition: isRegularFocusableElement(before) ||
> isExceptionalStartingPointWhichIsGuessedByRange(before)
> 
> But I am not sure that keeping A' is worth doing because we need additional
> *flag* or parameter to distinguish these cases, I guess.

What do you mean by pre/post-condition?

[kochi, 2016-05-10 12:21:14]

On 2016/05/10 11:16:21, kochi wrote:
> On 2016/05/10 11:10:13, hayato wrote:
> > Thanks. The point is how we should minimize the scarification.
> > 
> > The current sequential focus navigation code is, implicitly, assuming that
> both
> > of pre-condition and post-condition are satisfied by this ASSERT:
> > 
> > A. Pre-condition: isRegularFocusableElement(before)
> > B. Post-condition: isRegularFocusableElement(after))
> > 
> > As far as I can read, it turned out that only A became invalid, but B is
still
> > valid assumption, right?
> > If we remove ASESRT, as this CL does, we lost both assumptions from our code
> > base.
> > What we have to give up should be only A. We can still assert
post-condition.
> > 
> > Ideally, we should keep A somehow with an exceptional case because A is
still
> a
> > valid assumption in most cases.
> > 
> > i.e.
> > A': Pre-condition: isRegularFocusableElement(before) ||
> > isExceptionalStartingPointWhichIsGuessedByRange(before)
> > 
> > But I am not sure that keeping A' is worth doing because we need additional
> > *flag* or parameter to distinguish these cases, I guess.
> 
> What do you mean by pre/post-condition?

Discussed off-line.
To answer my own question, pre-condition is what was written in the original
assert,
and post-condition is an assertion for the return value.

[kochi, 2016-05-10 12:28:04]

Description was changed from

==========
Remove false assert in FocusNavigationController.

Assertion in FocusController.cpp assumed that focus
navigation happens from a focused element to another,
but with the introduction of sequential focus navigation
starting point, it is no longer true.

This also adds
window.internals.setSequentialFocusNavigationStartingPoint().

BUG=609012
==========

to

==========
Remove false assert in FocusNavigationController.

Assertion in FocusController.cpp assumed that focus
navigation happens from a focused element to another,
but with the introduction of sequential focus navigation
starting point, it is no longer true.

BUG=609012
TEST=FocusControllerTest.DoNotCrash{1,2}
==========

[kochi, 2016-05-10 12:28:28]

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

[commit-bot: I haz the power, 2016-05-10 12:28:42]

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

[commit-bot: I haz the power, 2016-05-10 13:43:13]

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

[commit-bot: I haz the power, 2016-05-10 13:43:14]

Dry run: This issue passed the CQ dry run.

[kochi, 2016-05-11 01:45:06]

PTAL

[kochi, 2016-05-11 10:11:09]

On 2016/05/11 01:45:06, kochi wrote:
> PTAL

Ping?

[hayato, 2016-05-11 12:44:44]

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

https://codereview.chromium.org/1954023002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FocusController.cpp:907: current =
document->sequentialFocusNavigationStartingPoint(type);
I think we can distinguish special cases, where `current` is not focusable,
around here.

It looks that L907 - L913 are the source of the exceptional cases.

If we can distinguish exceptional cases and usual cases (where element is
focusable) by using bool flag around here, we do not have to remove ASSERTION
for usual cases.

Could you try to preserve ASSERTION?

[kochi, 2016-05-12 01:10:12]

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

[commit-bot: I haz the power, 2016-05-12 01:10:37]

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

[kochi, 2016-05-12 01:13:58]

Description was changed from

==========
Remove false assert in FocusNavigationController.

Assertion in FocusController.cpp assumed that focus
navigation happens from a focused element to another,
but with the introduction of sequential focus navigation
starting point, it is no longer true.

BUG=609012
TEST=FocusControllerTest.DoNotCrash{1,2}
==========

to

==========
Move asserts in FocusNavigationController.

An assertion in
FocusController::findFocusableElementAcrossFocusScopesForward
assumed that focus navigation happens from a focused
element to another, but with the introduction of
sequential focus navigation starting point,
it is no longer true.

Moving the assertion to where appropriate.

BUG=609012
TEST=FocusControllerTest.DoNotCrash{1,2}
==========

[kochi, 2016-05-12 01:14:52]

Description was changed from

==========
Move asserts in FocusNavigationController.

An assertion in
FocusController::findFocusableElementAcrossFocusScopesForward
assumed that focus navigation happens from a focused
element to another, but with the introduction of
sequential focus navigation starting point,
it is no longer true.

Moving the assertion to where appropriate.

BUG=609012
TEST=FocusControllerTest.DoNotCrash{1,2}
==========

to

==========
Move asserts in FocusNavigationController.

An assertion in
FocusController::findFocusableElementAcrossFocusScopesForward and
FocusController::findFocusableElementAcrossFocusScopesBackward
assumed that focus navigation happens from a focused
element to another, but with the introduction of
sequential focus navigation starting point,
it is no longer true.

Moving the assertion to where appropriate.

BUG=609012
TEST=FocusControllerTest.DoNotCrash{1,2}
==========

[kochi, 2016-05-12 02:09:21]

PTAL

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

https://codereview.chromium.org/1954023002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FocusController.cpp:907: current =
document->sequentialFocusNavigationStartingPoint(type);
On 2016/05/11 12:44:44, hayato wrote:
> I think we can distinguish special cases, where `current` is not focusable,
> around here.
> 
> It looks that L907 - L913 are the source of the exceptional cases.
> 
> If we can distinguish exceptional cases and usual cases (where element is
> focusable) by using bool flag around here, we do not have to remove ASSERTION
> for usual cases.
> 
> Could you try to preserve ASSERTION?

Done.

[commit-bot: I haz the power, 2016-05-12 03:16:48]

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

[commit-bot: I haz the power, 2016-05-12 03:16:49]

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

[hayato, 2016-05-12 03:49:59]

lgtm

[kochi, 2016-05-12 03:59:30]

The CQ bit was checked by kochi@chromium.org

[commit-bot: I haz the power, 2016-05-12 03:59:46]

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

[commit-bot: I haz the power, 2016-05-12 06:08:32]

Description was changed from

==========
Move asserts in FocusNavigationController.

An assertion in
FocusController::findFocusableElementAcrossFocusScopesForward and
FocusController::findFocusableElementAcrossFocusScopesBackward
assumed that focus navigation happens from a focused
element to another, but with the introduction of
sequential focus navigation starting point,
it is no longer true.

Moving the assertion to where appropriate.

BUG=609012
TEST=FocusControllerTest.DoNotCrash{1,2}
==========

to

==========
Move asserts in FocusNavigationController.

An assertion in
FocusController::findFocusableElementAcrossFocusScopesForward and
FocusController::findFocusableElementAcrossFocusScopesBackward
assumed that focus navigation happens from a focused
element to another, but with the introduction of
sequential focus navigation starting point,
it is no longer true.

Moving the assertion to where appropriate.

BUG=609012
TEST=FocusControllerTest.DoNotCrash{1,2}
==========

[commit-bot: I haz the power, 2016-05-12 06:08:33]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-12 06:09:55]

Description was changed from

==========
Move asserts in FocusNavigationController.

An assertion in
FocusController::findFocusableElementAcrossFocusScopesForward and
FocusController::findFocusableElementAcrossFocusScopesBackward
assumed that focus navigation happens from a focused
element to another, but with the introduction of
sequential focus navigation starting point,
it is no longer true.

Moving the assertion to where appropriate.

BUG=609012
TEST=FocusControllerTest.DoNotCrash{1,2}
==========

to

==========
Move asserts in FocusNavigationController.

An assertion in
FocusController::findFocusableElementAcrossFocusScopesForward and
FocusController::findFocusableElementAcrossFocusScopesBackward
assumed that focus navigation happens from a focused
element to another, but with the introduction of
sequential focus navigation starting point,
it is no longer true.

Moving the assertion to where appropriate.

BUG=609012
TEST=FocusControllerTest.DoNotCrash{1,2}

Committed: https://crrev.com/65a82e070e5102bc29de52ddf6071018d703c304
Cr-Commit-Position: refs/heads/master@{#393194}
==========

[commit-bot: I haz the power, 2016-05-12 06:09:56]

Patchset 4 (id:??) landed as
https://crrev.com/65a82e070e5102bc29de52ddf6071018d703c304
Cr-Commit-Position: refs/heads/master@{#393194}