Fix assigning to MediaElement.controller

[Steve Block]

The spec [1] states that it should be possible to assign null to MediaElement.controller. This succeeds in Safari, but Chrome throws an exception due to logic in V8HTMLMediaElementCustom.cpp. See attached test case.

We should fix the V8 bindings, or at least make sure that the JSC and V8 bindings behave the same. 

[1] http://www.w3.org/html/wg/drafts/html/master/embedded-content-0.html#dom-media-controller

[Steve Block]

Created attachment 193696 [details]
Test case

[Steve Block]

Created attachment 194656 [details]
Patch

[Kentaro Hara]

Comment on attachment 194656 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194656&action=review

> Source/WebCore/bindings/v8/custom/V8HTMLMediaElementCustom.cpp:45
> -    HTMLMediaElement* imp = V8HTMLMediaElement::toNative(info.Holder());
>      MediaController* controller = 0;
> -    if (V8MediaController::HasInstance(value, info.GetIsolate(), worldType(info.GetIsolate())))
> +    if (!value->IsNull()) {
> +        if (!V8MediaController::HasInstance(value, info.GetIsolate(), worldType(info.GetIsolate()))) {
> +            throwTypeError("Value is not of type MediaController", info.GetIsolate());
> +            return;
> +        }
>          controller = V8MediaController::toNative(value->ToObject());
> -    
> -    if (!controller) {
> -        throwTypeError("Value is not of type MediaController", info.GetIsolate());
> -        return;
>      }

JSC binding is as follows:

  HTMLMediaElement* imp = static_cast<HTMLMediaElement*>(impl());
  imp->setMediaGroup(String());
  imp->setController(toMediaController(value));

It looks like JSC binding does not have a branch for throwTypeError() nor treat IsNull() specially. What's the difference between JSC and V8?

[Steve Block]

I think the JSC bindings are wrong, as assigning the wrong type to the controller attribute should throw. I'm new to this code, but have filed Bug 113120 to look into this (though it's currently blocked on a bug with the existing tests).

[Kentaro Hara]

Comment on attachment 194656 [details]
Patch

> I think the JSC bindings are wrong, as assigning the wrong type to the controller attribute should throw. I'm new to this code, but have filed Bug 113120 to look into this (though it's currently blocked on a bug with the existing tests).

Would it be possible to fix both JSC and V8 consistently in this patch? It looks like that your patch fixes one inconsistency but introduces another inconsistency (i.e. Only V8 binding treats IsNull() specially).

BTW, why do you want to treat only null specially? What about undefined? I'd be happy if you could point to the spec.

[Kentaro Hara]

(In reply to comment #5)
> BTW, why do you want to treat only null specially? What about undefined? I'd be happy if you could point to the spec.

Sorry, you pointed to the spec in https://bugs.webkit.org/show_bug.cgi?id=112641#c0 . Per the spec, only null should be treated specially. I understood.

[Steve Block]

> Would it be possible to fix both JSC and V8 consistently in this patch? It looks like that your patch fixes
> one inconsistency but introduces another inconsistency (i.e. Only V8 binding treats IsNull() specially).
I don't think this patch introduces a new inconsistency: with this patch both JSC and V8 allow null. Only V8 does type checking correctly, but that was true before too.

In any case, I'm happy to fix both bugs together. It means that this bug will be blocked on Bug 113119 so we can test the type checking exceptions.

[Steve Block]

*** Bug 113120 has been marked as a duplicate of this bug. ***

[Steve Block]

This is V8-specific, so closing