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

RESOLVED FIXED in mozilla12

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: till, Assigned: till)

Tracking

Trunk
mozilla12
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 589006 [details] [diff] [review]
Change usages of String.prototype.replace to remove third argument

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)
(Assignee)

Comment 1

5 years ago
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 2

5 years ago
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)

Updated

5 years ago
Attachment #589006 - Flags: review?(dao) → review+

Updated

5 years ago
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+
Keywords: checkin-needed

Updated

5 years ago
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
(Assignee)

Comment 4

5 years ago
Thanks for the checkin and the feedback on the patch format. Will improve that for my next patch.

Comment 5

5 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/0f8b66a93333

you can also reference mdn updated entry
https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.