Remember that we've white listed a certificate when we switch to a new tab....

Remember that we've white listed a certificate when we switch to a new tab.

R=wtc
BUG=6456
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=9747

[abarth-chromium, 2009-02-12 08:47:53]

I have no idea if I edited the non-Window build files correctly.

[wtc, 2009-02-12 18:32:56]

LGTM.  Nice work!  And thanks for your help.

Please review my comments below before you check this in.
In particular, the issue on whether the OTR profile should
share the SSL host state with the main profile, and the
thread safety of the lazy creation of SSLHostState.

Re: adding the new files the the Mac Xcode project file:
I use src/tools/xcodebodge/xcodebodge.py to do that.  I
forwarded an email from Alex Harper (the author of that
script) to you.  Also, the try server will tell you if you
added the files correctly.

http://codereview.chromium.org/20296/diff/1/9
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/20296/diff/1/9#newcode317
Line 317: // We don't want SSLHostState from the OTR profile to leak back to the
main
This is a design decision.  You can make good arguments for
both ways.  My first reaction was that the OTR profile
should share the SSL host state with the main profile.  Why
did you choose the other way?

http://codereview.chromium.org/20296/diff/1/6
File chrome/browser/ssl/ssl_host_state.h (right):

http://codereview.chromium.org/20296/diff/1/6#newcode5
Line 5: #ifndef CHROME_BROWSER_SSL_HOST_STATE_H_
Please fix the nits reported by cpplint.py.  You can
probably ignore "Could not find a newline character at the
end of the file."  I never understand that error.

The header include guard macro should be named
CHROME_BROWSER_SSL_SSL_HOST_STATE_H_, with an extra _SSL,
to match the file's pathname.

http://codereview.chromium.org/20296/diff/1/6#newcode17
Line 17: // The SSLHostState encapulates the host-specific state for SSL.  For
example,
Nit: "state for SSL" is vague.  How about "state for SSL
errors" or "state for SSL cert errors"?

http://codereview.chromium.org/20296/diff/1/6#newcode18
Line 18: // SSLHostState rememebers whether the user has white-listed a
particular cert
Nit: how about "broken cert"?  We should be clear that the
cert has errors.

http://codereview.chromium.org/20296/diff/1/3
File chrome/browser/ssl/ssl_manager.cc (right):

http://codereview.chromium.org/20296/diff/1/3#newcode99
Line 99: ssl_host_state_(controller->profile()->GetSSLHostState()) {
Question on thread safety: are SSLManager objects always
created on the UI thread?

controller->profile()->GetSSLHostState() lazily creates the
SSLHostState object in a thread-unsafe manner, so we need to
make sure we only call
controller->profile()->GetSSLHostState() from a single
thread.

http://codereview.chromium.org/20296/diff/1/3#newcode100
Line 100: DCHECK(controller_);
We can probably remove this DCHECK now that we're already
dereferencing controller in the ssl_host_state_ initializer.

http://codereview.chromium.org/20296/diff/1/3#newcode189
Line 189: ssl_host_state_->DenyCertForHost(cert, host);
Should we just inline these five methods in the
ssl_manager.h header?

[abarth-chromium, 2009-02-12 21:00:48]

On 2009/02/12 18:32:56, wtc wrote:
> Please review my comments below before you check this in.
> In particular, the issue on whether the OTR profile should
> share the SSL host state with the main profile,

I think we don't want to share state because we don't want the main profile to
learn which sites the user visited while in the OTR profile.  Because this state
is per-host, sharing it would tell the main profile that OTR went to
fuzzypinkelephants.com

> and the
> thread safety of the lazy creation of SSLHostState.

I made this explicit by making SSLHostState inherit from NonThreadSafe and added
lots of DCHECKs to ensure that we're always on the right thread.

> Line 100: DCHECK(controller_);

I left this in because it makes it obvious that controller_ must not be null.

> Line 189: ssl_host_state_->DenyCertForHost(cert, host);
> Should we just inline these five methods in the
> ssl_manager.h header?

I don't see a big value in inlining these methods.  The fact that they forward
to SSLHostState seems like an implementation detail that shouldn't be in the
header, but it doesn't matter that much to me.

[wtc, 2009-02-12 21:41:46]

LGTM++.  Thank you for running cpplint.py on all the files.

http://codereview.chromium.org/20296/diff/26/34
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/20296/diff/26/34#newcode319
Line 319: // profile because then the main profile would learn some of the host
names
I like this comment!

http://codereview.chromium.org/20296/diff/26/29
File chrome/browser/ssl/ssl_manager.h (right):

http://codereview.chromium.org/20296/diff/26/29#newcode404
Line 404: : message(text),
Nit: this colon should be indented 4 spaces.  See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Initializer_Lists