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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

Instead of passing JSON and strings around.

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Encapsulate identity data/security mode behind a type-safe API (r=margaret)

Comment 2

[Lucas Rocha (:lucasr)]

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.

Comment 3

[: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.

Comment 4

[Lucas Rocha (:lucasr)]

(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).

Comment 5

[Lucas Rocha (:lucasr)]

Attachment: Encapsulate identity data/security mode behind a type-safe API (r=margaret)

Comment 6

[Lucas Rocha (:lucasr)]

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.

Comment 7

[: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!

Comment 8

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/e61d36bd385f

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e61d36bd385f