Closed Bug 1355756 Opened 2 years ago Closed 2 years ago

Change the type of MediaData::mDuration to TimeUnit

Categories

(Core :: Audio/Video: Playback, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

No description provided.
Assignee: nobody → jwwang
Blocks: 1245019
Priority: -- → P3
Attachment #8858699 - Flags: review?(jyavenard)
Attachment #8858700 - Flags: review?(jyavenard)
Attachment #8858701 - Flags: review?(jyavenard)
Attachment #8858699 - Flags: review?(jyavenard) → review?(gsquelart)
Attachment #8858700 - Flags: review?(jyavenard) → review?(gsquelart)
Attachment #8858701 - Flags: review?(jyavenard) → review?(gsquelart)
Comment on attachment 8858699 [details]
Bug 1355756. P1 - change the type of MediaData::mDuration to TimeUnit.

https://reviewboard.mozilla.org/r/130708/#review133292
Attachment #8858699 - Flags: review?(gsquelart) → review+
Comment on attachment 8858700 [details]
Bug 1355756. P2 - let VideoData::UpdateDuration() take TimeUnit.

https://reviewboard.mozilla.org/r/130710/#review133294
Attachment #8858700 - Flags: review?(gsquelart) → review+
Comment on attachment 8858701 [details]
Bug 1355756. P3 - let CreateAndCopyData() and its friends take TimeUnit for duration.

https://reviewboard.mozilla.org/r/130712/#review133296
Attachment #8858701 - Flags: review?(gsquelart) → review+
Thanks for the review!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2534b8ae23de
P1 - change the type of MediaData::mDuration to TimeUnit. r=gerald
https://hg.mozilla.org/integration/autoland/rev/c077fb83e09b
P2 - let VideoData::UpdateDuration() take TimeUnit. r=gerald
https://hg.mozilla.org/integration/autoland/rev/58c6ac2e352c
P3 - let CreateAndCopyData() and its friends take TimeUnit for duration. r=gerald
A reminder to myself, and a heads-up for you&me&everybody:

It would be nice(r) if the commit descriptions also contained the "why" for the changes, e.g. in this case something like "TimeUnit gives better source readability, and compile-time contextual checks, than a naked int64_t to record millisecond durations".

Sorry that I didn't catch this in past reviews. In the future, I will try and enforce this, and I would invite you to do the same. ;-)
I know it's a bit more work, but hopefully the extra few minutes needed to write the longer descriptions will be useful to future readers&maintainers of our code.

See https://goo.gl/AkK7jl for some discussions as to why this is useful.
You need to log in before you can comment on or make changes to this bug.