Remove non-standard flag argument from String.prototype.{search,match,replace}

RESOLVED FIXED in Firefox 49

Status

()

Core
JavaScript: Standard Library
RESOLVED FIXED
3 years ago
16 days ago

People

(Reporter: evilpie, Assigned: arai)

Tracking

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

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

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [DocArea=JS])

Attachments

(1 attachment)

Comment hidden (empty)
Keywords: dev-doc-needed

Updated

3 years ago
Keywords: site-compat
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: [DocArea=JS]
(Assignee)

Updated

2 years ago
Depends on: 1131107
(Assignee)

Updated

2 years ago
Depends on: 1133301
(Assignee)

Updated

2 years ago
Can you please add a more thorough description about what this actually means? Thanks!
(Assignee)

Comment 2

2 years ago
`flags` in 3rd argument of String.prototype.replace and 2nd argument of String.prototype.match/search are non-standard extension. there you can pass flags like "g", "i", etc (same as RegExp flags). For almost case, "g" is used to replace all strings, like str.replace("foo", "bar", "g"), and some case "i" is used.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/search
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/match
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replace
(search and match have no description about the parameter)

They need to be removed before supporting @@search, @@match and @@replace in ES6, to make it simple.

http://people.mozilla.org/~jorendorff/es6-draft.html#sec-string.prototype.search
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-regexp.prototype-@@search
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-string.prototype.match
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-regexp.prototype-@@match
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-string.prototype.replace
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-regexp.prototype-@@replace

I'll post patches for bugs 1138093-1138099 shortly.
Comment hidden (typo)
Comment hidden (typo)
(Assignee)

Updated

2 years ago
Depends on: 1141748
(Assignee)

Updated

2 years ago
Depends on: 1141752
(Assignee)

Updated

2 years ago
Depends on: 1142351
(Assignee)

Updated

2 years ago
Blocks: 887016
(Assignee)

Updated

2 years ago
Depends on: 1185455
(Assignee)

Updated

2 years ago
No longer blocks: 887016
(Assignee)

Comment 5

2 years ago
So far, telemetry result for flags argument until previous version is following.
It's decreasing, but still high.

   | Nightly || Aurora  | Beta  | Release
---+----------+---------+-------+---------
39 |  32.17k* | 503.47k | 2.96M | 1.41M
40 | 134.55k  | 286k    | 2.16M |   -
41 |  56.72k  | 222.75k |   -   |   -
42 |  14.53k  |    -    |   -   |   -

* Telemetry added here
Status: REOPENED → NEW
(Assignee)

Comment 6

2 years ago
Unified Telemetry shows different result, and it's not decreasing :/

Histogram Dashboard V4 (Unified Telemetry)
https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2015-09-21&keys=__none__!__none__!__none__&max_channel_version=nightly%252F43&measure=JS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_CONTENT&min_channel_version=null&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2015-08-10&table=1&trim=1&use_submission_date=0

   | Nightly | Aurora  | Beta  | Release
---+---------+---------+-------+---------
39 |     310 |   4.38k | 1.03M |   5.43M
40 |   1.64k |  60.17k | 5.79M |   8.93M
41 |  38.55k | 516.65k | 4.73M |       -
42 | 303.43k | 528.87k |     - |       -
43 | 266.99k |       - |     - |       -

Histogram Dashboard V2 (Telemetry v2)
https://telemetry.mozilla.org/dist.html#!cumulative=0&end_date=2015-09-11&max_channel_version=nightly%252F43&measure=JS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_CONTENT&min_channel_version=null&product=Firefox&sanitize=1&start_date=2015-08-10&table=1&trim=1&use_submission_date=0

   | Nightly | Aurora  | Beta  | Release
---+---------+---------+-------+---------
39 |  32.5k  | 511.2k  | 3.07M |   1.43M
40 | 136.45k | 291.35k | 2.17M | 908.42k
41 |  57.2k  | 223.33k | 1.07M |       -
42 |  14.54k | 	90.95k |     - |       -
43 |   8.68k |       - |     - |       -
(Assignee)

Comment 7

2 years ago
Might be better to check the argument more strictly.  Currently the telemetry counts when flags argument is presented, but we might have to count only if the flags argument has meaning, like "i", "g", etc, so that those cases are actually affected if we remove the flags argument.
Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/non-standard-flags-argument-will-be-removed-from-string-search-methods/
(Assignee)

