Closed Bug 1102219 Opened 10 years ago Closed 9 years ago

Rename String.prototype.contains to String.prototype.includes

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: till, Assigned: arai)

References

(Blocks 1 open bug)

Details

(Keywords: addon-compat, dev-doc-complete, site-compat, Whiteboard: [DocArea=JS])

Attachments

(6 files, 10 obsolete files)

20.00 KB, patch
arai
: review+
Details | Diff | Splinter Review
3.11 KB, patch
till
: review+
Details | Diff | Splinter Review
328.49 KB, patch
arai
: review+
Details | Diff | Splinter Review
91.96 KB, patch
till
: review+
Details | Diff | Splinter Review
7.47 KB, patch
till
: review+
Details | Diff | Splinter Review
7.79 KB, patch
till
: review+
Details | Diff | Splinter Review
In bug 1075059 we discovered that `Array.prototype.contains` breaks the web. Not only is it desirable to have the equivalent String method have the same name, the IE team also reported web-compat issues with `String.prototype.contains`.

For these reasons, TC39 decided to rename `String.prototype.contains` to `includes`. (See bug 1069063 comment 29.)

This is going to be really hard to pull of: we've shipped `String.prototype.contains` since Firefox 18 and have lots of chrome and, presumably, addon usage. We might have to keep `contains` around as an alias with a warning for some releases, after cleaning up chrome code, of course.

Ziyunfei, would you be interested in taking this?
Flags: needinfo?(446240525)
Whiteboard: [DocArea=JS]
Assignee: nobody → 446240525
Flags: needinfo?(446240525)
Attachment #8527270 - Flags: review?(till)
Comment on attachment 8527269 [details] [diff] [review]
Add `includes`; keep `contains` around as an alias with a warning

Review of attachment 8527269 [details] [diff] [review]:
-----------------------------------------------------------------

Huh, I would've thought that we had JIT optimizations for String#contains. Not so, apparently; which is nice here, because otherwise we'd have to change lots more code.

This looks a  good except for the tiny nit below and slight changes to the commit message required. Please make it say
"Bug 1102219 - Part 1: Add `String.prototype.includes`; keep `String.prototype.contains` around as an alias with a warning. r=till"
(I.e., change the format slightly, be explicit about which builtin's methods you're talking about.)

::: js/src/jsstr.cpp
@@ +1598,5 @@
>      args.rval().setBoolean(StringMatch(text, searchStr, start) != -1);
>      return true;
>  }
>  
> +/* XXX TODO: remove String.prototype.contains (bug 1102219) */

Please file a follow-up bug depending on this one for the actual removal. Then, mention that here without the "XXX" at the beginning.
Attachment #8527269 - Flags: review?(till) → review+
Comment on attachment 8527270 [details] [diff] [review]
part 2 - Replace `contains` with `includes` in JS code

Review of attachment 8527270 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!

Please change the commit message to match the pattern from part 1.

Before we land these patches, it's important to clean up as much platform JS as possible, too. I think the easiest way to go about that is doing a full try run with three platforms: one desktop, one Android, one B2G. Then look at the full log files (which are huge :() and search for the warning. I can fully understand if you're not all that interested in doing that, of course. Every bit would help, though. Also, if you do the try run and post the link here, say which logs you're looking at and I'll pick some others to wade through, too.
Attachment #8527270 - Flags: review?(till) → review+
Blocks: 1103588
Attached patch change the commit message (obsolete) — Splinter Review
Attachment #8527269 - Attachment is obsolete: true
Attachment #8527301 - Flags: review+
change the commit message
Attachment #8527270 - Attachment is obsolete: true
Attachment #8527302 - Flags: review+
warning spew on "perfectly good, feature-tested, best practices code that polyfills only if the method doesn't exist" in firefox, until we actually remove this is also.. suboptimal (i know, nothing about this is close to "optimal").

