Get event targetting working for mouse events.

Get event targetting working for mouse events.

In 28b50b9614, I committed a patch which broke input handling as I
removed window_manager's aura dependency. This has been fixed for mouse
events, and the wm_flow demo now switches windows on clicks.

Key input is still broken here; that will be handled in another
followup.

BUG=431047
R=sky@chromium.org

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

[Elliot Glaysher, 2014-11-13 23:28:23]

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

[Elliot Glaysher, 2014-11-13 23:28:23]

This implements enough event targeting to get the wm_flow demo working
correctly.

[sky, 2014-11-14 01:43:57]

https://codereview.chromium.org/724973003/diff/20001/mojo/services/window_man...
File mojo/services/window_manager/view_target.cc (right):

https://codereview.chromium.org/724973003/diff/20001/mojo/services/window_man...
mojo/services/window_manager/view_target.cc:21: static
base::LazyInstance<std::map<View*, ViewTarget*>> g_target_map =
We may use the client lib on multiple threads. You're going to need to be
careful with access to this.

https://codereview.chromium.org/724973003/diff/20001/mojo/services/window_man...
mojo/services/window_manager/view_target.cc:99:
translation.Translate(static_cast<float>(bounds.x()),
Bounds are all ints and we don't support arbitrary transforms yet. Seems like
all you care about here is a Vector2d offset.

https://codereview.chromium.org/724973003/diff/20001/mojo/services/window_man...
File mojo/services/window_manager/view_targeter_unittest.cc (right):

https://codereview.chromium.org/724973003/diff/20001/mojo/services/window_man...
mojo/services/window_manager/view_targeter_unittest.cc:40: TestViewTarget* root
=
Why not declare TestViewTArget, and one/two on the stack?

https://codereview.chromium.org/724973003/diff/20001/mojo/services/window_man...
File mojo/services/window_manager/window_manager_test_util.h (right):

https://codereview.chromium.org/724973003/diff/20001/mojo/services/window_man...
mojo/services/window_manager/window_manager_test_util.h:52: class ViewCollector
{
It's not clear to me why TestView and TestViewTarget need ViewCollector? If
someone wants to use viewcollector can't they use it without making TestView and
TestViewTarget depend upon it?

SHould TestViewTarget own the view it creates?

https://codereview.chromium.org/724973003/diff/20001/ui/events/event_target_i...
File ui/events/event_target_iterator.h (right):

https://codereview.chromium.org/724973003/diff/20001/ui/events/event_target_i...
ui/events/event_target_iterator.h:56: virtual ~CopyingEventTargetIteratorImpl()
{}
nit: no virtual, just override.

https://codereview.chromium.org/724973003/diff/20001/ui/events/event_target_i...
ui/events/event_target_iterator.h:58: virtual EventTarget* GetNextTarget()
override {
same thing, no virtual, just override.

https://codereview.chromium.org/724973003/diff/20001/ui/events/event_target_i...
ui/events/event_target_iterator.h:60: return NULL;
nullptr.

[Elliot Glaysher, 2014-11-14 21:04:30]

https://codereview.chromium.org/724973003/diff/20001/mojo/services/window_man...
File mojo/services/window_manager/view_target.cc (right):

https://codereview.chromium.org/724973003/diff/20001/mojo/services/window_man...
mojo/services/window_manager/view_target.cc:21: static
base::LazyInstance<std::map<View*, ViewTarget*>> g_target_map =
On 2014/11/14 01:43:57, sky wrote:
> We may use the client lib on multiple threads. You're going to need to be
> careful with access to this.

Moved everything regarding the mapping to a ported version of aura's
WindowProperties.

https://codereview.chromium.org/724973003/diff/20001/mojo/services/window_man...
mojo/services/window_manager/view_target.cc:99:
translation.Translate(static_cast<float>(bounds.x()),
On 2014/11/14 01:43:57, sky wrote:
> Bounds are all ints and we don't support arbitrary transforms yet. Seems like
> all you care about here is a Vector2d offset.

Redid all of this operating on Vector2ds.

https://codereview.chromium.org/724973003/diff/20001/mojo/services/window_man...
File mojo/services/window_manager/window_manager_test_util.h (right):

https://codereview.chromium.org/724973003/diff/20001/mojo/services/window_man...
mojo/services/window_manager/window_manager_test_util.h:52: class ViewCollector
{
On 2014/11/14 01:43:57, sky wrote:
> It's not clear to me why TestView and TestViewTarget need ViewCollector? If
> someone wants to use viewcollector can't they use it without making TestView
and
> TestViewTarget depend upon it?
> 
> SHould TestViewTarget own the view it creates?

This class is gone due to the rejiggering of the ownership semantics.

[sky, 2014-11-14 23:11:27]

I only looked at the view changes. In fact how about separating those out into
their own patch and adding test coverage?

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

https://codereview.chromium.org/724973003/diff/40001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/view.cc:391: for (std::map<const
void*, Value>::const_iterator iter = prop_map_.begin();
enhanced for loop? auto?

https://codereview.chromium.org/724973003/diff/40001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/lib/view.cc:428: //
FOR_EACH_OBSERVER(WindowObserver, observers_,
Seems reasonable to notify observers when the property changes.

https://codereview.chromium.org/724973003/diff/40001/mojo/services/public/cpp...
File mojo/services/public/cpp/view_manager/view.h (right):

https://codereview.chromium.org/724973003/diff/40001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/view.h:57: // shared with all other
clients of the view manager.
Actually, it's only the window manager that can see these now. How about:
"These properties are shared the view manager."

https://codereview.chromium.org/724973003/diff/40001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/view.h:68: // These properties are only
visible in the current process and are not
nit: 'current process' is kind of misleading here. I would nuke that part and
just 'These properties are not visible to the view manager.'

https://codereview.chromium.org/724973003/diff/40001/mojo/services/public/cpp...
File mojo/services/public/cpp/view_manager/view_property.h (right):

https://codereview.chromium.org/724973003/diff/40001/mojo/services/public/cpp...
mojo/services/public/cpp/view_manager/view_property.h:106: #define
DECLARE_EXPORTED_VIEW_PROPERTY_TYPE(EXPORT, T)  \
My eyes have glazed over from this code in the past.

Is there anything we have to worry about with being in a shared library now?
Weirdness with deallocators say?

[Elliot Glaysher, 2014-11-17 21:55:40]

I've now rebased on top of https://codereview.chromium.org/732223002 .

[sky, 2014-11-17 22:39:28]

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

https://codereview.chromium.org/724973003/diff/60001/mojo/services/window_man...
mojo/services/window_manager/view_target.cc:29:
view_->SetLocalProperty(kViewTargetKey, this);
DCHECK GetLocalProperty(kViewTargetKey, this) == null)?

https://codereview.chromium.org/724973003/diff/60001/mojo/services/window_man...
mojo/services/window_manager/view_target.cc:34: ViewTarget*
ViewTarget::TargetFromView(View* view) {
Keep definitions & declarations in order.

https://codereview.chromium.org/724973003/diff/60001/mojo/services/window_man...
mojo/services/window_manager/view_target.cc:79: const ViewTarget* root =
nullptr;
Should this be set to this first?

https://codereview.chromium.org/724973003/diff/60001/mojo/services/window_man...
mojo/services/window_manager/view_target.cc:133: view->RemoveObserver(this);
Is it necessary to add an observer anymore?

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

https://codereview.chromium.org/724973003/diff/60001/mojo/services/window_man...
mojo/services/window_manager/view_target.h:31: // Instead of maintaining a
shadow tree, we keep a static mapping between a
Update description to reflect how you now store mapping.

https://codereview.chromium.org/724973003/diff/60001/mojo/services/window_man...
mojo/services/window_manager/view_target.h:37: ViewTarget(View* view_to_wrap);
explicit

https://codereview.chromium.org/724973003/diff/60001/mojo/services/window_man...
mojo/services/window_manager/view_target.h:41: static ViewTarget*
TargetFromView(View* view);
Did you consider making the constructor private and having this create
ViewTarget if one hasn't been created yet?

https://codereview.chromium.org/724973003/diff/60001/mojo/services/window_man...
mojo/services/window_manager/view_target.h:52: std::vector<ViewTarget*>
GetChildren() const;
Should return vector of const ViewTarget (or not be const).

https://codereview.chromium.org/724973003/diff/60001/mojo/services/window_man...
mojo/services/window_manager/view_target.h:53: ViewTarget* GetParent() const;
const methods should return const pointers, otherwise you can trivially
circumvent const'ness.

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

https://codereview.chromium.org/724973003/diff/60001/mojo/services/window_man...
mojo/services/window_manager/view_targeter.h:34: // Deals with cases where a the
|root_view| needs to change how things are
You've got an extra 'a' here.

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

https://codereview.chromium.org/724973003/diff/60001/mojo/services/window_man...
mojo/services/window_manager/window_manager_app.cc:276: ViewTarget* target = new
ViewTarget(view);
Document how ViewTarget is deleted.

https://codereview.chromium.org/724973003/diff/60001/ui/events/event_target_i...
File ui/events/event_target_iterator.h (right):

https://codereview.chromium.org/724973003/diff/60001/ui/events/event_target_i...
ui/events/event_target_iterator.h:60: return NULL;
nullptr.

[Elliot Glaysher, 2014-11-17 23:33:02]

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

https://codereview.chromium.org/724973003/diff/60001/mojo/services/window_man...
mojo/services/window_manager/view_target.cc:79: const ViewTarget* root =
nullptr;
On 2014/11/17 22:39:27, sky wrote:
> Should this be set to this first?

Rewrote this to not use a parent temporary.

(I think this explicit version reads better, but whatever.)

https://codereview.chromium.org/724973003/diff/60001/mojo/services/window_man...
mojo/services/window_manager/view_target.cc:133: view->RemoveObserver(this);
On 2014/11/17 22:39:27, sky wrote:
> Is it necessary to add an observer anymore?

It isn't.

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

https://codereview.chromium.org/724973003/diff/60001/mojo/services/window_man...
mojo/services/window_manager/view_target.h:41: static ViewTarget*
TargetFromView(View* view);
On 2014/11/17 22:39:27, sky wrote:
> Did you consider making the constructor private and having this create
> ViewTarget if one hasn't been created yet?

Done. This also changes WindowManagerApp so that it no longer maintains a
mapping of view ids to ViewTargets. (And just replaces it with a set to keep
track of what Views were registered in our system, which is what we were using
it for in practice.)

https://codereview.chromium.org/724973003/diff/60001/mojo/services/window_man...
mojo/services/window_manager/view_target.h:52: std::vector<ViewTarget*>
GetChildren() const;
On 2014/11/17 22:39:27, sky wrote:
> Should return vector of const ViewTarget (or not be const).

Removed const, added interface to re-add const here (and in several other places
that I had to remove to make this compile) once we remove aura from the tree.
(The EventTarget interface doesn't play well with const.)

[sky, 2014-11-18 00:29:58]

LGTM

[Elliot Glaysher, 2014-11-18 00:45:09]

Committed patchset #5 (id:80001) manually as
c89cc3b5229ffebab3050389d6be7b234097cafa (presubmit successful).