928756 - [geckoview] Convert prompt-based actions into separate GeckoView endpoints

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: GRE Core, defect)

Description

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

Currently, we use Prompt.jsm in the Gecko/JS part of Firefox to display a prompt based on some feature or behavior. By the time this gets to Java, we only treat it as a prompt.

This means it's hard to figure out the context of the prompt. Is it a remote debugging request? Is it a permission request? Or even, what type of prompt request is it?

Since GeckoView wraps web content, we need to pass web content prompts (alert, confirm and prompt) to the embedding app so it can display the prompt however it wants.

We also need to split out the context for permission requests. Requests for things like remote debugging or HTML5 WebAPIs.

Comment 1

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

Attachment: geckoview-prompts WIP v1

Wes - Take a look at the "context" hint I am using in a few places in this patch. Also look at how I use it to filter generic Prompt:Show messages.

Lastly, see the PromptResult Java class. It's very much like WebView's way of doing async callbacks from prompts.

Examples of using this can be found here:
https://github.com/mfinkle/geckobrowser/blob/master/src/com/starkravingfinkle/geckobrowser/MainActivity.java

Look for MyGeckoViewChrome

Comment 2

[Wesley Johnston (:wesj)]

Comment on attachment 819496 [details] [diff] [review]
geckoview-prompts WIP v1

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

Not my favorite thing in the world. I worry we're going to end up with a whole lot of very specialized messages. "Cert:Add", "Addon:InstallPrompt". etc? Maybe thats what we really need in the end...

::: mobile/android/base/GeckoView.java
@@ +84,5 @@
>          GeckoAppShell.registerEventListener("Content:PageShow", this);
>          GeckoAppShell.registerEventListener("DOMTitleChanged", this);
>          GeckoAppShell.registerEventListener("Link:Favicon", this);
> +        GeckoAppShell.registerEventListener("Prompt:Show", this);
> +        GeckoAppShell.registerEventListener("Prompt:ShowTop", this);

Can we keep this in the PromptService class (to avoid GeckoView becoming a dumping ground for messages)? ShowTop should die, but that's a different bug.

