875119 - Create a Prompts.jsm to wrap prompt messages

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch v1

This requires the async prompt service code i'm writing in bug 870062 to work. Asking for review, but happy to wait a bit for some feedback first too (will post this same message to the newsgroup). The API is pretty simple. I think most of it is fairly self explanatory. Quick overview:

Prompt(window)
    setTitle: function(aTitle)
    setMessage: function(aMessage)

    // adds buttons at the bottom of the dialog.
    // throws if you try to add more than three buttons
    // XXX Is this confusing with the other inputs below?
    addButton: function(aLabel)

    // order of calling these dictates the order they're shown
    addCheckbox: function(aLabel, aChecked)
    addTextbox: function(aValue, aHint, aAutofocus)
    addPassword: function(aValue, aHint, aAutofocus)
    addMenulist: function(aValues)
    // XXX: Add date-time inputs...

    show: function(callback)

    // aItems can be an array of strings or objects
    // these should (but don't currently) throw if you've set a message
    // or other inputs as we currently don't support showing both
    // simultaneously
    setSingleChoiceItems: function(aItems) 
    setMultiChoiceItems: function(aItems)

Examples:

var p = new Prompt(window) // in case we ever have tab modal prompts
            .setTitle("Title")
            .setMessage("this is a big long message")
            .addCheckbox("My checkbox", true)
            .addButton("B1")
            .show(function(data) {
  // the return value isn't well formalized here...
  var button = data.button
});

or

var p = new Prompt(window);
p.setTitle("My Title");
p.setSingleChoiceItems(["One", "Two", "Three"]);
p.show(function(data) {
  var indexClicked = data.button;
});

or

var p = new Prompt(window);
p.setTitle("My Title");
p.setMultichoiceItems([
  { label: "One", id: 1, disabled: true },
  // Shows as a group header with children underneath
  { label: "Two", id: 2, children: [
    { label: "Three", id: 3 }
  ]},
  // We also support sub-menus, but I'm not sure how to expose them.
  // This is stubbed right now so that we can at least show the submenu arrow
  { label: "Four", id 4, submenu: true },
]).show(function(data) {
  // button contains an array of booleans indicating whether
  // a row is now selected or unselected...
  var selected = data.button;
});

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Patch v1.1

Added an id parameter to inputs so that you can identify them when they're returned. i.e.

prompt.addCheckbox("Checkbox", true, "myCheckbox")
      .show(function(result) {
  var checked = result.myCheckbox;
});

if you don't set an id, the result will just be in a generic checkbox attribute


prompt.addCheckbox("Checkbox", true)
      .show(function(result) {
  var checked = result.checkbox;
});

Comment 3

[:Margaret Leibovic]

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

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

Nit: two-space indentation for JS.

Disclaimer: I haven't looked at bug 870062.

::: mobile/android/modules/Prompt.jsm
@@ +1,5 @@
> +// -*- Mode: Java; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

Let's make new files use strict mode.

