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

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

Setting HTMLMediaElement.controller does not properly remove the 'mediagroup' content attribute,
as mandated by the specification:
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#dom-media-controller

We had custom bindings code for the controller attribute setter which attempt to remove the
mediaGroup attribute by calling HTMLMediaElement::setMediaGroup(String()). However, while this
updates the m_mediaGroup member in HTMLMediaElement, this does not really remove the attribute.

Since mediaGroup is a reflected attribute, accessing the mediaGroup attribute from JS will end up
calling Element::fastGetAttribute(mediaGroupAttr) on native side, and bypass the value stored in
HTMLMediaElement::m_mediaGroup.

This CL updates HTMLMediaElement::setController() so that it calls removeAttribute(mediaGroupAttr)
to actually remove the attribute. It also gets rid of the custom bindings code and the m_mediaGroup
member entirely as it is bug prone.

BUG=316636
TEST=media/media-controller.html

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

[Inactive, 2013-11-07 18:52:20]

For comparison, the generated code looks like:
static void controllerAttributeSetter(v8::Local<v8::Value> jsValue, const
v8::PropertyCallbackInfo<void>& info)
{
    if (!isUndefinedOrNull(jsValue) && !V8MediaController::HasInstance(jsValue,
info.GetIsolate(), worldType(info.GetIsolate()))) {
        throwTypeError(ExceptionMessages::failedToSet("controller",
"HTMLMediaElement", "The provided value is not of type 'MediaController'."),
info.GetIsolate());
        return;
    }
    HTMLMediaElement* imp = V8HTMLMediaElement::toNative(info.Holder());
    V8TRYCATCH_VOID(MediaController*, cppValue,
V8MediaController::HasInstance(jsValue, info.GetIsolate(),
worldType(info.GetIsolate())) ?
V8MediaController::toNative(v8::Handle<v8::Object>::Cast(jsValue)) : 0);
    imp->setController(WTF::getPtr(cppValue));
}

[haraken, 2013-11-07 23:31:34]

https://codereview.chromium.org/59233014/diff/1/Source/bindings/v8/custom/V8H...
File Source/bindings/v8/custom/V8HTMLMediaElementCustom.cpp (left):

https://codereview.chromium.org/59233014/diff/1/Source/bindings/v8/custom/V8H...
Source/bindings/v8/custom/V8HTMLMediaElementCustom.cpp:36: if (!value->IsNull())
{

Won't this CL change existing behavior when undefined is passed to |value|?

Could you add a test case of |value|=undefined?

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

https://codereview.chromium.org/59233014/diff/1/Source/core/html/HTMLMediaEle...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/59233014/diff/1/Source/core/html/HTMLMediaEle...
Source/core/html/HTMLMediaElement.cpp:3776: setMediaGroup(String());
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]

https://codereview.chromium.org/59233014/diff/1/Source/bindings/v8/custom/V8H...
File Source/bindings/v8/custom/V8HTMLMediaElementCustom.cpp (left):

https://codereview.chromium.org/59233014/diff/1/Source/bindings/v8/custom/V8H...
Source/bindings/v8/custom/V8HTMLMediaElementCustom.cpp:36: if (!value->IsNull())
{
On 2013/11/07 23:31:35, haraken wrote:
> 
> Won't this CL change existing behavior when undefined is passed to |value|?
> 
> Could you add a test case of |value|=undefined?

Yes, you are right that the generated handles undefined differently than the
custom one. We used to throw when given undefined and now we treat it as null.
The compatibility risk is low but you are right that it should be highlighted in
the Changelog and covered by layout test.

https://codereview.chromium.org/59233014/diff/1/Source/core/html/HTMLMediaEle...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/59233014/diff/1/Source/core/html/HTMLMediaEle...
Source/core/html/HTMLMediaElement.cpp:3776: setMediaGroup(String());
On 2013/11/08 07:00:17, philipj wrote:
> 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?

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:30]

