weblayer: more attempt at fixing bottom test flake

The main thing this adds is waiting for cc to see the bottom-controls height by way of cc::RenderFrameMetadata. This is necessary because if we send scroll events before cc (BrowserControlsOffsetManager) gets the height, then no scrolling happens and the test fails. This also turns off delaying hide/show for tests. Unfortunately waiting for cc to get the height requires private api, so this test is moved to the private tests. I'm starting with a single test, if this proves stable I'll update the rest (and likely refactor this to be shared with the TopControlsTest). BUG=1077825 TEST=BottomControlsTest Change-Id: I1a1eadbee7b6f19f759b133df1739071150b6e25 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2194233 Reviewed-by: Bo <boliu@chromium.org> Commit-Queue: Scott Violet <sky@chromium.org> Cr-Commit-Position: refs/heads/master@{#769867}
Denger-Network · May 18, 2020 · 5b44999 · 5b44999
1 parent 29fbbeb
commit 5b44999
Show file tree

Hide file tree

Showing 20 changed files with 292 additions and 54 deletions.
diff --git a/content/public/test/browser_test_utils.cc b/content/public/test/browser_test_utils.cc
@@ -2671,6 +2671,13 @@ RenderFrameSubmissionObserver::LastRenderFrameMetadata() const {
   return render_frame_metadata_provider_->LastRenderFrameMetadata();
 }
 
+void RenderFrameSubmissionObserver::NotifyOnNextMetadataChange(
+    base::OnceClosure closure) {
+  DCHECK(closure);
+  DCHECK(metadata_change_closure_.is_null());
+  metadata_change_closure_ = std::move(closure);
+}
+
 void RenderFrameSubmissionObserver::Quit() {
   if (quit_closure_)
     std::move(quit_closure_).Run();
@@ -2689,6 +2696,8 @@ void RenderFrameSubmissionObserver::
 void RenderFrameSubmissionObserver::
     OnRenderFrameMetadataChangedAfterActivation() {
   Quit();
+  if (metadata_change_closure_)
+    std::move(metadata_change_closure_).Run();
 }
 
 void RenderFrameSubmissionObserver::OnRenderFrameSubmission() {

diff --git a/content/public/test/browser_test_utils.h b/content/public/test/browser_test_utils.h
@@ -1289,6 +1289,9 @@ class RenderFrameSubmissionObserver
   // Returns the number of frames submitted since the observer's creation.
   int render_frame_count() const { return render_frame_count_; }
 
+  // Runs |closure| the next time metadata changes.
+  void NotifyOnNextMetadataChange(base::OnceClosure closure);
+
  private:
   // Exits |run_loop_| unblocking the UI thread. Execution will resume in Wait.
   void Quit();
@@ -1311,6 +1314,8 @@ class RenderFrameSubmissionObserver
 
   RenderFrameMetadataProviderImpl* render_frame_metadata_provider_ = nullptr;
   base::OnceClosure quit_closure_;
+  // If non-null, run when metadata changes.
+  base::OnceClosure metadata_change_closure_;
   int render_frame_count_ = 0;
 };
 

diff --git a/weblayer/BUILD.gn b/weblayer/BUILD.gn
@@ -223,6 +223,8 @@ source_set("weblayer_lib_base") {
     "browser/weblayer_browser_interface_binders.h",
     "browser/weblayer_content_browser_overlay_manifest.cc",
     "browser/weblayer_content_browser_overlay_manifest.h",
+    "browser/weblayer_features.cc",
+    "browser/weblayer_features.h",
     "browser/weblayer_field_trials.h",
     "browser/weblayer_security_blocking_page_factory.cc",
     "browser/weblayer_security_blocking_page_factory.h",
@@ -524,6 +526,19 @@ source_set("weblayer_lib_base") {
 }
 
 if (is_android) {
+  source_set("weblayer_android_test_jni_impl") {
+    testonly = true
+    sources = [ "browser/test/test_weblayer_impl.cc" ]
+    deps = [
+      ":weblayer_lib_base",
+      "//base",
+      "//content/public/browser",
+      "//content/test:test_support",
+      "//testing/gtest",
+      "//weblayer/browser/java:test_jni",
+    ]
+  }
+
   # Lib used in standalone WebView which allows manual JNI registration.
   static_library("weblayer_lib_webview") {
     public_deps = [ ":weblayer_lib_base" ]
@@ -550,6 +565,33 @@ if (is_android) {
     }
   }
 
+  static_library("weblayer_lib_webview_test") {
+    testonly = true
+    public_deps = [ ":weblayer_lib_base" ]
+    deps = [
+      ":weblayer_android_test_jni_impl",
+      "//base",
+      "//weblayer/browser/java:jni",
+      "//weblayer/browser/java:weblayer_jni_registration",
+    ]
+    sources = [
+      "$target_gen_dir/browser/java/weblayer_jni_registration.h",
+      "browser/web_view_compatibility_helper_impl.cc",
+      "browser/web_view_compatibility_helper_impl.h",
+    ]
+    defines = [ "WEBLAYER_MANUAL_JNI_REGISTRATION" ]
+
+    # Explicit dependency required for JNI registration to be able to
+    # find the native side functions.
+    if (is_component_build) {
+      deps += [
+        "//device/gamepad",
+        "//media/midi",
+        "//ui/events/devices",
+      ]
+    }
+  }
+
   # Lib used in Monochrome which does not support manual JNI registration.
   # Separate from the standalone WebView version to reduce APK size.
   static_library("weblayer_lib") {
@@ -560,6 +602,18 @@ if (is_android) {
       "browser/web_view_compatibility_helper_impl.h",
     ]
   }
+
+  shared_library("libweblayer_test") {
+    testonly = true
+    sources = [ "app/entry_point.cc" ]
+    deps = [
+      ":weblayer_lib_webview_test",
+      "//base",
+      "//content/public/app",
+    ]
+    configs -= [ "//build/config/android:hide_all_but_jni_onload" ]
+    configs += [ "//build/config/android:hide_all_but_jni" ]
+  }
 } else {
   source_set("weblayer_lib") {
     public_deps = [ ":weblayer_lib_base" ]
@@ -579,19 +633,5 @@ grit("resources") {
   ]
   deps = [ "//weblayer/browser/webui:mojo_bindings_js" ]
 }
-
 # TODO(jam): move weblayer_shell_resources_grit and copy_shell_resources here in
 # a way that's shareable?
-
-if (is_android) {
-  shared_library("libweblayer") {
-    sources = [ "app/entry_point.cc" ]
-    deps = [
-      ":weblayer_lib_webview",
-      "//base",
-      "//content/public/app",
-    ]
-    configs -= [ "//build/config/android:hide_all_but_jni_onload" ]
-    configs += [ "//build/config/android:hide_all_but_jni" ]
-  }
-}
diff --git a/weblayer/browser/android/javatests/BUILD.gn b/weblayer/browser/android/javatests/BUILD.gn
@@ -9,15 +9,13 @@ import("//build/config/android/rules.gni")
 android_library("weblayer_java_tests") {
   testonly = true
   sources = [
-    "src/org/chromium/weblayer/test/BottomControlsTest.java",
     "src/org/chromium/weblayer/test/BrowserFragmentLifecycleTest.java",
     "src/org/chromium/weblayer/test/CookieManagerTest.java",
     "src/org/chromium/weblayer/test/CrashReporterTest.java",
     "src/org/chromium/weblayer/test/DataClearingTest.java",
     "src/org/chromium/weblayer/test/DowngradeTest.java",
     "src/org/chromium/weblayer/test/DownloadCallbackTest.java",
     "src/org/chromium/weblayer/test/ErrorPageCallbackTest.java",
-    "src/org/chromium/weblayer/test/EventUtils.java",
     "src/org/chromium/weblayer/test/ExecuteScriptTest.java",
     "src/org/chromium/weblayer/test/ExternalNavigationTest.java",
     "src/org/chromium/weblayer/test/FindInPageTest.java",
@@ -62,6 +60,7 @@ android_library("weblayer_java_tests") {
 android_library("weblayer_private_java_tests") {
   testonly = true
   sources = [
+    "src/org/chromium/weblayer/test/BottomControlsTest.java",
     "src/org/chromium/weblayer/test/GeolocationTest.java",
     "src/org/chromium/weblayer/test/MediaCaptureTest.java",
     "src/org/chromium/weblayer/test/NetworkChangeNotifierTest.java",
@@ -87,6 +86,7 @@ android_library("weblayer_java_test_support") {
   testonly = true
   sources = [
     "src/org/chromium/weblayer/test/BoundedCountDownLatch.java",
+    "src/org/chromium/weblayer/test/EventUtils.java",
     "src/org/chromium/weblayer/test/InstrumentationActivityTestRule.java",
     "src/org/chromium/weblayer/test/MinWebLayerVersion.java",
     "src/org/chromium/weblayer/test/MinWebLayerVersionSkipCheck.java",

diff --git a/weblayer/browser/android/javatests/skew/expectations.txt b/weblayer/browser/android/javatests/skew/expectations.txt
@@ -31,10 +31,6 @@
 # Fixed in https://crrev.com/c/2180022, see https://crbug.com/1077243.
 [ impl_lte_83 ] org.chromium.weblayer.test.FullscreenCallbackTest#testExitFullscreenWhenTabDestroyed [ Skip ]
 
-# https://crbug.com/1079489.
-[ impl_lte_83 ] org.chromium.weblayer.test.BottomControlsTest#testBasic [ Skip ]
-[ impl_lte_83 ] org.chromium.weblayer.test.BottomControlsTest#testNoTopControl [ Skip ]
-
 # https://crbug.com/1079491.
 [ impl_lte_83 ] org.chromium.weblayer.test.NavigationTest#testSetUserAgentString [ Skip ]
 

diff --git a/weblayer/browser/android/javatests/src/org/chromium/weblayer/test/BottomControlsTest.java b/weblayer/browser/android/javatests/src/org/chromium/weblayer/test/BottomControlsTest.java
@@ -5,6 +5,7 @@
 package org.chromium.weblayer.test;
 
 import android.os.Build;
+import android.os.RemoteException;
 import android.support.test.filters.SmallTest;
 import android.view.View;
 import android.widget.TextView;
@@ -21,6 +22,7 @@
 import org.chromium.content_public.browser.test.util.Criteria;
 import org.chromium.content_public.browser.test.util.CriteriaHelper;
 import org.chromium.content_public.browser.test.util.TestThreadUtils;
+import org.chromium.weblayer.TestWebLayer;
 import org.chromium.weblayer.shell.InstrumentationActivity;
 
 /**
@@ -34,6 +36,8 @@ public class BottomControlsTest {
             new InstrumentationActivityTestRule();
 
     private int mMaxControlsHeight;
+    private int mTopControlsHeight;
+    private int mBottomControlsHeight;
     private int mInitialVisiblePageHeight;
 
     /**
@@ -44,26 +48,49 @@ private int getVisiblePageHeight() {
         return mActivityTestRule.executeScriptAndExtractInt("window.innerHeight");
     }
 
+    private void waitForBrowserControlsViewToBeVisible(View v) {
+        CriteriaHelper.pollUiThread(() -> {
+            Assert.assertTrue(v.getHeight() > 0);
+            Assert.assertEquals(View.VISIBLE, v.getVisibility());
+        });
+    }
+
+    // See TestWebLayer.waitForBrowserControlsMetadataState() for details on this.
+    private void waitForBrowserControlsMetadataState(
+            InstrumentationActivity activity, int top, int bottom) throws Exception {
+        BoundedCountDownLatch latch = new BoundedCountDownLatch(1);
+        TestThreadUtils.runOnUiThreadBlocking(() -> {
+            try {
+                TestWebLayer.getTestWebLayer(activity.getApplicationContext())
+                        .waitForBrowserControlsMetadataState(activity.getBrowser().getActiveTab(),
+                                top, bottom, () -> { latch.countDown(); });
+            } catch (RemoteException e) {
+                throw new RuntimeException(e);
+            }
+        });
+        latch.timedAwait();
+    }
+
     // Disabled on L bots due to unexplained flakes. See crbug.com/1035894.
     @MinAndroidSdkLevel(Build.VERSION_CODES.M)
     @Test
     @SmallTest
-    @DisabledTest
     public void testBasic() throws Exception {
-        final String url = UrlUtils.encodeHtmlDataUri("<body><p style='height:5000px'>");
+        final String url = mActivityTestRule.getTestDataURL("tall_page.html");
         InstrumentationActivity activity = mActivityTestRule.launchShellWithUrl(url);
 
         // Poll until the top view becomes visible.
-        CriteriaHelper.pollUiThread(Criteria.equals(
-                View.VISIBLE, () -> activity.getTopContentsContainer().getVisibility()));
+        waitForBrowserControlsViewToBeVisible(activity.getTopContentsContainer());
         TestThreadUtils.runOnUiThreadBlocking(() -> {
-            mMaxControlsHeight = activity.getTopContentsContainer().getHeight();
+            mMaxControlsHeight = mTopControlsHeight =
+                    activity.getTopContentsContainer().getHeight();
             Assert.assertTrue(mMaxControlsHeight > 0);
         });
 
-        // Ask for the page height. While the height isn't needed, the call to the renderer helps
-        // ensure the renderer's height has updated based on the top-control.
-        getVisiblePageHeight();
+        // Wait for cc to see the top-controls height.
+        waitForBrowserControlsMetadataState(activity, mTopControlsHeight, 0);
+
+        int pageHeightWithTopView = getVisiblePageHeight();
 
         View bottomView = TestThreadUtils.runOnUiThreadBlocking(() -> {
             TextView view = new TextView(activity);
@@ -72,36 +99,43 @@ public void testBasic() throws Exception {
             return view;
         });
 
-        // Poll until the bottom view becomes visible.
-        CriteriaHelper.pollUiThread(
-                Criteria.equals(View.VISIBLE, () -> bottomView.getVisibility()));
-
-        // Ask for the page height. While the height isn't needed, the call to the renderer helps
-        // ensure the renderer's height has updated based on the top-control.
-        getVisiblePageHeight();
+        waitForBrowserControlsViewToBeVisible(bottomView);
 
         TestThreadUtils.runOnUiThreadBlocking(() -> {
-            // Scrolling code ends up using the max height.
-            mMaxControlsHeight = Math.max(mMaxControlsHeight, bottomView.getHeight());
-            Assert.assertTrue(mMaxControlsHeight > 0);
-            Assert.assertTrue(bottomView.getHeight() > 0);
+            // The amount necessary to scroll is the sum of the two views. This is because the page
+            // height is reduced by the sum of these two.
+            mBottomControlsHeight = bottomView.getHeight();
+            Assert.assertTrue(mBottomControlsHeight > 0);
+            mMaxControlsHeight += mBottomControlsHeight;
         });
 
+        // Wait for cc to see the bottom height. This is very important, as scrolling is gated by
+        // cc getting the bottom height.
+        waitForBrowserControlsMetadataState(activity, mTopControlsHeight, mBottomControlsHeight);
+
+        // Adding a bottom view should change the page height.
+        CriteriaHelper.pollInstrumentationThread(
+                () -> Assert.assertNotEquals(getVisiblePageHeight(), pageHeightWithTopView));
+        int pageHeightWithTopAndBottomViews = getVisiblePageHeight();
+        Assert.assertTrue(pageHeightWithTopAndBottomViews < pageHeightWithTopView);
+
         // Move by the size of the controls.
         EventUtils.simulateDragFromCenterOfView(
                 activity.getWindow().getDecorView(), 0, -mMaxControlsHeight);
 
-        // Moving should hide the bottom-controls View.
+        // Moving should hide the bottom View.
         CriteriaHelper.pollUiThread(
                 Criteria.equals(View.INVISIBLE, () -> bottomView.getVisibility()));
+        CriteriaHelper.pollInstrumentationThread(
+                Criteria.equals(true, () -> getVisiblePageHeight() > pageHeightWithTopView));
 
-        // Move so bottom-controls are shown again.
+        // Move so top and bottom-controls are shown again.
         EventUtils.simulateDragFromCenterOfView(
                 activity.getWindow().getDecorView(), 0, mMaxControlsHeight);
 
-        // bottom-controls are shown async.
-        CriteriaHelper.pollUiThread(
-                Criteria.equals(View.VISIBLE, () -> bottomView.getVisibility()));
+        waitForBrowserControlsViewToBeVisible(bottomView);
+        CriteriaHelper.pollInstrumentationThread(
+                () -> Assert.assertEquals(getVisiblePageHeight(), pageHeightWithTopAndBottomViews));
     }
 
     // Disabled on L bots due to unexplained flakes. See crbug.com/1035894.

diff --git a/weblayer/browser/browser_controls_container_view.cc b/weblayer/browser/browser_controls_container_view.cc
@@ -5,6 +5,7 @@
 #include "weblayer/browser/browser_controls_container_view.h"
 
 #include "base/android/jni_string.h"
+#include "base/feature_list.h"
 #include "cc/layers/ui_resource_layer.h"
 #include "content/public/browser/android/compositor.h"
 #include "content/public/browser/render_view_host.h"
@@ -16,6 +17,7 @@
 #include "ui/android/view_android.h"
 #include "weblayer/browser/content_view_render_view.h"
 #include "weblayer/browser/java/jni/BrowserControlsContainerView_jni.h"
+#include "weblayer/browser/weblayer_features.h"
 
 using base::android::AttachCurrentThread;
 using base::android::JavaParamRef;
@@ -151,4 +153,9 @@ JNI_BrowserControlsContainerView_CreateBrowserControlsContainerView(
       is_top));
 }
 
+static jboolean JNI_BrowserControlsContainerView_ShouldDelayVisibilityChange(
+    JNIEnv* env) {
+  return !base::FeatureList::IsEnabled(kImmediatelyHideBrowserControlsForTest);
+}
+
 }  // namespace weblayer
diff --git a/weblayer/browser/browser_controls_navigation_state_handler.cc b/weblayer/browser/browser_controls_navigation_state_handler.cc
@@ -13,11 +13,10 @@
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/url_constants.h"
 #include "weblayer/browser/browser_controls_navigation_state_handler_delegate.h"
+#include "weblayer/browser/weblayer_features.h"
 
 namespace weblayer {
 namespace {
-const base::Feature kImmediatelyHideBrowserControlsForTest{
-    "ImmediatelyHideBrowserControlsForTest", base::FEATURE_DISABLED_BY_DEFAULT};
 
 // The time that must elapse after a navigation before the browser controls can
 // be hidden. This value matches what chrome has in

diff --git a/weblayer/browser/java/BUILD.gn b/weblayer/browser/java/BUILD.gn
@@ -179,7 +179,9 @@ android_library("test_java") {
   testonly = true
   sources = [ "org/chromium/weblayer_private/test/TestWebLayerImpl.java" ]
   deps = [
+    ":java",
     ":weblayer_test_resources",
+    "//base:jni_java",
     "//components/permissions/android:java",
     "//content/public/test/android:content_java_test_support",
     "//net/android:net_java",
@@ -188,6 +190,12 @@ android_library("test_java") {
     "//ui/android:ui_full_java",
   ]
   srcjar_deps = [ ":test_aidl" ]
+  annotation_processor_deps = [ "//base/android/jni_generator:jni_processor" ]
+}
+
+generate_jni("test_jni") {
+  testonly = true
+  sources = [ "org/chromium/weblayer_private/test/TestWebLayerImpl.java" ]
 }
 
 generate_jni("jni") {