Remove media controls when not in use.

Source/core/html/HTMLMediaElement.cpp

Index: Source/core/html/HTMLMediaElement.cpp
diff --git a/Source/core/html/HTMLMediaElement.cpp b/Source/core/html/HTMLMediaElement.cpp
index 35f8cd734ac77d7fa55aba347ec080f1add8dda1..635ccc6c2502ef121e0931b03833bf43618478bf 100644
--- a/Source/core/html/HTMLMediaElement.cpp
+++ b/Source/core/html/HTMLMediaElement.cpp
@@ -410,7 +410,7 @@ bool HTMLMediaElement::supportsFocus() const
         return false;
 
     // If no controls specified, we should still be able to focus the element if it has tabIndex.
-    return controls() || HTMLElement::supportsFocus();
+    return shouldDisplayControls() || HTMLElement::supportsFocus();
 }
 
 bool HTMLMediaElement::isMouseFocusable() const
@@ -468,7 +468,7 @@ void HTMLMediaElement::finishParsingChildren()
 
 bool HTMLMediaElement::rendererIsNeeded(const RenderStyle& style)
 {
-    return controls() ? HTMLElement::rendererIsNeeded(style) : false;
+    return shouldDisplayControls() ? HTMLElement::rendererIsNeeded(style) : false;
 }
 
 RenderObject* HTMLMediaElement::createRenderer(RenderStyle*)
@@ -2297,6 +2297,11 @@ void HTMLMediaElement::setLoop(bool b)
 
 bool HTMLMediaElement::controls() const
 {
+    return fastHasAttribute(controlsAttr);
+}
+
+bool HTMLMediaElement::shouldDisplayControls() const
+{
     LocalFrame* frame = document().frame();
 
     // always show controls when scripting is disabled
@@ -2307,7 +2312,7 @@ bool HTMLMediaElement::controls() const
     if (isFullscreen())
         return true;
 
-    return fastHasAttribute(controlsAttr);
+    return controls();
 }
 
 void HTMLMediaElement::setControls(bool b)
@@ -3586,7 +3591,7 @@ bool HTMLMediaElement::createMediaControls()
 
     ensureUserAgentShadowRoot().appendChild(mediaControls);
 
-    if (!controls() || !inDocument())
+    if (!shouldDisplayControls() || !inDocument())
         mediaControls->hide();
 
     return true;
@@ -3594,9 +3599,13 @@ bool HTMLMediaElement::createMediaControls()
 
 void HTMLMediaElement::configureMediaControls()
 {
-    if (!controls() || !inDocument()) {
-        if (hasMediaControls())
-            mediaControls()->hide();
+    if (!shouldDisplayControls() || !inDocument()) {
+        if (hasMediaControls()) {
+            if (!shouldDisplayControls())
+                userAgentShadowRoot()->removeChild(mediaControls());

[acolwell GONE FROM CHROMIUM, 2014/03/05 00:09:50]

This still doesn't seem right to me. These controls could get recreated by
createMediaControls() in various ways. Why doesn't the MediaControlsAndroid,
just overload show() & hide() to make sure the button doesn't get displayed at
the wrong time?

[Fredrik Öhrn, 2014/03/06 10:20:16]

I can probably patch something together using that method but I think it's ugly.

What I wanted to do is get away from the confusion between the controls being
enabled/disabled (i.e. the controls attribute is true or false) and the controls
being enabled but visible or hidden due to other state like the element having
focus or not, faded out out when video is playing etc.

Especially since since these "temporary" visibility changes are driven from two
directions, both HTMLMediaElement and MediaControlsAndroid, and MediaControls
and it's descendants don't have the full picture.

I think destroying the controls when they are not needed is the most obvious
solution, however if that is deemed too heavy handed, I think it would be better
to give the MediaControls class access to call
HTMLMediaElement::shouldDisplayControls() so they can query the the
enabled/disabled state and avoid showing themselves when not wanted.

[acolwell GONE FROM CHROMIUM, 2014/03/06 16:30:56]

I guess, I'm confused as to why this is ugly.
But hide() and show() are how the HTMLMediaElement is signalling whether the
controls should be displayed or not. It sounds like the Android controls are not
paying attention to these signals. It seems to me that it is the responsibility
of HTMLMediaElement to properly call hide/show() when shouldDisplayControls()
changes state. If that is not happening, then we should fix that first.

It is also still not clear to me that what you have here is the right solution
because all the application has to do is add a TextTrack cue and I believe the
controls will get recreated and hidden by createMediaControls(). Why doesn't
createMediaControls() have to have the same logic as this method? It looks like
the controls can just blindly get recreated w/o checking shouldDisplayControls()
so I think we are still missing something here.

+            else
+                mediaControls()->hide();
+        }
         return;
     }