Fix PiP window control visibility handling and tests

MediaSessionPictureInPictureWindowControllerBrowserTest.PreviousTrackButtonVisibility
(and a few similar tests too) didn't test the exact scenario it was meant to
test.  If it had, it would have _always_ failed when trying to verify the
button bounds at the end: the button size is set to 0x0 upon hiding, so bounds
comparison of a hidden and visible button should always fail.

The reason it didn't fail most of the time is because the button size was,
unintentionally, 0x0 throughout the test.  The size was kept at 0x0 because
never throughout the test was OverlayWindowViews::OnUpdateControlsBounds()
called with `show_previous_track_button_` true.

The test didn't execute the intended paths because RunUntilIdle() is misleading
here.  Seeing it used after setMediaSessionActionHandler('previoustrack') and
video.play(), we were inclined to think the previous track button was now
visible and the layout had been updated.  What really happened was we would
become idle even though the delayed OnUpdateControlsBounds() task had been
posted.  As the mouse was hovered over the window the button became visible,
but the layout was still pending.  And so a plausible explanation for the flaky
failures is: when executing under heavy load sometimes the 1-second delayed
layout did happen early enough to store the 20x20 size in
`previous_track_bounds`.

In this CL, we rework the test to wait for the desired events explicitly
instead of calling RunUntilIdle().  Other tests checking control visibility are
mirated as well.  The tests that add/remove buttons with the controls hidden
are moved to the unit test because it's hard for a browser test to know when
it's the right time to make assertions about layout if all the controls are
hidden.

Once fixed, the test uncovered two issues with the implementation which were
subsequently fixed:

 - When creating the window, UpdateControlsVisibility(false) was called too
   early.  The operations in OnRootViewReady() would override its effect.

 - Control visibility must be updated along with their layout.  Otherwise, a
   change in control visibility has no effect on the views::View visibility if
   it happens while the controls are shown (during hover).

With that done, we can enable
MediaSessionPictureInPictureWindowControllerBrowserTest.PreviousTrackButtonVisibility
again.

