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

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[:Gijs (out for now; he/him)]

I screwed up in bug 1430511 and bug 1430974 and messed up the order of "$1". Localized strings use "%1$S" not "%$1S".

Comment 1

[Francesco Lodolo [:flod]]

(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 2

[:Gijs (out for now; he/him)]

Attachment: Bug 1431471 - fix regexes for l10n parameter replacement in two places,

Review commit: https://reviewboard.mozilla.org/r/214104/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214104/

Comment 3

[Francesco Lodolo [:flod]]

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.

Comment 4

[:Gijs (out for now; he/him)]

Comment on attachment 8943704 [details]
Bug 1431471 - fix regexes for l10n parameter replacement in two places,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214104/diff/1-2/

Comment 5

[:Gijs (out for now; he/him)]

(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.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/86e363a74816
fix regexes for l10n parameter replacement in two places, r=flod

Comment 7

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/86e363a74816