Closed Bug 1204486 Opened 4 years ago Closed 4 years ago

Simplify gIdentityHandler and show connection type in the fullscreen notification

Categories

(Firefox :: Address Bar, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 44
Iteration:
44.1 - Oct 5
Tracking Status
firefox43 --- verified
firefox44 --- verified

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

(Keywords: addon-compat, Whiteboard: [fxprivacy])

Attachments

(1 file)

Finish the simplification of gIdentityHandler and fix an issue with the full screen notification not showing the icon with the connection security level.

I think we can also safely remove some legacy code that was kept for some time for third-party theme compatibility.
Flags: qe-verify+
Priority: -- → P1
Blocks: 1188121
Bug 1204486 - Simplify gIdentityHandler and show connection type in the fullscreen notification. r=ttaubert
Attachment #8660751 - Flags: review?(ttaubert)
I could continue with the refactoring but stopped at the point where the code is simplified enough to make it easier to add the insecure passwords notification.

This does not change which CSS classes we use for styling the identity block, so I don't expect tests to be affected. Maybe at some point we should unify those classes with the attributes we use for the Control Center. Tryserver build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c3d1674797c

Setting addon-compat for the slightly unrelated theme change for the obsolete "level" attribute, however theme authors have probably already updated their themes for the new identity block and Control Center anyways.
Keywords: addon-compat
Comment on attachment 8660751 [details]
MozReview Request: Bug 1204486 - Simplify gIdentityHandler and show connection type in the fullscreen notification. r=ttaubert

Bug 1204486 - Simplify gIdentityHandler and show connection type in the fullscreen notification. r=ttaubert
Comment on attachment 8660751 [details]
MozReview Request: Bug 1204486 - Simplify gIdentityHandler and show connection type in the fullscreen notification. r=ttaubert

https://reviewboard.mozilla.org/r/19161/#review17705

::: browser/base/content/browser.js:6746
(Diff revision 2)
> +  get _isBroken() {
> +    return this._state & Ci.nsIWebProgressListener.STATE_IS_BROKEN;
> +  },
> +
> +  get _isSecure() {
> +    return this._state & Ci.nsIWebProgressListener.STATE_IS_SECURE;
> +  },

If we ever manage to move this out into its own file it would be nice of the connection status had its own class/object that we could ask those questions.

::: browser/base/content/browser.js:6945
(Diff revision 2)
> -  checkIdentity : function(state, uri) {
> +  checkIdentity(state, uri) {

Do we maybe want to rename this to updateIdentity() or something along those lines? It's clear that this function doesn't only check things.

::: browser/base/content/browser.js:7074
(Diff revision 2)
> +    } else {
> +      this._identityBox.className = "unknownIdentity";
> +      if (this._isMixedActiveContentLoaded) {
> +        this._identityBox.classList.add("mixedActiveContent");
> +      } else if (this._isMixedActiveContentBlocked) {
> +        this._identityBox.classList.add("mixedDisplayContentLoadedActiveBlocked");
> +      } else if (this._isMixedPassiveContentLoaded) {
> +        this._identityBox.classList.add("mixedDisplayContent");
> +      } else if (this._isBroken) {
> +        this._identityBox.classList.add("weakCipher");
> +      }

This is indeed a little nicer than before, but still quite complicated to grasp. Especially with the "mixedActiveBlocked" state being handled separately. I wonder if in the future we'd maybe find some way to simplify this.

LGTM, thanks. We should definitely verify that all the MCB states are still as they were before.
Attachment #8660751 - Flags: review?(ttaubert) → review+
https://reviewboard.mozilla.org/r/19161/#review17895

::: browser/base/content/browser.js:6766
(Diff revision 2)
> +  get _isMixedPassiveContentLoaded() {

For completeness, we need an _isMixedPassiveContentBlocked().  If we don't add it here, we will end up adding it at some point.

::: browser/base/content/browser.js
(Diff revision 2)
> -    // Chrome URIs however get special treatment. Some chrome URIs are

Can you add back this comment?

::: browser/base/content/browser.js:7063
(Diff revision 2)
> +

Do you need to add back the "This can't throw..." comment here?

::: browser/base/content/browser.js:7075
(Diff revision 2)
> +      this._identityBox.className = "unknownIdentity";

You might be missing a STATE_IS_BROKEN check here?  What if we are on an HTTP page that has an HTTPS subframe that has mixed passive content.  We need to make sure we don't end up showing any type of lock in the url bar for this case.

I will take a closer look tonight or tomorrow.
https://reviewboard.mozilla.org/r/19161/#review17929

::: browser/base/content/browser.js
(Diff revision 2)
> -      // We don't style the Location Bar based on the the 'level' attribute

Do we need this "level" attribute for anything?  I'm not sure what the note about third party themes means.

::: browser/base/content/browser.js
(Diff revision 2)
> -  IDENTITY_MODE_MIXED_DISPLAY_LOADED                   : "unknownIdentity mixedContent mixedDisplayContent",  // SSL with unauthenticated display content

Some of these have multiple classes, but maybe they are redundant.  I notice in the new implementation you don't use the "mixedContent" class.  I can't seem to find where that is defined.  But there are a number of tests that seem to reference it that you may need to update.
https://dxr.mozilla.org/mozilla-central/search?q=mixedContent&redirect=false&case=true&limit=65&offset=65

::: browser/base/content/browser.js
(Diff revision 2)
> -    // For some URIs like data: we can't get a host. URIs without a host will

Please add this comment back in.

::: browser/base/content/browser.js:7074
(Diff revision 2)
> +    } else {
> +      this._identityBox.className = "unknownIdentity";
> +      if (this._isMixedActiveContentLoaded) {
> +        this._identityBox.classList.add("mixedActiveContent");
> +      } else if (this._isMixedActiveContentBlocked) {
> +        this._identityBox.classList.add("mixedDisplayContentLoadedActiveBlocked");
> +      } else if (this._isMixedPassiveContentLoaded) {
> +        this._identityBox.classList.add("mixedDisplayContent");
> +      } else if (this._isBroken) {
> +        this._identityBox.classList.add("weakCipher");
> +      }

Yes, as mentioned in my previous comment, you need an "if (this._isBroken)" around everything in this else block.

I don't recall what we do to get the right text in control center for a top level HTTP page that has mixed active content blocked.  Here are some test cases you can look at and a bug that talks about this:
http://people.mozilla.org/~tvyas/mixedgrandiframe.html (HTTP top level page, HTTPS frame that tries to load HTTP content.  This text could use some work but it's okay.)

http://people.mozilla.org/~tvyas/http_parent_with_mixed_passive_https_child.html (HTTP top level page with HTTPS frame that loads HTTP passive content.  The text in the control center really doesn't make sense in this case and I'm happy to see it change to the basic HTTP text instead, but that is a separate bug.)

https://bugzilla.mozilla.org/show_bug.cgi?id=1182551
Thanks for the review!

(In reply to Tanvi Vyas [:tanvi] from comment #5)
> Do you need to add back the "This can't throw..." comment here?

That's now clear from the context, since we're checking _uriHasHost just a few lines above. Having a cleaner code flow means that more of the assumptions made by the code are now understandable just by reading the code itself.

Part of the refactoring in fact consisted in replacing in-flow comments with reworded, cleaner descriptions using JSDoc-style comments on the data stored in the class, which at least in theory can be cross-referenced by smart IDEs.

(In reply to Tanvi Vyas [:tanvi] from comment #6)
> Do we need this "level" attribute for anything?  I'm not sure what the note
> about third party themes means.

No, we're not using it anymore. Installable themes from the distant past that overrode the internal CSS styles for the URL bar to give it a different appearance based on this attribute also probably don't exist anymore.

> in the new implementation you don't use the "mixedContent" class.  I can't
> seem to find where that is defined.

Yeah, it's not used for styling so I removed it.

> But there are a number of tests that
> seem to reference it that you may need to update.

And had to update the tests as well.

> ::: browser/base/content/browser.js:7074
> > +    } else {
> > +      this._identityBox.className = "unknownIdentity";
> 
> Yes, as mentioned in my previous comment, you need an "if (this._isBroken)"
> around everything in this else block.

Yeah, I made the wrong assumptions about the correlation between isBroken and mixed content states. This was actually caught by the browser_mixedContentFramesOnHttp.js test so I think we're fine with test coverage here.

(I did a local browser-chrome test run before the tryserver push, but for some reason all tests succeeded, maybe I didn't rebuild properly at that time.)
Comment on attachment 8660751 [details]
MozReview Request: Bug 1204486 - Simplify gIdentityHandler and show connection type in the fullscreen notification. r=ttaubert

Bug 1204486 - Simplify gIdentityHandler and show connection type in the fullscreen notification. r=ttaubert
Iteration: 43.3 - Sep 21 → 44.1 - Oct 5
https://hg.mozilla.org/mozilla-central/rev/15fd1ffcc1be
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
If the connection type is not displayed in 43, we should uplift this patch to 43.
Flags: needinfo?(paolo.mozmail)
Comment on attachment 8660751 [details]
MozReview Request: Bug 1204486 - Simplify gIdentityHandler and show connection type in the fullscreen notification. r=ttaubert

Approval Request Comment
[Feature/regressing bug #]: Insecure password fields warning, fullscreen notification
[User impact if declined]: Missing icon in the fullscreen notification. This is also a prerequisite for uplifting the insecure password fields warnings feature to Firefox 43.
[Describe test coverage new/current, TreeHerder]: Landed on mozilla-central, has automated test coverage.
[Risks and why]: Changes are to the Control Center and Identity Block code, risk of regressions is low since that's well covered by tests.
[String/UUID change made/needed]: None
Flags: needinfo?(paolo.mozmail)
Attachment #8660751 - Flags: approval-mozilla-aurora?
I'll note that we've just decided not to uplift the insecure password fields warning feature, but the patch still fixes the fullscreen notification icon.
Comment on attachment 8660751 [details]
MozReview Request: Bug 1204486 - Simplify gIdentityHandler and show connection type in the fullscreen notification. r=ttaubert

Let's uplift this, since it has test coverage and fixes the full screen notification issue.
Attachment #8660751 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to :Paolo Amadini from comment #14)
> I'll note that we've just decided not to uplift the insecure password fields
> warning feature
Is this feature enabled in Nightly? How can I see it?
Flags: needinfo?(paolo.mozmail)
The simpler way to verify this is to check that the fullscreen notification shows a lock or a lock with a strikethrough depending on the connection type of the page requesting fullscreen.
Flags: needinfo?(paolo.mozmail)
Both notification displayed correctly on youtube.com and imdb.com.
Verified fixed 43.0a2 (2015-10-16), 44.0a1 (2015-10-15) Win 7.
Status: RESOLVED → VERIFIED
Depends on: 1213178
Depends on: 1233974
You need to log in before you can comment on or make changes to this bug.