Add EventForwarder for routing touch events

ui/android/view_android.cc

Index: ui/android/view_android.cc
diff --git a/ui/android/view_android.cc b/ui/android/view_android.cc
index 1d5b146653458a83b93c58810f743f6df78e5c46..2f15751db6aba04bccf24ba576736ebac451c0b6 100644
--- a/ui/android/view_android.cc
+++ b/ui/android/view_android.cc
@@ -10,9 +10,10 @@
 #include "base/android/jni_string.h"
 #include "cc/layers/layer.h"
 #include "jni/ViewAndroidDelegate_jni.h"
+#include "ui/android/event_handler.h"
+#include "ui/android/view_client.h"
 #include "ui/android/window_android.h"
-#include "ui/display/display.h"
-#include "ui/display/screen.h"
+#include "ui/base/layout.h"
 #include "url/gurl.h"
 
 namespace ui {
@@ -71,11 +72,10 @@ ViewAndroid::ScopedAnchorView::view() const {
   return view_.get(env);
 }
 
-ViewAndroid::ViewAndroid(const JavaRef<jobject>& delegate)
-    : parent_(nullptr),
-      delegate_(base::android::AttachCurrentThread(), delegate.obj()) {}
+ViewAndroid::ViewAndroid(ViewClient* view_client)
+    : parent_(nullptr), client_(view_client) {}
 
-ViewAndroid::ViewAndroid() : parent_(nullptr) {}
+ViewAndroid::ViewAndroid() : ViewAndroid(nullptr) {}
 
 ViewAndroid::~ViewAndroid() {
   RemoveFromParent();
@@ -88,21 +88,66 @@ ViewAndroid::~ViewAndroid() {
 }
 
 void ViewAndroid::SetDelegate(const JavaRef<jobject>& delegate) {
+  // A ViewAndroid may have its own delegate or otherwise will use the next
+  // available parent's delegate.
   JNIEnv* env = base::android::AttachCurrentThread();
   delegate_ = JavaObjectWeakGlobalRef(env, delegate);
 }
 
+float ViewAndroid::GetDipScale() {
+  return ui::GetScaleFactorForNativeView(this);
+}
+
+EventHandler* ViewAndroid::CreateEventHandler() {
+  DCHECK(!ViewTreeHasEventHandler(GetViewRoot()))

[Khushal, 2017/03/02 21:46:15]

That feels like a lot of unnecessary complexity just to ensure that we don't
create multiple java bridges in the tree.

[boliu, 2017/03/02 22:02:16]

I asked for these checks. Imo it's a correctness guarantee that nothing can
receive events from two different sources.

[Khushal, 2017/03/02 22:08:59]

What 2 sources are we worried about? The event methods are public on VA, so what
exactly are we trying to restrict?

[boliu, 2017/03/02 22:17:41]

I want to make sure nothing can be receiving events from two different sources
(ie EventHandlers)

The original condition I wanted was there's at most one EventHandler along any
leaf to root path. The currently implemented thing is that only the immediate
children of WindowAndroid can have EventHandlers, which is more restrictive.

This is all bikeshedding a bit though..

The public methods are easy to fix though, make the private and friend
EventHandler.

[Khushal, 2017/03/02 22:34:34]

Is there a good reason for why we need to have a constraint like this though?
With android views, anyone can inject an event into a view. We use it to do
things like parents stealing events from their children mid-gesture and send
synthetic events to the child.

[boliu, 2017/03/02 22:41:50]

Can keep doing that *within* ViewAndroids/ViewClients. And we can build in
explicit support if current thing is not good enough.

I don't think that's equivalent to having multiple event sources into
ViewAndroid tree. And imo it's a lot easier to make a mistake of creating a
source on the wrong level.

[Khushal, 2017/03/02 23:11:36]

So here is where my understanding of source. We are basically arguing whether
there should be a constraint that only the root of the VA tree can get the event
and should trickle it down to the descendants or should anyone be able to inject
an event into any View right? Because I don't think merely having an
EventHandler hook that lets you plumb events from java should be seen as a
different source.

And I don't think there is a huge potential for error with someone using a
public event API on a View incorrectly. The android View has one, and I don't
think that has been a problem so far...

[boliu, 2017/03/02 23:16:06]

Totally disagree with that assessment. Having any view inject its own event
stream has caused loads of integration/refactor problem or just plain odd
behavior, from experience working on webview

[Khushal, 2017/03/03 00:37:55]

Okay, if you know this has caused pain in the past. :)  I tried taking a look at
aura to see what happens there, and it seems like it is structured around this
idea too. So final goal would be to have only ViewRoot have a public API for
getting events, and VA get them only from their parent. For things we do in UI,
we can find a way to structure them in the VA framework then. ui/ folks will be
able to comment on it better too.

It would be nice to not have these methods be public on VA then. Friend-ing
EventHandler sounds better.

[Jinsuk Kim, 2017/03/03 06:29:21]

Hid native EventHandler instance (owned by ViewAndroid), and let VA be a friend
of EventHandler so only VA can get java EventHandler object.

+      << "Root of the ViewAndroid can have at most one handler.";
+  event_handler_.reset(EventHandler::Create(this));
+  return event_handler_.get();
+}
+
+ViewAndroid* ViewAndroid::GetViewRoot() {

[boliu, 2017/03/02 20:58:14]

So.. viewroot is the immediate children of windowandroid?

I'm not sure we really want to enforce that yet. I imagine chrome refactor will
probably want to separate changes to structure of the tree vs when event
forwarding is updated.

I think we should just not have a viewroot concept right now (root is the
windowandroid), and go from there.

[Jinsuk Kim, 2017/03/03 06:29:21]

Removed GetViewRoot(). It was used for |ViewTreeHasEventHandler| only - rewrote
not to use it.

+  DCHECK(parent_) << "View is not attached to a tree yet.";

[Khushal, 2017/03/02 21:46:15]

nit: This doesn't really need to be a DCHECK. Just return a null root if
detached?

[Jinsuk Kim, 2017/03/03 06:29:22]

Removed this method.

+
+  // The root of VA tree is that of WCVA for now.
+  return parent_ == GetWindowAndroid() ? this : nullptr;
+}
+
 void ViewAndroid::AddChild(ViewAndroid* child) {
   DCHECK(child);
   DCHECK(std::find(children_.begin(), children_.end(), child) ==
          children_.end());
+  DCHECK(!ViewTreeHasEventHandler(child) ||
+         !ViewTreeHasEventHandler(GetViewRoot()))
+      << "Only one event handler is allowed.";
 
+  // The new child goes to the top, which is the end of the list.
   children_.push_back(child);
   if (child->parent_)
     child->RemoveFromParent();
   child->parent_ = this;
 }
 
+// static
+bool ViewAndroid::ViewTreeHasEventHandler(ViewAndroid* view) {
+  if (view->has_event_handler())
+    return true;
+  for (auto& child : view->children_) {
+    if (child->has_event_handler())
+      return true;
+  }
+  return false;
+}
+
+void ViewAndroid::MoveToTop(ViewAndroid* child) {

[David Trainor- moved to gerrit, 2017/03/03 23:04:34]

MoveToFront feels more clear

[Jinsuk Kim, 2017/03/06 04:07:33]

Done.

+  DCHECK(child);
+  auto it = std::find(children_.begin(), children_.end(), child);
+  DCHECK(it != children_.end());
+
+  // Top element is placed at the end of the list.
+  if (*it != children_.back())
+    children_.splice(children_.rbegin().base(), children_, it);

[Khushal, 2017/03/02 21:46:15]

nit: children_.end() instead of children_.rbegin().base()?

[Jinsuk Kim, 2017/03/03 06:29:22]

Done.

+}
+
 void ViewAndroid::RemoveFromParent() {
   if (parent_)
     parent_->RemoveChild(this);
@@ -124,15 +169,13 @@ void ViewAndroid::SetAnchorRect(const JavaRef<jobject>& anchor,
   if (delegate.is_null())
     return;
 
-  float scale = display::Screen::GetScreen()
-                    ->GetDisplayNearestWindow(this)
-                    .device_scale_factor();
-  int left_margin = std::round(bounds.x() * scale);
-  int top_margin = std::round((content_offset().y() + bounds.y()) * scale);
+  float dip_scale = GetDipScale();
+  int left_margin = std::round(bounds.x() * dip_scale);
+  int top_margin = std::round((content_offset().y() + bounds.y()) * dip_scale);
   JNIEnv* env = base::android::AttachCurrentThread();
   Java_ViewAndroidDelegate_setViewPosition(
       env, delegate, anchor, bounds.x(), bounds.y(), bounds.width(),
-      bounds.height(), scale, left_margin, top_margin);
+      bounds.height(), dip_scale, left_margin, top_margin);
 }
 
 ScopedJavaLocalRef<jobject> ViewAndroid::GetContainerView() {
@@ -177,6 +220,18 @@ void ViewAndroid::SetLayer(scoped_refptr<cc::Layer> layer) {
   layer_ = layer;
 }
 
+void ViewAndroid::SetLayout(int x,

[Khushal, 2017/03/02 21:46:15]

Do you want to split this into 2 methods: SetBounds and SetMatchParent?

[Jinsuk Kim, 2017/03/03 06:29:22]

I haven't found a case that needs the splitting yet. I guess it would be okay to
do that on the first time I run into it.

[boliu, 2017/03/03 19:46:18]

Actually, should not do split this. This is our version of setLayoutParams,
everything should be set in one call. We can always create a struct or something
if parameter list gets too long, but this is fine for now..

[David Trainor- moved to gerrit, 2017/03/03 23:04:34]

The behavior of this feels a bit weird to me.  match_parent just overrides the
rest of the parameters.  Referring to LayoutParams, the width and height use
special constants for the match_parent effect, and x and y aren't part of that
call at all.  So it's impossible to create a call that disregards half the
parameters of the struct.

[boliu, 2017/03/03 23:10:27]

We certainly do the same thing. Make a struct with nice factory methods.

[Khushal, 2017/03/03 23:36:06]

It looked a bit awkward for me to see an API where you ask for 2 params which
are always supposed to be mutually exclusive.

[Jinsuk Kim, 2017/03/06 04:07:33]

Added a struct to make match_parent and other params exclusive. PTAL
(I'm afraid I've got too many a reviewer on this :/)

+                            int y,
+                            int width,
+                            int height,
+                            bool match_parent) {
+  DCHECK(!match_parent || (x == 0 && y == 0 && width == 0 && height == 0))
+      << "Should give empty dimension when view layout matches its parent.";

[boliu, 2017/03/02 20:58:14]

this can be better still. like if I trigger this DCHECK, then I have to add
*more* logging to figure out what if match_parent was true or false and etc..
just do that here

[Jinsuk Kim, 2017/03/03 06:29:21]

Logged all the parameters.

+  origin_.SetPoint(x, y);
+  size_.SetSize(width, height);
+  match_parent_ = match_parent;
+}
+
 bool ViewAndroid::StartDragAndDrop(const JavaRef<jstring>& jtext,
                                    const JavaRef<jobject>& jimage) {
   ScopedJavaLocalRef<jobject> delegate(GetViewAndroidDelegate());
@@ -227,4 +282,48 @@ void ViewAndroid::StartContentIntent(const GURL& content_url,
                                                 is_main_frame);
 }
 
+bool ViewAndroid::OnTouchEvent(const MotionEventData& event) {
+  if (!children_.empty()) {
+    const MotionEventData& e =
+        origin_.IsOrigin() ? event : event.Offset(-origin_.x(), -origin_.y());
+
+    for (auto it = children_.rbegin(); it != children_.rend(); ++it) {

[Khushal, 2017/03/02 21:46:15]

nit: for (auto* child : children_)?

[boliu, 2017/03/02 22:02:16]

back to forward order is desired here. we modeled this on cc::Layer, which is
painted in forward order, which means for hit testing should be in the reverse
order

probably worth a comment though

[Khushal, 2017/03/02 22:08:59]

Oh Duh, didn't notice that. Yeah, a comment for that would be great. Would the
following read cleaner:
for (auto* child : base::Reversed(children_))?

[Jinsuk Kim, 2017/03/03 06:29:22]

Thanks for the suggestion. Reversed looks nicer. Done.

+      bool matched = (*it)->match_parent_;
+      if (!matched) {
+        gfx::Rect bound((*it)->origin_, (*it)->size_);
+        matched = bound.Contains(e.GetX(), e.GetY());

[boliu, 2017/03/02 20:58:14]

can we abstract out hit testing into its own method, rather than duplicate the
logic in two places? probably would make it easier to test as well?

[Jinsuk Kim, 2017/03/03 06:29:22]

Good idea. Done.

+      }
+      if (matched && (*it)->OnTouchEvent(e))
+        return true;
+    }
+  }
+
+  if (client_ && client_->OnTouchEvent(event))

[Khushal, 2017/03/02 21:46:15]

nit: return client_ ? client_->OnTouchEvent(event) : false;

[Jinsuk Kim, 2017/03/03 06:29:22]

Done.

+    return true;
+
+  return false;
+}
+
+bool ViewAndroid::OnMouseEvent(const MotionEventData& event) {
+  if (!children_.empty()) {
+    const MotionEventData& e =
+        origin_.IsOrigin() ? event : event.Offset(-origin_.x(), -origin_.y());
+
+    for (auto it = children_.rbegin(); it != children_.rend(); ++it) {
+      bool matched = (*it)->match_parent_;
+      if (!matched) {
+        gfx::Rect bound((*it)->origin_, (*it)->size_);
+        matched = bound.Contains(e.GetX(), e.GetY());
+      }
+      if (matched && (*it)->OnMouseEvent(e))
+        return true;
+    }
+  }
+
+  if (client_ && client_->OnMouseEvent(event))
+    return true;
+
+  return false;
+}
+
 }  // namespace ui