bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Remove String.prototype.contains

VERIFIED FIXED in Firefox 48

Status

()

Core
JavaScript: Standard Library
VERIFIED FIXED
4 years ago
2 years ago

People

(Reporter: ziyunfei, Assigned: cpeterson)

Tracking

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

Trunk
mozilla48
addon-compat, dev-doc-complete, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 affected, firefox48 verified)

Details

(Whiteboard: [DocArea=JS])

Attachments

(6 attachments)

(Reporter)

Description

4 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
Keywords: addon-compat

Updated

3 years ago
Keywords: site-compat
(Assignee)

Comment 2

2 years ago
Created attachment 8739716 [details] [diff] [review]
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?
Assignee: 446240525 → cpeterson
Status: NEW → ASSIGNED
Attachment #8739716 - Flags: review?(dolske)
(Assignee)

Comment 3

2 years ago
Created attachment 8739717 [details] [diff] [review]
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.
Attachment #8739717 - Flags: review?(jryans)
(Assignee)

Comment 4

2 years ago
Created attachment 8739725 [details] [diff] [review]
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.
Attachment #8739725 - Flags: review?(bzbarsky)
(Assignee)

Comment 5

2 years ago
Created attachment 8739734 [details] [diff] [review]
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.
Attachment #8739734 - Flags: review?(till)
(Assignee)

Comment 6

2 years ago
Created attachment 8739735 [details] [diff] [review]
part-5-js-remove-contains.patch

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+

Updated

2 years ago
See Also: → bug 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

2 years ago
Florian and Kohei, thanks for updating the docs!
Status: RESOLVED → VERIFIED
status-firefox47: --- → wontfix
status-firefox48: fixed → verified
So we never actually showed a warning in release builds?
(Assignee)

Comment 18

2 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

2 years ago
Created attachment 8741074 [details] [diff] [review]
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.
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

2 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+

Updated

2 years ago
status-firefox47: wontfix → affected
(Assignee)

Comment 24

2 years ago
Thanks, Till and Ritu.
status-firefox47: affected → fixed
Blocks: 1273353
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.
status-firefox47: fixed → affected
You need to log in before you can comment on or make changes to this bug.