Closed Bug 830175 Opened 8 years ago Closed 8 years ago

PromptService code needs cleanup

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 21

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(5 files, 5 obsolete files)

7.84 KB, patch
wesj
: review+
Details | Diff | Splinter Review
7.97 KB, patch
kats
: review+
cpeterson
: feedback+
Details | Diff | Splinter Review
21.80 KB, patch
wesj
: review+
Details | Diff | Splinter Review
6.39 KB, patch
wesj
: review+
Details | Diff | Splinter Review
2.21 KB, patch
wesj
: review+
Details | Diff | Splinter Review
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
Attached patch Part 2 - Add some helpers (obsolete) — Splinter Review
Attachment #701633 - Flags: review?(cpeterson)
Attached patch Part 3 - Rename member variables (obsolete) — Splinter Review
Attachment #701634 - Flags: review?(cpeterson)
Attached patch Part 4 - Minor optimizations (obsolete) — Splinter Review
Attachment #701635 - Flags: review?(cpeterson)
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.
Attachment #701636 - Flags: review?(cpeterson)
Assignee: nobody → bugmail.mozilla
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 }
Attachment #701632 - Flags: review?(cpeterson) → review-
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.
Attachment #701633 - Flags: review+
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
Attachment #701634 - Flags: review+
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?
Attachment #701635 - Flags: review?(cpeterson) → review-
(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 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 = [])
Attachment #701636 - Flags: review?(cpeterson) → review-
(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.
Carrying r+
Attachment #701633 - Attachment is obsolete: true
Attachment #701633 - Flags: review?(cpeterson)
Attachment #701904 - Flags: review+
Attachment #701634 - Attachment is obsolete: true
Attachment #701634 - Flags: review?(cpeterson)
Attachment #701905 - Flags: review?(wjohnston)
Attachment #701635 - Attachment is obsolete: true
Attachment #701636 - Attachment is obsolete: true
Attachment #701906 - Flags: review?(wjohnston)
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.
Attachment #701904 - Flags: feedback+
(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.
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/
Here's a simpler part 5 - it allows you to have multiple inputs of the same type and still get back meaningful result data.
Attachment #702376 - Flags: review?(wjohnston)
Attachment #701905 - Flags: review?(wjohnston) → review+
Attachment #701906 - Flags: review?(wjohnston) → review+
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.
Attachment #702376 - Flags: review?(wjohnston) → review+
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
Attachment #701903 - Flags: review?(wjohnston) → review+
Duplicate of this bug: 732746
You need to log in before you can comment on or make changes to this bug.