1381536 - Replace GeckoBundle passed to ProgressListener.onSecurityChanged with a class

Status: RESOLVED FIXED

(GeckoView :: General, enhancement)

Description

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

Followup to bug 1376587; after talking to snorp about that patch we decided it would be better to create a class to expose security information instead of the GeckoBundle we currently expose.

Comment 1

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

Attachment: Add a class to represent security info rather than using a GeckoBundle

This adds a class, SecurityInformation, and passes it to ProgressListener.onSecurityChanged() rather than the GeckoBundle that is currently being passed. Also updates CustomTabsActivity and GeckoViewActivity to use the SecurityInformation.

Comment 2

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

Comment on attachment 8890529 [details] [diff] [review]
Add a class to represent security info rather than using a GeckoBundle

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

I think you forgot to include SecurityInformation in the patch. In any case, it'd be better if SecurityInformation is a simple inner class of ProgressListener, with public final fields. And we should use string or integer constants instead of Java enums, because Java enums are really cumbersome.

Comment 3

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

Attachment: Add a class to represent security info rather than using a GeckoBundle

Thanks for the feedback; this patch has the recommended changes. And no missing files :)

Comment 4

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

Comment on attachment 8890872 [details] [diff] [review]
Add a class to represent security info rather than using a GeckoBundle

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

Thanks! One trick I do is to preview the patch first (e.g. git log -p) whenever I'm uploading a patch, and a lot of times that helps me pick up things I forgot to include in the patch.

::: mobile/android/base/java/org/mozilla/gecko/customtabs/ActionBarPresenter.java
@@ +106,5 @@
>       *
>       * @param url Url String to display
>       */
>      public void displayUrlOnly(@NonNull final String url) {
> +        updateCustomView(null, url, false);

/* secure */ false);

@@ +114,5 @@
>       * Update appearance of CustomView of ActionBar
>       *
>       * @param title          Title for current website. Could be null if don't want to show title.
>       * @param url            URL for current website. At least Custom will show this url.
> +     * @param securityStatus A boolean representing whether or not the site is secure.

Simpler to just name it `isSecure`.

@@ +216,3 @@
>       * @param title    Title for current website. Could be null if don't want to show title.
>       * @param url      URL for current website. At least Custom will show this url.
> +     * @param securityStatus A boolean representing whether or not the site is secure.

Same

::: mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java
@@ +32,5 @@
>  
>  import org.mozilla.gecko.ActivityHandlerHelper;
>  import org.mozilla.gecko.EventDispatcher;
>  import org.mozilla.gecko.GeckoView;
> +import org.mozilla.gecko.GeckoView.ProgressListener.SecurityInformation;

I don't think you need this import.

@@ +76,5 @@
>      private boolean mCanGoForward = false;
>      private boolean mCanStop = false;
>      private String mCurrentUrl;
>      private String mCurrentTitle;
> +    private boolean mSecurityStatus = false;

mIsSecure

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java
@@ +1227,5 @@
> +        public class SecurityInformation {
> +            /**
> +             * Indicates whether or not the site is secure.
> +             */
> +            public final boolean mSecure;

Don't use `m` prefix for public fields (e.g. this field should be `isSecure`)

@@ +1231,5 @@
> +            public final boolean mSecure;
> +            /**
> +             * Indicates whether or not the site is a security exception.
> +             */
> +            public final boolean mSecurityException;

`isException`

@@ +1278,5 @@
> +             * "unknown", "blocked", and "loaded".
> +             */
> +            public final String mTrackingMode;
> +
> +            public SecurityInformation(GeckoBundle identityData) {

/* package */

@@ +1279,5 @@
> +             */
> +            public final String mTrackingMode;
> +
> +            public SecurityInformation(GeckoBundle identityData) {
> +                if (identityData == null || !identityData.containsKey("mode")) {

I don't think you have to worry about this case?

@@ +1301,5 @@
> +                mMixedModePassive = mode.getString("mixed_display");
> +                mMixedModeActive = mode.getString("mixed_active");
> +                mTrackingMode = mode.getString("tracking");
> +
> +                if (!mode.containsKey("identity") || !identityData.containsKey("origin")) {

Don't have to worry about this case?

@@ +1317,5 @@
> +
> +                mSecurityMode = mode.getString("identity");
> +
> +                mSecure = identityData.getBoolean("secure");
> +                mSecurityException = identityData.getBoolean("security_exception");

Key name should be `securityException` to be consistent with other keys.

::: mobile/android/geckoview_example/src/main/java/org/mozilla/geckoview_example/GeckoViewActivity.java
@@ +18,5 @@
>  
>  import java.util.Locale;
>  
>  import org.mozilla.gecko.GeckoView;
> +import org.mozilla.gecko.GeckoView.ProgressListener.SecurityInformation;

Don't need this import.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/526b29eb6c64
Replace GeckoBundle passed to ProgressListener.onSecurityChanged with a class. r=jchen

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/526b29eb6c64