Validate finiteness of HTMLMediaElement properties.

Validate finiteness of HTMLMediaElement properties.

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

[sandersd (OOO until July 31), 2014-03-29 00:32:32]

Added LayoutTests, PTAL.

[acolwell GONE FROM CHROMIUM, 2014-03-29 01:40:08]

https://codereview.chromium.org/217053009/diff/20001/LayoutTests/media/media-...
File LayoutTests/media/media-controller-volume.html (right):

https://codereview.chromium.org/217053009/diff/20001/LayoutTests/media/media-...
LayoutTests/media/media-controller-volume.html:8: var start = function() {
nit: { on next line

https://codereview.chromium.org/217053009/diff/20001/LayoutTests/media/media-...
LayoutTests/media/media-controller-volume.html:14: var canplay = function() {
ditto

https://codereview.chromium.org/217053009/diff/20001/LayoutTests/media/video-...
File LayoutTests/media/video-test.js (right):

https://codereview.chromium.org/217053009/diff/20001/LayoutTests/media/video-...
LayoutTests/media/video-test.js:228: logResult(exception !== undefined &&
exception + '' === exceptionString,
Why did you need to make this change? This could potentially effect a bunch of
tests if it changes the output.

[philipj_slow, 2014-03-29 09:59:09]

nbarth, isn't there an IDL compiler feature for this? Maybe [StrictTypeChecking]
should enable the check, given that "unrestricted double" is what should be used
if NaN/Infinity are allowed...

[acolwell GONE FROM CHROMIUM, 2014-03-29 18:35:04]

On 2014/03/29 09:59:09, philipj wrote:
> nbarth, isn't there an IDL compiler feature for this? Maybe
[StrictTypeChecking]
> should enable the check, given that "unrestricted double" is what should be
used
> if NaN/Infinity are allowed...

philipj: Good catch. I sent a side channel email to nbarth asking about this
because I was surprised we didn't have something for this already. Hadn't heard
back yet.
sandersd: Please try applying the [StrictTypeChecking] attribute in the IDL
instead and see if this results in the desired behavior.

[Nils Barth (inactive), 2014-03-31 04:36:01]

On 2014/03/29 09:59:09, philipj wrote:
> nbarth, isn't there an IDL compiler feature for this? Maybe
[StrictTypeChecking]
> should enable the check, given that "unrestricted double" is what should be
used
> if NaN/Infinity are allowed...

Nope, currently the IDL compiler assumes all floats and doubles are
unrestricted,
just like it assumes all interface arguments and return values are nullable.

The relevant bug is:
IDL: unrestricted float and unrestricted double
https://code.google.com/p/chromium/issues/detail?id=354298

This is the third request I've gotten for this, so I'll prioritize it
(i.e., start on it in the next day or so).
As you suggest, we should have this opt-in via [StrictTypeChecking].

Concretely, I expect I'll rename the existing [StrictTypeChecking] to
[StrictTypeChecking=NullableArguments]
and then add:
[StrictTypeChecking=UnrestrictedDoubleFloat]
...and also:
[StrictTypeChecking=NullableReturnValue]
...so we can turn these on separately without breaking anything.

[Nils Barth (inactive), 2014-03-31 04:39:21]

On 2014/03/29 18:35:04, acolwell wrote:
> sandersd: Please try applying the [StrictTypeChecking] attribute in the IDL
> instead and see if this results in the desired behavior.

Dan, [StrictTypeChecking] won't do anything yet.
If you need this urgently, feel free to land this CL but include a FIXME so
we can clean it up once implemented, like:
// FIXME: generated bindings should check isfinite: http://crbug.com/354298

This should be an easy fix, so I hope to get it done this week;
don't know how this works with your schedule.

[sandersd (OOO until July 31), 2014-03-31 20:41:07]

https://codereview.chromium.org/217053009/diff/20001/LayoutTests/media/media-...
File LayoutTests/media/media-controller-volume.html (right):

https://codereview.chromium.org/217053009/diff/20001/LayoutTests/media/media-...
LayoutTests/media/media-controller-volume.html:8: var start = function() {
On 2014/03/29 01:40:08, acolwell wrote:
> nit: { on next line

Done.

https://codereview.chromium.org/217053009/diff/20001/LayoutTests/media/media-...
LayoutTests/media/media-controller-volume.html:14: var canplay = function() {
On 2014/03/29 01:40:08, acolwell wrote:
> ditto

Done.

https://codereview.chromium.org/217053009/diff/20001/LayoutTests/media/video-...
File LayoutTests/media/video-test.js (right):

https://codereview.chromium.org/217053009/diff/20001/LayoutTests/media/video-...
LayoutTests/media/video-test.js:228: logResult(exception !== undefined &&
exception + '' === exceptionString,
On 2014/03/29 01:40:08, acolwell wrote:
> Why did you need to make this change? This could potentially effect a bunch of
> tests if it changes the output.

Looks like I didn't quite understand how the other tests were using this
function (it's not pretty, but cleaning it up is a different CL). I've reverted
this change.

[acolwell GONE FROM CHROMIUM, 2014-03-31 21:58:29]

lgtm % nit

https://codereview.chromium.org/217053009/diff/40001/Source/core/html/MediaCo...
File Source/core/html/MediaController.cpp (right):

https://codereview.chromium.org/217053009/diff/40001/Source/core/html/MediaCo...
Source/core/html/MediaController.cpp:163: if (!std::isfinite(time)) {
nit: You should probably add FIXME's to above this code, and everywhere else
you've added something similar, to indicate that this should be removed once the
bindings enforce this for us.

[sandersd (OOO until July 31), 2014-03-31 22:05:59]

The CQ bit was checked by sandersd@chromium.org

[commit-bot: I haz the power, 2014-03-31 22:06:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sandersd@chromium.org/217053009/60001

[commit-bot: I haz the power, 2014-04-01 00:09:53]

Change committed as 170495