Closed Bug 1431471 Opened 6 years ago Closed 6 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
https://hg.mozilla.org/mozilla-central/rev/86e363a74816
Status: NEW → RESOLVED
Closed: 6 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: