Set ViewAndroid layout properly

In ViewAndroid tree, the top view for WebContents should be of
normal layout that can adjust its size to the OnSizeChanged
event, while the bottom one for RenderWidgetHostViewAndroid
should be of type match-parent to keep its size always in sync with
its parent. This CL takes care of that.

BUG=622847

Change-Id: Ie928a68334fc8c7342643081d85cdb88da075bb4
Reviewed-on: https://chromium-review.googlesource.com/654397
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501474}

content/browser/renderer_host/render_widget_host_view_android.cc

@@ -460,7 +460,7 @@ RenderWidgetHostViewAndroid::RenderWidgetHostViewAndroid(
       text_suggestion_host_(nullptr),
       background_color_(SK_ColorWHITE),
       cached_background_color_(SK_ColorWHITE),
-      view_(this),
+      view_(this, ui::ViewAndroid::LayoutType::MATCH_PARENT),
       gesture_provider_(ui::GetGestureProviderConfig(
                             ui::GestureProviderConfigType::CURRENT_PLATFORM),
                         this),

content/browser/web_contents/web_contents_view_android.cc

@@ -89,7 +89,7 @@ WebContentsViewAndroid::WebContentsViewAndroid(
     : web_contents_(web_contents),
       content_view_core_(NULL),
       delegate_(delegate),
-      view_(this),
+      view_(this, ui::ViewAndroid::LayoutType::NORMAL),
       synchronous_compositor_client_(nullptr) {}
 
 WebContentsViewAndroid::~WebContentsViewAndroid() {

ui/android/view_android.cc

@@ -80,12 +80,10 @@ ViewAndroid::ScopedAnchorView::view() const {
   return view_.get(env);
 }
 
-ViewAndroid::ViewAndroid(ViewClient* view_client)
-    : parent_(nullptr),
-      client_(view_client),
-      layout_params_(LayoutParams::MatchParent()) {}
+ViewAndroid::ViewAndroid(ViewClient* view_client, LayoutType layout_type)
+    : parent_(nullptr), client_(view_client), layout_type_(layout_type) {}
 
-ViewAndroid::ViewAndroid() : ViewAndroid(nullptr) {}
+ViewAndroid::ViewAndroid() : ViewAndroid(nullptr, LayoutType::NORMAL) {}
 
 ViewAndroid::~ViewAndroid() {
   observer_list_.Clear();
@@ -144,9 +142,8 @@ void ViewAndroid::AddChild(ViewAndroid* child) {
 
   // Empty view size also need not propagating down in order to prevent
   // spurious events with empty size from being sent down.
-  if (child->layout_params_.match_parent && layout_params_.width != 0 &&
-      layout_params_.height != 0) {
-    child->OnSizeChangedInternal(layout_params_.width, layout_params_.height);
+  if (child->match_parent() && !view_rect_.IsEmpty()) {
+    child->OnSizeChangedInternal(view_rect_.size());
     DispatchOnSizeChanged();
   }
 
@@ -383,31 +380,32 @@ int ViewAndroid::GetSystemWindowInsetBottom() {
 }
 
 void ViewAndroid::OnSizeChanged(int width, int height) {
-  // TODO(jinsukkim): Assert match-parent here. Match-parent view should keep
-  // its size in sync with its parent, ignoring the incoming size change event.
+  // Match-parent view must not receive size events.
+  DCHECK(!match_parent());
+
   float scale = GetDipScale();
-  OnSizeChangedInternal(std::ceil(width / scale), std::ceil(height / scale));
+  OnSizeChangedInternal(
+      gfx::Size(std::ceil(width / scale), std::ceil(height / scale)));
 
   // Signal resize event after all the views in the tree get the updated size.
   DispatchOnSizeChanged();
 }
 
-void ViewAndroid::OnSizeChangedInternal(int width, int height) {
-  if (layout_params_.width == width && layout_params_.height == height)
+void ViewAndroid::OnSizeChangedInternal(const gfx::Size& size) {
+  if (view_rect_.size() == size)
     return;
 
-  layout_params_.width = width;
-  layout_params_.height = height;
+  view_rect_.set_size(size);
   for (auto* child : children_) {
-    if (child->layout_params_.match_parent)
-      child->OnSizeChangedInternal(width, height);
+    if (child->match_parent())
+      child->OnSizeChangedInternal(size);
   }
 }
 
 void ViewAndroid::DispatchOnSizeChanged() {
   client_->OnSizeChanged();
   for (auto* child : children_) {
-    if (child->layout_params_.match_parent)
+    if (child->match_parent())
       child->DispatchOnSizeChanged();
   }
 }
@@ -487,18 +485,14 @@ bool ViewAndroid::HitTest(ViewClientCallback<E> send_to_client,
 
   if (!children_.empty()) {
     gfx::PointF offset_point(point);
-    offset_point.Offset(-layout_params_.x, -layout_params_.y);
+    offset_point.Offset(-view_rect_.x(), -view_rect_.y());
     gfx::Point int_point = gfx::ToFlooredPoint(offset_point);
 
     // Match from back to front for hit testing.
     for (auto* child : base::Reversed(children_)) {
-      bool matched = child->layout_params_.match_parent;
-      if (!matched) {
-        gfx::Rect bound(child->layout_params_.x, child->layout_params_.y,
-                        child->layout_params_.width,
-                        child->layout_params_.height);
-        matched = bound.Contains(int_point);
-      }
+      bool matched = child->match_parent();
+      if (!matched)
+        matched = child->view_rect_.Contains(int_point);
       if (matched && child->HitTest(send_to_client, event, offset_point))
         return true;
     }
@@ -506,4 +500,8 @@ bool ViewAndroid::HitTest(ViewClientCallback<E> send_to_client,
   return false;
 }
 
+void ViewAndroid::SetLayoutForTesting(int x, int y, int width, int height) {
+  view_rect_.SetRect(x, y, width, height);
+}
+
 }  // namespace ui

ui/android/view_android.h

@@ -24,6 +24,7 @@ class Layer;
 
 namespace gfx {
 class Point;
+class Size;
 }
 
 namespace ui {
@@ -83,33 +84,14 @@ class UI_ANDROID_EXPORT ViewAndroid {
     // Default copy/assign disabled by move constructor.
   };
 
-  // Layout parameters used to set the view's position and size.
-  // Position is in parent's coordinate space, and all the values
-  // are in CSS pixel.
-  struct LayoutParams {
-    static LayoutParams MatchParent() { return {true, 0, 0, 0, 0}; }
-    static LayoutParams Normal(int x, int y, int width, int height) {
-      return {false, x, y, width, height};
-    };
-
-    bool match_parent;  // Bounds matches that of the parent if true.
-    int x;
-    int y;
-    int width;
-    int height;
-
-    LayoutParams(const LayoutParams& p) = default;
-
-   private:
-    LayoutParams(bool match_parent, int x, int y, int width, int height)
-        : match_parent(match_parent),
-          x(x),
-          y(y),
-          width(width),
-          height(height) {}
+  enum class LayoutType {
+    // Can have its own size given by |OnSizeChanged| events.
+    NORMAL,
+    // Always follows its parent's size.
+    MATCH_PARENT
   };
 
-  explicit ViewAndroid(ViewClient* view_client);
+  ViewAndroid(ViewClient* view_client, LayoutType layout_type);
 
   ViewAndroid();
   virtual ~ViewAndroid();
@@ -205,6 +187,8 @@ class UI_ANDROID_EXPORT ViewAndroid {
   void OnAttachedToWindow();
   void OnDetachedFromWindow();
 
+  void SetLayoutForTesting(int x, int y, int width, int height);
+
   template <typename E>
   using ViewClientCallback =
       const base::Callback<bool(ViewClient*, const E&, const gfx::PointF&)>;
@@ -229,6 +213,8 @@ class UI_ANDROID_EXPORT ViewAndroid {
 
   bool has_event_forwarder() const { return !!event_forwarder_; }
 
+  bool match_parent() const { return layout_type_ == LayoutType::MATCH_PARENT; }
+
   // Checks if there is any event forwarder in any node up to root.
   static bool RootPathHasEventForwarder(ViewAndroid* view);
 
@@ -236,7 +222,7 @@ class UI_ANDROID_EXPORT ViewAndroid {
   // each leaf of subtree.
   static bool SubtreeHasEventForwarder(ViewAndroid* view);
 
-  void OnSizeChangedInternal(int width, int height);
+  void OnSizeChangedInternal(const gfx::Size& size);
   void DispatchOnSizeChanged();
 
   // Returns the Java delegate for this view. This is used to delegate work
@@ -253,8 +239,9 @@ class UI_ANDROID_EXPORT ViewAndroid {
   ViewClient* const client_;
 
   // Basic view layout information. Used to do hit testing deciding whether
-  // the passed events should be processed by the view.
-  LayoutParams layout_params_;
+  // the passed events should be processed by the view. Unit in DIP.
+  gfx::Rect view_rect_;
+  const LayoutType layout_type_;
 
   // In physical pixel.
   gfx::Size physical_size_;

ui/android/view_android_unittests.cc

@@ -17,7 +17,8 @@ using base::android::JavaParamRef;
 
 class TestViewAndroid : public ViewAndroid {
  public:
-  explicit TestViewAndroid(ViewClient* client) : ViewAndroid(client) {}
+  TestViewAndroid(ViewClient* client, ViewAndroid::LayoutType layout_type)
+      : ViewAndroid(client, layout_type) {}
 
   float GetDipScale() override { return 1.f; }
 };
@@ -50,18 +51,19 @@ class TestViewClient : public ViewClient {
 class ViewAndroidBoundsTest : public testing::Test {
  public:
   ViewAndroidBoundsTest()
-      : root_(nullptr),
-        view1_(&client1_),
-        view2_(&client2_),
-        view3_(&client3_) {
+      : root_(nullptr, ViewAndroid::LayoutType::MATCH_PARENT),
+        view1_(&client1_, ViewAndroid::LayoutType::NORMAL),
+        view2_(&client2_, ViewAndroid::LayoutType::NORMAL),
+        view3_(&client3_, ViewAndroid::LayoutType::NORMAL),
+        viewm_(&clientm_, ViewAndroid::LayoutType::MATCH_PARENT) {
     root_.GetEventForwarder();
-    root_.layout_params_ = ViewAndroid::LayoutParams::MatchParent();
   }
 
   void Reset() {
     client1_.Reset();
     client2_.Reset();
     client3_.Reset();
+    clientm_.Reset();
   }
 
   void GenerateTouchEventAt(float x, float y) {
@@ -74,7 +76,7 @@ class ViewAndroidBoundsTest : public testing::Test {
   }
 
   void ExpectHit(const TestViewClient& hitClient) {
-    TestViewClient* clients[3] = {&client1_, &client2_, &client3_};
+    TestViewClient* clients[4] = {&client1_, &client2_, &client3_, &clientm_};
     for (auto* client : clients) {
       if (&hitClient == client)
         EXPECT_TRUE(client->TouchEventHandled());
@@ -88,15 +90,16 @@ class ViewAndroidBoundsTest : public testing::Test {
   TestViewAndroid view1_;
   TestViewAndroid view2_;
   TestViewAndroid view3_;
+  TestViewAndroid viewm_;  // match-parent view
   TestViewClient client1_;
   TestViewClient client2_;
   TestViewClient client3_;
+  TestViewClient clientm_;
 };
 
 TEST_F(ViewAndroidBoundsTest, MatchesViewInFront) {
-  view1_.layout_params_ = ViewAndroid::LayoutParams::Normal(50, 50, 400, 600);
-  view1_.layout_params_ = ViewAndroid::LayoutParams::Normal(50, 50, 400, 600);
-  view2_.layout_params_ = ViewAndroid::LayoutParams::Normal(50, 50, 400, 600);
+  view1_.SetLayoutForTesting(50, 50, 400, 600);
+  view2_.SetLayoutForTesting(50, 50, 400, 600);
   root_.AddChild(&view2_);
   root_.AddChild(&view1_);
 
@@ -110,8 +113,8 @@ TEST_F(ViewAndroidBoundsTest, MatchesViewInFront) {
 }
 
 TEST_F(ViewAndroidBoundsTest, MatchesViewArea) {
-  view1_.layout_params_ = ViewAndroid::LayoutParams::Normal(50, 50, 200, 200);
-  view2_.layout_params_ = ViewAndroid::LayoutParams::Normal(20, 20, 400, 600);
+  view1_.SetLayoutForTesting(50, 50, 200, 200);
+  view2_.SetLayoutForTesting(20, 20, 400, 600);
 
   root_.AddChild(&view2_);
   root_.AddChild(&view1_);
@@ -126,27 +129,26 @@ TEST_F(ViewAndroidBoundsTest, MatchesViewArea) {
 }
 
 TEST_F(ViewAndroidBoundsTest, MatchesViewAfterMove) {
-  view1_.layout_params_ = ViewAndroid::LayoutParams::Normal(50, 50, 200, 200);
-  view2_.layout_params_ = ViewAndroid::LayoutParams::Normal(20, 20, 400, 600);
+  view1_.SetLayoutForTesting(50, 50, 200, 200);
+  view2_.SetLayoutForTesting(20, 20, 400, 600);
   root_.AddChild(&view2_);
   root_.AddChild(&view1_);
 
   GenerateTouchEventAt(100.f, 100.f);
   ExpectHit(client1_);
 
-  view1_.layout_params_ = ViewAndroid::LayoutParams::Normal(150, 150, 200, 200);
+  view1_.SetLayoutForTesting(150, 150, 200, 200);
   GenerateTouchEventAt(100.f, 100.f);
   ExpectHit(client2_);
 }
 
 TEST_F(ViewAndroidBoundsTest, MatchesViewSizeOfkMatchParent) {
-  view1_.layout_params_ = ViewAndroid::LayoutParams::Normal(20, 20, 400, 600);
-  view3_.layout_params_ = ViewAndroid::LayoutParams::MatchParent();
-  view2_.layout_params_ = ViewAndroid::LayoutParams::Normal(50, 50, 200, 200);
+  view1_.SetLayoutForTesting(20, 20, 400, 600);
+  view2_.SetLayoutForTesting(50, 50, 200, 200);
 
   root_.AddChild(&view1_);
   root_.AddChild(&view2_);
-  view1_.AddChild(&view3_);
+  view1_.AddChild(&viewm_);
 
   GenerateTouchEventAt(100.f, 100.f);
   ExpectHit(client2_);
@@ -157,13 +159,13 @@ TEST_F(ViewAndroidBoundsTest, MatchesViewSizeOfkMatchParent) {
   client1_.SetHandleEvent(false);
   GenerateTouchEventAt(300.f, 400.f);
   EXPECT_TRUE(client1_.TouchEventCalled());
-  ExpectHit(client3_);
+  ExpectHit(clientm_);
 }
 
 TEST_F(ViewAndroidBoundsTest, MatchesViewsWithOffset) {
-  view1_.layout_params_ = ViewAndroid::LayoutParams::Normal(10, 20, 150, 100);
-  view2_.layout_params_ = ViewAndroid::LayoutParams::Normal(20, 30, 40, 30);
-  view3_.layout_params_ = ViewAndroid::LayoutParams::Normal(70, 30, 40, 30);
+  view1_.SetLayoutForTesting(10, 20, 150, 100);
+  view2_.SetLayoutForTesting(20, 30, 40, 30);
+  view3_.SetLayoutForTesting(70, 30, 40, 30);
 
   root_.AddChild(&view1_);
   view1_.AddChild(&view2_);
@@ -184,38 +186,31 @@ TEST_F(ViewAndroidBoundsTest, MatchesViewsWithOffset) {
 
 TEST_F(ViewAndroidBoundsTest, OnSizeChanged) {
   root_.AddChild(&view1_);
-  view1_.AddChild(&view2_);
+  view1_.AddChild(&viewm_);
   view1_.AddChild(&view3_);
 
-  view1_.layout_params_ = ViewAndroid::LayoutParams::Normal(0, 0, 0, 0);
-  view2_.layout_params_ = ViewAndroid::LayoutParams::MatchParent();
-  view3_.layout_params_ = ViewAndroid::LayoutParams::Normal(0, 0, 0, 0);
-
   // Size event propagates to non-match-parent children only.
   view1_.OnSizeChanged(100, 100);
   EXPECT_TRUE(client1_.OnSizeCalled());
-  EXPECT_TRUE(client2_.OnSizeCalled());
+  EXPECT_TRUE(clientm_.OnSizeCalled());
   EXPECT_FALSE(client3_.OnSizeCalled());
 
   Reset();
 
-  // TODO(jinsukkim): Enable following test once the top view can be
-  //     set to have match-parent property.
-
-  // Match-parent view should ignore the size event.
-  // view2_.OnSizeChanged(100, 200);
-  // EXPECT_FALSE(client2_.OnSizeCalled());
-  // EXPECT_FALSE(client3_.OnSizeCalled());
+  // Match-parent view should not receivee size events in the first place.
+  EXPECT_DCHECK_DEATH(viewm_.OnSizeChanged(100, 200));
+  EXPECT_FALSE(clientm_.OnSizeCalled());
+  EXPECT_FALSE(client3_.OnSizeCalled());
 
   Reset();
 
-  view2_.RemoveFromParent();
+  viewm_.RemoveFromParent();
   view1_.OnSizeChanged(100, 100);
 
   // Size event is generated for a newly added, match-parent child view.
-  EXPECT_FALSE(client2_.OnSizeCalled());
-  view1_.AddChild(&view2_);
-  EXPECT_TRUE(client2_.OnSizeCalled());
+  EXPECT_FALSE(clientm_.OnSizeCalled());
+  view1_.AddChild(&viewm_);
+  EXPECT_TRUE(clientm_.OnSizeCalled());
   EXPECT_FALSE(client3_.OnSizeCalled());
 }