Media controls should fade out after a while regardless of mouse position

Media controls should fade out after a while regardless of mouse position

Media controls should fade out after 15sec regardless of the mouse position. Incase of fullscreen controls the above mentioned behavior is already implemented. 
The below patch implements same behavior for inline media controls.

BUG=151691
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170953

[Srirama, 2014-03-26 06:14:46]

Hi All,

Please take a look at the patch and provide your valuable feedback.

Regards,
Srirama

[nessy, 2014-03-26 08:58:00]

Why duplicate all the logic when you need those times for everything? Just
remove their restriction to fullscreen and remove the fullscreen part of the
function names.

[Srirama, 2014-03-26 09:19:38]

On 2014/03/26 08:58:00, nessy wrote:
> Why duplicate all the logic when you need those times for everything? Just
> remove their restriction to fullscreen and remove the fullscreen part of the
> function names.

Hi, Nessy,

Thank you for the quick reply. Your point is valid, but I have done it that way
because of two reasons.

1) For fullscreen controls, timeout is 3sec where as for inline controls
requirement is 15sec (can i make it 3sec for inline controls as well or have
same timer function with different timeout, something like
if (fullscreen)
    startHideMediaControlsTimer(3);
else
    startHideMediaControlsTimer(15);

2) Recently there is a change for merging MediaControlsAndroid to MediaControls
https://codereview.chromium.org/192453005.
As part of this for android port, it was forcefully made fullscreen controls to
hide always. I felt this behavior is not applicable for inline controls so i
just separated the logic for fullscreen and inline controls. If we can safely
force same behavior for fullscreen and inline for android port then i can go
ahead and do the changes as suggested by you.

Please let me know your opinion on the above two points so that I can proceed
further, and kindly guide me if I have to write any layout tests or unit tests
for these changes.

[philipj_slow, 2014-03-26 12:01:40]

On 2014/03/26 09:19:38, Srirama wrote:

> 2) Recently there is a change for merging MediaControlsAndroid to
MediaControls
> https://codereview.chromium.org/192453005.
> As part of this for android port, it was forcefully made fullscreen controls
to
> hide always. I felt this behavior is not applicable for inline controls so i
> just separated the logic for fullscreen and inline controls. If we can safely
> force same behavior for fullscreen and inline for android port then i can go
> ahead and do the changes as suggested by you.

This wasn't the intention, that rewrite tried to not change any behavior at all.
Before MediaControlsAndroid had "virtual bool shouldHideControls() OVERRIDE {
return true; }" and after alwaysHideFullscreenControls makes
MediaControls::shouldHideFullscreenControls() always return true on Android.
This seems exactly equivalent, am I missing something?

Like nessy I think we could make this work with a single timeout to hide the
controls. Something like what you suggested in your previous comments seems
reasonable.

It think that just removing the alwaysHideFullscreenControls setting could make
things simpler for you as well, I haven't tried it yet but I suspect that
doesn't actually matter on Android because the video will be shown with native
controls on top of everything anyway.

Finally, hiding the controls when the mouse is over the video but not moving
seems OK when not in fullscreen as well, if it might simplify matters.

[Srirama, 2014-03-26 12:18:24]

On 2014/03/26 12:01:40, philipj wrote:
> On 2014/03/26 09:19:38, Srirama wrote:
> 
> > 2) Recently there is a change for merging MediaControlsAndroid to
> MediaControls
> > https://codereview.chromium.org/192453005.
> > As part of this for android port, it was forcefully made fullscreen controls
> to
> > hide always. I felt this behavior is not applicable for inline controls so i
> > just separated the logic for fullscreen and inline controls. If we can
safely
> > force same behavior for fullscreen and inline for android port then i can go
> > ahead and do the changes as suggested by you.
> 
> This wasn't the intention, that rewrite tried to not change any behavior at
all.
> Before MediaControlsAndroid had "virtual bool shouldHideControls() OVERRIDE {
> return true; }" and after alwaysHideFullscreenControls makes
> MediaControls::shouldHideFullscreenControls() always return true on Android.
> This seems exactly equivalent, am I missing something?
> 
> Like nessy I think we could make this work with a single timeout to hide the
> controls. Something like what you suggested in your previous comments seems
> reasonable.
> 
> It think that just removing the alwaysHideFullscreenControls setting could
make
> things simpler for you as well, I haven't tried it yet but I suspect that
> doesn't actually matter on Android because the video will be shown with native
> controls on top of everything anyway.
> 
> Finally, hiding the controls when the mouse is over the video but not moving
> seems OK when not in fullscreen as well, if it might simplify matters.

