124470 2013-11-17 14:24:56 -0800 [WTF] Media time should not have a constructor which accepts a single int or float. 2013-11-19 12:11:46 -0800 1 1 1 Unclassified WebKit Media 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 124594 1 jer.noble jer.noble benjamin cmarcelo commit-queue darin eric.carlson glenn oldest_to_newest 951439 0 jer.noble 2013-11-17 14:24:56 -0800 [WTF] Media time should not have a constructor which accepts a single int or float. 951440 1 217152 jer.noble 2013-11-17 14:29:10 -0800 Created attachment 217152 Patch 951441 2 217153 jer.noble 2013-11-17 14:34:14 -0800 Created attachment 217153 Patch Now with correct ChangeLogs. 951704 3 217153 eric.carlson 2013-11-18 11:10:35 -0800 Comment on attachment 217153 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". 951736 4 jer.noble 2013-11-18 11:57:37 -0800 Committed r159443: <http://trac.webkit.org/changeset/159443> 952090 5 217153 darin 2013-11-19 09:14:52 -0800 Comment on attachment 217153 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. 952105 6 jer.noble 2013-11-19 09:36:41 -0800 (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. 952106 7 darin 2013-11-19 09:40:55 -0800 (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? 952109 8 jer.noble 2013-11-19 09:44:16 -0800 (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. 952153 9 darin 2013-11-19 11:46:43 -0800 (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. 952170 10 jer.noble 2013-11-19 12:11:46 -0800 (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. 217152 2013-11-17 14:29:10 -0800 2013-11-17 14:34:12 -0800 Patch bug-124470-20131117142909.patch text/plain 2929 jer.noble U3VidmVyc2lvbiBSZXZpc2lvbjogMTU5MzY4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IGMxYWY0Y2QzOTZjMjcxZGY5MjY3NGQ2 ZmU3NzU3Yzk2MzcxY2IxY2YuLmE0ZmEwNDNlNjYyMjk1MjBlYzM4ZTJhZjAyNzY0NTcwOWRjOTA2 ZWMgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMTUgQEAKKzIwMTMtMTEtMTcgIEplciBOb2JsZSAgPGplci5ub2Js ZUBhcHBsZS5jb20+CisKKyAgICAgICAgV0lQOiBNZWRpYVRpbWUgc2hvdWxkIG5vdCBoYXZlIGEg ZGVmYXVsdCBjb25zdHJ1Y3RvciB3aGljaCB0YWtlcyBhIHNpbmdsZSBudW1iZXIuCisgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMjQ0NzAKKworICAgICAg ICBIYXZpbmcgYSBjb25zdHJ1Y3RvciB0YWtpbmcgYSBzaW5nbGUgbnVtYmVyIHZhbHVlLCBhcyBp dCdzIHZlcnkgZWFzeSAgdG8gYWNjaWRlbnRpYWxseSBtaXMtCisgICAgICAgIGluaXRpYWxpemUg YSBNZWRpYVRpbWUgd2l0aCBhIGRvdWJsZSAoYXV0b21hdGljYWxseSBjYXN0ZWQgdG8gYSBpbnQ2 NF90KS4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAq IHd0Zi9NZWRpYVRpbWUuaDoKKwogMjAxMy0xMS0xNSAgSmVyIE5vYmxlICA8amVyLm5vYmxlQGFw cGxlLmNvbT4KIAogICAgICAgICBNZWRpYVRpbWUgYWRkaXRpb24gYW5kIHN1YnRyYWN0aW9uIG9w ZXJhdG9ycyBoYXZlIGVycm9ycyB3aGVuIHRoZSByaHMgaXMgaW5maW5pdGUuCmRpZmYgLS1naXQg YS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5k ZXggMjc1MTNmZDgzYjE3ZDhlNjJjYjZjYWMwZGIzM2Q1NzYxMDFlMDRmOS4uYzIzMWMwNTc3Zjcz NGI2NjkwYTg0MWI4ZjZmMTdhZjYwYWE3OTYxNSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUv Q2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBA CisyMDEzLTExLTE3ICBKZXIgTm9ibGUgIDxqZXIubm9ibGVAYXBwbGUuY29tPgorCisgICAgICAg IFdJUDogTWVkaWFUaW1lIHNob3VsZCBub3QgaGF2ZSBhIGRlZmF1bHQgY29uc3RydWN0b3Igd2hp Y2ggdGFrZXMgYSBzaW5nbGUgbnVtYmVyLgorCisgICAgICAgIFtXVEZdIE1lZGlhIHRpbWUgc2hv dWxkIG5vdCBoYXZlIGEgY29uc3RydWN0b3Igd2hpY2ggYWNjZXB0cyBhIHNpbmdsZSBpbnQgb3Ig ZmxvYXQuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0x MjQ0NzAKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBG aXggdGhlIGNvbXBpbGUgZXJyb3IgZXhwb3NlZCBieSByZW1vdmluZyB0aGUgZGVmYXVsdCBwYXJh bWV0ZXIgaW4gdGhlIE1lZGlhVGltZSBjb25zdHJ1Y3Rvci4KKworICAgICAgICAqIE1vZHVsZXMv bWVkaWFzb3VyY2UvU291cmNlQnVmZmVyLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlNvdXJjZUJ1 ZmZlcjo6c2V0VGltZXN0YW1wT2Zmc2V0KToKKwogMjAxMy0xMS0xNSAgSmVyIE5vYmxlICA8amVy Lm5vYmxlQGFwcGxlLmNvbT4KIAogICAgICAgICBbTWFjXVtBVkZdIEFsbG93IHZpZGVvIGFuZCBh dWRpbyB0cmFja3MgdG8gYmUgaW5pdGlhbGl6ZWQgd2l0aCBhbiBBVkFzc2V0VHJhY2suCmRpZmYg LS1naXQgYS9Tb3VyY2UvV1RGL3d0Zi9NZWRpYVRpbWUuaCBiL1NvdXJjZS9XVEYvd3RmL01lZGlh VGltZS5oCmluZGV4IDM2NzY0YWQ5ZTRkNDQ3ZjIxOWM2NTc4ODEwNjJhNDE3ZjRhNWZjZjMuLmFm NjU2MWFlMzRmMDQ4ZTQxMmUyMGUwMGQ0ODQxYmQ4OGQ4Zjk1NWIgMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9XVEYvd3RmL01lZGlhVGltZS5oCisrKyBiL1NvdXJjZS9XVEYvd3RmL01lZGlhVGltZS5oCkBA IC01MCw3ICs1MCw3IEBAIHB1YmxpYzoKICAgICB9OwogCiAgICAgTWVkaWFUaW1lKCk7Ci0gICAg TWVkaWFUaW1lKGludDY0X3QgdmFsdWUsIGludDMyX3Qgc2NhbGUgPSBEZWZhdWx0VGltZVNjYWxl LCB1aW50MzJfdCBmbGFncyA9IFZhbGlkKTsKKyAgICBNZWRpYVRpbWUoaW50NjRfdCB2YWx1ZSwg aW50MzJfdCBzY2FsZSwgdWludDMyX3QgZmxhZ3MgPSBWYWxpZCk7CiAgICAgTWVkaWFUaW1lKGNv bnN0IE1lZGlhVGltZSYgcmhzKTsKICAgICB+TWVkaWFUaW1lKCk7CiAKZGlmZiAtLWdpdCBhL1Nv dXJjZS9XZWJDb3JlL01vZHVsZXMvbWVkaWFzb3VyY2UvU291cmNlQnVmZmVyLmNwcCBiL1NvdXJj ZS9XZWJDb3JlL01vZHVsZXMvbWVkaWFzb3VyY2UvU291cmNlQnVmZmVyLmNwcAppbmRleCAyYzA2 OTA1NTA1ODBlODdmMWMyNzAxODIxY2EwYWFiYzk5ZmMxMDg0Li5jYjY3OGY1NGYyYWNiMjZhYzFk ZmNmZTgxZjcyOTg3NmYxNTkzMWI5IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9Nb2R1bGVz L21lZGlhc291cmNlL1NvdXJjZUJ1ZmZlci5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvTW9kdWxl cy9tZWRpYXNvdXJjZS9Tb3VyY2VCdWZmZXIuY3BwCkBAIC0xNjAsNyArMTYwLDcgQEAgdm9pZCBT b3VyY2VCdWZmZXI6OnNldFRpbWVzdGFtcE9mZnNldChkb3VibGUgb2Zmc2V0LCBFeGNlcHRpb25D b2RlJiBlYykKICAgICB9CiAKICAgICAvLyA2LiBVcGRhdGUgdGhlIGF0dHJpYnV0ZSB0byB0aGUg bmV3IHZhbHVlLgotICAgIG1fdGltZXN0YW1wT2Zmc2V0ID0gb2Zmc2V0OworICAgIG1fdGltZXN0 YW1wT2Zmc2V0ID0gTWVkaWFUaW1lOjpjcmVhdGVXaXRoRG91YmxlKG9mZnNldCk7CiB9CiAKIHZv aWQgU291cmNlQnVmZmVyOjphcHBlbmRCdWZmZXIoUGFzc1JlZlB0cjxBcnJheUJ1ZmZlcj4gZGF0 YSwgRXhjZXB0aW9uQ29kZSYgZWMpCg== 217153 2013-11-17 14:34:14 -0800 2013-11-19 09:14:52 -0800 Patch bug-124470-20131117143413.patch text/plain 2838 jer.noble U3VidmVyc2lvbiBSZXZpc2lvbjogMTU5MzY4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IGMxYWY0Y2QzOTZjMjcxZGY5MjY3NGQ2 ZmU3NzU3Yzk2MzcxY2IxY2YuLjU1NTdjYWViZGNiZTNmZjJlNmYzMTMzM2RjNzc4NzQyNmY5ZDQ3 NWIgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMTUgQEAKKzIwMTMtMTEtMTcgIEplciBOb2JsZSAgPGplci5ub2Js ZUBhcHBsZS5jb20+CisKKyAgICAgICAgW1dURl0gTWVkaWEgdGltZSBzaG91bGQgbm90IGhhdmUg YSBjb25zdHJ1Y3RvciB3aGljaCBhY2NlcHRzIGEgc2luZ2xlIGludCBvciBmbG9hdC4KKyAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEyNDQ3MAorCisgICAg ICAgIEhhdmluZyBhIGNvbnN0cnVjdG9yIHRha2luZyBhIHNpbmdsZSBudW1iZXIgdmFsdWUsIGFz IGl0J3MgdmVyeSBlYXN5ICB0byBhY2NpZGVudGlhbGx5IG1pcy0KKyAgICAgICAgaW5pdGlhbGl6 ZSBhIE1lZGlhVGltZSB3aXRoIGEgZG91YmxlIChhdXRvbWF0aWNhbGx5IGNhc3RlZCB0byBhIGlu dDY0X3QpLgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAg ICogd3RmL01lZGlhVGltZS5oOgorCiAyMDEzLTExLTE1ICBKZXIgTm9ibGUgIDxqZXIubm9ibGVA YXBwbGUuY29tPgogCiAgICAgICAgIE1lZGlhVGltZSBhZGRpdGlvbiBhbmQgc3VidHJhY3Rpb24g b3BlcmF0b3JzIGhhdmUgZXJyb3JzIHdoZW4gdGhlIHJocyBpcyBpbmZpbml0ZS4KZGlmZiAtLWdp dCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpp bmRleCAyNzUxM2ZkODNiMTdkOGU2MmNiNmNhYzBkYjMzZDU3NjEwMWUwNGY5Li5jZmFiOTNkZTgx NmI4OGYzNzc3YWM4NTBhMTk3OTk1MmU5ZmY5MjRlIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTUg QEAKKzIwMTMtMTEtMTcgIEplciBOb2JsZSAgPGplci5ub2JsZUBhcHBsZS5jb20+CisKKyAgICAg ICAgW1dURl0gTWVkaWEgdGltZSBzaG91bGQgbm90IGhhdmUgYSBjb25zdHJ1Y3RvciB3aGljaCBh Y2NlcHRzIGEgc2luZ2xlIGludCBvciBmbG9hdC4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtp dC5vcmcvc2hvd19idWcuY2dpP2lkPTEyNDQ3MAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9E WSAoT09QUyEpLgorCisgICAgICAgIEZpeCB0aGUgY29tcGlsZSBlcnJvciBleHBvc2VkIGJ5IHJl bW92aW5nIHRoZSBkZWZhdWx0IHBhcmFtZXRlciBpbiB0aGUgTWVkaWFUaW1lIGNvbnN0cnVjdG9y LgorCisgICAgICAgICogTW9kdWxlcy9tZWRpYXNvdXJjZS9Tb3VyY2VCdWZmZXIuY3BwOgorICAg ICAgICAoV2ViQ29yZTo6U291cmNlQnVmZmVyOjpzZXRUaW1lc3RhbXBPZmZzZXQpOgorCiAyMDEz LTExLTE1ICBKZXIgTm9ibGUgIDxqZXIubm9ibGVAYXBwbGUuY29tPgogCiAgICAgICAgIFtNYWNd W0FWRl0gQWxsb3cgdmlkZW8gYW5kIGF1ZGlvIHRyYWNrcyB0byBiZSBpbml0aWFsaXplZCB3aXRo IGFuIEFWQXNzZXRUcmFjay4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XVEYvd3RmL01lZGlhVGltZS5o IGIvU291cmNlL1dURi93dGYvTWVkaWFUaW1lLmgKaW5kZXggMzY3NjRhZDllNGQ0NDdmMjE5YzY1 Nzg4MTA2MmE0MTdmNGE1ZmNmMy4uYWY2NTYxYWUzNGYwNDhlNDEyZTIwZTAwZDQ4NDFiZDg4ZDhm OTU1YiAxMDA2NDQKLS0tIGEvU291cmNlL1dURi93dGYvTWVkaWFUaW1lLmgKKysrIGIvU291cmNl L1dURi93dGYvTWVkaWFUaW1lLmgKQEAgLTUwLDcgKzUwLDcgQEAgcHVibGljOgogICAgIH07CiAK ICAgICBNZWRpYVRpbWUoKTsKLSAgICBNZWRpYVRpbWUoaW50NjRfdCB2YWx1ZSwgaW50MzJfdCBz Y2FsZSA9IERlZmF1bHRUaW1lU2NhbGUsIHVpbnQzMl90IGZsYWdzID0gVmFsaWQpOworICAgIE1l ZGlhVGltZShpbnQ2NF90IHZhbHVlLCBpbnQzMl90IHNjYWxlLCB1aW50MzJfdCBmbGFncyA9IFZh bGlkKTsKICAgICBNZWRpYVRpbWUoY29uc3QgTWVkaWFUaW1lJiByaHMpOwogICAgIH5NZWRpYVRp bWUoKTsKIApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvTW9kdWxlcy9tZWRpYXNvdXJjZS9T b3VyY2VCdWZmZXIuY3BwIGIvU291cmNlL1dlYkNvcmUvTW9kdWxlcy9tZWRpYXNvdXJjZS9Tb3Vy Y2VCdWZmZXIuY3BwCmluZGV4IDJjMDY5MDU1MDU4MGU4N2YxYzI3MDE4MjFjYTBhYWJjOTlmYzEw ODQuLmNiNjc4ZjU0ZjJhY2IyNmFjMWRmY2ZlODFmNzI5ODc2ZjE1OTMxYjkgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9XZWJDb3JlL01vZHVsZXMvbWVkaWFzb3VyY2UvU291cmNlQnVmZmVyLmNwcAorKysg Yi9Tb3VyY2UvV2ViQ29yZS9Nb2R1bGVzL21lZGlhc291cmNlL1NvdXJjZUJ1ZmZlci5jcHAKQEAg LTE2MCw3ICsxNjAsNyBAQCB2b2lkIFNvdXJjZUJ1ZmZlcjo6c2V0VGltZXN0YW1wT2Zmc2V0KGRv dWJsZSBvZmZzZXQsIEV4Y2VwdGlvbkNvZGUmIGVjKQogICAgIH0KIAogICAgIC8vIDYuIFVwZGF0 ZSB0aGUgYXR0cmlidXRlIHRvIHRoZSBuZXcgdmFsdWUuCi0gICAgbV90aW1lc3RhbXBPZmZzZXQg PSBvZmZzZXQ7CisgICAgbV90aW1lc3RhbXBPZmZzZXQgPSBNZWRpYVRpbWU6OmNyZWF0ZVdpdGhE b3VibGUob2Zmc2V0KTsKIH0KIAogdm9pZCBTb3VyY2VCdWZmZXI6OmFwcGVuZEJ1ZmZlcihQYXNz UmVmUHRyPEFycmF5QnVmZmVyPiBkYXRhLCBFeGNlcHRpb25Db2RlJiBlYykK