Use [StrictTypeChecking] for HTMLMediaElement.setMediaKeys

Use [StrictTypeChecking] for HTMLMediaElement.setMediaKeys

The mediaKeys argument should be nullable, but passing in some other
type of object should throw an exception.

BUG=none

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

[philipj_slow, 2014-03-26 10:55:27]

David, PTAL. I had this sitting half-finished from when this was part of
HTMLMediaElement, since I promised nbarth to investigate in
https://code.google.com/p/chromium/issues/detail?id=249598#c25

If this function should be able to take null as a parameter, then the spec needs
fixing to say "void setMediaKeys(MediaKeys? mediaKeys)" instead.

[ddorwin, 2014-03-26 17:45:32]

On 2014/03/26 10:55:27, philipj wrote:
> David, PTAL. I had this sitting half-finished from when this was part of
> HTMLMediaElement, since I promised nbarth to investigate in
> https://code.google.com/p/chromium/issues/detail?id=249598#c25
> 
> If this function should be able to take null as a parameter, then the spec
needs
> fixing to say "void setMediaKeys(MediaKeys? mediaKeys)" instead.

Thanks for raising this issue.

setMediaKeys() needs more specification
(https://www.w3.org/Bugs/Public/show_bug.cgi?id=24216), but I believe the intent
is that the spec, though maybe not all implementations, should support
setMediaKeys(null) to "detach" the MediaKeys.

All examples using '?' in http://www.w3.org/TR/WebIDL/ are related to attributes
or constants (not parameters), but http://www.w3.org/TR/WebIDL/#idl-object does
seem to match what you say.
I think the IDL is probably correct for what we want, and I should update the
spec bug to include adding the '?'.

On a related note, there are checks for null array parameters in the
createSession() and update() algorithms. Those do not have a '?' either and
should not be null. Does that mean I should remove the null checks and we should
update Blink's MediaKeySession.idl to have [StrictTypeChecking] on those
methods?

[philipj_slow, 2014-03-26 17:54:03]

On 2014/03/26 17:45:32, ddorwin wrote:
> On 2014/03/26 10:55:27, philipj wrote:
> > David, PTAL. I had this sitting half-finished from when this was part of
> > HTMLMediaElement, since I promised nbarth to investigate in
> > https://code.google.com/p/chromium/issues/detail?id=249598#c25
> > 
> > If this function should be able to take null as a parameter, then the spec
> needs
> > fixing to say "void setMediaKeys(MediaKeys? mediaKeys)" instead.
> 
> Thanks for raising this issue.
> 
> setMediaKeys() needs more specification
> (https://www.w3.org/Bugs/Public/show_bug.cgi?id=24216), but I believe the
intent
> is that the spec, though maybe not all implementations, should support
> setMediaKeys(null) to "detach" the MediaKeys.
> 
> All examples using '?' in http://www.w3.org/TR/WebIDL/ are related to
attributes
> or constants (not parameters), but http://www.w3.org/TR/WebIDL/#idl-object
does
> seem to match what you say.
> I think the IDL is probably correct for what we want, and I should update the
> spec bug to include adding the '?'.

Yeah, and of course the spec also needs to say what actually happens when you
pass in null.

> On a related note, there are checks for null array parameters in the
> createSession() and update() algorithms. Those do not have a '?' either and
> should not be null. Does that mean I should remove the null checks and we
should
> update Blink's MediaKeySession.idl to have [StrictTypeChecking] on those
> methods?

StrictTypeChecking should become the default, so wherever adding it makes a
difference something needs to be done.

MediaKeys.createSession looks like it should have StrictTypeChecking added at
the interface level. Same for MediaKeySession.update(). You also have
webkitGenerateKeyRequest and webkitAddKey, but since there's no spec for those I
don't know what should be done. I guess either adding '?' or not to match the
existing behavior?

[ddorwin, 2014-03-26 18:08:34]

On 2014/03/26 17:54:03, philipj wrote:
> On 2014/03/26 17:45:32, ddorwin wrote:
> > On 2014/03/26 10:55:27, philipj wrote:
> > > David, PTAL. I had this sitting half-finished from when this was part of
> > > HTMLMediaElement, since I promised nbarth to investigate in
> > > https://code.google.com/p/chromium/issues/detail?id=249598#c25
> > > 
> > > If this function should be able to take null as a parameter, then the spec
> > needs
> > > fixing to say "void setMediaKeys(MediaKeys? mediaKeys)" instead.
> > 
> > Thanks for raising this issue.
> > 
> > setMediaKeys() needs more specification
> > (https://www.w3.org/Bugs/Public/show_bug.cgi?id=24216), but I believe the
> intent
> > is that the spec, though maybe not all implementations, should support
> > setMediaKeys(null) to "detach" the MediaKeys.
> > 
> > All examples using '?' in http://www.w3.org/TR/WebIDL/ are related to
> attributes
> > or constants (not parameters), but http://www.w3.org/TR/WebIDL/#idl-object
> does
> > seem to match what you say.
> > I think the IDL is probably correct for what we want, and I should update
the
> > spec bug to include adding the '?'.
> 
> Yeah, and of course the spec also needs to say what actually happens when you
> pass in null.

Do you want to update this CL to add the '?'? That should avoid any test
behavior changes, right?

> > On a related note, there are checks for null array parameters in the
> > createSession() and update() algorithms. Those do not have a '?' either and
> > should not be null. Does that mean I should remove the null checks and we
> should
> > update Blink's MediaKeySession.idl to have [StrictTypeChecking] on those
> > methods?
> 
> StrictTypeChecking should become the default, so wherever adding it makes a
> difference something needs to be done.
> 
> MediaKeys.createSession looks like it should have StrictTypeChecking added at
> the interface level. Same for MediaKeySession.update(). You also have
> webkitGenerateKeyRequest and webkitAddKey, but since there's no spec for those
I
> don't know what should be done. I guess either adding '?' or not to match the
> existing behavior?

HTMLMediaElementEncryptedMedia.idl defines several parameters as optional, which
is correct. Optional might not be handled correctly, though. Assuming things
continue to work with those parameters being optional, we should be able to add
StrictTypeChecking to all methods in that file.

[philipj_slow, 2014-03-26 18:17:20]

On 2014/03/26 18:08:34, ddorwin wrote:
> On 2014/03/26 17:54:03, philipj wrote:
> > On 2014/03/26 17:45:32, ddorwin wrote:
> > > On 2014/03/26 10:55:27, philipj wrote:
> > > > David, PTAL. I had this sitting half-finished from when this was part of
> > > > HTMLMediaElement, since I promised nbarth to investigate in
> > > > https://code.google.com/p/chromium/issues/detail?id=249598#c25
> > > > 
> > > > If this function should be able to take null as a parameter, then the
spec
> > > needs
> > > > fixing to say "void setMediaKeys(MediaKeys? mediaKeys)" instead.
> > > 
> > > Thanks for raising this issue.
> > > 
> > > setMediaKeys() needs more specification
> > > (https://www.w3.org/Bugs/Public/show_bug.cgi?id=24216), but I believe the
> > intent
> > > is that the spec, though maybe not all implementations, should support
> > > setMediaKeys(null) to "detach" the MediaKeys.
> > > 
> > > All examples using '?' in http://www.w3.org/TR/WebIDL/ are related to
> > attributes
> > > or constants (not parameters), but http://www.w3.org/TR/WebIDL/#idl-object
> > does
> > > seem to match what you say.
> > > I think the IDL is probably correct for what we want, and I should update
> the
> > > spec bug to include adding the '?'.
> > 
> > Yeah, and of course the spec also needs to say what actually happens when
you
> > pass in null.
> 
> Do you want to update this CL to add the '?'? That should avoid any test
> behavior changes, right?

Sure, I can do that, and the spec can catch up later.

> > > On a related note, there are checks for null array parameters in the
> > > createSession() and update() algorithms. Those do not have a '?' either
and
> > > should not be null. Does that mean I should remove the null checks and we
> > should
> > > update Blink's MediaKeySession.idl to have [StrictTypeChecking] on those
> > > methods?
> > 
> > StrictTypeChecking should become the default, so wherever adding it makes a
> > difference something needs to be done.
> > 
> > MediaKeys.createSession looks like it should have StrictTypeChecking added
at
> > the interface level. Same for MediaKeySession.update(). You also have
> > webkitGenerateKeyRequest and webkitAddKey, but since there's no spec for
those
> I
> > don't know what should be done. I guess either adding '?' or not to match
the
> > existing behavior?
> 
> HTMLMediaElementEncryptedMedia.idl defines several parameters as optional,
which
> is correct. Optional might not be handled correctly, though. Assuming things
> continue to work with those parameters being optional, we should be able to
add
> StrictTypeChecking to all methods in that file.

Oh right, passing null for an optional argument should behave as if it was
omitted. The bug for that is
https://code.google.com/p/chromium/issues/detail?id=293561

I suppose the prefixed bits are best left alone for now. I'll leave the rest to
you, now that you know about it :)

[philipj_slow, 2014-03-28 09:29:32]

Ping ddorwin. Adding StrictTypeChecking does result in an observable change, so
I updated the test as well.

[ddorwin, 2014-03-28 18:47:20]

LGTM. Thanks. I didn't know you had uploaded a new PS.

[philipj_slow, 2014-03-28 19:16:38]

On 2014/03/28 18:47:20, ddorwin wrote:
> LGTM. Thanks. I didn't know you had uploaded a new PS.

Oh right, I forgot that doesn't send an email.

[philipj_slow, 2014-03-28 19:16:47]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2014-03-28 19:16:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/philipj@opera.com/212643002/60001

[commit-bot: I haz the power, 2014-03-28 19:28:48]

Change committed as 170354