Bug 124470 – [WTF] Media time should not have a constructor which accepts a single int or float.

• Component: Media
• Status: RESOLVED FIXED

[Jer Noble]

[WTF] Media time should not have a constructor which accepts a single int or float.

[Jer Noble]

Created attachment 217152 [details]
Patch

[Jer Noble]

Created attachment 217153 [details]
Patch

Now with correct ChangeLogs.

[Eric Carlson]

Comment on attachment 217153 [details]
Patch

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

> Source/WTF/ChangeLog:6
> +        Having a constructor taking a single number value, as it's very easy  to accidentially mis-

Nit: extra space between "easy" and "to".

[Jer Noble]

Committed r159443: <http://trac.webkit.org/changeset/159443>

[Darin Adler]

Comment on attachment 217153 [details]
Patch

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

> Source/WTF/wtf/MediaTime.h:53
> -    MediaTime(int64_t value, int32_t scale = DefaultTimeScale, uint32_t flags = Valid);
> +    MediaTime(int64_t value, int32_t scale, uint32_t flags = Valid);

The other way to do this would be to just add “explicit”. You’d still get an error in code like what we had in SourceBuffer.cpp, yet the scale could continue to have a default value.

[Jer Noble]

(In reply to comment #5)
> (From update of attachment 217153 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217153&action=review
> 
> > Source/WTF/wtf/MediaTime.h:53
> > -    MediaTime(int64_t value, int32_t scale = DefaultTimeScale, uint32_t flags = Valid);
> > +    MediaTime(int64_t value, int32_t scale, uint32_t flags = Valid);
> 
> The other way to do this would be to just add “explicit”. You’d still get an error in code like what we had in SourceBuffer.cpp, yet the scale could continue to have a default value.

I considered that, but it would still mean that the following would be valid:

    double currentTime = video->currentTime();
    MediaTime currentMediaTime(currentTime);

Yet currentMediaTime wouldn't be 2.5seconds, but 2/defaultTimeScale seconds.

[Darin Adler]

(In reply to comment #6)
> I considered that, but it would still mean that the following would be valid:
> 
>     double currentTime = video->currentTime();
>     MediaTime currentMediaTime(currentTime);
> 
> Yet currentMediaTime wouldn't be 2.5seconds, but 2/defaultTimeScale seconds.

Yes, and this is valid even now:

    double currentTime = video->currentTime();
    MediaTime currentMediaTime(currentTime, 0);

I think maybe what we really want is a private constructor?

[Jer Noble]

(In reply to comment #7)
> (In reply to comment #6)
> > I considered that, but it would still mean that the following would be valid:
> > 
> >     double currentTime = video->currentTime();
> >     MediaTime currentMediaTime(currentTime);
> > 
> > Yet currentMediaTime wouldn't be 2.5seconds, but 2/defaultTimeScale seconds.
> 
> Yes, and this is valid even now:
> 
>     double currentTime = video->currentTime();
>     MediaTime currentMediaTime(currentTime, 0);

We should probably throw an ASSERT if you try to use 0 as a denominator in a MediaTime. :)

> I think maybe what we really want is a private constructor?

A private unimplemented constructor which took a double?  That might give the right kind of compile error when people (like me) accidentally cast a double to a MediaTime.

[Darin Adler]

(In reply to comment #8)
> > I think maybe what we really want is a private constructor?
> 
> A private unimplemented constructor which took a double?  That might give the right kind of compile error when people (like me) accidentally cast a double to a MediaTime.

No, all constructors except for copy constructors would be private. And we’d have named construction functions.

[Jer Noble]

(In reply to comment #9)
> (In reply to comment #8)
> > > I think maybe what we really want is a private constructor?
> > 
> > A private unimplemented constructor which took a double?  That might give the right kind of compile error when people (like me) accidentally cast a double to a MediaTime.
> 
> No, all constructors except for copy constructors would be private. And we’d have named construction functions.

Lets continue this discussion in http://webkit.org/b/124594.