Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(371)

Issue 10834313: Add histograms for network activity, and total/cumulative (Closed)

Created:
8 years, 4 months ago by tburkard
Modified:
8 years, 4 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, gavinp+disk_chromium.org, jam, joi+watch-content_chromium.org, pam+watch_chromium.org, darin-cc_chromium.org
Visibility:
Public.

Description

Add histograms for network activity, and total/cumulative histograms for cache/network activity. R=rvargas@chromium.org, mmenke@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=152398

Patch Set 1 #

Total comments: 2

Patch Set 2 : #

Total comments: 21

Patch Set 3 : #

Total comments: 24

Patch Set 4 : #

Patch Set 5 : #

Patch Set 6 : #

Total comments: 14

Patch Set 7 : #

Total comments: 6

Patch Set 8 : #

Total comments: 8

Patch Set 9 : #

Patch Set 10 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+536 lines, -769 lines) Patch
M chrome/browser/io_thread.h View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/io_thread.cc View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/net/cache_stats.h View 1 2 3 4 5 6 7 8 9 1 chunk +0 lines, -127 lines 0 comments Download
M chrome/browser/net/cache_stats.cc View 1 2 3 4 5 6 7 8 9 1 chunk +0 lines, -350 lines 0 comments Download
M chrome/browser/net/chrome_network_delegate.h View 1 2 3 4 5 6 7 8 9 4 chunks +5 lines, -5 lines 0 comments Download
M chrome/browser/net/chrome_network_delegate.cc View 1 2 3 4 5 6 7 8 9 4 chunks +7 lines, -7 lines 0 comments Download
M chrome/browser/net/chrome_url_request_context.h View 1 2 3 4 5 6 7 8 9 3 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/net/chrome_url_request_context.cc View 1 2 3 4 5 6 7 8 9 2 chunks +7 lines, -7 lines 0 comments Download
A + chrome/browser/net/load_time_stats.h View 1 2 3 4 5 6 7 4 chunks +37 lines, -26 lines 0 comments Download
A + chrome/browser/net/load_time_stats.cc View 1 2 3 4 5 6 7 8 10 chunks +261 lines, -140 lines 0 comments Download
M chrome/browser/profiles/off_the_record_profile_io_data.h View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/profiles/off_the_record_profile_io_data.cc View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/profiles/profile_impl_io_data.h View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/profiles/profile_impl_io_data.cc View 1 2 3 4 5 6 7 8 9 3 chunks +4 lines, -4 lines 0 comments Download
M chrome/browser/profiles/profile_io_data.h View 1 2 3 4 5 6 7 8 9 5 chunks +8 lines, -7 lines 0 comments Download
M chrome/browser/profiles/profile_io_data.cc View 1 2 3 4 5 6 7 8 9 4 chunks +7 lines, -7 lines 0 comments Download
M chrome/browser/ui/tab_contents/tab_contents.h View 1 2 3 4 5 6 7 8 9 2 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/ui/tab_contents/tab_contents.cc View 1 2 3 4 5 6 7 8 9 2 chunks +3 lines, -3 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -2 lines 0 comments Download
M content/shell/shell_network_delegate.h View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -2 lines 0 comments Download
M content/shell/shell_network_delegate.cc View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -2 lines 0 comments Download
M net/base/network_delegate.h View 1 2 3 4 5 6 7 8 9 3 chunks +13 lines, -15 lines 0 comments Download
M net/base/network_delegate.cc View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -3 lines 0 comments Download
M net/http/http_cache_transaction.h View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
M net/http/http_cache_transaction.cc View 1 2 3 4 5 6 7 8 9 10 chunks +25 lines, -1 line 0 comments Download
M net/http/http_cache_unittest.cc View 1 2 3 4 5 6 7 8 9 8 chunks +77 lines, -25 lines 0 comments Download
M net/http/http_transaction_delegate.h View 1 2 3 4 5 6 7 8 9 1 chunk +7 lines, -4 lines 0 comments Download
M net/proxy/network_delegate_error_observer_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -2 lines 0 comments Download
M net/proxy/proxy_script_fetcher_impl_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -2 lines 0 comments Download
M net/url_request/url_request_context_builder.cc View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -2 lines 0 comments Download
M net/url_request/url_request_http_job.cc View 1 2 3 4 5 6 7 8 9 3 chunks +37 lines, -7 lines 0 comments Download
M net/url_request/url_request_test_util.h View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -2 lines 0 comments Download
M net/url_request/url_request_test_util.cc View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -2 lines 0 comments Download
M webkit/tools/test_shell/simple_resource_loader_bridge.cc View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -2 lines 0 comments Download

Messages

