Closed Bug 870062 Opened 8 years ago Closed 8 years ago

Provide asynchronous option in PromptService

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 24

People

(Reporter: jchen, Assigned: wesj)

References

Details

Attachments

(2 files)

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.
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...
Whoops. Mistyped. I mean, we should kill all SYNCHRONOUS usage of the prompt service.
Attached patch Patch v1Splinter Review
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.
Attachment #751905 - Flags: review?(blassey.bugs)
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
Attachment #751905 - Flags: review?(blassey.bugs) → review+
Attached patch Patch 2/2Splinter Review
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.
Attachment #752433 - Flags: review?(blassey.bugs)
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.
Attachment #752433 - Flags: review?(blassey.bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/d484884d5699
Assignee: nobody → wjohnston
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Blocks: 877200
Blocks: 877911
No longer blocks: 877911
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.