Created attachment 254326[details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 254328[details]
Archive of layout-test-results from ews101 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 254320[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=254320&action=review
This is a great start! I'm not sure we're handling the undefined case right yet, but this looks close!
> Source/WebCore/html/HTMLMediaElement.cpp:876
> + return nanMedia.toDouble();
Does this get handled as NaN? We might need to look at the generated code to see what it's doing.
> Source/WebCore/platform/graphics/MediaPlayerPrivate.h:97
> + virtual MediaTime getStartDate() { return MediaTime::createWithDouble( currentTime() ); }
We don't use spacing around embedded functions like 'currentTime' here.
> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.h:210
> + virtual MediaTime getStartDate();
This should be 'MediaTime getStartDate() override;'
> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:726
> + return MediaTime::createWithDouble( date );
No spaces around 'date' here.
> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:728
> + double curr_time = CMTimeGetSeconds([m_avPlayerItem currentTime]) * 1000;
Our naming convention is camelCase, and very few truncated words. So, currentTime is better.
> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:729
> + printf("Date: %f\nTime: %f\n", date, curr_time);
This should be a LOG call, rather than a bare printf.
> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:730
> + return MediaTime::createWithDouble( date - curr_time );
No spaces here.
> LayoutTests/media/video-controller-getStartDate.html:17
> + testExpected("video.getStartDate()", null);
Do we expect null here? Or NaN? I thought the specification said NaN.
> LayoutTests/media/video-controller-getStartDate.html:26
> + <p>Test that getStartDate() returns appropriate NaN or date.</p>
NaN and null are not the same thing! :)
Attachment 254400[details] did not pass style-queue:
ERROR: LayoutTests/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5]
Total errors found: 1 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 254401[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=254401&action=review
This looks great! I think many of these could be 'const'. I also would like to see that bindings test we talked about.
But I think this is pretty close to done!
> Source/WebCore/ChangeLog:5
> + could inform users about the original date, etc.
This should just be a one-line "title". This paragraph should be placed between the "Test" line and the "*bindings/js/JSDOMBindings.cpp" line.
> Source/WebCore/html/HTMLMediaElement.cpp:878
> +double HTMLMediaElement::getStartDate()
This should be 'const'
> Source/WebCore/html/HTMLMediaElement.h:181
> + virtual double getStartDate();
This should be 'const'
> Source/WebCore/platform/graphics/MediaPlayerPrivate.h:97
> + virtual MediaTime getStartDate() { return MediaTime::createWithDouble(currentTime()); }
This should be 'const'
> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.h:210
> + MediaTime getStartDate() override;
This should be 'const'
> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:730
> +
Whoops! Extra space.
Comment on attachment 254401[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=254401&action=review>> Source/WebCore/platform/graphics/MediaPlayerPrivate.h:97
>> + virtual MediaTime getStartDate() { return MediaTime::createWithDouble(currentTime()); }
>
> This should be 'const'
Shouldn't this return NaN by default?
> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:726
> + double date = [[m_avPlayerItem currentDate] timeIntervalSince1970] * 1000;
> + if (!date)
> + return MediaTime::createWithDouble(date);
Shouldn't this return NaN?
> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:729
> + double curr_time = CMTimeGetSeconds([m_avPlayerItem currentTime]) * 1000;
> + return MediaTime::createWithDouble(date-curr_time);
Is this right? The startDate is the "current timeline offset", which is defined as:
Some video files also have an explicit date and time corresponding to the zero time
in the media timeline, known as the timeline offset. Initially, the timeline offset
must be set to Not-a-Number (NaN).
so why doesn't this just return MediaTime::createWithDouble(date)?
Comment on attachment 254401[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=254401&action=review
I don’t think there’s enough test coverage here.
I’m going to say review- because Eric thinks the "date-curr_time" thing is wrong. If that turns out to be mistaken then we could get it reviewed again.
> Source/WebCore/bindings/js/JSDOMBinding.cpp:121
> +JSC::JSValue jsDateOrNaN(ExecState* exec, double value)
> +{
> + if (std::isnan(value))
> + return jsDoubleNumber(value);
> + return jsDateOrNull(exec, value);
> +}
This seems a little strange to add. Will this really be such a common pattern that we need to support it in the bindings generator? Does WebIDL specifically allow for this now?
JSC::JSValue is wrong in this file, I think. Should just be JSValue.
> Source/WebCore/bindings/js/JSDOMBinding.h:321
> +JSC::JSValue jsDateOrNaN(JSC::ExecState*, double);
The two functions below each have a comment. Seems we should add that comment for this too. And if this is going to be the less common idiom, then it should come *after* jsDateOrNull.
Since this is a new function, we should probably make it take a JSC::ExecState& instead of a pointer. Easy to accommodate that in the bindings generator by including a "*" and more forward looking. In newer WebCore code we always use references for things like this that can’t be null.
The name of this function isn’t quite right, because this function can return a Date object, a NaN, or a null! Although I assume that getStartDate won’t ever be returning a null.
> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3899
> + my $conv = $signature->extendedAttributes->{"TreatReturnedNaNDateAs"};
It’s really ugly to have to add an attribute like this. We try to avoid these as much as possible. I don’t see an obvious alternative but it’s a annoying to be adding this.
> Source/WebCore/html/HTMLMediaElement.cpp:882
> + double timeOption = round(m_player->getStartDate().toDouble());
> + if (!timeOption)
> + return std::numeric_limits<double>::quiet_NaN();
I think there should be some kind of comment here explaining why we are translating a zero into a NaN. Is zero not a legal value?
> Source/WebCore/html/HTMLMediaElement.idl:68
> + [TreatReturnedNaNDateAs=NaN] Date getStartDate();
I think this is a really strange type for an attribute and doesn’t seem like something that would be in a new modern standard. Could you please double check that “either a Date object or a number with NaN value” is what we want here?
>>> Source/WebCore/platform/graphics/MediaPlayerPrivate.h:97
>>> + virtual MediaTime getStartDate() { return MediaTime::createWithDouble(currentTime()); }
>>
>> This should be 'const'
>
> Shouldn't this return NaN by default?
Is it OK to leave all the other back ends returning sort of “nonsense” values? Returning a wrong start date might be worse than not having the function at all. Maybe we have to make the binding conditional so the function will only appear when it’s going to work. But also, don’t we use multiple back ends in the Mac and/or iOS ports?
>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:726
>> + return MediaTime::createWithDouble(date);
>
> Shouldn't this return NaN?
I’d like to understand more about when exactly this is zero. We actually get back an object from currentDate that returns 0 when we call timIntervalSince1970 on it?
> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:728
> + double curr_time = CMTimeGetSeconds([m_avPlayerItem currentTime]) * 1000;
We never name local variables in this style. It should be currentTime, not cure_time.
>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:729
>> + return MediaTime::createWithDouble(date-curr_time);
>
> Is this right? The startDate is the "current timeline offset", which is defined as:
>
> Some video files also have an explicit date and time corresponding to the zero time
> in the media timeline, known as the timeline offset. Initially, the timeline offset
> must be set to Not-a-Number (NaN).
>
> so why doesn't this just return MediaTime::createWithDouble(date)?
The formatting here is also incorrect. We put spaces around operators like "-".
For reference:
The specification defines "getStarteDate()" in <http://www.w3.org/TR/2014/CR-html5-20140204/embedded-content-0.html>.
"Some video files also have an explicit date and time corresponding to the zero time in the media timeline, known as the timeline offset. Initially, the timeline offset must be set to Not-a-Number (NaN).
The getStartDate() method must return a new Date object representing the current timeline offset."
So this expects NaN, at least in the pre-playing video state.
Comment on attachment 254401[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=254401&action=review>> Source/WebCore/bindings/js/JSDOMBinding.cpp:121
>> +}
>
> This seems a little strange to add. Will this really be such a common pattern that we need to support it in the bindings generator? Does WebIDL specifically allow for this now?
>
> JSC::JSValue is wrong in this file, I think. Should just be JSValue.
That's my fault, Darin. I told Matt to create this binding because the specification specifically requires NaN return values in certain cases, and our default date bindings always convert those to JavaScript Nulls.
>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3899
>> + my $conv = $signature->extendedAttributes->{"TreatReturnedNaNDateAs"};
>
> It’s really ugly to have to add an attribute like this. We try to avoid these as much as possible. I don’t see an obvious alternative but it’s a annoying to be adding this.
We should probably talk to Anders or Sam about a better approach as a future (larger scale) enhancement. This seemed to be in keeping with existing coding style.
>> Source/WebCore/html/HTMLMediaElement.idl:68
>> + [TreatReturnedNaNDateAs=NaN] Date getStartDate();
>
> I think this is a really strange type for an attribute and doesn’t seem like something that would be in a new modern standard. Could you please double check that “either a Date object or a number with NaN value” is what we want here?
Perhaps my reading of 4.7.10.6 of the spec is wrong, but I think it states that NaN is needed at least in the case of pre-playback state.
Comment on attachment 254401[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=254401&action=review>> Source/WebCore/html/HTMLMediaElement.cpp:882
>> + return std::numeric_limits<double>::quiet_NaN();
>
> I think there should be some kind of comment here explaining why we are translating a zero into a NaN. Is zero not a legal value?
Actually I don't think we should translate to NaN here at all. Lets make it the responsibility of the media engine to return a valid Date (as a double) or NaN.
>>>> Source/WebCore/platform/graphics/MediaPlayerPrivate.h:97
>>>> + virtual MediaTime getStartDate() { return MediaTime::createWithDouble(currentTime()); }
>>>
>>> This should be 'const'
>>
>> Shouldn't this return NaN by default?
>
> Is it OK to leave all the other back ends returning sort of “nonsense” values? Returning a wrong start date might be worse than not having the function at all. Maybe we have to make the binding conditional so the function will only appear when it’s going to work. But also, don’t we use multiple back ends in the Mac and/or iOS ports?
It is more nuanced than that - not all files support startDate so even ports with a backend that supports it will sometimes (usually) return NaN because NaN is a signal that the file doesn't have a startDate.
Having said that, making the binding conditional with a build flag seems like a good idea so ports with a backend that doesn't support it won't have the method at all.
Created attachment 254516[details]
Archive of layout-test-results from ews102 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 254519[details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 254524[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=254524&action=review> Source/WebCore/bindings/js/JSDOMBinding.h:321
> +// Returns a Date instnace for the specified value, or NaN if the date is not a number.
Typo in "instance" here.
Confusing wording "if the date is not a number"; should say value.
If the value passed is infinity, does this return null? Given that, why doesn’t the comment say that?
> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:731
> + return MediaTime::createWithDouble(round(date - currentTime));
Why round this? Why not just leave it fractional?
Created attachment 254529[details]
Archive of layout-test-results from ews103 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 254530[details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 254524[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=254524&action=review
This is really close, but r- for now because the the test will be flakey as written.
> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:726
> + // No live streams were made during the epoch (1970). 0 is a default value returned if the system can't find a start time
Minor nit: I would prefer to see a blank line before this comment.
Also, "0 is a default value returned if the system can't find a start time" isn't quite correct, something like "AVFoundation returns 0 if the media file doesn't have a start date" would be better.
>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:731
>> + return MediaTime::createWithDouble(round(date - currentTime));
>
> Why round this? Why not just leave it fractional?
I agree.
> LayoutTests/http/tests/media/hls/video-controller-getStartDate.html:1
> +<html>
Minor nit: might as well include a doctype: <!DOCTYPE html>
> LayoutTests/http/tests/media/hls/video-controller-getStartDate.html:23
> + setTimeout(function() {
> + testExpected("video.getStartDate()", "Wed Nov 03 2010 01:00:00 GMT-0700 (PDT)");
> + endTest();
> + }, 1000);
Running your tests after a timeout is not a good idea - without a sufficiently long timeout you will sometimes fail, but you will usually waste time because the element will be ready before the timer fires. It is better to listen for one of the HTMLMediaElement state change events so your test runs as soon as possible. Something like this (written in the browser and not tested):
var canPlayThroughCount = 0;
function canPlayThrough()
{
consoleWrite("EVENT(canplaythrough)");
if (++canPlayThroughCount == 2)
testGetStartDate()
}
function start()
{
video = document.getElementById("source_without_start_date");
video.src = "../resources/hls/test-vod.m3u8";
video.addEventListener('canplaythrough', canPlayThrough, true);
video = document.getElementById('source_with_start_date');
video.src = "../resources/hls/test-vod-date-time.m3u8";
video.addEventListener('canplaythrough', canPlayThrough, true);
}
function testGetStartDate()
{
video = document.getElementById("source_without_start_date");
testExpected("isNaN(video.getStartDate())", true );
video = document.getElementById('source_with_start_date');
testExpected("video.getStartDate()", "Wed Nov 03 2010 01:00:00 GMT-0700 (PDT)");
endTest();
}
> LayoutTests/http/tests/media/hls/video-controller-getStartDate.html:30
> + <video id="source_with_start_date" autoplay="autoplay" controls="controls" width="640" height="480"></video>
> + <video id="source_without_start_date" autoplay="autoplay" controls="controls" width="640" height="480"></video>
Nit: 'controls' and 'autoplay' are boolean attributes so they don't need values.
Created attachment 254584[details]
Archive of layout-test-results from ews102 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 254588[details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 254582[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=254582&action=review
This looks good, thanks for being responsive to all of the feedback! Marking r+, but you will need to fix the build break and test results before landing.
> LayoutTests/http/tests/media/hls/video-controller-getStartDate.html:2
> +<!DOCTYPE html>
> + <head>
You need an <html> element here.
Created attachment 255319[details]
Archive of layout-test-results from ews103 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 255320[details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 255533[details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Created attachment 255535[details]
Archive of layout-test-results from ews101 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 255670[details]
Archive of layout-test-results from ews102 for mac-mavericks
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 255671[details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Hello,
It looks very odd to return directly "NaN".
I think we are meant to return a valid Date object anyway, even if the timeline offset is unknown.
It is valid JavaScript to initialize a Date object with NaN
> new Date(NaN);
Also I think it would fit our existing layout test
> isNaN(new Date(NaN)); // should return true
What do you think?
(In reply to comment #71)
>
> It looks very odd to return directly "NaN".
>
> I think we are meant to return a valid Date object anyway, even if the
> timeline offset is unknown.
>
>
> It is valid JavaScript to initialize a Date object with NaN
> > new Date(NaN);
>
> Also I think it would fit our existing layout test
> > isNaN(new Date(NaN)); // should return true
>
> What do you think?
The spec says [1]:
The getStartDate() method must return a new Date object representing
the current timeline offset.
where "a new Date object" is defined as:
When this specification requires a user agent to create a Date object
representing a particular time (which could be the special value
Not-a-Number) ...
so I think you are correct, thanks!
I will be happy to review a patch if you have time to prepare one.
[1] http://www.w3.org/TR/html5/embedded-content-0.html#dom-media-getstartdate
[2] http://www.w3.org/TR/html5/infrastructure.html#create-a-date-object
Comment on attachment 255677[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=255677&action=review> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3937
> + my $conv = $signature->extendedAttributes->{"TreatReturnedNaNDateAs"};
This is not part of Web IDL. When do we ever want to return null? We should not be adding new WebKit-specific IDL extended attributes unless strictly needed.
2015-06-04 17:43 PDT, Matthew Daiter
2015-06-04 18:23 PDT, Build Bot
2015-06-04 18:35 PDT, Build Bot
2015-06-05 14:19 PDT, Matthew Daiter
2015-06-05 17:35 PDT, Matthew Daiter
2015-06-05 17:46 PDT, Matthew Daiter
2015-06-08 15:09 PDT, Matthew Daiter
2015-06-08 15:50 PDT, Build Bot
2015-06-08 15:59 PDT, Build Bot
2015-06-08 16:57 PDT, Matthew Daiter
2015-06-08 17:00 PDT, Matthew Daiter
2015-06-08 17:48 PDT, Build Bot
2015-06-08 17:50 PDT, Build Bot
2015-06-09 11:05 PDT, Matthew Daiter
2015-06-09 11:52 PDT, Build Bot
2015-06-09 12:23 PDT, Build Bot
2015-06-19 13:28 PDT, Matthew Daiter
2015-06-19 15:15 PDT, Matthew Daiter
2015-06-21 01:12 PDT, Matthew Daiter
2015-06-21 04:03 PDT, Build Bot
2015-06-21 04:10 PDT, Build Bot
2015-06-24 16:42 PDT, Matthew Daiter
2015-06-24 17:32 PDT, Build Bot
2015-06-24 17:53 PDT, Build Bot
2015-06-26 15:06 PDT, Matthew Daiter
2015-06-26 15:46 PDT, Build Bot
2015-06-26 15:50 PDT, Build Bot
2015-06-26 15:59 PDT, Matthew Daiter
2015-06-26 17:08 PDT, Matthew Daiter