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

Checking instanceof Arrays cross frames (needed for the vjs.Player.prototype.src method) #1218

Closed
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@shmulik
Contributor

shmulik commented May 18, 2014

vjs.Player.prototype.src uses the instanceof Array method.
This will not work for arrays created on another frame,
Instead of instanceof we should use Array.isArray, but this is not supported on IE8.
So vjs.obj.isArray shim was created for the isArray method.

shmulik added some commits May 18, 2014

Checking instanceof Arrays cross frames. fix #1195
instanceof Array, will not work on arrays created cross frames.
Array,isArray is the method to check if an object isArray across frames.
IE8 does not suppoort the Array.isArray method, so we have to create a shim to support all browsers.
Checking instanceof Arrays cross frames. fix #1195
instanceof Array, will not work on arrays created cross frames.
Array,isArray is the method to check if an object isArray across frames.
IE8 does not suppoort the Array.isArray method, so we have to create a shim to support all browsers.
Show outdated Hide outdated src/js/lib.js
* @private
*/
vjs.obj.isArray = Array.isArray || function(arr) {
return Object.prototype.toString.call(arr) === "[object Array]";

This comment has been minimized.

@gkatsev

gkatsev May 18, 2014

Member

You need to use single quotes, which is why the build failed on travis.

@gkatsev

gkatsev May 18, 2014

Member

You need to use single quotes, which is why the build failed on travis.

Show outdated Hide outdated src/js/lib.js
*/
vjs.obj.isArray = Array.isArray || function(arr) {
return Object.prototype.toString.call(arr) === "[object Array]";
};

This comment has been minimized.

@gkatsev

gkatsev May 18, 2014

Member

You should add a new line between here and the next comment.

@gkatsev

gkatsev May 18, 2014

Member

You should add a new line between here and the next comment.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev May 18, 2014

Member

Other than that, looks good.

Are there any other cases of foo instanceof Array in the code base? Maybe hunt those down as well?

Member

gkatsev commented May 18, 2014

Other than that, looks good.

Are there any other cases of foo instanceof Array in the code base? Maybe hunt those down as well?

@mmcc mmcc added bug labels May 19, 2014

@shmulik

This comment has been minimized.

Show comment
Hide comment
@shmulik

shmulik May 19, 2014

Contributor

Thanks @gkatsev.
So I guess I need to make another pull request....
I did not find another instanceof Array in the code base

Contributor

shmulik commented May 19, 2014

Thanks @gkatsev.
So I guess I need to make another pull request....
I did not find another instanceof Array in the code base

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev May 19, 2014

Member

So I guess I need to make another pull request....

Nope. Just do another commit to the same branch and this PR will pick it up and re-run the build.

I did not find another instanceof Array in the code base

Cool. That's fine.

Member

gkatsev commented May 19, 2014

So I guess I need to make another pull request....

Nope. Just do another commit to the same branch and this PR will pick it up and re-run the build.

I did not find another instanceof Array in the code base

Cool. That's fine.

Checking instanceof Arrays cross frames. fix #1195
vjs.Player.prototype.src uses the instanceof Array method.
This will not work for arrays created on another frame,
Instead of instanceof we should use Array.isArray, but this is not supported on IE8.
So vjs.obj.isArray shim was created for the isArray method.
@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff May 19, 2014

Member

Great stuff! Thanks @shmulik.

We do need to add tests to finish this off, but that shouldn't be too difficult for this. The tests would be added to the test/unit/lib.js file.

You can follow how some of the other tests are written to get started. I think we would need at least two assertions here.

  • Check that an array is appropriately identified as an array
  • Check that an object is not identified as an array

Would you be up for doing that?

Member

heff commented May 19, 2014

Great stuff! Thanks @shmulik.

We do need to add tests to finish this off, but that shouldn't be too difficult for this. The tests would be added to the test/unit/lib.js file.

You can follow how some of the other tests are written to get started. I think we would need at least two assertions here.

  • Check that an array is appropriately identified as an array
  • Check that an object is not identified as an array

Would you be up for doing that?

@heff heff removed the needs: addressing label May 19, 2014

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev May 19, 2014

Member

Can you also do the same replacement for the instanceof Array here.

While we're at it, we also probably want to do a similar thing to objects:

} else if (source instanceof Object) {

Member

gkatsev commented May 19, 2014

Can you also do the same replacement for the instanceof Array here.

While we're at it, we also probably want to do a similar thing to objects:

} else if (source instanceof Object) {

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff May 19, 2014

Member

Good find.

we also probably want to do a similar thing to objects

should we just be using typeof source === 'object' there instead?

Member

heff commented May 19, 2014

Good find.

we also probably want to do a similar thing to objects

should we just be using typeof source === 'object' there instead?

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev May 19, 2014

Member

no, because if source is null, it will fail.
We also want to do the toString.call trick for objects as well.

Member

gkatsev commented May 19, 2014

no, because if source is null, it will fail.
We also want to do the toString.call trick for objects as well.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff May 19, 2014

Member

We also want to do the toString.call trick for objects as well

Except that won't work if the source object is a source element.

Object.prototype.toString.call(document.createElement('source'));
--> "[object HTMLSourceElement]"

Either way, if @shmulik doesn't want to take on the object piece for this specific issue we can create a new issue for.

Member

heff commented May 19, 2014

We also want to do the toString.call trick for objects as well

Except that won't work if the source object is a source element.

Object.prototype.toString.call(document.createElement('source'));
--> "[object HTMLSourceElement]"

Either way, if @shmulik doesn't want to take on the object piece for this specific issue we can create a new issue for.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev May 19, 2014

Member

Yep, that can be done in a different commit. The component piece, might as well be done here as well :D

Member

gkatsev commented May 19, 2014

Yep, that can be done in a different commit. The component piece, might as well be done here as well :D

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff May 19, 2014

Member

Yeah, the instance of Array in Component should be updated here

Member

heff commented May 19, 2014

Yeah, the instance of Array in Component should be updated here

@shmulik

This comment has been minimized.

Show comment
Hide comment
@shmulik

shmulik May 20, 2014

Contributor

Sorry for my late response, I am on a different time zone.....
I will take care for the instanceof Array in component.js.(missed it somehow)
And will take a look at using a similar trick for objects

Contributor

shmulik commented May 20, 2014

Sorry for my late response, I am on a different time zone.....
I will take care for the instanceof Array in component.js.(missed it somehow)
And will take a look at using a similar trick for objects

Checking instanceof Arrays cross frames. fix #1195
Using the instanceof Array method will not work for arrays created on another frame,
Instead of instanceof we should use Array.isArray, but this is not supported on IE8.
So vjs.obj.isArray shim was created for the isArray method.
@shmulik

This comment has been minimized.

Show comment
Hide comment
@shmulik

shmulik May 20, 2014

Contributor

I will also do the tests for the isArray, but only for arrays created on the same frame. Is this good enough?

Contributor

shmulik commented May 20, 2014

I will also do the tests for the isArray, but only for arrays created on the same frame. Is this good enough?

shmulik added some commits May 20, 2014

Testing the vjs.obj.isArray #1195
Adding test for the vjs.obj.isArray method
Testing the vjs.obj.isArray #1195
Added test for the vjs.obj.isArray method.
Testing the vjs.obj.isArray #1195
Added test for the vgs.obj.isArray
@shmulik

This comment has been minimized.

Show comment
Hide comment
@shmulik

shmulik May 20, 2014

Contributor

@gkatsev , @heff

What do you say about the following code for the Object trick:

vjs.obj.isObject = function(obj) {
return Object.prototype.toString.call(obj) === '[object Object];
};

vjs.obj.isSourceObject = function(src) {
return vjs.obj.isObject(src) || Object.prototype.toString.call(src) === '[object HTMLSourceElement]'
}

Contributor

shmulik commented May 20, 2014

@gkatsev , @heff

What do you say about the following code for the Object trick:

vjs.obj.isObject = function(obj) {
return Object.prototype.toString.call(obj) === '[object Object];
};

vjs.obj.isSourceObject = function(src) {
return vjs.obj.isObject(src) || Object.prototype.toString.call(src) === '[object HTMLSourceElement]'
}

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff May 20, 2014

Member

Yeah, same frame is fine.


Steve Heffernan (mobile)

On May 19, 2014, at 9:04 PM, shmulik notifications@github.com wrote:

I will also do the tests for the isArray, but only for arrays created on the same frame. Is this good enough?


Reply to this email directly or view it on GitHub.

Member

heff commented May 20, 2014

Yeah, same frame is fine.


Steve Heffernan (mobile)

On May 19, 2014, at 9:04 PM, shmulik notifications@github.com wrote:

I will also do the tests for the isArray, but only for arrays created on the same frame. Is this good enough?


Reply to this email directly or view it on GitHub.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff May 20, 2014

Member

We need to think a little more on where and how we'd use the object one because I don't think it's the right fit for that specific instance.


Steve Heffernan (mobile)

On May 19, 2014, at 10:19 PM, shmulik notifications@github.com wrote:

@gkatsev , @heff

What do you say about the following code for the Object trick:

vjs.obj.isObject = function(obj) {
return Object.prototype.toString.call(obj) === '[object Object];
};

vjs.obj.isSourceObject = function(src) {
return vjs.obj.isObject(src) || Object.prototype.toString.call(src) === '[object HTMLSourceElement]'
}


Reply to this email directly or view it on GitHub.

Member

heff commented May 20, 2014

We need to think a little more on where and how we'd use the object one because I don't think it's the right fit for that specific instance.


Steve Heffernan (mobile)

On May 19, 2014, at 10:19 PM, shmulik notifications@github.com wrote:

@gkatsev , @heff

What do you say about the following code for the Object trick:

vjs.obj.isObject = function(obj) {
return Object.prototype.toString.call(obj) === '[object Object];
};

vjs.obj.isSourceObject = function(src) {
return vjs.obj.isObject(src) || Object.prototype.toString.call(src) === '[object HTMLSourceElement]'
}


Reply to this email directly or view it on GitHub.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Jun 12, 2014

Member

Merged in. Thanks!

Member

heff commented Jun 12, 2014

Merged in. Thanks!

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