870062 - Provide asynchronous option in PromptService

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

This seems like a good way to migrate existing synchronous PromptService usage to asynchronous usage.

I think PromptService can take an 'async' flag in its options, and we need to provide a way for the result to find its way back to the JS caller.

Comment 1

[Wesley Johnston (:wesj)]

I think we can kill all asynchronous usage entirely to be honest. We recently fixed a bug so that we use domWindowUtils.enterModalStateWithWindow() for real prompts that (I think) should make them work in an async way:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/prompts/src/nsPrompter.js#387

I think that would remove all synchronous use cases, at which point we can kill this flag. In fact, maybe we'd be better to make the flag "sync" and force callers onto the async code path by default...

Comment 2

[Wesley Johnston (:wesj)]

Whoops. Mistyped. I mean, we should kill all SYNCHRONOUS usage of the prompt service.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch v1

Was playing with showing a prompt in bug 872388, and decided to fix this stuff at the same time. This adds the ability to pass in a listener when you create a PromptService object that will be called when the dialog is closed. It also creates a listener that, for the main prompt service, sends a message back to java when a prompt is closed.

I'd like to move away from having one promptService to having people just create them when they need one (they're not that heavy...).

PromptService ps = new PromptService(context, new PromptCallback() {
  ...
});

ps.show("Title", null, new String[] {
 "one", "two", "three"
}, false);

It also allows passing an async parameter with a json message (I backed off of my original statement there for now), that bypasses the normal spin-event-loop stuff. Eventually I want to kill that, but this works for now.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

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

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

::: mobile/android/base/PromptService.java
@@ +68,5 @@
> +    private static int mIconTextPadding;
> +    private static int mIconSize;
> +    private static int mInputPaddingSize;
> +    private static int mMinRowSize;
> +    private final Context mContext;

Is it possible for this context to get stale?

@@ +317,5 @@
>          ThreadUtils.assertOnUiThread();
>  
>          // treat actions that show a dialog as if preventDefault by content to prevent panning
> +        if (mContext instanceof GeckoApp) {
> +            ((GeckoApp)mContext).getLayerView().abortPanning();

I'd rather you pass the LayerView (or a getter) in than cast to a GeckoApp

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2

This is similar. Applies on top of the original patch. Moves me closer to having one Prompt per dialog being shown (I still worry about the state being overwritten somehow). I moved the current PromptService.java to a Prompt.java class, but removed the bits dealing with Prompt:Show messages. The new PromptService.java handles those messages (and is a much smaller object to keep around).

When you create a Prompt you can pass in a queue and a callback. When the prompt closes it (null checks) and notifies both. The caller can handle spinning a queue if it wants to (which is what the PromptService.java now does if the incoming message doesn't have an async parameter).

I also modified gecko GeckoEventResponder to take in the original message as a parameter. In this case right now I just use that to determine whether or not to spin the queue for sync/asnc prompt messages.

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 752433 [details] [diff] [review]
Patch 2/2

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

::: mobile/android/base/Prompt.java
@@ +77,3 @@
>  
> +
> +    public Prompt(Context context, ConcurrentLinkedQueue<String> queue, PromptCallback callback) {

is it ever valid to pass in a queue and a callback? If not, how about two different constructor signatures?

@@ +84,2 @@
>  
> +        if (!mInitialized) {

how would this be initialized prior to the constructor being called?

@@ +456,1 @@
>              mPromptQueue.offer(aReturn.toString());

This file seems to have a mix of brackets and not-brackets around single line ifs (see if (mCallback != null) { below). Mind cleaning it up in a follow up?

::: mobile/android/base/PromptSerivce.java
@@ +53,5 @@
> +            public void run() {
> +                boolean isAsync = message.optBoolean("async");
> +                Prompt p = new Prompt(GeckoApp.mAppContext,
> +                                      isAsync  ? null : mPromptQueue,
> +                                      !isAsync ? null : new PromptService.PromptCallback() {

this would look somewhat better with the two constructor signatures.

Comment 7

[Wesley Johnston (:wesj)]

Try looked good.
https://tbpl.mozilla.org/?tree=Try&pusher=[e-mail]

Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d484884d5699

Comment 8

[Wesley Johnston (:wesj)]

Whoops. Try:
https://tbpl.mozilla.org/?tree=Try&rev=e40d11c5bcdc

Comment 9

[Phil Ringnalda (:philor)]

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