Adding histograms showing fraction of page load times

Adding histograms showing fraction of page load times spent in the disk cache, for full and partial page loads.
R=mmenke@chromium.org, jam@chromium.org, willchan@chromium.org, brettw@chromium.org
BUG=139210
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=149007

[mmenke, 2012-07-16 20:42:38]

Preliminary comment:  I think this is sticking too much junk in net - too many
callbacks from the cache, and another delegate that I'd rather do without.

I'm trying to come up with an option I like more.  Looks like load states are
pull-based, and only polled once every 100 milliseconds, unfortunately, or I'd
suggest we make do with those.

[tburkard, 2012-07-17 21:00:06]

Per our discussion, I updated the change.

On Mon, Jul 16, 2012 at 1:42 PM, <mmenke@chromium.org> wrote:

> Preliminary comment:  I think this is sticking too much junk in net - too
> many
> callbacks from the cache, and another delegate that I'd rather do without.
>
> I'm trying to come up with an option I like more.  Looks like load states
> are
> pull-based, and only polled once every 100 milliseconds, unfortunately, or
> I'd
> suggest we make do with those.
>
>
http://codereview.chromium.**org/10736066/<http://codereview.chromium.org/107...
>

[mmenke, 2012-07-17 21:17:40]

On 2012/07/17 21:00:06, tburkard wrote:
> Per our discussion, I updated the change.

Great, I'll take a look tomorrow.

[mmenke, 2012-07-18 19:34:22]

On 2012/07/17 21:17:40, Matt Menke wrote:
> On 2012/07/17 21:00:06, tburkard wrote:
> > Per our discussion, I updated the change.
> 
> Great, I'll take a look tomorrow.

Probably won't be able get feedback to you today, unfortunately.

[mmenke, 2012-07-19 16:12:53]

I can live with this.  I'm assuming you don't yet want a full review (You're not
cleaning up your stats stuff, for instance).

https://chromiumcodereview.appspot.com/10736066/diff/19021/chrome/browser/net...
File chrome/browser/net/cache_stats.cc (right):

https://chromiumcodereview.appspot.com/10736066/diff/19021/chrome/browser/net...
chrome/browser/net/cache_stats.cc:104: if (!render_view_id_initialized_)
Can this happen?

https://chromiumcodereview.appspot.com/10736066/diff/19021/chrome/browser/net...
chrome/browser/net/cache_stats.cc:110: if (is_loading_)
This check worries me a bit, in terms of getting accurate stats.  If two main
frame navigations start in a row, without a DidStopLoading in between, we merge
the load times?

https://chromiumcodereview.appspot.com/10736066/diff/19021/net/url_request/ur...
File net/url_request/url_request_http_job.cc (right):

https://chromiumcodereview.appspot.com/10736066/diff/19021/net/url_request/ur...
net/url_request/url_request_http_job.cc:78: class
URLRequestHttpJob::HttpTransactionDelegateImpl
Was hoping we could get rid of this class, but because of the funky ref counting
of URLRequestJob, doesn't look like that's sane (I think it would actually be
safe, because of how things we use the ref counting, but doesn't seem like a
good idea).

[tburkard, 2012-07-19 16:36:15]

Added the cleanup, and addressed your concerns.  You can do the full review now.

https://chromiumcodereview.appspot.com/10736066/diff/19021/chrome/browser/net...
File chrome/browser/net/cache_stats.cc (right):

https://chromiumcodereview.appspot.com/10736066/diff/19021/chrome/browser/net...
chrome/browser/net/cache_stats.cc:104: if (!render_view_id_initialized_)
I don't think it can, but I wanted to consider this case and do a graceful
handling of it in case it does happen in some weird corner case.

On 2012/07/19 16:12:54, Matt Menke wrote:
> Can this happen?

https://chromiumcodereview.appspot.com/10736066/diff/19021/chrome/browser/net...
chrome/browser/net/cache_stats.cc:110: if (is_loading_)
Fixed.
On 2012/07/19 16:12:54, Matt Menke wrote:
> This check worries me a bit, in terms of getting accurate stats.  If two main
> frame navigations start in a row, without a DidStopLoading in between, we
merge
> the load times?

https://chromiumcodereview.appspot.com/10736066/diff/19021/net/url_request/ur...
File net/url_request/url_request_http_job.cc (right):

https://chromiumcodereview.appspot.com/10736066/diff/19021/net/url_request/ur...
net/url_request/url_request_http_job.cc:78: class
URLRequestHttpJob::HttpTransactionDelegateImpl
Yes I think we need this.

On 2012/07/19 16:12:54, Matt Menke wrote:
> Was hoping we could get rid of this class, but because of the funky ref
counting
> of URLRequestJob, doesn't look like that's sane (I think it would actually be
> safe, because of how things we use the ref counting, but doesn't seem like a
> good idea).

[mmenke, 2012-07-19 17:55:34]

