DiskBasedCertCache method name change + readability fixes.

DiskBasedCertCache method name change + readability fixes.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287464

[brandonsalmon, 2014-07-31 19:27:19]

A CL to make minor improvements to DBCC.

[Ryan Sleevi, 2014-07-31 19:56:37]

lgtm

[wtc, 2014-08-01 03:03:55]

Patch set 3 LGTM.

1. Please avoid the DBCC abbreviation in the CL's description. It is not obvious
to people unfamiliar with this code.

2. Note the comment marked with "IMPORTANT". That seems like a bug.

https://codereview.chromium.org/432053002/diff/80001/net/http/disk_based_cert...
File net/http/disk_based_cert_cache.cc (right):

https://codereview.chromium.org/432053002/diff/80001/net/http/disk_based_cert...
net/http/disk_based_cert_cache.cc:449: RecordCacheResult(DISK_CACHE_ERROR);

IMPORTANT: can you explain this change? With this change, the CACHE_MISS cache
result becomes unused.

https://codereview.chromium.org/432053002/diff/80001/net/http/disk_based_cert...
File net/http/disk_based_cert_cache_unittest.cc (right):

https://codereview.chromium.org/432053002/diff/80001/net/http/disk_based_cert...
net/http/disk_based_cert_cache_unittest.cc:304: // first SetCertificate
operation is not yet complete when the second Set is

second Set => second SetCertificate

[brandonsalmon, 2014-08-01 22:14:29]

I thought I might as well remove the CACHE_MISS error code too since it isn't
used. Luckily, CACHE_MISS was the last defined cache result in the enum, so
removing it shouldn't cause any issues with the histograms.

Should I change Histograms.xml in this cl or save it for another one?

https://codereview.chromium.org/432053002/diff/80001/net/http/disk_based_cert...
File net/http/disk_based_cert_cache.cc (right):

https://codereview.chromium.org/432053002/diff/80001/net/http/disk_based_cert...
net/http/disk_based_cert_cache.cc:449: RecordCacheResult(DISK_CACHE_ERROR);
On 2014/08/01 03:03:55, wtc wrote:
> 
> IMPORTANT: can you explain this change? With this change, the CACHE_MISS cache
> result becomes unused.

Yep, that was intentional.

When I was originally designing this I wanted to differentiate between cache
misses and other errors, so I looked up the error code for cache misses.
Unfortunately, I made a mistake by not checking if OpenEntry actually returns
ERR_CACHE_MISS. It turns out it doesn't, if I remember correctly it just returns
ERR_FAILED.

As a result of rv not being ERR_CACHE_MISS, cache misses have been recorded as
DISK_CACHE_ERROR. Here I am modifying the code to reflect that.

[rvargas (doing something else), 2014-08-01 23:06:49]

lgtm in case you're waiting for me.

[brandonsalmon, 2014-08-04 20:52:50]

The CQ bit was checked by brandonsalmon@chromium.org

[commit-bot: I haz the power, 2014-08-04 20:54:23]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/brandonsalmon@chromium.org/432053002/1...

[wtc, 2014-08-04 22:44:42]

Patch set 4 LGTM.

On 2014/08/01 22:14:29, brandonsalmon wrote:
> 
> Should I change Histograms.xml in this cl or save it for another one?

You can change histograms.xml in another CL.

https://codereview.chromium.org/432053002/diff/120001/net/http/disk_based_cer...
File net/http/disk_based_cert_cache.cc (right):

https://codereview.chromium.org/432053002/diff/120001/net/http/disk_based_cer...
net/http/disk_based_cert_cache.cc:447: if (rv < 0) {

It seems useful to be able to distinguish between a cache miss and a cache
error. Can we change OpenEntry to return ERR_CACHE_MISS or ERR_FILE_NOT_FOUND in
the cache miss case?

[rvargas (doing something else), 2014-08-04 22:59:48]

https://codereview.chromium.org/432053002/diff/120001/net/http/disk_based_cer...
File net/http/disk_based_cert_cache.cc (right):

https://codereview.chromium.org/432053002/diff/120001/net/http/disk_based_cer...
net/http/disk_based_cert_cache.cc:447: if (rv < 0) {
On 2014/08/04 22:44:42, wtc wrote:
> 
> It seems useful to be able to distinguish between a cache miss and a cache
> error. Can we change OpenEntry to return ERR_CACHE_MISS or ERR_FILE_NOT_FOUND
in
> the cache miss case?

What would you do with specific codes other than log the result and move on?

Errors should be logged at the lower level (close to where they actually
happen), and the code close to the interface basically just returns the entry if
found, so failure is pretty much not found (but ERR_CACHE_MISS has a very
specific meaning, returned by the HTTP cache, and FILE_NOT_FOUND suggests a
specific OS level error to me). That is to say, I favor code that doesn't rely
on specific error codes unless really needed.

[commit-bot: I haz the power, 2014-08-05 05:43:58]

Change committed as 287464