Update all float attributes in HTMLMediaElement and related objects to double

Update all float attributes in HTMLMediaElement and related objects to double.

This will bring HTMLMediaElement and TimeRanges into compliance with the HTML5 spec declarations and will remove script visible truncations to the float range once the Chromium-side changes land.


BUG=227156
TEST=All existing media LayoutTests still pass.

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

[jamesr, 2013-04-05 21:30:12]

Seems reasonable, but I'm probably not the best reviewer for something like
this.  Do we have someone in mind to have ownership of the media systems in
blink?

[acolwell GONE FROM CHROMIUM, 2013-04-05 21:36:12]

On 2013/04/05 21:30:12, jamesr wrote:
> Seems reasonable, but I'm probably not the best reviewer for something like
> this.  Do we have someone in mind to have ownership of the media systems in
> blink?

Unfortunately eric.carlson@ & jer.noble@ were the primary reviewers for the
WebKit media code. scherkus@ & myself are probably the best people to own this
in Blink, but neither of us were WebKit reviewers so we are in a bit of a bind
right now. 

If it is any consolation, eric.carlson@ reviewed all but the Source/WebKit
changes here (https://bugs.webkit.org/show_bug.cgi?id=113615) but these didn't
get to land before the fork.

[eseidel, 2013-04-05 21:44:14]

Can you add a bit more description to your change?  Did you try this in b.w.o
but faced resistance?  Is the size change OK for all these elements?

Yes, I would like you and Sherkus to own media/ if you're willing. :)

https://codereview.chromium.org/13454026/diff/2002/Source/WebCore/html/HTMLMe...
File Source/WebCore/html/HTMLMediaElement.idl (right):

https://codereview.chromium.org/13454026/diff/2002/Source/WebCore/html/HTMLMe...
Source/WebCore/html/HTMLMediaElement.idl:68: readonly attribute double
startTime;
This is a web-observable change, no?  I guess all numbers in JS are doubles
anyway?

https://codereview.chromium.org/13454026/diff/2002/Source/WebCore/html/TimeRa...
File Source/WebCore/html/TimeRanges.h (right):

https://codereview.chromium.org/13454026/diff/2002/Source/WebCore/html/TimeRa...
Source/WebCore/html/TimeRanges.h:66: TimeRanges(double start, double end);
Do we have a lot of these?  Is doubling the size a problem?

https://codereview.chromium.org/13454026/diff/2002/Source/WebCore/platform/Cl...
File Source/WebCore/platform/Clock.h (right):

https://codereview.chromium.org/13454026/diff/2002/Source/WebCore/platform/Cl...
Source/WebCore/platform/Clock.h:39: virtual void setCurrentTime(double) = 0;
Why is this OK?  Is clock only specific to Media elements?

[eseidel, 2013-04-05 21:44:38]

You're going to have trouble landing this with non-chromium changes in the diff,
as those files are being deleted left and right right now. :)

[acolwell GONE FROM CHROMIUM, 2013-04-05 22:18:10]

On 2013/04/05 21:44:14, Eric Seidel (Google) wrote:
> Can you add a bit more description to your change?  

Yes. Done.

> Did you try this in b.w.o but faced resistance?  Is the size change OK for all
these elements?

I didn't face resistance. This change didn't make it in time primarily because
there were symbol export problems on the Apple Mac builds, that I couldn't
resolve w/o their help. I was then OOO and hadn't had a chance to see if they
still wanted the change post fork.

The size change is minimal. There aren't usually a ton of HTMLMediaElements or
TimeRanges on a page.

> 
> Yes, I would like you and Sherkus to own media/ if you're willing. :)
> 
Yes. I'm willing and I'm pretty sure scherkus@ is too.

[acolwell GONE FROM CHROMIUM, 2013-04-05 22:29:11]

https://codereview.chromium.org/13454026/diff/2002/Source/WebCore/html/HTMLMe...
File Source/WebCore/html/HTMLMediaElement.idl (right):

https://codereview.chromium.org/13454026/diff/2002/Source/WebCore/html/HTMLMe...
Source/WebCore/html/HTMLMediaElement.idl:68: readonly attribute double
startTime;
On 2013/04/05 21:44:14, Eric Seidel (Google) wrote:
> This is a web-observable change, no?  I guess all numbers in JS are doubles
> anyway?
Yes it is observable. If you set any of these properties with extreme values
then they get truncated to the float range. This causes the following code to go
BOOM().

video.playbackRate = Number.MAX_VALUE
if (video.playbackRate != Number.MAX_VALUE)
  BOOM();

This change is really just about making HTMLMediaElement conform to the HTML5
spec and avoid these truncations to the float range.

https://codereview.chromium.org/13454026/diff/2002/Source/WebCore/html/TimeRa...
File Source/WebCore/html/TimeRanges.h (right):

https://codereview.chromium.org/13454026/diff/2002/Source/WebCore/html/TimeRa...
Source/WebCore/html/TimeRanges.h:66: TimeRanges(double start, double end);
On 2013/04/05 21:44:14, Eric Seidel (Google) wrote:
> Do we have a lot of these?  Is doubling the size a problem?
No. There is usually only one per HTMLMediaElement for normal playback.