You should also have rvargas review this.  Up to you if you want to add him
sooner or later.

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
File chrome/browser/net/cache_stats.cc (right):

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.cc:21: #include <stdio.h>
Fix include order.

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.cc:105: content::RenderViewHost*
render_view_host) {
Fix indent

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.cc:125:
NotifyCacheStats(CacheStats::SPINNER_STOP);
It's actually possible for this not to be for the page with the last
DidStartProvisionalLoadForFrame, I believe.  Not sure there's much we can do
about that case, but with this code, we will always get the render view id wrong
in that case.

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.cc:132:
NotifyCacheStats(CacheStats::RENDERER_DESTROY);
This is incorrect - if it's a cross process navigation, it could end up being
cancelled, and then we'd be keeping the old render view around, so process_id,
route_id, and render_view_id_ would all be incorrect.

Also, I'm not sure what happens for interstitial pages - a new RenderView is
created, but I'm not sure whether or not we'd ever hear about it here.  I don't
believe they'll access the cache (Or do any over the wire requests), but I'm not
positive.

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.cc:139: void
CacheStatsTabHelper::NotifyCacheStats(CacheStats::TabEvent event) {
Fix indent

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.cc:165: DCHECK(intermediate_histograms_.size() ==
arraysize(kStatsCollectionTimesMs));
DCHECK_EQ?

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.cc:200: int render_view, route_id;
render_view should be process_id.

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.cc:232: DCHECK(entry->second >= 0);
DCHECK_GE?

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.cc:233: DCHECK(!newly_started ||
!newly_finished);
I find this a little hard to follow.  I'd suggest just doing a
DCHECK(!newly_finished) in the newly_started case.

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.cc:243: base::TimeTicks::Now() -
stats->cache_start_time_;
Should we remove it from the map?  Otherwise, as long as there's no main frame
navigation, we'll continue to accumulate objects.

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.cc:270: RemoveTabLoadStats(render_view_id);
So when navigating though an HTTPS site...We just accumulate objects until the
tab is closed or we navigate to a non-https site?

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
File chrome/browser/net/cache_stats.h (right):

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.h:36: class CacheStats : public
MessageLoop::DestructionObserver {
Document this class - singleton, lives on IO thread, etc.

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.h:49: private:
nit:  Add blank line before "private"

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.h:60: bool is_load_done);
These functions should be documented

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.h:61: typedef std::map< std::pair<int, int>,
TabLoadStats*> TabLoadStatsMap;
typedefs should go before method declarations.

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.h:62: TabLoadStatsMap tab_load_stats_;
Please add a blank line before the class variables.

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.h:66: };
Fix indent

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.h:68: class CacheStatsTabHelper : public
content::WebContentsObserver {
Document the class.

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.h:81: private:
nit:  Add blank line before "private"

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.h:82: void NotifyCacheStats(CacheStats::TabEvent
event);
This should be documented.

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.h:83: CacheStats* cache_stats_;
Please add a blank line before the class variables.

[tburkard, 2012-07-19 22:54:33]

Addressed everything except (as noted below):
-- Fixing race conditions if there are multiple render views, including https.
-- cleaning up old requests.

I will address these separately later today or tomorrow, I will ping you once
that's done.

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
File chrome/browser/net/cache_stats.cc (right):

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.cc:21: #include <stdio.h>
On 2012/07/19 17:55:34, Matt Menke wrote:
> Fix include order.

Done.

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.cc:105: content::RenderViewHost*
render_view_host) {
On 2012/07/19 17:55:34, Matt Menke wrote:
> Fix indent

Done.

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.cc:125:
NotifyCacheStats(CacheStats::SPINNER_STOP);
will fix separately.

On 2012/07/19 17:55:34, Matt Menke wrote:
> It's actually possible for this not to be for the page with the last
> DidStartProvisionalLoadForFrame, I believe.  Not sure there's much we can do
> about that case, but with this code, we will always get the render view id
wrong
> in that case.

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.cc:132:
NotifyCacheStats(CacheStats::RENDERER_DESTROY);
will fix separately.

On 2012/07/19 17:55:34, Matt Menke wrote:
> This is incorrect - if it's a cross process navigation, it could end up being
> cancelled, and then we'd be keeping the old render view around, so process_id,
> route_id, and render_view_id_ would all be incorrect.
> 
> Also, I'm not sure what happens for interstitial pages - a new RenderView is
> created, but I'm not sure whether or not we'd ever hear about it here.  I
don't
> believe they'll access the cache (Or do any over the wire requests), but I'm
not
> positive.

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.cc:139: void
CacheStatsTabHelper::NotifyCacheStats(CacheStats::TabEvent event) {
On 2012/07/19 17:55:34, Matt Menke wrote:
> Fix indent

Done.

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.cc:165: DCHECK(intermediate_histograms_.size() ==
arraysize(kStatsCollectionTimesMs));
On 2012/07/19 17:55:34, Matt Menke wrote:
> DCHECK_EQ?

Done.

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.cc:200: int render_view, route_id;
On 2012/07/19 17:55:34, Matt Menke wrote:
> render_view should be process_id.

Done.

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.cc:232: DCHECK(entry->second >= 0);
On 2012/07/19 17:55:34, Matt Menke wrote:
> DCHECK_GE?

Done.

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.cc:233: DCHECK(!newly_started ||
!newly_finished);
On 2012/07/19 17:55:34, Matt Menke wrote:
> I find this a little hard to follow.  I'd suggest just doing a
> DCHECK(!newly_finished) in the newly_started case.

Done.

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.cc:243: base::TimeTicks::Now() -
stats->cache_start_time_;
will fix separately.

On 2012/07/19 17:55:34, Matt Menke wrote:
> Should we remove it from the map?  Otherwise, as long as there's no main frame
> navigation, we'll continue to accumulate objects.

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.cc:270: RemoveTabLoadStats(render_view_id);
will fix separately.

On 2012/07/19 17:55:34, Matt Menke wrote:
> So when navigating though an HTTPS site...We just accumulate objects until the
> tab is closed or we navigate to a non-https site?

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
File chrome/browser/net/cache_stats.h (right):

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.h:36: class CacheStats : public
MessageLoop::DestructionObserver {
On 2012/07/19 17:55:34, Matt Menke wrote:
> Document this class - singleton, lives on IO thread, etc.

Done.

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.h:49: private:
On 2012/07/19 17:55:34, Matt Menke wrote:
> nit:  Add blank line before "private"

Done.

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.h:60: bool is_load_done);
On 2012/07/19 17:55:34, Matt Menke wrote:
> These functions should be documented

Done.

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.h:61: typedef std::map< std::pair<int, int>,
TabLoadStats*> TabLoadStatsMap;
On 2012/07/19 17:55:34, Matt Menke wrote:
> typedefs should go before method declarations.

Done.

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.h:62: TabLoadStatsMap tab_load_stats_;
On 2012/07/19 17:55:34, Matt Menke wrote:
> Please add a blank line before the class variables.

Done.

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.h:66: };
On 2012/07/19 17:55:34, Matt Menke wrote:
> Fix indent

Done.

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.h:68: class CacheStatsTabHelper : public
content::WebContentsObserver {
On 2012/07/19 17:55:34, Matt Menke wrote:
> Document the class.

Done.

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.h:81: private:
On 2012/07/19 17:55:34, Matt Menke wrote:
> nit:  Add blank line before "private"

Done.

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.h:82: void NotifyCacheStats(CacheStats::TabEvent
event);
On 2012/07/19 17:55:34, Matt Menke wrote:
> This should be documented.

Done.

https://chromiumcodereview.appspot.com/10736066/diff/41006/chrome/browser/net...
chrome/browser/net/cache_stats.h:83: CacheStats* cache_stats_;
On 2012/07/19 17:55:34, Matt Menke wrote:
> Please add a blank line before the class variables.

Done.

[tburkard, 2012-07-20 18:08:10]

Addressed all the outstanding points per our discussion, please re-review.

[mmenke, 2012-07-20 18:37:44]

The code looks correct to me, though I do want to take the time to carefully go
over it, and you should have Ricardo go over it carefully as well.

I still remain unconvinced that this is something we really want to do - seems
more than a little intrusive for just collecting stats, to me.

http://codereview.chromium.org/10736066/diff/38054/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.cc (right):

http://codereview.chromium.org/10736066/diff/38054/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:424: new
chrome_browser_net::CacheStatsRenderViewHostObserver(render_view_host);
Is this called for all RenderViews?  How about dev tools and the like?  Code
should be fine either way, just want to understand what's going on.

http://codereview.chromium.org/10736066/diff/38054/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.cc (right):

http://codereview.chromium.org/10736066/diff/38054/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:87: }
Constructor should go first.  Also, add a blank line after it.

http://codereview.chromium.org/10736066/diff/38054/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:128:
NotifyCacheStats(CacheStats::SPINNER_STOP);
Wonder if we should still record stats for cancelled loads.

http://codereview.chromium.org/10736066/diff/38054/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:262: } else {
DCHECK_EQ(SPINNER_STOP, RENDERER_DESTROY);

http://codereview.chromium.org/10736066/diff/38054/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.h (right):

http://codereview.chromium.org/10736066/diff/38054/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:84: public content::RenderViewHostObserver {
Fix indent.  Also, I believe the more common style is to put the colon on the
same line as public.

[tburkard, 2012-07-20 19:36:42]

Addressed all comments.

As for whether we need those stats or not, we have discussed this extensively
and reached the conclusion that we want to collect this.  Especially now that
the code is all done, I would prefer not to question this decision again.

Adding Ricardo, FYI, if you have any comments.

Matt, please complete your review and let me know when you feel you can LGTM
this.  Thanks.

https://chromiumcodereview.appspot.com/10736066/diff/38054/chrome/browser/chr...
File chrome/browser/chrome_content_browser_client.cc (right):

https://chromiumcodereview.appspot.com/10736066/diff/38054/chrome/browser/chr...
chrome/browser/chrome_content_browser_client.cc:424: new
chrome_browser_net::CacheStatsRenderViewHostObserver(render_view_host);
Yes.

On 2012/07/20 18:37:44, Matt Menke wrote:
> Is this called for all RenderViews?  How about dev tools and the like?  Code
> should be fine either way, just want to understand what's going on.

https://chromiumcodereview.appspot.com/10736066/diff/38054/chrome/browser/net...
File chrome/browser/net/cache_stats.cc (right):

https://chromiumcodereview.appspot.com/10736066/diff/38054/chrome/browser/net...
chrome/browser/net/cache_stats.cc:87: }
On 2012/07/20 18:37:44, Matt Menke wrote:
> Constructor should go first.  Also, add a blank line after it.

Done.

https://chromiumcodereview.appspot.com/10736066/diff/38054/chrome/browser/net...
chrome/browser/net/cache_stats.cc:128:
NotifyCacheStats(CacheStats::SPINNER_STOP);
I'd say so.. some pages take long to load, and they might essentially be done,
but still spinning.  A user then going elsewhere should not completely discount
the page.

On 2012/07/20 18:37:44, Matt Menke wrote:
> Wonder if we should still record stats for cancelled loads.

https://chromiumcodereview.appspot.com/10736066/diff/38054/chrome/browser/net...
chrome/browser/net/cache_stats.cc:262: } else {
On 2012/07/20 18:37:44, Matt Menke wrote:
> DCHECK_EQ(SPINNER_STOP, RENDERER_DESTROY);

Done.

https://chromiumcodereview.appspot.com/10736066/diff/38054/chrome/browser/net...
File chrome/browser/net/cache_stats.h (right):

https://chromiumcodereview.appspot.com/10736066/diff/38054/chrome/browser/net...
chrome/browser/net/cache_stats.h:84: public content::RenderViewHostObserver {
On 2012/07/20 18:37:44, Matt Menke wrote:
> Fix indent.  Also, I believe the more common style is to put the colon on the
> same line as public.

Done.

[rvargas (doing something else), 2012-07-21 03:04:46]

+willchan: you may be interested.

I definitely want to review this code. I have only seen a few files so far :(

http://codereview.chromium.org/10736066/diff/32088/net/base/network_delegate.h
File net/base/network_delegate.h (right):

http://codereview.chromium.org/10736066/diff/32088/net/base/network_delegate....
net/base/network_delegate.h:232: // START called several times for the same
request.  It is the responsibility
nit: start _is_ called (or may be called)

http://codereview.chromium.org/10736066/diff/32088/net/http/http_transaction_...
File net/http/http_transaction_delegate.h (right):

http://codereview.chromium.org/10736066/diff/32088/net/http/http_transaction_...
net/http/http_transaction_delegate.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
It's been a long time in the oven :)

http://codereview.chromium.org/10736066/diff/32088/net/http/http_transaction_...
net/http/http_transaction_delegate.h:5: #ifndef
NET_HTTP_HTTP_TRANSACTION_DELEGATE_H__
We moved away from two underscores a long time ago.

http://codereview.chromium.org/10736066/diff/32088/net/http/http_transaction_...
net/http/http_transaction_delegate.h:11: // i.e. when the object starts and
finishes waiting on an underlying cache.
Reading this header I should be able to understand how to use it. It's not clear
what the delegate is for, or what the methods do.

http://codereview.chromium.org/10736066/diff/32088/net/http/http_transaction_...
net/http/http_transaction_delegate.h:12: class HttpTransactionDelegate {
needs a destructor.

http://codereview.chromium.org/10736066/diff/32088/net/url_request/url_reques...
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/10736066/diff/32088/net/url_request/url_reques...
net/url_request/url_request_job.h:290: virtual void OnDetachRequest() { }
nit: remove the space {}

[rvargas (doing something else), 2012-07-23 22:22:36]

I didn't review cache_stats carefully.

https://chromiumcodereview.appspot.com/10736066/diff/32088/chrome/browser/net...
File chrome/browser/net/cache_stats.cc (right):

https://chromiumcodereview.appspot.com/10736066/diff/32088/chrome/browser/net...
chrome/browser/net/cache_stats.cc:88: PerRequestNumActiveMap
per_request_num_active_;
This deserves a comment.

https://chromiumcodereview.appspot.com/10736066/diff/32088/chrome/browser/net...
chrome/browser/net/cache_stats.cc:153: "DiskCache.FractionCacheUseFinalPLT_" +
Don't use DiskCache.x here

https://chromiumcodereview.appspot.com/10736066/diff/32088/chrome/browser/net...
chrome/browser/net/cache_stats.cc:215: entry->second++;
Isn't second > 1 a bug?

https://chromiumcodereview.appspot.com/10736066/diff/32088/chrome/browser/net...
chrome/browser/net/cache_stats.cc:311: double cache_fraction =
This is not needed. TimeXx are 64 bit values.

https://chromiumcodereview.appspot.com/10736066/diff/32088/chrome/browser/net...
File chrome/browser/net/cache_stats.h (right):

https://chromiumcodereview.appspot.com/10736066/diff/32088/chrome/browser/net...
chrome/browser/net/cache_stats.h:35: 
nit: remove line

https://chromiumcodereview.appspot.com/10736066/diff/32088/chrome/browser/net...
chrome/browser/net/cache_stats.h:53: typedef std::map< std::pair<int, int>,
TabLoadStats*> TabLoadStatsMap;
nit: remove extra space before std

https://chromiumcodereview.appspot.com/10736066/diff/32088/chrome/browser/net...
chrome/browser/net/cache_stats.h:81: 
nit: remove line

https://chromiumcodereview.appspot.com/10736066/diff/32088/chrome/browser/net...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://chromiumcodereview.appspot.com/10736066/diff/32088/chrome/browser/net...
chrome/browser/net/chrome_network_delegate.cc:138:
cache_stats_(chrome_browser_net::CacheStats::GetInstance()) {
This bundles together info from all profiles / request contexts, right? Each
cache type has a different backend implementation so putting all the data
together is not that good.

https://chromiumcodereview.appspot.com/10736066/diff/32088/net/http/http_cach...
File net/http/http_cache_transaction.cc (right):

https://chromiumcodereview.appspot.com/10736066/diff/32088/net/http/http_cach...
net/http/http_cache_transaction.cc:725: int rv =
cache_->network_layer_->CreateTransaction(&network_trans_, NULL);
It would be interesting to track the network activity too. Of course, not as
part of this CL.

https://chromiumcodereview.appspot.com/10736066/diff/32088/net/http/http_cach...
net/http/http_cache_transaction.cc:962: ReportCacheActionStart();
We should treat this one some other way. This is not waiting for the disk cache,
it is waiting for the http cache.

https://chromiumcodereview.appspot.com/10736066/diff/32088/net/http/http_cach...
net/http/http_cache_transaction.cc:1228: ReportCacheActionStart();
This is where we start getting confusing results.

Previous times are blocking in that no other request can progress if it is in
the same state (they can make progress in other states). 

States after this one are non blocking so multiple requests can be reading the
response form the cache and all can make progress at the same time.

So a single notification for both things is not appropriate.

https://chromiumcodereview.appspot.com/10736066/diff/32088/net/http/http_cach...
File net/http/http_cache_transaction.h (right):

https://chromiumcodereview.appspot.com/10736066/diff/32088/net/http/http_cach...
net/http/http_cache_transaction.h:60: explicit Transaction(HttpCache* cache,
HttpTransactionDelegate* delegate);
Not explicit anymore

https://chromiumcodereview.appspot.com/10736066/diff/32088/net/http/http_cach...
File net/http/http_cache_unittest.cc (right):

https://chromiumcodereview.appspot.com/10736066/diff/32088/net/http/http_cach...
net/http/http_cache_unittest.cc:4952: }
We need a unit test for the delegate.

https://chromiumcodereview.appspot.com/10736066/diff/32088/net/url_request/ur...
File net/url_request/url_request_http_job.cc (right):

https://chromiumcodereview.appspot.com/10736066/diff/32088/net/url_request/ur...
net/url_request/url_request_http_job.cc:93:
NetworkDelegate::CACHE_WAIT_STATE_DONE);
This generates a notification when the first job goes away... what happens with
subsequent jobs for the same request?

https://chromiumcodereview.appspot.com/10736066/diff/32088/net/url_request/ur...
File net/url_request/url_request_http_job.h (right):

https://chromiumcodereview.appspot.com/10736066/diff/32088/net/url_request/ur...
net/url_request/url_request_http_job.h:94: 
nit: remove line

https://chromiumcodereview.appspot.com/10736066/diff/32088/net/url_request/ur...
net/url_request/url_request_http_job.h:147: 
nit: remove line

[tburkard, 2012-07-24 01:03:12]

Thanks, Ricardo, for your helpful comments.

Matt, can you review the one comment that states your name -- in the light of
Ricardo's feedback, are you ok with putting the accounting logic back in?  It is
only a few lines of code.

Thanks.

http://codereview.chromium.org/10736066/diff/32088/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.cc (right):

http://codereview.chromium.org/10736066/diff/32088/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:88: PerRequestNumActiveMap
per_request_num_active_;
On 2012/07/23 22:22:36, rvargas wrote:
> This deserves a comment.

Done.

http://codereview.chromium.org/10736066/diff/32088/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:153: "DiskCache.FractionCacheUseFinalPLT_" +
On 2012/07/23 22:22:36, rvargas wrote:
> Don't use DiskCache.x here

Done.

http://codereview.chromium.org/10736066/diff/32088/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:215: entry->second++;
Not necessarily, you could imagine at some point having two separate cache
actions pending for a given URLRequest at the same time.
On 2012/07/23 22:22:36, rvargas wrote:
> Isn't second > 1 a bug?

http://codereview.chromium.org/10736066/diff/32088/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:311: double cache_fraction =
But if I divide two 64 bit integers, I will get an integer result, which would
be 0, no?  That's why I want to convert to double first?
On 2012/07/23 22:22:36, rvargas wrote:
> This is not needed. TimeXx are 64 bit values.

http://codereview.chromium.org/10736066/diff/32088/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.h (right):

http://codereview.chromium.org/10736066/diff/32088/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:35: 
On 2012/07/23 22:22:36, rvargas wrote:
> nit: remove line

Done.

http://codereview.chromium.org/10736066/diff/32088/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:53: typedef std::map< std::pair<int, int>,
TabLoadStats*> TabLoadStatsMap;
On 2012/07/23 22:22:36, rvargas wrote:
> nit: remove extra space before std

Done.

http://codereview.chromium.org/10736066/diff/32088/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:81: 
On 2012/07/23 22:22:36, rvargas wrote:
> nit: remove line

Done.

http://codereview.chromium.org/10736066/diff/32088/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_network_delegate.cc (right):

http://codereview.chromium.org/10736066/diff/32088/chrome/browser/net/chrome_...
chrome/browser/net/chrome_network_delegate.cc:138:
cache_stats_(chrome_browser_net::CacheStats::GetInstance()) {
Changed it to only operate on non-OTR profiles, which are all be using the disk
cache (and no in memory cache).  Would that be better?
On 2012/07/23 22:22:36, rvargas wrote:
> This bundles together info from all profiles / request contexts, right? Each
> cache type has a different backend implementation so putting all the data
> together is not that good.

http://codereview.chromium.org/10736066/diff/32088/net/base/network_delegate.h
File net/base/network_delegate.h (right):

http://codereview.chromium.org/10736066/diff/32088/net/base/network_delegate....
net/base/network_delegate.h:232: // START called several times for the same
request.  It is the responsibility
On 2012/07/21 03:04:46, rvargas wrote:
> nit: start _is_ called (or may be called)

Done.

http://codereview.chromium.org/10736066/diff/32088/net/http/http_cache_transa...
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/10736066/diff/32088/net/http/http_cache_transa...
net/http/http_cache_transaction.cc:725: int rv =
cache_->network_layer_->CreateTransaction(&network_trans_, NULL);
(nothing to do here).

On 2012/07/23 22:22:36, rvargas wrote:
> It would be interesting to track the network activity too. Of course, not as
> part of this CL.

http://codereview.chromium.org/10736066/diff/32088/net/http/http_cache_transa...
net/http/http_cache_transaction.cc:962: ReportCacheActionStart();
Why is the distinction important?  What is the difference?

On 2012/07/23 22:22:36, rvargas wrote:
> We should treat this one some other way. This is not waiting for the disk
cache,
> it is waiting for the http cache.

http://codereview.chromium.org/10736066/diff/32088/net/http/http_cache_transa...
net/http/http_cache_transaction.cc:1228: ReportCacheActionStart();
That doesn't matter for the purpose of what I'm trying to do.

I only care about how much time is spent in cache actions.  Whether other cache
actions can run in parallel is not important for my metrics.

On 2012/07/23 22:22:36, rvargas wrote:
> This is where we start getting confusing results.
> 
> Previous times are blocking in that no other request can progress if it is in
> the same state (they can make progress in other states). 
> 
> States after this one are non blocking so multiple requests can be reading the
> response form the cache and all can make progress at the same time.
> 
> So a single notification for both things is not appropriate.

http://codereview.chromium.org/10736066/diff/32088/net/http/http_cache_transa...
File net/http/http_cache_transaction.h (right):

http://codereview.chromium.org/10736066/diff/32088/net/http/http_cache_transa...
net/http/http_cache_transaction.h:60: explicit Transaction(HttpCache* cache,
HttpTransactionDelegate* delegate);
On 2012/07/23 22:22:36, rvargas wrote:
> Not explicit anymore

Done.

http://codereview.chromium.org/10736066/diff/32088/net/http/http_cache_unitte...
File net/http/http_cache_unittest.cc (right):

http://codereview.chromium.org/10736066/diff/32088/net/http/http_cache_unitte...
net/http/http_cache_unittest.cc:4952: }
Since this is just instrumentation for testing, I don't think this is a huge
priority right now.
On 2012/07/23 22:22:36, rvargas wrote:
> We need a unit test for the delegate.

http://codereview.chromium.org/10736066/diff/32088/net/http/http_transaction_...
File net/http/http_transaction_delegate.h (right):

http://codereview.chromium.org/10736066/diff/32088/net/http/http_transaction_...
net/http/http_transaction_delegate.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2012/07/21 03:04:46, rvargas wrote:
> It's been a long time in the oven :)

Done.

http://codereview.chromium.org/10736066/diff/32088/net/http/http_transaction_...
net/http/http_transaction_delegate.h:5: #ifndef
NET_HTTP_HTTP_TRANSACTION_DELEGATE_H__
On 2012/07/21 03:04:46, rvargas wrote:
> We moved away from two underscores a long time ago.

Done.

http://codereview.chromium.org/10736066/diff/32088/net/http/http_transaction_...
net/http/http_transaction_delegate.h:11: // i.e. when the object starts and
finishes waiting on an underlying cache.
On 2012/07/21 03:04:46, rvargas wrote:
> Reading this header I should be able to understand how to use it. It's not
clear
> what the delegate is for, or what the methods do.

Done.

http://codereview.chromium.org/10736066/diff/32088/net/http/http_transaction_...
net/http/http_transaction_delegate.h:12: class HttpTransactionDelegate {
On 2012/07/21 03:04:46, rvargas wrote:
> needs a destructor.

Done.

http://codereview.chromium.org/10736066/diff/32088/net/url_request/url_reques...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/10736066/diff/32088/net/url_request/url_reques...
net/url_request/url_request_http_job.cc:93:
NetworkDelegate::CACHE_WAIT_STATE_DONE);
Excellent comment -- I was not aware that a request could have multiple jobs.  I
originally had this class keep track of how many cache actions were pending, and
then remove the appropriate amount.  Matt asked me to remove this and put that
logic into a class upstream.  It seems I have to put this back in, right?
On 2012/07/23 22:22:36, rvargas wrote:
> This generates a notification when the first job goes away... what happens
with
> subsequent jobs for the same request?

http://codereview.chromium.org/10736066/diff/32088/net/url_request/url_reques...
File net/url_request/url_request_http_job.h (right):

http://codereview.chromium.org/10736066/diff/32088/net/url_request/url_reques...
net/url_request/url_request_http_job.h:94: 
On 2012/07/23 22:22:36, rvargas wrote:
> nit: remove line

Done.

http://codereview.chromium.org/10736066/diff/32088/net/url_request/url_reques...
net/url_request/url_request_http_job.h:147: 
On 2012/07/23 22:22:36, rvargas wrote:
> nit: remove line

Done.

http://codereview.chromium.org/10736066/diff/32088/net/url_request/url_reques...
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/10736066/diff/32088/net/url_request/url_reques...
net/url_request/url_request_job.h:290: virtual void OnDetachRequest() { }
On 2012/07/21 03:04:46, rvargas wrote:
> nit: remove the space {}

Done.

[mmenke, 2012-07-24 01:21:25]

http://codereview.chromium.org/10736066/diff/32088/net/url_request/url_reques...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/10736066/diff/32088/net/url_request/url_reques...
net/url_request/url_request_http_job.cc:93:
NetworkDelegate::CACHE_WAIT_STATE_DONE);
On 2012/07/24 01:03:12, tburkard wrote:
> Excellent comment -- I was not aware that a request could have multiple jobs. 
I
> originally had this class keep track of how many cache actions were pending,
and
> then remove the appropriate amount.  Matt asked me to remove this and put that
> logic into a class upstream.  It seems I have to put this back in, right?
> On 2012/07/23 22:22:36, rvargas wrote:
> > This generates a notification when the first job goes away... what happens
> with
> > subsequent jobs for the same request?
> 

I actually thought about this, surprisingly ;).  We always orphan the old job
before creating the new one (Since we have to detach the request, to prevent
subsequent calls from the Job back to the request), so it should all work out
fine.

[rvargas (doing something else), 2012-07-24 03:10:37]

http://codereview.chromium.org/10736066/diff/32088/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.cc (right):

http://codereview.chromium.org/10736066/diff/32088/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:215: entry->second++;
On 2012/07/24 01:03:12, tburkard wrote:
> Not necessarily, you could imagine at some point having two separate cache
> actions pending for a given URLRequest at the same time.

We should wait until we have the need to support that use case (if ever). At
this point it seems better to detect that as a problem.

> On 2012/07/23 22:22:36, rvargas wrote:
> > Isn't second > 1 a bug?
>

http://codereview.chromium.org/10736066/diff/32088/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:311: double cache_fraction =
On 2012/07/24 01:03:12, tburkard wrote:
> But if I divide two 64 bit integers, I will get an integer result, which would
> be 0, no?  That's why I want to convert to double first?

Not if you multiply the numerator by 100 beforehand.

> On 2012/07/23 22:22:36, rvargas wrote:
> > This is not needed. TimeXx are 64 bit values.
>

http://codereview.chromium.org/10736066/diff/32088/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_network_delegate.cc (right):

http://codereview.chromium.org/10736066/diff/32088/chrome/browser/net/chrome_...
chrome/browser/net/chrome_network_delegate.cc:138:
cache_stats_(chrome_browser_net::CacheStats::GetInstance()) {
On 2012/07/24 01:03:12, tburkard wrote:
> Changed it to only operate on non-OTR profiles, which are all be using the
disk
> cache (and no in memory cache).  Would that be better?

That still treats the media cache and the http cache as the same, and the
implementation is not the same.


> On 2012/07/23 22:22:36, rvargas wrote:
> > This bundles together info from all profiles / request contexts, right? Each
> > cache type has a different backend implementation so putting all the data
> > together is not that good.
>

http://codereview.chromium.org/10736066/diff/32088/net/http/http_cache_transa...
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/10736066/diff/32088/net/http/http_cache_transa...
net/http/http_cache_transaction.cc:962: ReportCacheActionStart();
On 2012/07/24 01:03:12, tburkard wrote:
> Why is the distinction important?  What is the difference?

No matter what we change at the disk cache layer, we won't be touching this
number, so the final histogram will not change. It is not important if we only
say "the percentage of time attributed to the cache is x, let's move on", but
otherwise it adds a form of noise.


> 
> On 2012/07/23 22:22:36, rvargas wrote:
> > We should treat this one some other way. This is not waiting for the disk
> cache,
> > it is waiting for the http cache.
>

http://codereview.chromium.org/10736066/diff/32088/net/http/http_cache_transa...
net/http/http_cache_transaction.cc:1228: ReportCacheActionStart();
On 2012/07/24 01:03:12, tburkard wrote:
> That doesn't matter for the purpose of what I'm trying to do.
> 
> I only care about how much time is spent in cache actions.  Whether other
cache
> actions can run in parallel is not important for my metrics.

I think it matters.

If we only care about a single request, then fine, because we can say x% of the
time was spent on cache related things.

On the other hand, if we look at 100 requests that are all part of the same
page, it makes a difference what part of the time directly affects the total
time or not. You may end up with total cache time that is 150% the total time
used for loading the page (or any percentage for that matter). And then?

Maybe it doesn't really matter, I'm wondering what we would be able to extract
from the data though.


> 
> On 2012/07/23 22:22:36, rvargas wrote:
> > This is where we start getting confusing results.
> > 
> > Previous times are blocking in that no other request can progress if it is
in
> > the same state (they can make progress in other states). 
> > 
> > States after this one are non blocking so multiple requests can be reading
the
> > response form the cache and all can make progress at the same time.
> > 
> > So a single notification for both things is not appropriate.
>

http://codereview.chromium.org/10736066/diff/32088/net/http/http_cache_unitte...
File net/http/http_cache_unittest.cc (right):

http://codereview.chromium.org/10736066/diff/32088/net/http/http_cache_unitte...
net/http/http_cache_unittest.cc:4952: }
On 2012/07/24 01:03:12, tburkard wrote:
> Since this is just instrumentation for testing, I don't think this is a huge
> priority right now.

I have to disagree. This is a change in the interface of production code... it
deserves a unit test, and the time to add it is with the same CL that adds the
code.

> On 2012/07/23 22:22:36, rvargas wrote:
> > We need a unit test for the delegate.
>

http://codereview.chromium.org/10736066/diff/32088/net/url_request/url_reques...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/10736066/diff/32088/net/url_request/url_reques...
net/url_request/url_request_http_job.cc:93:
NetworkDelegate::CACHE_WAIT_STATE_DONE);
On 2012/07/24 01:21:25, Matt Menke wrote:
> On 2012/07/24 01:03:12, tburkard wrote:
> > Excellent comment -- I was not aware that a request could have multiple
jobs. 
> I
> > originally had this class keep track of how many cache actions were pending,
> and
> > then remove the appropriate amount.  Matt asked me to remove this and put
that
> > logic into a class upstream.  It seems I have to put this back in, right?
> > On 2012/07/23 22:22:36, rvargas wrote:
> > > This generates a notification when the first job goes away... what happens
> > with
> > > subsequent jobs for the same request?
> > 
> 
> I actually thought about this, surprisingly ;).  We always orphan the old job
> before creating the new one (Since we have to detach the request, to prevent
> subsequent calls from the Job back to the request), so it should all work out
> fine.

