877911 - Make PromptService component use async api

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

Attachment: Patch

Comment 1

[:Margaret Leibovic]

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

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

This is generally looking good, but I'm curious to hear the answer to some questions before giving an r+.

::: mobile/android/components/PromptService.js
@@ +123,5 @@
>  
>    QueryInterface: XPCOMUtils.generateQI([Ci.nsIPrompt, Ci.nsIAuthPrompt, Ci.nsIAuthPrompt2]),
>  
>    /* ---------- internal methods ---------- */
> +  _getPrompt: function(aTitle, aText, aButtons, aCheckMsg, aCheckState) {

Nit: For consistency, make this a named function.

@@ +124,5 @@
>    QueryInterface: XPCOMUtils.generateQI([Ci.nsIPrompt, Ci.nsIAuthPrompt, Ci.nsIAuthPrompt2]),
>  
>    /* ---------- internal methods ---------- */
> +  _getPrompt: function(aTitle, aText, aButtons, aCheckMsg, aCheckState) {
> +    var p = new Prompt({

Nit: Use let (same goes for down below).

@@ +137,5 @@
>  
> +    if (aCheckMsg) {
> +      p.addCheckbox({
> +        label: PromptUtils.cleanUpLabel(aCheckMsg),
> +        checked: aCheckState.value

It seems potentially buggy that you're just checking for aCheckMsg and then assuming aCheckState isn't null. I know we're in control of all the places this is called, but maybe if you need aCheckMsg and aCheckState to both not be null, you should check that, or combine them into one parameter.

@@ +144,5 @@
> +
> +    return p;
> +  },
> +
> +  showPrompt: function showPrompt(aPrompt) {

Can you add a comment to the top of this explaining that it spins the event loop waiting for a response?

@@ +222,2 @@
>      if (aCheckMsg)
> +      aCheckState.value = data.checkbox0 == "true";

What's with this checkbox0 property? I haven't looked at exactly how the prompt API works. Could you explain?

::: mobile/android/modules/Prompt.jsm
@@ +49,5 @@
>      this[aOptions.type + "_count"]++;
>  
> +    if (!this.msg.inputs)
> +        this.msg.inputs = [];
> +

As far as I can tell, this is just fixing something that's busted in Prompt.jsm. Am I right? (It's fine if it is, I'm just trying to understand the need for this change.)

Comment 2

[Wesley Johnston (:wesj)]

(In reply to :Margaret Leibovic from comment #1)
> It seems potentially buggy that you're just checking for aCheckMsg and then
> assuming aCheckState isn't null. I know we're in control of all the places
> this is called, but maybe if you need aCheckMsg and aCheckState to both not
> be null, you should check that, or combine them into one parameter.

Hmm.. let me run some tests on us and desktop and try to make sure we're in line with one another. I think we likely SHOULD throw if you pass a null checkValue or checkMessage, but we want to not show the checkbox if checkMessage.value is empty, null, or undefined. If checkValue.value is null or undefined... I don't think we care.

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/prompts/src/CommonDialog.jsm#133

> What's with this checkbox0 property? I haven't looked at exactly how the
> prompt API works. Could you explain?

With prompt messages before prompt.jsm, you could do something to add a checkbox like:

var d = sendMessageToJava({
  // normal type properties and stuff
  inputs: [ { type: "checkbox", label: "my checkbox" }]
});
var res = d.checkbox

and in the return value d.checkbox would have the checkbox state. That worked well until kats tried to create a dialog with two of the same type of input on it (two textboxes I think). Then d.textbox only held the value of one textbox. So we add an optional id attribute:

var d = sendMessageToJava({
  // normal type properties and stuff
  inputs: [
    { type: "checkbox", label: "my checkbox" },
    { type: "checkbox", id: "lenny", label: "my checkbox" }
  ]
});
var res = d.checkbox;
var res2 = d.lenny;

Here I wanted to remove the chances of dataloss altogether, so if you don't pass an id, we autogenerate one with an int on the end based on the order that you added the inputs.

new Prompt({ }).addCheckbox({ label: "my checkbox" })
               .addCheckbox({ label: "my other checkbox" })
               .show(function(d) {
                 var res = d.checkbox0;
                 var res2 = d.checkbox1;
               });

> As far as I can tell, this is just fixing something that's busted in
> Prompt.jsm. Am I right? (It's fine if it is, I'm just trying to understand
> the need for this change.)

Yes, and this change landed in a different bug (or will soon...)

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch

(In reply to :Margaret Leibovic from comment #1)
> It seems potentially buggy that you're just checking for aCheckMsg and then
> assuming aCheckState isn't null. I know we're in control of all the places
> this is called, but maybe if you need aCheckMsg and aCheckState to both not
> be null, you should check that, or combine them into one parameter.

Passing a null checkState into the prompt service will throw from xpconnect (its an outParam so it has to be defined).

But this is an internal function, so maybe its best to just check. We probably WANT to throw if you pass a checkmsg but an invalid checkstate though... I'm torn and just added the check.

Comment 4

[:Margaret Leibovic]

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

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

Given your last comment, maybe it does make sense to just check for aCheckMsg. But it would be good to comment on why that's okay, if only to help out future people who look at this code.

::: mobile/android/components/PromptService.js
@@ +124,5 @@
>    QueryInterface: XPCOMUtils.generateQI([Ci.nsIPrompt, Ci.nsIAuthPrompt, Ci.nsIAuthPrompt2]),
>  
>    /* ---------- internal methods ---------- */
> +  _getPrompt: function _getPrompt(aTitle, aText, aButtons, aCheckMsg, aCheckState) {
> +    var p = new Prompt({

Nit: let

@@ +225,2 @@
>      if (aCheckMsg)
> +      aCheckState.value = data.checkbox0 == "true";

I know this is consistent with the current code, but this is that same aCheckMsg/aCheckState dependency. It seems like it would make more sense for this check to be fore aCheckState, since that's what's being used.

@@ +238,3 @@
>      let ok = data.button == 0;
>      if (aCheckMsg)
> +      aCheckState.value = data.checkbox0 == "true";

Same thing here.

@@ +281,5 @@
>      }
>  
> +    let p = this._getPrompt(aTitle, aText, buttons, aCheckMsg, aCheckState);
> +    let data = this.showPrompt(p);
> +    aCheckState.value = data.checkbox0 == "true";

Seems weird that there's no check for aCheckMsg (or aCheckState) here.

Comment 5

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0f606080683

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a0f606080683