Views: Fixed exponential behaviour on reverse focus search (Shift+Tab).

Views: Fixed exponential behaviour on reverse focus search (Shift+Tab).

In FindPreviousFocusableViewImpl, in the recursive call to visit the
sibling node, preserves can_go_up, rather than forcing it to true.

Added FocusTraversalNonFocusableTest (test the pathological case
outlined in the bug report).

Also fixed existing comment in test file describing the view hierarchy
to match reality.

BUG=453699
BUG=438425
BUG=451140
Committed: https://crrev.com/9e437a3204667d850636918942ac01f0d933ae83
Cr-Commit-Position: refs/heads/master@{#314285}

[Matt Giuca, 2015-01-30 03:03:04]

mgiuca@chromium.org changed reviewers:
+ dmazzoni@chromium.org, sky@chromium.org

[Matt Giuca, 2015-01-30 03:03:04]

Please see http://crbug.com/453699 for a detailed explanation of the problem and
why my test case triggers it.

[Matt Giuca, 2015-01-30 03:03:57]

(dmazzoni, because you were the original author; sky, because you were the
original reviewer and for OWNERS.)

[dmazzoni, 2015-02-02 22:07:05]

lgtm

Nice job with the tests and comments, it's really clear.

https://codereview.chromium.org/886033002/diff/1/ui/views/focus/focus_travers...
File ui/views/focus/focus_traversal_unittest.cc (right):

https://codereview.chromium.org/886033002/diff/1/ui/views/focus/focus_travers...
ui/views/focus/focus_traversal_unittest.cc:790: void
FocusTraversalNonFocusableTest::InitContentView() {
Just to clarify, is your intent that this test would time out if exponential?

Maybe make that explicit - in case somebody in the future re-introduces a
similar bug and the test times out signaling it's exponential.

[Matt Giuca, 2015-02-02 23:17:31]

https://codereview.chromium.org/886033002/diff/1/ui/views/focus/focus_travers...
File ui/views/focus/focus_traversal_unittest.cc (right):

https://codereview.chromium.org/886033002/diff/1/ui/views/focus/focus_travers...
ui/views/focus/focus_traversal_unittest.cc:790: void
FocusTraversalNonFocusableTest::InitContentView() {
Yes, that is the behaviour of the test prior to the fix.

Good suggestion, added a comment below.

[sky, 2015-02-03 00:42:01]

LGTM

[Matt Giuca, 2015-02-03 02:39:36]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2015-02-03 02:40:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/886033002/20001

[commit-bot: I haz the power, 2015-02-03 04:42:20]

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

[commit-bot: I haz the power, 2015-02-03 04:42:21]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium.win (None)

[Matt Giuca, 2015-02-03 07:34:39]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2015-02-03 07:34:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/886033002/20001

[commit-bot: I haz the power, 2015-02-03 07:35:45]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-02-03 07:37:01]

Patchset 2 (id:??) landed as
https://crrev.com/9e437a3204667d850636918942ac01f0d933ae83
Cr-Commit-Position: refs/heads/master@{#314285}