Isn't _STATE_DONE supposed to indicate that all processing with this request is
complete/aborted? It is weird to generate more start/stop notifications (for the
next job) after done is generated.

If we want that model then the name should be changed.

[tburkard, 2012-07-24 22:23:26]

http://codereview.chromium.org/10736066/diff/32088/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.cc (right):

http://codereview.chromium.org/10736066/diff/32088/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:215: entry->second++;
On 2012/07/24 03:10:38, rvargas wrote:
> On 2012/07/24 01:03:12, tburkard wrote:
> > Not necessarily, you could imagine at some point having two separate cache
> > actions pending for a given URLRequest at the same time.
> 
> We should wait until we have the need to support that use case (if ever). At
> this point it seems better to detect that as a problem.
> 
> > On 2012/07/23 22:22:36, rvargas wrote:
> > > Isn't second > 1 a bug?
> > 
> 

Done.

http://codereview.chromium.org/10736066/diff/32088/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:311: double cache_fraction =
On 2012/07/24 03:10:38, rvargas wrote:
> On 2012/07/24 01:03:12, tburkard wrote:
> > But if I divide two 64 bit integers, I will get an integer result, which
would
> > be 0, no?  That's why I want to convert to double first?
> 
> Not if you multiply the numerator by 100 beforehand.
> 
> > On 2012/07/23 22:22:36, rvargas wrote:
> > > This is not needed. TimeXx are 64 bit values.
> > 
> 

