Closed Bug 986724 Opened 6 years ago Closed 6 years ago

security > passwords: strings for error message if FIPS active not defined, error popup not shown when deactivating master password

Categories

(Thunderbird :: Preferences, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 31.0

People

(Reporter: aryx, Assigned: aryx)

Details

Attachments

(2 files, 6 obsolete files)

If FIPS has been activated and the master password shall be removed, the message informing the user that this is not possible is not shown.

Error message:
Error: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIStringBundle.GetStringFromName]
Source File: XStringBundle
Line: 21

Reason:
The strings

pw_change_failed_title
pw_change2empty_in_fips_mode

are missing from preferences.properties. Same applies to /im/
Attached patch mail, v1 (obsolete) — Splinter Review
Attachment #8395095 - Flags: review?(mkmelin+mozilla)
Attached patch im, v1 (obsolete) — Splinter Review
Attachment #8395096 - Flags: review?(florian)
Comment on attachment 8395095 [details] [diff] [review]
mail, v1

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

Nice catch! r=mkmelin
Attachment #8395095 - Flags: review?(mkmelin+mozilla) → review+
Have you considered changing the code to use the bundle at http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/locales/en-US/chrome/mozapps/preferences/preferences.properties instead of duplicating these strings?

Eg. add a <stringbundle id="bundlePasswordPreferences" src="chrome://mozapps/locale/preferences/preferences.properties"/> element to the xul file, and change the JS code to use this bundle.
Comment on attachment 8395096 [details] [diff] [review]
im, v1

r- until comment 4 is answered; but thanks for uncovering this bug! :-)
Attachment #8395096 - Flags: review?(florian) → review-
FWIW, all the other kids are doing it :)
(In reply to Magnus Melin from comment #6)
> FWIW, all the other kids are doing it :)

By "all the other kids", do you mean http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/preferences/preferences.properties#29 ? (It's the only example I can find of a toolkit based application duplicating these strings.)
Depends on what you mean by application. It's also in mozilla/security/manager (where it might belong for real)
(In reply to Magnus Melin from comment #8)
> Depends on what you mean by application. It's also in
> mozilla/security/manager (where it might belong for real)

And there it seems to be used by compiled code that likely can't really depend on toolkit/.
I think flod can tell us if it makes sense to duplicate these 2 strings to mail/ and im/ or if the toolkit/ strings should be used (and also we could remove them from browser/).
Flags: needinfo?(francesco.lodolo)
From a translation point of view, it makes sense to reuse these strings (same strings, same context, etc.). 

I wonder if there's a technical reason for not doing that in the first place, considering this is what browser did.
Flags: needinfo?(francesco.lodolo)
Attached patch mail, v2 (obsolete) — Splinter Review
Now using existing toolkit strings.
Attachment #8395095 - Attachment is obsolete: true
Attachment #8413420 - Flags: review?(mkmelin+mozilla)
Attached patch im, v2 (obsolete) — Splinter Review
Now using existing toolkit strings.
Attachment #8395096 - Attachment is obsolete: true
Attachment #8413421 - Flags: review?(florian)
Comment on attachment 8413420 [details] [diff] [review]
mail, v2

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

Looks good, thx! r=mkmelin
Attachment #8413420 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 8413421 [details] [diff] [review]
im, v2

Thanks!
Attachment #8413421 - Flags: review?(florian) → review+
Attached patch im, v3. r=florian (obsolete) — Splinter Review
Attachment #8413421 - Attachment is obsolete: true
Attached patch mail, v3, r=mkmelin (obsolete) — Splinter Review
Attachment #8413420 - Attachment is obsolete: true
Had exported the patches from the outdated patch queue.
Attachment #8413586 - Attachment is obsolete: true
Had exported the patches from the outdated patch queue.
Attachment #8413587 - Attachment is obsolete: true
https://hg.mozilla.org/comm-central/rev/77e5b383391e
https://hg.mozilla.org/comm-central/rev/f006681a23c0
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 32.0
Target Milestone: Thunderbird 32.0 → Thunderbird 31.0
You need to log in before you can comment on or make changes to this bug.