Port more focus controller unit tests and fix our focus rules.

Port more focus controller unit tests and fix our focus rules.

Now that we're seriously testing the focus controller stuff, the tests
are pointing out major problems in our focus rules implementation. This
brings our BasicFocusRules up to the aura version, which is needed for
the tests ported over.

BUG=431047
R=sky@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/d0d96fc2569a45bc290830627331f7757798f318

[Elliot Glaysher, 2014-11-21 22:03:21]

erg@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2014-11-21 22:42:19]

https://codereview.chromium.org/747163002/diff/1/mojo/services/window_manager...
File mojo/services/window_manager/basic_focus_rules.cc (right):

https://codereview.chromium.org/747163002/diff/1/mojo/services/window_manager...
mojo/services/window_manager/basic_focus_rules.cc:18: if (view->GetRoot() ==
view)
I don't get this logic. Isn't the same as:

return view->GetRoot()->visible()?

Which doesn't seem right either. Maybe you want view->IsDrawn()? (I see a bug in
that implementation, I'll fix it shortly).

https://codereview.chromium.org/747163002/diff/1/mojo/services/window_manager...
mojo/services/window_manager/basic_focus_rules.cc:38: if (!view->GetRoot())
I don't think this is the check you're after. GetRoot() always returns non-null.
I think you want the old logic, possibly with a fixed IsDrawn().

[sky, 2014-11-21 22:48:28]

I take back my comments on IsDrawn(), it should be correct.

[Elliot Glaysher, 2014-11-24 20:29:54]

https://codereview.chromium.org/747163002/diff/1/mojo/services/window_manager...
File mojo/services/window_manager/basic_focus_rules.cc (right):

https://codereview.chromium.org/747163002/diff/1/mojo/services/window_manager...
mojo/services/window_manager/basic_focus_rules.cc:18: if (view->GetRoot() ==
view)
On 2014/11/21 22:42:19, sky wrote:
> I don't get this logic. Isn't the same as:
> 
> return view->GetRoot()->visible()?

Correct, this is equivalent.

> Which doesn't seem right either. Maybe you want view->IsDrawn()? (I see a bug
in
> that implementation, I'll fix it shortly).

So the aura version is this:

bool Window::CanFocus() const {
  if (IsRootWindow())
    return IsVisible();

  // A bunch of checks we can't implement.

  return parent_->CanFocus();
}

IsDrawn() is probably not what we want. It appears to have the additional
restriction that the current view has to be visible, which isn't present in the
aura version which only checks if the root is visible. More importantly,
IsDrawn() appears to always be false in unit tests because we aren't actually
connected to a view manager.

As for why I wrote it this way, it was to emphasize that there's a check on each
view as it goes up the hierarchy. I've added a comment here explaining that.

https://codereview.chromium.org/747163002/diff/1/mojo/services/window_manager...
mojo/services/window_manager/basic_focus_rules.cc:38: if (!view->GetRoot())
On 2014/11/21 22:42:19, sky wrote:
> I don't think this is the check you're after. GetRoot() always returns
non-null.
> I think you want the old logic, possibly with a fixed IsDrawn().

Made the old logic work on unit tests.

[sky, 2014-11-24 21:49:27]

https://codereview.chromium.org/747163002/diff/40001/mojo/services/window_man...
File mojo/services/window_manager/basic_focus_rules.cc (right):

https://codereview.chromium.org/747163002/diff/40001/mojo/services/window_man...
mojo/services/window_manager/basic_focus_rules.cc:17: bool ViewCanFocus(View*
view) {
This name is awful. CanFocusView is much better, but conflicts with the same
name in this class. How about CanFocusViewImpl?

https://codereview.chromium.org/747163002/diff/40001/mojo/services/window_man...
mojo/services/window_manager/basic_focus_rules.cc:20: if (view->GetRoot() ==
view)
GetRoot() is not the same as aura::Window::IsRootWindow. The equivalent of
aura::Window::IsRootWindow is something like:

view->view_manager()->GetRoots().contains(view);

https://codereview.chromium.org/747163002/diff/40001/mojo/services/window_man...
mojo/services/window_manager/basic_focus_rules.cc:23: // TODO(erg): Add the
intermediary delegate and event client checks once we
This TODO makes it clearer why you have this structure, thanks.

[Elliot Glaysher, 2014-11-25 00:15:26]

https://codereview.chromium.org/747163002/diff/40001/mojo/services/window_man...
File mojo/services/window_manager/basic_focus_rules.cc (right):

https://codereview.chromium.org/747163002/diff/40001/mojo/services/window_man...
mojo/services/window_manager/basic_focus_rules.cc:20: if (view->GetRoot() ==
view)
On 2014/11/24 21:49:27, sky wrote:
> GetRoot() is not the same as aura::Window::IsRootWindow. The equivalent of
> aura::Window::IsRootWindow is something like:
> 
> view->view_manager()->GetRoots().contains(view);

Done.

To support this, I extracted the actual ViewManager interface from
ViewManagerClientImpl. Most of the usage of the interface was previously
static_cast<ViewManagerClientImpl*>(view_manager_), which obviously doesn't work
when you have a testing subclass of your interface.

[Elliot Glaysher, 2014-12-02 23:38:24]

OK, this redoes how we test for roots.

While working on the app test version, I noticed that things stopped working
when I brought in the test on view manager roots. With some experimentation, I
found that I didn't want to test for view manager root, I wanted to test for
being a direct child of the window manager view. (Which is a child of the root.)

AFAICT, the whole thing regarding roots worked in aura because you didn't have
an extra window between the root window and the client windows. (I might be
wrong about that though; I only glanced at the aura code here.)

[sky, 2014-12-02 23:59:30]

LGTM

[Elliot Glaysher, 2014-12-03 00:03:00]

Committed patchset #7 (id:120001) manually as
d0d96fc2569a45bc290830627331f7757798f318 (presubmit successful).