Closed Bug 1381536 Opened 7 years ago Closed 7 years ago

Replace GeckoBundle passed to ProgressListener.onSecurityChanged with a class

Categories

(GeckoView :: General, enhancement)

53 Branch
All
Android
enhancement
Not set
normal

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: droeh, Assigned: droeh)

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
Attachment #8890529 - Flags: review?(nchen)
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.
Attachment #8890529 - Flags: review?(nchen) → feedback+
Thanks for the feedback; this patch has the recommended changes. And no missing files :)
Attachment #8890529 - Attachment is obsolete: true
Attachment #8890872 - Flags: review?(nchen)
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.
Attachment #8890872 - Flags: review?(nchen) → review+
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/526b29eb6c64
Replace GeckoBundle passed to ProgressListener.onSecurityChanged with a class. r=jchen
https://hg.mozilla.org/mozilla-central/rev/526b29eb6c64
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 56 → mozilla56
You need to log in before you can comment on or make changes to this bug.