Closed Bug 853126 Opened 8 years ago Closed 8 years ago

Use plural form for blocked pop-up dialog

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 22

People

(Reporter: flod, Assigned: mbrubeck)

References

Details

Attachments

(3 files, 2 obsolete files)

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.
Assignee: nobody → mbrubeck
Attached patch metro patchSplinter Review
Desktop and mobile patches will follow shortly.
Attachment #727784 - Flags: review?(ally)
Attached patch patchSplinter Review
Attachment #727788 - Flags: review?(dao)
Attached patch android patch (obsolete) — Splinter Review
Attachment #727790 - Flags: review?(margaret.leibovic)
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
Attachment #727788 - Flags: review?(dao) → review+
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.
Attachment #727790 - Flags: review?(margaret.leibovic) → review-
Attached patch android patch v2 (obsolete) — Splinter Review
Oops!  Good catch.
Attachment #727790 - Attachment is obsolete: true
Attachment #727895 - Flags: review?(margaret.leibovic)
Attachment #727895 - Flags: review?(margaret.leibovic) → review+
Attached patch android patch v3Splinter Review
Argh, messed this up again.  Sorry.
Attachment #727895 - Attachment is obsolete: true
Attachment #727900 - Flags: review?(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 :)
Attachment #727900 - Flags: review?(margaret.leibovic) → review+
Attachment #727895 - Flags: review+
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.
Attachment #727784 - Flags: review?(ally) → review+
(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
Status: NEW → ASSIGNED
See Also: → 973346
You need to log in before you can comment on or make changes to this bug.