Total messages: 20 (0 generated)
tburkard
8 years, 4 months ago (2012-08-14 18:22:44 UTC) #1
mmenke
http://codereview.chromium.org/10834313/diff/12001/net/http/http_cache_transaction.cc File net/http/http_cache_transaction.cc (right): http://codereview.chromium.org/10834313/diff/12001/net/http/http_cache_transaction.cc#newcode748 net/http/http_cache_transaction.cc:748: ReportNetworkActionStart(); I'm a bit concerned about mismatched Report*() calls, ...
8 years, 4 months ago (2012-08-15 15:09:53 UTC) #2
tburkard
https://chromiumcodereview.appspot.com/10834313/diff/12001/net/http/http_cache_transaction.cc File net/http/http_cache_transaction.cc (right): https://chromiumcodereview.appspot.com/10834313/diff/12001/net/http/http_cache_transaction.cc#newcode748 net/http/http_cache_transaction.cc:748: ReportNetworkActionStart(); I can add that to the HttpTransactionDelegateImpl, however, ...
8 years, 4 months ago (2012-08-15 16:50:46 UTC) #3
mmenke
I completely forgot to look at cache stats last time. Not completely done with it ...
8 years, 4 months ago (2012-08-15 18:45:41 UTC) #4
mmenke
http://codereview.chromium.org/10834313/diff/12001/net/http/http_cache_transaction.cc File net/http/http_cache_transaction.cc (right): http://codereview.chromium.org/10834313/diff/12001/net/http/http_cache_transaction.cc#newcode748 net/http/http_cache_transaction.cc:748: ReportNetworkActionStart(); On 2012/08/15 18:45:41, Matt Menke wrote: > On ...
8 years, 4 months ago (2012-08-15 19:08:34 UTC) #5
tburkard
https://chromiumcodereview.appspot.com/10834313/diff/12001/net/http/http_cache_transaction.cc File net/http/http_cache_transaction.cc (right): https://chromiumcodereview.appspot.com/10834313/diff/12001/net/http/http_cache_transaction.cc#newcode748 net/http/http_cache_transaction.cc:748: ReportNetworkActionStart(); On 2012/08/15 15:09:53, Matt Menke wrote: > I'm ...
8 years, 4 months ago (2012-08-15 20:42:23 UTC) #6
tburkard
https://chromiumcodereview.appspot.com/10834313/diff/7005/chrome/browser/net/cache_stats.cc File chrome/browser/net/cache_stats.cc (right): https://chromiumcodereview.appspot.com/10834313/diff/7005/chrome/browser/net/cache_stats.cc#newcode129 chrome/browser/net/cache_stats.cc:129: struct PerRequestStatusData { On 2012/08/15 18:45:41, Matt Menke wrote: ...
8 years, 4 months ago (2012-08-15 21:05:56 UTC) #7
rvargas (doing something else)
I should have sent this yesterday. I'm not done with cache_stats.cc http://codereview.chromium.org/10834313/diff/1/net/base/network_delegate.h File net/base/network_delegate.h (right): ...
8 years, 4 months ago (2012-08-15 22:36:49 UTC) #8
tburkard
http://codereview.chromium.org/10834313/diff/1/net/base/network_delegate.h File net/base/network_delegate.h (right): http://codereview.chromium.org/10834313/diff/1/net/base/network_delegate.h#newcode231 net/base/network_delegate.h:231: // CACHE_WAIT_STATE_RESET indicates for a given URLRequest On 2012/08/15 ...
8 years, 4 months ago (2012-08-15 23:00:47 UTC) #9
rvargas (doing something else)
http://codereview.chromium.org/10834313/diff/12001/chrome/browser/net/cache_stats.cc File chrome/browser/net/cache_stats.cc (right): http://codereview.chromium.org/10834313/diff/12001/chrome/browser/net/cache_stats.cc#newcode129 chrome/browser/net/cache_stats.cc:129: struct PerRequestStatusData { On 2012/08/15 23:00:47, tburkard wrote: > ...
8 years, 4 months ago (2012-08-16 01:18:40 UTC) #10
tburkard
http://codereview.chromium.org/10834313/diff/12001/chrome/browser/net/cache_stats.cc File chrome/browser/net/cache_stats.cc (right): http://codereview.chromium.org/10834313/diff/12001/chrome/browser/net/cache_stats.cc#newcode129 chrome/browser/net/cache_stats.cc:129: struct PerRequestStatusData { On 2012/08/16 01:18:40, rvargas wrote: > ...
8 years, 4 months ago (2012-08-16 22:37:43 UTC) #11
rvargas (doing something else)
http://codereview.chromium.org/10834313/diff/10030/chrome/browser/net/cache_stats.cc File chrome/browser/net/cache_stats.cc (right): http://codereview.chromium.org/10834313/diff/10030/chrome/browser/net/cache_stats.cc#newcode235 chrome/browser/net/cache_stats.cc:235: 0, 10000, 500, base::Histogram::kUmaTargetedHistogramFlag)); On 2012/08/16 22:37:43, tburkard wrote: ...
8 years, 4 months ago (2012-08-17 02:36:36 UTC) #12
tburkard
Matt: ping, do you any more concerns? if not, please lgtm. http://codereview.chromium.org/10834313/diff/10030/chrome/browser/net/cache_stats.cc File chrome/browser/net/cache_stats.cc (right): ...
8 years, 4 months ago (2012-08-17 20:21:00 UTC) #13
mmenke
Sorry, was waiting for you to finish responding to ricardo's comments. I'll carefully go over ...
8 years, 4 months ago (2012-08-17 20:34:42 UTC) #14
rvargas (doing something else)
Did something else actually changed between 6 and 7, other than http_cache_unittest? http://codereview.chromium.org/10834313/diff/10030/chrome/browser/net/cache_stats.cc File chrome/browser/net/cache_stats.cc ...
8 years, 4 months ago (2012-08-17 22:05:00 UTC) #15
tburkard
What changed from 6 to 7 besides the cache unit test: - remove the explicit ...
8 years, 4 months ago (2012-08-17 22:10:43 UTC) #16
tburkard
http://codereview.chromium.org/10834313/diff/10030/chrome/browser/net/cache_stats.cc File chrome/browser/net/cache_stats.cc (right): http://codereview.chromium.org/10834313/diff/10030/chrome/browser/net/cache_stats.cc#newcode235 chrome/browser/net/cache_stats.cc:235: 0, 10000, 500, base::Histogram::kUmaTargetedHistogramFlag)); First, there was an error ...
8 years, 4 months ago (2012-08-17 22:39:55 UTC) #17
rvargas (doing something else)
lgtm http://codereview.chromium.org/10834313/diff/6008/chrome/browser/net/load_time_stats.cc File chrome/browser/net/load_time_stats.cc (right): http://codereview.chromium.org/10834313/diff/6008/chrome/browser/net/load_time_stats.cc#newcode334 chrome/browser/net/load_time_stats.cc:334: status_stats->set_num_active(status_stats->num_active() - 1); nit: this (and the previous) ...
8 years, 4 months ago (2012-08-18 02:01:01 UTC) #18
mmenke
LGTM. Sorry I didn't get to this earlier. I should know by now that I ...
8 years, 4 months ago (2012-08-20 16:15:44 UTC) #19
tburkard
8 years, 4 months ago (2012-08-20 19:23:38 UTC) #20
http://codereview.chromium.org/10834313/diff/6008/chrome/browser/net/load_tim...
File chrome/browser/net/load_time_stats.cc (right):

