1167003 - JavaScript warning: chrome://messenger/content/newmailaccount/accountProvisioner.js, line 776: flags argument of String.prototype.{search,match,replace} is deprecated

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect)

Description

[ISHIKAWA, Chiaki]

Attachment: Do not use deprecated flags argument of String.prototype.{search,match,replace}

During local make mozmill| test suite run, I noticed that the following
warnings were produced.  This is rather new.

From the locally produced summary of warnings/errors:
     12 JavaScript warning: chrome://messenger/content/newmailaccount/accountProvisioner.js, line 776: flags argument of String.prototype.{search,match,replace} is deprecated

12 at the beginning means 12 (twelve) times.

The code in question:
mail/components/newmailaccount/content/accountProvisioner.js

  773		let templateElement = document.querySelector("#result_tmpl");
  774		let result = document.importNode(templateElement.content, true).children[0];
  775		result.innerHTML =
* 776		  result.innerHTML.replace("${address}",
  777					   address.address ? address.address : address, "g")
  778				  .replace("${priceStr}", priceStr, "g");
  779

According to

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replace

flags, in this case, "g" seems to be non-standard(?) or not usable in
V8 core, "Chrome or node.js".

--- begin quote ---
str.replace(regexp|substr, newSubStr|function[, flags])

Parameters

regexp
    A RegExp object. The match is replaced by the return value of parameter #2.
substr
    A String that is to be replaced by newSubStr.
newSubStr
    The String that replaces the substring received from parameter #1. A number of special replacement patterns are supported; see the "Specifying a string as a parameter" section below.
function
    A function to be invoked to create the new substring (to put in place of the substring received from parameter #1). The arguments supplied to this function are described in the "Specifying a function as a parameter" section below.
flags

    Note: The flags argument does not work in v8 Core (Chrome and
    Node.js). A string specifying a combination of regular expression
    flags. The use of the flags parameter in the
    String.prototype.replace() method is non-standard and
    deprecated. Instead of using this parameter, use a RegExp object
    with the corresponding flags. The value of this parameter if it is
    used should be a string consisting of one or more of the following
    characters to affect the operation as described:

    g	     global match
    i	     ignore case
    m	     match over multiple lines
    y	     sticky
--- end quote ---

I suspect that "g" here means "replace" ALL the instances?

But I am not JS expert, and I thought I would file a bugzilla and would hope someone in the know can fix this
issue, and thought of leaving it at that.

However, when I tried to submit this bug, I noticed a similar entry.

Bug 1138095 - Fix in-tree consumers that use non-standard flag
argument of String.prototype.{search,match,replace} in mail/.

I looked at the patch posted there, and it seems to
be a rather straightforward change to remove the flag argument.

So I am mimicking the solution there to come up with this patch posted
here.
(I have no idea why this particular usage of the flag argument reported here was
missed in Bug 1139095. Maybe the usage here is new or missed since it may
not trigger easily during interactive usage.)

I hope people in the know can comment on this.

I ran |make mozmill| and the particular warnings went away and I don't think this
change has introduced any new bugs/warnings.
(To be honest, I think now that the deprecated errors are gone, the
processing goes further, but then I see some timeout errors.
On this particular test PC, I usually could not get to the ISP
database server for unfathomable reason, and and the test fails there.)

So I think the patch is OK, but someone may want to run it through
tryserver.  (Right now, my local source tree has so many patches and it is awkward for me to test this patch only on tryserver.)

BTW, the amount of warnings and errors reported during |make mozmill|
by running full DEBUG build of C-C TB is surprisingly large these days
(reflecting added internal checks over the last 12 months or so, I
think ) and so I may have missed a few new errors, but that is
very unlikely (the famous last words).

TIA

Comment 1

[Magnus Melin [:mkmelin]]

Comment on attachment 8608428 [details] [diff] [review]
Do not use deprecated flags argument of String.prototype.{search,match,replace}

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

::: mail/components/newmailaccount/content/accountProvisioner.js
@@ +774,5 @@
>          let result = document.importNode(templateElement.content, true).children[0];
>          result.innerHTML =
> +          result.innerHTML.replace(/${address}/g,
> +                                   address.address ? address.address : address)
> +                          .replace(/${priceStr}/g, priceStr);

i doubt this works as intended. $ is special in regexps and need to be escaped as \$

Comment 2

[ISHIKAWA, Chiaki]

Attachment: Do not use deprecated flags argument of String.prototype.{search,match,replace}

(In reply to Magnus Melin from comment #1)
> Comment on attachment 8608428 [details] [diff] [review]
> Do not use deprecated flags argument of
> String.prototype.{search,match,replace}
> 
> Review of attachment 8608428 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/components/newmailaccount/content/accountProvisioner.js
> @@ +774,5 @@
> >          let result = document.importNode(templateElement.content, true).children[0];
> >          result.innerHTML =
> > +          result.innerHTML.replace(/${address}/g,
> > +                                   address.address ? address.address : address)
> > +                          .replace(/${priceStr}/g, priceStr);
> 
> i doubt this works as intended. $ is special in regexps and need to be
> escaped as \$

Escaped with "\" as suggested.

(I thought, though, $ meaning End of line is only when it appears at the end of regular expression, but I don't know much about JS RE.)

I verified this using |make mozmill| and it suppressed the warning messages and did not seem to introduce new errors.

TIA

Comment 3

[Magnus Melin [:mkmelin]]

(In reply to ISHIKAWA, Chiaki from comment #2)
> (I thought, though, $ meaning End of line is only when it appears at the end
> of regular expression, but I don't know much about JS RE.)

Not so in JavaScript.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 8622083 [details] [diff] [review]
Do not use deprecated flags argument of String.prototype.{search,match,replace}

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

LGTM, r=mkmelin

Comment 5

[Magnus Melin [:mkmelin]]

https://hg.mozilla.org/comm-central/rev/d8c669e7433b -> FIXED

Please in the future, include the bug number in your hg commit message.