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.

Bug 1204486 - Simplify gIdentityHandler and show connection type in the fullscreen notification. r=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.

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.

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

If the connection type is not displayed in 43, we should uplift this patch to 43.

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

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.

(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?

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.

Both notification displayed correctly on youtube.com and imdb.com. Verified fixed 43.0a2 (2015-10-16), 44.0a1 (2015-10-15) Win 7.