Bug: 985303
Change-Id: I411cdd2d0a55489ec5b7eb82bab95e05d84d323d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2587053Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Commit-Queue: Wojciech Dzierżanowski <wdzierzanowski@opera.com>
Cr-Commit-Position: refs/heads/master@{#837992}

chrome/browser/ui/views/overlay/overlay_window_views.cc

@@ -480,7 +480,6 @@ void OverlayWindowViews::SetUpViews() {
   resize_handle_view_ =
       AddChildView(&view_holder_, std::move(resize_handle_view));
 #endif
-  UpdateControlsVisibility(false);
 }
 
 void OverlayWindowViews::OnRootViewReady() {
@@ -496,6 +495,9 @@ void OverlayWindowViews::OnRootViewReady() {
   for (std::unique_ptr<views::View>& child : view_holder_)
     contents_view->AddChildView(std::move(child));
   view_holder_.clear();
+
+  // Don't show the controls until the mouse hovers over the window.
+  UpdateControlsVisibility(false);
 }
 
 void OverlayWindowViews::UpdateLayerBoundsWithLetterboxing(
@@ -626,7 +628,7 @@ void OverlayWindowViews::OnUpdateControlsBounds() {
       visible_controls_views[0]->SetPosition(
           gfx::Point(mid_window_x - kSecondaryControlSize.width() / 2,
                      secondary_control_y));
-      return;
+      break;
     }
     case 2: {
       /* | ----- [ ] [ ] ----- | */
@@ -638,7 +640,7 @@ void OverlayWindowViews::OnUpdateControlsBounds() {
       visible_controls_views[1]->SetSize(kSecondaryControlSize);
       visible_controls_views[1]->SetPosition(
           gfx::Point(mid_window_x + kControlMargin / 2, secondary_control_y));
-      return;
+      break;
     }
     case 3: {
       /* | --- [ ] [ ] [ ] --- | */
@@ -669,7 +671,7 @@ void OverlayWindowViews::OnUpdateControlsBounds() {
             mid_window_x + kSecondaryControlSize.width() / 2 + kControlMargin,
             secondary_control_y));
       }
-      return;
+      break;
     }
     case 4: {
       /* | - [ ] [ ] [ ] [ ] - | */
@@ -692,10 +694,15 @@ void OverlayWindowViews::OnUpdateControlsBounds() {
       visible_controls_views[3]->SetPosition(gfx::Point(
           mid_window_x + kControlMargin * 3 / 2 + kSecondaryControlSize.width(),
           secondary_control_y));
-      return;
+      break;
     }
+    default:
+      NOTREACHED();
   }
-  DCHECK(false);
+
+  // This will actually update the visibility of a control that was just added
+  // or removed, see SetPlayPauseButtonVisibility(), etc.
+  UpdateControlsVisibility(AreControlsVisible());
 }
 
 gfx::Rect OverlayWindowViews::CalculateControlsBounds(int x,
@@ -779,7 +786,11 @@ void OverlayWindowViews::SetPlayPauseButtonVisibility(bool is_visible) {
 }
 
 void OverlayWindowViews::SetSkipAdButtonVisibility(bool is_visible) {
+  if (show_skip_ad_button_ == is_visible)
+    return;
+
   show_skip_ad_button_ = is_visible;
+  UpdateControlsBounds();
 }
 
 void OverlayWindowViews::SetNextTrackButtonVisibility(bool is_visible) {
@@ -1032,6 +1043,11 @@ bool OverlayWindowViews::AreControlsVisible() const {
   return controls_scrim_view_->layer()->visible();
 }
 
+bool OverlayWindowViews::IsLayoutPendingForTesting() const {
+  return update_controls_bounds_timer_ &&
+         update_controls_bounds_timer_->IsRunning();
+}
+
 ui::Layer* OverlayWindowViews::GetControlsScrimLayer() {
   return controls_scrim_view_->layer();
 }
@@ -1108,8 +1124,8 @@ OverlayWindowViews::skip_ad_controls_view_for_testing() const {
   return skip_ad_controls_view_;
 }
 
-gfx::Point OverlayWindowViews::back_to_tab_image_position_for_testing() const {
-  return back_to_tab_controls_view_->origin();
+views::View* OverlayWindowViews::back_to_tab_controls_for_testing() const {
+  return back_to_tab_controls_view_;
 }
 
 gfx::Point OverlayWindowViews::close_image_position_for_testing() const {

chrome/browser/ui/views/overlay/overlay_window_views.h

@@ -84,11 +84,15 @@ class OverlayWindowViews : public content::OverlayWindow,
   // visible.
   bool AreControlsVisible() const;
 
+  // Determines whether a layout of the window controls has been scheduled but
+  // is not done yet.
+  bool IsLayoutPendingForTesting() const;
+
   views::PlaybackImageButton* play_pause_controls_view_for_testing() const;
   views::TrackImageButton* next_track_controls_view_for_testing() const;
   views::TrackImageButton* previous_track_controls_view_for_testing() const;
   views::SkipAdLabelButton* skip_ad_controls_view_for_testing() const;
-  gfx::Point back_to_tab_image_position_for_testing() const;
+  views::View* back_to_tab_controls_for_testing() const;
   gfx::Point close_image_position_for_testing() const;
   gfx::Point resize_handle_position_for_testing() const;
   OverlayWindowViews::PlaybackState playback_state_for_testing() const;

chrome/browser/ui/views/overlay/overlay_window_views_unittest.cc

@@ -6,6 +6,7 @@
 #include <utility>
 
 #include "chrome/browser/ui/views/overlay/overlay_window_views.h"
+#include "chrome/browser/ui/views/overlay/track_image_button.h"
 #include "chrome/test/base/testing_profile.h"
 #include "chrome/test/views/chrome_views_test_base.h"
 #include "content/public/browser/picture_in_picture_window_controller.h"
@@ -276,3 +277,40 @@ TEST_F(OverlayWindowViewsTest, IgnoreInvalidMaximumSize) {
   overlay_window().OnNativeWidgetMove();
   EXPECT_EQ(gfx::Size(500, 500), overlay_window().GetMaximumSize());
 }
+
+// Tests that Next Track button bounds are updated right away when window
+// controls are hidden.
+TEST_F(OverlayWindowViewsTest, NextTrackButtonAddedWhenControlsHidden) {
+  ASSERT_FALSE(overlay_window().AreControlsVisible());
+  ASSERT_TRUE(overlay_window()
+                  .next_track_controls_view_for_testing()
+                  ->size()
+                  .IsEmpty());
+
+  const auto origin_before_layout =
+      overlay_window().next_track_controls_view_for_testing()->origin();
+
+  overlay_window().SetNextTrackButtonVisibility(true);
+  EXPECT_NE(overlay_window().next_track_controls_view_for_testing()->origin(),
+            origin_before_layout);
+  EXPECT_FALSE(overlay_window().IsLayoutPendingForTesting());
+}
+
+// Tests that Previous Track button bounds are updated right away when window
+// controls are hidden.
+TEST_F(OverlayWindowViewsTest, PreviousTrackButtonAddedWhenControlsHidden) {
+  ASSERT_FALSE(overlay_window().AreControlsVisible());
+  ASSERT_TRUE(overlay_window()
+                  .previous_track_controls_view_for_testing()
+                  ->size()
+                  .IsEmpty());
+
+  const auto origin_before_layout =
+      overlay_window().previous_track_controls_view_for_testing()->origin();
+
+  overlay_window().SetPreviousTrackButtonVisibility(true);
+  EXPECT_NE(
+      overlay_window().previous_track_controls_view_for_testing()->origin(),
+      origin_before_layout);
+  EXPECT_FALSE(overlay_window().IsLayoutPendingForTesting());
+}