https://codereview.chromium.org/59233014/diff/1/Source/core/html/HTMLMediaEle...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/59233014/diff/1/Source/core/html/HTMLMediaEle...
Source/core/html/HTMLMediaElement.cpp:3776: setMediaGroup(String());
On 2013/11/08 07:00:17, philipj wrote:
> 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?

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.

[Inactive, 2013-11-08 17:11:11]

Fixed the mediaGroup removal bug and extended the layout test to cover this.

+tkent since this is a web exposed change.

[haraken, 2013-11-08 17:23:59]

> Yes, you are right that the generated handles undefined differently than the
> custom one. We used to throw when given undefined and now we treat it as null.
> The compatibility risk is low but you are right that it should be highlighted
in
> the Changelog and covered by layout test.

What's the behavior of Firefox and IE about undefined?

[Inactive, 2013-11-08 19:46:17]

On 2013/11/08 17:23:59, haraken wrote:
> > Yes, you are right that the generated handles undefined differently than the
> > custom one. We used to throw when given undefined and now we treat it as
null.
> > The compatibility risk is low but you are right that it should be
highlighted
> in
> > the Changelog and covered by layout test.
> 
> What's the behavior of Firefox and IE about undefined?

Firefox 25 and IE10 to not appear to support mediacontroller or mediagroup
attribute. For Firefox, I found the following open bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=847377

[Inactive, 2013-11-08 19:46:45]

On 2013/11/08 19:46:17, Chris Dumez wrote:
> On 2013/11/08 17:23:59, haraken wrote:
> > > Yes, you are right that the generated handles undefined differently than
the
> > > custom one. We used to throw when given undefined and now we treat it as
> null.
> > > The compatibility risk is low but you are right that it should be
> highlighted
> > in
> > > the Changelog and covered by layout test.
> > 
> > What's the behavior of Firefox and IE about undefined?
> 
> Firefox 25 and IE10 to not appear to support mediacontroller or mediagroup
> attribute. For Firefox, I found the following open bug:
> https://bugzilla.mozilla.org/show_bug.cgi?id=847377

"...do not appear..."

[philipj_slow, 2013-11-08 19:58:04]

https://codereview.chromium.org/59233014/diff/90002/LayoutTests/media/media-c...
File LayoutTests/media/media-controller.html (right):

https://codereview.chromium.org/59233014/diff/90002/LayoutTests/media/media-c...
LayoutTests/media/media-controller.html:50: testExpected("video.mediaGroup",
"");
Also checking !video.hasAttribute('mediagroup') here and below would be good,
since I think this would also pass if the attribute was present but empty.

https://codereview.chromium.org/59233014/diff/90002/Source/core/html/HTMLMedi...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/59233014/diff/90002/Source/core/html/HTMLMedi...
Source/core/html/HTMLMediaElement.cpp:3772:
removeAttribute(HTMLNames::mediagroupAttr);
Is the HTMLNames:: prefix needed? The vast majority of places in this file don't
include it, crossoriginAttr being the exception.

[Inactive, 2013-11-08 20:12:05]

https://codereview.chromium.org/59233014/diff/90002/LayoutTests/media/media-c...
File LayoutTests/media/media-controller.html (right):

https://codereview.chromium.org/59233014/diff/90002/LayoutTests/media/media-c...
LayoutTests/media/media-controller.html:50: testExpected("video.mediaGroup",
"");
On 2013/11/08 19:58:04, philipj wrote:
> Also checking !video.hasAttribute('mediagroup') here and below would be good,
> since I think this would also pass if the attribute was present but empty.

Done.

https://codereview.chromium.org/59233014/diff/90002/Source/core/html/HTMLMedi...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/59233014/diff/90002/Source/core/html/HTMLMedi...
Source/core/html/HTMLMediaElement.cpp:3772:
removeAttribute(HTMLNames::mediagroupAttr);
On 2013/11/08 19:58:04, philipj wrote:
> Is the HTMLNames:: prefix needed? The vast majority of places in this file
don't
> include it, crossoriginAttr being the exception.

