Closed Bug 945212 Opened 6 years ago Closed 6 years ago

Encapsulate identity data/security mode behind a type-safe API

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: lucasr, Assigned: lucasr)

References

Details

Attachments

(1 file, 1 obsolete file)

Instead of passing JSON and strings around.
Comment on attachment 8341029 [details] [diff] [review]
Encapsulate identity data/security mode behind a type-safe API (r=margaret)

Part of some of the major refactorings being worked on in bug 942862, bug 944533, and bug 943915.
Attachment #8341029 - Flags: review?(margaret.leibovic)
Comment on attachment 8341029 [details] [diff] [review]
Encapsulate identity data/security mode behind a type-safe API (r=margaret)

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

This looks like a good improvement, but I think this patch breaks the loaded mixed content notification. I tried applying this patch locally to test, but it didn't apply.

::: mobile/android/base/SiteIdentity.java
@@ +2,5 @@
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +package org.mozilla.gecko;

I wonder if this should go in the toolbar package, since that's where SiteIdentityPopup is.

::: mobile/android/base/toolbar/SiteIdentityPopup.java
@@ -124,5 @@
> -            // Hide the identity data if there isn't valid site identity data.
> -            // Set some top padding on the popup content to create a of light blue
> -            // between the popup arrow and the mixed content notification.
> -            mContent.setPadding(0, (int) mResources.getDimension(R.dimen.identity_padding_top), 0, 0);
> -            mIdentity.setVisibility(View.GONE);

This logic was added to handle pages that have loaded mixed content. In this case, we don't want to show the site identity because we've loaded unencrypted content, but we still want to show the popup so that we can include a notification that allows the user to re-enable mixed content blocking. See the patch where this logic was added:
http://hg.mozilla.org/mozilla-central/diff/17523d827afa/mobile/android/base/SiteIdentityPopup.java

In retrospect, this seems pretty hacky, and we should just be checking the security mode in here to do this.

We need to make sure this still works. You can test by visiting this page, then disabling mixed content blocking, then tapping on the yellow triangle to open the popup:
https://people.mozilla.org/~tvyas/mixedcontent.html

@@ +133,5 @@
>       * @param identityData A JSONObject that holds the current tab's identity data.
>       */
> +    void updateIdentity(SiteIdentity siteIdentity) {
> +        final SecurityMode mode = siteIdentity.getSecurityMode();
> +        if (mode == SecurityMode.UNKNOWN) {

We now never call updateIdentity in this case, so you could get rid of this check.
Attachment #8341029 - Flags: review?(margaret.leibovic) → review-
(In reply to :Margaret Leibovic from comment #3)
> Comment on attachment 8341029 [details] [diff] [review]
> Encapsulate identity data/security mode behind a type-safe API (r=margaret)
> 
> Review of attachment 8341029 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks like a good improvement, but I think this patch breaks the loaded
> mixed content notification. I tried applying this patch locally to test, but
> it didn't apply.

This is part of a big patch queue I have. You'd need to apply this patch on top of the ones from bug 943915. 

> ::: mobile/android/base/SiteIdentity.java
> @@ +2,5 @@
> > + * This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +package org.mozilla.gecko;
> 
> I wonder if this should go in the toolbar package, since that's where
> SiteIdentityPopup is.

I considered doing that but SiteIdentity is something meant to be owned by the Tab. It just happens to be used by SiteIdentityPopup now but it could be used in a different type of UI in the future.

> ::: mobile/android/base/toolbar/SiteIdentityPopup.java
> @@ -124,5 @@
> > -            // Hide the identity data if there isn't valid site identity data.
> > -            // Set some top padding on the popup content to create a of light blue
> > -            // between the popup arrow and the mixed content notification.
> > -            mContent.setPadding(0, (int) mResources.getDimension(R.dimen.identity_padding_top), 0, 0);
> > -            mIdentity.setVisibility(View.GONE);
> 
> This logic was added to handle pages that have loaded mixed content. In this
> case, we don't want to show the site identity because we've loaded
> unencrypted content, but we still want to show the popup so that we can
> include a notification that allows the user to re-enable mixed content
> blocking. See the patch where this logic was added:
> http://hg.mozilla.org/mozilla-central/diff/17523d827afa/mobile/android/base/
> SiteIdentityPopup.java
> 
> In retrospect, this seems pretty hacky, and we should just be checking the
> security mode in here to do this.

Oh, that wasn't obvious at all :-) I'll change the patch to handle this case based on the security mode instead.

> We need to make sure this still works. You can test by visiting this page,
> then disabling mixed content blocking, then tapping on the yellow triangle
> to open the popup:
> https://people.mozilla.org/~tvyas/mixedcontent.html

Thanks, I'll double-check this case works before submitting the new patch.

> @@ +133,5 @@
> >       * @param identityData A JSONObject that holds the current tab's identity data.
> >       */
> > +    void updateIdentity(SiteIdentity siteIdentity) {
> > +        final SecurityMode mode = siteIdentity.getSecurityMode();
> > +        if (mode == SecurityMode.UNKNOWN) {
> 
> We now never call updateIdentity in this case, so you could get rid of this
> check.

Yeah, I've changing the SiteIdentityPopup API a bit in bug 944533 (which covers your point here).
Attachment #8341029 - Attachment is obsolete: true
Comment on attachment 8341690 [details] [diff] [review]
Encapsulate identity data/security mode behind a type-safe API (r=margaret)

I confirm this works with the mixed content blocking as expected.
Attachment #8341690 - Flags: review?(margaret.leibovic)
Comment on attachment 8341690 [details] [diff] [review]
Encapsulate identity data/security mode behind a type-safe API (r=margaret)

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

Looks good, thanks. Sorry about that confusing bit, but at least it's much clearer now!
Attachment #8341690 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/e61d36bd385f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Depends on: 951605
Depends on: 966866
You need to log in before you can comment on or make changes to this bug.