853126 - Use plural form for blocked pop-up dialog

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Francesco Lodolo [:flod]]

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#428

Current implementation
popup.warning=%S prevented this site from opening a pop-up window. Would you like to show it?
popup.warningMultiple=%S prevented this site from opening %S pop-up windows. Would you like to show them?

This code should use the current plural form
https://developer.mozilla.org/en/docs/Localization_and_Plurals

Note that the same problem exists in Firefox for Android (https://bugzilla.mozilla.org/show_bug.cgi?id=739757#c30) and Firefox Metro (https://bugzilla.mozilla.org/show_bug.cgi?id=840888#c21), not sure if a single bug makes sense or we need a separate bug for each platform.

Comment 1

[Matt Brubeck (:mbrubeck)]

Attachment: metro patch

Desktop and mobile patches will follow shortly.

Comment 2

[Matt Brubeck (:mbrubeck)]

Attachment: patch

Comment 3

[Matt Brubeck (:mbrubeck)]

Attachment: android patch

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 727788 [details] [diff] [review]
patch

>+# LOCALIZATION NOTE (popupWarning2): Semicolon-separated list of plural forms.
>+# #1 is brandShortName and #2 is the number of pop-ups blocked.
>+popupWarning2=#1 prevented this site from opening a pop-up window.;#1 prevented this site from opening #2 pop-up windows.

I'd prefer popupWarning.message

Comment 5

[:Margaret Leibovic]

Comment on attachment 727790 [details] [diff] [review]
android patch

Review of attachment 727790 [details] [diff] [review]:
-----------------------------------------------------------------

r- because the string wasn't copied over correctly.

::: mobile/android/locales/en-US/chrome/browser.properties
@@ +46,5 @@
>  # Popup Blocker
> +
> +# LOCALIZATION NOTE (popup.warning2): Semicolon-separated list of plural forms.
> +# #1 is brandShortName and #2 is the number of pop-ups blocked.
> +popup.warning2=#1 prevented this site from opening a pop-up window.;#1 prevented this site from opening #2 pop-up windows.

Looks like you forgot to include the question part at the end (our strings are different from desktop now).

Naming the string popup.message or popup.ask would also be more consistent with our other notification entity names, if you care to do that.

Comment 6

[Matt Brubeck (:mbrubeck)]

Attachment: android patch v2

Oops!  Good catch.

Comment 7

[Matt Brubeck (:mbrubeck)]

Attachment: android patch v3

Argh, messed this up again.  Sorry.

Comment 8

[:Margaret Leibovic]

Comment on attachment 727900 [details] [diff] [review]
android patch v3

Oops, and I missed the mistake that time! Good thing there are two of us :)

Comment 9

[Allison Naaktgeboren :ally]

Comment on attachment 727784 [details] [diff] [review]
metro patch

Review of attachment 727784 [details] [diff] [review]:
-----------------------------------------------------------------

Ship it!

The little winged :gps on my shoulder points out that if it takes >2 function invocations & >2 lines for 1 assignment, to split things across intermediate variables for readability. 
That said, this matches the other two patches, so please ignore the winged :gps in favor of keeping the code consistent across 3 products.

Comment 10

[Matt Brubeck (:mbrubeck)]

(In reply to Dão Gottwald [:dao] from comment #4)
> I'd prefer popupWarning.message

Changed the name to popupWarning.message in the desktop and Metro patches.

https://hg.mozilla.org/integration/mozilla-inbound/rev/926a4d3be235
https://hg.mozilla.org/integration/mozilla-inbound/rev/62b9af51ea89
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b76d9b32786

Comment 11

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/926a4d3be235
https://hg.mozilla.org/mozilla-central/rev/62b9af51ea89
https://hg.mozilla.org/mozilla-central/rev/8b76d9b32786