Bug 124594 – [WTF] Clean up the constructors in MediaTime

• Status: NEW
• Component: Web Template Framework

[Jer Noble]

In bug #124470, many options for cleaning up the constructors in MediaTime were discussed.  This bug will continue that discussion.

[Darin Adler]

Since we think the units of the arguments to the MediaTime constructor are unclear, I suggest we make the actual constructors private and have the public construction be through named functions that make the argument types clearer. It seems like we already mostly use that pattern; I suggest going the rest of the way. Step 1: Make constructors private and find out what fails. Step 2: Add named public static member functions that are easy to read at each of those call sites.

[Jer Noble]

Created attachment 217324 [details]
Patch

[Darin Adler]

Comment on attachment 217324 [details]
Patch

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

> Source/WebCore/svg/animation/SMILTimeContainer.cpp:134
> -    ASSERT(!m_beginTime);
> +//    ASSERT(!m_beginTime);

?

[Jer Noble]

(In reply to comment #3)
> (From update of attachment 217324 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217324&action=review
> 
> > Source/WebCore/svg/animation/SMILTimeContainer.cpp:134
> > -    ASSERT(!m_beginTime);
> > +//    ASSERT(!m_beginTime);
> 
> ?

Whoops. This was a local workaround for an ASSERT crash.  Does not belong.

[Jer Noble]

Created attachment 217327 [details]
Patch for landing

[Ahmad Saleem]

Checking by BugID, it seems this r+ patch didn't landed from Webkit GitHub and I also tried to look one change in in MediaTime.cpp and confirmed that this didn't landed.

https://github.com/WebKit/WebKit/blob/72f5e43aa81dcd59f4ad7e44bacf66e6ecf5b0db/Source/WTF/wtf/MediaTime.cpp#L140

Do we need this? Thanks!