refactor(indexOf) use Array.prototype.indexOf exclusively

[realityking]

This saves around 30 bytes in the minified build for the main file.

[caitp]

I think perf-tests would be good here. Native implementations are good because they can be smart when dealing with sparse arrays (theoretically), but it would still be better to measure it.

I don't think 30 bytes is that big of a deal, though.

[realityking]

The native version was already used when available (this can be seen here: https://github.com/angular/angular.js/pull/8847/files#diff-1d54c5f722aebc473dbe96f836ddf974L658), so there should not be a performance difference.

I agree though, 30 bytes is no big deal. I only discovered the amount after I was trough with the changes. Since it's (IMO) cleaner code, I still wanted to submit it.

[caitp]

The native version was already used when available (this can be seen here: https://github.com/angular/angular.js/pull/8847/files#diff-1d54c5f722aebc473dbe96f836ddf974L658), so there should not be a performance difference.

You'd be surprised, we'd get one fewer [[Call]] this way. At any rate, measurements are much more important than 30 bytes (I expect we'll see a benefit from the measurements, so there should be a good reason to ditch indexOf() anyways) --- Make sure to test array-like objects as well as sparse and non-sparse variants

[realityking]

Here's a jsperf test case: http://jsperf.com/indexof-angularjs-vs-native/3

Performance is within the margin of error the same, except for small arrays and Chrome. Here the native function is much faster - no idea why.

However, what I hadn't considered is, that the current function also accepts array like objects. This doesn't seem to be used in any of the tests cases - is it really necessary?

[caitp]

However, what I hadn't considered is, that the current function also accepts array like objects. This doesn't seem to be used in any of the tests cases - is it really necessary?

jQuery / jqLite objects are really array-like objects, I'm not sure if we're using indexOf when searching them though.

[caitp]

Anyways, the last thing is this: removing the function is a breaking change to the public API. We're probably better off not using it, but I don't think we can really remove it, unless Igor says it's alright

[realityking]

Why is this considered public API? It's not shared on the angular object. I don't think there's any way to access it from outside this repository.

[caitp]

Yeah, you're right. I thought it was exported. Either way, still needs a second opinion

[thebigredgeek]

I think it might be a good idea to simply leverage a simple inversion of control here. Instead of invoking indexOf directly, it might be best to simply write a wrapper function that uses indexOf, or some other algorithm depending on what is the most performant solution on a given user agent. Thoughts?

[caitp]

indexOf is going to be O(n) pretty much regardless, so I don't think we're going to get major wins using alternate implementations --- if we see that a particular UA is doing really badly, it would be better to just tell them, with metrics, and explain that it's a problem.

I think this is unlikely to be a major performance issue since we're already leveraging it where possible anyways, and throwing away the polyfill is probably in our best interest. WDYT @IgorMinar

[thebigredgeek]

@caitp I definitely agree with cutting down on bloat heh. The leaner the better IMO

[IgorMinar]

Lgtm

[petebacondarwin]

Doh! It seems to be failing on Jenkins in browserTrigger and a $location spec with Firefox 31 on OS/X

[realityking]

Can I See the output someplace?

[caitp]

Wait, ie8 has Array.prototype.indexOf?

[caitp]

According to http://kangax.github.io/compat-table/es5/, no it does not ._.

[petebacondarwin]

My comment about 1.2.x branch was on the incorrect PR

[caitp]

My comment about 1.2.x branch was on the incorrect PR

You scared me for a second there =)