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

RESOLVED FIXED in Firefox 60

Status

()

Firefox
Page Info Window
RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: Artem Polivanchuk, Assigned: flod)

Tracking

unspecified
Firefox 60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
Created attachment 8648594 [details]
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.
(Assignee)

Updated

3 years ago
Component: uk / Ukrainian → Page Info Window
Product: Mozilla Localizations → Firefox
Summary: [uk] Firefox: String "securityNVisits" (Yes, %S times) does not have plurals → Page info: string "securityNVisits" (count of previous visits) should use a proper plural form
(Assignee)

Comment 1

3 years ago
String originally introduced in bug 377076.
Comment hidden (mozreview-request)
(Assignee)

Comment 3

3 months ago
@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".
Assignee: nobody → francesco.lodolo

Comment 4

3 months ago
mozreview-review
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? :)
Attachment #8944234 - Flags: review?(jhofmann) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 6

3 months ago
(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 hidden (mozreview-request)
(Assignee)

Comment 8

3 months ago
mozreview-review
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

3 months ago
Pushed by francesco.lodolo@mozillaitalia.org:
https://hg.mozilla.org/integration/autoland/rev/980a3a48f2af
Use proper plural form for number of visits in Page Info r=johannh

Comment 10

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/980a3a48f2af
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
You need to log in before you can comment on or make changes to this bug.