Setting HTMLMediaElement.controller does not properly remove the 'mediagroup' content attribute

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 8402ac95f43129d136bc87f19f493f4cb18fb99d..0f9a7b31818b94aeb19e2b0db77fb2d3905f679b 100644
--- a/Source/core/html/HTMLMediaElement.cpp
+++ b/Source/core/html/HTMLMediaElement.cpp
@@ -3738,7 +3738,7 @@ void HTMLMediaElement::setMediaGroup(const String& group)
     // attribute is set, changed, or removed, the user agent must run the following steps:
     // 1. Let m [this] be the media element in question.
     // 2. Let m have no current media controller, if it currently has one.
-    setController(0);
+    setControllerInternal(0);
 
     // 3. If m's mediagroup attribute is being removed, then abort these steps.
     if (group.isNull() || group.isEmpty())
@@ -3755,13 +3755,13 @@ void HTMLMediaElement::setMediaGroup(const String& group)
         // the new value of m's mediagroup attribute,
         if ((*i)->mediaGroup() == group) {
             //  then let controller be that media element's current media controller.
-            setController((*i)->controller());
+            setControllerInternal((*i)->controller());
             return;
         }
     }
 
     // Otherwise, let controller be a newly created MediaController.
-    setController(MediaController::create(Node::executionContext()));
+    setControllerInternal(MediaController::create(Node::executionContext()));
 }
 
 MediaController* HTMLMediaElement::controller() const
@@ -3771,6 +3771,15 @@ MediaController* HTMLMediaElement::controller() const
 
 void HTMLMediaElement::setController(PassRefPtr<MediaController> controller)
 {
+    // 4.8.10.11.2 Media controllers: controller attribute.
+    // On setting, it must first remove the element's mediagroup attribute, if any,
+    setMediaGroup(String());

[philipj_slow, 2013/11/08 07:00:17]

I think removeAttribute(mediagroupAttr) and letting parseAttribute deal with the
internals would be more clear here. It's not obvious to my why this will remove
the content attribute, if it even does.

Can you also update the test to verify that setting the controller removes the
mediagroup attribute?

[Inactive, 2013/11/08 15:35:38]

I did the same thing as the previous custom bindings code to be safe and make
sure I don't change behavior. However, I agree that your proposal would be more
readable. I will look into it and make sure it is covered by layout test.

[Inactive, 2013/11/08 17:08:31]

Congratulations, you found a bug in the custom bindings :)
setMediaGroup(String()) actually does not remove the attribute, it only updates
the m_mediaGroup member in HTMLMediaElement. mediaGroup is actually a reflected
attribute so it is a bad idea to store m_mediaGroup in HTMLMediaElement. We
should simply retrieve it using fastGetAttribute(mediaGroupAttr).

I am working on fixing this now.

+    // and then set the current media controller to the given value.
+    setControllerInternal(controller);
+}
+
+void HTMLMediaElement::setControllerInternal(PassRefPtr<MediaController> controller)
+{
     if (m_mediaController)
         m_mediaController->removeMediaElement(this);