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

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 3720 matching lines @@
 void HTMLMediaElement::setMediaGroup(const String& group)
 {
     if (m_mediaGroup == group)
         return;
     m_mediaGroup = group;
 
     // When a media element is created with a mediagroup attribute, and when a media element's mediagroup
     // 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())
         return;
 
     // 4. If there is another media element whose Document is the same as m's Document (even if one or both
     // of these elements are not actually in the Document),
     HashSet<HTMLMediaElement*> elements = documentToElementSetMap().get(&document());
     for (HashSet<HTMLMediaElement*>::iterator i = elements.begin(); i != elements.end(); ++i) {
         if (*i == this)
             continue;
 
         // and which also has a mediagroup attribute, and whose mediagroup attribute has the same value as
         // 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
 {
     return m_mediaController.get();
 }
 
 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);
 
     m_mediaController = controller;
 
     if (m_mediaController)
         m_mediaController->addMediaElement(this);
 
     if (hasMediaControls())
         mediaControls()->setMediaController(m_mediaController ? m_mediaController.get() : static_cast<MediaControllerInterface*>(this));
@@ skipping 87 matching lines @@
 {
     scheduleLayerUpdate();
 }
 
 bool HTMLMediaElement::isInteractiveContent() const
 {
     return fastHasAttribute(controlsAttr);
 }
 
 }