Closed Bug 1103588 Opened 9 years ago Closed 8 years ago

Remove String.prototype.contains

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox47 --- affected
firefox48 --- verified

People

(Reporter: 446240525, Assigned: cpeterson)

References

(Blocks 1 open bug)

Details

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

Attachments

(6 files)

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.
Depends on: 1110166
Blocks: 1103158
Keywords: site-compat
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?
Assignee: 446240525 → cpeterson
Status: NEW → ASSIGNED
Attachment #8739716 - Flags: review?(dolske)
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.
Attachment #8739717 - Flags: review?(jryans)
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.
Attachment #8739725 - Flags: review?(bzbarsky)
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.
Attachment #8739734 - Flags: review?(till)
Part 5: Remove deprecated String#contains function; use String#includes instead.
Attachment #8739735 - Flags: review?(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.
Attachment #8739734 - Flags: review?(till) → review+
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/
Attachment #8739735 - Flags: review?(till) → review+
See Also: → 1081520
Comment on attachment 8739725 [details] [diff] [review]
part-3-sharedWorker-contains.patch

r=me
Attachment #8739725 - Flags: review?(bzbarsky) → review+
Attachment #8739716 - Flags: review?(dolske) → review+
(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).
Florian and Kohei, thanks for updating the docs!
Status: RESOLVED → VERIFIED
So we never actually showed a warning in release builds?
(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?
Flags: needinfo?(evilpies)
Yes that sounds like a good idea. We should at least warn in release for one cycle.
Flags: needinfo?(evilpies)
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.
Attachment #8741074 - Flags: review?(till)
Comment on attachment 8741074 [details] [diff] [review]
remove-warning-ifndef-RELEASE_BUILD.patch

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

Makes sense.
Attachment #8741074 - Flags: review?(till) → review+
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
Attachment #8741074 - Flags: approval-mozilla-aurora?
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.
Attachment #8741074 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Thanks, Till and Ritu.
Blocks: 1265865
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.
You need to log in before you can comment on or make changes to this bug.