Closed Bug 878848 Opened 7 years ago Closed 7 years ago

Make NSS dialogs use async prompts

Categories

(Firefox for Android :: General, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 24

People

(Reporter: wesj, Assigned: wesj)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Blocks: 870060
Attached patch PatchSplinter Review
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?
Attachment #758288 - Flags: feedback?(bugmail.mozilla)
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 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.
Attachment #758288 - Flags: feedback?(bugmail.mozilla) → feedback+
Attached patch Patch v1Splinter Review
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.
Attachment #758713 - Flags: review?(bugmail.mozilla)
Flags: needinfo?(zebardy)
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.
Attachment #758713 - Flags: review?(bugmail.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/9bc4ee218fc0
Assignee: nobody → wjohnston
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
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
Flags: needinfo?(zebardy)
You need to log in before you can comment on or make changes to this bug.