CPM Page Load timing

CPM Page Load timing

Add metric recording in CPM for how long it takes to load a page.

BUG=130212
TEST=included browsertest


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

[Devlin, 2012-08-09 22:39:39]

Please review when time permits :)

[Yoyo Zhou, 2012-08-10 18:34:04]

LGTM with nits.
I saw there were some comments in the design doc about using median for this
metric, because it's liable to be noisy. (And page load time is suspect because
not all pages are alike, but it's still a useful thing to measure, I guess.) Is
that going to appear in a different part of the code?

http://codereview.chromium.org/10829078/diff/4002/chrome/browser/performance_...
File chrome/browser/performance_monitor/performance_monitor.cc (right):

http://codereview.chromium.org/10829078/diff/4002/chrome/browser/performance_...
chrome/browser/performance_monitor/performance_monitor.cc:231:
database_->AddMetric(type, value);
Maybe DCHECK that you're on the right thread?

http://codereview.chromium.org/10829078/diff/4002/chrome/browser/performance_...
File chrome/browser/performance_monitor/performance_monitor_browsertest.cc
(right):

http://codereview.chromium.org/10829078/diff/4002/chrome/browser/performance_...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:643: const
base::TimeDelta kMaxLoadTime= base::TimeDelta::FromSeconds(30);
nit: space before =

[commit-bot: I haz the power, 2012-08-11 18:13:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/10829078/14

[Devlin, 2012-08-11 18:22:24]

The database stores all data as pure data; that way we can tweak it however we
want without worrying about whether or not it will harm the integrity of the
data. The UI is responsible for displaying it in a reasonable fashion (in this
case, and most others, that means taking the median). The only time we would
worry about having the raw data changed would be if we want to eventually deal
with database aggregation, but that is postponed to a later version.

https://chromiumcodereview.appspot.com/10829078/diff/4002/chrome/browser/perf...
File chrome/browser/performance_monitor/performance_monitor.cc (right):

https://chromiumcodereview.appspot.com/10829078/diff/4002/chrome/browser/perf...
chrome/browser/performance_monitor/performance_monitor.cc:231:
database_->AddMetric(type, value);
On 2012/08/10 18:34:04, Yoyo Zhou wrote:
> Maybe DCHECK that you're on the right thread?

Done.

https://chromiumcodereview.appspot.com/10829078/diff/4002/chrome/browser/perf...
File chrome/browser/performance_monitor/performance_monitor_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/10829078/diff/4002/chrome/browser/perf...
chrome/browser/performance_monitor/performance_monitor_browsertest.cc:643: const
base::TimeDelta kMaxLoadTime= base::TimeDelta::FromSeconds(30);
On 2012/08/10 18:34:04, Yoyo Zhou wrote:
> nit: space before =

Done.

[commit-bot: I haz the power, 2012-08-12 02:29:31]

Change committed as 151197