Various Prototype-related fixes.

[metaweta, 2008-11-17 16:49:37]

http://codereview.appspot.com/8674/diff/209/14
File src/com/google/caja/cajita.js (right):

http://codereview.appspot.com/8674/diff/209/14#newcode135
Line 135: i < 0 && (i += len);
Is this line supposed to force it into the range [0, len)?  If so, then if i <
-len, it won't work.

http://codereview.appspot.com/8674/diff/209/14#newcode150
Line 150: i < 0 && (i += this.length);
Ditto

[ben, 2008-11-18 11:09:18]

http://codereview.appspot.com/8674/diff/209/14
File src/com/google/caja/cajita.js (right):

http://codereview.appspot.com/8674/diff/209/14#newcode135
Line 135: i < 0 && (i += len);
On 2008/11/17 16:49:37, metaweta wrote:
> Is this line supposed to force it into the range [0, len)?  If so, then if i <
> -len, it won't work.

It wasn't the intention, but re-reading the spec, I see I have not really
implemented it correctly...

http://codereview.appspot.com/8674/diff/209/14#newcode135
Line 135: i < 0 && (i += len);
On 2008/11/17 16:49:37, metaweta wrote:
> Is this line supposed to force it into the range [0, len)?  If so, then if i <
> -len, it won't work.

It was not the intention, but re-reading the spec, I see this is not correctly
implemented anyway

[metaweta, 2008-11-18 20:36:00]

http://codereview.appspot.com/8674/diff/604/409
File src/com/google/caja/cajita.js (right):

http://codereview.appspot.com/8674/diff/604/409#newcode138
Line 138: i < 0 && (i += len);
i < 0 && (i = Math.max(0, i + len))

http://codereview.appspot.com/8674/diff/604/409#newcode140
Line 140: if (i in this && identical(this[i], specimen)) {
Why check "i in this"?  The only time it would be relevant is if
Array.prototype.indexOf was applied to an object rather than an array; does the
spec address that issue?

http://codereview.appspot.com/8674/diff/604/409#newcode162
Line 162: i += len;
i = Math.max(0, i + len);

http://codereview.appspot.com/8674/diff/604/409#newcode163
Line 163: } else if(i >= len) {
space between "if" and paren

[ihab.awad, 2008-11-18 20:54:31]

LGTM; some comments.

http://codereview.appspot.com/8674/diff/604/409
File src/com/google/caja/cajita.js (right):

http://codereview.appspot.com/8674/diff/604/409#newcode133
Line 133: if(!Array.prototype.indexOf) {
If we're going to rely on indexOf and lastIndexOf -- i.e. if they are part of
our TCB -- we should override them unconditionally. Conversely, if they are not
part of our TCB, why are we bothering to override them in cajita.js? I vote we
just make the overriding unconditional.

http://codereview.appspot.com/8674/diff/604/409#newcode138
Line 138: i < 0 && (i += len);
If the magnitude of 'i' is greater than 'len', this function could be subverted
to iterate over negative indices of the array. This seems harmless, but -- maybe
setting a lower bound of zero on 'i' at this point may be helpful? At the very
least, the caller can set a very large negative number for 'i' and trick this
function into a "soft" DoS attack.

http://codereview.appspot.com/8674/diff/604/409#newcode157
Line 157: if (isNaN(i)) {
So folks should explicitly pass in NaN to mean "infinity"? Hm. Ok.

http://codereview.appspot.com/8674/diff/604/408
File src/com/google/caja/plugin/domita.js (right):

http://codereview.appspot.com/8674/diff/604/408#newcode463
Line 463: 
Extra blank line.

[metaweta, 2008-11-18 21:22:08]

http://codereview.appspot.com/8674/diff/415/612
File src/com/google/caja/cajita.js (right):

http://codereview.appspot.com/8674/diff/415/612#newcode132
Line 132: * Reference:
https://developer.mozilla.org/En/Core_JavaScript_1.5_Reference:Objects:Array:...
That page can't possibly be correct, since I get the following behavior on FF:

y={"-10":4, 0:5, 1:6, length:2}
// [object Object]
Array.prototype.indexOf.call(y, 6, -1000)
// 1
Array.prototype.indexOf.call(y, 5, -1000)
// 0
Array.prototype.indexOf.call(y, 4, -1000)
// -1

[ben, 2008-11-19 04:27:18]

http://codereview.appspot.com/8674/diff/604/409
File src/com/google/caja/cajita.js (right):

http://codereview.appspot.com/8674/diff/604/409#newcode138
Line 138: i < 0 && (i += len);
On 2008/11/18 20:36:00, metaweta wrote:
> i < 0 && (i = Math.max(0, i + len))

For this, and all your other comments, I am following the spec at
https://developer.mozilla.org/En/Core_JavaScript_1.5_Reference:Objects:Array:...,
I will add a comment to that effect.

[ben, 2008-11-19 16:44:40]

http://codereview.appspot.com/8674/diff/604/409
File src/com/google/caja/cajita.js (right):

http://codereview.appspot.com/8674/diff/604/409#newcode140
Line 140: if (i in this && identical(this[i], specimen)) {
On 2008/11/18 20:36:00, metaweta wrote:
> Why check "i in this"?  The only time it would be relevant is if
> Array.prototype.indexOf was applied to an object rather than an array; does
the
> spec address that issue?

Indeed ES3.1 specifically says this can be used on objects. There is also a
comment remarking that the check for "in this" is missing.

http://codereview.appspot.com/8674/diff/604/409#newcode157
Line 157: if (isNaN(i)) {
On 2008/11/18 20:54:31, ihab.awad wrote:
> So folks should explicitly pass in NaN to mean "infinity"? Hm. Ok.

undefined isNaN.

[metaweta, 2008-11-20 20:54:11]

LGTM

[Jasvir, 2008-11-20 23:14:45]

LGTM

http://codereview.appspot.com/8674/diff/422/624
File src/com/google/caja/cajita.js (right):

http://codereview.appspot.com/8674/diff/422/624#newcode144
Line 144: i = (i < 0) ? Math.ceil(i) : Math.floor(i);
Thank you.  LGTM

[Jasvir, 2009-05-19 23:13:16]

This appears to have been submit.  Please close this issue.