Closed Bug 1431471 Opened 7 years ago Closed 7 years ago

Fix use of "%$1/2/3...S" to use "%1/2/3...$S" instead

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 59
Tracking Status
firefox58 --- fixed
firefox59 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

Attachments

(1 file)

I screwed up in bug 1430511 and bug 1430974 and messed up the order of "$1". Localized strings use "%1$S" not "%$1S".
(In reply to :Gijs from comment #0) > I screwed up in bug 1430511 and bug 1430974 and messed up the order of "$1". > Localized strings use "%1$S" not "%$1S". I don't have access to the second bug, but the first one (which I reviewed) is using %S
Comment on attachment 8943704 [details] Bug 1431471 - fix regexes for l10n parameter replacement in two places, https://reviewboard.mozilla.org/r/214104/#review219812 Ah, now the bug makes a lot of sense. I should have looked at the code, not just the string. ::: browser/components/preferences/in-content/findInPage.js:306 (Diff revision 1) > strings.getFormattedString("searchResults.sorryMessageWin", [this.query]) : > strings.getFormattedString("searchResults.sorryMessageUnix", [this.query]); > let helpUrl = Services.urlFormatter.formatURLPref("app.support.baseURL") + "preferences"; > let brandName = document.getElementById("bundleBrand").getString("brandShortName"); > let helpString = strings.getString("searchResults.needHelp3"); > - let helpItems = helpString.split(/%(?:\$1)?S/); > + let helpItems = helpString.split(/%(?:1\$)?S/); I feel like these two regex should be identical, either use 1 or \d. Practical result is identical, since there's only one parameter.
Attachment #8943704 - Flags: review?(francesco.lodolo) → review+
(In reply to Francesco Lodolo [:flod] from comment #3) > Comment on attachment 8943704 [details] > Bug 1431471 - fix regexes for l10n parameter replacement in two places, > > https://reviewboard.mozilla.org/r/214104/#review219812 > > Ah, now the bug makes a lot of sense. I should have looked at the code, not > just the string. Yeah, sorry. I'm not sure if it's really necessary but I don't think we should risk one of the locale strings using the indexed variant in some locale and breaking. :-( > ::: browser/components/preferences/in-content/findInPage.js:306 > (Diff revision 1) > > strings.getFormattedString("searchResults.sorryMessageWin", [this.query]) : > > strings.getFormattedString("searchResults.sorryMessageUnix", [this.query]); > > let helpUrl = Services.urlFormatter.formatURLPref("app.support.baseURL") + "preferences"; > > let brandName = document.getElementById("bundleBrand").getString("brandShortName"); > > let helpString = strings.getString("searchResults.needHelp3"); > > - let helpItems = helpString.split(/%(?:\$1)?S/); > > + let helpItems = helpString.split(/%(?:1\$)?S/); > > I feel like these two regex should be identical, either use 1 or \d. > Practical result is identical, since there's only one parameter. Right, fixed to use only 1. I'm going to add a generic helper for this in BrowserUtils in bug 1431428 and switch consumers to that, but I didn't manage to finish that patch on Thursday and the other patches landed for 59 so I wanted to get something in before the merge.
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/86e363a74816 fix regexes for l10n parameter replacement in two places, r=flod
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: