878848 - Make NSS dialogs use async prompts

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/NSSDialogService.js#42

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch

I haven't tested this yet. Hoping kats can point me to an easy way to do so? In the meantime, feedback instead of review?

Comment 2

[Kartikaya Gupta (email:[e-mail])]

You can test one of the dialogs by trying to install a cert, e.g. from http://www.cacert.org/index.php?id=3

Some of the other dialogs (added in bug 774964 and bug 845375) were tested by other people and so I don't know how to exercise that code exactly. You can try needinfo'ing the contributor for that info.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 758288 [details] [diff] [review]
Patch

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

::: mobile/android/components/NSSDialogService.js
@@ +105,5 @@
> +    let prompt = this.getPrompt(this.getString("pkcs12.getpassword.title"),
> +                                this.getString("pkcs12.getpassword.message"),
> +                                [ this.getString("nssdialogs.ok.label"),
> +                                  this.getString("nssdialogs.cancel.label")
> +                                ]).addPassword({id: "pw"});

I'd prefer the addPassword call be bumped down to a new line.

@@ +198,5 @@
> +        checked: rememberSetting
> +      }).show(function(data) {
> +        selectedIndex = data.nicknames;
> +        response = data.button;
> +        rememberBox = data.rememberBox;

Is this show function needed here? You don't use these variables, and this callback probably gets clobbered anyway in the call to showPrompt below.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch v1

Thanks. At least that helped catch a few things. notifyCACertExists was also easy to test. Need-infoing zebardy for info on how to test other prompts, but I feel fine starting review before that.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

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

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

::: mobile/android/components/NSSDialogService.js
@@ +105,5 @@
> +    let prompt = this.getPrompt(this.getString("pkcs12.getpassword.title"),
> +                                this.getString("pkcs12.getpassword.message"),
> +                                [ this.getString("nssdialogs.ok.label"),
> +                                  this.getString("nssdialogs.cancel.label")
> +                                ]).addPassword({id: "pw"});

Add a separate statement for the addPassword call. In other places you've done this:
let prompt = getPrompt(...);
prompt.add(...)
      .add(...);
showPrompt(prompt);

and it would be good to do that here also for consistency.

Comment 6

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9bc4ee218fc0

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9bc4ee218fc0

Comment 8

[zebardy]

Hi,

Sorry have been snowed under with suff. I know this is resolved now, but just for future reference I thought I'd reply to the Info request. I couldn't find any procedure for testing such features automatically already in use by firefox for android, and ended up testing these prompts manually in the emulator and a couple of devices. Where I work we use Client certs extensively for a lot of our systems so I din't need a lot of setup.

Sorry I can't be of more help!

Aaron