Done.

http://codereview.chromium.org/10736066/diff/32088/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_network_delegate.cc (right):

http://codereview.chromium.org/10736066/diff/32088/chrome/browser/net/chrome_...
chrome/browser/net/chrome_network_delegate.cc:138:
cache_stats_(chrome_browser_net::CacheStats::GetInstance()) {
I will get back to you on this one in a separate email/update.

On 2012/07/24 03:10:38, rvargas wrote:
> On 2012/07/24 01:03:12, tburkard wrote:
> > Changed it to only operate on non-OTR profiles, which are all be using the
> disk
> > cache (and no in memory cache).  Would that be better?
> 
> That still treats the media cache and the http cache as the same, and the
> implementation is not the same.
> 
> 
> > On 2012/07/23 22:22:36, rvargas wrote:
> > > This bundles together info from all profiles / request contexts, right?
Each
> > > cache type has a different backend implementation so putting all the data
> > > together is not that good.
> > 
>

http://codereview.chromium.org/10736066/diff/32088/net/http/http_cache_transa...
File net/http/http_cache_transaction.cc (right):

http://codereview.chromium.org/10736066/diff/32088/net/http/http_cache_transa...
net/http/http_cache_transaction.cc:962: ReportCacheActionStart();
On 2012/07/24 03:10:38, rvargas wrote:
> On 2012/07/24 01:03:12, tburkard wrote:
> > Why is the distinction important?  What is the difference?
> 
> No matter what we change at the disk cache layer, we won't be touching this
> number, so the final histogram will not change. It is not important if we only
> say "the percentage of time attributed to the cache is x, let's move on", but
> otherwise it adds a form of noise.
> 
> 
> > 
> > On 2012/07/23 22:22:36, rvargas wrote:
> > > We should treat this one some other way. This is not waiting for the disk
> > cache,
> > > it is waiting for the http cache.
> > 
> 

Done.

http://codereview.chromium.org/10736066/diff/32088/net/http/http_cache_transa...
net/http/http_cache_transaction.cc:1228: ReportCacheActionStart();
Per discussion on the phone, we are in agreement on this now.

On 2012/07/24 03:10:38, rvargas wrote:
> On 2012/07/24 01:03:12, tburkard wrote:
> > That doesn't matter for the purpose of what I'm trying to do.
> > 
> > I only care about how much time is spent in cache actions.  Whether other
> cache
> > actions can run in parallel is not important for my metrics.
> 
> I think it matters.
> 
> If we only care about a single request, then fine, because we can say x% of
the
> time was spent on cache related things.
> 
> On the other hand, if we look at 100 requests that are all part of the same
> page, it makes a difference what part of the time directly affects the total
> time or not. You may end up with total cache time that is 150% the total time
> used for loading the page (or any percentage for that matter). And then?
> 
> Maybe it doesn't really matter, I'm wondering what we would be able to extract
> from the data though.
> 
> 
> > 
> > On 2012/07/23 22:22:36, rvargas wrote:
> > > This is where we start getting confusing results.
> > > 
> > > Previous times are blocking in that no other request can progress if it is
> in
> > > the same state (they can make progress in other states). 
> > > 
> > > States after this one are non blocking so multiple requests can be reading
> the
> > > response form the cache and all can make progress at the same time.
> > > 
> > > So a single notification for both things is not appropriate.
> > 
>

http://codereview.chromium.org/10736066/diff/32088/net/http/http_cache_unitte...
File net/http/http_cache_unittest.cc (right):

http://codereview.chromium.org/10736066/diff/32088/net/http/http_cache_unitte...
net/http/http_cache_unittest.cc:4952: }
On 2012/07/24 03:10:38, rvargas wrote:
> On 2012/07/24 01:03:12, tburkard wrote:
> > Since this is just instrumentation for testing, I don't think this is a huge
> > priority right now.
> 
> I have to disagree. This is a change in the interface of production code... it
> deserves a unit test, and the time to add it is with the same CL that adds the
> code.
> 
> > On 2012/07/23 22:22:36, rvargas wrote:
> > > We need a unit test for the delegate.
> > 
> 

Done.

http://codereview.chromium.org/10736066/diff/32088/net/url_request/url_reques...
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/10736066/diff/32088/net/url_request/url_reques...
net/url_request/url_request_http_job.cc:93:
NetworkDelegate::CACHE_WAIT_STATE_DONE);
On 2012/07/24 03:10:38, rvargas wrote:
> On 2012/07/24 01:21:25, Matt Menke wrote:
> > On 2012/07/24 01:03:12, tburkard wrote:
> > > Excellent comment -- I was not aware that a request could have multiple
> jobs. 
> > I
> > > originally had this class keep track of how many cache actions were
pending,
> > and
> > > then remove the appropriate amount.  Matt asked me to remove this and put
> that
> > > logic into a class upstream.  It seems I have to put this back in, right?
> > > On 2012/07/23 22:22:36, rvargas wrote:
> > > > This generates a notification when the first job goes away... what
happens
> > > with
> > > > subsequent jobs for the same request?
> > > 
> > 
> > I actually thought about this, surprisingly ;).  We always orphan the old
job
> > before creating the new one (Since we have to detach the request, to prevent
> > subsequent calls from the Job back to the request), so it should all work
out
> > fine.
> 
> Isn't _STATE_DONE supposed to indicate that all processing with this request
is
> complete/aborted? It is weird to generate more start/stop notifications (for
the
> next job) after done is generated.
> 
> If we want that model then the name should be changed.

Done.

[willchan no longer on Chromium, 2012-07-24 22:49:25]

FYI: chiming in quickly to say that you should wait for my review at some point,
in particular for the chrome/browser/net stuff with DestructionObserver. I need
to read more to give a good recommendation on how to fix that, but
DestructionObserver is almost certainly the wrong way to do what you're doing.