maybe we can warn only in nightly+dev+beta, and hope that blog posts about .includes that mention .contains are going to get lots of pickup -- which, given the popularity of array.contains, looks likely.
Try run for "Part 1: Add `String.prototype.includes`; remove `String.prototype.contains` totally":

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=30947b0fcfb8

to see if I've cleaned them up enough.
(In reply to Tomislav Jovanovic [:zombie] from comment #9)
> warning spew on "perfectly good, feature-tested, best practices code that
> polyfills only if the method doesn't exist" in firefox, until we actually
> remove this is also.. suboptimal

Maybe I'm missing something but isn't warning *before* we remove it the whole point of the warning? If we have plans to remove the non-standard .contains, I don't see the problem with a console warning. It's not like the warning is going to break websites or annoy normal users...
IIUC the problem is that we can't differentiates between 'code that uses .contains' and 'code that supplies its own version of .contains but uses the native version if available', even though the latter case will continue to work just fine after the native version is removed.

But perhaps, getting people to update their code to check for .includes instead wouldn't be a bad thing? After all if they keep their existing checks, they'll never get the native versions that browsers implement.
(In reply to Jan de Mooij [:jandem] from comment #12)
> Maybe I'm missing something but isn't warning *before* we remove it the
> whole point of the warning? If we have plans to remove the non-standard
> .contains, I don't see the problem with a console warning. It's not like the
> warning is going to break websites or annoy normal users...

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #13)
> But perhaps, getting people to update their code to check for .includes
> instead wouldn't be a bad thing? After all if they keep their existing
> checks, they'll never get the native versions that browsers implement.

The idea as discussed on IRC yesterday is to only warn in non-release builds for now, and start warning in those later, after we have taken some (to be added) telemetry and know that there isn't *too* much content out there anymore that would trigger the warning.
Comment on attachment 8527303 [details] [diff] [review]
part 3 - Replace `contains` with `includes` in chrome code

Review of attachment 8527303 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for taking so long on this. I just realized that that's because I feel uneasy about reviewing this at all. One question on that: did you instrument String#contains to dump all callers and then went and fixed them, or did you do a search and replace over the tree? I hope/assume it's the former, because the latter would quite likely mean that some of the replacements are invalid because they really call some other, content-provided `contains` method. If it's the former, though, I don't think a thorough review of all callsites is even necessary and a green try run should be proof enough for the correctness of the patch.
(In reply to Till Schneidereit [:till] from comment #15)
> Comment on attachment 8527303 [details] [diff] [review]
> part 3 - Replace `contains` with `includes` in chrome code
> 
> Review of attachment 8527303 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for taking so long on this. I just realized that that's because I feel
> uneasy about reviewing this at all. One question on that: did you instrument
> String#contains to dump all callers and then went and fixed them, or did you
> do a search and replace over the tree? I hope/assume it's the former,
> because the latter would quite likely mean that some of the replacements are
> invalid because they really call some other, content-provided `contains`
> method. If it's the former, though, I don't think a thorough review of all
> callsites is even necessary and a green try run should be proof enough for
> the correctness of the patch.

Neither of them. I did a full try run (without String#contains) about 6 times and eliminated the related failures (.contains is not a function) as much as possible. The last try run was in comment 10.
Comment on attachment 8527303 [details] [diff] [review]
part 3 - Replace `contains` with `includes` in chrome code

Review of attachment 8527303 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, that sounds absolutely like it should just work; let's land this. Thanks for the hard work on this!
Attachment #8527303 - Flags: review?(till) → review+
Comment on attachment 8527338 [details] [diff] [review]
Part 1 - Add `String.prototype.includes`; keep `String.prototype.contains` around as an alias with a (non-release builds only) warning

Review of attachment 8527338 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #8527338 - Flags: review?(till) → review+
Part 3 needs rebasing.
Keywords: checkin-needed
Setting dev-doc-needed back; for tracking purpose we shouldn't consider a doc finished before a bug is closed: it can miss a train or not land at all. This has to be reflected in the doc.
Attached patch patch rebased (obsolete) — Splinter Review
Attachment #8527303 - Attachment is obsolete: true
Attachment #8534805 - Flags: review+
see bug 1107416
Attachment #8534812 - Flags: review?(till)
Comment on attachment 8534812 [details] [diff] [review]
Increment XDR_BYTECODE_VERSION_SUBTRAHEND

Review of attachment 8534812 [details] [diff] [review]:
-----------------------------------------------------------------

Oh, right. Good thing we have that assert :)

Please merge this patch into the one that actually changes js.msg before landing.
Attachment #8534812 - Flags: review?(till) → review+
Attached patch merged part 4 into part 1 (obsolete) — Splinter Review
Attachment #8527338 - Attachment is obsolete: true
Attachment #8534812 - Attachment is obsolete: true
Attachment #8534960 - Flags: review+
Depends on: 1110166
Blocks: 1110166
No longer depends on: 1110166
Blocks: es6
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #28)
> Backed out for Gaia unit test failures.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/57725f8e7aa1
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=4555778&repo=mozilla-
> inbound

I don't know what cause this failure! does anyone know anyone who can help me?
(In reply to ziyunfei from comment #29)
> I don't know what cause this failure! does anyone know anyone who can help
> me?

Hum, I'm not sure why this particular test fails, really. That said, there are lots of usages of `contains` in gaia: https://github.com/mozilla-b2g/gaia/search?q=.contains

Can you do a new try push with the b2g-related tests all activated? Something like "try: -b o -p linux32_gecko -u all -t none" should do the trick.

I'm not sure what to do about the `contains` usages in gaia: ideally we'd fix them all, but I don't want to ask you to wade through them. Particularly because the vast majority will be false-positives: they're calls to the DOMTokenList#contains method (https://dom.spec.whatwg.org/#interface-domtokenlist).

Anyway, a new try run would be great so we can take a look at the logs.
So it fails here:

http://mxr.mozilla.org/gaia/source/apps/communications/contacts/test/unit/views/list_test.js#429


    test('adding one at the beginning', function() {
      var newContact = new MockContactAllFields();
      newContact.id = '4';
      newContact.familyName = ['AA'];
      newContact.name = [newContact.givenName + ' ' + newContact.familyName];
      newContact.category = null;
      doRefreshContact(subject, newContact);

      assertNoGroup(groupFav, containerFav);
      var aContacts = assertGroup(groupA, containerA, 2);
      assert.isTrue(noContacts.classList.contains('hide'));
      assert.isTrue(aContacts[0].querySelector('p').innerHTML.indexOf('AA') >
                    -1);
      assert.isTrue(aContacts[1].querySelector('p').innerHTML.indexOf('AD') >
                    -1);
      assertTotal(3, 4);
    });
If this bug is just making String#contains an alias for String#includes, do the Gaia uses of String#contains need to block landing this patch? Separate bugs could be filed for Gaia devs to fix their code.
Depends on: 1136359
Is somebody going to finish this?
Is this going to be finished soon? It would be nice to have native ES6 support for the feature so we don't have to use the non-standard "contains" prototype.
- ziyunfei: do you have time to fix the Gaia test failures?

- evilpie: if ziyunfei is not available, would you like to carry this patch across the finish line? :)
Flags: needinfo?(evilpies)
Flags: needinfo?(446240525)
Thanks!  Also, I'm sure you are all aware, but Chromium and Chrome dropped .contains from the string prototype at some point and replaced it with 'includes'.  Which should make your path forward on depreciating this less painful.
I am busy with other stuff, maybe arai can take a look?
Flags: needinfo?(evilpies) → needinfo?(arai.unmht)
sure, but let's wait for ziyunfei's response first :)
Flags: needinfo?(arai.unmht)
okay, taking over :)

I don't see any gaia related test failure with rebased patches + some more patches for recent changes.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3044e0117bdd (linux64)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b833ab870bd8 (others)
all red/orange seem to be intermittent.

I'll post extra patches after brushing them up.
somehow non-trivial change from previous one.

in previous patch, WARNED_STRING_CONTAINS_DEPRECATED has same value as FOR_OF_PIC_CHAIN
diff --git a/js/src/vm/GlobalObject.h b/js/src/vm/GlobalObject.h
>     static const unsigned FOR_OF_PIC_CHAIN        = INT32X4_TYPE_DESCR + 1;
>+    static const unsigned WARNED_STRING_CONTAINS_DEPRECATED = INT32X4_TYPE_DESCR + 1;

but it should be sequential, right?
diff --git a/js/src/vm/GlobalObject.h b/js/src/vm/GlobalObject.h
>     static const unsigned FOR_OF_PIC_CHAIN        = INT32X4_TYPE_DESCR + 1;
>+    static const unsigned WARNED_STRING_CONTAINS_DEPRECATED = FOR_OF_PIC_CHAIN + 1;
> 
>     /* Total reserved-slot count for global objects. */
>-    static const unsigned RESERVED_SLOTS = FOR_OF_PIC_CHAIN + 1;
>+    static const unsigned RESERVED_SLOTS = WARNED_STRING_CONTAINS_DEPRECATED + 1;

it requires following change also.
diff --git a/js/public/Class.h b/js/public/Class.h
>-#define JSCLASS_GLOBAL_SLOT_COUNT      (JSCLASS_GLOBAL_APPLICATION_SLOTS + JSProto_LIMIT * 3 + 31)
>+#define JSCLASS_GLOBAL_SLOT_COUNT      (JSCLASS_GLOBAL_APPLICATION_SLOTS + JSProto_LIMIT * 3 + 32)
Assignee: 446240525 → arai.unmht
Attachment #8534960 - Attachment is obsolete: true
Attachment #8595756 - Flags: review?(till)
change for recent JS code in addition to previous Part 2.
Attachment #8595758 - Flags: review?(till)
trivial rebase of previous Part 3.
Attachment #8534805 - Attachment is obsolete: true
Attachment #8595759 - Flags: review+
non-trivial rebase and recent change for chrome code in addition to previous Part 3 (current Part 4)
Attachment #8595760 - Flags: review?(till)
Comment on attachment 8595756 [details] [diff] [review]
Part 1: Add `String.prototype.includes`; keep `String.prototype.contains` around as an alias with a (non-release builds only) warning. r=till

Review of attachment 8595756 [details] [diff] [review]:
-----------------------------------------------------------------

F+ instead of r+ because I think it'd be good to change warn-once to use a bitset instead of adding more and more fields to the global.

::: js/src/vm/GlobalObject.h
@@ +107,5 @@
>      static const unsigned FLOAT32X4_TYPE_DESCR    = INTRINSICS + 1;
>      static const unsigned FLOAT64X2_TYPE_DESCR    = FLOAT32X4_TYPE_DESCR + 1;
>      static const unsigned INT32X4_TYPE_DESCR      = FLOAT64X2_TYPE_DESCR + 1;
>      static const unsigned FOR_OF_PIC_CHAIN        = INT32X4_TYPE_DESCR + 1;
> +    static const unsigned WARNED_STRING_CONTAINS_DEPRECATED = FOR_OF_PIC_CHAIN + 1;

It would be good to combine this with WARNED_PROTO_SETTINGS_SLOW and turn it into a bitset. Then you wouldn't even need to increment the subtrahend and all that, and we could use the same mechanism for lots of other warnings. Perhaps WARNED_ONCE_FLAGS?
Attachment #8595756 - Flags: review?(till) → feedback+
Comment on attachment 8595758 [details] [diff] [review]
Part 3: Replace more `String.prototype.contains` with `String.prototype.includes` in JS code.

Review of attachment 8595758 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.
Attachment #8595758 - Flags: review?(till) → review+
Comment on attachment 8595760 [details] [diff] [review]
Part 5: Replace more `String.prototype.contains` with `String.prototype.includes` in chrome code.

Review of attachment 8595760 [details] [diff] [review]:
-----------------------------------------------------------------

Nice. Isn't it great that "contains" and "includes" are the same length? :)
Attachment #8595760 - Flags: review?(till) → review+
Thank you for reviewing :)