@@ +10,5 @@
> +
> +this.EXPORTED_SYMBOLS = ["Prompt"];
> +
> +function log(msg) {
> +    if (true) {

This will be factored out to some debug variable in your final version, right? :)

@@ +32,5 @@
> +    get inputs() {
> +        if (!this.msg.inputs)
> +            this.msg.inputs = [];
> +        return this.msg.inputs;
> +    },

I know you like these getters, but I feel like they just add a layer of misdirection that makes it harder to understand what's going on, especially if they're returning properties on a different object. I think it would be clearer to just initialize all these things in this.msg in the Prompt constructor, like:

this.msg = {
  type: "Prompt:Show",
  async: true,
  inputs: [],
  buttons: [],
  ...
}

Then just call this.msg.whatever.push() directly. Or do you think there's too much overhead associated with potentially passing empty arrays in a message?

@@ +65,5 @@
> +        return this;
> +    },
> +
> +    addButton: function(aLabel) {
> +        if (this.buttons.length == 3)

Did you mean for this check to be > 3?

@@ +73,5 @@
> +        this.buttons.push(aLabel);
> +        return this;
> +    },
> +
> +    _addInput: function(aType, aRequired, aOptional) {

Can you add comments about what these parameters mean? I'm confused by why you have separate required and optional params objects, since you're not checking for any specific properties in aRequried. And given the way you call _addInput down below, I'm having a tough time figuring out what the difference is.

@@ +77,5 @@
> +    _addInput: function(aType, aRequired, aOptional) {
> +        log("Add input " + aType);
> +        let obj = { type: aType };
> +        for (let param of aRequired) {
> +            obj[param] = aRequired[param];

Does this work? I think for ... of only works for iterable objects, and it doesn't look like that's what you call this with.

@@ +80,5 @@
> +        for (let param of aRequired) {
> +            obj[param] = aRequired[param];
> +        }
> +        for (let param of aOptional) {
> +            if (aOptional[param] != undefined)

If you're iterating through the properties of aOptional, would param never be undefined?

@@ +128,5 @@
> +        let data = JSON.parse(aData);
> +        if (data.guid != this.guid)
> +            return;
> +
> +        Services.obs.removeObserver(this, "Prompt:Reply", false);

Would a developer ever expect to be able to reuse a prompt? Because removing this observer means that wouldn't work :) Perhaps we should add the observer in show().

@@ +135,5 @@
> +    },
> +
> +    _setListItems: function(aItems, aInGroup) {
> +        aItems.forEach(function(item) {
> +            let obj = { id: item.id };

Why do you need to make another object like this? Why can't the callers of _setListItems just pass in an aItems parameter that's formatted they way you'd want?

@@ +140,5 @@
> +            if (aInGroup !== undefined)
> +                obj.inGroup = aInGroup;
> +
> +            if (item instanceof String) {
> +                obj.label = item;

I'm not really a fan of letting API consumers do things different ways if we don't need to. If they just want a label, they could just pass { "label": "whatever" } instead of "whatever". It makes the API feel more consistent and less magical.

Comment 4

[Chenxia Liu [:liuche]]

Comment on attachment 753014 [details] [diff] [review]
Patch v1.1

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

Just a quick drive-by to see what's going on; just small nits/suggestions.

::: mobile/android/modules/Prompt.jsm
@@ +1,1 @@
> +// -*- Mode: Java; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-

Shouldn't be Java mode.

@@ +20,5 @@
> +    log("Create prompt for window " + window);
> +    this.window = window;
> +    this.msg = { type: "Prompt:Show", async: true };
> +
> +    let idService = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator); 

nit: trailing space

@@ +64,5 @@
> +        this.msg.text = aMessage;
> +        return this;
> +    },
> +
> +    addButton: function(aLabel) {

Perhaps to be clearer that buttons are different from Prompt content, could rename this to addBottomButton? Optionally (or in addition) a comment here on the difference.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 753014 [details] [diff] [review]
Patch v1.1

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

> XPCOMUtils.defineLazyGetter(this, "Sanitizer", function() {
>   Cu.import("resource://gre/modules/Sanitizer.jsm");
>   return Sanitizer;
> });
> 
>+XPCOMUtils.defineLazyGetter(this, "Prompt", function() {
>+  let temp = {};
>+  Cu.import("resource://gre/modules/Prompt.jsm", temp);
>+  return temp.Prompt;

Do we need the "temp" scope? Sanitizer doesn't use it above

>+});
> // Lazily-loaded browser scripts:

Add a blank line

>diff --git a/mobile/android/modules/Prompt.jsm b/mobile/android/modules/Prompt.jsm

General:
* I like that you made a JS object/API to wrap the the sending/receiving of RPC JSON messages. Long live APIs! Death to RPC messages.
* The API has a bit too many conveniences and should be more plain. We can add a PromptBuilder to the JSM that can be used to build up the internal JSON needed by Prompt.
* We use too many method parameters. I'd rather see passing JSON literals:

  var p = new Prompt(window).setTitle("Title").setMessage("Hello World").addButton("OK").addButton("Cancel");

  var p = new Prompt(window, { title: "Title", message: "Hello World", buttons: ["OK", "Cancel"] });

Not because I think it takes up less space, but because I think it's more flexible. As I mentioned above, if we want to add a helper to "build up" JSON, we can do that separately. Let's not burden the raw API with too much sugar. I also like the fact that the values are "documented" so we always know what they are.

For the record, I don't mind having addCheckbox, for example, but I would like to see:

  p.addCheckbox({ label: "Never show me this again", checked: true, id: "foobar" });

>+function log(msg) {
>+    if (true) {
>+        Services.console.logStringMessage(msg);
>+    }
>+}

I wish we had a better way of removing this from production code. I assume commenting out the body is the "way".

>+function Prompt(window) {
>+    log("Create prompt for window " + window);

I am worrying too much about logging, but even commenting out the body of "log" won't make the string concatenation of the message go away.

>+    let idService = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator); 

I wonder if this could be global to Prompt.jsm so we don't create one every time we make a Prompt object.

>+    Services.obs.addObserver(this, "Prompt:Reply", false);

Can you do this in Prompt.show ? That way removing it in Prompt.observe won't break reusing the Prompt object.

>+    get inputs() {
>+        if (!this.msg.inputs)
>+            this.msg.inputs = [];
>+        return this.msg.inputs;
>+    },

I kinda agree with Margaret on these. I could be swayed by a convincing use case though.

>+    show: function(callback) {
>+        this.callback = callback;
>+        log("Sending message " + JSON.stringify(this.msg));

This log should be removed. JSON.stringify is not cheap

>+    _setListItems: function(aItems, aInGroup) {

Keep this method, but just take raw JSON

>+    setSingleChoiceItems: function(aItems) {
>+        return this._setListItems(aItems);
>+    },
>+
>+    setMultiChoiceItems: function(aItems) {
>+        this.msg.multiple = true;
>+        return this._setListItems(aItems);
>+    },

I think we'd move these methods to the PromptBuilder

>\ No newline at end of file

Add one

This is really just feedback, so switching to f+. I want to see this concept land.

Comment 6

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

Comment on attachment 753014 [details] [diff] [review]
Patch v1.1

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

I like! Just the one comment below.

::: mobile/android/modules/Prompt.jsm
@@ +24,5 @@
> +    let idService = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator); 
> +    this.guid = idService.generateUUID().toString();
> +    this.msg.guid = this.guid;
> +
> +    Services.obs.addObserver(this, "Prompt:Reply", false);

I think it might be better to move this to the top of the show() function. There might be cases where somebody creates this Prompt object, and then while configuring it, gets an exception, and so never calls show(). In that case this would be leaked forever and ever.

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch v2

(In reply to :Margaret Leibovic from comment #3)
> This will be factored out to some debug variable in your final version,
> right? :)
Yeah. I've seen quite a few files in the b2g source that just comment/uncomment these things out. Based on the reactions, I'd say our team doesn't like it :) Its still in here (but without the JSON.stringify bits...). I can remove for landing.

> I know you like these getters, but I feel like they just add a layer of

I do like getters, but moreso I'm a bit worried that that Java prompt service will take the presence of an attribute as a sign to do something (I think we do use the presences of a selected attribute to indicate if we should show checkmarks on rows or not)..

They were mostly a convenience for instantiating the arrays, but that could have the same bad effects, so I just removed them.

> Did you mean for this check to be > 3?

No... >= 3 would probably be better though. I just removed the whole thing since I wasn't being all that consistent with my throws.

> Why do you need to make another object like this? Why can't the callers of
> _setListItems just pass in an aItems parameter that's formatted they way
> you'd want?

So I kinda hate the JSON syntax for list items. Its written to match well with what Java needed, but its not very intuitive (IMO). Here I'd like to make something non-awful. This gives us:

[
  { label: "Option 1", disabled: true },
  { label: "Option 2", children: [ // shows a group header row
    // having a selected attribute item makes every item in the list
    // show a checkmark
    { label: "Option 3", selected: false },
  ] },
  { label: "Option 4", submenu: [ // shows a dropmarker indicating a submenu
    { label: "Option 5" },
  ] },
]

I removed the string parameter support. I still like it, but its likely not useful for much but pretty tutorial code.

> Do we need the "temp" scope? Sanitizer doesn't use it above

Sanitizer is a singleton. I tried this without the temp and had no luck, but can look harder.

> * The API has a bit too many conveniences and should be more plain. We can
> add a PromptBuilder to the JSM that can be used to build up the internal
> JSON needed by Prompt.
All the methods now take objects as parameters.

> >+    setSingleChoiceItems: function(aItems) {
> >+    setMultiChoiceItems: function(aItems) {

I left these in. It seems like with the alternative we wind up with;

var p
if (select.multiple)
  p = new Prompt({multiple: true, button: "Done"});
else
  p = new Prompt({...});
p.setListItems(items);

as opposed to:

var p = new Prompt({...})
if (select.multiple) {
  p.setMultiChoiceItems(items)
   .addButton({ label: "Done" });
} else {
  p.setSingleChoiceItems(items)
}

Not sure which is better really, but I kinda hate having the multiple part far from where the list is added.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

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

>diff --git a/mobile/android/modules/Prompt.jsm b/mobile/android/modules/Prompt.jsm

>+function Prompt(options) {

You use aOptions in other places

>+  log("Create prompt for window ");

I don't mind keeping (and commenting out) the log() function, but this type is debugging log call could be removed.

>+  _addInput: function(aOptions) {

>+    if (this[aOptions.type + "Count"] === undefined)
>+      this[aOptions.type + "Count"] = 0;

Is "Count" only used for making unique IDs? Could we use "_count" since the type is always lowercase?

>+  setSingleChoiceItems: function(aItems) {
>+    return this._setListItems(aItems);
>+  },
>+
>+  setMultiChoiceItems: function(aItems) {
>+    this.msg.multiple = true;
>+    return this._setListItems(aItems);
>+  },

Still seems sugary, but I won't fight it

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Patch v2.1

Since blassey is chomping at the bit to fix these ANR's, seems good to move to review. This fixes the bits from the feedback review.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 755033 [details] [diff] [review]
Patch v2.1

>diff --git a/mobile/android/modules/Prompt.jsm b/mobile/android/modules/Prompt.jsm

>+function Prompt(options) {

You use aOptions in the rest of the code. Switch?

>+  _addInput: function(aOptions) {
>+    log("Add input");

Remove

Comment 11

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7feae3bc43fa

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7feae3bc43fa

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Wes - Can you open a new bug to fix bug 872137 using the Prompt.jsm code?

Comment 14

[Wesley Johnston (:wesj)]

Put together the start of some docs here:

https://developer.mozilla.org/en-US/docs/Extensions/Mobile/API/Prompt.jsm

Realized as I was writing examples some changes I made to our API are kinda awful. Filing some follow ups.

Comment 15

[Richard Z.]

Thanks for this API, saved my day. There is one "side effect" which I am not quite sure if it is a bug or expected:

Displaying a prompt in fullscreen mode ( after setting window.fullScreen ) it appears the full screen mode is partially exited. The battery/notification line on top is displayed again but window.fullScreen remains unchanged and browser seems to think it is still in full screen mode.