On 2012/07/24 22:23:26, tburkard wrote:
>
http://codereview.chromium.org/10736066/diff/32088/chrome/browser/net/cache_s...
> File chrome/browser/net/cache_stats.cc (right):
> 
>
http://codereview.chromium.org/10736066/diff/32088/chrome/browser/net/cache_s...
> chrome/browser/net/cache_stats.cc:215: entry->second++;
> On 2012/07/24 03:10:38, rvargas wrote:
> > On 2012/07/24 01:03:12, tburkard wrote:
> > > Not necessarily, you could imagine at some point having two separate cache
> > > actions pending for a given URLRequest at the same time.
> > 
> > We should wait until we have the need to support that use case (if ever). At
> > this point it seems better to detect that as a problem.
> > 
> > > On 2012/07/23 22:22:36, rvargas wrote:
> > > > Isn't second > 1 a bug?
> > > 
> > 
> 
> Done.
> 
>
http://codereview.chromium.org/10736066/diff/32088/chrome/browser/net/cache_s...
> chrome/browser/net/cache_stats.cc:311: double cache_fraction =
> On 2012/07/24 03:10:38, rvargas wrote:
> > On 2012/07/24 01:03:12, tburkard wrote:
> > > But if I divide two 64 bit integers, I will get an integer result, which
> would
> > > be 0, no?  That's why I want to convert to double first?
> > 
> > Not if you multiply the numerator by 100 beforehand.
> > 
> > > On 2012/07/23 22:22:36, rvargas wrote:
> > > > This is not needed. TimeXx are 64 bit values.
> > > 
> > 
> 
> Done.
> 
>
http://codereview.chromium.org/10736066/diff/32088/chrome/browser/net/chrome_...
> File chrome/browser/net/chrome_network_delegate.cc (right):
> 
>
http://codereview.chromium.org/10736066/diff/32088/chrome/browser/net/chrome_...
> chrome/browser/net/chrome_network_delegate.cc:138:
> cache_stats_(chrome_browser_net::CacheStats::GetInstance()) {
> I will get back to you on this one in a separate email/update.
> 
> On 2012/07/24 03:10:38, rvargas wrote:
> > On 2012/07/24 01:03:12, tburkard wrote:
> > > Changed it to only operate on non-OTR profiles, which are all be using the
> > disk
> > > cache (and no in memory cache).  Would that be better?
> > 
> > That still treats the media cache and the http cache as the same, and the
> > implementation is not the same.
> > 
> > 
> > > On 2012/07/23 22:22:36, rvargas wrote:
> > > > This bundles together info from all profiles / request contexts, right?
> Each
> > > > cache type has a different backend implementation so putting all the
data
> > > > together is not that good.
> > > 
> >
> 
>
http://codereview.chromium.org/10736066/diff/32088/net/http/http_cache_transa...
> File net/http/http_cache_transaction.cc (right):
> 
>
http://codereview.chromium.org/10736066/diff/32088/net/http/http_cache_transa...
> net/http/http_cache_transaction.cc:962: ReportCacheActionStart();
> On 2012/07/24 03:10:38, rvargas wrote:
> > On 2012/07/24 01:03:12, tburkard wrote:
> > > Why is the distinction important?  What is the difference?
> > 
> > No matter what we change at the disk cache layer, we won't be touching this
> > number, so the final histogram will not change. It is not important if we
only
> > say "the percentage of time attributed to the cache is x, let's move on",
but
> > otherwise it adds a form of noise.
> > 
> > 
> > > 
> > > On 2012/07/23 22:22:36, rvargas wrote:
> > > > We should treat this one some other way. This is not waiting for the
disk
> > > cache,
> > > > it is waiting for the http cache.
> > > 
> > 
> 
> Done.
> 
>
http://codereview.chromium.org/10736066/diff/32088/net/http/http_cache_transa...
> net/http/http_cache_transaction.cc:1228: ReportCacheActionStart();
> Per discussion on the phone, we are in agreement on this now.
> 
> On 2012/07/24 03:10:38, rvargas wrote:
> > On 2012/07/24 01:03:12, tburkard wrote:
> > > That doesn't matter for the purpose of what I'm trying to do.
> > > 
> > > I only care about how much time is spent in cache actions.  Whether other
> > cache
> > > actions can run in parallel is not important for my metrics.
> > 
> > I think it matters.
> > 
> > If we only care about a single request, then fine, because we can say x% of
> the
> > time was spent on cache related things.
> > 
> > On the other hand, if we look at 100 requests that are all part of the same
> > page, it makes a difference what part of the time directly affects the total
> > time or not. You may end up with total cache time that is 150% the total
time
> > used for loading the page (or any percentage for that matter). And then?
> > 
> > Maybe it doesn't really matter, I'm wondering what we would be able to
extract
> > from the data though.
> > 
> > 
> > > 
> > > On 2012/07/23 22:22:36, rvargas wrote:
> > > > This is where we start getting confusing results.
> > > > 
> > > > Previous times are blocking in that no other request can progress if it
is
> > in
> > > > the same state (they can make progress in other states). 
> > > > 
> > > > States after this one are non blocking so multiple requests can be
reading
> > the
> > > > response form the cache and all can make progress at the same time.
> > > > 
> > > > So a single notification for both things is not appropriate.
> > > 
> >
> 
>
http://codereview.chromium.org/10736066/diff/32088/net/http/http_cache_unitte...
> File net/http/http_cache_unittest.cc (right):
> 
>
http://codereview.chromium.org/10736066/diff/32088/net/http/http_cache_unitte...
> net/http/http_cache_unittest.cc:4952: }
> On 2012/07/24 03:10:38, rvargas wrote:
> > On 2012/07/24 01:03:12, tburkard wrote:
> > > Since this is just instrumentation for testing, I don't think this is a
huge
> > > priority right now.
> > 
> > I have to disagree. This is a change in the interface of production code...
it
> > deserves a unit test, and the time to add it is with the same CL that adds
the
> > code.
> > 
> > > On 2012/07/23 22:22:36, rvargas wrote:
> > > > We need a unit test for the delegate.
> > > 
> > 
> 
> Done.
> 
>
http://codereview.chromium.org/10736066/diff/32088/net/url_request/url_reques...
> File net/url_request/url_request_http_job.cc (right):
> 
>
http://codereview.chromium.org/10736066/diff/32088/net/url_request/url_reques...
> net/url_request/url_request_http_job.cc:93:
> NetworkDelegate::CACHE_WAIT_STATE_DONE);
> On 2012/07/24 03:10:38, rvargas wrote:
> > On 2012/07/24 01:21:25, Matt Menke wrote:
> > > On 2012/07/24 01:03:12, tburkard wrote:
> > > > Excellent comment -- I was not aware that a request could have multiple
> > jobs. 
> > > I
> > > > originally had this class keep track of how many cache actions were
> pending,
> > > and
> > > > then remove the appropriate amount.  Matt asked me to remove this and
put
> > that
> > > > logic into a class upstream.  It seems I have to put this back in,
right?
> > > > On 2012/07/23 22:22:36, rvargas wrote:
> > > > > This generates a notification when the first job goes away... what
> happens
> > > > with
> > > > > subsequent jobs for the same request?
> > > > 
> > > 
> > > I actually thought about this, surprisingly ;).  We always orphan the old
> job
> > > before creating the new one (Since we have to detach the request, to
prevent
> > > subsequent calls from the Job back to the request), so it should all work
> out
> > > fine.
> > 
> > Isn't _STATE_DONE supposed to indicate that all processing with this request
> is
> > complete/aborted? It is weird to generate more start/stop notifications (for
> the
> > next job) after done is generated.
> > 
> > If we want that model then the name should be changed.
> 
> Done.

[willchan no longer on Chromium, 2012-07-24 23:17:13]

