1103588 - Remove String.prototype.contains

Status: VERIFIED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[ziyunfei]

In bug 1102219, we make `String.prototype.contains` as an alias of `String.prototype.includes`, and warn people not to use it any more. When possible, we will remove it.

Comment 1

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/string-prototype-contains-will-be-removed/

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: part-1-browser-contains.patch

Part 1: Replace deprecated String#contains function with String#includes in browser tests and extensions.

Q: Is browser/extensions/pocket/content/pktApi.jsm third-party code that should be fixed upstream somewhere?

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: part-2-devtools-contains.patch

Part 2: Replace deprecated String#contains function with String#includes in devtools tests.

And add "use strict" to the browser_dbg_worker-window.js test.

Comment 4

[Chris Peterson [:cpeterson]]

Attachment: part-3-sharedWorker-contains.patch

Part 3: Replace deprecated String#contains warning with function expression warning in sharedWorker_sharedWorker.js test.

This "".contains("") call was added in bug 1252592 to purposely generate a JavaScript warning.

Comment 5

[Chris Peterson [:cpeterson]]

Attachment: part-4-js-tests-contains.patch

Part 4: Replace String#contains function in js tests.

- Remove sudoku.js test's unnecessary String#contains and Array#contains polyfill functions.
- Replace warning.js test's String#contains warning with an expression closure warning.

Comment 6

[Chris Peterson [:cpeterson]]

Attachment: part-5-js-remove-contains.patch

Part 5: Remove deprecated String#contains function; use String#includes instead.

Comment 7

[Till Schneidereit [:till]]

Comment on attachment 8739734 [details] [diff] [review]
part-4-js-tests-contains.patch

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

Thanks!

I'm not entirely sure how useful it is to have the sudoku test, but it runs fairly quickly, so I guess it doesn't matter much.

Comment 8

[Till Schneidereit [:till]]

Comment on attachment 8739735 [details] [diff] [review]
part-5-js-remove-contains.patch

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

Down to one warnOnce warning, nice \o/

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8739725 [details] [diff] [review]
part-3-sharedWorker-contains.patch

r=me

Comment 10

[Justin Dolske [:Dolske]]

(In reply to Chris Peterson [:cpeterson] from comment #2)
> Created attachment 8739716 [details] [diff] [review]
> part-1-browser-contains.patch
> 
> Q: Is browser/extensions/pocket/content/pktApi.jsm third-party code that
> should be fixed upstream somewhere?

Nope. The m-c code is canonical at this point (and this particular code was only for m-c).

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0f16e09b673
https://hg.mozilla.org/integration/mozilla-inbound/rev/28e0d6720135
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4cd474335e4
https://hg.mozilla.org/integration/mozilla-inbound/rev/6891e4414ffa
https://hg.mozilla.org/integration/mozilla-inbound/rev/47162e3a9e02

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e0f16e09b673
https://hg.mozilla.org/mozilla-central/rev/28e0d6720135
https://hg.mozilla.org/mozilla-central/rev/a4cd474335e4
https://hg.mozilla.org/mozilla-central/rev/6891e4414ffa
https://hg.mozilla.org/mozilla-central/rev/47162e3a9e02

Comment 13

[Mark Banner (:standard8)]

FTR also pushed to the loop repo: https://github.com/mozilla/loop/commit/32bb7de2b40b022816bb2f33eb94ff70b1cc4e93

Comment 14

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/48#JavaScript
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/includes

Comment 15

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/string-prototype-contains-has-been-removed-in-favour-of-includes/

Comment 16

[Chris Peterson [:cpeterson]]

Florian and Kohei, thanks for updating the docs!

Comment 17

[Tom S [:evilpie]]

So we never actually showed a warning in release builds?

Comment 18

[Chris Peterson [:cpeterson]]

(In reply to Tom Schuster [:evilpie] from comment #17)
> So we never actually showed a warning in release builds?

That's a good point! A warning was added in FF 40 (bug 1103588), but it was #ifndef RELEASE_BUILD.

Do you think we should uplift a patch for Aurora 47 to remove the #ifndef RELEASE_BUILD from the warning and/or postpone the removal of String#contains to FF 49?

Comment 19

[Tom S [:evilpie]]

Yes that sounds like a good idea. We should at least warn in release for one cycle.

Comment 20

[Chris Peterson [:cpeterson]]

Attachment: remove-warning-ifndef-RELEASE_BUILD.patch

Remove the #ifndef RELEASE_BUILD around the String#contains warning so we have at least one release cycle (FF 47) in the Beta and Release channels.

Comment 21

[Till Schneidereit [:till]]

Comment on attachment 8741074 [details] [diff] [review]
remove-warning-ifndef-RELEASE_BUILD.patch

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

Makes sense.

Comment 22

[Chris Peterson [:cpeterson]]

Comment on attachment 8741074 [details] [diff] [review]
remove-warning-ifndef-RELEASE_BUILD.patch

Approval Request Comment
[Feature/regressing bug #]: JavaScript warning landed in FF 40 (bug 1102219), but the warning was limited to Nightly and Aurora/DevEdition users.
[User impact if declined]: Web and add-on developers who don't develop using Nightly or DevEdition will be surprised when their website or add-on stops working.
[Describe test coverage new/current, TreeHerder]: The warning has already been reported in Nightly and Aurora since FF 40, so there should be no surprises showing it to more people.
[Risks and why]: The warning is low risk because it is only shown once per page.
[String/UUID change made/needed]: None

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8741074 [details] [diff] [review]
remove-warning-ifndef-RELEASE_BUILD.patch

This warning has been around for a while, it will be shown on Beta47 and Release47. Taking it.

Comment 24

[Chris Peterson [:cpeterson]]

Thanks, Till and Ritu.

Comment 25

[Kohei Yoshino]

I think the status-firefox47:fixed flag is confusing here; 47 has just enabled the warning on release builds, and the method itself has been removed from 48, IIUC.