WebLayer: adjust browser controls visibility code

Since WebLayer was sending old_state for the constraint, a switch from
SHOWN to HIDDEN, as when entering fullscreen, could hit a DCHECK in
browser_controls_offset_manager.cc

I think some confusion arises from the re-use of a single enum for both
the _constraint_ and the _current state_. Code used the terms old_state
and new_state, when really the RenderFrameHost method expected a new
constraint and new desired state. The BOTH value in the context of a
constraint means unconstrained, and in the context of a state means
"don't change unless you have to to conform to the constraint".

Test: requestFullscreen on an element in weblayer shell doesn't cause
      the renderer to crash in debug

Bug: 1107647
Change-Id: I4f72e1144a9fc9a0cf0934b9640c660867c0d971
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2310652
Commit-Queue: Evan Stade <estade@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#790603}

weblayer/browser/java/org/chromium/weblayer_private/TabImpl.java

@@ -255,7 +255,8 @@ public final class TabImpl extends ITab.Stub implements LoginPrompt.Observer {
             mBrowserControlsDelegates.add(delegate);
             mBrowserControlsVisibility.addDelegate(delegate);
         }
-        mConstraintsUpdatedCallback = (constraints) -> onBrowserControlsStateUpdated(constraints);
+        mConstraintsUpdatedCallback =
+                (constraint) -> onBrowserControlsConstraintUpdated(constraint);
         mBrowserControlsVisibility.addObserver(mConstraintsUpdatedCallback);
 
         mInterceptNavigationDelegateClient = new InterceptNavigationDelegateClientImpl(this);
@@ -914,7 +915,7 @@ public final class TabImpl extends ITab.Stub implements LoginPrompt.Observer {
 
         if (mBrowser.getActiveTab() != this) return;
 
-        onBrowserControlsStateUpdated(mBrowserControlsVisibility.get());
+        onBrowserControlsConstraintUpdated(mBrowserControlsVisibility.get());
     }
 
     @VisibleForTesting
@@ -922,22 +923,22 @@ public final class TabImpl extends ITab.Stub implements LoginPrompt.Observer {
         return mBrowserControlsVisibility.get() == BrowserControlsState.BOTH;
     }
 
