Make a pure mojo::View version of the aura::Window FocusController.

Remove aura and make a pure mojo::View version of the aura::Window FocusController.

This patch is only half done, however the main driver (removing aura) is needed by other teams for them to make progress.

Input handling is regressed in this patch; while FocusController is rebuilt to the point where it should theoretically work, all of the details about ui::EventTargeter don't work correctly. There is an empty subclass ViewTargeter which will need to be filled out before

Thankfully, this should only regress people who are using the wm_flow demo, and this should unblock the android guys who don't have aura support on their platform.

BUG=431047
R=eseidel@chromium.org, sky@chromium.org

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

[Elliot Glaysher, 2014-11-06 21:11:20]

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

[Elliot Glaysher, 2014-11-06 21:17:00]

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

[Elliot Glaysher, 2014-11-06 21:29:54]

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

[Elliot Glaysher, 2014-11-06 21:29:55]

This patch is in a weird state to be sent out for review. It does two things:
remove aura from window_manager and sky, and partially reimplement the focus
controller, but in a way that doesn't actually deal with mouse input events
because event targeting doesn't work yet.

After talking to Ben, he mentioned that I should at least get something in to
the tree to unblock the android people (they don't want the aura dependency). I
wanted to get something more working, so I gave myself another day, but the
details about EventTargeter look subtle enough that it'll take more time to
figure out how to get this stuff working.

Likewise, I'm not sure how feasible it is to go backwards; some of the event
handling stuff could be pulled out, but I think we need some analog of the
FocusController for the unit tests to pass.

https://codereview.chromium.org/698543005/diff/40001/mojo/services/public/cpp...
File mojo/services/public/cpp/view_manager/lib/view.cc (right):

https://codereview.chromium.org/698543005/diff/40001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/view.cc:316: if (child == this)
This comes from aura: an aura window says it contains itself. It turns out that
this is needed in how the focus controller worked.

[eseidel, 2014-11-06 21:33:14]

sky parts lgtm once you add back the debugger/tester bits to BUILD.gn

https://codereview.chromium.org/698543005/diff/40001/sky/BUILD.gn
File sky/BUILD.gn (left):

https://codereview.chromium.org/698543005/diff/40001/sky/BUILD.gn#oldcode21
sky/BUILD.gn:21: "//sky/tools/debugger",
I think you just want to move these into the main deps.  This made it so
debugger and tester are never built. :/

https://codereview.chromium.org/698543005/diff/40001/sky/tools/debugger/debug...
File sky/tools/debugger/debugger.cc (right):

https://codereview.chromium.org/698543005/diff/40001/sky/tools/debugger/debug...
sky/tools/debugger/debugger.cc:64: new
mojo::BasicFocusRules(window_manager_app_.get(), content_));
It's kinda silly we have to call InitFocus at all, but I guess we need to tell
BasicFocusRules what our root view is...

[Elliot Glaysher, 2014-11-06 21:36:43]

https://codereview.chromium.org/698543005/diff/40001/sky/BUILD.gn
File sky/BUILD.gn (left):

https://codereview.chromium.org/698543005/diff/40001/sky/BUILD.gn#oldcode21
sky/BUILD.gn:21: "//sky/tools/debugger",
On 2014/11/06 21:33:14, eseidel wrote:
> I think you just want to move these into the main deps.  This made it so
> debugger and tester are never built. :/

<i-have-no-idea-what-I'm-doing-dog/>

(Done.)

[eseidel, 2014-11-06 21:43:56]

