Closed Bug 1108382 Opened 10 years ago Closed 8 years ago

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

Categories

(Core :: JavaScript: Standard Library, defect)

Product:
Core
Component:
JavaScript: Standard Library
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox49 --- fixed

People

(Reporter: evilpie, Assigned: arai)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file)

Remove non-standard flag argument from String.prototype.{search,match,replace}.
38.21 KB, patch
till
: review+
Details | Diff | Splinter Review 
      No description provided.
Keywords: site-compat
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: [DocArea=JS]
Depends on: 1131107
Depends on: 1133301
Can you please add a more thorough description about what this actually means? Thanks!
`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.
Depends on: 1141748
Depends on: 1141752
Depends on: 1142351
Blocks: 887016
Depends on: 1185455
No longer blocks: 887016
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
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 |       - |     - |       -
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.
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
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 |       - |       - |       -
Depends on: 1245801
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 |       - |       - |       -
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);
Seems like the "i" is unnecessary, because they do "a = a.toLowerCase()" right before that. So it will still match "android".
(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.
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+
(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.
See Also: → 1271147
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1b90728ce14ed9988383975b3a0399dc8ef81f1
Bug 1108382 - Remove non-standard flag argument from String.prototype.{search,match,replace}. r=till
https://hg.mozilla.org/mozilla-central/rev/d1b90728ce14
Status: NEW → RESOLVED
Closed: 9 years ago → 8 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: