273949 - Replace cacertexists.xul with a call to nsIPromptService::alert

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect, P5)

Description

[Stefan Borggraefe]

Follow-up to bug 251991. See bug 251991 comment 27 and bug 251991 comment 33.

The file cacertexists.xul only contains a <description> and an Ok button. So
this file isn't really needed, because we have nsIPromptService for this kind of
standard dialog.

Comment 1

[Kai Engert (:KaiE:)]

I'm reassigning to nobody.
The suggestion sounds reasonable, but I don't think it's urgent to work on the bug.

Comment 2

[:Cykesiopka]

Attachment: WIP: Conversion Patch v1

I tested using a very basic extension, and it seems to work...

Comment 3

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

Comment on attachment 720360 [details] [diff] [review]
WIP: Conversion Patch v1

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

It seems like this patch should remove, at least, cacertexists.xul.

David, could you review this please?

Comment 4

[:Cykesiopka]

(In reply to Brian Smith (:bsmith) from comment #3)
> Comment on attachment 720360 [details] [diff] [review]
> WIP: Conversion Patch v1
> 
> Review of attachment 720360 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It seems like this patch should remove, at least, cacertexists.xul.
> 
> David, could you review this please?

Yes, but I thought I would get feedback on the switch to the prompt service part first..

Comment 5

[Justin Dolske [:Dolske]]

Comment on attachment 720360 [details] [diff] [review]
WIP: Conversion Patch v1

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

::: security/manager/pki/src/nsNSSDialogs.cpp
@@ +221,3 @@
>  
> +    nsXPIDLString title;
> +    mPIPStringBundle->GetStringFromName(NS_ConvertASCIItoUTF16("caCertExistsTitle").get(), getter_Copies(title));

You should be able to use bundle->GetStringFromName(NS_LITERAL_STRING("brandShortName").get(), ...), as well as nsAutoString instead of the acronym-heavy nsXPIDLString.

@@ -223,4 @@
>  
> -  
> -  rv = nsNSSDialogHelper::openDialog(parent, 
> -                                     "chrome://pippki/content/cacertexists.xul",

You'll presumably also want to remove this file and the strings/manifests associated with it.

Comment 6

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

Comment on attachment 720360 [details] [diff] [review]
WIP: Conversion Patch v1

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

Looks like a good start. I don't have anything to add to what's already been said.

Comment 7

[:Cykesiopka]

Attachment: Patch v1

Thanks for the feedback!

I tried to match the coding style of the rest of the file. Hope I haven't failed horribly...

Comment 8

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

Comment on attachment 720977 [details] [diff] [review]
Patch v1

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

This looks good to me, but I'm not really familiar with this code, nor am I a PSM peer, so I'll just mark this as feedback.

::: security/manager/pki/src/nsNSSDialogs.cpp
@@ +232,2 @@
>  
> +  promptSvc->Alert(parent, title.get(), msg.get());

I think you want to do nsresult rv = promptSvc->Alert... here and return rv (the same might go for the mPIPStringBundle->GetStringFromName calls, except using NS_ENSURE_SUCCESS(rv, rv) instead of return, but since those should really never fail, maybe not).

Comment 9

[:Cykesiopka]

(In reply to David Keeler (:keeler) from comment #8)
> Comment on attachment 720977 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 720977 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good to me, but I'm not really familiar with this code, nor am I
> a PSM peer, so I'll just mark this as feedback.
> 
> ::: security/manager/pki/src/nsNSSDialogs.cpp
> @@ +232,2 @@
> >  
> > +  promptSvc->Alert(parent, title.get(), msg.get());
> 
> I think you want to do nsresult rv = promptSvc->Alert... here and return rv
> (the same might go for the mPIPStringBundle->GetStringFromName calls, except
> using NS_ENSURE_SUCCESS(rv, rv) instead of return, but since those should
> really never fail, maybe not).

Thanks for the feedback!

I've updated the patch with the rv and NS_ENSURE_SUCCESS bits (may as well err on the side of caution).

Comment 10

[:Cykesiopka]

Attachment: Patch v2

Comment 11

[Kai Engert (:KaiE:)]

Comment on attachment 721036 [details] [diff] [review]
Patch v2

please ask the module owner for review, I'm currently not able to help with PSM

Comment 12

[:Cykesiopka]

(In reply to Kai Engert (:kaie) from comment #11)
> Comment on attachment 721036 [details] [diff] [review]
> Patch v2
> 
> please ask the module owner for review, I'm currently not able to help with
> PSM

Ok, thanks anyways!

Comment 13

[:Cykesiopka]

Review ping?

Comment 14

[:Cykesiopka]

bsmith: Since you seem busy with a bunch of other bugs, is there someone you recommend to review the patch?

Comment 15

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

(In reply to Cykesiopka from comment #14)
> bsmith: Since you seem busy with a bunch of other bugs, is there someone you
> recommend to review the patch?

Somebody from Toolkit/Firefox team should look at it because it is a UI patch. But, it looks reasonable to me.

Comment 16

[Justin Dolske [:Dolske]]

Comment on attachment 721036 [details] [diff] [review]
Patch v2

Looks fine, barely even a UI "change". :)

Comment 17

[:Cykesiopka]

Thanks for the reviews!

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/78db908ccff3

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/78db908ccff3