https://chromiumcodereview.appspot.com/10736066/diff/45013/chrome/browser/net...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://chromiumcodereview.appspot.com/10736066/diff/45013/chrome/browser/net...
chrome/browser/net/chrome_network_delegate.cc:138:
cache_stats_(chrome_browser_net::CacheStats::GetInstance()) {
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/P9tWR4mlUpE...
TL;DR: DestructionObserver results in problems due to shutdown ordering. We
generally try to avoid it. What's better is for you to provide defined
ownership. Put it in IOThread::Globals since it is a global that lives on the IO
thread. That way, it will be destroyed properly on shutdown when the IO thread
goes away. ProfileIOData has a pointer to IOThread::Globals on the IO thread, so
you can pass in the CacheStats object to ChromeNetworkDelegate via the
constructor.

It's possible that you could be cleaner and avoid having to check OTR in
CacheStats by simply having the ChromeNetworkDelegate for
OffTheRecordProfileIOData constructed with a NULL CacheStats so it doesn't try
to update it. Doesn't matter too much.

[willchan no longer on Chromium, 2012-07-25 19:26:41]

Sorry, I didn't get to my nits last night. I'm going to send more after lunch.

https://chromiumcodereview.appspot.com/10736066/diff/45013/chrome/browser/net...
File chrome/browser/net/cache_stats.cc (right):

https://chromiumcodereview.appspot.com/10736066/diff/45013/chrome/browser/net...
chrome/browser/net/cache_stats.cc:99: std::pair<int, int> render_view_id_;
Annoying nit: struct members don't have trailing underscores, only class member
variables do.

https://chromiumcodereview.appspot.com/10736066/diff/45013/chrome/browser/net...
chrome/browser/net/cache_stats.cc:174: CacheStats::~CacheStats() {
Delete the TabLoadStatsMap elements? Otherwise we may get (benign) leaks and
valgrind will complain.

https://chromiumcodereview.appspot.com/10736066/diff/45013/chrome/browser/net...
chrome/browser/net/cache_stats.cc:197: it->second->timer_.Stop();
Is this line necessary? Deleting it should stop the timer.

[tburkard, 2012-07-25 20:06:51]

Incorporated all comments now, including Ricardo's last outstanding concern.

Please re-review.

http://codereview.chromium.org/10736066/diff/45013/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.cc (right):

http://codereview.chromium.org/10736066/diff/45013/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:99: std::pair<int, int> render_view_id_;
On 2012/07/25 19:26:41, willchan wrote:
> Annoying nit: struct members don't have trailing underscores, only class
member
> variables do.

Done.

http://codereview.chromium.org/10736066/diff/45013/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:174: CacheStats::~CacheStats() {
On 2012/07/25 19:26:41, willchan wrote:
> Delete the TabLoadStatsMap elements? Otherwise we may get (benign) leaks and
> valgrind will complain.

Done.

http://codereview.chromium.org/10736066/diff/45013/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:197: it->second->timer_.Stop();
On 2012/07/25 19:26:41, willchan wrote:
> Is this line necessary? Deleting it should stop the timer.

Done.

http://codereview.chromium.org/10736066/diff/45013/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_network_delegate.cc (right):

http://codereview.chromium.org/10736066/diff/45013/chrome/browser/net/chrome_...
chrome/browser/net/chrome_network_delegate.cc:138:
cache_stats_(chrome_browser_net::CacheStats::GetInstance()) {
On 2012/07/24 23:17:13, willchan wrote:
>
https://groups.google.com/a/chromium.org/forum/#%21msg/chromium-dev/P9tWR4mlU...
> TL;DR: DestructionObserver results in problems due to shutdown ordering. We
> generally try to avoid it. What's better is for you to provide defined
> ownership. Put it in IOThread::Globals since it is a global that lives on the
IO
> thread. That way, it will be destroyed properly on shutdown when the IO thread
> goes away. ProfileIOData has a pointer to IOThread::Globals on the IO thread,
so
> you can pass in the CacheStats object to ChromeNetworkDelegate via the
> constructor.
> 
> It's possible that you could be cleaner and avoid having to check OTR in
> CacheStats by simply having the ChromeNetworkDelegate for
> OffTheRecordProfileIOData constructed with a NULL CacheStats so it doesn't try
> to update it. Doesn't matter too much.

Done.

[willchan no longer on Chromium, 2012-07-26 01:39:37]

I think I finally went through and got all my nitpicks done.

I didn't review the cache timing and histograms. I'm deferring to rvargas on
that.

http://codereview.chromium.org/10736066/diff/56009/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.cc (right):

http://codereview.chromium.org/10736066/diff/56009/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:59: static CacheStats* cache_stats =
io_thread->globals()->cache_stats.get();
Why bother with the static?

http://codereview.chromium.org/10736066/diff/56009/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.h (right):

http://codereview.chromium.org/10736066/diff/56009/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:77: void TimerCb(TabLoadStats* stats, int
timer_index);
No abbreviation please, TimerCallback.

http://codereview.chromium.org/10736066/diff/56009/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:113: bool is_profile_otr_;
Is this still needed?

http://codereview.chromium.org/10736066/diff/56009/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_network_delegate.h (right):

http://codereview.chromium.org/10736066/diff/56009/chrome/browser/net/chrome_...
chrome/browser/net/chrome_network_delegate.h:124:
chrome_browser_net::CacheStats* cache_stats_;
Document that this an IOThread global, so it should outlive
ChromeNetworkDelegate.

http://codereview.chromium.org/10736066/diff/56009/chrome/browser/profiles/of...
File chrome/browser/profiles/off_the_record_profile_io_data.h (right):

http://codereview.chromium.org/10736066/diff/56009/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_io_data.h:90: virtual
chrome_browser_net::CacheStats* GetCacheStats(
Move this to the private section.

http://codereview.chromium.org/10736066/diff/56009/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/10736066/diff/56009/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.h:136:
AppRequestContext(chrome_browser_net::CacheStats* cache_stats);
explicit

http://codereview.chromium.org/10736066/diff/56009/net/http/http_network_laye...
File net/http/http_network_layer.cc (right):

http://codereview.chromium.org/10736066/diff/56009/net/http/http_network_laye...
net/http/http_network_layer.cc:106: HttpTransactionDelegate* delegat) {
delegate

http://codereview.chromium.org/10736066/diff/56009/net/url_request/url_reques...
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/10736066/diff/56009/net/url_request/url_reques...
net/url_request/url_request_job.h:290: virtual void OnDetachRequest() {}
I think this can be private, right?

[tburkard, 2012-07-26 02:10:39]

http://codereview.chromium.org/10736066/diff/56009/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.cc (right):

http://codereview.chromium.org/10736066/diff/56009/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:59: static CacheStats* cache_stats =
io_thread->globals()->cache_stats.get();
On 2012/07/26 01:39:37, willchan wrote:
> Why bother with the static?

Done.

http://codereview.chromium.org/10736066/diff/56009/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.h (right):

http://codereview.chromium.org/10736066/diff/56009/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:77: void TimerCb(TabLoadStats* stats, int
timer_index);
On 2012/07/26 01:39:37, willchan wrote:
> No abbreviation please, TimerCallback.

Done.

http://codereview.chromium.org/10736066/diff/56009/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:113: bool is_profile_otr_;
Yes, so that WebContents observed events that are OTR are ignored.
On 2012/07/26 01:39:37, willchan wrote:
> Is this still needed?

http://codereview.chromium.org/10736066/diff/56009/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_network_delegate.h (right):

http://codereview.chromium.org/10736066/diff/56009/chrome/browser/net/chrome_...
chrome/browser/net/chrome_network_delegate.h:124:
chrome_browser_net::CacheStats* cache_stats_;
On 2012/07/26 01:39:37, willchan wrote:
> Document that this an IOThread global, so it should outlive
> ChromeNetworkDelegate.

Done.

http://codereview.chromium.org/10736066/diff/56009/chrome/browser/profiles/of...
File chrome/browser/profiles/off_the_record_profile_io_data.h (right):

http://codereview.chromium.org/10736066/diff/56009/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_io_data.h:90: virtual
chrome_browser_net::CacheStats* GetCacheStats(
On 2012/07/26 01:39:37, willchan wrote:
> Move this to the private section.

Done.

http://codereview.chromium.org/10736066/diff/56009/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/10736066/diff/56009/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.h:136:
AppRequestContext(chrome_browser_net::CacheStats* cache_stats);
On 2012/07/26 01:39:37, willchan wrote:
> explicit

Done.

http://codereview.chromium.org/10736066/diff/56009/net/http/http_network_laye...
File net/http/http_network_layer.cc (right):

http://codereview.chromium.org/10736066/diff/56009/net/http/http_network_laye...
net/http/http_network_layer.cc:106: HttpTransactionDelegate* delegat) {
On 2012/07/26 01:39:37, willchan wrote:
> delegate

Done.

http://codereview.chromium.org/10736066/diff/56009/net/url_request/url_reques...
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/10736066/diff/56009/net/url_request/url_reques...
net/url_request/url_request_job.h:290: virtual void OnDetachRequest() {}
no, because the derived class needs to override this.

On 2012/07/26 01:39:37, willchan wrote:
> I think this can be private, right?

[rvargas (doing something else), 2012-07-26 02:13:43]

http://codereview.chromium.org/10736066/diff/32088/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.cc (right):

http://codereview.chromium.org/10736066/diff/32088/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:88: PerRequestNumActiveMap
per_request_num_active_;
On 2012/07/24 01:03:12, tburkard wrote:
> On 2012/07/23 22:22:36, rvargas wrote:
> > This deserves a comment.
> 
> Done.

I meant the map.

http://codereview.chromium.org/10736066/diff/32088/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:215: entry->second++;
On 2012/07/24 22:23:26, tburkard wrote:
> On 2012/07/24 03:10:38, rvargas wrote:
> > On 2012/07/24 01:03:12, tburkard wrote:
> > > Not necessarily, you could imagine at some point having two separate cache
> > > actions pending for a given URLRequest at the same time.
> > 
> > We should wait until we have the need to support that use case (if ever). At
> > this point it seems better to detect that as a problem.
> > 
> > > On 2012/07/23 22:22:36, rvargas wrote:
> > > > Isn't second > 1 a bug?
> > > 
> > 
> 
> Done.

Actually, using a boolean is better. In fact, a set instead of a map would be
even better. Right now the code is happy keeping counters up and down except
that there is a dcheck that the counter will never go beyond 1.

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.cc (right):

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:44: namespace {
nit: Do you mind moving this namespace out of the file namespace?

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:129: if (!validated_url.SchemeIs("http"))
no https?

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.h (right):

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:46: // This class collects UMA stats about
cache performance.  It is a singleton
nit: outdated comment.

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:48: class CacheStats {
I'm afraid the class name (and file name) will be obsolete pretty soon... when
we start recording network numbers, this will not be cache specific.

I guess the renaming can be done later, but it may be simpler to do it now.

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:64: typedef std::map<std::pair<int, int>,
TabLoadStats*> TabLoadStatsMap;
This map deserves a comment.

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:102: OVERRIDE;
nit: I believe this cannot be in a line by itself.

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:111: bool is_profile_otr_;
nit: is_otr_profile_

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/profiles/of...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_io_data.cc:323:
chrome_browser_net::CacheStats* OffTheRecordProfileIOData::GetCacheStats()
The order on the cc should follow the order on the header.

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/profiles/of...
File chrome/browser/profiles/off_the_record_profile_io_data.h (right):

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_io_data.h:89: protected:
why protected?

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.cc:596: chrome_browser_net::CacheStats*
ProfileIOData::GetCacheStats() const {
ditto

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.cc:597: DCHECK(profile_params_.get());
We usually don't dcheck things that will crash the next line.

http://codereview.chromium.org/10736066/diff/56005/net/http/http_cache_unitte...
File net/http/http_cache_unittest.cc (right):

http://codereview.chromium.org/10736066/diff/56005/net/http/http_cache_unitte...
net/http/http_cache_unittest.cc:67: action_in_progress_(false){
nit: missing space before {

http://codereview.chromium.org/10736066/diff/56005/net/http/http_cache_unitte...
net/http/http_cache_unittest.cc:101: void
RunTransactionTestWithRequestAndLogAndNumDelegateTransactions(
nit: ...LogAndDelegate(

http://codereview.chromium.org/10736066/diff/56005/net/http/http_cache_unitte...
net/http/http_cache_unittest.cc:107: int num_delegate_transactions) {
num_actions ? if anything, they are not transactions.

http://codereview.chromium.org/10736066/diff/56005/net/http/http_cache_unitte...
net/http/http_cache_unittest.cc:5003: // write to the cache
nit: Starts with uppercase and ends with period. Same below.

http://codereview.chromium.org/10736066/diff/56005/net/http/http_transaction_...
File net/http/http_transaction_delegate.h (right):

http://codereview.chromium.org/10736066/diff/56005/net/http/http_transaction_...
net/http/http_transaction_delegate.h:13: // receive notifications when
transactions on an underlying cache start and
transactions __ start and stop? needs rewording.

[rvargas (doing something else), 2012-07-26 02:14:34]

note to self: reviewed @23.

[willchan no longer on Chromium, 2012-07-26 02:15:06]

http://codereview.chromium.org/10736066/diff/56009/net/url_request/url_reques...
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/10736066/diff/56009/net/url_request/url_reques...
net/url_request/url_request_job.h:290: virtual void OnDetachRequest() {}
On 2012/07/26 02:10:40, tburkard wrote:
> no, because the derived class needs to override this.
> 
> On 2012/07/26 01:39:37, willchan wrote:
> > I think this can be private, right?
> 

public/protected/private only indicates who can *call* the function. A virtual
is *always* overridable by derived classes.

[tburkard, 2012-07-26 02:50:53]

http://codereview.chromium.org/10736066/diff/32088/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.cc (right):

http://codereview.chromium.org/10736066/diff/32088/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:215: entry->second++;
On 2012/07/26 02:13:43, rvargas wrote:
> On 2012/07/24 22:23:26, tburkard wrote:
> > On 2012/07/24 03:10:38, rvargas wrote:
> > > On 2012/07/24 01:03:12, tburkard wrote:
> > > > Not necessarily, you could imagine at some point having two separate
cache
> > > > actions pending for a given URLRequest at the same time.
> > > 
> > > We should wait until we have the need to support that use case (if ever).
At
> > > this point it seems better to detect that as a problem.
> > > 
> > > > On 2012/07/23 22:22:36, rvargas wrote:
> > > > > Isn't second > 1 a bug?
> > > > 
> > > 
> > 
> > Done.
> 
> Actually, using a boolean is better. In fact, a set instead of a map would be
> even better. Right now the code is happy keeping counters up and down except
> that there is a dcheck that the counter will never go beyond 1.

Done.

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.cc (right):

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:44: namespace {
On 2012/07/26 02:13:43, rvargas wrote:
> nit: Do you mind moving this namespace out of the file namespace?

Done.

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:129: if (!validated_url.SchemeIs("http"))
Wanted to focus on http first, since a lot of sites don't permit caching for
https.  Can add this later.

On 2012/07/26 02:13:43, rvargas wrote:
> no https?

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.h (right):

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:46: // This class collects UMA stats about
cache performance.  It is a singleton
On 2012/07/26 02:13:43, rvargas wrote:
> nit: outdated comment.

Done.

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:48: class CacheStats {
yes
On 2012/07/26 02:13:43, rvargas wrote:
> I'm afraid the class name (and file name) will be obsolete pretty soon... when
> we start recording network numbers, this will not be cache specific.
> 
> I guess the renaming can be done later, but it may be simpler to do it now.

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:64: typedef std::map<std::pair<int, int>,
TabLoadStats*> TabLoadStatsMap;
On 2012/07/26 02:13:43, rvargas wrote:
> This map deserves a comment.

Done.

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:102: OVERRIDE;
On 2012/07/26 02:13:43, rvargas wrote:
> nit: I believe this cannot be in a line by itself.

Done.

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:111: bool is_profile_otr_;
On 2012/07/26 02:13:43, rvargas wrote:
> nit: is_otr_profile_

Done.

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/profiles/of...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_io_data.cc:323:
chrome_browser_net::CacheStats* OffTheRecordProfileIOData::GetCacheStats()
It is.  It comes after CreateFtpProtocolHandler in both.  Or what do you mean?
On 2012/07/26 02:13:43, rvargas wrote:
> The order on the cc should follow the order on the header.

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/profiles/of...
File chrome/browser/profiles/off_the_record_profile_io_data.h (right):

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_io_data.h:89: protected:
On 2012/07/26 02:13:43, rvargas wrote:
> why protected?

Done.

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.cc:596: chrome_browser_net::CacheStats*
ProfileIOData::GetCacheStats() const {
On 2012/07/26 02:13:43, rvargas wrote:
> ditto

Done.

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.cc:597: DCHECK(profile_params_.get());
On 2012/07/26 02:13:43, rvargas wrote:
> We usually don't dcheck things that will crash the next line.

Done.

http://codereview.chromium.org/10736066/diff/56005/net/http/http_cache_unitte...
File net/http/http_cache_unittest.cc (right):

http://codereview.chromium.org/10736066/diff/56005/net/http/http_cache_unitte...
net/http/http_cache_unittest.cc:67: action_in_progress_(false){
On 2012/07/26 02:13:43, rvargas wrote:
> nit: missing space before {

Done.

http://codereview.chromium.org/10736066/diff/56005/net/http/http_cache_unitte...
net/http/http_cache_unittest.cc:101: void
RunTransactionTestWithRequestAndLogAndNumDelegateTransactions(
I'd rather keep it so to disambiguate "RequestLog" from "Request, Log"

On 2012/07/26 02:13:43, rvargas wrote:
> nit: ...LogAndDelegate(

http://codereview.chromium.org/10736066/diff/56005/net/http/http_cache_unitte...
net/http/http_cache_unittest.cc:107: int num_delegate_transactions) {
On 2012/07/26 02:13:43, rvargas wrote:
> num_actions ? if anything, they are not transactions.

Done.

http://codereview.chromium.org/10736066/diff/56005/net/http/http_cache_unitte...
net/http/http_cache_unittest.cc:5003: // write to the cache
On 2012/07/26 02:13:43, rvargas wrote:
> nit: Starts with uppercase and ends with period. Same below.

Done.

http://codereview.chromium.org/10736066/diff/56005/net/http/http_transaction_...
File net/http/http_transaction_delegate.h (right):

http://codereview.chromium.org/10736066/diff/56005/net/http/http_transaction_...
net/http/http_transaction_delegate.h:13: // receive notifications when
transactions on an underlying cache start and
On 2012/07/26 02:13:43, rvargas wrote:
> transactions __ start and stop? needs rewording.

Done.

http://codereview.chromium.org/10736066/diff/56009/net/url_request/url_reques...
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/10736066/diff/56009/net/url_request/url_reques...
net/url_request/url_request_job.h:290: virtual void OnDetachRequest() {}
On 2012/07/26 02:15:07, willchan wrote:
> On 2012/07/26 02:10:40, tburkard wrote:
> > no, because the derived class needs to override this.
> > 
> > On 2012/07/26 01:39:37, willchan wrote:
> > > I think this can be private, right?
> > 
> 
> public/protected/private only indicates who can *call* the function. A virtual
> is *always* overridable by derived classes.

Done.

[tburkard, 2012-07-26 02:51:19]

All comments by everyone incorporated, please re-review.

[rvargas (doing something else), 2012-07-26 18:47:32]

Almost there. (@32)

I think you missed Will's comment about not needing is_otr_profile_.

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/profiles/of...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_io_data.cc:323:
chrome_browser_net::CacheStats* OffTheRecordProfileIOData::GetCacheStats()
On 2012/07/26 02:50:54, tburkard wrote:
> It is.  It comes after CreateFtpProtocolHandler in both.  Or what do you mean?
> On 2012/07/26 02:13:43, rvargas wrote:
> > The order on the cc should follow the order on the header.
> 

It is now, not when I wrote the comment (declaration moved after rev 23).

http://codereview.chromium.org/10736066/diff/56005/net/http/http_cache_unitte...
File net/http/http_cache_unittest.cc (right):

http://codereview.chromium.org/10736066/diff/56005/net/http/http_cache_unitte...
net/http/http_cache_unittest.cc:101: void
RunTransactionTestWithRequestAndLogAndNumDelegateTransactions(
On 2012/07/26 02:50:54, tburkard wrote:
> I'd rather keep it so to disambiguate "RequestLog" from "Request, Log"

I don't understand what you mean.

RunTransactionTestWithRequestAndLogAndDelegate
RunTransactionTestWithDelegate

I don't like AndNumDelegateTransactions because there is no such thing as
"number of delegate transactions"... the test adds a delegate... saying just
"Delegate" is specific enough.


> 
> On 2012/07/26 02:13:43, rvargas wrote:
> > nit: ...LogAndDelegate(
>

http://codereview.chromium.org/10736066/diff/46124/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.cc (right):

http://codereview.chromium.org/10736066/diff/46124/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:217: if
(stats->active_requests.count(&request) > 0) {
DCHECK this instead?

http://codereview.chromium.org/10736066/diff/46124/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.h (right):

http://codereview.chromium.org/10736066/diff/46124/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:65: // representing that renderer's load
statistics.
nit: move the comment one line down (to the map, not the struct).

http://codereview.chromium.org/10736066/diff/46124/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_url_request_context.h (right):

http://codereview.chromium.org/10736066/diff/46124/chrome/browser/net/chrome_...
chrome/browser/net/chrome_url_request_context.h:79: // be added to CopyFrom.
did you see this?

http://codereview.chromium.org/10736066/diff/46124/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/10736066/diff/46124/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.h:297: virtual
chrome_browser_net::CacheStats* GetCacheStats(
Shouldn't this be before line 273?

http://codereview.chromium.org/10736066/diff/46124/net/url_request/url_reques...
File net/url_request/url_request_http_job.h (right):

http://codereview.chromium.org/10736066/diff/46124/net/url_request/url_reques...
net/url_request/url_request_http_job.h:191: virtual void OnDetachRequest()
OVERRIDE;
nit: add a comment that this comes from URLRequestJob private interface.

[rvargas (doing something else), 2012-07-26 18:56:12]

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.cc (right):

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:129: if (!validated_url.SchemeIs("http"))
On 2012/07/26 02:50:54, tburkard wrote:
> Wanted to focus on http first, since a lot of sites don't permit caching for
> https.  Can add this later.

Note that https goes through exactly the same logic though... and we won't be
able to compare the results with other data that aggregates the two protocols.
Not that there would be a lot to compare.


> 
> On 2012/07/26 02:13:43, rvargas wrote:
> > no https?
>

[tburkard, 2012-07-26 19:12:49]

Ricardo, I did reply to Will's OTR question (see my email from 7:10pm last
night, 3rd item).

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.cc (right):

http://codereview.chromium.org/10736066/diff/56005/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:129: if (!validated_url.SchemeIs("http"))
I am aware of that.  I may add separate histograms for http and https, along
with the other histograms that we discussed that I will add soon once this
change is landed.

On 2012/07/26 18:56:12, rvargas wrote:
> On 2012/07/26 02:50:54, tburkard wrote:
> > Wanted to focus on http first, since a lot of sites don't permit caching for
> > https.  Can add this later.
> 
> Note that https goes through exactly the same logic though... and we won't be
> able to compare the results with other data that aggregates the two protocols.
> Not that there would be a lot to compare.
> 
> 
> > 
> > On 2012/07/26 02:13:43, rvargas wrote:
> > > no https?
> > 
>

http://codereview.chromium.org/10736066/diff/56005/net/http/http_cache_unitte...
File net/http/http_cache_unittest.cc (right):

http://codereview.chromium.org/10736066/diff/56005/net/http/http_cache_unitte...
net/http/http_cache_unittest.cc:101: void
RunTransactionTestWithRequestAndLogAndNumDelegateTransactions(
On 2012/07/26 18:47:33, rvargas wrote:
> On 2012/07/26 02:50:54, tburkard wrote:
> > I'd rather keep it so to disambiguate "RequestLog" from "Request, Log"
> 
> I don't understand what you mean.
> 
> RunTransactionTestWithRequestAndLogAndDelegate
> RunTransactionTestWithDelegate
> 
> I don't like AndNumDelegateTransactions because there is no such thing as
> "number of delegate transactions"... the test adds a delegate... saying just
> "Delegate" is specific enough.
> 
> 
> > 
> > On 2012/07/26 02:13:43, rvargas wrote:
> > > nit: ...LogAndDelegate(
> > 
> 

Done.

http://codereview.chromium.org/10736066/diff/46124/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.cc (right):

http://codereview.chromium.org/10736066/diff/46124/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:217: if
(stats->active_requests.count(&request) > 0) {
No, because when the spinner stops, we clear the entire stats entry for that
renderer.  Therefore, when some URLrequests come back after the spinner stopped,
the fact that those were pending is no longer stored.
On 2012/07/26 18:47:33, rvargas wrote:
> DCHECK this instead?

http://codereview.chromium.org/10736066/diff/46124/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.h (right):

http://codereview.chromium.org/10736066/diff/46124/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:65: // representing that renderer's load
statistics.
On 2012/07/26 18:47:33, rvargas wrote:
> nit: move the comment one line down (to the map, not the struct).

Done.

http://codereview.chromium.org/10736066/diff/46124/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_url_request_context.h (right):

http://codereview.chromium.org/10736066/diff/46124/chrome/browser/net/chrome_...
chrome/browser/net/chrome_url_request_context.h:79: // be added to CopyFrom.
Yes, nothing to be done here though, because it is taken care of in the
constructor (initialization of the newly added variables/state).

On 2012/07/26 18:47:33, rvargas wrote:
> did you see this?

http://codereview.chromium.org/10736066/diff/46124/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/10736066/diff/46124/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.h:297: virtual
chrome_browser_net::CacheStats* GetCacheStats(
On 2012/07/26 18:47:33, rvargas wrote:
> Shouldn't this be before line 273?

Done.

http://codereview.chromium.org/10736066/diff/46124/net/url_request/url_reques...
File net/url_request/url_request_http_job.h (right):

http://codereview.chromium.org/10736066/diff/46124/net/url_request/url_reques...
net/url_request/url_request_http_job.h:191: virtual void OnDetachRequest()
OVERRIDE;
On 2012/07/26 18:47:33, rvargas wrote:
> nit: add a comment that this comes from URLRequestJob private interface. 

Done.

[rvargas (doing something else), 2012-07-26 19:48:58]

Please create a feature request and add the bug number to the CL.

LGTM but wait for other folks.

[mmenke, 2012-07-26 20:01:27]

A couple nits.

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.cc (right):

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:7: #include <vector>
Not needed, since it's in cache_stats.h

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:9: #include "base/hash_tables.h"
Also already in the header.

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:26: using content::ResourceRequestInfo;
nit:  May as well alphabetize

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.h (right):

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:9: #include <string>
Don't see any strings in this file.

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:12: #include "base/basictypes.h"
#include "base/compiler_specific.h" (Needed for OVERRIDE)

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:61: virtual ~CacheStats();
nit:  Constructor / destructor should be the first methods, per Google style
guide.

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:61: virtual ~CacheStats();
Since we aren't inheriting from anything, and nothing inherits from us,
destructor doesn't need to be virtual.

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:88: base::hash_set<const
net::URLRequestContext*> main_request_contexts_;
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:97: virtual ~CacheStatsTabHelper();
nit:  Suggest a line break between constructor/destructor and memory functions.

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:98: virtual void
DidStartProvisionalLoadForFrame(
nit:  "// content::WebContentsObserver implementation"

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:114: bool is_otr_profile_;
DISALLOW_COPY_AND_ASSIGN

[willchan no longer on Chromium, 2012-07-26 20:03:37]

I'm done reviewing. profiles/, net/, c/b/n all LGTM. But wait for mmenke's stamp
too.

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.cc:459: cache_stats_ =
GetCacheStats(io_thread);
Can you pass in io_thread_globals instead of io_thread?

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.h:274: virtual
chrome_browser_net::CacheStats* GetCacheStats(
Can you move this to the virtual interface section below? And make it pure like
all the other virtuals to be consistent.

http://codereview.chromium.org/10736066/diff/53097/net/http/http_cache_unitte...
File net/http/http_cache_unittest.cc (right):

http://codereview.chromium.org/10736066/diff/53097/net/http/http_cache_unitte...
net/http/http_cache_unittest.cc:70: EXPECT_EQ(num_remaining_actions_to_observe_,
0);
You've got the params reversed. Should be expected, and actual.

http://codereview.chromium.org/10736066/diff/53097/net/http/http_cache_unitte...
net/http/http_cache_unittest.cc:80: EXPECT_EQ(action_in_progress_, true);
EXPECT_TRUE.

[tburkard, 2012-07-26 20:52:31]

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.cc:459: cache_stats_ =
GetCacheStats(io_thread);
On 2012/07/26 20:03:38, willchan wrote:
> Can you pass in io_thread_globals instead of io_thread?

Done.

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.h:274: virtual
chrome_browser_net::CacheStats* GetCacheStats(
On 2012/07/26 20:03:38, willchan wrote:
> Can you move this to the virtual interface section below? And make it pure
like
> all the other virtuals to be consistent.

Done.

http://codereview.chromium.org/10736066/diff/53097/net/http/http_cache_unitte...
File net/http/http_cache_unittest.cc (right):

http://codereview.chromium.org/10736066/diff/53097/net/http/http_cache_unitte...
net/http/http_cache_unittest.cc:70: EXPECT_EQ(num_remaining_actions_to_observe_,
0);
On 2012/07/26 20:03:38, willchan wrote:
> You've got the params reversed. Should be expected, and actual.

Done.

http://codereview.chromium.org/10736066/diff/53097/net/http/http_cache_unitte...
net/http/http_cache_unittest.cc:80: EXPECT_EQ(action_in_progress_, true);
On 2012/07/26 20:03:38, willchan wrote:
> EXPECT_TRUE.

Done.

[tburkard, 2012-07-26 21:22:33]

Everything should be addressed now.

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.cc (right):

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:7: #include <vector>
On 2012/07/26 20:01:27, Matt Menke wrote:
> Not needed, since it's in cache_stats.h

Done.

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:9: #include "base/hash_tables.h"
On 2012/07/26 20:01:27, Matt Menke wrote:
> Also already in the header.

Done.

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:26: using content::ResourceRequestInfo;
On 2012/07/26 20:01:27, Matt Menke wrote:
> nit:  May as well alphabetize

Done.

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.h (right):

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:9: #include <string>
On 2012/07/26 20:01:27, Matt Menke wrote:
> Don't see any strings in this file.

Done.

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:12: #include "base/basictypes.h"
On 2012/07/26 20:01:27, Matt Menke wrote:
> #include "base/compiler_specific.h" (Needed for OVERRIDE)

Done.

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:61: virtual ~CacheStats();
On 2012/07/26 20:01:27, Matt Menke wrote:
> nit:  Constructor / destructor should be the first methods, per Google style
> guide.

Done.

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:61: virtual ~CacheStats();
On 2012/07/26 20:01:27, Matt Menke wrote:
> Since we aren't inheriting from anything, and nothing inherits from us,
> destructor doesn't need to be virtual.

Done.

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:88: base::hash_set<const
net::URLRequestContext*> main_request_contexts_;
On 2012/07/26 20:01:27, Matt Menke wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:97: virtual ~CacheStatsTabHelper();
On 2012/07/26 20:01:27, Matt Menke wrote:
> nit:  Suggest a line break between constructor/destructor and memory
functions.

Done.

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:98: virtual void
DidStartProvisionalLoadForFrame(
On 2012/07/26 20:01:27, Matt Menke wrote:
> nit:  "// content::WebContentsObserver implementation"

Done.

http://codereview.chromium.org/10736066/diff/53097/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:114: bool is_otr_profile_;
On 2012/07/26 20:01:27, Matt Menke wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

[mmenke, 2012-07-27 19:03:53]

http://codereview.chromium.org/10736066/diff/54202/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.cc (right):

http://codereview.chromium.org/10736066/diff/54202/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:41: bool GetRenderView(const net::URLRequest&
request,
Suggest a comment that this can only be called on the IO thread, or maybe just a
DCHECK.

http://codereview.chromium.org/10736066/diff/54202/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:55: if (io_thread)
Is this if really needed?

http://codereview.chromium.org/10736066/diff/54202/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:108: base::Timer timer;
Might want to just make this a base::ResourceRequestInfo::OneShotTimer.  Think
it's a little clearer than "timer(false, false)".

http://codereview.chromium.org/10736066/diff/54202/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:108: base::Timer timer;
The Google style guide requires non-POD types to be classes rather than structs.
 Not sure that something that owns a timer can be considered POD.

http://codereview.chromium.org/10736066/diff/54202/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:137: }
nit:  This should be up with the constructor, to match the order in the header
file.

http://codereview.chromium.org/10736066/diff/54202/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:181: if (tab_load_stats_.count(render_view_id)
< 1)
Currently, this code could result in always doing 2 or 3 searches through the
map.  It shouldn't be a huge map, so this is probably fine as-is, but could
reduce it to 1 or 2 by using std::find and not doing another lookup after
inserting the new TabLoadStats.

http://codereview.chromium.org/10736066/diff/54202/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:203: if
(main_request_contexts_.count(request.context()) < 1)
Can this happen?  We should delete the request strictly before we delete its
context, right?  Otherwise, we have bigger problems.

http://codereview.chromium.org/10736066/diff/54202/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:307: DCHECK(cache_fraction_percentage >= 0 &&
cache_fraction_percentage < 100);
I'd suggest using <= 100.  Yes, in general, it should not be 100, but... 
Suppose have a lot of contention, and get the initial cache read start event
before the spinner starts (Since the cache read start event never goes through
the UI thread, this is possible).

http://codereview.chromium.org/10736066/diff/54202/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:311:
base::TimeDelta::FromMilliseconds(kStatsCollectionTimesMs[index + 1]) <
This seems a little weird for final histograms.  final_histograms_[0]
("FractionCacheUseFinalPLT_500") will be recording from 0 to 1000 milliseconds.

My suggestion is to make the function take the index, and to calculate the index
only for final_histograms.  For the other histograms just pass in |timer_index|.
 For the final histogram, use the same loop, but with index instead of [index+1]
as the index.

http://codereview.chromium.org/10736066/diff/54202/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.h (right):

http://codereview.chromium.org/10736066/diff/54202/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:70: // Gets TabLoadStats for a given render
view.
nit:  RenderView.

http://codereview.chromium.org/10736066/diff/54202/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:74: // Sets a timer for a given tab to collect
stats.  timer_index indicates
nit:  |timer_index|.

http://codereview.chromium.org/10736066/diff/54202/net/http/http_cache_transa...
File net/http/http_cache_transaction.h (right):

http://codereview.chromium.org/10736066/diff/54202/net/http/http_cache_transa...
net/http/http_cache_transaction.h:378: HttpTransactionDelegate* delegate_;
Suggest you call this transaction_delegate_, to make it clearer this is not the
NetworkDelegate.

http://codereview.chromium.org/10736066/diff/54202/net/http/http_cache_unitte...
File net/http/http_cache_unittest.cc (right):

http://codereview.chromium.org/10736066/diff/54202/net/http/http_cache_unitte...
net/http/http_cache_unittest.cc:99: static const int kNoDelegateTransactionCheck
= -1;
nit:  static not needed, since we're in an anonymous namespace.

http://codereview.chromium.org/10736066/diff/54202/net/http/http_cache_unitte...
net/http/http_cache_unittest.cc:112: scoped_ptr<net::HttpTransaction> trans;
The transaction should be destroyed after the delegate.  Suggest you declare it
right above initialization, per Google style.

[tburkard, 2012-07-27 23:25:40]

http://codereview.chromium.org/10736066/diff/54202/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.cc (right):

http://codereview.chromium.org/10736066/diff/54202/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:41: bool GetRenderView(const net::URLRequest&
request,
On 2012/07/27 19:03:54, Matt Menke wrote:
> Suggest a comment that this can only be called on the IO thread, or maybe just
a
> DCHECK.

Done.

http://codereview.chromium.org/10736066/diff/54202/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:55: if (io_thread)
Yes, otherwise, some tests fail!
On 2012/07/27 19:03:54, Matt Menke wrote:
> Is this if really needed?

http://codereview.chromium.org/10736066/diff/54202/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:108: base::Timer timer;
I wanted to use a oneshot timer at first, but it doesn't work, because it only
lets me run a "Run" method without any bound arguments, like I need to (see
arguments passed to the TimerCallback).

On 2012/07/27 19:03:54, Matt Menke wrote:
> Might want to just make this a base::ResourceRequestInfo::OneShotTimer.  Think
> it's a little clearer than "timer(false, false)".

http://codereview.chromium.org/10736066/diff/54202/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:108: base::Timer timer;
base::TimeTicks/TimeDelta are probably also not POD, since they have a
constructor.  I think these are small enough though (and especially
TimeTicks/TimeDelta is commonly used in structs, and this timer is similar), so
that it should be ok.
On 2012/07/27 19:03:54, Matt Menke wrote:
> The Google style guide requires non-POD types to be classes rather than
structs.
>  Not sure that something that owns a timer can be considered POD.

http://codereview.chromium.org/10736066/diff/54202/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:137: }
On 2012/07/27 19:03:54, Matt Menke wrote:
> nit:  This should be up with the constructor, to match the order in the header
> file.

Done.

http://codereview.chromium.org/10736066/diff/54202/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:181: if (tab_load_stats_.count(render_view_id)
< 1)
Reduced by 1, since I don't want to construct TabLoadStats unnecessarily each
time.

http://codereview.chromium.org/10736066/diff/54202/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:203: if
(main_request_contexts_.count(request.context()) < 1)
Not exactly sure what you mean.. but I put the check first, which is where it
should go, so that we don't bother doing any operations if we are in a request
context we don't care about.
On 2012/07/27 19:03:54, Matt Menke wrote:
> Can this happen?  We should delete the request strictly before we delete its
> context, right?  Otherwise, we have bigger problems.

http://codereview.chromium.org/10736066/diff/54202/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:307: DCHECK(cache_fraction_percentage >= 0 &&
cache_fraction_percentage < 100);
On 2012/07/27 19:03:54, Matt Menke wrote:
> I'd suggest using <= 100.  Yes, in general, it should not be 100, but... 
> Suppose have a lot of contention, and get the initial cache read start event
> before the spinner starts (Since the cache read start event never goes through
> the UI thread, this is possible).

Done.

http://codereview.chromium.org/10736066/diff/54202/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:311:
base::TimeDelta::FromMilliseconds(kStatsCollectionTimesMs[index + 1]) <
Fixed it slightly differently.  Put the timer_index into the struct, and just
using that for both histogram types.
On 2012/07/27 19:03:54, Matt Menke wrote:
> This seems a little weird for final histograms.  final_histograms_[0]
> ("FractionCacheUseFinalPLT_500") will be recording from 0 to 1000
milliseconds.
> 
> My suggestion is to make the function take the index, and to calculate the
index
> only for final_histograms.  For the other histograms just pass in
|timer_index|.
>  For the final histogram, use the same loop, but with index instead of
[index+1]
> as the index.

http://codereview.chromium.org/10736066/diff/54202/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.h (right):

http://codereview.chromium.org/10736066/diff/54202/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:70: // Gets TabLoadStats for a given render
view.
On 2012/07/27 19:03:54, Matt Menke wrote:
> nit:  RenderView.

Done.

http://codereview.chromium.org/10736066/diff/54202/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.h:74: // Sets a timer for a given tab to collect
stats.  timer_index indicates
On 2012/07/27 19:03:54, Matt Menke wrote:
> nit:  |timer_index|.

Done.

http://codereview.chromium.org/10736066/diff/54202/net/http/http_cache_transa...
File net/http/http_cache_transaction.h (right):

http://codereview.chromium.org/10736066/diff/54202/net/http/http_cache_transa...
net/http/http_cache_transaction.h:378: HttpTransactionDelegate* delegate_;
On 2012/07/27 19:03:54, Matt Menke wrote:
> Suggest you call this transaction_delegate_, to make it clearer this is not
the
> NetworkDelegate.

Done.

http://codereview.chromium.org/10736066/diff/54202/net/http/http_cache_unitte...
File net/http/http_cache_unittest.cc (right):

http://codereview.chromium.org/10736066/diff/54202/net/http/http_cache_unitte...
net/http/http_cache_unittest.cc:99: static const int kNoDelegateTransactionCheck
= -1;
On 2012/07/27 19:03:54, Matt Menke wrote:
> nit:  static not needed, since we're in an anonymous namespace.

Done.

http://codereview.chromium.org/10736066/diff/54202/net/http/http_cache_unitte...
net/http/http_cache_unittest.cc:112: scoped_ptr<net::HttpTransaction> trans;
On 2012/07/27 19:03:54, Matt Menke wrote:
> The transaction should be destroyed after the delegate.  Suggest you declare
it
> right above initialization, per Google style.

Done.

[mmenke, 2012-07-27 23:38:42]

I'll do a quick final pass this weekend.

http://codereview.chromium.org/10736066/diff/54202/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.cc (right):

http://codereview.chromium.org/10736066/diff/54202/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:203: if
(main_request_contexts_.count(request.context()) < 1)
On 2012/07/27 23:25:44, tburkard wrote:
> Not exactly sure what you mean.. but I put the check first, which is where it
> should go, so that we don't bother doing any operations if we are in a request
> context we don't care about.
> On 2012/07/27 19:03:54, Matt Menke wrote:
> > Can this happen?  We should delete the request strictly before we delete its
> > context, right?  Otherwise, we have bigger problems.
> 

Sorry - wrote that comment before I noticed we weren't logging for all contexts,
and forgot about it later.

[mmenke, 2012-07-29 23:45:57]

LGTM

http://codereview.chromium.org/10736066/diff/54202/chrome/browser/net/cache_s...
File chrome/browser/net/cache_stats.cc (right):

http://codereview.chromium.org/10736066/diff/54202/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:108: base::Timer timer;
On 2012/07/27 23:25:44, tburkard wrote:
> base::TimeTicks/TimeDelta are probably also not POD, since they have a
> constructor.  I think these are small enough though (and especially
> TimeTicks/TimeDelta is commonly used in structs, and this timer is similar),
so
> that it should be ok.
> On 2012/07/27 19:03:54, Matt Menke wrote:
> > The Google style guide requires non-POD types to be classes rather than
> structs.
> >  Not sure that something that owns a timer can be considered POD.
> 

I think that since they actually does stuff to the message loop, timers are much
less POD-ish than TimeTicks and TimeDelta, which just have constructors that
initializes them to 0.  Anyways, I don't feel that strongly about it.

http://codereview.chromium.org/10736066/diff/54202/chrome/browser/net/cache_s...
chrome/browser/net/cache_stats.cc:311:
base::TimeDelta::FromMilliseconds(kStatsCollectionTimesMs[index + 1]) <
Like you solution better than mine.

On 2012/07/27 23:25:44, tburkard wrote:
> Fixed it slightly differently.  Put the timer_index into the struct, and just
> using that for both histogram types.
> On 2012/07/27 19:03:54, Matt Menke wrote:
> > This seems a little weird for final histograms.  final_histograms_[0]
> > ("FractionCacheUseFinalPLT_500") will be recording from 0 to 1000
> milliseconds.
> > 
> > My suggestion is to make the function take the index, and to calculate the
> index
> > only for final_histograms.  For the other histograms just pass in
> |timer_index|.
> >  For the final histogram, use the same loop, but with index instead of
> [index+1]
> > as the index.
>

[tburkard, 2012-07-30 18:30:19]

Brett: Could you review the tiny change in webkit?

jam: everything else?

Thanks.

[tburkard, 2012-07-30 18:31:37]

brettw: could you review only the bits in webkit?

Thanks.

[mmenke1, 2012-07-30 18:40:27]

On 2012/07/30 18:30:19, tburkard wrote:
> Brett: Could you review the tiny change in webkit?
> 
> jam: everything else?
> 
> Thanks.

Jam's been very heavily loaded with reviews lately.  Might want to look for
someone else.

[jam, 2012-07-30 19:18:24]

lgtm