@@ +340,5 @@
>              }
>          }
>      }
> +
> +    public class PromptResult {

We have a similar class in PromptService.java. Can we extend it for this?

@@ +350,5 @@
> +        public void confirm() {
> +            JSONObject result = new JSONObject();
> +            try {
> +                result.put("guid", mGUID);
> +                result.put("button", 0);

We should move the duplicated logic (for guid and button) into its own method. Also, use a final int OK = 0;

@@ +369,5 @@
> +        public void cancel() {
> +            JSONObject result = new JSONObject();
> +            try {
> +                result.put("guid", mGUID);
> +                result.put("button", 1);

private static final int CANCEL = 1;

::: mobile/android/base/GeckoViewChrome.java
@@ +31,5 @@
> +    * @param message The string to display in the dialog.
> +    * @param result A PromptResult used to send back the result without blocking.
> +    * Defaults to cancel requests.
> +    */
> +    public void onAlert(GeckoView view, GeckoView.Browser browser, String message, GeckoView.PromptResult result) {

I would rather these return a boolean to indicate if they're handling the prompt or not. I have to think most embeddors would still want our default implementation. I'm a bit worried that people who are overriding this are overriding it because they're using it for a hacky way to communicate from content js->java.

::: mobile/android/chrome/content/browser.js
@@ +7035,5 @@
>  
>      // Make prompt. Note: button order is in reverse.
>      let prompt = new Prompt({
>        window: null,
> +      context: "remotedebug",

I don't love the word "context". I wonder if something like "details" or "description" would be better?

::: mobile/android/components/PromptService.js
@@ +217,5 @@
>    /* ----------  nsIPrompt  ---------- */
>  
>    alert: function alert(aTitle, aText) {
>      let p = this._getPrompt(aTitle, aText, [ PromptUtils.getLocaleString("OK") ]);
> +    p.setContext("alert");

Hmm... This won't necessarily filter out things that don't come from content. Maybe that's fine, (but should we also set the context for these other prompts?) Hacky fix, looks like the platform tries to cast the prompt to a writablePropertyBag and set a showAsModal value in it if its from the window.alert/confirm/prompt interface [1]. We could implement that and use it to determine whether or not to send this to the embedder?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#5482

::: mobile/android/modules/Prompt.jsm
@@ +10,5 @@
>  
>  this.EXPORTED_SYMBOLS = ["Prompt"];
>  
>  function log(msg) {
> +  Services.console.logStringMessage(msg);

comment this back out.

Comment 3

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

(In reply to Wesley Johnston (:wesj) from comment #2)
> Comment on attachment 819496 [details] [diff] [review]
> geckoview-prompts WIP v1
> 
> Review of attachment 819496 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Not my favorite thing in the world. I worry we're going to end up with a
> whole lot of very specialized messages. "Cert:Add", "Addon:InstallPrompt".
> etc? Maybe thats what we really need in the end...

Yes. I think that's what we actually need to do. It's not a hack, IMO. It's the right thing to do.

> ::: mobile/android/base/GeckoView.java
> >          GeckoAppShell.registerEventListener("Content:PageShow", this);
> >          GeckoAppShell.registerEventListener("DOMTitleChanged", this);
> >          GeckoAppShell.registerEventListener("Link:Favicon", this);
> > +        GeckoAppShell.registerEventListener("Prompt:Show", this);
> > +        GeckoAppShell.registerEventListener("Prompt:ShowTop", this);
> 
> Can we keep this in the PromptService class (to avoid GeckoView becoming a
> dumping ground for messages)? ShowTop should die, but that's a different bug.

I think PromptService.java is not decoupled enough from Fennec. I think that we want GeckoView to handle these, and pass them off to Prompt.java if we want a default impl.

> > +
> > +    public class PromptResult {
> 
> We have a similar class in PromptService.java. Can we extend it for this?

I don't really want to do that, for the above reasons. I don't want embedders to know about PromptService.

> > +        public void confirm() {
> > +            JSONObject result = new JSONObject();
> > +            try {
> > +                result.put("guid", mGUID);
> > +                result.put("button", 0);
> 
> We should move the duplicated logic (for guid and button) into its own
> method. Also, use a final int OK = 0;

We could do that

> ::: mobile/android/base/GeckoViewChrome.java

> > +    * Defaults to cancel requests.
> > +    */
> > +    public void onAlert(GeckoView view, GeckoView.Browser browser, String message, GeckoView.PromptResult result) {
> 
> I would rather these return a boolean to indicate if they're handling the
> prompt or not. I have to think most embeddors would still want our default
> implementation. I'm a bit worried that people who are overriding this are
> overriding it because they're using it for a hacky way to communicate from
> content js->java.

I'm not so sure about the boolean or our default impl. WebView returns a boolean only to let WebView know if it needs to worry about a prompt being shown or not. Returning false doesn't make WebView show a built-in prompt, AIUI.

As for custom message passing, we need to impl a first class message passing system anyway. Embedders want to have support for adding their own code to the web DOM. WebView has a solid Java way of doing it, but we want have that. We'll need JS components/modules and messages.

> ::: mobile/android/chrome/content/browser.js

> >      // Make prompt. Note: button order is in reverse.
> >      let prompt = new Prompt({
> >        window: null,
> > +      context: "remotedebug",
> 
> I don't love the word "context". I wonder if something like "details" or
> "description" would be better?

I could live with "hint"

> ::: mobile/android/components/PromptService.js

> >    alert: function alert(aTitle, aText) {
> >      let p = this._getPrompt(aTitle, aText, [ PromptUtils.getLocaleString("OK") ]);
> > +    p.setContext("alert");
> 
> Hmm... This won't necessarily filter out things that don't come from
> content. Maybe that's fine, (but should we also set the context for these
> other prompts?) Hacky fix, looks like the platform tries to cast the prompt
> to a writablePropertyBag and set a showAsModal value in it if its from the
> window.alert/confirm/prompt interface [1]. We could implement that and use
> it to determine whether or not to send this to the embedder?

Might be worth doing. Anything to help with the filter.

> ::: mobile/android/modules/Prompt.jsm

> >  this.EXPORTED_SYMBOLS = ["Prompt"];
> >  
> >  function log(msg) {
> > +  Services.console.logStringMessage(msg);
> 
> comment this back out.

Right!

Comment 4

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

Attachment: geckoview-prompts v1

This is the basic patch, tweaked with some of the feedback comments. We expect to get more feedback from Android developers and we'll adjust the API more when we do.

This is experimental, but ready for external developers.

(Wes is on PTO, so passing to Brian)

Comment 5

[Brian Nicholson (:bnicholson)]

Comment on attachment 821620 [details] [diff] [review]
geckoview-prompts v1

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

::: mobile/android/base/GeckoView.java
@@ +253,5 @@
> +        if (mChrome != null) {
> +            String hint = message.optString("hint");
> +            if ("alert".equals(hint)) {
> +                String text = message.optString("text");
> +                mChrome.onAlert(GeckoView.this, null, text, new PromptResult(message.optString("guid")));

The loose contract here between these callbacks and the PromptResults is awkward, and I think the generic nature of PromptResult makes it difficult to understand how to use it. From the client's perspective, how do I know whether to use confirm() or confirmWithValue()? Do I need to confirm at all? What happens if I don't confirm() or cancel() when I'm supposed to? What happens if I use confirm() when confirmWithValue() is expected, or vice versa? If it doesn't make sense to call certain result methods in different situations, we shouldn't give clients the ability to do so.

I think you may have even confused this logic yourself here: https://github.com/mfinkle/geckobrowser/blob/master/src/com/starkravingfinkle/geckobrowser/MainActivity.java#L102. As far as I can tell, confirming (or canceling) an alert does nothing.

I think we can simplify this API a bit and drop the PromptResult class -- or at least make it internal to GeckoView. Each of these callbacks (onAlert, onConfirm, onPrompt, onDebugRequest) can instead use their return values to indicate the desired action. onAlert would simply be void, onConfirm and onDebugRequest would return a boolean, and onPrompt would return a string (where null == cancel). This enforces the prompt contract, requires clients to give a valid response, and reduces the opportunity for confusion and mistakes.

::: mobile/android/chrome/content/browser.js
@@ +7111,5 @@
>  
>      // Make prompt. Note: button order is in reverse.
>      let prompt = new Prompt({
>        window: null,
> +      context: "remotedebug",

s/context/hint/

Comment 6

[Brian Nicholson (:bnicholson)]

(In reply to Mark Finkle (:mfinkle) from comment #3)
> I'm not so sure about the boolean or our default impl. WebView returns a
> boolean only to let WebView know if it needs to worry about a prompt being
> shown or not. Returning false doesn't make WebView show a built-in prompt,
> AIUI.

Just saw this -- why do we care about what WebView does if we aren't API compatible with them?

But if you don't like using return values here, we could alternatively pass different kinds of PromptResult objects to clients to solve the issues above. onAlert() gets nothing, onConfirm()/onDebugRequest() get a PromptResult, and onPrompt() gets a InputPromptResult. PromptResult would mirror WebView's JsResult (http://developer.android.com/reference/android/webkit/JsResult.html), and InputPromptResult would mirror WebView's JsPromptResult (http://developer.android.com/reference/android/webkit/JsPromptResult.html).

Also, if we go with this approach, we still need to guard against confirm()/cancel() not being called by the client. We might want to have PromptResult just be a dumb object that holds a tri-state (undefined/true/false) set by confirm()/cancel(). GeckoView can then read this state and send the response itself after the callback has finished, and it can then throw or do some other behavior if the client didn't respond at all.

(In reply to Brian Nicholson (:bnicholson) from comment #5)
> I think you may have even confused this logic yourself here:
> https://github.com/mfinkle/geckobrowser/blob/master/src/com/
> starkravingfinkle/geckobrowser/MainActivity.java#L102. As far as I can tell,
> confirming (or canceling) an alert does nothing.

After typing the last paragraph above, I realized you probably still need to confirm (or cancel) the prompt to end the modal state. If that's the case, how about we confirm the prompt internally in GeckoView, right after you call mChrome.onAlert()? Then the client doesn't need to confirm/cancel the alert (which it shouldn't, since alerts don't care about the result).

Comment 7

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

(In reply to Brian Nicholson (:bnicholson) from comment #5)

> ::: mobile/android/base/GeckoView.java
> @@ +253,5 @@
> > +        if (mChrome != null) {
> > +            String hint = message.optString("hint");
> > +            if ("alert".equals(hint)) {
> > +                String text = message.optString("text");
> > +                mChrome.onAlert(GeckoView.this, null, text, new PromptResult(message.optString("guid")));
> 
> The loose contract here between these callbacks and the PromptResults is
> awkward, and I think the generic nature of PromptResult makes it difficult
> to understand how to use it. From the client's perspective, how do I know
> whether to use confirm() or confirmWithValue()? Do I need to confirm at all?
> What happens if I don't confirm() or cancel() when I'm supposed to? What
> happens if I use confirm() when confirmWithValue() is expected, or vice
> versa? If it doesn't make sense to call certain result methods in different
> situations, we shouldn't give clients the ability to do so.

I can add this to the javadocs, but all prompts must be confirmed or canceled, just like WebView. WebView uses two PromptResult classes: one for alert and confirm, and one for prompt. I combined these together.

Using confirmWithValue will work fine for alert and confirm, but the value will not be used.

> I think you may have even confused this logic yourself here:
> https://github.com/mfinkle/geckobrowser/blob/master/src/com/
> starkravingfinkle/geckobrowser/MainActivity.java#L102. As far as I can tell,
> confirming (or canceling) an alert does nothing.

You must confirm or cancel, even an alert, or the async nature is screwed up. This is even the case with WebView, where you must confirm() an alert if you override onJsAlert (their method name).

> I think we can simplify this API a bit and drop the PromptResult class -- or
> at least make it internal to GeckoView. Each of these callbacks (onAlert,
> onConfirm, onPrompt, onDebugRequest) can instead use their return values to
> indicate the desired action. onAlert would simply be void, onConfirm and
> onDebugRequest would return a boolean, and onPrompt would return a string
> (where null == cancel). This enforces the prompt contract, requires clients
> to give a valid response, and reduces the opportunity for confusion and
> mistakes.

You can't depend on the return value of onAlert, onConfirm or onPrompt. The action is async. WebView uses the return value to let the JS context know that the embedding app will handle the prompt, otherwise continue executing. We don't support that nature so I dropped the return values.


> ::: mobile/android/chrome/content/browser.js
> @@ +7111,5 @@
> >  
> >      // Make prompt. Note: button order is in reverse.
> >      let prompt = new Prompt({
> >        window: null,
> > +      context: "remotedebug",
> 
> s/context/hint/

Yikes! Will fix.

Comment 8

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

(In reply to Brian Nicholson (:bnicholson) from comment #6)
> (In reply to Mark Finkle (:mfinkle) from comment #3)
> > I'm not so sure about the boolean or our default impl. WebView returns a
> > boolean only to let WebView know if it needs to worry about a prompt being
> > shown or not. Returning false doesn't make WebView show a built-in prompt,
> > AIUI.
> 
> Just saw this -- why do we care about what WebView does if we aren't API
> compatible with them?

Their API is nice for async, which we need to support.

> But if you don't like using return values here, we could alternatively pass
> different kinds of PromptResult objects to clients to solve the issues
> above. onAlert() gets nothing, onConfirm()/onDebugRequest() get a
> PromptResult, and onPrompt() gets a InputPromptResult. PromptResult would
> mirror WebView's JsResult
> (http://developer.android.com/reference/android/webkit/JsResult.html), and
> InputPromptResult would mirror WebView's JsPromptResult
> (http://developer.android.com/reference/android/webkit/JsPromptResult.html).

I kinda hate passing one-off classes for each type of action. Seems like overkill. To embedders I say "read the docs".

> Also, if we go with this approach, we still need to guard against
> confirm()/cancel() not being called by the client. We might want to have
> PromptResult just be a dumb object that holds a tri-state
> (undefined/true/false) set by confirm()/cancel(). GeckoView can then read
> this state and send the response itself after the callback has finished, and
> it can then throw or do some other behavior if the client didn't respond at
> all.

I don't think that works with our async model.

> (In reply to Brian Nicholson (:bnicholson) from comment #5)
> > I think you may have even confused this logic yourself here:
> > https://github.com/mfinkle/geckobrowser/blob/master/src/com/
> > starkravingfinkle/geckobrowser/MainActivity.java#L102. As far as I can tell,
> > confirming (or canceling) an alert does nothing.
> 
> After typing the last paragraph above, I realized you probably still need to
> confirm (or cancel) the prompt to end the modal state. If that's the case,
> how about we confirm the prompt internally in GeckoView, right after you
> call mChrome.onAlert()? Then the client doesn't need to confirm/cancel the
> alert (which it shouldn't, since alerts don't care about the result).

Making alert work one way while confirm and prompt work differently seems like a mistake. I'd rather follow EebView here and make them all work the same way regarding confirm/cancel.

Comment 9

[Brian Nicholson (:bnicholson)]

Comment on attachment 821620 [details] [diff] [review]
geckoview-prompts v1

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

As Mark pointed out, GeckoView is still a WIP, so no need to block on the comments above. Just some things to think about before the API is finalized.

Comment 10

[Wesley Johnston (:wesj)]

I still very much don't like that this API is basically duplicating a bunch of code we already have in the tree.

> Yes. I think that's what we actually need to do. It's not a hack, IMO. It's
> the right thing to do.

I still don't think I agree with this. The frontend doesn't really need to be aware of why Gecko is doing everything (our current one certainly isn't and works fine). This feels like a hack to hide the debugger prompts, which I think most apps would be fine with showing. i.e. my overall vote after looking at this for a bit is that we shouldn't do it at all.

> I think PromptService.java is not decoupled enough from Fennec. I think that
> we want GeckoView to handle these, and pass them off to Prompt.java if we
> want a default impl.

I'm not sure what you mean by this. PromptService.java is just a message listener right now. It passes the message off to the implementation. Its basically exactly what you've got here, except its not making GeckoView a dumping ground for messages.

> I don't really want to do that, for the above reasons. I don't want
> embedders to know about PromptService.

Same here. I don't care if you change or extend the class, but now we've got two classes in the tree that do almost exactly the same thing. I don't really want that. That seems to be happening in a lot of places with GeckoView right now. We're not making good separations between backend and frontend, and instead just duplicating code across both.

> I'm not so sure about the boolean or our default impl. WebView returns a
> boolean only to let WebView know if it needs to worry about a prompt being
> shown or not. Returning false doesn't make WebView show a built-in prompt,
> AIUI.

I don't really care what webview does, but I think for most MOST MOST embedders, the default implementation here will be what they want. We should make not doing anything do the right thing. This whole thing seems really edge casey to me. What's the use case for a GeckoView app calling window.alert and not showing an alert? It seems like something that only a very few people will care about?

> I could live with "hint"

Your current patch doesn't use this for the debugger prompts.

Comment 11

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

(In reply to Wesley Johnston (:wesj) from comment #10)

> I still don't think I agree with this. The frontend doesn't really need to
> be aware of why Gecko is doing everything (our current one certainly isn't
> and works fine). This feels like a hack to hide the debugger prompts, which
> I think most apps would be fine with showing. i.e. my overall vote after
> looking at this for a bit is that we shouldn't do it at all.

Let's say that an absolute requirement is to *not* show Fennec prompts. What is your solution in that case?

> > I think PromptService.java is not decoupled enough from Fennec. I think that
> > we want GeckoView to handle these, and pass them off to Prompt.java if we
> > want a default impl.
> 
> I'm not sure what you mean by this. PromptService.java is just a message
> listener right now. It passes the message off to the implementation. Its
> basically exactly what you've got here, except its not making GeckoView a
> dumping ground for messages.

It's also 100% Fennec specific. Again, we absolutely can not fallback to just show Fennec prompts. That will not be acceptable.

> > I don't really want to do that, for the above reasons. I don't want
> > embedders to know about PromptService.
> 
> Same here. I don't care if you change or extend the class, but now we've got
> two classes in the tree that do almost exactly the same thing. I don't
> really want that. That seems to be happening in a lot of places with
> GeckoView right now. We're not making good separations between backend and
> frontend, and instead just duplicating code across both.

We can't do a great separation in the initial phases. It will take a long time to get it right and I am not willing to wait a long time to get something useable released. What you describe is internal implementation issues. We can move the internal code to the "ideal" situation over time. There is no way we'll get it right the first time. I am trying to focus more on the external, public APIs. We should try to get them "right" sooner than the internal code.

> > I'm not so sure about the boolean or our default impl. WebView returns a
> > boolean only to let WebView know if it needs to worry about a prompt being
> > shown or not. Returning false doesn't make WebView show a built-in prompt,
> > AIUI.
> 
> I don't really care what webview does, but I think for most MOST MOST
> embedders, the default implementation here will be what they want. We should
> make not doing anything do the right thing. This whole thing seems really
> edge casey to me. What's the use case for a GeckoView app calling
> window.alert and not showing an alert? It seems like something that only a
> very few people will care about?

As I mention above, that is a non-starter. The sooner you stop thinking we can just use the Fennec prompts the better.

> > I could live with "hint"
> 
> Your current patch doesn't use this for the debugger prompts.

Yeah. Brian caught it too. Fixed locally.

Comment 12

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

https://hg.mozilla.org/integration/fx-team/rev/513900da2dce

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/513900da2dce