Bug 124594

Summary: [WTF] Clean up the constructors in MediaTime
Product: WebKit Reporter: Jer Noble <jer.noble>
Component: Web Template FrameworkAssignee: Jer Noble <jer.noble>
Status: NEW    
Severity: Normal CC: ahmad.saleem792, benjamin, cmarcelo, commit-queue, darin, d-r, eric.carlson, fmalita, glenn, gyuyoung.kim, pdr, schenney
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 124470    
Bug Blocks:    
Attachments:
Description Flags
Patch
darin: review+, darin: commit-queue-
Patch for landing none

Jer Noble
Reported 2013-11-19 12:10:51 PST
In bug #124470, many options for cleaning up the constructors in MediaTime were discussed. This bug will continue that discussion.
Attachments
Patch (23.36 KB, patch)
2013-11-19 13:28 PST, Jer Noble 		darin: review+
darin: commit-queue-
DetailsFormatted DiffDiff
Patch for landing (22.72 KB, patch)
2013-11-19 13:41 PST, Jer Noble 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Darin Adler
Comment 1 2013-11-19 12:44:54 PST
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
Comment 2 2013-11-19 13:28:08 PST
Created attachment 217324 [details] Patch
Darin Adler
Comment 3 2013-11-19 13:35:17 PST
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
Comment 4 2013-11-19 13:37:58 PST
(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
Comment 5 2013-11-19 13:41:19 PST
Created attachment 217327 [details] Patch for landing
Ahmad Saleem
Comment 6 2022-10-10 12:11:41 PDT
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!
Note You need to log in before you can comment on or make changes to this bug.