Done.

[philipj_slow, 2013-11-08 21:09:44]

Non-owner LGTM.

[haraken, 2013-11-08 21:54:13]

> Firefox 25 and IE10 to not appear to support mediacontroller or mediagroup
> attribute. For Firefox, I found the following open bug:
> https://bugzilla.mozilla.org/show_bug.cgi?id=847377
>
> "...do not appear..."

Thanks! Regarding undefined, the new behavior in this CL looks more consistent
with other DOM features, but it's not clear from the MediaController spec what
the right behavior is. Would you cc MediaController people and ask them about
how undefined should be treated?

(I'm sorry you have to be troubled with such edge cases...)

[haraken, 2013-11-08 22:04:50]

On 2013/11/08 21:54:13, haraken wrote:
> > Firefox 25 and IE10 to not appear to support mediacontroller or mediagroup
> > attribute. For Firefox, I found the following open bug:
> > https://bugzilla.mozilla.org/show_bug.cgi?id=847377
> >
> > "...do not appear..."
> 
> Thanks! Regarding undefined, the new behavior in this CL looks more consistent
> with other DOM features, but it's not clear from the MediaController spec what
> the right behavior is. Would you cc MediaController people and ask them about
> how undefined should be treated?
> 
> (I'm sorry you have to be troubled with such edge cases...)

I'll be offline in the weekend, so let me stamp LGTM (if MediaController people
say this change is OK). Also, this change needs an approval from an API owner.

[Inactive, 2013-11-08 22:34:45]

On 2013/11/08 22:04:50, haraken wrote:
> On 2013/11/08 21:54:13, haraken wrote:
> > > Firefox 25 and IE10 to not appear to support mediacontroller or mediagroup
> > > attribute. For Firefox, I found the following open bug:
> > > https://bugzilla.mozilla.org/show_bug.cgi?id=847377
> > >
> > > "...do not appear..."
> > 
> > Thanks! Regarding undefined, the new behavior in this CL looks more
consistent
> > with other DOM features, but it's not clear from the MediaController spec
what
> > the right behavior is. Would you cc MediaController people and ask them
about
> > how undefined should be treated?
> > 
> > (I'm sorry you have to be troubled with such edge cases...)
> 
> I'll be offline in the weekend, so let me stamp LGTM (if MediaController
people
> say this change is OK). Also, this change needs an approval from an API owner.

I have just checked the WebKit implementation as it seems MediaController was
initially implemented by Apple. They treat undefined as null. As a matter of
fact, they don't even do strict type checking. Any input that is not a
MediaController will be treated as null. It might make sense to align with
WebKit/Safari as it seems to be the only other implementation of MediaController
out there.

Otherwise, do you know who I can ping on Blink side for MediaController?

[Inactive, 2013-11-08 22:52:30]

+acolwell

[philipj_slow, 2013-11-09 06:14:05]

On 2013/11/08 22:34:45, Chris Dumez wrote:
> On 2013/11/08 22:04:50, haraken wrote:
> > On 2013/11/08 21:54:13, haraken wrote:
> > > > Firefox 25 and IE10 to not appear to support mediacontroller or
mediagroup
> > > > attribute. For Firefox, I found the following open bug:
> > > > https://bugzilla.mozilla.org/show_bug.cgi?id=847377
> > > >
> > > > "...do not appear..."
> > > 
> > > Thanks! Regarding undefined, the new behavior in this CL looks more
> consistent
> > > with other DOM features, but it's not clear from the MediaController spec
> what
> > > the right behavior is. Would you cc MediaController people and ask them
> about
> > > how undefined should be treated?
> > > 
> > > (I'm sorry you have to be troubled with such edge cases...)
> > 
> > I'll be offline in the weekend, so let me stamp LGTM (if MediaController
> people
> > say this change is OK). Also, this change needs an approval from an API
owner.
> 
> I have just checked the WebKit implementation as it seems MediaController was
> initially implemented by Apple. They treat undefined as null. As a matter of
> fact, they don't even do strict type checking. Any input that is not a
> MediaController will be treated as null. It might make sense to align with
> WebKit/Safari as it seems to be the only other implementation of
MediaController
> out there.
> 
> Otherwise, do you know who I can ping on Blink side for MediaController?

I really think we should go with the spec here. MediaController isn't widely
used yet, so there's likely no compatible issues yet. Filing a WebKit bug might
be good though.

[Inactive, 2013-11-09 18:24:31]

On 2013/11/09 06:14:05, philipj wrote:
> On 2013/11/08 22:34:45, Chris Dumez wrote:
> > On 2013/11/08 22:04:50, haraken wrote:
> > > On 2013/11/08 21:54:13, haraken wrote:
> > > > > Firefox 25 and IE10 to not appear to support mediacontroller or
> mediagroup
> > > > > attribute. For Firefox, I found the following open bug:
> > > > > https://bugzilla.mozilla.org/show_bug.cgi?id=847377
> > > > >
> > > > > "...do not appear..."
> > > > 
> > > > Thanks! Regarding undefined, the new behavior in this CL looks more
> > consistent
> > > > with other DOM features, but it's not clear from the MediaController
spec
> > what
> > > > the right behavior is. Would you cc MediaController people and ask them
> > about
> > > > how undefined should be treated?
> > > > 
> > > > (I'm sorry you have to be troubled with such edge cases...)
> > > 
> > > I'll be offline in the weekend, so let me stamp LGTM (if MediaController
> > people
> > > say this change is OK). Also, this change needs an approval from an API
> owner.
> > 
> > I have just checked the WebKit implementation as it seems MediaController
was
> > initially implemented by Apple. They treat undefined as null. As a matter of
> > fact, they don't even do strict type checking. Any input that is not a
> > MediaController will be treated as null. It might make sense to align with
> > WebKit/Safari as it seems to be the only other implementation of
> MediaController
> > out there.
> > 
> > Otherwise, do you know who I can ping on Blink side for MediaController?
> 
> I really think we should go with the spec here. MediaController isn't widely
> used yet, so there's likely no compatible issues yet. Filing a WebKit bug
might
> be good though.

Well, as per Web IDL specification [1] for nullable types, if V is null or
*undefined*, then return the IDL nullable type T? value null.
Also according to Web IDL [2], we should throw a TypeError if the input value
does not have the expected type.

Therefore, the current patch *is* according to specification.

[1] http://heycam.github.io/webidl/#es-nullable-type
[2] http://heycam.github.io/webidl/#es-interface

[philipj_slow, 2013-11-10 14:22:37]

On 2013/11/09 18:24:31, Chris Dumez wrote:
> On 2013/11/09 06:14:05, philipj wrote:
> > On 2013/11/08 22:34:45, Chris Dumez wrote:
> > > On 2013/11/08 22:04:50, haraken wrote:
> > > > On 2013/11/08 21:54:13, haraken wrote:
> > > > > > Firefox 25 and IE10 to not appear to support mediacontroller or
> > mediagroup
> > > > > > attribute. For Firefox, I found the following open bug:
> > > > > > https://bugzilla.mozilla.org/show_bug.cgi?id=847377
> > > > > >
> > > > > > "...do not appear..."
> > > > > 
> > > > > Thanks! Regarding undefined, the new behavior in this CL looks more
> > > consistent
> > > > > with other DOM features, but it's not clear from the MediaController
> spec
> > > what
> > > > > the right behavior is. Would you cc MediaController people and ask
them
> > > about
> > > > > how undefined should be treated?
> > > > > 
> > > > > (I'm sorry you have to be troubled with such edge cases...)
> > > > 
> > > > I'll be offline in the weekend, so let me stamp LGTM (if MediaController
> > > people
> > > > say this change is OK). Also, this change needs an approval from an API
> > owner.
> > > 
> > > I have just checked the WebKit implementation as it seems MediaController
> was
> > > initially implemented by Apple. They treat undefined as null. As a matter
of
> > > fact, they don't even do strict type checking. Any input that is not a
> > > MediaController will be treated as null. It might make sense to align with
> > > WebKit/Safari as it seems to be the only other implementation of
> > MediaController
> > > out there.
> > > 
> > > Otherwise, do you know who I can ping on Blink side for MediaController?
> > 
> > I really think we should go with the spec here. MediaController isn't widely
> > used yet, so there's likely no compatible issues yet. Filing a WebKit bug
> might
> > be good though.
> 
> Well, as per Web IDL specification [1] for nullable types, if V is null or
> *undefined*, then return the IDL nullable type T? value null.
> Also according to Web IDL [2], we should throw a TypeError if the input value
> does not have the expected type.
> 
> Therefore, the current patch *is* according to specification.
> 
> [1] http://heycam.github.io/webidl/#es-nullable-type
> [2] http://heycam.github.io/webidl/#es-interface

Thanks for the explanation!

[tkent, 2013-11-11 00:28:22]

lgtm.
Looks reasonable.

[commit-bot: I haz the power, 2013-11-11 00:28:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/59233014/250001

[commit-bot: I haz the power, 2013-11-11 00:28:47]

Failed to apply patch for Source/core/html/HTMLMediaElement.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/html/HTMLMediaElement.cpp
  Hunk #1 succeeded at 3722 (offset -2 lines).
  Hunk #2 succeeded at 3750 (offset -2 lines).
  Hunk #3 succeeded at 3766 (offset -2 lines).
  Hunk #4 FAILED at 3861.
  1 out of 4 hunks FAILED -- saving rejects to file
Source/core/html/HTMLMediaElement.cpp.rej

Patch:       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
ef33ad5a05d2644c3abb38ea7856bdbb08f9d5a1..836880f19b28d31893c07d1d7e7f7e692e36e05c
100644
--- a/Source/core/html/HTMLMediaElement.cpp
+++ b/Source/core/html/HTMLMediaElement.cpp
@@ -3724,22 +3724,18 @@ AudioSourceProvider*
HTMLMediaElement::audioSourceProvider()
 }
 #endif
 
-const String& HTMLMediaElement::mediaGroup() const
+const AtomicString& HTMLMediaElement::mediaGroup() const
 {
-    return m_mediaGroup;
+    return fastGetAttribute(mediagroupAttr);
 }
 
-void HTMLMediaElement::setMediaGroup(const String& group)
+void HTMLMediaElement::setMediaGroup(const AtomicString& 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())
@@ -3756,13 +3752,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
@@ -3772,6 +3768,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,
+    removeAttribute(mediagroupAttr);
+    // 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);
 
@@ -3856,9 +3861,9 @@ void HTMLMediaElement::applyMediaFragmentURI()
 
 MediaPlayerClient::CORSMode HTMLMediaElement::mediaPlayerCORSMode() const
 {
-    if (!fastHasAttribute(HTMLNames::crossoriginAttr))
+    if (!fastHasAttribute(crossoriginAttr))
         return Unspecified;
-    if (equalIgnoringCase(fastGetAttribute(HTMLNames::crossoriginAttr),
"use-credentials"))
+    if (equalIgnoringCase(fastGetAttribute(crossoriginAttr),
"use-credentials"))
         return UseCredentials;
     return Anonymous;
 }

[commit-bot: I haz the power, 2013-11-11 14:04:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/59233014/430001

[philipj_slow, 2013-11-12 10:06:38]

The pending trybot probably isn't going to fix itself, I had to uncheck the
commit checkbox, wait for a while, and then check it again to land things I
queued up yesterday.

[commit-bot: I haz the power, 2013-11-12 13:33:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/59233014/430001

[commit-bot: I haz the power, 2013-11-12 15:07:22]

Change committed as 161812