Bug 124470 - [WTF] Media time should not have a constructor which accepts a single int or float.
Summary: [WTF] Media time should not have a constructor which accepts a single int or ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords:
Depends on:
Blocks: 124594
  Show dependency treegraph
 
Reported: 2013-11-17 14:24 PST by Jer Noble
Modified: 2013-11-19 12:11 PST (History)
6 users (show)

See Also:


Attachments
Patch (2.86 KB, patch)
2013-11-17 14:29 PST, Jer Noble
no flags Details | Formatted Diff | Diff
Patch (2.77 KB, patch)
2013-11-17 14:34 PST, Jer Noble
eric.carlson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2013-11-17 14:24:56 PST
[WTF] Media time should not have a constructor which accepts a single int or float.
Comment 1 Jer Noble 2013-11-17 14:29:10 PST
Created attachment 217152 [details]
Patch
Comment 2 Jer Noble 2013-11-17 14:34:14 PST
Created attachment 217153 [details]
Patch

Now with correct ChangeLogs.
Comment 3 Eric Carlson 2013-11-18 11:10:35 PST
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".
Comment 4 Jer Noble 2013-11-18 11:57:37 PST
Committed r159443: <http://trac.webkit.org/changeset/159443>
Comment 5 Darin Adler 2013-11-19 09:14:52 PST
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.
Comment 6 Jer Noble 2013-11-19 09:36:41 PST
(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.
Comment 7 Darin Adler 2013-11-19 09:40:55 PST
(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?
Comment 8 Jer Noble 2013-11-19 09:44:16 PST
(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.
Comment 9 Darin Adler 2013-11-19 11:46:43 PST
(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.
Comment 10 Jer Noble 2013-11-19 12:11:46 PST
(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.