Thanks Philip, as I understand by your clarification, i will go ahead and use 
single timer function with 3sec delay for both inline and fullscreen as waiting 
15sec inline mode before fading out doesn't look good. Also i will remove the 
android setting alwaysHideFullscreenControls as well to make things simpler and 
remove unnecessary ifdef for that setting.

[Srirama, 2014-03-26 14:11:26]

Hi,

Please review the new set of changes.
I have removed duplicated code and simplified the code as suggested.
Also removed m_isFullscreen variable as it is not required now.

[Srirama, 2014-03-27 13:32:16]

ping ?

[Srirama, 2014-04-02 11:55:38]

Adding acolwell for review.

[Srirama, 2014-04-04 07:21:15]

PTAL

[acolwell GONE FROM CHROMIUM, 2014-04-04 19:37:06]

lgtm

[Srirama, 2014-04-07 05:17:35]

The CQ bit was checked by srirama.m@samsung.com

[commit-bot: I haz the power, 2014-04-07 05:17:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/srirama.m@samsung.com/212173002/90001

[commit-bot: I haz the power, 2014-04-07 05:18:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-07 05:18:05]

Failed to apply patch for Source/core/html/shadow/MediaControls.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/html/shadow/MediaControls.cpp
  Hunk #1 succeeded at 36 (offset -1 lines).
  Hunk #2 succeeded at 59 (offset -1 lines).
  Hunk #3 succeeded at 212 (offset -9 lines).
  Hunk #4 succeeded at 226 with fuzz 1 (offset -9 lines).
  Hunk #5 succeeded at 245 (offset -9 lines).
  Hunk #6 succeeded at 323 (offset -14 lines).
  Hunk #7 succeeded at 344 with fuzz 2 (offset -14 lines).
  Hunk #8 FAILED at 368.
  1 out of 8 hunks FAILED -- saving rejects to file
Source/core/html/shadow/MediaControls.cpp.rej

Patch:       Source/core/html/shadow/MediaControls.cpp
Index: Source/core/html/shadow/MediaControls.cpp
diff --git a/Source/core/html/shadow/MediaControls.cpp
b/Source/core/html/shadow/MediaControls.cpp
index
17efb600f9bd20ab826198f3ad3c1282c1341d4f..4b3d4b4db0f75fca3b15de5f3f7cbee226ca8a06
100644
--- a/Source/core/html/shadow/MediaControls.cpp
+++ b/Source/core/html/shadow/MediaControls.cpp
@@ -37,14 +37,12 @@
 namespace WebCore {
 
 #if OS(ANDROID)
-static const bool alwaysHideFullscreenControls = true;
 static const bool needOverlayPlayButton = true;
 #else
-static const bool alwaysHideFullscreenControls = false;
 static const bool needOverlayPlayButton = false;
 #endif
 
-static const double timeWithoutMouseMovementBeforeHidingFullscreenControls = 3;
+static const double timeWithoutMouseMovementBeforeHidingMediaControls = 3;
 
 MediaControls::MediaControls(HTMLMediaElement& mediaElement)
     : HTMLDivElement(mediaElement.document())
@@ -62,8 +60,7 @@ MediaControls::MediaControls(HTMLMediaElement& mediaElement)
     , m_fullScreenButton(0)
     , m_durationDisplay(0)
     , m_enclosure(0)
-    , m_hideFullscreenControlsTimer(this,
&MediaControls::hideFullscreenControlsTimerFired)
-    , m_isFullscreen(false)
+    , m_hideMediaControlsTimer(this,
&MediaControls::hideMediaControlsTimerFired)
     , m_isMouseOverControls(false)
     , m_isPausedForScrubbing(false)
 {
@@ -224,9 +221,9 @@ void MediaControls::makeTransparent()
     m_panel->makeTransparent();
 }
 
-bool MediaControls::shouldHideFullscreenControls()
+bool MediaControls::shouldHideMediaControls()
 {
-    return alwaysHideFullscreenControls || !m_panel->hovered();
+    return !m_panel->hovered();
 }
 
 void MediaControls::playbackStarted()
@@ -238,8 +235,7 @@ void MediaControls::playbackStarted()
     m_timeline->setPosition(mediaControllerInterface().currentTime());
     updateCurrentTimeDisplay();
 
-    if (m_isFullscreen)
-        startHideFullscreenControlsTimer();
+    startHideMediaControlsTimer();
 }
 
 void MediaControls::playbackProgressed()
@@ -258,7 +254,7 @@ void MediaControls::playbackStopped()
     updateCurrentTimeDisplay();
     makeOpaque();
 
-    stopHideFullscreenControlsTimer();
+    stopHideMediaControlsTimer();
 }
 
 void MediaControls::updatePlayState()
