1439968 - Remove string-based enum usage

Status: RESOLVED FIXED

(GeckoView :: General, enhancement)

Description

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

The permissions delegate has some of these, where the app is required to do something like:

            if ("geolocation".equals(type)) {
                resId = R.string.request_geolocation;
            } else if ("desktop-notification".equals(type)) {
                resId = R.string.request_notification;
            } else {
                Log.w(LOGTAG, "Unknown permission: " + type);
                callback.reject();
                return;
            }

The "type" here should be an integer and compared against a set of static values as is the convention.

Comment 1

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

Attachment: Kill off PermissionDelegate string-based enums

This removes the remaining public-facing string APIs where it makes sense to do so, and fixes some documentation fallout from bug 1432233.

Comment 2

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

Comment on attachment 8954160 [details] [diff] [review]
Kill off PermissionDelegate string-based enums

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

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
@@ +258,5 @@
> +                        type = PermissionDelegate.PERMISSION_GEOLOCATION;
> +                    } else if ("desktop_notification".equals(typeString)) {
> +                        type = PermissionDelegate.PERMISSION_DESKTOP_NOTIFICATION;
> +                    } else {
> +                        type = PermissionDelegate.PERMISSION_UNKNOWN;

Throw an Exception. PERMISSION_UNKNOWN doesn't make sense.

@@ +1970,5 @@
> +
> +        /**
> +         * Permission for media access.
> +         */
> +        public static final int PERMISSION_MEDIA = 2;

This is only used internally. Don't expose it publically.

@@ +1975,5 @@
> +
> +        /**
> +         * Android app permissions.
> +         */
> +        public static final int PERMISSION_ANDROID = 3;

This is only used internally. Don't expose it publically.

@@ +1980,5 @@
> +
> +        /**
> +         * Unknown permission request.
> +         */
> +        public static final int PERMISSION_UNKNOWN = 4;

This doesn't make sense.

@@ +2023,5 @@
> +         *             PERMISSION_GEOLOCATION
> +         *             PERMISSION_DESKTOP_NOTIFICATION
> +         *             PERMISSION_MEDIA
> +         *             PERMISSION_ANDROID
> +         *             PERMISSION_UNKNOWN

Only PERMISSION_GEOLOCATION and PERMISSION_DESKTOP_NOTIFICATION are valid for requestContentPermission

::: mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java
@@ +274,5 @@
>          public void requestContentPermission(final GeckoSession session, final String uri,
>                                               final String type, final String access,
>                                               final Callback callback) {
>              final int resId;
> +            if (PermissionDelegate.PERMISSION_GEOLOCATION.equals(type)) {

Use == instead of equals

@@ +279,2 @@
>                  resId = R.string.request_geolocation;
> +            } else if (PermissionDelegate.PERMISSION_DESKTOP_NOTIFICATION.equals(type)) {

Use == instead of equals

Comment 3

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

Attachment: Kill off PermissionDelegate string-based enums

Now with fixes for stuff Jim brought up above.

Comment 4

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

Comment on attachment 8954216 [details] [diff] [review]
Kill off PermissionDelegate string-based enums

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

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
@@ +258,5 @@
> +                        type = PermissionDelegate.PERMISSION_GEOLOCATION;
> +                    } else if ("desktop_notification".equals(typeString)) {
> +                        type = PermissionDelegate.PERMISSION_DESKTOP_NOTIFICATION;
> +                    } else {
> +                        throw new IllegalArgumentException();

You should add an exception message.

@@ +1963,5 @@
> +         */
> +        public static final int PERMISSION_GEOLOCATION = 0;
> +
> +        /**
> +         * Permission for using the notifications API.

I think we should include links to the relevant specs here if we can. The names are alone are sometimes confusing.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d0e13ca41ea
Remove public String-based enums from PermissionDelegate and update documentation. r=snorp,jchen

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6d0e13ca41ea