GitLab

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}