1268159 - Use GreD in addition to XCurProcD for browser_misused_characters_in_strings.js

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

GreD covers more string files and is showing 338 failures in this test when GreD is used instead in the checkAllProperties function.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: MozReview Request: Bug 1268159 - Use GreD in addition to XCurProcD for browser_misused_characters_in_strings.js to cover more string files. r?gijs

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

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3cbb753c754

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8746216 [details]
MozReview Request: Bug 1268159 - Use GreD in addition to XCurProcD for browser_misused_characters_in_strings.js to cover more string files. r?gijs

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

Comment 4

[:Gijs (he/him)]

Comment on attachment 8746216 [details]
MozReview Request: Bug 1268159 - Use GreD in addition to XCurProcD for browser_misused_characters_in_strings.js to cover more string files. r?gijs

https://reviewboard.mozilla.org/r/49309/#review46261

As try has noted, you also need to update https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_aboutCertError.js#228 which depends on the error message whose quotes you're modifying.

::: browser/base/content/test/general/browser_misused_characters_in_strings.js:180
(Diff revision 2)
> +  let urisGreD = yield generateURIsFromDirTree(appDirGreD, [extension]);
> +  let urisXCurProcD = yield generateURIsFromDirTree(appDirXCurProcD, [extension]);

I think you can avoid work here in some cases by using nsIFile's `contains` method to check if one of the two dirs contains the other, rather than doing some of the same work twice and then using a `Set` to remove the dupes. Certainly here on OS X, on a beta distributed build, `GreD` is simply the parent directory of `XCurProcD`.

::: browser/base/content/test/general/browser_misused_characters_in_strings.js:182
(Diff revision 2)
> +  let uris = Array.from(new Set(urisGreD.concat(appDirXCurProcD)));
> +  return uris;

Nit: no need for the 'uris' temp, AFAICT.

::: toolkit/locales/en-US/chrome/mozapps/handling/handling.properties:10
(Diff revision 2)
>  protocol.title=Launch Application
>  protocol.description=This link needs to be opened with an application.
>  protocol.choices.label=Send to:
>  protocol.checkbox.label=Remember my choice for %S links.
>  protocol.checkbox.accesskey=R
> -protocol.checkbox.extra=This can be changed in %S's preferences. 
> +protocol.checkbox.extra=This can be changed in %S’s preferences. 

Nit: kill trailing whitespace please.

Comment 5

[Francesco Lodolo [:flod]]

Can you clarify what's the rationale between ‘%1$S’ and “%1$S”? I have the feeling you automated replacing ' with ’, but in several cases “” should be used. Example:

AutomaticAuth=You are about to log in to the site “%1$S” with the username “%2$S”.
PEVariableEmpty=Expected variable value but found ‘%1$S’.

To be clear, I'm perfectly fine with it, just wanted to understand if there's a reason or it's just a script's expected outcome.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Francesco Lodolo [:flod] from comment #5)
> Can you clarify what's the rationale between ‘%1$S’ and “%1$S”? I have the
> feeling you automated replacing ' with ’, but in several cases “” should be
> used. Example:
> 
> AutomaticAuth=You are about to log in to the site “%1$S” with the username
> “%2$S”.
> PEVariableEmpty=Expected variable value but found ‘%1$S’.
> 
> To be clear, I'm perfectly fine with it, just wanted to understand if
> there's a reason or it's just a script's expected outcome.

We haven't formulated the recommended quote usage yet. I maintained the current quote usage, replacing single straight quotes with single curly quotes and double straight quotes with double curly quotes.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/0af3c129a3665692271cda977178286d5937e326
Bug 1268159 - Use GreD in addition to XCurProcD for browser_misused_characters_in_strings.js to cover more string files. r=gijs

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/cc898e5dfa22a5263e34488baccd32c2ca275f30
Bug 1268159 - followup, update some tests that got missed in the conversion from straight quotes to curly quotes and revert the Sync client name change until a follow-up bug can track down the deeper regression. r=me on a CLOSED TREE.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

http://hg.mozilla.org/mozilla-central/rev/0af3c129a366
http://hg.mozilla.org/mozilla-central/rev/cc898e5dfa22