Not yet in the spec draft, but got approval at the last F2F. Spec at https://github.com/domenic/Array.prototype.contains/blob/master/spec.md Implementation should be very straight-forward, essentially duplicating most of indexOf[1]. Tests could also essentially duplicate the ones for indexOf, with the exception of having to add tests for the behavior around NaNs. [1]: http://mxr.mozilla.org/mozilla-central/source/js/src/builtin/Array.js?rev=bb579e3de64b#6

Oh, I forgot: we don't have an intrinsic for SameValueZero. The best course of action here would be to self-host that, too, and add it to builtins/Utilities.js. Just inlining the check would work, too, though.

Pretty darn comprehensive test262 tests are already written: https://github.com/tc39/test262/pull/95 Reference implementation at https://github.com/domenic/Array.prototype.contains/blob/master/reference-implementation/index.js

Comment on attachment 8491350 [details] [diff] [review] bug1069063.patch Review of attachment 8491350 [details] [diff] [review]: ----------------------------------------------------------------- Do we need to move Array/contains.js to tests/ecma_7

Comment on attachment 8491350 [details] [diff] [review] bug1069063.patch Review of attachment 8491350 [details] [diff] [review]: ----------------------------------------------------------------- This looks great, thanks! Before landing it though, we should figure out how to deal with the test262 tests Domenic mentioned in comment 2. I posted to the JS internals mailing list about this[1], so let's see what other people think. For now, can you import the tests locally by putting them into a subfolder of tests/test262 and make sure that they all pass? [1] https://groups.google.com/forum/#!topic/mozilla.dev.tech.js-engine.internals/VmjEa2ATcNo ::: js/src/builtin/Array.js @@ +602,5 @@ > + return false; > + > + // Step 9. > + var k; > + if (n >= 0) Nit: We add braces for all branches if at least one requires them. Since the `else` branch does, please add them here. @@ +619,5 @@ > + // Steps a-b. > + var elementK = O[k]; > + > + // Step c. > + if (SameValueZero(searchElement, elementK)) I wouldn't mind if you inlined elementK here, and made steps a-c one thing. ::: js/src/builtin/Utilities.js @@ +155,5 @@ > // Math.pow(2, 53) - 1 = 0x1fffffffffffff > return v < 0x1fffffffffffff ? v : 0x1fffffffffffff; > } > > +/* Spec: ECMAScript Draft, 6 edition Aug 24, 2014, 7.2.4 */ Nits: use line comment, change "6 edition" to "6th edition", and add a "." at the end. (Yes, this file is really inconsistent about comment formatting, but this is as close to the usual style in self-hosting as it gets.) @@ +157,5 @@ > } > > +/* Spec: ECMAScript Draft, 6 edition Aug 24, 2014, 7.2.4 */ > +function SameValueZero(x, y) { > + return x !== x && y !== y || x === y Neat solution. Only small quibble: please switch the order of the || operands around; the NaN case should be far less likely. I know that just affects cases where `x === y` actually holds, which is one at most per call of `[].contains`, but still. Also, nit: semicolon missing.

