Closed
Bug 1381536
Opened 7 years ago
Closed 7 years ago
Replace GeckoBundle passed to ProgressListener.onSecurityChanged with a class
Categories
(GeckoView :: General, enhancement)
Tracking
(firefox56 fixed)
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: droeh, Assigned: droeh)
Details
Attachments
(1 file, 1 obsolete file)
18.99 KB,
patch
|
jchen
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•7 years ago
|
||
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 2•7 years ago
|
||
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+
Assignee | ||
Comment 3•7 years ago
|
||
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 4•7 years ago
|
||
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
Comment 6•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/526b29eb6c64
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Updated•5 years ago
|
Product: Firefox for Android → GeckoView
Updated•5 years ago
|
Target Milestone: Firefox 56 → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•