Comment 9

a year ago
it's a little decreasing in content
maybe we need one more year...?

   | Nightly | Aurora  | Beta  | Release
---+---------+---------+-------+---------
39 |     378 |   7.61k | 1.25M |   5.6 M
40 |   3.17k |  64.9 k | 6.09M |   9.79M
41 |  40.59k | 526.82k | 5.38M |   5.63M
42 | 340.38k | 548.91k | 4.59M |   5.72M
43 | 277.72k | 416.02k | 3.69M |       -
44 | 207.02k | 343.87k |     - |       -
45 | 160.81k |       - |     - |       -

https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2015-12-14&keys=__none__!__none__!__none__&max_channel_version=nightly%252F45&measure=JS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_CONTENT&min_channel_version=null&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2015-10-29&table=1&trim=1&use_submission_date=0


telemetry for add-on needs some more cycles

   | Nightly | Aurora  | Beta  | Release
---+---------+---------+-------+---------
44 |   2.61k |  55.43k |     - |       -
45 |  25.77k |       - |     - |       -

https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2015-12-14&keys=__none__!__none__!__none__&max_channel_version=nightly%252F45&measure=JS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_ADDONS&min_channel_version=null&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2015-10-29&table=1&trim=1&use_submission_date=0
(Assignee)

Comment 10

a year ago
it's still decreasing in content

Content

   | Nightly | Aurora  | Beta    | Release
---+---------+---------+---------+---------
39 |    476  |   9.2k  |   1.34M |   5.64M
40 |   3.63k |  68.11k |   6.19M |   9.85M
41 |  41.19k | 529.98k |   5.49M |   5.74M
42 | 342.51k | 551.17k |   4.79M |   6.44M
43 | 279.65k | 418.75k |   4.27M |   5.85M
44 | 208.99k | 368.09k |   3.56M |       -
45 | 175.65k | 329.12k |       - |       -
46 | 149.19k |       - |       - |       -

Add-on

   | Nightly | Aurora  | Beta    | Release
---+---------+---------+---------+---------
44 |   2.61k |  56.8 k | 449.28k |       -
45 |  31.19k |  64.42k |       - |       -
45 |  27.93k |       - |       - |       -
(Assignee)

Updated

a year ago
Depends on: 1245801
(Assignee)

Comment 11

a year ago
so far, they're still used (at least some argument is passed) both in content/add-ons on nightly 47, that dropped the feature (bug 1245801, fixed at 2016-02-09), but we don't get any site/add-on compat bug report related to it.  it would mean that, either passed flag argument have no meaning for the input there (including empty flag), or the change introduced by dropping the flag-argument is not critical for them, I guess.

If we don't get any bug report for aurora 47 too, I think we can drop it entirely in next cycle.


Content

   | Nightly | Aurora  | Beta    | Release
---+---------+---------+---------+---------
39 |    502  |  10.62k |   1.42M |   5.68M
40 |   4.95k |  71.34k |   6.27M |   9.89M
41 |  41.64k | 532.23k |   5.57M |   5.79M
42 | 344.60k | 552.40k |   4.91M |   6.57M
43 | 280.42k | 420.96k |   4.41M |   6.92M
44 | 210.51k | 374.13k |   4.10M |   4.06M
45 | 184.57k | 353.37k |   2.92M |       -
46 | 153.79k | 347.40k |       - |       -
47 | 173.65k |       - |       - |       -

Add-on

   | Nightly | Aurora  | Beta    | Release
---+---------+---------+---------+---------
44 |   2.61k |  58.06k | 511.86k | 529.37k
45 |  38.07k |  68.72k |  50.72k |       -
46 |  28.32k |  71.25k |       - |       -
47 |  26.78k |       - |       - |       -
(Assignee)

Comment 12

a year ago
Finally, found an actual usage of flags property on web content.

  URL: https://www.kayak.co.jp/

  > flags argument of String.prototype.{search,match,replace} is no longer supported   bk-coretag.js:2:30825

Here's beautified code from https://tags.bkrtx.com/js/bk-coretag.js

  parseUserAgentString: function(a) {
      if (a = a.toLowerCase(), a.match("android", "i")) {
          var b = this.androidRegex.exec(a);
          return b ? b[1] : "ANDROID";
      }
      if (a.match("ip(hone|ad|od|od\\stouch)")) {
          var c = this.iosRegex.exec(a);
          return c ? c[1] : "IOS";
      }
      return a;
  },

