Johnathan Nightingale [:johnath] Assignee
	Description • 17 years ago

In all the conversation about EV certificates, the need has become apparent for firefox to have some way of presenting website identity information to the user.  While the padlock has traditionally been co-opted to do identity signalling as well as encryption, it's become a confused metaphor that tries to talk about security and identity simultaneously.  

While bug 348677 speaks specifically about the Microsoft approach to EV certificates, the resulting UI should not be couched in terms of EV exclusively.  In principle (though I know of no other relevant identity technologies at the moment) this indicator should not be tied to EV as a technology, but to strongly verified identity in general.

The current discussion around such things points to a passport-inspector icon which, on click, presents an identity summary including site owner information (extracted from the subject information of a cert, for example), verifier information (extracted from a cert's issuer information, for example) and an affordance to get more information (launching the security tab of Page Info).

This has been implemented in the "Larry" extension, attached, and available during development from the supplied url.

For background, please see:

http://blog.johnath.com/index.php/2007/03/13/revisiting-security-ui-part-1-of-2/
http://blog.johnath.com/index.php/2007/03/21/revisiting-security-ui-part-2/
http://blog.johnath.com/index.php?p=87

This also addresses Firefox PRD line item SPI-001b

	Ben Lerner
	Comment 1 • 17 years ago

I'd vote for a UI closer to your second mock-up: have the icon be inside the address bar, but with no text initially visible.  On mouseover on the icon, I'd show the text, and on mousedown, I'd pop up the alert you currently show.  (For visual feedback, you might want to have the icon glow or look like the buttons on the navigation toolbar already, so it looks like there might be more to see.)

Other than gray/black, it's pretty hard to see the difference between Confused-Larry and OK-Larry -- and what would Unhappy-Larry look like if the EV cert. seems wrong somehow?

	Dão Gottwald [:dao]
	Comment 2 • 17 years ago

Bear in mind to display the country code (as in attachment 258709 [details]).
See also <http://wiki.mozilla.org/Firefox3/Location_Bar#Display_EV_business_name_and_country>.

	Stephen Davidson
	Comment 3 • 17 years ago

Some clarification of how the UI would handle EV SSL vs non-EV SSL from a trust root vs SSL from an untrusted root would be useful.

	Johnathan Nightingale [:johnath] Assignee
	Comment 4 • 17 years ago

Larry is now on the left, against the location bar, and uses green colour treatment of identity text only (not the entire location bar.)

	Johnathan Nightingale [:johnath] Assignee
	Comment 6 • 17 years ago

This is the first pass conversion of the XPI identity indicator to a patch.  Notable details:

 - it uses the passport icon in primary chrome, with Larry in the popup
 - it indicates subject organization and country code in primary chrome, verifier in primary chrome tooltip, and in the elaboration popup adds location data (city, state, country) and encryption status.  
 - it treats all valid SSL as EV.  This will change when bug 386654 lands by way of a follow-up bug, but I think it's okay to have it behave this way for testing, and ups the chances that beta testers will actually see the UI doing things.  My hope is to file a follow-up bug to handle all 3 levels (non-SSL, SSL-non-EV, and EV) once support exists to do so.
 - it requires adding 3 graphics files to each of browser/themes/pinstripe/browser and browser/themes/winstripe/browser, which I will attach below.

	Dão Gottwald [:dao]
	Comment 11 • 17 years ago

Comment on attachment 271565 [details] [diff] [review]
Patch against trunk

>+  setMode : function(newMode) {
>+    document.getElementById("identity-popup").className = newMode;
>+    document.getElementById("identity-box").className = newMode;

Shouldn't this be an attribute rather than a class?

	Kai Engert (:KaiE:)
	Comment 12 • 17 years ago

(In reply to comment #1)
> Other than gray/black, it's pretty hard to see the difference between
> Confused-Larry and OK-Larry

Maybe it would be an improvement to show a red cross on top of Confused-Larry, or maybe, do not show a Confused-Larry at all? Just a random idea.


> -- and what would Unhappy-Larry look like if the EV
> cert seems wrong somehow?

Bad cert == No identity confirmation == Unhappy Larry

	Kai Engert (:KaiE:)
	Comment 13 • 17 years ago

(In reply to comment #3)
> Some clarification of how the UI would handle EV SSL vs non-EV SSL from a trust
> root vs SSL from an untrusted root would be useful.

Untrusted Root == No identity confirmation == Unhappy Larry

	Kai Engert (:KaiE:)
	Comment 14 • 17 years ago

For testing purposes (only) your patch shows Happy-Larry for all SSL sites. The final version of your patch will (obviously) need to depend on a real EV notification. So this is blocked by bug 386654.

We must define how the security implementation will announce the EV status, and Happy-Larry should only be shown when we detect a strong identity proof (such as an EV cert).

Johnathan, when we discussed a while ago, we kind of agreed to add another state bit to the existing OnSecurityChange event, and bug 386654 proposes to add STATE_SECURITY_EV_TOPLEVEL (FYI).

	Kai Engert (:KaiE:)
	Comment 15 • 17 years ago

Johnathan, have you tested your patch on an https site that contains some insecure content? 

(For example, use https://kuix.de/misc/test16/delayed-insecure.html
After a delay the left frame will load an insecure page)

If I understand correctly, you have hooked up Happy-Larry to the OnLocationChange progress event.

While a document might initially appear to have good security/identity, this may change while loading all the pieces of a complex page.

As soon as an insecure items gets loaded on the page, you'll be notified using the OnSecurityChange function with a state of "insecure" or "broken".

I think it's necessary that you hook up your code to the OnSecurityChange function and potentially switch back to Unhappy-Larry. Maybe it makes sense to make all decisions regarding Larry in that function?

(When you change OnSecurityChange please also note bug 386681 that is currently in progress)

	Johnathan Nightingale [:johnath] Assignee
	Comment 16 • 17 years ago

(In reply to comment #14)
> For testing purposes (only) your patch shows Happy-Larry for all SSL sites. The
> final version of your patch will (obviously) need to depend on a real EV
> notification. So this is blocked by bug 386654.
> 
> Johnathan, when we discussed a while ago, we kind of agreed to add another
> state bit to the existing OnSecurityChange event, and bug 386654 proposes to
> add STATE_SECURITY_EV_TOPLEVEL (FYI).

I do remember that, and thank you for adding the blocker flag.  If it expedites the process, I don't mind having this bug land in testing mode, and a follow-up, firefox3-blocker being filed to implement the change to EV-only.  Then that bug could block on 386654, instead of this one.  What do you think?

(In reply to comment #15)
> Johnathan, have you tested your patch on an https site that contains some
> insecure content? 
> 
> If I understand correctly, you have hooked up Happy-Larry to the
> OnLocationChange progress event.
> 
> While a document might initially appear to have good security/identity, this
> may change while loading all the pieces of a complex page.
> 
> As soon as an insecure items gets loaded on the page, you'll be notified using
> the OnSecurityChange function with a state of "insecure" or "broken".
> 
> I think it's necessary that you hook up your code to the OnSecurityChange
> function and potentially switch back to Unhappy-Larry. Maybe it makes sense to
> make all decisions regarding Larry in that function?

The current patch adds hooks in two places in browser.js:

http://mxr.mozilla.org/mozilla/source/browser/base/content/browser.js#3825 - within onLocationChange in nsBrowserStatusHandler, and

http://mxr.mozilla.org/mozilla/source/browser/base/content/browser.js#3946 - within onSecurityChange in the same class.

	Eddy Nigg (StartCom)
	Comment 17 • 17 years ago

Johnathan, does your statement from the first post still apply and how?

> this indicator should not be tied to EV as a
> technology, but to strongly verified identity in general

	Johnathan Nightingale [:johnath] Assignee
	Comment 18 • 17 years ago

(In reply to comment #0)

> While bug 348677 speaks specifically about the Microsoft approach to EV
> certificates, the resulting UI should not be couched in terms of EV
> exclusively.  In principle (though I know of no other relevant identity
> technologies at the moment) this indicator should not be tied to EV as a
> technology, but to strongly verified identity in general.

(In reply to comment #17)
> Johnathan, does your statement from the first post still apply and how?

As I said in the opening post, I don't think it's appropriate to make any visible or terminological link to "EV" certificates in the UI, since I think it would educate users into thinking there was only one standard.  The current code does not mention EV anywhere.  However, EV is the technology that will back this indicator for the moment.  We're leaving the door open to other, equivalent technologies by not making a visible link that we would later have to undo, but we are not considering any other certificates for EV-like treatment at the moment.

	Kai Engert (:KaiE:)
	Comment 19 • 17 years ago

(In reply to comment #16)
> If it expedites
> the process, I don't mind having this bug land in testing mode, and a
> follow-up, firefox3-blocker being filed to implement the change to EV-only. 
> Then that bug could block on 386654, instead of this one.  What do you think?

I'm not sure.

FWIW, I tried your patch, combined with the patches from bug 386654. I think there is only a small change required in your patch.

Where you currently have:

+    // FIXME - Right now, for convenience of testing, we'll pretend that any site with SECURE_HIGH is identified
+    if (state & Components.interfaces.nsIWebProgressListener.STATE_SECURE_HIGH) {

the new code would be:
+    if (state & Components.interfaces.nsIWebProgressListener2.STATE_SECURITY_EV_TOPLEVEL) {



> The current patch adds hooks in two places in browser.js:

Sorry, I had missed that, thanks for the pointer.

	Jez
	Comment 20 • 17 years ago

Just to barge into the discussion:

What does the new UI do when a site uses HTTPS, but has a self-signed certificate?  I'd like to basically see 4 separate levels of security in the UI:

1. Unencrypted
2. Encrypted w/ self-signed SSL
3. Encrypted w/ SSL
4. Encrypted w/ EV

I think allowing sites to self-sign SSL certs and not appear to be 'fraudulent' would be a useful step forward, for people who don't wish to go through the hassle of getting an SSL cert from a CA but do want to encrypt the connection.  Users should not be told that such certificates are a Bad Thing, as they are in FF2 ('please inform the site's webmaster about this problem').  Surely encryption without authentication is better than nothing?  Yet, the current UI seems to suggest that it's actually less trustworthy.

	Eddy Nigg (StartCom)
	Comment 21 • 17 years ago

Encryption without any authentication whatsoever (self-signed) is a bad thing, since it pretends to provide security where there isn't any. Except of course if anyone accessing the site has received for example the fingerprint of the certificate via a different channel. Obviously this is not very common practise. There is also a page which addresses your concerns: http://wiki.mozilla.org/Security:SSLErrorPages#Motivations_.26_Objections

The StartCom CA provides certification (DV Class 1) without charge, making the use of self-signed certificates unnecessary if the price factor is the motivation for it. See http://cert.startcom.org/

	Nelson Bolyard (seldom reads bugmail)
	Comment 22 • 17 years ago

There is another issue: weak encryption (e.g. the old "40-bit" encryption).
I might suggest adding another level to your list of 5, but perhaps it is 
simplest to lump "weak encryption" together with "unauthenticated encryption"
(that is, self-signed certs).  Both constitute "meaningless encryption" or
"false sense of security".

Self-signed certs don't provide reliable authentication.  People who trust 
them are vulnerable to attacks, and do not (in general) know if they are 
really connected to the party named in the cert, or an impostor.
Encryption without authentication gives a false sense of security.  

There have been proposals in the past to signify weak encryption and 
unauthenticated encryption as no encryption at all.  There have been 
other proposals to separate the indicator of encryption from the indicator
of authentication.  I thought we had settled that issue long ago, after
much wrangling, but I don't recall (at the moment) where that was recorded
(probably in some bug, but which?).  I hope we're not about to take another
lap around that track. :-(

	Jez
	Comment 23 • 17 years ago

(In reply to comment #21)
> Encryption without any authentication whatsoever (self-signed) is a bad thing,
> since it pretends to provide security where there isn't any.

Not true.  It simply matters what you're trying to provide security from.  SSH generally doesn't involve a 3rd party CA, so why do people use it?  To make it harder for middlemen to just sniff packets and get useful information, that's why.  (Strong) encryption without authentication does the same, and I think it's doing it a disservice to call it useless (or even worse, tell the user that it's somehow suspicious).

	Eddy Nigg (StartCom)
	Comment 24 • 17 years ago

(In reply to comment #22)
> There is another issue: weak encryption (e.g. the old "40-bit" encryption).

Aren't those disabled by now? I thought we got rid of SSLv2 and weak encryption support by now?

> I hope we're not about to take another
> lap around that track. :-(

Sorry that I replied. No intention to start anything here, just wanted to point out that there are alternatives for self-singed certs by now.

	Nelson Bolyard (seldom reads bugmail)
	Comment 25 • 17 years ago

Eddy, thanks for your comments.  

Your offer of free SSL server certs clearly puts the final nail in the 
coffin for the excuses for net getting a valid SSL server cert.  

	Jez
	Comment 26 • 17 years ago

Nelson: Not really.  It's still a pain in the ass to get one.  If you want to do a quick-and-dirty bit of HTTPS stuff, you don't really want to go through the process of getting even a free cert.  Also, there's no guaranteeing that the free service won't go up in smoke in 6 months' time when they discover they're losing too much money.

	Jez
	Comment 27 • 17 years ago

OK, basically what it seems to me we want to do is split what the padlock currently merges into 1, authentication and encryption, into 2.

Now this bug is dealing with displaying the authentication part pretty well, IMHO.  I like it.  The authentication stuff is at the left of the URLbar.  Why don't we deal with the encryption element on the right of the bar?  We could use.... a padlock.  Simply display a padlock for HTTPS connections.  Then we've successfully split the two.  It's not really a problem for the user, because it should make it quite easy to see that a site is TOTALLY unauthenticated (you'll have the 'puzzled Larry' or whatever), yet encrypted.  I really don't think it's too tough to understand, and at the end of the day, considering that with HTTPS encryption is mandatory anyway, you can tell the really dumb users that all they should look at is the auth info on the left.  They're always getting encryption if that's there.  But for users who recognise that encryption without authentication is still useful for less serious purposes, the padlock tells them that.

	Nelson Bolyard (seldom reads bugmail)
	Comment 28 • 17 years ago

If you separate authentication indicator from encryption indicator, then 
either the padlock should not be used for either one, or the padlock 
should mean authentication.  Encryption without authentication is false
security.  Users who have been conditioned to thing "padlock == secure"
should not be misled by a padlock when they have no authentication.

	Jez
	Comment 29 • 17 years ago

(In reply to comment #28)
> Users who have been conditioned to thing "padlock == secure"
> should not be misled by a padlock when they have no authentication.

At the risk of sounding like Gene Ray, those users have been educated stupid.  The padlock traditionally means 'encyrpted', and it does in just about every other internet application that uses it.  They should be educated properly, not allowed for that retarded paradigm to continue.

	Kai Engert (:KaiE:)
	Comment 30 • 17 years ago

Johnathan, I hope you don't mind, while I was trying out your patch, I also updated it to apply to the latest version of the trunk, there were are a couple of changes in the near context...

This version applies cleanly as of today.

Note this version has the change that I proposed in comment 19.

	Kai Engert (:KaiE:)
	Comment 31 • 17 years ago

I might have found a bug, when I have 2 tabs open and switch back and forth between them, the bold passport and green identity area is lost.

I haven't yet tested whether this was already in the original patch, or whether I have introduced this with my updated patch. Maybe the new state flag is not being preserved somehow?

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 32 • 17 years ago

Comment on attachment 271668 [details]
Grid of identity indicator visuals

I think the combination of the passport and the favicon ends up being visually noisy, and in the grey form, looks disabled, not "empty".

I'd rather we treat the favicon itself as the avatar/representor of the "site", and then just always have the identity information displayed in association with that.

Other things that johnath and I have discussed out of band include having Larry validate/verify the domain name in DV certificates (since we can verify that you are, indeed, connected to the domain being reported) and perhaps rendering that information with the TLD+1 path in boldface.

There's an implicit ui-r+ here, by the by, for just dropping the passport-button part of this and keeping the rest. :)

	Jez
	Comment 33 • 17 years ago

(In reply to comment #32)
> (From update of attachment 271668 [details])
> I think the combination of the passport and the favicon ends up being visually
> noisy, and in the grey form, looks disabled, not "empty".
> 
> I'd rather we treat the favicon itself as the avatar/representor of the "site",
> and then just always have the identity information displayed in association
> with that.

I'm surprised you'd say that.  I think the passport icon is very well-distinguished from the URLbar with the favicon.  If you use your suggestion, you're asking users to click on the favicon to get information about the website's authentity?  That seems unintuative, especially considering that a phishing site is going to use the exact same favicon as $BANK.  Clicking on the HSBC icon to find out that HSBC isn't really HSBC?  Hmm.

The grey passport seems much better to me.

	Johnathan Nightingale [:johnath] Assignee
	Comment 34 • 17 years ago

This is an update of the original patch that mostly alters the UI presentation.  The current implementation uses the favicon as the affordance (since clicking the favicon used to do nothing) rather than adding a new button, and lives within the URL bar instead of outside.  This has the effect of also obsoleting the "primary chrome icons" attachment.  As mentioned in comment 32, this presentation has an implicit ui-r (at least as far as landing it so we can see how it works in broader deployment).

This patch does not include the change proposed by Kai in attachment 275448 [details] [diff] [review] because the requisite code has not yet landed, however if we can get bug 386654 it should be easy to make the change.

	Johnathan Nightingale [:johnath] Assignee
	Comment 35 • 17 years ago

Now that bug 386654 has landed, I've updated the patch to handle the new EV state flag.  I've also taken the opportunity to combine the larry icons into one stacked graphic.  I'll upload the new graphic separately.

	Johnathan Nightingale [:johnath] Assignee
	Comment 36 • 17 years ago

Like the older, obsoleted versions, this file needs to be dropped in both:

browser/themes/pinstripe/browser/

and

browser/themes/winstripe/browser/

	Jez
	Comment 38 • 17 years ago

Johnathan: what's the plan for when a website encrypts the connection with a self-signed certifiate?  I think it should be (positively) distinguished from no encryption, perhaps without the user having to actually click on the left part of the URLbar.

	Nelson Bolyard (seldom reads bugmail)
	Comment 39 • 17 years ago

If mozilla can convince its own community of the truth that encryption 
without authentication offers NO protection from active attacks, it will
have done a great service.

	Nelson Bolyard (seldom reads bugmail)
	Comment 40 • 17 years ago

Regarding the string 
    "This site does not supply information identifying its owner"
which appears in the DV elaboration popup, 
That's not necessarily true.  It may provide UNVERIFIED information 
asserting identity of its owner.  A user who looks at the cert may say
"I see information identifying its owner" and hence disbelieve the 
statement in the DV elaboration popup.  

A more accurate statement would be something like this:
  "The information identifying the owner of this site has not been verified"
or "has not been verified by the issuer of this certificate".

	Eddy Nigg (StartCom)
	Comment 41 • 17 years ago

Nelson, both statements might not be true! You don't make a distinction between pure DV validated and otherwise higher validated certification, something about which I made the Mozilla community aware of some time ago and made a proposal which could provide a solution.

If for example either string would appear with a Class 2 or 3 verified certificate issued by us (StartCom), then it would not be true either. Obviously holders of such certificates know otherwise and might complain to Mozilla because of the misinterpretation and wrongful statement!

I believe that in the current situation and new identity indicators, Mozilla must be very careful about which statements it makes about various types of digital certification and perhaps something very smart must be found!

	Nelson Bolyard (seldom reads bugmail)
	Comment 42 • 17 years ago

Eddy, you're right, so here's a second take:
"The information identifying the owner of this site may not have been verified"

	Eddy Nigg (StartCom)
	Comment 43 • 17 years ago

Yes, this statement is more carefully worded. In that case, it moves the burden to find out about it to the user obviously. 
I don't know what the proposed string is for EV, but I suggest an equally careful worded statement should apply - after all Mozilla isn't going to make any guaranties or (can) take full responsibility either.
The identity indicator should provide good indications, but refrain from bold statements. Hope this has been taken care of accordingly.

	Jez
	Comment 44 • 17 years ago

(In reply to comment #39)
> If mozilla can convince its own community of the truth that encryption 
> without authentication offers NO protection from active attacks, it will
> have done a great service.

That's bollocks.  A man in the middle cannot intercept your traffic if it's encrypted, UNLESS he has the ability to intercept the session from the beginning and rewrite public keys.  I'd say that's a significant hurdle.

	Eddy Nigg (StartCom)
	Comment 45 • 17 years ago

The classic Man-in-the-Middle attack relies upon convincing two hosts that the computer in the middle is the other host. This can be accomplished with a domain name spoof if the system is using DNS to identify the other host. Only if the domain of the SSL certificate has been authenticated (DV) by a third party, known to verify ownership of the domain, can a MITM prevented. Obviously self-signed certificates don't fall under this criteria. 

	Jez
	Comment 46 • 17 years ago

What are you referring to by 'domain name spoof'?

	Eddy Nigg (StartCom)
	Comment 47 • 17 years ago

There are various attack vectors, DNS cache poisoning one of them. Explaining all the possible options would go beyond the usefulness of this forum, but a quick search can lead you to many answers. I found http://www.securesphere.net/download/papers/dnsspoof.htm on my first try by searching for "dns spoofing". I'm sure you'll find a lot more useful information.

	Jesse Ruderman
	Comment 48 • 17 years ago

Once an attacker inserts himself between you and the server (whether through direct MITM or subverting DNS), using encryption without authentication merely presents him with a technical hurdle (requiring the attacker to write additional software, once) rather than a security hurdle (requiring the attacker to gain control over more systems, every time).

I don't find the "attacker hacks a router while you're already in the middle of a session" scenario to be a compelling argument for the use of encryption without authentication.  For one thing, you probably wouldn't even notice if the connection were dropped and re-established with new session keys.

	Jez
	Comment 49 • 17 years ago

But people use encryption without authentication for things like SSH...

	Eddy Nigg (StartCom)
	Comment 50 • 17 years ago

One can use certificate authentication with SSH (suggested). Without it, it can fail similar to SSL without authentication. Widely used, but insecure implementation doesn't make it more secure because of that. Also casual visitors of web sites don't have the same knowledge like the ones using the secure shell.

	Jez
	Comment 51 • 17 years ago

So you're really telling me that there's no security difference between submitting a username and password via HTTP, and submitting via self-signed HTTPS?  No potential attackers will be eliminated by the encryption?

	Nelson Bolyard (seldom reads bugmail)
	Comment 52 • 17 years ago

In reply to comment 44:
It's utterly trivial for any ISP to MITM its users PROVIDED THAT the ISP 
has a root CA cert in the user's list of trusted roots.  There are many
financial incentives to do so.  If your ISP has a root CA cert in your
root list, be afraid, or (better yet) turn off the trust for that CA cert
so that your products will stop trusting certs from that CA.

In most corporate environments, it is easy for the corporation to MITM 
its employees IFF the corporation has a root CA cert in the user's trusted
roots list, which many do.  

It's easy for ANY http proxy server to MITM its users, provided that its
users have a root CA cert that is operated by the operator of that proxy.
There have been entire businesses on the Internet whose business model is
to do exactly that, and aggregate statistics about users' use of https 
sites and sell that aggregated information.  To do it, they had an 
installer that installed their root into Windows' cert store (trivially
easy).  Fortunately, FireFox users were immune to that attack because 
the installer did not attempt to install their root into FF's cert store.

MITM is more common than you think.  Don't think of the attacker being 
some kid down the street.  Think of the attacker being much more monied
than that.  Trust me, Jeremy, as the lead SSL developer for mozilla for 
the last 9 years, I've seen a LOT of attacks you might not imagine.

I suggest we move further discussion of the viability of MITM attacks to
m.d.t.crypto.  

	Nelson Bolyard (seldom reads bugmail)
	Comment 53 • 17 years ago

In reply to comment 51, the only attacks that are prevents by self-signed
certs are "passive" attacks, not "active" attacks.  Self-signed certs stop
sniffing, and that's all.  

Another common place for MITM attacks is Wifi Hotspots.  The Wifi hotspot
router intercepts connections from all clients who aren't paying, and 
routes them to its own server, who responds to all requests with its own
pages.  That goes for http and https too.  This is a standard feature of 
turnkey WiFi hotspot products.  Usually your browser notices that the https 
server cert doesn't match the host name you wanted (a "host name mismatch"), 
but if you override it, you're fair game for the WiFi's MITM.  
Followup to mozilla.dev.tech.crypto

	Nelson Bolyard (seldom reads bugmail)
	Comment 54 • 17 years ago

Johnathan, After examining your "updated grid of visuals", I cannot
tell which (if any) of those visuals are the ones intended to be 
shown for self-signed certs.  I would hope that the "elaboration 
popup" for such certs would be the "unknown site" popup.   Yes?

	Steffen Schulz
	Comment 55 • 17 years ago

Some comments to what I've seen above. If there is more
discussion/documentation about this somewhere, please provide me with a pointer.
Otherwise, I'd really suggest to do a more generic discussion about how these
interfaces impact the users reaction and what can be done to further support
the user to make the correct decision.

- Why is the lock not used anymore? Is the semantic of the yellow star very
different from what the user thought the lock means?

- Verified by XRamp Security Services. Even I don't know this company. How does
this String or Name provide any clue to the user?

My opinion: I don't need my browser to tell me that some site is secure. I need
it to tell me when it is not secure. The list of trusted roots must be handled
very restrictive. Additional sites should gain trust over a period of time or
explicit user interaction. There is a "site continuity management" addon but I
think they do not include certificate information yet. And of course it should
be possible to reduce the scope(domains) where I as a user trust some CA that I
imported. The user interface should be obvious and non-intrusive, like a
traffic light red/yellow/green next to the lock.

	Eddy Nigg (StartCom)
	Comment 56 • 17 years ago

Some discussions have taken place on the dev.security and dev.security.crypto mailing list. Johnathan also created this page: http://wiki.mozilla.org/Security:SSLErrorPages
It's his baby after all ;-) , but now can't find his user page anymore, where some more information might be.

	Mike Connor [:mconnor]
	Comment 57 • 17 years ago

Comment on attachment 278106 [details] [diff] [review]
Complete patch (add EV flag checking, clean up icons)

>? obj-i386-apple-darwin8.10.1
>? browser/themes/pinstripe/browser/identity.png
>? browser/themes/winstripe/browser/identity.png
>Index: browser/base/content/browser.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/base/content/browser.js,v
>@@ -3565,16 +3565,19 @@ nsBrowserStatusHandler.prototype =

>+    // Update identity indicator
>+    getIdentityHandler().checkIdentity(aWebProgress, aRequest, aLocationURI);

er, this seems wrong.  aLocationURI isn't likely to be a PRUint.... :)


>+  /**
>+   * Handler for mouseclicks on the "Tell me more about this website" link text
>+   * in the speech bubble
>+   */

speech bubble?

>+  /**
>+   * Determine the identity of the page being displayed by examining its SSL cert
>+   * (if available) and, if necessary, update the UI to reflect this.  Intended to
>+   * be called by an nsIWebProgressListener.
>+   *
>+   * @param nsIWebProgress webProgress
>+   * @param nsIRequest request
>+   * @param PRUnit32 state

PRUint32 :)

>+   */
>+  checkIdentity : function(webProgress, request, state) {
>+
>+    if (state & Components.interfaces.nsIWebProgressListener.STATE_IDENTITY_EV_TOPLEVEL) {
>+      window.setTimeout(function() {
>+        gIdentityHandler.setMode(gIdentityHandler.IDENTITY_MODE_IDENTIFIED)
>+      }, 0);
>+    }
>+    else if (state & Components.interfaces.nsIWebProgressListener.STATE_SECURE_HIGH) {
>+      window.setTimeout(function() {
>+        gIdentityHandler.setMode(gIdentityHandler.IDENTITY_MODE_DOMAIN_VERIFIED)
>+      }, 0);
>+    }
>+    else {
>+      window.setTimeout(function() {
>+        gIdentityHandler.setMode(gIdentityHandler.IDENTITY_MODE_UNKNOWN)
>+      }, 0);
>+    }
>+  },

why are we using timeouts here?


>+  /**
>+   * Set up the title and content messages for the identity message bubble, based
>+   * on the specified mode.
>+   *
>+   * @param newMode The newly set identity mode.  Should be one of the IDENTITY_MODE_* constants.
>+   */
>+  setMessages : function(newMode) {
>+      
>+    var stringBundle = document.getElementById("bundle_browser");
>+    var brandShortName = document.getElementById("bundle_brand").getString("brandShortName");
>+  
>+    // Strings to set on appropriate elements
>+    var title = "";
>+    var body = "";
>+    var supplemental = "";
>+    var verifier = "";
>+    var tooltip = "";
>+    var icon_label = "";

pretty sure you can just define these in the first if statement (JS global scope ftw)

 
>+      // Push the appropriate strings out to the UI
>+      document.getElementById("identity-popup-title").value = title;
>+      document.getElementById("identity-popup-content").textContent = body;
>+      document.getElementById("identity-popup-content-supplemental")
>+              .textContent = supplemental;
>+      document.getElementById("identity-popup-content-verifier")
>+              .textContent = verifier;
>+      document.getElementById("identity-box").tooltipText = tooltip;
>+      document.getElementById("identity-icon-label").value = icon_label;
>+      document.getElementById("identity-popup-encryption-label")
>+              .textContent = encryption_label;

this makes me wish there was a less ugly way of doing this, but there probably isn't

>+var gIdentityHandler; 
>+
>+/**
>+ * Returns the singleton instance of the identity handler class.  Should always be
>+ * used instead of referencing the global variable directly or creating new instances
>+ */

_gIdentityHandler maybe?



>Index: browser/base/content/urlbarBindings.xml
>===================================================================
>RCS file: /cvsroot/mozilla/browser/base/content/urlbarBindings.xml,v
>retrieving revision 1.16
>diff -u -8 -p -r1.16 urlbarBindings.xml
>--- browser/base/content/urlbarBindings.xml	4 Aug 2007 08:35:30 -0000	1.16
>+++ browser/base/content/urlbarBindings.xml	24 Aug 2007 20:04:04 -0000
>@@ -41,17 +41,17 @@
> <bindings id="urlbarBindings" xmlns="http://www.mozilla.org/xbl"
>           xmlns:html="http://www.w3.org/1999/xhtml"
>           xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>           xmlns:xbl="http://www.mozilla.org/xbl">
> 
>   <binding id="urlbar" extends="chrome://global/content/bindings/autocomplete.xml#autocomplete">
>     <content sizetopopup="pref">
>       <xul:hbox class="autocomplete-textbox-container" flex="1">
>-        <children includes="image|deck|stack">
>+        <children includes="image|deck|stack|box">

this might break stuff, luckily we're pre-beta so we'll find out early...

using box instead of hbox/vbox should help...

>+# Identity information
>+identity.domainverified.title=Location Verified
>+identity.domainverified.body=You are currently visiting:
>+identity.domainverified.supplemental=This site does not supply information identifying its owner.

might be worth considering Nelson's comments about verified/trustable information identifying its owner.

overall, looks good, after a long strange trip.  r=me with comments addressed appropriately.

Hopefully we'll get the NSS hookups soon to make this dance right.

	Johnathan Nightingale [:johnath] Assignee
	Comment 58 • 17 years ago

(In reply to comment #55)
> Some comments to what I've seen above. If there is more
> discussion/documentation about this somewhere, please provide me with a
> pointer.

There has been discussion in the dev.apps.firefox newsgroup about some of these changes, and Eddy has linked to the related discussion of SSL error pages above.

> Otherwise, I'd really suggest to do a more generic discussion about how these
> interfaces impact the users reaction and what can be done to further support
> the user to make the correct decision.
> 
> - Why is the lock not used anymore? Is the semantic of the yellow star very
> different from what the user thought the lock means?

The yellow star is unrelated, actually, it is a bookmarking feature being introduced in Firefox 3.

Moving the padlock to secondary chrome (you'll notice it's still present in the popup) is a deliberate attempt to, as you say, help users make better decisions.  The padlock was an avatar of SSL-is-present, which is not a particularly helpful signal to send users - it's certainly not identical to "the site is secure, and trustworthy, and is the one you think you're dealing with."  Changing the discussion to focus on identity is an attempt to at least focus on this last step - are you verifiably dealing with the person you think you are?  If you want to know about the presence of encryption, it is still indicated in secondary displays, but the identity information is arguably far more valuable.  

For academic reading on, e.g., the weakness of the padlock as a security indicator, I recommend several of the articles here:

http://www.w3.org/2006/WSC/wiki/SharedBookmarks

For reading more specifically on the journey to this point in Firefox UI, you can see my own posts here:

http://blog.johnath.com/index.php/2007/03/13/revisiting-security-ui-part-1-of-2/
http://blog.johnath.com/index.php/2007/03/21/revisiting-security-ui-part-2/

> - Verified by XRamp Security Services. Even I don't know this company. How does
> this String or Name provide any clue to the user?

The verification, as I'm sure you know, has always been provided by third parties, not by us.  We make the calls on which trust anchors to include by default in the product, but the CAs don't get a free ride here - they are the ones verifying and signing their name to this information.  The only way users are ever GOING to be able to make informed decisions about which trust providers they like is if that information starts being visible somewhere other than the certificate viewer dialog.

> My opinion: I don't need my browser to tell me that some site is secure. I need
> it to tell me when it is not secure. The list of trusted roots must be handled
> very restrictive. Additional sites should gain trust over a period of time or
> explicit user interaction. There is a "site continuity management" addon but I
> think they do not include certificate information yet. 

I am a fan of things like Garfinkel's KCM notions of how to manage keys, but I think they're outside the scope of what we're doing here.  If you want to see that kind of thing in Firefox in the future, I would recommend starting a newsgroup thread about it, or opening a bug with specific requirements.  Doing that right will require us to remember more about browsing history than we currently do, which is worth discussing, but not in the context of this bug, I think.

> And of course it should
> be possible to reduce the scope(domains) where I as a user trust some CA that I
> imported. The user interface should be obvious and non-intrusive, like a
> traffic light red/yellow/green next to the lock.

See bug 387480 for a version of scoping trust on imported certificates (though not CAs).  Traffic lights are a popular metaphor, but scoping CA trust rules is far, far, too specialized an activity for primary chrome, in my opinion.  Unsophisticated users should not be expected to make those decisions, though I think it might be rich fodder for an extension.

(In reply to comment #54)
> Johnathan, After examining your "updated grid of visuals", I cannot
> tell which (if any) of those visuals are the ones intended to be 
> shown for self-signed certs.  I would hope that the "elaboration 
> popup" for such certs would be the "unknown site" popup.   Yes?

Keep in mind that the treatment of self signed certs will be substantially different in light of bug 327181, requiring explicit trust.  Assuming a user manually overrides an SSL cert and tells us to trust it to identify a given domain, it will be treated like other trusted DV certs.

	Kai Engert (:KaiE:)
	Comment 59 • 17 years ago

(In reply to comment #58)
> > Johnathan, After examining your "updated grid of visuals", I cannot
> > tell which (if any) of those visuals are the ones intended to be 
> > shown for self-signed certs.  I would hope that the "elaboration 
> > popup" for such certs would be the "unknown site" popup.   Yes?
> 
> Keep in mind that the treatment of self signed certs will be substantially
> different in light of bug 327181, requiring explicit trust.  Assuming a user
> manually overrides an SSL cert and tells us to trust it to identify a given
> domain, it will be treated like other trusted DV certs.

In other words:

By default, when attempting to visit a site with a self-signed cert, the cert will be treated as invalid/untrusted, and you'll get an error page.

	Jez
	Comment 60 • 17 years ago

(In reply to comment #59)
> In other words:
> 
> By default, when attempting to visit a site with a self-signed cert, the cert
> will be treated as invalid/untrusted, and you'll get an error page.
> 

Yipes, really?  That's terible behaviour.  I personally *want* to use pages that present self-signed certs; my cPanel and WHM interfaces do just that.  There's no way it should go to an error page.

	Kai Engert (:KaiE:)
	Comment 61 • 17 years ago

(In reply to comment #60)
> (In reply to comment #59)
> > In other words:
> > 
> > By default, when attempting to visit a site with a self-signed cert, the cert
> > will be treated as invalid/untrusted, and you'll get an error page.
> > 
> 
> Yipes, really?  That's terible behaviour.  I personally *want* to use pages
> that present self-signed certs; my cPanel and WHM interfaces do just that. 
> There's no way it should go to an error page.


There will be a way to configure your profile to explicitly allow a self signed cert for a particular server, this is what bug 327181 and bug 387480 are about.

But by default, end users shall be protected from accidentially connecting to a site which uses a certificate with unknown identity.

	Johnathan Nightingale [:johnath] Assignee
	Comment 62 • 17 years ago

(In reply to comment #57)
> (From update of attachment 278106 [details] [diff] [review])
> 
> >+    // Update identity indicator
> >+    getIdentityHandler().checkIdentity(aWebProgress, aRequest, aLocationURI);
> 
> er, this seems wrong.  aLocationURI isn't likely to be a PRUint.... :)

Actually, that shouldn't be there at all - I've removed this call to checkIdentity() since the one in onSecurityChange is the one that matters.

> >+  /**
> >+   * Handler for mouseclicks on the "Tell me more about this website" link text
> >+   * in the speech bubble
> >+   */
> 
> speech bubble?

Fixed

> >+   * @param nsIWebProgress webProgress
> >+   * @param nsIRequest request
> >+   * @param PRUnit32 state
> 
> PRUint32 :)

Fixed

> >+   */
> >+  checkIdentity : function(webProgress, request, state) {
> >+
> >+    if (state & Components.interfaces.nsIWebProgressListener.STATE_IDENTITY_EV_TOPLEVEL) {
> >+      window.setTimeout(function() {
> >+        gIdentityHandler.setMode(gIdentityHandler.IDENTITY_MODE_IDENTIFIED)
> >+      }, 0);
> >+    }
> >+    else if (state & Components.interfaces.nsIWebProgressListener.STATE_SECURE_HIGH) {
> >+      window.setTimeout(function() {
> >+        gIdentityHandler.setMode(gIdentityHandler.IDENTITY_MODE_DOMAIN_VERIFIED)
> >+      }, 0);
> >+    }
> >+    else {
> >+      window.setTimeout(function() {
> >+        gIdentityHandler.setMode(gIdentityHandler.IDENTITY_MODE_UNKNOWN)
> >+      }, 0);
> >+    }
> >+  },
> 
> why are we using timeouts here?

The idea was not to insert ourselves in a blocking way into the onSecurityChange (and originally location change too) codepath, just to queue the string updates up, but that's optimizing-before-the-fact, which I think is a cardinal sin.  Shortened down to the naked setMode calls.

> >+    // Strings to set on appropriate elements
> >+    var title = "";
> >+    var body = "";
> >+    var supplemental = "";
> >+    var verifier = "";
> >+    var tooltip = "";
> >+    var icon_label = "";
> 
> pretty sure you can just define these in the first if statement (JS global
> scope ftw)

I've moved most of the declarations in, there are a few optional strings like "supplemental" which I've left up top to make sure they at least get initialized to empty values in all cases.

> >+var gIdentityHandler; 
> 
> _gIdentityHandler maybe?

Done.

> >   <binding id="urlbar" extends="chrome://global/content/bindings/autocomplete.xml#autocomplete">
> >     <content sizetopopup="pref">
> >       <xul:hbox class="autocomplete-textbox-container" flex="1">
> >-        <children includes="image|deck|stack">
> >+        <children includes="image|deck|stack|box">
> 
> this might break stuff, luckily we're pre-beta so we'll find out early...
> 
> using box instead of hbox/vbox should help...

I'm hoping that since the change is URL-bar specific, and since it's box, not hbox/vbox, there shouldn't be fallout, yeah.  <fingers crossed>

> >+# Identity information
> >+identity.domainverified.title=Location Verified
> >+identity.domainverified.body=You are currently visiting:
> >+identity.domainverified.supplemental=This site does not supply information identifying its owner.
> 
> might be worth considering Nelson's comments about verified/trustable
> information identifying its owner.

I've used a variant of Nelson's language for this string.  I replaced "verified" with "validated" since they are close enough for government work, and it avoids us saying "verified" 28 times in a 32 word popup.  :)

This patch also fixes a regression the previous version would have introduced around favicon drag/drop.  Because the favicon was being wrapped in a box with a popup= attribute, the popup handling behaviour was eating the mousedown, preventing drag events from hitting the favicon.  Instead Neil D suggested removing the explicit popup= attr, and just using an onmouseup handler instead.  I've included a comment to explain the weird construction.

	Eddy Nigg (StartCom)
	Comment 63 • 17 years ago

Johnathan, if the string identity.domainverified.supplemental remained "This site does not supply information identifying its owner." than this is a problem. 
Please see alternative suggestion by Nelson on https://bugzilla.mozilla.org/show_bug.cgi?id=383183#c42

	Johnathan Nightingale [:johnath] Assignee
	Comment 64 • 17 years ago

(In reply to comment #63)
> Johnathan, if the string identity.domainverified.supplemental remained "This
> site does not supply information identifying its owner." than this is a
> problem. 

Eddy, as mentioned in comment 62, I have used a variant of Nelson's string.  The patch to the properties file now contains:

+identity.domainverified.supplemental=Information identifying the owner of this site may not have been validated.

	Eddy Nigg (StartCom)
	Comment 65 • 17 years ago

Excellent! Thanks!

	Dão Gottwald [:dao]
	Comment 66 • 17 years ago

(In reply to comment #62)
> Created an attachment (id=278604) [details]
> [..] drag and drop fixed

Won't your hack make the panel inaccessible without a mouse?

	Johnathan Nightingale [:johnath] Assignee
	Comment 67 • 17 years ago

It actually wasn't (unless there's popup= magic I don't know about) accessible before anyhow.  The information in the popup is a subset of what's in the Page Info security tab anyhow.  It's there to give people a less-intrusive check-at-a-glance, but Accel-I already exists (ditto the Page Info menu item) for accessibility of this information. 

	Jesse Ruderman
	Comment 68 • 17 years ago

I think "Information identifying the owner of this site may not have been validated" is too weak (weasel-wordy) and confusing.  I'd prefer "Firefox is unable to validate information about who owns this site".  Split it into multiple messages covering the different reasons if necessary.

	Nelson Bolyard (seldom reads bugmail)
	Comment 69 • 17 years ago

I'm all for avoiding weasel words, but I disagree with the particular 
suggested words in comment 68.  The problem is not an inability or failure
of FireFox, and I think we want to avoid saying something that sounds like
it is.  FireFox doesn't validate subject name info.  Cert issuers do that.
The fault (if any) lies with the cert issuer.  FireFox's dilemma is that 
not all CAs adequately validate the information in the name; some do, 
some don't.  (EV is an attempt to correct that.)  I think we want to say 
something that makes it clear that the (potential) problem is with 
the information received, not with FireFox.

Maybe "Firefox has received insufficient information" or 
"FireFox cannot determine identity with certainty from the information received"
or words to that effect. 

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 70 • 17 years ago

Comment on attachment 278604 [details] [diff] [review]
Review comments addressed, drag and drop fixed

>Index: browser/base/content/browser.js

>+IdentityHandler.prototype = {

>+  checkIdentity : function(webProgress, request, state) {
>+
>+    if (state & Components.interfaces.nsIWebProgressListener.STATE_IDENTITY_EV_TOPLEVEL)
>+      _gIdentityHander.setMode(_gIdentityHander.IDENTITY_MODE_IDENTIFIED);
>+    else if (state & Components.interfaces.nsIWebProgressListener.STATE_SECURE_HIGH)
>+      _gIdentityHander.setMode(_gIdentityHander.IDENTITY_MODE_DOMAIN_VERIFIED);
>+    else
>+      _gIdentityHander.setMode(_gIdentityHander.IDENTITY_MODE_UNKNOWN);
>+  },

s/_gIdentityHander/this/ in this function and anywhere else in the prototype.

>+  setMessages : function(newMode) {

>+    // Initialize the optional strings to empty values

I prefer the declarations where they were in your first patch, but \/\/!

>+        if (subjectNameFields.C)
>+          icon_label = stringBundle.getFormattedString("identity.identified.title_with_country",
>+                                                       [body, subjectNameFields.C]);
>+          else
>+            icon_label = body;
>+        }

Looks like the indentation is wrong here, and in the rest of the lines after following this in the function.

>+var _gIdentityHander; 

browser.js' style for globals is just "g", not "_g", so I'd leave this like it was.

>Index: browser/base/content/browser.xul

The potential Ts/Txul effects of adding extra elements here in "identity-popup" make me a bit nervous (especially given the results of bug 391385), but as we discussed there isn't much that can be done about that. We'll have to keep a close eye on perf tests when this lands.

>-                <image id="lock-icon" onclick="if (event.button == 0) displaySecurityInfo(); event.stopPropagation();"/>

If you remove this you'll need to remove all the code/style rules that deal with it, as well. I think just removing all of the /browser hits in http://lxr.mozilla.org/seamonkey/search?string=lock-icon should be sufficient - although perhaps the antiphishing code that previously hid the lock icon wants to do something similar to the Larry popup.

>Index: browser/themes/pinstripe/browser/browser.css
>Index: browser/themes/winstripe/browser/browser.css

It looks like there are a few rules in here that might not by optimal; use of the descendant selector in particular is known to be expensive (you've probably seen this document already, but in case you haven't: http://developer.mozilla.org/en/docs/Writing_Efficient_CSS ). It is challenging to write compact and efficient CSS when styles need to dynamically change across a large part of the DOM, though. Maybe ask Ryan to take a look at these and see if he can suggest any optimizations?

r+ with these addressed, but I'd like to take a quick look at the updated patch.

	Johnathan Nightingale [:johnath] Assignee
	Comment 71 • 17 years ago

This patch fixes Gavin's nits, removes the other lock-icon code in browser/, and cleans up the CSS a lot, based on conversation with Gavin and bsmedberg.  There are no more descendant selectors, adding the identity mode class to one extra element allowed me to style most of the UI using simple child selectors.

Gavin wanted one final looksee, and there's no "final looksee" flag, so it's back into his review queue.  Sorry G!

	Johnathan Nightingale [:johnath] Assignee
	Comment 72 • 17 years ago

Marking checkin-needed.  Please remember to land the new identity.png image in both:

browser/themes/pinstripe/browser/

and

browser/themes/winstripe/browser/

when landing the patch.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 73 • 17 years ago

I checked this in a little while ago, but had to back it out due to performance regressions (Ts/Txul) and unit test failures (test for bug 344861). The unit test failures should be easy to fix, but I don't think there's much we'll be able to do about perf - adding chrome seems to cost us perf no matter how we do it. Will investigate further tomorrow.

	Bill Gianopoulos [:WG9s]
	Comment 74 • 17 years ago

I noticed 2 issues right away with this from using a Linux build from the short time it was checked-in.

1.  The identity part of the URLbar goes to far to the left and down obliterating the borders.

2.   If you right click on that area, it both opens the customize toolbar menu as well as the identity pop-up.  It should probably do neither.

	Alfred Kayser
	Comment 75 • 17 years ago

Note, there are still a number of inefficient style rules in this patch:
.unknownIdentity > #identity-icon-label {
.verifiedIdentity > #identity-popup-content,
.verifiedDomain > #identity-popup-content {

.verifiedDomain > #identity-popup-container > #identity-popup-icon {
.verifiedIdentity > #identity-popup-container > #identity-popup-icon {

.verifiedIdentity > #identity-popup-title {
.unknownIdentity > #identity-popup-title {
.verifiedDomain > #identity-popup-title {

.verifiedIdentity > #identity-popup-encryption > * > #identity-popup-encryption-icon,
.verifiedDomain > #identity-popup-encryption > * >#identity-popup-encryption-icon 


According to 'Question all usages of the child selector!' in the 'Writing Efficient CSS' (http://developer.mozilla.org/en/docs/Writing_Efficient_CSS), this all needs to be replaced by using RDF attributes passed to childs, like so:
#identity-icon-label[verified="unknown"] {
#identity-popup-content[verified="identity"],
#identity-popup-content[verified="identity"] {

#identity-popup-icon[verified="domain"] {
#identity-popup-icon[verified="identity"] {

#identity-popup-title[verified="identity"] {
#identity-popup-title[verified="unknown"] {
#identity-popup-title[verified="domain"] {

#identity-popup-encryption-icon[verified="identity"],
#identity-popup-encryption-icon[verified="domain"] 

It is much cheaper to use attributes like this.

	Johnathan Nightingale [:johnath] Assignee
	Comment 76 • 17 years ago

This version passes mochitest (including the failed test in windows/linux in the last patch) and should fix a couple little niggles spotted during the 28 seconds when it was on trunk yesterday.  :)

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 77 • 17 years ago

Looking at the performance numbers on tinderbox a bit closer:
http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox&maxdate=1188531845&legend=0&norules=1

It looks to me like the Linux perf machine was the only one to show a regression. The numbers for the Mac and Windows machines look to be within the normal variance. This could just mean that our Windows and Mac numbers aren't fine-grained enough to detect a regression of this size.

Linux numbers:
Txul: ~303 while patch was in, ~296 while out = ~2.4%
Ts: ~1026 while patch was in, ~1015 while out = ~1%
Tp2: ~200 while patch was in, ~197 while out = ~1.5%
Tp: no visible change
Tdhtml: no visible change

I'm not sure I believe the Tp2 number - I guess we'll have to watch carefully to see whether it regresses again when this relands. It also looks like Txul dropped back down to normal levels before the patch was backed out, but that may just be timing issues related to perf test machines.

I don't think there's much that can be done about the performance regression, given that we're adding chrome. It's well known that adding primary chrome always impacts perf tests negatively. Assuming that we absolutely do want this functionality, I think we'll have to accept the slight perf hit. We can of course look at suggestions for improving the CSS, but I doubt that can make a significant difference in terms of perf impact.

	Robert Sayre
	Comment 78 • 17 years ago

(In reply to comment #77)
> 
> I don't think there's much that can be done about the performance regression,
> given that we're adding chrome. It's well known that adding primary chrome
> always impacts perf tests negatively. Assuming that we absolutely do want this
> functionality, I think we'll have to accept the slight perf hit. 

You could delete chrome elsewhere. This would also keep the overall complexity of the interface from growing.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 79 • 17 years ago

I filed bug 394887 on the idea Neil had for potentially reducing the perf cost of adding the Larry popup.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 80 • 17 years ago

Perf results from the second landing, last night:
                      Before  After   Change
bl-bldlnx03     Ts    1013    1024    +1.09%
                Txul  285     293     +2.81%
                Tp    270     278     +2.96%
                Tp2   198     202     +2.02%

bl-bldxp01      Txul  574     596     +3.83%

I wasn't able to see any clear regressions on any of the other tests/machines.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 81 • 17 years ago

To address the Tp regression, it seems like we could cache the SSLStatus, security state, and host (if needed) on the IdentityHandler, and only call set setMode() from checkIdentity() if any of those change. Given that onSecurityChange is fired for each load associated with a given pageload (e.g. multiple times per page) but rarely gets status changes, this would reduce the number of calls to setMode() substantially.

I also noticed something I missed in the initial review:
  gBrowser.contentWindow.location.host
would be more efficiently retrieved using:
  gBrowser.currentURI.host

	Robert Sayre
	Comment 82 • 17 years ago

(In reply to comment #80)
> 
> I wasn't able to see any clear regressions on any of the other tests/machines.

qm-pxp01 Tp clearly spiked as well, but in a range that could have been unrelated. We'll need to watch that when this lands again. Tp2 on the xserve is too noisy to see a 2% regression.

	Johnathan Nightingale [:johnath] Assignee
	Comment 83 • 17 years ago

Based on conversations with gavin & mconnor, this version has a couple perf-related modifications:

- The panel is display:hidden by default.  This attribute is cleared on the first mouseup event in the identity box (i.e. the first time the popup is requested).  The hope is that this might mitigate Ts/Txul since we won't even create a frame for it initially.

- setMessages() has been refactored into setIdentityString() and setPopupMessages().  Only setIdentityString (which sets the primary chrome string and tooltip) is in the onSecurityChange path now.  The strings in the popup are all set when requested onmouseup.  This should lighten the Tp load since less work needs to be done, and fewer change events will occur.  It's also cleaner than "cache the cert" approaches to string-change-reduction, which open up potentially-messy synchronization issues.

- replaced gBrowser.contentWindow.location.host with gBrowser.currentURI.host as suggested

	Johnathan Nightingale [:johnath] Assignee
	Comment 84 • 17 years ago

Attachment 279682 [details] [diff] would still make several unnecessary calls to setIdentityString(), albeit shorter ones.  This patch checks whether SSLStatus has changed before starting down the setMode() code path.  This has benefits even when the status has changed, since we now only obtain SSLStatus once, and since this version reduces code duplication in setIdentityString() and setPopupMessages()

	Dão Gottwald [:dao]
	Comment 85 • 17 years ago

You might want to use hidden="true" / .hidden = false; instead of inline CSS.

	Dão Gottwald [:dao]
	Comment 86 • 17 years ago

Also, I've just seen in an extension that toggling hidden or style.display right before the panel is opened causes the panel to not show up. On the second try, as it wasn't hidden anymore, it worked.
Maybe you're lucky and it doesn't happen here. Otherwise, my workaround was to set hidden on the panel's first child. (Which might not help performance at all, as I could imagine.)

	Johnathan Nightingale [:johnath] Assignee
	Comment 87 • 17 years ago

After more discussion with Gavin, we think that the caching and hiding-on-start are likely to be the important wins here.  Splitting the message setting up creates some doubled-up code with little expected additional gain, so this patch recombines the two string setters into one.

	Johnathan Nightingale [:johnath] Assignee
	Comment 88 • 17 years ago

(In reply to comment #85)
> You might want to use hidden="true" / .hidden = false; instead of inline CSS.

Yeah, the one ends up just binding to the other in the end, but when in XUL we should stick to XUL.

(In reply to comment #86)
> Also, I've just seen in an extension that toggling hidden or style.display
> right before the panel is opened causes the panel to not show up. On the second
> try, as it wasn't hidden anymore, it worked.

Do you know if there's a bug filed on that?

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 89 • 17 years ago

Fixes a problem with the caching: non-SSL pages have a null SSLStatus, which means they're "the same" as each other and the default state, so the UI wasn't being updated for initial HTTP loads, or http->http loads. Fix this by caching the URI as well.

	Dão Gottwald [:dao]
	Comment 90 • 17 years ago

(In reply to comment #88)
> (In reply to comment #86)
> > Also, I've just seen in an extension that toggling hidden or style.display
> > right before the panel is opened causes the panel to not show up. On the second
> > try, as it wasn't hidden anymore, it worked.
> 
> Do you know if there's a bug filed on that?

I doubt it, but I didn't look for one.

	Johnathan Nightingale [:johnath] Assignee
	Comment 91 • 17 years ago

Opened bug 395062 to track the hidden-first-time popup issue mentioned by Dao 

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 92 • 17 years ago

Did this regress tp/tp2 (bl-bldlnx03).

	u279076
	Comment 93 • 17 years ago

I realize this is a work in progress, I just have one little suggestion.  There is a separator between the secure site name and the actual URL now, which is a good idea.  This makes for better readability.  However, this separator is a little annoying to the eye when you are not on a secure site and only the favicon displays.  Is there any way that we could make this feature intuitive enough so that if only the favicon is being displayed, the separator is not displayed?

	Tony Mechelynck [:tonymec]
	Comment 94 • 17 years ago

the click feature could use some "discoverability" IMHO.

	Jesper Kristensen
	Comment 95 • 17 years ago

Comments on: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007090604 Minefield/3.0a8pre

- The favicon is visually bound to the identity information. It looks like the favicon is a piece of verified information. This is unfortunate. I like the proposal in attachment 271668 [details] better than the current design.

- The DV popup is confusing. First it says "verified", then it says "not validated", and finally it says "verified by". I only figured out after reading this bug, that it was talking about two different things (location and owner). You may want to make that more clear.

- There is a lot of information in the popup. That is a little confusing. Is the encryption paragraph really some valuable information for the user, or is it just a technical detail, that clutters up the popup?

- There are many background colors, and it may not be clear what they mean. Currently there is:
EV: green|yellow
DV: white|yellow
noV: white|white
What about only coloring the identity UI, like this:
EV: green|white
DV: yellow|white
noV: white|white

- The places star sits in nearly the same place as the padlock icon did. The star might be confused with security/identity.

	Dão Gottwald [:dao]
	Comment 96 • 17 years ago

(In reply to comment #95)
> Comments on: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre)
> Gecko/2007090604 Minefield/3.0a8pre
> 
> - The favicon is visually bound to the identity information. It looks like the
> favicon is a piece of verified information. This is unfortunate.

True. Moreover, going with the favicon instead of a dedicated indicator icon leaves this problem (originally brought up by Johnathan) unfixed: "Users are expected to notice the absence of an indicator."

If you want to reduce the clutter, then reconsider removing the favicon. It's currently overloaded multiple times, whereas its features are either duplicated (drag source, selecting the address) or badly assigned (identity UI).

	Jesper Kristensen
	Comment 97 • 17 years ago

With the identity indicator in the location bar, I think the indicator in the status bar becomes unnecessary, since it just shows a subset of the information shown in the new UI.

	Steve England [:stevee]
	Comment 98 • 17 years ago

(In reply to comment #97)
> With the identity indicator in the location bar, I think the indicator in the
> status bar becomes unnecessary, since it just shows a subset of the information
> shown in the new UI.

Yes. It also says Authenticated By where as Larry says Verified By - so if it stays it needs to be changed for consistency's sake.

	Jesper Kristensen
	Comment 99 • 17 years ago

For bugzilla, maybe the location shown should be "*.mozilla.org" and not "bugzilla.mozilla.org", since "*.mozilla.org" is the information in the certificate.

Does anyone know a website out there with an EV certificate?

I would like to explain my fourth point in comment 95:
The yellow background has the same meaning as the padlock. Its functionality is to make it more visible when the padlock comes and goes. Now that the padlock is removed, the yellow background should also be removed. But then there is no color separating noV (no SSL) and DV (SSL, no EV). To make up for that, Larry could get a yellow background for DV sites.

There should be a hover effect to hint that something will happen if you click.

	Nelson Bolyard (seldom reads bugmail)
	Comment 100 • 17 years ago

In reply to comment 99,
> Does anyone know a website out there with an EV certificate?
https://www.paypal.com/

Regarding use of color backgrounds: they're fine but no important information
(including security information) should ever be indicated solely by choice of 
color.

	Robert Kaiser
	Comment 101 • 17 years ago

Right. Color should never be the only indicator of something - remember that we also want people as users who are colorblind other otherwise visually impaired, or who use display settings with very low color depths. In all those cases, color differences might be hard to parse.

	Jesper Kristensen
	Comment 102 • 17 years ago

(In reply to comment #100)
> In reply to comment 99,
> > Does anyone know a website out there with an EV certificate?
> https://www.paypal.com/

I only see "Location Verified" on that page in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007090905 Minefield/3.0a8pre

> Regarding use of color backgrounds: they're fine but no important information
> (including security information) should ever be indicated solely by choice of 
> color.

Exactly, that is why I think the yellow background should be removed, now that the padlock is removed.

	Michael Clackler
	Comment 103 • 17 years ago

> > https://www.paypal.com/
> 
> I only see "Location Verified" on that page in Mozilla/5.0 (Windows; U; Windows
> NT 5.1; en-US; rv:1.9a8pre) Gecko/2007090905 Minefield/3.0a8pre

I also do not see the "Location Verified" status on paypal or on https://www.entrust.net/  Both show a validated state on IE7.  I have not found any site that shows the "Location Verified" state on Minefield.  Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a8pre) Gecko/2007090905 Minefield/3.0a8pre ID:2007090905

	Scott A.
	Comment 104 • 17 years ago

I don't like this new indicator, I think it clutters up the navigation bar too much with identified and verified domains and is not very distinguished from the address bar (could be even more of a problem with different themes). It is an Aesthetic eye sore... Perhaps just use a different color / design of Lock icon for verified/identified/unknown domains.

	Eddy Nigg (StartCom)
	Comment 105 • 17 years ago

The colored address bar should be kept in my opinion in order gain a certain attention and for consistency reasons. Users are used to see the colored address bar and too many changes in one go might be counterproductive. The indicator itself is nice, but this additional URL/Domain name next to the "captain" is ugly, irritating, confusing and superfluous in my opinion. I don't really see the benefit of it, because if the web site doesn't match the one from the certificate, the browser will not connect anyway...

Also when clicking on "Learn more about this site", the indicator stays on top the window which pops up. I expect it to disappear once the window with the certificate details pops up.

	shirish
	Comment 106 • 17 years ago

Same here, I see on paypal "Information identifying the owner of this site may have not been validated" while the location is identified & said to be then "Verified by Verisign" . This is on Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a8pre) Gecko/2007091000 Minefield/3.0a8pre 

 As a user I'm more confused than ever before :(

 Also Larry looks the same at https://www.paypal.com as he looks in https://bugzilla.mozilla.org/show_bug.cgi?id=383183 there is just no difference, shouldn't there be a difference with this so-called EV SSL certificates. 

	Johnathan Nightingale [:johnath] Assignee
	Comment 107 • 17 years ago

A couple notes on recurring themes here:

- "I don't see EV treatment on known EV sites" - true.  This bug tracks the implementation of the UI, which includes treatment for EV sites.  However bug 374336 tracks the required changes to PSM which would allow us to identity EV certificates in the cert DB, and it has not yet closed.  As a result, the notification that the UI looks for on EV sites is never sent, so the sites are treated as vanilla SSL.

- "The UI isn't right" - true.  bug 395248

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 108 • 17 years ago

This was landed for M8:
browser/base/content/browser.js 	1.840
browser/base/content/browser.xul 	1.368
browser/base/content/urlbarBindings.xml 	1.28
browser/components/safebrowsing/content/phishing-afterload-displayer.js 	1.21
browser/locales/en-US/chrome/browser/browser.dtd 	1.68
browser/locales/en-US/chrome/browser/browser.properties 	1.42
browser/themes/pinstripe/browser/browser.css 	1.74
browser/themes/pinstripe/browser/jar.mn 	1.52
browser/themes/winstripe/browser/browser.css 	1.89
browser/themes/winstripe/browser/jar.mn 	1.48

The perf numbers from the landing:
              Before  After
bl-bldlnx03
Tp            268     270
Tp2           195     196.5
Txul          284     288
      
bl-bldxp01
Txul          565     575

The bl-bldxp01 Txul number is the initial jump, it now seems to be back within the normal variance. Ts on bl-bldlnx03 also seemed to peak around the landing, but also seems to have gone back to normal. Overall this looks like a noticeable improvement over the previous numbers across the board.	

	Johnathan Nightingale [:johnath] Assignee
	Comment 109 • 17 years ago

Closing this bug out - the patch has landed and there's a whole host of follow-ups for tracking new stuff.

	Marcia Knous [:marcia]
	Comment 110 • 17 years ago

Litmus Triage Team: Tomcat will handle this Litmus test case.

	Stuart Parmenter
	Comment 111 • 17 years ago

this was backed out due to talos Tp regression (bug 396064)

	Alfred Kayser
	Comment 112 • 17 years ago

A simple way to fix this, is to change the mouseup/panel thing to a 'onclick/dialog' thing (which is then the PageInfo/Security panel?)
See also bug 395314:
As soon one adds a 'close' button, the panel should be a popup/dialogbox, and then why not combine the panel with the Page Info/Security panel instead of having two different forms for the same info?

	Johnathan Nightingale [:johnath] Assignee
	Comment 113 • 17 years ago

(In reply to comment #112)
> A simple way to fix this, is to change the mouseup/panel thing to a
> 'onclick/dialog' thing (which is then the PageInfo/Security panel?)
> See also bug 395314:
> As soon one adds a 'close' button, the panel should be a popup/dialogbox, and
> then why not combine the panel with the Page Info/Security panel instead of
> having two different forms for the same info?

I understand what you're saying, but dialogs are a lot more visually heavyweight, with title bars, resize handles, and window management buttons (besides close).  They also don't auto-dismiss when you click back on the page (modal ones won't let you click away, non modal ones just get themselves pushed down the Z-stack), making them substantially more intrusive.  Being in a dialog also reduces the visual association between the information we're presenting and the location bar, and reduces our ability to extend the panel in the future to have other site related functionality (e.g. Alex Faaborg's favicon menu mockups).  

Moreover, it feels like the Tp hit is unlikely to be improved by this change anyhow - this would be more likely to impact Txul.  The Tp hit is probably as a result of adding code to the onSecurityChange path, though I don't really know how that code can be reduced, since status caching was added between the second and third checkin which reduces our onSecurityChange code to a minimum.

	Dão Gottwald [:dao]
	Comment 114 • 17 years ago

Comment on attachment 279746 [details] [diff] [review]
Final patch #2? Maybe?

(In reply to comment #113)
> Being in a dialog
> also reduces the visual association between the information we're presenting
> and the location bar, and reduces our ability to extend the panel in the future
> to have other site related functionality (e.g. Alex Faaborg's favicon menu
> mockups).

A panel is not a menu; not sure how you want to combine that, let alone that adding unrelated things to that UI doesn't sound like a great idea.

> The Tp hit is probably as a
> result of adding code to the onSecurityChange path, though I don't really know
> how that code can be reduced, since status caching was added between the second
> and third checkin which reduces our onSecurityChange code to a minimum.

The old authentication UI wasn't removed from the status bar, was it?

Why a singleton IdentityHandler class rather than an identityHandler object? Prototyping is detouring in that case.

There are some other things that I noticed while reading the code, although I don't think they are overly performance-relevant:

>+    if (state & Components.interfaces.nsIWebProgressListener.STATE_IDENTITY_EV_TOPLEVEL)
>+      this.setMode(this.IDENTITY_MODE_IDENTIFIED);
>+    else if (state & Components.interfaces.nsIWebProgressListener.STATE_SECURE_HIGH)
>+      this.setMode(this.IDENTITY_MODE_DOMAIN_VERIFIED);

var nsIWebProgressListener = Ci.nsIWebProgressListener;
if (...

>+  setMode : function(newMode) {
>+    document.getElementById("identity-popup").className = newMode;
>+    document.getElementById("identity-box").className = newMode;
>+    document.getElementById("identity-popup-content-box").className = newMode;

I would make each getElementById call only once, and as I've wrote earlier, I would put the mode into an attribute.

>+  setMessages : function(newMode) {
>+      
>+    var stringBundle = document.getElementById("bundle_browser");
...
>+    if (newMode == this.IDENTITY_MODE_DOMAIN_VERIFIED) {
>+      var title = stringBundle.getString("identity.domainverified.title");
>+      var encryption_label = stringBundle.getString("identity.encrypted");
...
>+      supplemental = stringBundle.getString("identity.domainverified.supplemental");
...
>+      title = stringBundle.getString("identity.identified.title");
>+      encryption_label = stringBundle.getString("identity.encrypted");
...
>+      encryption_label = stringBundle.getString("identity.unencrypted");
>+      title = stringBundle.getString("identity.unknown.title");
>+      body = stringBundle.getString("identity.unknown.body");

can be cached

>+    // Push the appropriate strings out to the UI
>+    document.getElementById("identity-popup-title").value = title;
>+    document.getElementById("identity-popup-content").textContent = body;
>+    document.getElementById("identity-popup-content-supplemental")
>+            .textContent = supplemental;
>+    document.getElementById("identity-popup-content-verifier")
>+            .textContent = verifier;
>+    document.getElementById("identity-popup-encryption-label")
>+            .textContent = encryption_label;
>+    document.getElementById("identity-box").tooltipText = tooltip;
>+    document.getElementById("identity-icon-label").value = icon_label;

Maybe it's worth doing this only if panel.state == "open" respectively in a "popupshowing" event handler.

	Kai Engert (:KaiE:)
	Comment 115 • 17 years ago

Johnathan, FYI, the latest patch (that was backed out) still has the problem that I reported in comment 31.

	Johnathan Nightingale [:johnath] Assignee
	Comment 116 • 17 years ago

(In reply to comment #115)
> Johnathan, FYI, the latest patch (that was backed out) still has the problem
> that I reported in comment 31.

Uh, really?  I can't recreate that - is it possible your profile still has the addon version installed?  It was a pretty simple thing to reproduce in the addon days, but I haven't seen it at all in the patch-against-trunk version, and running a straw poll in IRC, haven't found anyone reporting that either.

If it's still happening (and particularly if there are STR) I think we should open a new bug to track that down once larry re-lands.

	Tony Chung [:tchung]
	Comment 117 • 17 years ago

Can you please update the new target milestone?  Thanks.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 118 • 17 years ago

Johnathan and I discussed some additional fixes that he'll be making, attaching this here so he can use it as a base.

	Johnathan Nightingale [:johnath] Assignee
	Comment 119 • 17 years ago

Compared to the patch checked in last time this version:

 - pre-caches elements to avoid redundant getElementById calls
 - pre-caches static strings
 - Moves popup string/style setting to the click handler, so that we aren't setting strings on the usually-hidden popup
 - picks up some minutiae in my local tree (consistency of "web site" vs. "website", click to close, 1px padding adjustment on mac)
 - applies cleanly against trunk

Re-requesting gavin's look-see (and checkin!), at least on the changed portions.

	Dão Gottwald [:dao]
	Comment 120 • 17 years ago

As of bug 394887, the "hidden" hack shouldn't be needed anymore, right?

	Johnathan Nightingale [:johnath] Assignee
	Comment 121 • 17 years ago

(In reply to comment #120)
> As of bug 394887, the "hidden" hack shouldn't be needed anymore, right?

Hiding the panel is still a win, since it means not having to even build a frame, aiui.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 122 • 17 years ago

I made a minor tweak to address page navigation that happens while the popup is open. It now updates the popup from setMode if the popup is open, since it's possible to use shortcuts to move back/forward or from tab to tab.

I also notice that the "click to close" behavior doesn't seem to be working for me, even though it was working for you. Something we can sort out later (in bug 395314).

I'm going to land this version.

	Kai Engert (:KaiE:)
	Comment 123 • 17 years ago

(In reply to comment #116)
> Uh, really?  I can't recreate that - is it possible your profile still has the
> addon version installed?  

No. Fresh profile. Always reproducable. Open site that triggers EV UI, open a new tab, go back to previous tab, EV UI gone.

Maybe it's only happening on Linux.
Maybe it's somehow depending on the patches I have not yet checked in, but I think you are using them.


> If it's still happening (and particularly if there are STR) I think we should
> open a new bug to track that down once larry re-lands.

I will open a bug once I can reproduce this with code checked in to trunk.

	Dave Townsend [:mossop]
	Comment 124 • 17 years ago

This has been backed out to investigate the Tp regression talos is showing

	Dave Townsend [:mossop]
	Comment 125 • 17 years ago

I've reopened the tree as it looks like we are back down to previous Tp levels but do not have time to go back checking in things. This looked like it certainly cause a Tp regression but probably <1%. assuming this is acceptable I think it would be sensible to close the tree when checking this back in and letting talos run for a few cycles so we have an accurate figure.

	Bill Gianopoulos [:WG9s]
	Comment 126 • 17 years ago

I would actually go one further.  Close the tree and let talos run a few cycles without the noise of any other checkins to get an accurate baseline, check this in and leave the tree closed the same number of talos cycles.  The regardless of the regression numbers I think it is important to leave this in and open the Tree.  This will give a better understanding of the perf hit as well as establish a new "post Larry" baseline for detecting performance regressions elsewhere in the codebase.

At this point I think UI feedback is desperately needed, and not much occurs if it is going to be checked out a few hours after each time it is checked in.

Besides, it is possible that the UI feedback might provoke enough of a UI change so that the current perf numbers are irrelevant.  It can always be decided later after we get adequate UI feedback to pull this for performance reasons.

Just my $0.02.

	pal-moz
	Comment 128 • 17 years ago

(In reply to comment #123)
> (In reply to comment #116)
> > Uh, really?  I can't recreate that - is it possible your profile still has the
> > addon version installed?  
> 
> No. Fresh profile. Always reproducable. Open site that triggers EV UI, open a
> new tab, go back to previous tab, EV UI gone.
> 
> Maybe it's only happening on Linux.
> Maybe it's somehow depending on the patches I have not yet checked in, but I
> think you are using them.
> 
> 
> > If it's still happening (and particularly if there are STR) I think we should
> > open a new bug to track that down once larry re-lands.
> 
> I will open a bug once I can reproduce this with code checked in to trunk.
> 

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007092621 Minefield/3.0a9pre ID:2007092621

same here (EV UI is gone), build with new check-in.
see http://img294.imageshack.us/img294/834/evuied7.jpg
new profile with NTT.
XP SP2

	Robert Sayre
	Comment 129 • 17 years ago

Comment on attachment 282179 [details] [diff] [review]
one additional fix

DTrace shows that some of the Ts hit from this patch is a tripling of prepareForStartup's cost.

prepareForStartup() 
  new nsBrowserStatusHandler()
    nsBrowserStatusHandler.init()
      nsBrowserStatusHandler.onSecurityChange()
        getIdentityHandler()

	juan becerra [:juanb]
	Comment 130 • 17 years ago

Litmus triage team: juanb will add test case in Litmus

	Nelson Bolyard (seldom reads bugmail)
	Comment 131 • 17 years ago

Johnathan, I think we need an EV umbrella bug.  We need a bug that is blocked
by all the other bugs that must be resolved before EV works.  It appears 
that THIS bug has more-or-less served that purpose in the past, but now it
blocks other bugs that seem to themselves be blockers of a true EV umbrella
bug.  DO you want to continue to try to use this bug as the EV umbrella bug?
Or shall we create a new EV umbrella bug?

Right now, I see one major missing component for EV.  The new EV root certs 
are missing.  I have filed bug 398944, an umbrella bug for all the new EV 
root CAs that need to be added, and that bug should block the overall EV 
umbrella bug.  In the meantime, I have marked bug 398944 as blocking this 
bug.  When an EV umbrella bug is created, bug 398944 should be changed to 
block that bug.

	Eddy Nigg (StartCom)
	Comment 132 • 17 years ago

I posted under Bug 398944 comment 6 that the current Mozilla CA policy
doesn't mention EV. Various parts such as section 7 have to be expanded
and defined clearly before EV certificates shall be treated any different then
section 7 suggests. What are the accepted minimum requirements to have an EV
certificate marked as such in the browser? What is the criteria to have a CA
root marked as EV worthy? There might be other sections of the Mozilla policy
which might require adjustments as well. So far anything related to this and interrelated changes to the UI and NSS library might be not in accordance of the Mozilla CA policy. Obviously this is not about cosmetics, but about defining a binding and relevant policy in relation to EV.

	Eddy Nigg (StartCom)
	Comment 133 • 17 years ago

Sections 8 and 14 also requires an update IMO. Perhaps an additional policy dedicated to EV might be appropriate, something like the "Mozilla EV extension policy" or similar.

	Johnathan Nightingale [:johnath] Assignee
	Comment 134 • 17 years ago

After the recent work on cleaning up onSecurityChange performance, the patches here no longer applied cleanly.  Moreover, mconnor's most recent backout of Larry during his perf experimentation left some pieces behind in browser.js.

This patch applies cleanly, and removes the caching work that was done in the last couple iterations, since that has moved up to all of onSecurityChange and hence Larry gets it for free.

	Asaf Romano (gone)
	Comment 135 • 17 years ago

FYI: For the places panel, Ts went down after we've moved the panel contents into an overlay and loaded that dynamically.

	Asaf Romano (gone)
	Comment 136 • 17 years ago

(Ts and Txul)

	Mike Connor [:mconnor]
	Comment 137 • 17 years ago

landed again, let's see what the current state of the art is...

	Alfred Kayser
	Comment 138 • 17 years ago

The following rules in the .css are not very performance friendly:
.verifiedIdentity > #identity-popup-encryption > * > #identity-popup-encryption-icon,
.verifiedDomain > #identity-popup-encryption > * >#identity-popup-encryption-icon {

Instead of passing the state like through CSS child selectors, why not make an attribute 'verified' (or something like that) and let the XUL inherit it to the right elements. So that each element can do:
#identity-popup-encryption-icon[verified="identity"]{...}

	Mike Connor [:mconnor]
	Comment 139 • 17 years ago

Alfred, can you file a followup on Johnathan to get that cleaned up?  It shouldn't affect anything except the popup itself, but it'd be good to look into.

Resolving fixed, perf numbers all seem inside of the normal ranges, definitely talos Tp, which is the biggest worry.

	Reed Loden [:reed]
	Comment 140 • 17 years ago

(In reply to comment #139)
> Resolving fixed, perf numbers all seem inside of the normal ranges, definitely
> talos Tp, which is the biggest worry.

Note that Larry regressed both Txul and Ts on Linux and Windows and regressed Ts on Mac (Txul change on Mac is too small to say for sure).

	Stephen Donner [:stephend] Not actively reading bugmail
	Comment 141 • 17 years ago

I'm verifying that the presence and functionality of Larry is included in all trunk builds on Mac OS X, Windows, and Linux platforms, using today's 2007-10-25 builds; verified FIXED for that (there are plenty of spin-off bugs that will make this both feature complete and more performant).  Leaving the Txul and Ts metric contentedness to the app team to evaluate/approve.

	Eddy Nigg (StartCom)
	Comment 142 • 16 years ago

The identity indicator says today:

"You are connected to mozilla.org

which is run by
(unknown)

Verified by:..."

When clicking on "More Information" the certificate viewer screen says:

"Website: bugzilla.mozilla.org
Owner: This site does not supply identity information..."

Both statements above are simply incorrect! The site is run by the "Mozilla Corporation" and the owner of the site is Mozilla. I expect that Firefox doesn't make wrong statements, instead omit the non relevant bits altogether, if Firefox doesn't want to show it (because of EV).

Instead the identity indicator should show:

"You are connected to mozilla.org

Verified by:..."

and omit the Owner section if it's not relevant. Currently it looks like Firefox is lying to me. When encountering an EV certificate, the indicator can show as it does now.

	Nelson Bolyard (seldom reads bugmail)
	Comment 143 • 16 years ago

In reply to comment 142,
sounds like a FF3 bug.  I suggest that a bug be filed about that.

SM trunk reports:

> This web site is owned by Mozilla Corporation
>
> This has been verified by Equifax

	Marcia Knous [:marcia]
	Comment 144 • 16 years ago

Adding in-litmus+ to this bug. stephend has written a number of larry test cases which cover all the possible scenarios, so I think we are covered here.

	Thomas Nardone
	Comment 145 • 16 years ago

This feature sucks.  I used to click on the site icon to highlight the URL, since manually double clicking or dragging the URL text never works properly.  Now I get a stupid popup.  There should be an option to disable this feature.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 146 • 16 years ago

You can just click the URL bar directly to select it all (unless you're on Linux, in which case you need to first change browser.urlbar.clickSelectsAll to "true" using about:config).

	Eddy Nigg (StartCom)
	Comment 147 • 16 years ago

The address bar lost its yellow color when on https connection on Linux. I don't know if this again one of your tricks Johnathan, but that makes it very hard to distinguish between plain and SSL.

	Jesse Ruderman
	Comment 148 • 16 years ago

Eddy, see bug 417844.

	Eddy Nigg (StartCom)
	Comment 149 • 16 years ago

Thanks, please see comment on bug 417844#c110 and attachment 321422 [details].