445371 - In </security/*>, "use a xul <stringbundle/> instead of including the strres.js code"

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[Serge Gautherie (:sgautherie)]

Found 23 matching lines in 23 files

Comment 1

[Serge Gautherie (:sgautherie)]

These are the last actual uses of strres.js in mozilla-central...

Comment 2

[:Cykesiopka]

Attachment: Proposed Patch

Comment 3

[:Cykesiopka]

(In reply to Cykesiopka from comment #2)
> Created attachment 720398 [details] [diff] [review]
> Proposed Patch

Probably should have split this up into the .xul and .js parts, but oh well...

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 720398 [details] [diff] [review]
Proposed Patch

David, can you review this? You probably know more about front-end stuff than I do.

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 720398 [details] [diff] [review]
Proposed Patch

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

The usage of stringbundle looks consistent to other uses I've seen.
I just have a few comments on formatting, etc.

::: security/manager/pki/resources/content/certManager.js
@@ +418,5 @@
>      return;
>  
>    var params = Components.classes[nsDialogParamBlock].createInstance(nsIDialogParamBlock);
>    
> +  var bundle = document.getElementById("pippki_bundle");

It doesn't look like this is even used in this function anymore - you can probably just delete it.

::: security/manager/pki/resources/content/certManager.xul
@@ +21,5 @@
>          buttons="accept"
>          style="width: 48em; height: 32em;"
>          persist="screenX screenY width height">
>  
> +  <stringbundle id="pippki_bundle" src="chrome://pippki/locale/pippki.properties"/>

It looks like this is called pkiBundle elsewhere (e.g. page info), but I'm not sure it's that important.

::: security/manager/pki/resources/content/device_manager.js
@@ +107,5 @@
>  {
>    var fipsButton = document.getElementById("fipsbutton");
>    var label;
>    if (secmoddb.isFIPSEnabled) {
> +   label = bundle.getString("disable_fips"); 

Trailing space here and on the next 15 changed lines.

@@ +343,5 @@
>      selected_token.login(false);
>      var tok_status = document.getElementById("tok_status");
>      if (selected_token.isLoggedIn()) {
>        tok_status.setAttribute("label", 
> +                          bundle.getString("devinfo_stat_loggedin"));

I assume this line and the next changed one have strange alignment due to an 80 character limit. I'm not sure what the correct style is here - maybe Brian will know.

@@ +473,5 @@
>  function showTokenInfo()
>  {
>    //ClearInfoList();
>    var selected_token = selected_slot.getToken();
> +  AddInfoRow(bundle.getString("devinfo_label"), 

Trailing space here and again a few lines later.

::: security/manager/pki/resources/content/exceptionDialog.js
@@ +55,1 @@
>    

I'm not sure this empty line was ever necessary here.

::: security/manager/pki/resources/content/pippki.js
@@ +152,5 @@
>    }
>    if (written != content.length) {
>      if (!msg.length)
> +      msg = bundle.getString("writeFileUnknownError");
> +    alertPromptService(bundle.getString("writeFileFailure"),

This should be indented to line up with msg (use spaces, please).

::: security/manager/pki/resources/content/pref-crlupdate.js
@@ +171,5 @@
>    if( cnt > 0 ){
>      errorCountText.setAttribute("value",cnt);
>      errorDetailsText.setAttribute("value",txt);
>    } else {
> +    errorCountText.setAttribute("value",bundle.getString("NoUpdateFailure"));

Have a space here after the comma.

Comment 6

[:Cykesiopka]

(In reply to David Keeler (:keeler) from comment #5)
> Comment on attachment 720398 [details] [diff] [review]
> Proposed Patch
> 
> Review of attachment 720398 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The usage of stringbundle looks consistent to other uses I've seen.
> I just have a few comments on formatting, etc.
> 
> ::: security/manager/pki/resources/content/certManager.js
> @@ +418,5 @@
> >      return;
> >  
> >    var params = Components.classes[nsDialogParamBlock].createInstance(nsIDialogParamBlock);
> >    
> > +  var bundle = document.getElementById("pippki_bundle");
> 
> It doesn't look like this is even used in this function anymore - you can
> probably just delete it.

Done.

> ::: security/manager/pki/resources/content/certManager.xul
> @@ +21,5 @@
> >          buttons="accept"
> >          style="width: 48em; height: 32em;"
> >          persist="screenX screenY width height">
> >  
> > +  <stringbundle id="pippki_bundle" src="chrome://pippki/locale/pippki.properties"/>
> 
> It looks like this is called pkiBundle elsewhere (e.g. page info), but I'm
> not sure it's that important.

Mmm, "pippki_bundle" was already in some of the xul files, so I just reused it...
I'm leaving it for now, but I can change it to "pkiBundle" later if needed.

> ::: security/manager/pki/resources/content/device_manager.js
> @@ +107,5 @@
> >  {
> >    var fipsButton = document.getElementById("fipsbutton");
> >    var label;
> >    if (secmoddb.isFIPSEnabled) {
> > +   label = bundle.getString("disable_fips"); 
> 
> Trailing space here and on the next 15 changed lines.

Fixed. Hopefully...

> @@ +343,5 @@
> >      selected_token.login(false);
> >      var tok_status = document.getElementById("tok_status");
> >      if (selected_token.isLoggedIn()) {
> >        tok_status.setAttribute("label", 
> > +                          bundle.getString("devinfo_stat_loggedin"));
> 
> I assume this line and the next changed one have strange alignment due to an
> 80 character limit. I'm not sure what the correct style is here - maybe
> Brian will know.

I went ahead and aligned them.
The rest of the code has mostly aligned stuff, and it doesn't seem like this line or the others like it break 80 chars post alignment anyways...

> @@ +473,5 @@
> >  function showTokenInfo()
> >  {
> >    //ClearInfoList();
> >    var selected_token = selected_slot.getToken();
> > +  AddInfoRow(bundle.getString("devinfo_label"), 
> 
> Trailing space here and again a few lines later.

Again hopefully fixed.

> ::: security/manager/pki/resources/content/exceptionDialog.js
> @@ +55,1 @@
> >    
> 
> I'm not sure this empty line was ever necessary here.

Done.

> ::: security/manager/pki/resources/content/pippki.js
> @@ +152,5 @@
> >    }
> >    if (written != content.length) {
> >      if (!msg.length)
> > +      msg = bundle.getString("writeFileUnknownError");
> > +    alertPromptService(bundle.getString("writeFileFailure"),
> 
> This should be indented to line up with msg (use spaces, please).

Hmm, aren't they of different logic levels...? The if statement isn't braced...

> ::: security/manager/pki/resources/content/pref-crlupdate.js
> @@ +171,5 @@
> >    if( cnt > 0 ){
> >      errorCountText.setAttribute("value",cnt);
> >      errorDetailsText.setAttribute("value",txt);
> >    } else {
> > +    errorCountText.setAttribute("value",bundle.getString("NoUpdateFailure"));
> 
> Have a space here after the comma.

Done.

Thanks for the review!

Comment 7

[:Cykesiopka]

Attachment: Patch for check in

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Needs to be reviewed by a module peer first.

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 721123 [details] [diff] [review]
Patch for check in

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

Thanks for the patch. I will review this in the next couple of days.

Comment 10

[:Cykesiopka]

(In reply to Brian Smith (:bsmith) from comment #8)
> Needs to be reviewed by a module peer first.

Oops! Sorry about that...

Comment 11

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

LGTM.

David, when you reviewed this, did you check out the results of the build? There are no tests for this stuff so it's worth making sure that nothing broke before checking it in. I can test it if you didn't.

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to Brian Smith (:bsmith) from comment #11)
> LGTM.
> 
> David, when you reviewed this, did you check out the results of the build?
> There are no tests for this stuff so it's worth making sure that nothing
> broke before checking it in. I can test it if you didn't.

I didn't actually test it out, no.

(In reply to Cykesiopka from comment #6)
> > ::: security/manager/pki/resources/content/pippki.js
> > @@ +152,5 @@
> > >    }
> > >    if (written != content.length) {
> > >      if (!msg.length)
> > > +      msg = bundle.getString("writeFileUnknownError");
> > > +    alertPromptService(bundle.getString("writeFileFailure"),
> > 
> > This should be indented to line up with msg (use spaces, please).
> 
> Hmm, aren't they of different logic levels...? The if statement isn't
> braced...

Ah, yes - good catch :)

Comment 13

[:Cykesiopka]

bsmith: can this be checked in now, or is testing still needed?

Comment 14

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

David, could you please test this out and check it in? Otherwise, I can do so, but there will be a pretty big delay.

Comment 15

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

I built locally with no problems, tried out some of the UI that would have been affected, and everything looked good. Here's a try run for what it's worth: https://tbpl.mozilla.org/?tree=Try&rev=b36f7e42b117
I'll check that in if everything looks good.

Comment 16

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d24c96dfd4d

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0d24c96dfd4d

Comment 18

[Serge Gautherie (:sgautherie)]

V.Fixed, per MXR search.
(Thanks!)