1432233 - Get rid of GeckoBundle in public API

Status: RESOLVED FIXED

(GeckoView :: General, enhancement)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

Unless there is somewhere we absolutely must use it. String-based API bad.

Comment 1

[Dylan Roeh (:droeh) (he/him)]

Attachment: Kill public GeckoBundles

Here's a first take at it. We probably need a better name for the class I called "Choice", but I'm drawing a blank on what would make for a good name there.

Comment 2

[Dylan Roeh (:droeh) (he/him)]

Comment on attachment 8951025 [details] [diff] [review]
Kill public GeckoBundles

Forgot to flag for review.

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8951025 [details] [diff] [review]
Kill public GeckoBundles

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

Pretty good, but I want to eliminate "string-based enums" as well.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoSession.java
@@ +1968,5 @@
> +             */
> +            public String name;
> +
> +            /**
> +             * A string giving the media source type. 

Whitespace

@@ +1974,5 @@
> +             * "camera", "screen", "application", "window", "browser", and "other".
> +             * Possible values for an audio source are:
> +             * "microphone", "audioCapture", and "other".
> +             */
> +            public String source;

I think this should be an integer and we should have constants for the possible values.

@@ +1979,5 @@
> +
> +            /**
> +             * A string giving the type of media, must be either "video" or "audio".
> +             */
> +            public String type;

Same here

Comment 4

[Dylan Roeh (:droeh) (he/him)]

Attachment: Kill public GeckoBundles

Noted. This updates MediaSource to use ints for source and type (and fixes the extra whitespace).

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8951069 [details] [diff] [review]
Kill public GeckoBundles

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

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoSession.java
@@ +1607,5 @@
>           */
>          void promptForAuth(GeckoSession session, String title, String msg,
> +                           AuthenticationOptions options, AuthCallback callback);
> +
> +        class Choice {

Wow, this part of the prompt API is completely ridiculously. Can you file a followup to kill it? I want to move whatever is using this to special-purpose prompt APIs (similar to promptForAuth).

@@ +2032,5 @@
> +             */
> +            public int type;
> +
> +            private static int getSourceFromString(String src) {
> +                if ("camera".equals(src)) {

Can you leave a comment indicating where these strings come from? Presumably we'll have to keep them in sync.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cc483ee3ffb
Eliminate GeckoBundles from public APIs in GeckoSession. r=snorp

Comment 7

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/8cc483ee3ffb

Comment 8

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

Comment on attachment 8951069 [details] [diff] [review]
Kill public GeckoBundles

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

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoSession.java
@@ +1627,5 @@
> +            /**
> +             * A boolean indicating if the item is disabled. Item should not be
> +             * selectable if this is true.
> +             */
> +            public boolean disabled;

These fields should all be final. Doesn't really make sense for the consumer to change them.

Comment 9

[Dylan Roeh (:droeh) (he/him)]

(In reply to Jim Chen [:jchen] [:darchons] from comment #8)
> Comment on attachment 8951069 [details] [diff] [review]
> Kill public GeckoBundles
> 
> Review of attachment 8951069 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoSession.java
> @@ +1627,5 @@
> > +            /**
> > +             * A boolean indicating if the item is disabled. Item should not be
> > +             * selectable if this is true.
> > +             */
> > +            public boolean disabled;
> 
> These fields should all be final. Doesn't really make sense for the consumer
> to change them.

We do actually rely on changing some of these (label and selected) in BasicGeckoViewPrompt.java; is it safe to assume the rest should be final?

Comment 10

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

I think so. Even for "label" and "selected", they should be final, and BasicGeckoViewPrompt should store the active states elsewhere.

Comment 11

[Dylan Roeh (:droeh) (he/him)]

Alright, I'll follow up on that in bug 1438994.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a80d1c180aac
[focus] Updating GeckoViewPrompt after 1432233