sky/* lgtm

[Elliot Glaysher, 2014-11-10 20:15:59]

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

[Elliot Glaysher, 2014-11-10 20:16:00]

redirecting review from ben to sky.

sky: For context, this patch actually regresses the wm_flow demo, but talking in
person with Ben, he said it didn't need to be perfect, just remove the aura
dependency so that I unblock the android folks. (I wrote more about this in the
first message.)

[sky, 2014-11-10 20:45:31]

https://codereview.chromium.org/698543005/diff/60001/mojo/services/public/cpp...
File mojo/services/public/cpp/view_manager/lib/view.cc (right):

https://codereview.chromium.org/698543005/diff/60001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/view.cc:314: if (!child)
Usually null means something is wrong. Seems like we should enforce non-null
here (like we do in aura::Window and views::View).

https://codereview.chromium.org/698543005/diff/60001/mojo/services/public/cpp...
File mojo/services/public/cpp/view_manager/view_tracker.h (right):

https://codereview.chromium.org/698543005/diff/60001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/view_tracker.h:18: typedef std::set<View*>
Views;
using Views = std::set<View*>

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

https://codereview.chromium.org/698543005/diff/60001/mojo/services/window_man...
mojo/services/window_manager/basic_focus_rules.cc:25: return view->parent() ==
window_container_;
Don't you want to check visibility/drawn in any of these? If you don't need to
check here because it's done in FocusController, document in FocusRules that
subclasses don't need to check it.

https://codereview.chromium.org/698543005/diff/60001/mojo/services/window_man...
mojo/services/window_manager/basic_focus_rules.cc:37: return NULL;
nullptr every where.

https://codereview.chromium.org/698543005/diff/60001/mojo/services/window_man...
File mojo/services/window_manager/basic_focus_rules.h (right):

https://codereview.chromium.org/698543005/diff/60001/mojo/services/window_man...
mojo/services/window_manager/basic_focus_rules.h:15: class BasicFocusRules :
public FocusRules {
Description.

https://codereview.chromium.org/698543005/diff/60001/mojo/services/window_man...
File mojo/services/window_manager/focus_controller.cc (right):

https://codereview.chromium.org/698543005/diff/60001/mojo/services/window_man...
mojo/services/window_manager/focus_controller.cc:17:
FocusController::FocusController(FocusRules* rules)
Who owns FocusRules? Seems like FocusController should and this should take a
scoped_ptr.

https://codereview.chromium.org/698543005/diff/60001/mojo/services/window_man...
mojo/services/window_manager/focus_controller.cc:67: 
I'm not clear on if visibility/drawn state should be checked here. Please
document why we should or shouldn't check it in this class.

https://codereview.chromium.org/698543005/diff/60001/mojo/services/window_man...
File mojo/services/window_manager/focus_controller.h (right):

https://codereview.chromium.org/698543005/diff/60001/mojo/services/window_man...
mojo/services/window_manager/focus_controller.h:20: // TODO: Maybe write a
better comments.
At least write a description for the class.

https://codereview.chromium.org/698543005/diff/60001/mojo/services/window_man...
mojo/services/window_manager/focus_controller.h:58: 
nit: generally no newline here.

https://codereview.chromium.org/698543005/diff/60001/mojo/services/window_man...
File mojo/services/window_manager/focus_rules.h (right):

https://codereview.chromium.org/698543005/diff/60001/mojo/services/window_man...
mojo/services/window_manager/focus_rules.h:13: class FocusRules {
Description?

https://codereview.chromium.org/698543005/diff/60001/mojo/services/window_man...
File mojo/services/window_manager/view_target.h (right):

https://codereview.chromium.org/698543005/diff/60001/mojo/services/window_man...
mojo/services/window_manager/view_target.h:22: // is
is?

https://codereview.chromium.org/698543005/diff/60001/mojo/services/window_man...
mojo/services/window_manager/view_target.h:38: scoped_ptr<ViewTargeter>
SetEventTargeter(scoped_ptr<ViewTargeter> targeter);
I wonder if we should support local properties on View. Would doing that more
natural than shadowing the whole tree?

https://codereview.chromium.org/698543005/diff/60001/mojo/services/window_man...
mojo/services/window_manager/view_target.h:58: };
DISALLOW...

[Elliot Glaysher, 2014-11-10 22:18:50]

ptal

https://codereview.chromium.org/698543005/diff/60001/mojo/services/public/cpp...
File mojo/services/public/cpp/view_manager/lib/view.cc (right):

https://codereview.chromium.org/698543005/diff/60001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/view.cc:314: if (!child)
On 2014/11/10 20:45:30, sky wrote:
> Usually null means something is wrong. Seems like we should enforce non-null
> here (like we do in aura::Window and views::View).

We don't enforce non-nullness in aura::Window or views::View, and rely on this
behaviour. Look at this:

bool Window::Contains(const Window* other) const {
  for (const Window* parent = other; parent; parent = parent->parent_) {
    if (parent == this)
      return true;
  }
  return false;
}

(The views::View is similar.)

I am not happy about this, but unraveling all the times we call
X::Contains(nullptr) isn't what I want to do yet. There's a bunch of things just
to get to the point where we can start doing something like that.

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

https://codereview.chromium.org/698543005/diff/60001/mojo/services/window_man...
mojo/services/window_manager/basic_focus_rules.cc:25: return view->parent() ==
window_container_;
On 2014/11/10 20:45:30, sky wrote:
> Don't you want to check visibility/drawn in any of these? If you don't need to
> check here because it's done in FocusController, document in FocusRules that
> subclasses don't need to check it.

You do, but right now this is a straight port of the focus rules as they exist
in mojo right now. I've documented that this needs to have a real implementation
in a followup patch.

https://codereview.chromium.org/698543005/diff/60001/mojo/services/window_man...
File mojo/services/window_manager/focus_controller.cc (right):

https://codereview.chromium.org/698543005/diff/60001/mojo/services/window_man...
mojo/services/window_manager/focus_controller.cc:17:
FocusController::FocusController(FocusRules* rules)
On 2014/11/10 20:45:30, sky wrote:
> Who owns FocusRules? Seems like FocusController should and this should take a
> scoped_ptr.

Threaded scoped_ptrs<> through the interface.

https://codereview.chromium.org/698543005/diff/60001/mojo/services/window_man...
mojo/services/window_manager/focus_controller.cc:67: 
On 2014/11/10 20:45:30, sky wrote:
> I'm not clear on if visibility/drawn state should be checked here. Please
> document why we should or shouldn't check it in this class.

Visibility should be checked in CanActivateView(); its just that we currently
don't check visibility anywhere inside the window manager code. I added the todo
comment in the focus rules class.

[sky, 2014-11-10 23:29:16]

LGTM

[Elliot Glaysher, 2014-11-10 23:42:40]

Committed patchset #6 (id:100001) manually as
bb29e4293b6e60ee721b5be17f9fa9f4989a1981 (presubmit successful).