> +function SameValueZero(x, y) { > + return x !== x && y !== y || x === y Also, please parenthesize around this, and add the optional semicolon: return x === y || (x !== x && y !== y);

(In reply to Till Schneidereit [:till] from comment #5) > Before landing it though, we should figure out how to deal with the test262 > tests Domenic mentioned in comment 2. I posted to the JS internals mailing > list about this[1], so let's see what other people think. > > For now, can you import the tests locally by putting them into a subfolder > of tests/test262 and make sure that they all pass? tests/test262 should largely be considered read-only. It's generally a pain to have an import-folder that also has modifications made to it, that must be preserved across updates. I'd really, really, rather you not change tests/test262 at all. (The only case we've ever done it was to modify existing tests that a change on our end deliberately broke. Those changes are recorded in the old-but-currently-dysfunctional update-test262.sh script. It's on my todo list to update that to work again, soon.)

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7) > tests/test262 should largely be considered read-only. It's generally a pain > to have an import-folder that also has modifications made to it, that must > be preserved across updates. I'd really, really, rather you not change > tests/test262 at all. Sorry, I didn't mean to imply that the tests should be *landed* there. I meant putting them there in a WIP patch for testing purposes to make sure the implementation passes them. Putting them under the test262 folder makes them pick up the harness stuff required, so it's easiest. I really didn't make that very clear.

(In reply to Till Schneidereit [:till] from comment #5) > For now, can you import the tests locally by putting them into a subfolder > of tests/test262 and make sure that they all pass? Some tests failed, seems like due to our jstests.py doesn't support "negative:" flag? https://github.com/tc39/test262/issues/94

Yes, we don't support @negative (or negative: if they changed the format on us, maybe). Thus far we've worked around this by just skipping such tests in js/src/tests/jstests.list. Long run we won't, but it was a quick and dirty way to get many tests imported, even if not all could run.

Comment on attachment 8491980 [details] [diff] [review] bug-1069063-v2.patch Review of attachment 8491980 [details] [diff] [review]: ----------------------------------------------------------------- Great, thanks. The patch is entirely ok, but I'll still cancel review instead r+'ing for now: we need to figure out what to do with the test262 tests, and I have to talk to other people about whether we want to make this Nightly-only for now. My guess would be "yes". I'll get back to you once I have an answer to these questions.

`Array.prototype.contains.length` should be 1, did I miss that?

(In reply to John-David Dalton from comment #13) > `Array.prototype.contains.length` should be 1, did I miss that? I'm not sure I follow. The proposed spec says "The length property of the contains method is 1", and that's what we implement. Check line 12 in the test file. Perhaps the line "JS_SELF_HOSTED_FN("contains", "ArrayContains", 2,0)" is what's confusing here? That's just an artifact of how our function definition works: the number of arguments at that point has to include default arguments, so it's not necessarily what'll be reflected by Function#length.

Comment on attachment 8491980 [details] [diff] [review] bug-1069063-v2.patch (In reply to Till Schneidereit [:till] from comment #12) > we need to figure out what to do with the test262 > tests Waldo agrees that we should just leave them out for now. We know that we pass the ones our harness would support (and have no reason whatsoever to assume we wouldn't pass the others), so getting them once they land in test262 proper is enough. > I have to talk to other people about whether we want to make this > Nightly-only for now. My guess would be "yes". I'll get back to you once I > have an answer to these questions. Waldo agrees again: this should be Nightly-only for now. I'll add the required #ifdef to jsarray.cpp and feature test to contains.js before landing this.

> I'm not sure I follow. The proposed spec says "The length property of the contains method is 1", and that's what we implement. No worries, the default param threw me off.

Oh, same issue here as in bug 1062860: please confirm that you're ok with the test's license being changed to https://creativecommons.org/publicdomain/zero/1.0/ so I can land this. (Except if you're explicitly not ok with that for some reason, of course. In that case, please just say that.)

(In reply to Till Schneidereit [:till] from comment #17) > Oh, same issue here as in bug 1062860: please confirm that you're ok with > the test's license being changed to > https://creativecommons.org/publicdomain/zero/1.0/ so I can land this. > (Except if you're explicitly not ok with that for some reason, of course. In > that case, please just say that.) Of course I'm ok with that!

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/2c92e43e29d8 (In reply to ziyunfei from comment #18) > Of course I'm ok with that! Great, thanks for the quick confirmation.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c902ebaa8520 for https://tbpl.mozilla.org/php/getParsedLog.php?id=48543781&tree=Mozilla-Inbound and to let you guys figure out whether https://tbpl.mozilla.org/php/getParsedLog.php?id=48543667&tree=Mozilla-Inbound was from this or from bug 1062860.

securityAudit=bholley. Please update the test and make sure to make it conditional on the right things so that the test doesn't go orange when merged to aurora/beta/release.

And re-landed. Thanks, Bobby! https://hg.mozilla.org/integration/mozilla-inbound/rev/178df0290ee1

(In reply to Till Schneidereit [:till] from comment #22) > And re-landed. Thanks, Bobby! > > https://hg.mozilla.org/integration/mozilla-inbound/rev/178df0290ee1 Till, it seems this changeset used the legacy code from patch v1.

(In reply to ziyunfei from comment #23) > (In reply to Till Schneidereit [:till] from comment #22) > > And re-landed. Thanks, Bobby! > > > > https://hg.mozilla.org/integration/mozilla-inbound/rev/178df0290ee1 > > Till, it seems this changeset used the legacy code from patch v1. s/legacy code/obsolete code/ Sorry for my bad english.

Urgh, thanks for noticing! Follow-up pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/15aebfcdbde5

https://hg.mozilla.org/mozilla-central/rev/2c92e43e29d8 https://hg.mozilla.org/mozilla-central/rev/15aebfcdbde5

Unfortunately, we have to back this out for now because of bug 1075059. There is some hope that we'll be able to re-land it sometime down the road, when MooTools is less widely (or at least less *prominently*) used on the Web. It's also possible that TC39 will pick a different name for the method, and then we'd be able to land it right away. We'll see.

At the November TC39 meeting we agreed to rename to Array.prototype.includes to dodge this issue. (String.prototype.contains also needs to be renamed, as IE had compat problems with it.) Anyone interested in re-landing under the new name? :D Spec updated at https://github.com/domenic/Array.prototype.includes

Comment on attachment 8525790 [details] [diff] [review] rename `contains` to `includes` Review of attachment 8525790 [details] [diff] [review]: ----------------------------------------------------------------- Great, thank you for doing this! Domenic, requesting feedback from you to verify that the rename is the only change here. If it is, just give an f+ and we'll land this.

Comment on attachment 8526368 [details] [diff] [review] Fix up test_xrayToJS.xul Review of attachment 8526368 [details] [diff] [review]: ----------------------------------------------------------------- We still want Domenic's feedback here.

Yep, that was the only change. Although in the process of code review over on V8 we found a couple redundant algorithm steps that you might be interested in: https://github.com/tc39/Array.prototype.includes/commit/4550016b71f13dd5097e2c4ac9d2962a494f41a4?short_path=958e727#diff-958e7270f96f5407d7d980f500805b1b They do not affect observable behavior though.

Comment on attachment 8526368 [details] [diff] [review] Fix up test_xrayToJS.xul Review of attachment 8526368 [details] [diff] [review]: ----------------------------------------------------------------- Great, thanks for the feedback, Domenic. ::: js/src/builtin/Array.js @@ +596,5 @@ > + > + // Steps 6-7. > + var n = ToInteger(fromIndex); > + > + // Step 8. As Domenic points out, this step is redundant (if it's removed, the loop in step 11 will have 0 iterations in the affected cases). Please remove it and fix the following steps' numbering. @@ +608,5 @@ > + } > + // Step 10. > + else { > + // Step a. > + k = len + n; We already do this the way the spec does it now, too :)

Got an unrelated failure. https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=119d4fb5b66e

Followup to actually make this Nightly-only as stated on dev-platform: https://hg.mozilla.org/integration/mozilla-inbound/rev/160459853756

Ms2ger, this looks documented to me... https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/includes