872142 - Make SelectHelper use PromptService asynchronously

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/SelectHelper.js#24

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch v1

This uses the Prompt.jsm stuff I put up in bug 875119 for the SelectHelper. Because I changed message formats a bit, there are some non-trivial logic changes here. 

Before we sent (and still do send) a message to java with a flat list of menuitems to show. Some of those are labeled as group header rows, and some as rows inside a group:
 
msg = {
  ...
  listitems: [
    { label: "one", isGroup: true },
    { label: "two", inGroup: true },
  ]
}

to simplify life, I didn't want consumers to have to think about this, so the api would take instead:

prompt.setSingleChoiceListItems([
    { label: "one", children: ["two"] },
]);

This adapts this service to handle the new format (and makes it a bit more recursive which I think is probably a good thing for code reuse). Note, they still have to do some work to decipher the results. I wouldn't block either of these bugs on having that perfectly defined, but I could be talked into it.

Comment 2

[Wesley Johnston (:wesj)]

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

I think I'll hold off here until I've got the API a little better defined. Sorry for the churn brian!

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch

I've come to accept my "children" syntax was a bad idea. This updates the select helper to use the patch in bug 877467, which removes it.

Comment 4

[Brian Nicholson (:bnicholson)]

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

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

r- for fixing selected array and the aParent parameter.

::: mobile/android/chrome/content/SelectHelper.js
@@ +26,2 @@
>  
> +    var p = new Prompt({

I'd s/var/let/ to be consistent with the rest of the function.

@@ +37,5 @@
>      }
>  
> +    p.show((function(data) {
> +      let selected = data.button;
> +      console.log(selected);

Drop logging

@@ +40,5 @@
> +      let selected = data.button;
> +      console.log(selected);
> +      if (selected == -1)
> +          return;
> +  

Nit: trailing whitespace

@@ +41,5 @@
> +      console.log(selected);
> +      if (selected == -1)
> +          return;
> +  
> +      var changed = false;

s/var/let

@@ +47,5 @@
> +        aElement.selectedIndex = selected;
> +      } else if (aElement instanceof HTMLSelectElement) {
> +        if (!Array.isArray(selected)) {
> +          let temp = [];
> +          for (let i = 0; i <= selected; i++) {

Shouldn't this be "i <= list.listitems.length" like it was before? If you have a list of size 10 and a selected index of 5, this will create a list only of size 6. Indexes 6-9 would be undefined, and since undefined != false, that would trigger fake change events below.

@@ +52,5 @@
> +            temp[i] = (i == selected);
> +          }
> +          selected = temp;
> +        }
> +        console.log("2: " + selected);

Drop logging

@@ +56,5 @@
> +        console.log("2: " + selected);
> +
> +        let i = 0;
> +        this.forOptions(aElement, function(aNode) {
> +          console.log("3: " + i + " - " + selected[i]);

Drop logging

@@ +59,5 @@
> +        this.forOptions(aElement, function(aNode) {
> +          console.log("3: " + i + " - " + selected[i]);
> +          if (aNode.selected != selected[i])
> +            changed = true;
> +          aNode.selected = selected[i++];

I'd add this line to the inside of the if statement (and move the i++ outside of it) since there's no need to set its selected state if it hasn't changed.

@@ +62,5 @@
> +            changed = true;
> +          aNode.selected = selected[i++];
> +        });
> +      }
> +  

Nit: trailing whitespace

@@ +101,3 @@
>      let parent = aElement;
>      if (aElement instanceof Ci.nsIDOMXULMenuListElement)
>        parent = aElement.menupopup;

What is this new aParent parameter for? By default, parent is set to aElement right here. I never see you setting it to anything other than the element you're passing in, so couldn't we just use the parent variable here? It's confusing that we have two different parent variables in this function used in different places.

@@ +110,3 @@
>  
>      for (let i = 0; i < numChildren; i++) {
> +

Nit: remove newline

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch v2

(In reply to Brian Nicholson (:bnicholson) from comment #4)
> Shouldn't this be "i <= list.listitems.length" like it was before? If you
> have a list of size 10 and a selected index of 5, this will create a list
> only of size 6. Indexes 6-9 would be undefined, and since undefined !=
> false, that would trigger fake change events below.

I wanted to do something like this to try and reduce the workload, so instead I added a break in the forOptions loop that can be triggered by the caller if they want to stop iterating.

> What is this new aParent parameter for? By default, parent is set to
> aElement right here. I never see you setting it to anything other than the
> element you're passing in, so couldn't we just use the parent variable here?
> It's confusing that we have two different parent variables in this function
> used in different places.

These two "parents" are different. I just renamed one to element. We need aParent to determine 1.) If this element is a child and 2.) If it should be disabled.

Comment 6

[Brian Nicholson (:bnicholson)]

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

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

::: mobile/android/chrome/content/SelectHelper.js
@@ +39,5 @@
> +    p.show((function(data) {
> +      let selected = data.button;
> +      if (selected == -1)
> +          return;
> +  

Nit: trailing space

@@ +59,5 @@
> +            changed = true;
> +            aNode.selected = selected[i];
> +          }
> +          i++
> +          return i < selected.length;

I think there's still a potential bug here if we return early. Say we have a multi-select list of two items, and the selection was previously [true, true]. Then we show the prompt and get data.button = 0, which is equivalent to getting [true, false] if I understand correctly. So returning early would compare only the first element and not fire a change event even though the second element changed, right?

(In reply to Wesley Johnston (:wesj) from comment #5)
> I wanted to do something like this to try and reduce the workload, so
> instead I added a break in the forOptions loop that can be triggered by the
> caller if they want to stop iterating.

OK, though the workload just to initialize an array to some booleans would be infinitesimal unless the list was massive (and we'd have much bigger problems at that point). If you want to save some condition checks, you could initialize the entire array to false and then set the selected index to true in a single statement after that.

@@ +62,5 @@
> +          i++
> +          return i < selected.length;
> +        });
> +      }
> +  

Nit: trailing space

@@ +103,2 @@
>      if (aElement instanceof Ci.nsIDOMXULMenuListElement)
> +      element = aElement.menupopup;

If you want, you could save yourself a variable here and just reuse aElement.

Comment 7

[Wesley Johnston (:wesj)]

(In reply to Brian Nicholson (:bnicholson) from comment #6)
> I think there's still a potential bug here if we return early. Say we have a
> multi-select list of two items, and the selection was previously [true,
> true]. Then we show the prompt and get data.button = 0, which is equivalent
> to getting [true, false] if I understand correctly. So returning early would
> compare only the first element and not fire a change event even though the
> second element changed, right?
This code path is only hit for single select elements. But having to think this hard about it makes me agree with you, its not worth the effort for the tiny win. I'll update the patch.

> problems at that point). If you want to save some condition checks, you
> could initialize the entire array to false and then set the selected index
> to true in a single statement after that.
Good idea. Will do.

Comment 8

[Brian Nicholson (:bnicholson)]

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

Sounds good to me.

Comment 9

[Wesley Johnston (:wesj)]

Despite what I thought, I don't think there is a simple way to initialize a javascript array. I thought I could use:

var a = new Array(10).map(function() return false; );

but map actually has some strangeness where this doesn't work. I just landed with the same for loop I always had:

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

Comment 10

[Ed Morley [:emorley]]

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