https://codereview.chromium.org/13454026/diff/2002/Source/WebCore/platform/Cl...
File Source/WebCore/platform/Clock.h (right):

https://codereview.chromium.org/13454026/diff/2002/Source/WebCore/platform/Cl...
Source/WebCore/platform/Clock.h:39: virtual void setCurrentTime(double) = 0;
On 2013/04/05 21:44:14, Eric Seidel (Google) wrote:
> Why is this OK?  Is clock only specific to Media elements?
Source/WebCore/html/MediaController.cpp	is the only class that makes any calls
on this interface.

[acolwell GONE FROM CHROMIUM, 2013-04-05 22:29:39]

On 2013/04/05 21:44:38, Eric Seidel (Google) wrote:
> You're going to have trouble landing this with non-chromium changes in the
diff,
> as those files are being deleted left and right right now. :)

I removed the changes to the Apple & Blackberry media controls.

[eseidel, 2013-04-05 22:30:51]

Since this is web observable, I would like to see a LayoutTest, otherwise I'm
ready to r+.

[acolwell GONE FROM CHROMIUM, 2013-04-06 00:26:53]

On 2013/04/05 22:30:51, Eric Seidel (Google) wrote:
> Since this is web observable, I would like to see a LayoutTest, otherwise I'm
> ready to r+.

I updated the volume and playbackRate layout tests to verify proper behavior
with extreme values. 

The time related fields will be difficult to test until I land the appropriate
Chromium webkit/media changes. If it is ok, I'd like to provide the updated
LayoutTests in the followup Blink patch, after the webkit/media changes land,
that gets rid of the XXXFloat methods added here.

[acolwell GONE FROM CHROMIUM, 2013-04-06 02:13:56]

This patch now relies on https://codereview.chromium.org/13431009/ landing
first.

[eseidel, 2013-04-06 04:04:46]

https://codereview.chromium.org/13454026/diff/13001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp (right):

https://codereview.chromium.org/13454026/diff/13001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:530:
m_webMediaPlayer->setVolumeFloat(volume);
Float?

https://codereview.chromium.org/13454026/diff/13001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:550: return
m_webMediaPlayer->maxTimeSeekableFloat();
Float?

https://codereview.chromium.org/13454026/diff/13001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:668: return
m_webMediaPlayer->mediaTimeForTimeValueFloat(timeValue);
This looks wrong?

[acolwell GONE FROM CHROMIUM, 2013-04-06 04:17:06]

https://codereview.chromium.org/13454026/diff/13001/Source/WebKit/chromium/sr...
File Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp (right):

https://codereview.chromium.org/13454026/diff/13001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:530:
m_webMediaPlayer->setVolumeFloat(volume);
On 2013/04/06 04:04:46, Eric Seidel (Google) wrote:
> Float?
Yes. These are the new temporary method names. The plan is to move the calls
over to these methods temporarily so that things continue to work and then
implement setVolume(double) and finally change the code back to use setVolume().


For this method at least I could provide a setVolume(double) if we are ok
temporarily having overloads. Would that be a preferred route?

https://codereview.chromium.org/13454026/diff/13001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:550: return
m_webMediaPlayer->maxTimeSeekableFloat();
On 2013/04/06 04:04:46, Eric Seidel (Google) wrote:
> Float?
Yes. The idea here is the same as above. In this case, I have to use a different
name because only the return type is different.

https://codereview.chromium.org/13454026/diff/13001/Source/WebKit/chromium/sr...
Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:668: return
m_webMediaPlayer->mediaTimeForTimeValueFloat(timeValue);
On 2013/04/06 04:04:46, Eric Seidel (Google) wrote:
> This looks wrong?
No. Same reasoning as above.

[acolwell GONE FROM CHROMIUM, 2013-04-06 04:27:55]

I'm sorry. I should have outlined this from the start. 

Here is the plan:
1. Land https://codereview.chromium.org/13431009/
2. Land this patch. 
3. Land a Chromium patch that updates the original signatures in the files
modified by step 1 to use doubles instead of floats.
4. Land a Blink patch that restores the original method names with doubles,
update all xxxFloat() calls with the original method names now that they used
doubles, remove the xxxFloat methods, and updates the LayoutTests.
5. Land a Chromium patch that removes the xxxFloat methods.

If you can think of a dance w/ less steps I'd appreciate the suggestion.

[eseidel, 2013-04-06 04:51:53]

lgtm

[eseidel, 2013-04-06 04:51:53]

lgtm

lgtm

[acolwell GONE FROM CHROMIUM, 2013-04-09 20:56:33]

jamesr: Can I get an OWNERS LGTM for the following files:

Source/WebKit/chromium/public/WebMediaPlayer.h
Source/WebKit/chromium/public/WebMediaPlayerClient.h

This are just double -> float signature changes and temporary method renames.

[jamesr, 2013-04-09 21:01:57]

/public/ lgtm

[acolwell GONE FROM CHROMIUM, 2013-04-09 21:08:40]

Committed patchset #6 manually as r147968 (presubmit successful).