ViewRoot class for event forwarding on Android

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 6c55a7bf9df724ce0eea47170c138cd146ba2d85..78714b25da60364895ab8a66a42f05ac34cbbccf 100644
--- a/ui/android/view_android.cc
+++ b/ui/android/view_android.cc
@@ -18,6 +18,7 @@ namespace ui {
 using base::android::JavaRef;
 using base::android::ScopedJavaLocalRef;
 
+// ViewAndroid::ScopedAndroidView
 ViewAndroid::ScopedAnchorView::ScopedAnchorView(
     JNIEnv* env,
     const JavaRef<jobject>& jview,
@@ -68,34 +69,49 @@ ViewAndroid::ScopedAnchorView::view() const {
   return view_.get(env);
 }
 
-ViewAndroid::ViewAndroid(const JavaRef<jobject>& delegate)
-    : parent_(nullptr)
-    , delegate_(base::android::AttachCurrentThread(),
-                delegate.obj()) {}
+// ViewAndroid::Bounds
+void ViewAndroid::Bounds::SetBounds(const gfx::Point& origin,
+                                    int width,
+                                    int height) {
+  rect_.SetRect(origin.x(), origin.y(), width, height);
+}
+
+bool ViewAndroid::Bounds::IsInBounds(const MotionEventData& event) {
+  bool width_matched = (rect_.width() == Bounds::kMatchParent) ?

[boliu, 2017/01/24 23:47:12]

this only holds during the root to child recursion, it doesn't hold if I get a
random node and just call IsInBounds

there is only one caller, maybe should just move the logic to callsite

[Jinsuk Kim, 2017/01/25 02:34:33]

Removed the method and check it directly in the callsite (OnTouchEventInternal)

+      true : rect_.x() <= event.GetX() && event.GetX() < rect_.right();
+  bool height_matched = (rect_.height() == Bounds::kMatchParent) ?
+      true : rect_.y() <=  event.GetY() && event.GetY() < rect_.bottom();
+  return width_matched && height_matched;
+}
 
-ViewAndroid::ViewAndroid() : parent_(nullptr) {}
+// ViewAndroid
+ViewAndroid::ViewAndroid(ViewClient* client) : parent_(nullptr),
+                                               client_(client) {}
+ViewAndroid::ViewAndroid() : ViewAndroid(nullptr) {}
 
 ViewAndroid::~ViewAndroid() {
   RemoveFromParent();
 
-  for (std::list<ViewAndroid*>::iterator it = children_.begin();
-       it != children_.end(); it++) {
-    DCHECK_EQ((*it)->parent_, this);
-    (*it)->parent_ = nullptr;
+  for (auto& child : children_) {
+    DCHECK_EQ(child->parent_, this);
+    child->parent_ = nullptr;
   }
 }
 
 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);
 }
 
 void ViewAndroid::AddChild(ViewAndroid* child) {
   DCHECK(child);
+  DCHECK(!child->IsViewRoot());  // ViewRoot cannot be a child.
   DCHECK(std::find(children_.begin(), children_.end(), child) ==
          children_.end());
 
-  children_.push_back(child);
+  children_.push_front(child);

[boliu, 2017/01/24 23:47:12]

why push front?

[Jinsuk Kim, 2017/01/25 02:34:33]

This is for newly added view to go to front among the siblings. I put a comment
at the definition of |children_| in the header. Added additional comment here to
make it more explicit/clear.

   if (child->parent_)
     child->RemoveFromParent();
   child->parent_ = this;
@@ -106,6 +122,14 @@ void ViewAndroid::RemoveFromParent() {
     parent_->RemoveChild(this);
 }
 
+void ViewAndroid::SetBounds(const gfx::Point& origin, int width, int height) {
+  bounds_.SetBounds(origin, width, height);
+}
+
+bool ViewAndroid::IsInBounds(const MotionEventData& event) {
+  return bounds_.IsInBounds(event);
+}
+
 ViewAndroid::ScopedAnchorView ViewAndroid::AcquireAnchorView() {
   ScopedJavaLocalRef<jobject> delegate(GetViewAndroidDelegate());
   if (delegate.is_null())
@@ -116,15 +140,19 @@ ViewAndroid::ScopedAnchorView ViewAndroid::AcquireAnchorView() {
       env, Java_ViewAndroidDelegate_acquireView(env, delegate), delegate);
 }
 
+float ViewAndroid::GetDipScale() {
+  return display::Screen::GetScreen()
+      ->GetDisplayNearestWindow(this)
+      .device_scale_factor();
+}
+
 void ViewAndroid::SetAnchorRect(const JavaRef<jobject>& anchor,
                                 const gfx::RectF& bounds) {
   ScopedJavaLocalRef<jobject> delegate(GetViewAndroidDelegate());
   if (delegate.is_null())
     return;
 
-  float scale = display::Screen::GetScreen()
-      ->GetDisplayNearestWindow(this)
-      .device_scale_factor();
+  float scale = GetDipScale();
   int left_margin = std::round(bounds.x() * scale);
   int top_margin = std::round((content_offset().y() + bounds.y()) * scale);
   JNIEnv* env = base::android::AttachCurrentThread();
@@ -175,6 +203,14 @@ void ViewAndroid::SetLayer(scoped_refptr<cc::Layer> layer) {
   layer_ = layer;
 }
 
+ViewAndroid* ViewAndroid::GetViewRoot() {
+  return parent_ ? parent_->GetViewRoot() : nullptr;
+}
+
+bool ViewAndroid::IsViewRoot() {
+  return GetViewRoot() == this;
+}
+
 bool ViewAndroid::StartDragAndDrop(const JavaRef<jstring>& jtext,
                                    const JavaRef<jobject>& jimage) {
   ScopedJavaLocalRef<jobject> delegate(GetViewAndroidDelegate());
@@ -185,4 +221,25 @@ bool ViewAndroid::StartDragAndDrop(const JavaRef<jstring>& jtext,
                                                    jimage);
 }
 
+bool ViewAndroid::OnTouchEventInternal(const MotionEventData& event) {
+  if (!IsInBounds(event))
+    return false;
+
+  if (!children_.empty()) {
+    gfx::Point delta = bounds_.origin();

[boliu, 2017/01/24 23:47:12]

this is not wrong, but just a bit odd while reading this

the invariant right now is when calling view_root->OnTouchEventInternal, the
coordinate of the event is in the coordiante space of *parent* of view_root

why not have it be the child's space? ie a child will never see the event at all
if it's outside of its bounds

hmm.. perhaps the bounds of the child should be stored in the parent, or maybe
just the coordinate? hmm...

[Jinsuk Kim, 2017/01/25 02:34:33]

Would you elaborate? I *think* the event is translated to child's coord by
offsetting by parent's origin.... 

Would it help if I do the layout checking before getting into children's
OnTouchEventInternal? I think it will come to the same effect of out-of-bounds
child views not seeing the event at all. 

FYI my intention is that view_root always matches its parent's layout, so it
won't be in the way at the beginning of hit testing. And will be matched last
after all the children are tested.  

+    const MotionEventData& e =

[boliu, 2017/01/24 23:47:12]

I'm blanking on whether this is safe c++..

if IsOrigin is false, you are retaining a reference to a temporary variable, in
which case the variable just goes away immediately after the statement?

[Jinsuk Kim, 2017/01/25 02:34:33]

Const guarantees its lifetime:
https://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-...

+      delta.IsOrigin() ? event : event.Offset(-delta.x(), -delta.y());
+
+    for (auto& child: children_) {
+      if (child->OnTouchEventInternal(e))
+        return true;
+    }
+  }
+
+  if (client_ && client_->OnTouchEvent(event))

[boliu, 2017/01/24 23:47:12]

hmm, should this happen before looping over children?

feels like we need a design doc or something about all these decisions we are
just making implicitly here, also maybe should look at what aura does?

[Jinsuk Kim, 2017/01/25 02:34:33]

To the best of my knowledge, child views receives events first(unless their
parent intercepts it), and the parent receives it afterwards if there's no match
in Android. 

Let me check how Aura does it in general.

+    return true;
+
+  return false;
+}
+
 }  // namespace ui