1195196 - Page info: string "securityNVisits" (count of previous visits) should use a proper plural form

Status: RESOLVED FIXED

(Firefox :: Page Info Window, defect)

Description

[Artem Polivanchuk]

Attachment: 2015-08-17 09_57_53.png

The following string does not have plural forms:

browser -> chrome -> browser -> pageInfo.properties -> securityNVisits

As a result, localized string looks wrong.

Comment 1

[Francesco Lodolo [:flod]]

String originally introduced in bug 377076.

Comment 2

[Francesco Lodolo [:flod]]

Attachment: Bug 1195196 - Use proper plural form for number of visits in Page Info

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

Comment 3

[Francesco Lodolo [:flod]]

@johannh 
Sending this in your direction, based on the file's log.

I don't usually write JavaScript code, and had a hard time finding clues on how to format a multiline ternary expression (or if that's even frowned upon). At this point, I'm only sure that it passes eslint.

Note on the new string for "No": reusing Yes/No is a bad idea, there are languages that don't have that, and reply with the entire sentence, e.g. "Do you want a cookie? I want a cookie".

Comment 4

[Johann Hofmann [:johannh]]

Comment on attachment 8944234 [details]
Bug 1195196 - Use proper plural form for number of visits in Page Info

https://reviewboard.mozilla.org/r/214510/#review220170

This works for me (the ternary expression looks a tiny bit unwieldy but still fine), but please fix the comments below.

::: browser/base/content/pageinfo/security.js:7
(Diff revision 1)
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  Components.utils.import("resource://gre/modules/BrowserUtils.jsm");
> +Components.utils.import("resource://gre/modules/PluralForm.jsm");

You don't need to import this when you do the lazy getter below.

::: browser/base/content/pageinfo/security.js:236
(Diff revision 1)
> -            pageInfoBundle.getString("securityOneVisit"));
> -  } else {
> -    setText("security-privacy-history-value", noStr);
> -  }
>  
> +  var visitCountStr = visitCount === 0

Nit: we usually use let instead of var for new code

::: browser/locales/en-US/chrome/browser/pageInfo.properties:48
(Diff revision 1)
>  feedXML=XML
>  
>  securityNoOwner=This website does not supply ownership information.
> -securityOneVisit=Yes, once
> -securityNVisits=Yes, %S times
> +# LOCALIZATION NOTE (securityVisitsNumber):
> +# Semi-colon list of plural forms.
> +# See: http://developer.mozilla.org/en/docs/Localization_and_Plurals

Nit: can you use an https link, please? :)

Comment 5

[Francesco Lodolo [:flod]]

Comment on attachment 8944234 [details]
Bug 1195196 - Use proper plural form for number of visits in Page Info

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

Comment 6

[Francesco Lodolo [:flod]]

(In reply to Johann Hofmann [:johannh] from comment #4)
> >  Components.utils.import("resource://gre/modules/BrowserUtils.jsm");
> > +Components.utils.import("resource://gre/modules/PluralForm.jsm");
> 
> You don't need to import this when you do the lazy getter below.

Ugh, too much copy and paste :-\

> ::: browser/base/content/pageinfo/security.js:236
> (Diff revision 1)
> > -            pageInfoBundle.getString("securityOneVisit"));
> > -  } else {
> > -    setText("security-privacy-history-value", noStr);
> > -  }
> >  
> > +  var visitCountStr = visitCount === 0
> 
> Nit: we usually use let instead of var for new code

Thanks, I could only find 'var' used in the file, didn't considered that was due to legacy code.

> ::: browser/locales/en-US/chrome/browser/pageInfo.properties:48
> (Diff revision 1)
> >  feedXML=XML
> >  
> >  securityNoOwner=This website does not supply ownership information.
> > -securityOneVisit=Yes, once
> > -securityNVisits=Yes, %S times
> > +# LOCALIZATION NOTE (securityVisitsNumber):
> > +# Semi-colon list of plural forms.
> > +# See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
> 
> Nit: can you use an https link, please? :)

Fixed (double checked that compare-locales doesn't look at the full URL for plural checks)

Comment 7

[Francesco Lodolo [:flod]]

Comment on attachment 8944234 [details]
Bug 1195196 - Use proper plural form for number of visits in Page Info

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

Comment 8

[Francesco Lodolo [:flod]]

Comment on attachment 8944234 [details]
Bug 1195196 - Use proper plural form for number of visits in Page Info

https://reviewboard.mozilla.org/r/214510/#review220190

::: browser/base/content/pageinfo/security.js:235
(Diff revisions 2 - 3)
>    setText("security-privacy-passwords-value",
>            realmHasPasswords(uri) ? yesStr : noStr);
>  
>    var visitCount = previousVisitCount(info.hostName);
>  
> -  let visitCountStr = visitCount === 0
> +  let visitCountStr = visitCount > 0

Noticed an issue while giving a last test to the updated patch: I completely missed that visitCount can be false (no host, e.g. for about: pages).

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/980a3a48f2af
Use proper plural form for number of visits in Page Info r=johannh

Comment 10

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/980a3a48f2af