Last Comment Bug 751428 - [AccessFu] Remove spaces and catch exceptions when localizing role names
: [AccessFu] Remove spaces and catch exceptions when localizing role names
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: mozilla15
Assigned To: Eitan Isaacson [:eeejay]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-02 17:12 PDT by Eitan Isaacson [:eeejay]
Modified: 2012-05-04 02:30 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Remove spaces and catch exceptions when localizing role names. (1.97 KB, patch)
2012-05-02 17:13 PDT, Eitan Isaacson [:eeejay]
no flags Details | Diff | Splinter Review
Remove spaces and catch exceptions when localizing role names. (2.68 KB, patch)
2012-05-02 17:22 PDT, Eitan Isaacson [:eeejay]
dbolter: review+
Details | Diff | Splinter Review

Description Eitan Isaacson [:eeejay] 2012-05-02 17:12:29 PDT
Some role names have spaces, we need to remove the spaces to make it a legal token for localization. Also, use an empty string when the role name is not localizable instead of throwing an exception.
Comment 1 Eitan Isaacson [:eeejay] 2012-05-02 17:13:53 PDT
Created attachment 620532 [details] [diff] [review]
Remove spaces and catch exceptions when localizing role names.
Comment 2 Eitan Isaacson [:eeejay] 2012-05-02 17:16:08 PDT
Comment on attachment 620532 [details] [diff] [review]
Remove spaces and catch exceptions when localizing role names.

Oops
Comment 3 Eitan Isaacson [:eeejay] 2012-05-02 17:22:25 PDT
Created attachment 620536 [details] [diff] [review]
Remove spaces and catch exceptions when localizing role names.

Made a special new method for this, instead of doing the try/catch dance everywhere.
Comment 4 David Bolter [:davidb] ***PTO until 29th*** 2012-05-02 18:18:41 PDT
Comment on attachment 620536 [details] [diff] [review]
Remove spaces and catch exceptions when localizing role names.

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

r=me. Thanks for the explanation over on bug 751435 and IRC - that you need to trim whitespace so that you can use these as the key for l10n lookups.

::: accessible/src/jsat/UtteranceGenerator.jsm
@@ +220,5 @@
>        return [desc, name];
>      }
> +  },
> +
> +  _getLocalizedRole: function(aRoleStr) {

Are you using this convention for denoting mock encapsulation?
Comment 5 David Bolter [:davidb] ***PTO until 29th*** 2012-05-02 18:19:12 PDT
(I mean the underscore prefix)
Comment 6 Eitan Isaacson [:eeejay] 2012-05-02 18:27:47 PDT
(In reply to David Bolter [:davidb] from comment #4)
> Comment on attachment 620536 [details] [diff] [review]
> Remove spaces and catch exceptions when localizing role names.
> 
> Review of attachment 620536 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me. Thanks for the explanation over on bug 751435 and IRC - that you need
> to trim whitespace so that you can use these as the key for l10n lookups.

Thanks!

> 
> ::: accessible/src/jsat/UtteranceGenerator.jsm
> @@ +220,5 @@
> >        return [desc, name];
> >      }
> > +  },
> > +
> > +  _getLocalizedRole: function(aRoleStr) {
> 
> Are you using this convention for denoting mock encapsulation?

No, I am using it for private methods on an object that otherwise has public methods. Something from Python, although I have seen it in other mozilla js. Need to be more consistent with it.
Comment 7 David Bolter [:davidb] ***PTO until 29th*** 2012-05-02 18:33:54 PDT
(In reply to Eitan Isaacson [:eeejay] from comment #6)
> (In reply to David Bolter [:davidb] from comment #4)
> > > +  _getLocalizedRole: function(aRoleStr) {
> > 
> > Are you using this convention for denoting mock encapsulation?
> 
> No, I am using it for private methods on an object that otherwise has public
> methods. Something from Python, although I have seen it in other mozilla js.
> Need to be more consistent with it.

Heh. Encapsulation has two meanings (who knew?) but I meant "a language mechanism for restricting access to some of the object's components." (wikipedia) Anyways, all good. As long as you plan on consistency using _ for mock privacy it is fine with me. It isn't worth doing the Crockford tricks for actual privacy IMO.
Comment 8 alexander :surkov 2012-05-02 20:42:02 PDT
Comment on attachment 620536 [details] [diff] [review]
Remove spaces and catch exceptions when localizing role names.

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

::: accessible/src/jsat/UtteranceGenerator.jsm
@@ +220,5 @@
>        return [desc, name];
>      }
> +  },
> +
> +  _getLocalizedRole: function(aRoleStr) {

pls don't use unnamed functions

@@ +222,5 @@
> +  },
> +
> +  _getLocalizedRole: function(aRoleStr) {
> +    try {
> +      return gStringBundle.GetStringFromName(aRoleStr.replace(' ', ''));

just curious if properties file is updated to this logic already
Comment 9 Eitan Isaacson [:eeejay] 2012-05-02 22:16:26 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/c608de1b6a53
Comment 10 Ed Morley [:emorley] 2012-05-04 02:30:26 PDT
https://hg.mozilla.org/mozilla-central/rev/c608de1b6a53

Note You need to log in before you can comment on or make changes to this bug.