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
Created attachment 193696 [details] Test case
Created attachment 194656 [details] Patch
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?
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).
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.
(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.
> 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.
*** Bug 113120 has been marked as a duplicate of this bug. ***
This is V8-specific, so closing