@@ -341,16 +337,16 @@ void MediaControls::closedCaptionTracksChanged()
 
 void MediaControls::enteredFullscreen()
 {
-    m_isFullscreen = true;
     m_fullScreenButton->setIsFullscreen(true);
-    startHideFullscreenControlsTimer();
+    stopHideMediaControlsTimer();
+    startHideMediaControlsTimer();
 }
 
 void MediaControls::exitedFullscreen()
 {
-    m_isFullscreen = false;
     m_fullScreenButton->setIsFullscreen(false);
-    stopHideFullscreenControlsTimer();
+    stopHideMediaControlsTimer();
+    startHideMediaControlsTimer();
 }
 
 void MediaControls::defaultEventHandler(Event* event)
@@ -362,8 +358,8 @@ void MediaControls::defaultEventHandler(Event* event)
             m_isMouseOverControls = true;
             if (!mediaControllerInterface().canPlay()) {
                 makeOpaque();
-                if (shouldHideFullscreenControls())
-                    startHideFullscreenControlsTimer();
+                if (shouldHideMediaControls())
+                    startHideMediaControlsTimer();
             }
         }
         return;
@@ -372,48 +368,40 @@ void MediaControls::defaultEventHandler(Event* event)
     if (event->type() == EventTypeNames::mouseout) {
         if (!containsRelatedTarget(event)) {
             m_isMouseOverControls = false;
-            stopHideFullscreenControlsTimer();
+            stopHideMediaControlsTimer();
         }
         return;
     }
 
     if (event->type() == EventTypeNames::mousemove) {
-        if (m_isFullscreen) {
-            // When we get a mouse move in fullscreen mode, show the media
controls, and start a timer
-            // that will hide the media controls after a 3 seconds without a
mouse move.
-            makeOpaque();
-            if (shouldHideFullscreenControls())
-                startHideFullscreenControlsTimer();
-        }
+        // When we get a mouse move, show the media controls, and start a timer
+        // that will hide the media controls after a 3 seconds without a mouse
move.
+        makeOpaque();
+        if (shouldHideMediaControls())
+            startHideMediaControlsTimer();
         return;
     }
 }
 
-void MediaControls::hideFullscreenControlsTimerFired(Timer<MediaControls>*)
+void MediaControls::hideMediaControlsTimerFired(Timer<MediaControls>*)
 {
     if (mediaControllerInterface().paused())
         return;
 
-    if (!m_isFullscreen)
-        return;
-
-    if (!shouldHideFullscreenControls())
+    if (!shouldHideMediaControls())
         return;
 
     makeTransparent();
 }
 
-void MediaControls::startHideFullscreenControlsTimer()
+void MediaControls::startHideMediaControlsTimer()
 {
-    if (!m_isFullscreen)
-        return;
-
-   
m_hideFullscreenControlsTimer.startOneShot(timeWithoutMouseMovementBeforeHidingFullscreenControls,
FROM_HERE);
+   
m_hideMediaControlsTimer.startOneShot(timeWithoutMouseMovementBeforeHidingMediaControls,
FROM_HERE);
 }
 
-void MediaControls::stopHideFullscreenControlsTimer()
+void MediaControls::stopHideMediaControlsTimer()
 {
-    m_hideFullscreenControlsTimer.stop();
+    m_hideMediaControlsTimer.stop();
 }
 
 const AtomicString& MediaControls::shadowPseudoId() const

[Srirama, 2014-04-07 05:30:42]

The CQ bit was checked by srirama.m@samsung.com

[commit-bot: I haz the power, 2014-04-07 05:30:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/srirama.m@samsung.com/212173002/160001

[commit-bot: I haz the power, 2014-04-07 06:06:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-07 06:06:10]

Try jobs failed on following builders:
  tryserver.blink on win_blink_compile_dbg

[Srirama, 2014-04-07 07:00:29]

The CQ bit was checked by srirama.m@samsung.com

[commit-bot: I haz the power, 2014-04-07 07:00:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/srirama.m@samsung.com/212173002/160001

[commit-bot: I haz the power, 2014-04-07 07:25:23]

Change committed as 170953