830175 - PromptService code needs cleanup

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

While taking a crack in fleshing out my patch on bug 807606, I got lost in the PromptService code and couldn't restrain myself from cleaning it up somehwat. Patches incoming. Try build at https://tbpl.mozilla.org/?tree=Try&rev=7eaf03afc03e

Comment 1

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

Attachment: Part 1 - Make buttons strings in the JSON instead of objects with one property

Comment 2

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

Attachment: Part 2 - Add some helpers

Comment 3

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

Attachment: Part 3 - Rename member variables

Comment 4

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

Attachment: Part 4 - Minor optimizations

Comment 5

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

Attachment: Part 5 - Make the commonPrompt function more generic

This one is kind of half-baked, I realized only afterwards that I can't modify the confirmEx function to take three checkboxes (which is what I wanted to do for bug 807606) since it is defined in an IDL somewhere.

Comment 6

[Wesley Johnston (:wesj)]

Comment on attachment 701632 [details] [diff] [review]
Part 1 - Make buttons strings in the JSON instead of objects with one property

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

This is code I would like to be cc'd on for changes. You'll need to update a few other places here as well (see SelectHelper.js and FormInputHelper.js).

::: mobile/android/components/PromptService.js
@@ +289,5 @@
>    },
>  
>    select: function select(aTitle, aText, aCount, aSelectList, aOutSelection) {
> +    let data = this.commonPrompt(aTitle, aText, [ PromptUtils.getLocaleString("OK") ], "",
> +                                {value: false}, [ {type: "menulist",  values: aSelectList} ]);

I think we always leave spaces after the { and before }

Comment 7

[Wesley Johnston (:wesj)]

Comment on attachment 701633 [details] [diff] [review]
Part 2 - Add some helpers

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

Looks fine. Left Chris in if you still want him to review.

Comment 8

[Wesley Johnston (:wesj)]

Comment on attachment 701634 [details] [diff] [review]
Part 3 - Rename member variables

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

::: mobile/android/base/PromptService.java
@@ +555,5 @@
> +            isGroup = false;
> +            inGroup = false;
> +            disabled = false;
> +            id = 0;
> +            isParent = false;

I don't understand why we'd move the initialization from the declaration to here. It just means more places you have to look to find out what a variable is. I don't see anything about that in ours [1] or Androids [2] coding style guidelines?

Even if we don't though, AFAIK, these are the Java "sensible" defaults, so we don't need to do the initialization at all right?
[1] https://wiki.mozilla.org/Fennec/NativeUI/CodingStyle
[2] http://source.android.com/source/code-style.html

@@ +564,5 @@
>          private static final int VIEW_TYPE_ITEM = 0;
>          private static final int VIEW_TYPE_GROUP = 1;
>          private static final int VIEW_TYPE_COUNT = 2;
>  
> +        public ListView mListView;

public variables don't use m

Comment 9

[Wesley Johnston (:wesj)]

Comment on attachment 701635 [details] [diff] [review]
Part 4 - Minor optimizations

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

::: mobile/android/base/PromptService.java
@@ +122,5 @@
>                      // We can't use our custom version of the DatePicker widget because the sdk is too old.
>                      // But we can fallback on the native one.
>                      DatePicker input = new DatePicker(GeckoApp.mAppContext);
>                      try {
> +                        if (mValue.length() > 0) {

Why don't we just use TextUtils.isEmpty() here? The only reason the old style check is in place is because I didn't know about TextUtils when I was writing this. Same throughout?

Comment 10

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

(In reply to Wesley Johnston (:wesj) from comment #8)
> I don't understand why we'd move the initialization from the declaration to
> here. It just means more places you have to look to find out what a variable
> is. I don't see anything about that in ours [1] or Androids [2] coding style
> guidelines?

I made the variables final, and so they need to be initialized in the constructor. It's also more efficient this way because now it doesn't get initialized twice (once during class init and once during the constructor).

> 
> Even if we don't though, AFAIK, these are the Java "sensible" defaults, so
> we don't need to do the initialization at all right?

Ordinarily, yes, but with final member variables they need to be explicitly initialized.

I'll patch up the other things. For the TextUtils.isEmpty() I figured that we always make sure the strings are non-null so using TextUtils was unnecessary, but it is more readable so I can switch it over.

Comment 11

[Wesley Johnston (:wesj)]

Comment on attachment 701636 [details] [diff] [review]
Part 5 - Make the commonPrompt function more generic

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

I'm not sure I love this. Order is important for the input array, and I worry this will make it easy to accidentally move this code around. But I like moving more things into the input array. A better solution for us longer term might be to create a NativeWindow.prompt setup that takes some objects like this. addCheckbox (or any other input) might be a good addition to that.

Then from the prompt service we can get the chrome window and call those same methods, and you can use it for your 3 checkbox prompt as well.

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

I think this would be better as something like

getCheckboxInput(aMsg, aState) {
  return { type: "checkbox", ... };
}

input = [].concat(getCheckboxInput(msg, state));

@@ +136,2 @@
>  
> +  commonPrompt: function commonPrompt(aTitle, aText, aButtons, aInputs) {

Just make aInputs optional (i.e. aInputs = [])

Comment 12

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

(In reply to Wesley Johnston (:wesj) from comment #11)
> Comment on attachment 701636 [details] [diff] [review]
> >    /* ---------- internal methods ---------- */
> > +  addCheckboxInput: function(aArray, aCheckMsg, aCheckState) {
> 
> I think this would be better as something like
> 
> getCheckboxInput(aMsg, aState) {
>   return { type: "checkbox", ... };
> }
> 
> input = [].concat(getCheckboxInput(msg, state));
> 

I tried something this originally but I needed to have if (msg) checks in all the places that called getCheckboxInput and it became pretty ugly.

I think I'll just leave off this patch for now, and send custom Prompt:Show messages directly from the NSSDialog code.

Comment 13

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

Attachment: Part 1 - Make buttons strings in the JSON instead of objects with one property (v2)

Comment 14

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

Attachment: Part 2 - Add some helpers

Carrying r+

Comment 15

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

Attachment: Part 3 - Rename member variables

Comment 16

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

Attachment: Part 4 - Minor optimizations

Comment 17

[Chris Peterson [:cpeterson]]

Comment on attachment 701904 [details] [diff] [review]
Part 2 - Add some helpers

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

::: mobile/android/base/PromptService.java
@@ +481,5 @@
> +    private static boolean getSafeBool(JSONObject json, String key) {
> +        try {
> +            return json.getBoolean(key);
> +        } catch (Exception e) {
> +            return false;

Are "", false, and 0 always "safe" defaults for getSafe*()? For simple getters like these, I like the pattern where the caller can provide their own fallback value, e.g. getSafeBool(JSONObject json, String key, boolean defaultValue).

For the getSafeArray() methods (for JSONObjects and Strings), returning an empty array is probably good because allocating a default array when calling the function would be annoying and increase memory allocations.

At some point, we should consider moving all these utility functions into a facade around JSONObject or replacing the JSON library we use, if it is this problematic.

Comment 18

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

(In reply to Chris Peterson (:cpeterson) from comment #17)
> Are "", false, and 0 always "safe" defaults for getSafe*()?

That's what all the code was defaulting to before, so it shouldn't be a change in any existing behaviour.

> For simple
> getters like these, I like the pattern where the caller can provide their
> own fallback value, e.g. getSafeBool(JSONObject json, String key, boolean
> defaultValue).

If it were a more general utility method I would agree but in this case I think it would be unnecessarily overgeneralized.

> At some point, we should consider moving all these utility functions into a
> facade around JSONObject or replacing the JSON library we use, if it is this
> problematic.

Agreed. We have a lot of code blocks that catch JSONException or Exception to deal with this finickiness.

Comment 19

[Chris Peterson [:cpeterson]]

Fair enough. That makes sense.

I recall that the Android Sync team was considering moving from the json-simple library to Jackson [1], which has a more memory-efficient streaming API.

[1] http://jackson.codehaus.org/

Comment 20

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

Attachment: Part 5 - Allow specifying names for inputs

Here's a simpler part 5 - it allows you to have multiple inputs of the same type and still get back meaningful result data.

Comment 21

[Wesley Johnston (:wesj)]

Comment on attachment 702376 [details] [diff] [review]
Part 5 - Allow specifying names for inputs

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

Weird. I swear I wrote this patch before...

I think maybe we should call this mID instead of Name (and probably rename the getName() function). If you're attached to 'name' I don't have a big problem with it though.

Comment 22

[Wesley Johnston (:wesj)]

Comment on attachment 701903 [details] [diff] [review]
Part 1 - Make buttons strings in the JSON instead of objects with one property (v2)

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

Great! Thanks

Comment 23

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

Try run [1] was green enough for my liking so I aborted the rest of the pending jobs and landed. I replaced the "name" stuff with "id" which I like better as well.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0641e11598d6
https://hg.mozilla.org/integration/mozilla-inbound/rev/43cda5f233a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e7e867030d9
https://hg.mozilla.org/integration/mozilla-inbound/rev/1567487dd2c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/419db9f0b0cf

[1] https://tbpl.mozilla.org/?tree=Try&rev=aef677a74adf

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0641e11598d6
https://hg.mozilla.org/mozilla-central/rev/43cda5f233a7
https://hg.mozilla.org/mozilla-central/rev/6e7e867030d9
https://hg.mozilla.org/mozilla-central/rev/1567487dd2c2
https://hg.mozilla.org/mozilla-central/rev/419db9f0b0cf