Remove String.prototype.contains

VERIFIED FIXED in Firefox 48

Status

()

defect
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: 446240525, Assigned: cpeterson)

Tracking

(Blocks 1 bug, {addon-compat, dev-doc-complete, site-compat})

Trunk
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 affected, firefox48 verified)

Details

(Whiteboard: [DocArea=JS])

Attachments

(6 attachments)

(Reporter)

Description

5 years ago
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.

Updated

4 years ago
Depends on: 1110166
Blocks: 1103158
(Assignee)

Comment 2

3 years ago
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)
(Assignee)

Comment 3

3 years ago
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)
(Assignee)

Comment 4

3 years ago
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)
(Assignee)

Comment 5

3 years ago
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)
(Assignee)

Comment 6

3 years ago
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
Attachment #8739717 - Flags: review?(jryans) → review+
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).
(Assignee)

Comment 16

3 years ago
Florian and Kohei, thanks for updating the docs!
Status: RESOLVED → VERIFIED
So we never actually showed a warning in release builds?
(Assignee)

Comment 18

3 years ago
(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)
(Assignee)

Comment 20

3 years ago
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+
(Assignee)

Comment 22

3 years ago
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+
(Assignee)

Comment 24

3 years ago
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.