Remove media controls when not in use.

Source/core/html/HTMLMediaElement.cpp

 /*
  * Copyright (C) 2007, 2008, 2009, 2010, 2011, 2012, 2013 Apple 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
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 392 matching lines @@
 {
     return true;
 }
 
 bool HTMLMediaElement::supportsFocus() const
 {
     if (ownerDocument()->isMediaDocument())
         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
 {
     return false;
 }
 
 void HTMLMediaElement::parseAttribute(const QualifiedName& name, const AtomicString& value)
 {
     if (name == srcAttr) {
@@ skipping 37 matching lines @@
     for (Element* child = ElementTraversal::firstWithin(*this); child; child = ElementTraversal::nextSibling(*child)) {
         if (child->hasTagName(trackTag)) {
             scheduleDelayedAction(LoadTextTrackResource);
             break;
         }
     }
 }
 
 bool HTMLMediaElement::rendererIsNeeded(const RenderStyle& style)
 {
-    return controls() ? HTMLElement::rendererIsNeeded(style) : false;
+    return shouldDisplayControls() ? HTMLElement::rendererIsNeeded(style) : false;
 }
 
 RenderObject* HTMLMediaElement::createRenderer(RenderStyle*)
 {
     return new RenderMedia(this);
 }
 
 Node::InsertionNotificationRequest HTMLMediaElement::insertedInto(ContainerNode* insertionPoint)
 {
     WTF_LOG(Media, "HTMLMediaElement::insertedInto");
@@ skipping 1808 matching lines @@
 }
 
 void HTMLMediaElement::setLoop(bool b)
 {
     WTF_LOG(Media, "HTMLMediaElement::setLoop(%s)", boolString(b));
     setBooleanAttribute(loopAttr, b);
 }
 
 bool HTMLMediaElement::controls() const
 {
+    return fastHasAttribute(controlsAttr);
+}
+
+bool HTMLMediaElement::shouldDisplayControls() const
+{
     LocalFrame* frame = document().frame();
 
     // always show controls when scripting is disabled
     if (frame && !frame->script().canExecuteScripts(NotAboutToExecuteScript))
         return true;
 
     // Always show controls when in full screen mode.
     if (isFullscreen())
         return true;
 
-    return fastHasAttribute(controlsAttr);
+    return controls();
 }
 
 void HTMLMediaElement::setControls(bool b)
 {
     WTF_LOG(Media, "HTMLMediaElement::setControls(%s)", boolString(b));
     setBooleanAttribute(controlsAttr, b);
 }
 
 double HTMLMediaElement::volume() const
 {
@@ skipping 1258 matching lines @@
     if (!mediaControls)
         return false;
 
     mediaControls->setMediaController(m_mediaController ? m_mediaController.get() : static_cast<MediaControllerInterface*>(this));
     mediaControls->reset();
     if (isFullscreen())
         mediaControls->enteredFullscreen();
 
     ensureUserAgentShadowRoot().appendChild(mediaControls);
 
-    if (!controls() || !inDocument())
+    if (!shouldDisplayControls() || !inDocument())
         mediaControls->hide();
 
     return true;
 }
 
 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;
     }
 
     if (!hasMediaControls() && !createMediaControls())
         return;
 
     mediaControls()->show();
 }
 
 void HTMLMediaElement::configureTextTrackDisplay(VisibilityChangeAssumption assumption)
@@ skipping 288 matching lines @@
 {
     m_mediaSource->setWebMediaSourceAndOpen(adoptPtr(webMediaSource));
 }
 
 bool HTMLMediaElement::isInteractiveContent() const
 {
     return fastHasAttribute(controlsAttr);
 }
 
 }