-    private void onBrowserControlsStateUpdated(int state) {
+    private void onBrowserControlsConstraintUpdated(int constraint) {
         // If something has overridden the FIP's SHOWN constraint, cancel FIP. This causes FIP to
         // dismiss when entering fullscreen.
-        if (state != BrowserControlsState.SHOWN) {
+        if (constraint != BrowserControlsState.SHOWN) {
             hideFindInPageUiAndNotifyClient();
         }
 
         // Don't animate when hiding the controls.
-        boolean animate = state != BrowserControlsState.HIDDEN;
+        boolean animate = constraint != BrowserControlsState.HIDDEN;
 
-        // If the renderer is not controlling the offsets (possiblye hung or crashed). Then this
+        // If the renderer is not controlling the offsets (possibly hung or crashed). Then this
         // needs to force the controls to show (because notification from the renderer will not
         // happen). For js dialogs, the renderer's update will come when the dialog is hidden, and
         // since that animates from 0 height, it causes a flicker since the override is already set
         // to fully show. Thus, disable animation.
-        if (state == BrowserControlsState.SHOWN && mBrowser != null
+        if (constraint == BrowserControlsState.SHOWN && mBrowser != null
                 && mBrowser.getActiveTab() == this
                 && !TabImplJni.get().isRendererControllingBrowserControlsOffsets(mNativeTab)) {
             mViewAndroidDelegate.setIgnoreRendererUpdates(true);
@@ -947,7 +948,7 @@ public final class TabImpl extends ITab.Stub implements LoginPrompt.Observer {
             mViewAndroidDelegate.setIgnoreRendererUpdates(false);
         }
 
-        TabImplJni.get().updateBrowserControlsState(mNativeTab, state, animate);
+        TabImplJni.get().updateBrowserControlsConstraint(mNativeTab, constraint, animate);
     }
 
     /**
@@ -976,7 +977,8 @@ public final class TabImpl extends ITab.Stub implements LoginPrompt.Observer {
         WebContents getWebContents(long nativeTabImpl);
         void executeScript(long nativeTabImpl, String script, boolean useSeparateIsolate,
                 Callback<String> callback);
-        void updateBrowserControlsState(long nativeTabImpl, int newConstraint, boolean animate);
+        void updateBrowserControlsConstraint(
+                long nativeTabImpl, int newConstraint, boolean animate);
         String getGuid(long nativeTabImpl);
         void captureScreenShot(long nativeTabImpl, float scale,
                 ValueCallback<Pair<Bitmap, Integer>> valueCallback);

weblayer/browser/tab_impl.cc

@@ -610,12 +610,15 @@ void TabImpl::OnAutofillProviderChanged(
   provider->OnJavaAutofillProviderChanged(env, autofill_provider);
 }
 
-void TabImpl::UpdateBrowserControlsState(JNIEnv* env,
-                                         jint raw_new_state,
-                                         jboolean animate) {
-  UpdateBrowserControlsStateImpl(
-      static_cast<content::BrowserControlsState>(raw_new_state),
-      current_browser_controls_state_, animate);
+void TabImpl::UpdateBrowserControlsConstraint(JNIEnv* env,
+                                              jint constraint,
+                                              jboolean animate) {
+  current_browser_controls_visibility_constraint_ =
+      static_cast<content::BrowserControlsState>(constraint);
+  // Passing BOTH here means that it doesn't matter what state the controls are
+  // currently in; don't change the current state unless it's incompatible with
+  // the new constraint.
+  UpdateBrowserControlsState(content::BROWSER_CONTROLS_STATE_BOTH, animate);
 }
 
 ScopedJavaLocalRef<jstring> TabImpl::GetGuid(JNIEnv* env) {
@@ -666,15 +669,15 @@ TabImpl::ScreenShotErrors TabImpl::PrepareForCaptureScreenShot(
   return ScreenShotErrors::kNone;
 }
 
-void TabImpl::UpdateBrowserControlsStateImpl(
+void TabImpl::UpdateBrowserControlsState(
     content::BrowserControlsState new_state,
-    content::BrowserControlsState old_state,
     bool animate) {
-  current_browser_controls_state_ = new_state;
   if (base::FeatureList::IsEnabled(kImmediatelyHideBrowserControlsForTest))
     animate = false;
-  web_contents_->GetMainFrame()->UpdateBrowserControlsState(new_state,
-                                                            old_state, animate);
+  // The constraint is managed by Java code, so re-use the existing constraint
+  // and only update the desired state.
+  web_contents_->GetMainFrame()->UpdateBrowserControlsState(
+      current_browser_controls_visibility_constraint_, new_state, animate);
 }
 
 void TabImpl::CaptureScreenShot(
@@ -1104,19 +1107,18 @@ void TabImpl::OnUpdateBrowserControlsStateBecauseOfProcessSwitch(
   // This matches the logic of updateAfterRendererProcessSwitch() and
   // updateEnabledState() in Chrome's TabBrowserControlsConstraintsHelper.
   if (did_commit &&
-      current_browser_controls_state_ ==
+      current_browser_controls_visibility_constraint_ ==
           content::BROWSER_CONTROLS_STATE_SHOWN &&
       top_controls_container_view_ &&
       top_controls_container_view_->IsFullyVisible()) {
     // The top-control is fully visible, don't animate this else the controls
     // bounce around.
-    UpdateBrowserControlsStateImpl(current_browser_controls_state_,
-                                   current_browser_controls_state_, false);
+    UpdateBrowserControlsState(content::BROWSER_CONTROLS_STATE_SHOWN, false);
   } else {
-    UpdateBrowserControlsStateImpl(current_browser_controls_state_,
-                                   content::BROWSER_CONTROLS_STATE_SHOWN,
-                                   current_browser_controls_state_ !=
-                                       content::BROWSER_CONTROLS_STATE_HIDDEN);
+    UpdateBrowserControlsState(
+        content::BROWSER_CONTROLS_STATE_BOTH,
+        current_browser_controls_visibility_constraint_ !=
+            content::BROWSER_CONTROLS_STATE_HIDDEN);
   }
 }
 

weblayer/browser/tab_impl.h

@@ -165,9 +165,9 @@ class TabImpl : public Tab,
   void OnAutofillProviderChanged(
       JNIEnv* env,
       const base::android::JavaParamRef<jobject>& autofill_provider);
-  void UpdateBrowserControlsState(JNIEnv* env,
-                                  jint raw_new_state,
-                                  jboolean animate);
+  void UpdateBrowserControlsConstraint(JNIEnv* env,
+                                       jint constraint,
+                                       jboolean animate);
 
   base::android::ScopedJavaLocalRef<jstring> GetGuid(JNIEnv* env);
   void CaptureScreenShot(
@@ -307,9 +307,8 @@ class TabImpl : public Tab,
       gfx::Rect* src_rect,
       gfx::Size* output_size);
 
-  void UpdateBrowserControlsStateImpl(content::BrowserControlsState new_state,
-                                      content::BrowserControlsState old_state,
-                                      bool animate);
+  void UpdateBrowserControlsState(content::BrowserControlsState new_state,
+                                  bool animate);
 #endif
 
   // content::WebContentsObserver:
@@ -369,9 +368,14 @@ class TabImpl : public Tab,
       browser_controls_navigation_state_handler_;
   int top_controls_min_height_ = 0;
 
-  // Last value supplied to UpdateBrowserControlsState().
-  content::BrowserControlsState current_browser_controls_state_ =
-      content::BROWSER_CONTROLS_STATE_SHOWN;
+  // Last value supplied to UpdateBrowserControlsConstraint(). This *constraint*
+  // can be SHOWN, if for example a modal dialog is forcing the controls to be
+  // visible, HIDDEN, if for example fullscreen is forcing the controls to be
+  // hidden, or BOTH, if either state is viable (e.g. during normal browsing).
+  // When BOTH, the actual current state could be showing or hidden.
+  content::BrowserControlsState
+      current_browser_controls_visibility_constraint_ =
+          content::BROWSER_CONTROLS_STATE_SHOWN;
 
   std::map<std::string, std::unique_ptr<WebMessageHostFactoryProxy>>
       js_name_to_proxy_;