(In reply to Till Schneidereit [:till] from comment #46)
> Comment on attachment 8595756 [details] [diff] [review]
> Part 1: Add `String.prototype.includes`; keep `String.prototype.contains`
> around as an alias with a (non-release builds only) warning. r=till
> 
> Review of attachment 8595756 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> F+ instead of r+ because I think it'd be good to change warn-once to use a
> bitset instead of adding more and more fields to the global.
> 
> ::: js/src/vm/GlobalObject.h
> @@ +107,5 @@
> >      static const unsigned FLOAT32X4_TYPE_DESCR    = INTRINSICS + 1;
> >      static const unsigned FLOAT64X2_TYPE_DESCR    = FLOAT32X4_TYPE_DESCR + 1;
> >      static const unsigned INT32X4_TYPE_DESCR      = FLOAT64X2_TYPE_DESCR + 1;
> >      static const unsigned FOR_OF_PIC_CHAIN        = INT32X4_TYPE_DESCR + 1;
> > +    static const unsigned WARNED_STRING_CONTAINS_DEPRECATED = FOR_OF_PIC_CHAIN + 1;
> 
> It would be good to combine this with WARNED_PROTO_SETTINGS_SLOW and turn it
> into a bitset. Then you wouldn't even need to increment the subtrahend and
> all that, and we could use the same mechanism for lots of other warnings.
> Perhaps WARNED_ONCE_FLAGS?

Currently there is 2 WARNED_* slot, so combined them in Part 0.
Attachment #8596673 - Flags: review?(till)
Comment on attachment 8596673 [details] [diff] [review]
Part 0: Combine WARNED_* slots in GlobalObject and turn it into a bitset.

Review of attachment 8596673 [details] [diff] [review]:
-----------------------------------------------------------------

The changes to Xdr.h need to move from part 1 to this patch, because this is where the xdr format changes lie. r=me with that + nits addressed.

::: js/src/vm/GlobalObject.h
@@ +120,5 @@
>                    "global object slot counts are inconsistent");
>  
> +    enum WarnOnceFlag : int32_t {
> +        WARN_WATCH_DEPRECATED = 0x00000001,
> +        WARN_PROTO_SETTING_SLOW = 0x00000002

Nit: align the flags, perhaps a few spaces to the right so longer names don't break the alignment.

@@ +128,5 @@
>      // true, then set the slot to true.  Thus calling this method warns once
>      // for each global object it's called on, and every other call does
>      // nothing.
>      static bool
> +    warnOnceAbout(JSContext* cx, HandleObject obj, WarnOnceFlag slot, unsigned errorNumber);

Nit: s/slot/flag/.
Attachment #8596673 - Flags: review?(till) → review+
Comment on attachment 8596675 [details] [diff] [review]
Part 1: Add `String.prototype.includes`; keep `String.prototype.contains` around as an alias with a (non-release builds only) warning. r=till

Review of attachment 8596675 [details] [diff] [review]:
-----------------------------------------------------------------

Nice. As just mentioned in the other review, move the Xdr.h changes t part 0. r=me with that.
Attachment #8596675 - Flags: review?(till) → review+
Sorry, I forgot to address the review comment for alignment and argument name.
I'll land the patch after the tree open.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/7d5b9aed2c24
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/6cc6777fbc6e762bf4c29837bc2ff595fcdf0b09
Bug 1102219: Rename String.prototype.contains to String.prototype.includes. r=till
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/b01262fe8b1a8a8a3f6e1e35fde74673792cb989
Bug 1102219: Rename String.prototype.contains to String.prototype.includes. r=till
See Also: → 1081520
You need to log in before you can comment on or make changes to this bug.