http://codereview.chromium.org/10834313/diff/6008/chrome/browser/net/load_tim...
chrome/browser/net/load_time_stats.cc:334:
status_stats->set_num_active(status_stats->num_active() - 1);
On 2012/08/18 02:01:02, rvargas wrote:
> nit: this (and the previous) line should be a dedicated method

Done.

http://codereview.chromium.org/10834313/diff/6008/chrome/browser/net/load_tim...
chrome/browser/net/load_time_stats.cc:342:
status_stats->set_num_active(status_stats->num_active() + 1);
On 2012/08/18 02:01:02, rvargas wrote:
> nit: IncrementFoo()

Done.

http://codereview.chromium.org/10834313/diff/12019/chrome/browser/net/load_ti...
File chrome/browser/net/load_time_stats.cc (right):

http://codereview.chromium.org/10834313/diff/12019/chrome/browser/net/load_ti...
chrome/browser/net/load_time_stats.cc:116: void UpdateTotalTimes() {
On 2012/08/20 16:15:45, Matt Menke wrote:
> Suggest a DCHECK_GE(num_active_, 0)

Done.

http://codereview.chromium.org/10834313/diff/12019/chrome/browser/net/load_ti...
chrome/browser/net/load_time_stats.cc:413: RequestStatus status =
static_cast<RequestStatus>(status_int);
On 2012/08/20 16:15:45, Matt Menke wrote:
> Don't think you need status and status_int in either of these two loops.

Done.

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

http://codereview.chromium.org/10834313/diff/12019/net/http/http_cache_unitte...
net/http/http_cache_unittest.cc:101: virtual void OnNetworkActionFinish() {
On 2012/08/20 16:15:45, Matt Menke wrote:
> nit:  All these virtual functions should be OVERRIDE.

Done.

http://codereview.chromium.org/10834313/diff/12019/net/http/http_cache_unitte...
net/http/http_cache_unittest.cc:143: num_network_delegate_actions !=
kNoDelegateTransactionCheck) {
Choosing to keep it the way it is.

Powered by Google App Engine
This is Rietveld 408576698