SSL Host info: Make sure that we can update certificate chains in

SSL Host info: Make sure that we can update certificate chains in
the cache.

BUG=102554
TEST=net_unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112271

[rvargas (doing something else), 2011-11-29 22:34:48]

On 2011/11/23 01:47:46, rvargas wrote:

I know you were on vacation... just making sure the request was not lost :)

[wtc, 2011-11-30 00:22:09]

Patch Set 2 LGTM.

I suggest two alternative changes that may be easier to
understand.  See my comments in disk_cache_based_ssl_host_info.{cc,h} below.

http://codereview.chromium.org/8670009/diff/5001/net/http/disk_cache_based_ss...
File net/http/disk_cache_based_ssl_host_info.cc (right):

http://codereview.chromium.org/8670009/diff/5001/net/http/disk_cache_based_ss...
net/http/disk_cache_based_ssl_host_info.cc:174: }

Instead of checking found_entry_ here, I suggest we check it
in DoOpen().  See my comment in DoOpen() below.

http://codereview.chromium.org/8670009/diff/5001/net/http/disk_cache_based_ss...
net/http/disk_cache_based_ssl_host_info.cc:216: state_ = OPEN_COMPLETE;

I suggest we do
  state_ = found_entry_ ? CREATE_COMPLETE: OPEN_COMPLETE;
here.

http://codereview.chromium.org/8670009/diff/5001/net/http/disk_cache_based_ss...
File net/http/disk_cache_based_ssl_host_info.h (right):

http://codereview.chromium.org/8670009/diff/5001/net/http/disk_cache_based_ss...
net/http/disk_cache_based_ssl_host_info.h:112: bool found_entry_;

The way you use found_entry_ is quite tricky.  It would be
nice to document it somewhere.

Also, it seems better to change the
  CREATE
  CREATE_COMPLETE
states to
  CREATE_OR_OPEN
  CREATE_OR_OPEN_COMPLETE
states, and check found_entry_ just in DoCreateOrOpen.
This seems easier to understand that diverging the state
transitions based on found_entry_ for the Persist sequence.

http://codereview.chromium.org/8670009/diff/5001/net/http/disk_cache_based_ss...
File net/http/disk_cache_based_ssl_host_info_unittest.cc (right):

http://codereview.chromium.org/8670009/diff/5001/net/http/disk_cache_based_ss...
net/http/disk_cache_based_ssl_host_info_unittest.cc:80: EXPECT_EQ(net::OK,
callback.GetResult(rv));

We should check that state->certs.empty() is true.

http://codereview.chromium.org/8670009/diff/5001/net/http/disk_cache_based_ss...
net/http/disk_cache_based_ssl_host_info_unittest.cc:104:
cache.disk_cache()->set_double_create_check(false);

Is this necessary?  With your found_entry_ change, I
believe we won't be calling CreateEntry twice.  We should
be calling OpenEntry if found_entry_ is true.

http://codereview.chromium.org/8670009/diff/5001/net/http/disk_cache_based_ss...
net/http/disk_cache_based_ssl_host_info_unittest.cc:117: EXPECT_EQ(2U,
state->certs.size());

Should we check that state->certs[0] is "foo" and
state->certs[1] is "bar"?

http://codereview.chromium.org/8670009/diff/5001/net/http/mock_http_cache.cc
File net/http/mock_http_cache.cc (right):

http://codereview.chromium.org/8670009/diff/5001/net/http/mock_http_cache.cc#...
net/http/mock_http_cache.cc:399: return net::ERR_CACHE_CREATE_FAILURE;

Nit: add curly braces?

[rvargas (doing something else), 2011-11-30 01:05:24]

Thanks.

Please take another look.

http://codereview.chromium.org/8670009/diff/5001/net/http/disk_cache_based_ss...
File net/http/disk_cache_based_ssl_host_info_unittest.cc (right):

http://codereview.chromium.org/8670009/diff/5001/net/http/disk_cache_based_ss...
net/http/disk_cache_based_ssl_host_info_unittest.cc:104:
cache.disk_cache()->set_double_create_check(false);
On 2011/11/30 00:22:09, wtc wrote:
> 
> Is this necessary?  With your found_entry_ change, I
> believe we won't be calling CreateEntry twice.  We should
> be calling OpenEntry if found_entry_ is true.

I just want to make sure that if the test fails (without this fix), we don't
crash net_unittests.

http://codereview.chromium.org/8670009/diff/5001/net/http/disk_cache_based_ss...
net/http/disk_cache_based_ssl_host_info_unittest.cc:117: EXPECT_EQ(2U,
state->certs.size());
On 2011/11/30 00:22:09, wtc wrote:
> 
> Should we check that state->certs[0] is "foo" and
> state->certs[1] is "bar"?

It felt like testing the mock cache... but I guess we can test that the order is
not reversed...

http://codereview.chromium.org/8670009/diff/5001/net/http/mock_http_cache.cc
File net/http/mock_http_cache.cc (right):

http://codereview.chromium.org/8670009/diff/5001/net/http/mock_http_cache.cc#...
net/http/mock_http_cache.cc:399: return net::ERR_CACHE_CREATE_FAILURE;
On 2011/11/30 00:22:09, wtc wrote:
> 
> Nit: add curly braces?

Sorry, I missed this comment. Before I change it, do you mention it for the
NOTREACHED or for the else? I ask because I consistently don't add braces for
the else, but I'm ambivalent on macros.

[wtc, 2011-11-30 01:36:39]

Patch Set 3 LGTM.  Thanks!

http://codereview.chromium.org/8670009/diff/5001/net/http/mock_http_cache.cc
File net/http/mock_http_cache.cc (right):

http://codereview.chromium.org/8670009/diff/5001/net/http/mock_http_cache.cc#...
net/http/mock_http_cache.cc:399: return net::ERR_CACHE_CREATE_FAILURE;
On 2011/11/30 01:05:24, rvargas wrote:
>
> Sorry, I missed this comment. Before I change it, do you mention it for the
> NOTREACHED or for the else? I ask because I consistently don't add braces for
> the else, but I'm ambivalent on macros.

You can ignore this change request.

When I first started working on Chrome, Darin always made
me add curly braces to any if statements that have an "else",
even if the bodies are one-liners.

So I am just trying to follow Darin's style.  It is not
because NOTREACHED() is a macro.

[commit-bot: I haz the power, 2011-11-30 18:50:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rvargas@chromium.org/8670009/9002

[commit-bot: I haz the power, 2011-11-30 19:57:36]

Change committed as 112271