Closed Bug 1103588 Opened 6 years ago Closed 5 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.
See Also: → 789036
You need to log in before you can comment on or make changes to this bug.