New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flash tech duration getter returns Infinity instead of negative value #3128

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@vit-koumar

vit-koumar commented Feb 22, 2016

Description

Flash tech duration getter should return Infinity instead of negative values (which some players do return) as stated in HTML standard.

This situation is solved in upper layer Player object, but sometimes duration getter is called from inside the Flash tech for example here which situation ends for example in inability of setting currentTime when duration is negative.

Specific Changes proposed

Explicit duration() function defined in js/tech/flash.js which returns Infinity instead of negative Number.

Requirements Checklist

I will adapt the rest of project in case this change will be approved.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Feb 22, 2016

Member

Should this do a check to see if we have media enabled? Because the spec you linked says that a negative value can mean that there's no media. Otherwise, seems like a good change.

Member

gkatsev commented Feb 22, 2016

Should this do a check to see if we have media enabled? Because the spec you linked says that a negative value can mean that there's no media. Otherwise, seems like a good change.

@vit-koumar

This comment has been minimized.

Show comment
Hide comment
@vit-koumar

vit-koumar Feb 22, 2016

I think the spec states that if no media is loaded, then the return value must be NaN. Do you think there should be a check for media load and if no media is loaded, then the return value should be NaN regardless of the duration returned by the player?

vit-koumar commented Feb 22, 2016

I think the spec states that if no media is loaded, then the return value must be NaN. Do you think there should be a check for media load and if no media is loaded, then the return value should be NaN regardless of the duration returned by the player?

@vit-koumar

This comment has been minimized.

Show comment
Hide comment
@vit-koumar

vit-koumar Feb 24, 2016

So I changed the condition like if the readyState === 0 then it returns NaN (according to the standrd). Hope this is what you ment.

vit-koumar commented Feb 24, 2016

So I changed the condition like if the readyState === 0 then it returns NaN (according to the standrd). Hope this is what you ment.

Show outdated Hide outdated src/js/tech/flash.js
if (this.readyState() === 0) {
return NaN;
} else {
var duration = this.el_.vjs_getProperty('duration');

This comment has been minimized.

@gkatsev

gkatsev Mar 7, 2016

Member

This should be a let instead.

@gkatsev

gkatsev Mar 7, 2016

Member

This should be a let instead.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Mar 7, 2016

Member

One final small change, LGTM otherwise. Thanks.

Member

gkatsev commented Mar 7, 2016

One final small change, LGTM otherwise. Thanks.

@vit-koumar

This comment has been minimized.

Show comment
Hide comment
@vit-koumar

vit-koumar Mar 17, 2016

Thanks for the comment, I have amended the change. Is there anything else to do before merge, eg. some tests, docs, etc. ?

vit-koumar commented Mar 17, 2016

Thanks for the comment, I have amended the change. Is there anything else to do before merge, eg. some tests, docs, etc. ?

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Mar 17, 2016

Member

Adding unit tests would be great!

Member

gkatsev commented Mar 17, 2016

Adding unit tests would be great!

@vit-koumar

This comment has been minimized.

Show comment
Hide comment
@vit-koumar

vit-koumar Mar 22, 2016

Unit tests added.

vit-koumar commented Mar 22, 2016

Unit tests added.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev May 3, 2016

Member

Hey, looks like this branch got into conflict with master, can you rebase or merge in master and fix the conflicts?

Member

gkatsev commented May 3, 2016

Hey, looks like this branch got into conflict with master, can you rebase or merge in master and fix the conflicts?

@gkatsev gkatsev added the needs: LGTM label May 3, 2016

@vit-koumar

This comment has been minimized.

Show comment
Hide comment
@vit-koumar

vit-koumar May 11, 2016

Rebased, conflict solved (+changed commit messages to conform your style)

vit-koumar commented May 11, 2016

Rebased, conflict solved (+changed commit messages to conform your style)

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev May 11, 2016

Member

Awesome, thanks @vit-koumar.

Member

gkatsev commented May 11, 2016

Awesome, thanks @vit-koumar.

@BrandonOCasey

This comment has been minimized.

Show comment
Hide comment
@BrandonOCasey

BrandonOCasey Jun 21, 2016

Contributor

LGTM

Contributor

BrandonOCasey commented Jun 21, 2016

LGTM

@gkatsev gkatsev added confirmed and removed needs: LGTM labels Jul 18, 2016

@gkatsev gkatsev closed this in f36da27 Jul 18, 2016

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Jul 18, 2016

Member

Thanks @vit-koumar! Should have a release soon.

Member

gkatsev commented Jul 18, 2016

Thanks @vit-koumar! Should have a release soon.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Jul 20, 2016

Member

@vit-koumar hey, looks like there are some errors on IE. Would you be able to take a look? See https://travis-ci.org/videojs/video.js/builds/145613240

Member

gkatsev commented Jul 20, 2016

@vit-koumar hey, looks like there are some errors on IE. Would you be able to take a look? See https://travis-ci.org/videojs/video.js/builds/145613240

@vit-koumar

This comment has been minimized.

Show comment
Hide comment
@vit-koumar

vit-koumar Jul 20, 2016

Yes, I will have a look.

vit-koumar commented Jul 20, 2016

Yes, I will have a look.

}
};
result = duration.call(mockFlash);
ok(Number.isNaN(result), 'duration returns NaN when readyState equals 0');

This comment has been minimized.

@gkatsev

gkatsev Jul 22, 2016

Member

Looks like this and Number.isFinite below the two things causing issues. I'll fix it by including es6-shim in the test builds.

@gkatsev

gkatsev Jul 22, 2016

Member

Looks like this and Number.isFinite below the two things causing issues. I'll fix it by including es6-shim in the test builds.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Jul 22, 2016

Member

@vit-koumar hey, I fixed it. See #3453.

Member

gkatsev commented Jul 22, 2016

@vit-koumar hey, I fixed it. See #3453.

@vit-koumar

This comment has been minimized.

Show comment
Hide comment
@vit-koumar

vit-koumar Jul 22, 2016

Nice, thanks!

vit-koumar commented Jul 22, 2016

Nice, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment