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.

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

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

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?

(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 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 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.

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

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).

(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 on attachment 721036 [details] [diff] [review] Patch v2 please ask the module owner for review, I'm currently not able to help with PSM

(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!

Review ping?

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

(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 on attachment 721036 [details] [diff] [review] Patch v2 Looks fine, barely even a UI "change". :)

Thanks for the reviews!