New media playback UI.

Source/core/html/shadow/MediaControls.cpp

 /*
  * Copyright (C) 2011, 2012 Apple Inc. All rights reserved.
  * Copyright (C) 2011, 2012 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 50 matching lines @@
     , m_durationDisplay(nullptr)
     , m_muteButton(nullptr)
     , m_volumeSlider(nullptr)
     , m_toggleClosedCaptionsButton(nullptr)
     , m_castButton(nullptr)
     , m_fullScreenButton(nullptr)
     , m_hideMediaControlsTimer(this, &MediaControls::hideMediaControlsTimerFired)
     , m_hideTimerBehaviorFlags(IgnoreNone)
     , m_isMouseOverControls(false)
     , m_isPausedForScrubbing(false)
+    , m_panelWidthChangedTimer(this, &MediaControls::panelWidthChangedTimerFired)
+    , m_panelWidth(0)
 {
 }
 
 PassRefPtrWillBeRawPtr<MediaControls> MediaControls::create(HTMLMediaElement& mediaElement)
 {
     RefPtrWillBeRawPtr<MediaControls> controls = adoptRefWillBeNoop(new MediaControls(mediaElement));
     controls->setShadowPseudoId(AtomicString("-webkit-media-controls", AtomicString::ConstructFromLiteral));
     controls->initializeControls();
     return controls.release();
 }
 
 // The media controls DOM structure looks like:
 //
 // MediaControls                                       (-webkit-media-controls)
 // +-MediaControlOverlayEnclosureElement               (-webkit-media-controls-overlay-enclosure)
 // | +-MediaControlOverlayPlayButtonElement            (-webkit-media-controls-overlay-play-button)
 // | | {if mediaControlsOverlayPlayButtonEnabled}
 // | \-MediaControlCastButtonElement                   (-internal-media-controls-overlay-cast-button)
 // \-MediaControlPanelEnclosureElement                 (-webkit-media-controls-enclosure)
 //   \-MediaControlPanelElement                        (-webkit-media-controls-panel)
 //     +-MediaControlPlayButtonElement                 (-webkit-media-controls-play-button)
-//     +-MediaControlTimelineElement                   (-webkit-media-controls-timeline)
+//     +-MediaControlTimelineElement                   (-webkit-media-controls-timeline) (if !RTE::newMediaPlaybackUi())

[philipj_slow, 2015/07/09 08:56:10]

Can you use the same style as the above {if
mediaControlsOverlayPlayButtonEnabled}?

[liberato (no reviews please), 2015/07/09 22:35:34]

done.  didn't realize that's what it meant.

[philipj_slow, 2015/07/09 23:18:37]

fs pulled the syntax out of a hat, and I just like consistency :)

 //     +-MediaControlCurrentTimeDisplayElement         (-webkit-media-controls-current-time-display)
 //     +-MediaControlTimeRemainingDisplayElement       (-webkit-media-controls-time-remaining-display)
+//     +-MediaControlTimelineElement                   (-webkit-media-controls-timeline) (if RTE::newMediaPlaybackUi())
 //     +-MediaControlMuteButtonElement                 (-webkit-media-controls-mute-button)
 //     +-MediaControlVolumeSliderElement               (-webkit-media-controls-volume-slider)
 //     +-MediaControlToggleClosedCaptionsButtonElement (-webkit-media-controls-toggle-closed-captions-button)
 //     +-MediaControlCastButtonElement                 (-internal-media-controls-cast-button)
 //     \-MediaControlFullscreenButtonElement           (-webkit-media-controls-fullscreen-button)
 void MediaControls::initializeControls()
 {
     RefPtrWillBeRawPtr<MediaControlOverlayEnclosureElement> overlayEnclosure = MediaControlOverlayEnclosureElement::create(*this);
 
     if (document().settings() && document().settings()->mediaControlsOverlayPlayButtonEnabled()) {
@@ skipping 13 matching lines @@
     RefPtrWillBeRawPtr<MediaControlPanelEnclosureElement> enclosure = MediaControlPanelEnclosureElement::create(*this);
 
     RefPtrWillBeRawPtr<MediaControlPanelElement> panel = MediaControlPanelElement::create(*this);
 
     RefPtrWillBeRawPtr<MediaControlPlayButtonElement> playButton = MediaControlPlayButtonElement::create(*this);
     m_playButton = playButton.get();
     panel->appendChild(playButton.release());
 
     RefPtrWillBeRawPtr<MediaControlTimelineElement> timeline = MediaControlTimelineElement::create(*this);
     m_timeline = timeline.get();
-    panel->appendChild(timeline.release());
+    // In old UX, timeline is before the time / duration text.
+    if (!RuntimeEnabledFeatures::newMediaPlaybackUiEnabled())

[philipj_slow, 2015/07/09 08:56:10]

Break out this into a local variable like in ::reset()

[liberato (no reviews please), 2015/07/09 22:35:34]

Done.

+        panel->appendChild(timeline.release());
+    // else we will attach it later.
 
     RefPtrWillBeRawPtr<MediaControlCurrentTimeDisplayElement> currentTimeDisplay = MediaControlCurrentTimeDisplayElement::create(*this);
     m_currentTimeDisplay = currentTimeDisplay.get();
-    m_currentTimeDisplay->hide();
+    if (!RuntimeEnabledFeatures::newMediaPlaybackUiEnabled())

[philipj_slow, 2015/07/09 08:56:10]

Transform this into a single setIsWanted() call.

[liberato (no reviews please), 2015/07/09 22:35:34]

Done.

+        m_currentTimeDisplay->setIsWanted(false);
+    else
+        m_currentTimeDisplay->setIsWanted(true);
+
     panel->appendChild(currentTimeDisplay.release());
 
     RefPtrWillBeRawPtr<MediaControlTimeRemainingDisplayElement> durationDisplay = MediaControlTimeRemainingDisplayElement::create(*this);
     m_durationDisplay = durationDisplay.get();
     panel->appendChild(durationDisplay.release());
 
+    // In new UX, timeline is after the time / duration text.
+    if (RuntimeEnabledFeatures::newMediaPlaybackUiEnabled())
+        panel->appendChild(timeline.release());
+
     RefPtrWillBeRawPtr<MediaControlMuteButtonElement> muteButton = MediaControlMuteButtonElement::create(*this);
     m_muteButton = muteButton.get();
     panel->appendChild(muteButton.release());
+#if OS(ANDROID)

[philipj_slow, 2015/07/09 08:56:10]

Is this actually needed? Should work the same if done in
mediaControlsAndroidNew.css I think.

[liberato (no reviews please), 2015/07/09 12:10:57]

since we unhide mute to handle the 'media starts muted' bug, i wanted to treat
it the same here.  else MediaContolElement can get out of sync.

+    if (RuntimeEnabledFeatures::newMediaPlaybackUiEnabled())
+        m_muteButton->setIsWanted(false);
+#endif
 
     RefPtrWillBeRawPtr<MediaControlVolumeSliderElement> slider = MediaControlVolumeSliderElement::create(*this);
     m_volumeSlider = slider.get();
     panel->appendChild(slider.release());
+#if OS(ANDROID)
+    if (RuntimeEnabledFeatures::newMediaPlaybackUiEnabled())
+        m_volumeSlider->setIsWanted(false);
+#endif
 
     RefPtrWillBeRawPtr<MediaControlToggleClosedCaptionsButtonElement> toggleClosedCaptionsButton = MediaControlToggleClosedCaptionsButtonElement::create(*this);
     m_toggleClosedCaptionsButton = toggleClosedCaptionsButton.get();
     panel->appendChild(toggleClosedCaptionsButton.release());
 
     RefPtrWillBeRawPtr<MediaControlCastButtonElement> castButton = MediaControlCastButtonElement::create(*this, false);
     m_castButton = castButton.get();
     panel->appendChild(castButton.release());
 
     RefPtrWillBeRawPtr<MediaControlFullscreenButtonElement> fullscreenButton = MediaControlFullscreenButtonElement::create(*this);
     m_fullScreenButton = fullscreenButton.get();
     panel->appendChild(fullscreenButton.release());
 
     m_panel = panel.get();
     enclosure->appendChild(panel.release());
 
     m_enclosure = enclosure.get();
     appendChild(enclosure.release());
 }
 
 void MediaControls::reset()
 {
+    const bool useNewUi = RuntimeEnabledFeatures::newMediaPlaybackUiEnabled();

[philipj_slow, 2015/07/09 08:56:10]

Also make duration const if you prefer that. (If you happen to know if const on
local variable ever makes a difference to the generated code I'd also be curious
to know, seems to me like the compiler should be able to tell that it isn't
modified with or without it.)

[liberato (no reviews please), 2015/07/09 12:10:57]

i've always assumed that it's a note to myself more than to the compiler :)

     double duration = mediaElement().duration();
     m_durationDisplay->setInnerText(LayoutTheme::theme().formatMediaControlsTime(duration), ASSERT_NO_EXCEPTION);
     m_durationDisplay->setCurrentValue(duration);
 
+    if (useNewUi) {
+        // Show everything that we might hide.
+        // If we don't have a duration, then mark it to be hidden.  For the
+        // old UI case, want / don't want is the same as show / hide since
+        // it is never marked as not fitting.
+        if (std::isfinite(duration))
+            m_durationDisplay->setIsWanted(true);
+        else
+            m_durationDisplay->setIsWanted(false);
+        m_currentTimeDisplay->setIsWanted(true);
+        m_timeline->setIsWanted(true);
+    }
+
     updatePlayState();
 
     updateCurrentTimeDisplay();
 
     m_timeline->setDuration(duration);
     m_timeline->setPosition(mediaElement().currentTime());
 
-    if (!mediaElement().hasAudio())
-        m_volumeSlider->hide();
-    else
-        m_volumeSlider->show();
+    if (!mediaElement().hasAudio()) {
+        m_volumeSlider->setIsWanted(false);

[philipj_slow, 2015/07/09 08:56:10]

Ditto.

[liberato (no reviews please), 2015/07/09 22:35:34]

this one always ends up worse with the #ifdef.

[philipj_slow, 2015/07/09 23:18:37]

Oh, here it wasn't actually m_volumeSlider and m_muteButton, not the same, never
mind me.

+        if (useNewUi)
+            m_muteButton->setIsWanted(false);
+    } else {
+#if OS(ANDROID)

[philipj_slow, 2015/07/09 08:56:10]

Should also be possible to do with just CSS.

[liberato (no reviews please), 2015/07/09 22:35:34]

true, but this way it's all in one spot.  otherwise, we have to remember not to
touch volume slider state in the case of android.

[philipj_slow, 2015/07/09 23:18:37]

Oh, so before it was always an option to hide using CSS, and it didn't matter
what you did to the element. Now that doesn't work because of the hide logic
that doesn't look at the computed style...

Still don't want a OS ifdef here. I got rid of the last one by making the
overlay fullscreen button a setting. Would that be an option? Something like
settings()->mediaControlsVolumeSliderEnabled()? Possibly combined with the muted
thing depending on how we decide on that. A setting makes it possible to write
tests that won't have to run on Android too.

[liberato (no reviews please), 2015/07/10 07:20:30]

i've added settings()->preferHidden{VolumeSlider, MuteButton}().  i did it this
way rather than ...Enabled() since the mute button can show up even if it's not
enabled in that case.  this way, they're symmetric.

i've also added a flag to allow this behavior, since the old ui ignores it.  the
test works in both UIs that way.

+        // New UI always hides the volume slider on Android.
+        if (useNewUi)
+            m_volumeSlider->setIsWanted(false);

[philipj_slow, 2015/07/09 08:56:10]

Ditto.

[liberato (no reviews please), 2015/07/09 22:35:33]

Done.

+        else
+            m_volumeSlider->setIsWanted(true);
+#else
+        m_volumeSlider->setIsWanted(true);
+        m_muteButton->setIsWanted(true);
+#endif
+    }
+
     updateVolume();
 
     refreshClosedCaptionsButtonVisibility();
 
     // Unconditionally allow the user to exit fullscreen if we are in it
     // now.  Especially on android, when we might not yet know if
     // fullscreen is supported, we sometimes guess incorrectly and show
     // the button earlier, and we don't want to remove it here if the
     // user chose to enter fullscreen.  crbug.com/500732 .
     if ((mediaElement().hasVideo() && fullscreenIsSupported(document()))
-        || mediaElement().isFullscreen())
-        m_fullScreenButton->show();
-    else
-        m_fullScreenButton->hide();
+        || mediaElement().isFullscreen()) {
+        m_fullScreenButton->setIsWanted(true);

[philipj_slow, 2015/07/09 08:56:10]

Ditto.

[liberato (no reviews please), 2015/07/09 22:35:34]

done.

+    } else {
+        m_fullScreenButton->setIsWanted(false);
+    }
 
-    refreshCastButtonVisibility();
+    refreshCastButtonVisibilityWithoutUpdate();
     makeOpaque();
+    changedControlSelections();
 }
 
 LayoutObject* MediaControls::layoutObjectForTextTrackLayout()
 {
     return m_panel->layoutObject();
 }
 
 void MediaControls::show()
 {
     makeOpaque();
-    m_panel->show();
+    m_panel->setIsWanted(true);
     m_panel->setIsDisplayed(true);
     if (m_overlayPlayButton)
         m_overlayPlayButton->updateDisplayType();
 }
 
 void MediaControls::mediaElementFocused()
 {
     if (mediaElement().shouldShowControls()) {
         show();
         resetHideMediaControlsTimer();
     }
 }
 
 void MediaControls::hide()
 {
-    m_panel->hide();
+    m_panel->setIsWanted(false);
     m_panel->setIsDisplayed(false);
     if (m_overlayPlayButton)
-        m_overlayPlayButton->hide();
+        m_overlayPlayButton->setIsWanted(false);
 }
 
 void MediaControls::makeOpaque()
 {
     m_panel->makeOpaque();
 }
 
 void MediaControls::makeTransparent()
 {
     m_panel->makeTransparent();
@@ skipping 16 matching lines @@
     // controls/shadow tree. (Perform the checks separately to avoid going
     // through all the potential ancestor hosts for the focused element.)
     const bool ignoreFocus = behaviorFlags & IgnoreFocus;
     if (!ignoreFocus && (mediaElement().focused() || contains(document().focusedElement())))
         return false;
     return true;
 }
 
 void MediaControls::playbackStarted()
 {
-    m_currentTimeDisplay->show();
-    m_durationDisplay->hide();
+    if (!RuntimeEnabledFeatures::newMediaPlaybackUiEnabled()) {
+        m_currentTimeDisplay->setIsWanted(true);
+        m_durationDisplay->setIsWanted(false);
+    }
 
     updatePlayState();
     m_timeline->setPosition(mediaElement().currentTime());
     updateCurrentTimeDisplay();
 
     startHideMediaControlsTimer();
 }
 
 void MediaControls::playbackProgressed()
 {
@@ skipping 40 matching lines @@
             mediaElement().togglePlayState();
     }
 }
 
 void MediaControls::updateCurrentTimeDisplay()
 {
     double now = mediaElement().currentTime();
     double duration = mediaElement().duration();
 
     // After seek, hide duration display and show current time.
-    if (now > 0) {
-        m_currentTimeDisplay->show();
-        m_durationDisplay->hide();
+    if (!RuntimeEnabledFeatures::newMediaPlaybackUiEnabled() && now > 0) {
+        m_currentTimeDisplay->setIsWanted(true);
+        m_durationDisplay->setIsWanted(false);
     }
 
     // Allow the theme to format the time.
     m_currentTimeDisplay->setInnerText(LayoutTheme::theme().formatMediaControlsCurrentTime(now, duration), IGNORE_EXCEPTION);
     m_currentTimeDisplay->setCurrentValue(now);
 }
 
 void MediaControls::updateVolume()
 {
     m_muteButton->updateDisplayType();
     // Invalidate the mute button because it paints differently according to volume.
     if (LayoutObject* layoutObject = m_muteButton->layoutObject())
         layoutObject->setShouldDoFullPaintInvalidation();
 
-    if (mediaElement().muted())
+    if (mediaElement().muted()) {
         m_volumeSlider->setVolume(0);
-    else
+        // If the element is muted, then we want the unmute button.  Otherwise,

[philipj_slow, 2015/07/09 08:56:11]

I think that if we're going to change anything about this again, we should just
unmute and set volume to 1 when playing using the built-in controls. We don't
show the volume slider even though volume can be set to 0, so the current logic
seems strange to me, and a waste of pixels in the vast majority of cases.

Alternatively, show the mute button only for videos that are actually muted, but
of course leave it in after that point as well.

[liberato (no reviews please), 2015/07/09 22:35:34]

the latter one seems almost exactly what the code does.  if we have audio and
we're muted, then add the mute button.  it's also very similar to what the
original patch for crbug.com/467242 did, which seemed to fix it.  so, i suspect
that the problems were about the mute tag rather than volume==0.

i agree that doing something with the volume is a good idea, but not sure what.

i'm not sure that i understand the first suggestion -- unmuting automatically
seems like it might not go so well.

[philipj_slow, 2015/07/09 23:18:36]

FWIW, we unmute when pressing the overlay fullscreen button for the first time
in Opera for Android as a way to remove the mute button, but somewhere where's
possible/easy to play while muted would be nicer.

I actually wasn't paying attention to the surrounding code here, I just assumed
this was preserving the existing behavior. Not so. Can you write some tests for
this (easier with a setting) to verify the conditions under which the button
should be shown and not? A problem here is that ::reset() also fiddles with
this, so I think the logic for when it's shown should be in one place.

crbug.com/467242 doesn't look like the right bug.

[liberato (no reviews please), 2015/07/14 22:10:36]

added LayoutTests/media/video-controls-hidden-audio.html .

+        // one can't unmute it.  This makes a difference on android, where we
+        // hide the mute button by default.  crbug.com/467252 .
+        if (mediaElement().hasAudio()) {
+            m_muteButton->setIsWanted(true);
+        }
+    } else {
         m_volumeSlider->setVolume(mediaElement().volume());
+    }
+
     // Invalidate the volume slider because it paints differently according to volume.
     if (LayoutObject* layoutObject = m_volumeSlider->layoutObject())
         layoutObject->setShouldDoFullPaintInvalidation();
 }
 
 void MediaControls::changedClosedCaptionsVisibility()
 {
     m_toggleClosedCaptionsButton->updateDisplayType();
 }
 
 void MediaControls::refreshClosedCaptionsButtonVisibility()
 {
     if (mediaElement().hasClosedCaptions())
-        m_toggleClosedCaptionsButton->show();
+        m_toggleClosedCaptionsButton->setIsWanted(true);

[philipj_slow, 2015/07/09 08:56:11]

Ditto.

[liberato (no reviews please), 2015/07/09 22:35:34]

Done.

     else
-        m_toggleClosedCaptionsButton->hide();
+        m_toggleClosedCaptionsButton->setIsWanted(false);
 }
 
 static Element* elementFromCenter(Element& element)
 {
     ClientRect* clientRect = element.getBoundingClientRect();
     int centerX = static_cast<int>((clientRect->left() + clientRect->right()) / 2);
     int centerY = static_cast<int>((clientRect->top() + clientRect->bottom()) / 2);
 
     return element.document().elementFromPoint(centerX , centerY);
 }
 
 void MediaControls::tryShowOverlayCastButton()
 {
     // The element needs to be shown to have its dimensions and position.
-    m_overlayCastButton->show();
+    m_overlayCastButton->setIsWanted(true);
 
     if (elementFromCenter(*m_overlayCastButton) != &mediaElement())
-        m_overlayCastButton->hide();
+        m_overlayCastButton->setIsWanted(false);
 }
 
 void MediaControls::refreshCastButtonVisibility()
 {
+    refreshCastButtonVisibilityWithoutUpdate();
+    changedControlSelections();
+}
+
+void MediaControls::hideCastButtons()
+{
+    m_castButton->setIsWanted(false);
+    m_overlayCastButton->setIsWanted(false);
+}
+
+void MediaControls::refreshCastButtonVisibilityWithoutUpdate()
+{
     if (mediaElement().hasRemoteRoutes()) {
         // The reason for the autoplay test is that some pages (e.g. vimeo.com) have an autoplay background video, which
         // doesn't autoplay on Chrome for Android (we prevent it) so starts paused. In such cases we don't want to automatically
         // show the cast button, since it looks strange and is unlikely to correspond with anything the user wants to do.
         // If a user does want to cast a paused autoplay video then they can still do so by touching or clicking on the
         // video, which will cause the cast button to appear.
         if (!mediaElement().shouldShowControls() && !mediaElement().autoplay() && mediaElement().paused()) {
+            // Note that this is a case where we add the overlay cast button
+            // without wanting the bar cast button.  We depend on the fact

[philipj_slow, 2015/07/09 08:56:10]

s/bar/panel/ or "the button in the controls panel"

I'm also not sure what the "We depend on ..." bit is saying,
updateControlsVisibilityForSpace() isn't called here.

[liberato (no reviews please), 2015/07/09 22:35:33]

expanded comment.

+            // that updateControls...() won't change overlay cast button
+            // visibility in the case where the cast button isn't wanted.
             showOverlayCastButton();
+            m_castButton->setIsWanted(false);

[philipj_slow, 2015/07/09 08:56:10]

The mismatch between showOverlayCastButton() and
m_castButton->setIsWanted(false) looks odd. Any idea why we can't just call
m_overlayCastButton->setIsWanted(true) here? It seems strange to reset the hide
timer in a function like this that one should expect to be called to potentially
invalidate state, but have no side effect if everything is already in the
correct state.

[liberato (no reviews please), 2015/07/09 22:35:34]

yeah, i agree.  changed.

[liberato (no reviews please), 2015/07/14 22:10:36]

it turns out that the right answer is tryShowOverlayCast, else it doesn't notice
if there is an element over the player.  that should cause cast not to show up. 
luckily, it also failed a test, which is the only reason i realized the
difference.

         } else if (mediaElement().shouldShowControls()) {
-            m_overlayCastButton->hide();
-            m_castButton->show();
-            // Check that the cast button actually fits on the bar.
-            if (m_fullScreenButton->getBoundingClientRect()->right() > m_panel->getBoundingClientRect()->right()) {
-                m_castButton->hide();
+            m_overlayCastButton->setIsWanted(false);
+            m_castButton->setIsWanted(true);
+            // Check that the cast button actually fits on the bar.  For the
+            // new ui, we let changedControlSelections() handle this.
+            if ( !RuntimeEnabledFeatures::newMediaPlaybackUiEnabled()
+                && m_fullScreenButton->getBoundingClientRect()->right() > m_panel->getBoundingClientRect()->right()) {
+                m_castButton->setIsWanted(false);
                 tryShowOverlayCastButton();
             }
         }
     } else {
-        m_castButton->hide();
-        m_overlayCastButton->hide();
+        m_castButton->setIsWanted(false);
+        m_overlayCastButton->setIsWanted(false);
     }
+
+    // do NOT updateControls...() here.

[philipj_slow, 2015/07/09 08:56:11]

This comment isn't needed IMHO.

[liberato (no reviews please), 2015/07/09 22:35:34]

Done.

 }
 
 void MediaControls::showOverlayCastButton()
 {
     tryShowOverlayCastButton();
     resetHideMediaControlsTimer();
 }
 
 void MediaControls::enteredFullscreen()
 {
@@ skipping 69 matching lines @@
     unsigned behaviorFlags = m_hideTimerBehaviorFlags | IgnoreFocus | IgnoreVideoHover;
     m_hideTimerBehaviorFlags = IgnoreNone;
 
     if (mediaElement().togglePlayStateWillPlay())
         return;
 
     if (!shouldHideMediaControls(behaviorFlags))
         return;
 
     makeTransparent();
-    m_overlayCastButton->hide();
+    m_overlayCastButton->setIsWanted(false);
 }
 
 void MediaControls::startHideMediaControlsTimer()
 {
     m_hideMediaControlsTimer.startOneShot(timeWithoutMouseMovementBeforeHidingMediaControls, FROM_HERE);
 }
 
 void MediaControls::stopHideMediaControlsTimer()
 {
     m_hideMediaControlsTimer.stop();
@@ skipping 10 matching lines @@
 bool MediaControls::containsRelatedTarget(Event* event)
 {
     if (!event->isMouseEvent())
         return false;
     EventTarget* relatedTarget = toMouseEvent(event)->relatedTarget();
     if (!relatedTarget)
         return false;
     return contains(relatedTarget->toNode());
 }
 
+void MediaControls::notifyPanelWidthChanged(int panelWidth)
+{
+    // Don't bother to do any work if this matches the most recent panel
+    // width, since we're called after layout.
+    if (panelWidth == 0 || panelWidth == m_panelWidth)

[philipj_slow, 2015/07/09 08:56:10]

Shouldn't m_panelWidth still be updated? Seems simpler to just always have it in
sync with layout?

[fs, 2015/07/09 09:31:17]

Yeah, if checking this at all, it's probably better to do it just before the
actual controls "layout".

[liberato (no reviews please), 2015/07/09 12:10:56]

i can remove the check here safely, but i can't add it to
checkWhichControlsFit().  i'll remove.

+        return;
+
+    m_panelWidth = panelWidth;
+    m_panelWidthChangedTimer.startOneShot(0, FROM_HERE);

[philipj_slow, 2015/07/09 08:56:10]

Given that this is called from LayoutMedia::layout() I take it the reason for
this is to avoid some asserts about style invalidation during layout? Still, it
seems like doing it on a timer could cause one bad frame to be rendered. fs?

My hunch is that the only way to really make this work might be to move the
logic to layout somehow. It's true that JS controls couldn't do that, but if the
controls were in an iframe and listened to the resize event, I presume that
updating the layout in the resize event would not cause one bad frame. At least
I hope there's actually a way for web content to do this kind of thing
correctly, otherwise our web platform sucks! :)

[fs, 2015/07/09 09:31:17]

Correct on both accounts AFAIK.
Agreed.
Our web platform sucks =). AFAIK, 'resize' is detected after layout and the
event dispatched from the rAF callback (ScriptedAnimationController).
Technically that's an "implementation detail" though of course - i.e. nothing
preventing layout -> resize -> re-layout -> ...

[liberato (no reviews please), 2015/07/09 12:10:57]

to be clear: i'm about to go spelunking into layout code.  is this what you both
intend for me to do?

[fs, 2015/07/09 12:25:39]

Well, it's essentially a QoI issue - there could (will) be a single-frame glitch
when the "what fits?" algorithm decides to hide/unhide a control. Depending on
various factors (width-delta, padding, etc.) it'll end up looking less pretty
than it maybe could. I'm pretty sure that you - having seen this is action -
have a better feel for how "glitchy" it actually appears in practice though, so
as far as I'm concerned I'd leave it up to you to decided. philipj on the other
hand is perhaps a bit more adamant than I am ;-)

[philipj_slow, 2015/07/10 14:51:58]

So let's add a TODO that acknowledges that there's a problem here, in cast
someone comes along and sees this code and either gets suspicious or wants to do
something similar elsewhere.

[liberato (no reviews please), 2015/07/14 22:10:36]

Done.

+}
+
+void MediaControls::changedControlSelections()
+{
+    int panelWidth = m_panel->clientWidth();

[philipj_slow, 2015/07/09 08:56:11]

This may trigger a layout, is that intentional? It seems odd to both have
LayoutMedia notify about the panel width and to get it like this.

[liberato (no reviews please), 2015/07/09 22:35:34]

i expected that it would avoid a bad frame, but truthfully i never tested that.

+    // Don't short-circuit update if the panel hasn't changed.  The fit state
+    // is a function of the controls we want, so recompute if we can.
+    if (panelWidth == 0)
+        return;
+
+    m_panelWidth = panelWidth;
+    computeWhichControlsFit(panelWidth);
+}
+
+void MediaControls::panelWidthChangedTimerFired(Timer<MediaControls>*)
+{
+    if (!m_panelWidth)
+        return;
+
+    computeWhichControlsFit(m_panelWidth);
+}
+
+void MediaControls::computeWhichControlsFit(int panelWidth)

[philipj_slow, 2015/07/09 08:56:10]

Seems like this doesn't need an argument, use m_panelWidth?

[liberato (no reviews please), 2015/07/09 22:35:33]

yeah, i had the same thought.  i did it this way so that one doesn't
accidentally call computeWhichControlsFit() when one really wants to get the
most current width first.

however, i'll try removing all of changedControlSelections(), and just get the
width via notifyPanelWidthChanged(), then it'll be much simpler.  hopefully that
works.

i'm just not convinced yet that it won't introduce an extra bad frame.

[liberato (no reviews please), 2015/07/14 22:10:36]

this does cause an extra bad frame, so i've left it as-is.

+{
+    // Hide all controls that don't fit, and show the ones that do.
+    // This might be better suited for a layout, but since JS media controls
+    // won't benefit from that anwyay, we just do it here like JS will.
+    // The order, in order of decreasing droppiness:
+    // Volume, time, seek bar, cast.
+
+    // Controls that we'll hide / show, in order of decreasing priority.
+    MediaControlElement* elements[] = {
+        m_playButton.get(),
+        m_toggleClosedCaptionsButton.get(),
+        m_fullScreenButton.get(),
+        m_timeline.get(),
+        m_currentTimeDisplay.get(),
+        m_volumeSlider.get(),
+        m_castButton.get(),
+        m_muteButton.get(),
+        m_durationDisplay.get(),
+        0
+    };
+
+    int usedWidth = 0;
+    for (int i = 0; elements[i]; i++)  {

[philipj_slow, 2015/07/09 08:56:10]

for (MediaControlElement* element : elements) at least compiles, and I assume
that you could get rid of the trailing 0 in the array using it.

That would also make it explicit that none of the elements can be null, as that
would just silently terminate the loop with the current code. There are elements
that can be null, like m_overlayPlayButton, so it's not really obvious.

[liberato (no reviews please), 2015/07/09 22:35:34]

done, cool.

+        if (elements[i]->isWanted()) {
+            if (usedWidth + elements[i]->minimumWidth() <= panelWidth) {
+                elements[i]->setDoesFit(true);
+                usedWidth += elements[i]->minimumWidth();
+            } else {
+                elements[i]->setDoesFit(false);
+            }
+        }
+    }
+
+    // Special case for cast: if we want a cast button but dropped it, then
+    // show the overlay cast button instead.
+    if (m_castButton->isWanted()) {
+        if (!m_castButton->isShown())

[philipj_slow, 2015/07/09 08:56:10]

I think this and above are the only places where isWanted() and isShown() are
used, and it looks like it could be replaced by just remembering some state from
the loop. It would be nice if the price of ugliness for the cast button hack
were localized and we don't have exposed state of the buttons that invites more
special casing in other contexts.

Also merge into a single setIsWanted() call.

[liberato (no reviews please), 2015/07/09 22:35:33]

i've removed isShown(), but i don't see how to remove isWanted. it's a
consequence of moving the state to the MediaElement.

[philipj_slow, 2015/07/09 23:18:37]

Uh right, the loop above is also using isWanted(), for good reasons, I searched
badly.

+            m_overlayCastButton->setIsWanted(true);
+        else
+            m_overlayCastButton->setIsWanted(false);
+    } // else do not change overlay cast button state.

[philipj_slow, 2015/07/09 08:56:11]

Don't need this comment.

[liberato (no reviews please), 2015/07/09 22:35:34]

Done.

+}
+
 DEFINE_TRACE(MediaControls)
 {
     visitor->trace(m_mediaElement);
     visitor->trace(m_panel);
     visitor->trace(m_overlayPlayButton);
     visitor->trace(m_overlayEnclosure);
     visitor->trace(m_playButton);
     visitor->trace(m_currentTimeDisplay);
     visitor->trace(m_timeline);
     visitor->trace(m_muteButton);
     visitor->trace(m_volumeSlider);
     visitor->trace(m_toggleClosedCaptionsButton);
     visitor->trace(m_fullScreenButton);
     visitor->trace(m_durationDisplay);
     visitor->trace(m_enclosure);
     visitor->trace(m_castButton);
     visitor->trace(m_overlayCastButton);
     HTMLDivElement::trace(visitor);
 }
 
 }