Closed Bug 718518 Opened 8 years ago Closed 8 years ago

Remove all usages of non-standard flags argument to String.prototype.replace

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: till, Assigned: till)

Details

Attachments

(1 file)

As part of my analysis in bug 580480, I looked for usages of the optional third argument to String.prototype.{search,match,replace}.

Attached is a patch that removes all usages within mozilla-central found with the following grep command and then pruned down to remove false positives:
"search\s*\(\s*[^/]|match\s*\(\s*[^/]|replace\s*\(\s*[^/]"

This might make bug 481738 more feasible.
Attachment #589006 - Flags: review?(luke)
Oh, and Luke, sorry if asking you for a review wasn't right. The nature of this cleanup makes it a bit hard to find a good reviewer (or even the right component).
Comment on attachment 589006 [details] [diff] [review]
Change usages of String.prototype.replace to remove third argument

In theory the changes are simple enough I could review them, but I have not reviewed frontend JS before so I don't think I should.  Also, it would be good to spread the word about not using non-standard regexp flags.

To find reviewers, you can go to hg.mozilla.org/mozilla-central, browse to the file, then click 'annotate' and find either the last person that touched the line or someone who has touched most of the file.  Doing that:

dcamp, could you review CssRuleView.jsm;
jgriffin, could you review tps.jsm;
dao, could you review LightweightThemeConsumer.jsm;
ted, could you review CrashSubmit.jsm; and
dtownsend, could you review AddonRepository.jsm

?  Hopefully I didn't mess that up :)

(The use in XPIProvider.jsm seems to have been removed.)
Attachment #589006 - Flags: review?(ted.mielczarek)
Attachment #589006 - Flags: review?(luke)
Attachment #589006 - Flags: review?(jgriffin)
Attachment #589006 - Flags: review?(dtownsend)
Attachment #589006 - Flags: review?(dcamp)
Attachment #589006 - Flags: review?(dao)
Attachment #589006 - Flags: review?(dao) → review+
Attachment #589006 - Flags: review?(dcamp) → review+
Attachment #589006 - Flags: review?(jgriffin) → review+
Attachment #589006 - Flags: review?(ted.mielczarek) → review+
Attachment #589006 - Flags: review?(dtownsend) → review+
Assignee: general → tschneidereit+bmo
Status: UNCONFIRMED → NEW
Ever confirmed: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f8b66a93333

In the future, a checkin comment and User line would be really nice to have in the changeset.  Wasn't too big a problem for this bug, though.

Thank you for the patch!
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → mozilla12
Thanks for the checkin and the feedback on the patch format. Will improve that for my next patch.
http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed is a great reference for how to set all that up for future patches.
You need to log in before you can comment on or make changes to this bug.