Hide overlay play button if it can't be shown without clipping

third_party/WebKit/Source/core/html/shadow/MediaControls.cpp

Index: third_party/WebKit/Source/core/html/shadow/MediaControls.cpp
diff --git a/third_party/WebKit/Source/core/html/shadow/MediaControls.cpp b/third_party/WebKit/Source/core/html/shadow/MediaControls.cpp
index 3d5cff44270ef8cc0f3189a9601eb66169b10e9c..4681ae79da5393ab6069b4bb71dc74b29430c1ab 100644
--- a/third_party/WebKit/Source/core/html/shadow/MediaControls.cpp
+++ b/third_party/WebKit/Source/core/html/shadow/MediaControls.cpp
@@ -29,6 +29,9 @@
 #include "bindings/core/v8/ExceptionState.h"
 #include "core/dom/ClientRect.h"
 #include "core/dom/Fullscreen.h"
+#include "core/dom/ResizeObserver.h"
+#include "core/dom/ResizeObserverCallback.h"
+#include "core/dom/ResizeObserverEntry.h"
 #include "core/dom/TaskRunnerHelper.h"
 #include "core/events/MouseEvent.h"
 #include "core/frame/Settings.h"
@@ -45,6 +48,18 @@
 
 namespace blink {
 
+namespace {
+constexpr int kOverlayPlayButtonWidth = 48;
+constexpr int kOverlayPlayButtonHeight = 48;
+constexpr int kOverlayBottomMargin = 10;
+constexpr int kAndroidMediaPanelHeight = 48;
+
+constexpr int kMinWidthForOverlayPlayButton = kOverlayPlayButtonWidth;
+constexpr int kMinHeightForOverlayPlayButton = kOverlayPlayButtonHeight +
+                                               (2 * kAndroidMediaPanelHeight) +
+                                               (2 * kOverlayBottomMargin);
+}  // namespace
+
 // If you change this value, then also update the corresponding value in
 // LayoutTests/media/media-controls.js.
 static const double timeWithoutMouseMovementBeforeHidingMediaControls = 3;
@@ -114,8 +129,37 @@ class MediaControls::BatchedControlUpdate {
 // Count of number open batches for controls visibility.
 int MediaControls::BatchedControlUpdate::s_batchDepth = 0;
 
+class MediaControls::MediaControlsResizeObserverCallback

[whywhat, 2017/02/23 21:37:31]

nit: could you add a comment to the forward declarations in the header that
they're needed for private member access?

[steimel, 2017/02/24 00:42:43]

Done.

+    : public ResizeObserverCallback {
+ public:
+  explicit MediaControlsResizeObserverCallback(MediaControls* controls)
+      : m_controls(controls) {
+    DCHECK(controls);
+  }
+  ~MediaControlsResizeObserverCallback() override = default;
+
+  void handleEvent(const HeapVector<Member<ResizeObserverEntry>>& entries,
+                   ResizeObserver* observer) override {
+    DCHECK_EQ(1u, entries.size());

[whywhat, 2017/02/23 21:37:31]

nit: DCHECK entries[0]->target() is m_mediaElement?

[steimel, 2017/02/24 00:42:43]

Done.

+    ClientRect* contentRect = entries[0]->contentRect();
+    m_controls->notifyElementSizeChanged(contentRect->width(),
+                                         contentRect->height());
+  }
+
+  DEFINE_INLINE_TRACE() {
+    visitor->trace(m_controls);
+    ResizeObserverCallback::trace(visitor);
+  }
+
+ private:
+  Member<MediaControls> m_controls;
+};
+
 MediaControls::MediaControls(HTMLMediaElement& mediaElement)
     : HTMLDivElement(mediaElement.document()),
+      m_resizeObserver(ResizeObserver::create(
+          mediaElement.document(),
+          new MediaControlsResizeObserverCallback(this))),
       m_mediaElement(&mediaElement),
       m_overlayEnclosure(nullptr),
       m_overlayPlayButton(nullptr),
@@ -146,12 +190,15 @@ MediaControls::MediaControls(HTMLMediaElement& mediaElement)
       m_hideTimerBehaviorFlags(IgnoreNone),
       m_isMouseOverControls(false),
       m_isPausedForScrubbing(false),
-      m_panelWidthChangedTimer(TaskRunnerHelper::get(TaskType::UnspecedTimer,
-                                                     &mediaElement.document()),
-                               this,
-                               &MediaControls::panelWidthChangedTimerFired),
-      m_panelWidth(0),
-      m_keepShowingUntilTimerFires(false) {}
+      m_elementSizeChangedTimer(TaskRunnerHelper::get(TaskType::UnspecedTimer,

[whywhat, 2017/02/23 21:37:32]

rant: I'm sad unspeced is used as a word (however internal search reveals it's
being used sometimes).

[steimel, 2017/02/24 00:42:43]

Acknowledged :)

+                                                      &mediaElement.document()),
+                                this,
+                                &MediaControls::elementSizeChangedTimerFired),
+      m_elementWidth(0),
+      m_elementHeight(0),
+      m_keepShowingUntilTimerFires(false) {
+  m_resizeObserver->observe(m_mediaElement);
+}
 
 MediaControls* MediaControls::create(HTMLMediaElement& mediaElement,
                                      ShadowRoot& shadowRoot) {
@@ -788,25 +835,28 @@ void MediaControls::onExitedFullscreen() {
   startHideMediaControlsTimer();
 }
 
-void MediaControls::notifyPanelWidthChanged(const LayoutUnit& newWidth) {
+void MediaControls::notifyElementSizeChanged(int newWidth, int newHeight) {
   // Don't bother to do any work if this matches the most recent panel
-  // width, since we're called after layout.
+  // size, since we're called after layout.

[whywhat, 2017/02/23 21:37:31]

nit: I'm not sure this comment is up-to-date, I don't see any check for the
width/height to match the known ones.

[steimel, 2017/02/24 00:42:43]

You're right. At some point, I guess that logic was moved to LayoutMedia, though
in this patchset that has been removed to be handled by the ResizeObserver.
However, in the next patchset (in order to address your comment below) the logic
will be re-added here at the bottom of this method. I'll move this comment
there.

   // Note that this code permits a bad frame on resize, since it is
   // run after the relayout / paint happens.  It would be great to improve
   // this, but it would be even greater to move this code entirely to
   // JS and fix it there.
-  m_panelWidth = newWidth.toInt();
+  m_elementWidth = newWidth;
+  m_elementHeight = newHeight;
+  m_sizingInitialized = true;
 
   // Adjust for effective zoom.
   if (!m_panel->layoutObject() || !m_panel->layoutObject()->style())
     return;
-  m_panelWidth =
-      ceil(m_panelWidth / m_panel->layoutObject()->style()->effectiveZoom());
 
-  m_panelWidthChangedTimer.startOneShot(0, BLINK_FROM_HERE);
+  m_elementWidth =

[whywhat, 2017/02/23 21:37:31]

is it still the element's width or panel's width after the computation?
is it possible the new value of m_elementWidth is the same as the one used
before the resize event after this computation (so we get the event but don't
need to do anything since from the panel size perspective nothing changed)?

[steimel, 2017/02/24 00:42:43]

Those are both good points. For the first, I've changed the member to be called
|m_effective(Width|Height)|. For the second, I've added the logic for not firing
the size-change timer if the effectiveWidth hasn't changed. This also brings up
whether or not we need to worry about effectiveZoom for height. I assume the
height is affected in the same manner as the width, but since the height is only
used for the clipping calculation, which (at least currently) depends on
fixed-pixel-calculation that may be affected by the zoom as well, not sure if
it's necessary. I'm happy to look into it though

+      ceil(m_elementWidth / m_panel->layoutObject()->style()->effectiveZoom());
+
+  m_elementSizeChangedTimer.startOneShot(0, BLINK_FROM_HERE);
 }
 
-void MediaControls::panelWidthChangedTimerFired(TimerBase*) {
+void MediaControls::elementSizeChangedTimerFired(TimerBase*) {
   computeWhichControlsFit();
 }
 
@@ -834,7 +884,7 @@ void MediaControls::computeWhichControlsFit() {
   // element.
   const int sliderMargin = 36;  // Sliders have 18px margin on each side.
 
-  if (!m_panelWidth) {
+  if (!m_elementWidth) {
     // No layout yet -- hide everything, then make them show up later.
     // This prevents the wrong controls from being shown briefly
     // immediately after the first layout and paint, but before we have
@@ -879,7 +929,7 @@ void MediaControls::computeWhichControlsFit() {
       width += sliderMargin;
     element->shouldShowButtonInOverflowMenu(false);
     if (element->isWanted()) {
-      if (usedWidth + width <= m_panelWidth) {
+      if (usedWidth + width <= m_elementWidth) {
         element->setDoesFit(true);
         usedWidth += width;
       } else {
@@ -907,13 +957,20 @@ void MediaControls::computeWhichControlsFit() {
       if ((firstDisplacedElement == m_timeline.get()) ||
           (firstDisplacedElement == m_volumeSlider.get()))
         width += sliderMargin;
-      if (usedWidth + width <= m_panelWidth)
+      if (usedWidth + width <= m_elementWidth)
         firstDisplacedElement->setDoesFit(true);
     }
   } else if (overflowElements.size() == 1) {
     m_overflowMenu->setIsWanted(false);
     overflowElements.front()->setDoesFit(true);
   }
+
+  // Decide if the overlay play button fits.
+  if (m_elementWidth && m_elementHeight && m_overlayPlayButton) {
+    bool doesFit = m_elementWidth >= kMinWidthForOverlayPlayButton &&
+                   m_elementHeight >= kMinHeightForOverlayPlayButton;
+    m_overlayPlayButton->setDoesFit(doesFit);
+  }
 }
 
 void MediaControls::invalidate(Element* element) {
@@ -935,6 +992,12 @@ void MediaControls::networkStateChanged() {
   invalidate(m_volumeSlider);
 }
 
+void MediaControls::onLayout(int width, int height) {
+  if (!m_sizingInitialized) {

[whywhat, 2017/02/23 21:37:31]

nit: no need for {}

I wish we could somehow request the size in the ctor before registering the
ResizeObserver but I guess it's not feasible.

[steimel, 2017/02/24 00:42:43]

Right. I had tried to initialize in the ctor (and then in initializeControls),
however in order to get the size, I had to force a layout, which was breaking a
unit test that depended on a layout *not* occurring on initialization
(MediaControlsTest.ResetDoesNotTriggerInitialLayout). If not forcing a layout on
initialization is *not* a necessary restriction, then I can revisit that

+    notifyElementSizeChanged(width, height);
+  }
+}
+
 bool MediaControls::overflowMenuVisible() {
   return m_overflowList ? m_overflowList->isWanted() : false;
 }
@@ -957,6 +1020,7 @@ void MediaControls::hideAllMenus() {
 }
 
 DEFINE_TRACE(MediaControls) {
+  visitor->trace(m_resizeObserver);
   visitor->trace(m_mediaElement);
   visitor->trace(m_panel);
   visitor->trace(m_overlayPlayButton);