So, following code uses "i" flag for matching.

  a.match("android", "i")

I guess, it fails to detect Android now on Firefox for Android aurora/nightly, but not sure how it affects the actual behavior, as the return value is used like following:

  try {
      b = this.defaultHasher.x86Hash128(this.parseUserAgentString(navigator.userAgent), 31);
  } catch (c) {}
  a.push("ua=" + b);
(Reporter)

Comment 13

a year ago
Seems like the "i" is unnecessary, because they do "a = a.toLowerCase()" right before that. So it will still match "android".
(Assignee)

Comment 14

a year ago
(In reply to Tom Schuster [:evilpie] from comment #13)
> Seems like the "i" is unnecessary, because they do "a = a.toLowerCase()"
> right before that. So it will still match "android".

wow, thank you for pointing that out :)
So, it can be ingorable.
(Assignee)

Comment 15

a year ago
Created attachment 8749532 [details] [diff] [review]
Remove non-standard flag argument from String.prototype.{search,match,replace}.

Removed flag arguments handling, warning and telemetry, and testcases.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3699193813b2
Assignee: nobody → arai.unmht
Attachment #8749532 - Flags: review?(till)
Comment on attachment 8749532 [details] [diff] [review]
Remove non-standard flag argument from String.prototype.{search,match,replace}.

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

Nice! r=me with or without feedback addressed.

::: js/src/builtin/String.js
@@ +240,4 @@
>      }
>  
>      // Step 4.
> +    var rx = RegExpCreate(regexp, undefined);

Any reason not to change intrinsic_RegExpCreate so it doesn't expect two arguments anymore? Seems like js::RegExpCreate should also not need the flagsValue parameter anymore. (Though I find that a bit confusing since it's the implementation of a spec algorithm that does indeed take a flags argument. We probably implement the cases where that's used differently?)

::: js/src/tests/ecma_6/extensions/String-match-flags.js
@@ +1,1 @@
>  // |reftest| skip-if(!xulRuntime.shell) -- needs enableMatchFlagArgument()

Can remove this skip-if now.
Attachment #8749532 - Flags: review?(till) → review+
(Assignee)

Comment 17

a year ago
(In reply to Till Schneidereit [:till] from comment #16)
> Comment on attachment 8749532 [details] [diff] [review]
> Remove non-standard flag argument from
> String.prototype.{search,match,replace}.
> 
> Review of attachment 8749532 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice! r=me with or without feedback addressed.
> 
> ::: js/src/builtin/String.js
> @@ +240,4 @@
> >      }
> >  
> >      // Step 4.
> > +    var rx = RegExpCreate(regexp, undefined);
> 
> Any reason not to change intrinsic_RegExpCreate so it doesn't expect two
> arguments anymore? Seems like js::RegExpCreate should also not need the
> flagsValue parameter anymore. (Though I find that a bit confusing since it's
> the implementation of a spec algorithm that does indeed take a flags
> argument. We probably implement the cases where that's used differently?)

Thank you for pointing that out :)
apparently RegExp creation needs some cleanup, as we now have 2 almost identical functions regexp_construct and RegExpCreate (previously different for statics flags, but no more).
I'll rename regexp_construct to RegExpCreate and remove old RegExpCreate, in separated bug.
(Assignee)

Updated

a year ago
See Also: → bug 1271147
(Assignee)

Comment 18

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1b90728ce14ed9988383975b3a0399dc8ef81f1
Bug 1108382 - Remove non-standard flag argument from String.prototype.{search,match,replace}. r=till

Comment 19

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1b90728ce14

Comment 20

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d1b90728ce14
Status: NEW → RESOLVED
Last Resolved: 2 years agoa year ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(Assignee)

Comment 21

a year ago
Updated following documentation:
  https://developer.mozilla.org/en-US/Firefox/Releases/49
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/match
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/search
  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replace
Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/non-standard-flags-argument-has-been-removed-from-string-search-methods/
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 1271857
(Assignee)

Updated

10 months ago
Duplicate of this bug: 481738

Updated

16 days ago
Duplicate of this bug: 1056450
You need to log in before you can comment on or make changes to this bug.