Chrome Performance Monitor: MetricDetails refactor.

Chrome Performance Monitor: Moved MetricDetails lookup to metric_details.*.
Database keys and MetricDetails lookups are now made through the MetricType enum.

BUG=130212
TEST=


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

[Matt Tytel, 2012-07-21 00:20:48]

Eriq, would you do me the courtesy of reviewing this first?

https://chromiumcodereview.appspot.com/10805050/diff/1/chrome/browser/perform...
File chrome/browser/performance_monitor/database_unittest.cc (right):

https://chromiumcodereview.appspot.com/10805050/diff/1/chrome/browser/perform...
chrome/browser/performance_monitor/database_unittest.cc:103: // TODO(mtytel):
Once more metrics are added, populate the database with
This will happen before this CL goes in.

[eaugusti, 2012-07-21 02:00:40]

https://chromiumcodereview.appspot.com/10805050/diff/5001/chrome/browser/perf...
File chrome/browser/performance_monitor/database.cc (right):

https://chromiumcodereview.appspot.com/10805050/diff/5001/chrome/browser/perf...
chrome/browser/performance_monitor/database.cc:371: UpdateActiveInterval();
Humm... I think we decided that active intervals should only be updated for
writes. You probably weren't in on this conversation, but could you remove them
from reads.

https://chromiumcodereview.appspot.com/10805050/diff/5001/chrome/browser/perf...
File chrome/browser/performance_monitor/database_unittest.cc (right):

https://chromiumcodereview.appspot.com/10805050/diff/5001/chrome/browser/perf...
chrome/browser/performance_monitor/database_unittest.cc:105: void PopulateDB() {
I think that testing with only one metric type does not exercise enough code
paths. Could we do something to get another metric type in here?

https://chromiumcodereview.appspot.com/10805050/diff/5001/chrome/browser/perf...
chrome/browser/performance_monitor/database_unittest.cc:308: // stats =
db_->GetStatsForActivityAndMetric(METRIC_NUMBER_OF_METRICS);
Why are these left in?

https://chromiumcodereview.appspot.com/10805050/diff/5001/chrome/browser/perf...
File chrome/browser/performance_monitor/metric_details.h (right):

https://chromiumcodereview.appspot.com/10805050/diff/5001/chrome/browser/perf...
chrome/browser/performance_monitor/metric_details.h:14: METRIC_SAMPLE,
Are there any more metrics that we can add now?
CPU? MEM? JANK?

[Matt Tytel, 2012-07-21 02:11:40]

https://chromiumcodereview.appspot.com/10805050/diff/5001/chrome/browser/perf...
File chrome/browser/performance_monitor/database_unittest.cc (right):

https://chromiumcodereview.appspot.com/10805050/diff/5001/chrome/browser/perf...
chrome/browser/performance_monitor/database_unittest.cc:105: void PopulateDB() {
Dern.. I deleted the patch set with the comment here.
Mitchell has a CL up that adds some metrics. I'm going to add the additional
tests when they go in and before this CL is through.
http://codereview.chromium.org/10656052/

https://chromiumcodereview.appspot.com/10805050/diff/5001/chrome/browser/perf...
chrome/browser/performance_monitor/database_unittest.cc:308: // stats =
db_->GetStatsForActivityAndMetric(METRIC_NUMBER_OF_METRICS);
Because I'm careless. When Mitchell's patch goes through I'll fix this up.

https://chromiumcodereview.appspot.com/10805050/diff/5001/chrome/browser/perf...
File chrome/browser/performance_monitor/metric_details.h (right):

https://chromiumcodereview.appspot.com/10805050/diff/5001/chrome/browser/perf...
chrome/browser/performance_monitor/metric_details.h:14: METRIC_SAMPLE,
In Mitchell's CL:
http://codereview.chromium.org/10656052/

[Matt Tytel, 2012-07-23 22:24:00]

https://chromiumcodereview.appspot.com/10805050/diff/5001/chrome/browser/perf...
File chrome/browser/performance_monitor/database.cc (right):

https://chromiumcodereview.appspot.com/10805050/diff/5001/chrome/browser/perf...
chrome/browser/performance_monitor/database.cc:371: UpdateActiveInterval();
On 2012/07/21 02:00:40, eaugusti wrote:
> Humm... I think we decided that active intervals should only be updated for
> writes. You probably weren't in on this conversation, but could you remove
them
> from reads.

Done.

https://chromiumcodereview.appspot.com/10805050/diff/5001/chrome/browser/perf...
File chrome/browser/performance_monitor/database_unittest.cc (right):

https://chromiumcodereview.appspot.com/10805050/diff/5001/chrome/browser/perf...
chrome/browser/performance_monitor/database_unittest.cc:105: void PopulateDB() {
Actually I just added a couple into mine.
Done.

https://chromiumcodereview.appspot.com/10805050/diff/5001/chrome/browser/perf...
chrome/browser/performance_monitor/database_unittest.cc:308: // stats =
db_->GetStatsForActivityAndMetric(METRIC_NUMBER_OF_METRICS);
On 2012/07/21 02:11:40, Matt Tytel wrote:
> Because I'm careless. When Mitchell's patch goes through I'll fix this up.

Done.

https://chromiumcodereview.appspot.com/10805050/diff/5001/chrome/browser/perf...
File chrome/browser/performance_monitor/metric_details.h (right):

https://chromiumcodereview.appspot.com/10805050/diff/5001/chrome/browser/perf...
chrome/browser/performance_monitor/metric_details.h:14: METRIC_SAMPLE,
On 2012/07/21 02:11:40, Matt Tytel wrote:
> In Mitchell's CL:
> http://codereview.chromium.org/10656052/

Done.

[eaugusti, 2012-07-23 22:53:43]

On 2012/07/23 22:24:00, Matt Tytel wrote:
>
https://chromiumcodereview.appspot.com/10805050/diff/5001/chrome/browser/perf...
> File chrome/browser/performance_monitor/database.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10805050/diff/5001/chrome/browser/perf...
> chrome/browser/performance_monitor/database.cc:371: UpdateActiveInterval();
> On 2012/07/21 02:00:40, eaugusti wrote:
> > Humm... I think we decided that active intervals should only be updated for
> > writes. You probably weren't in on this conversation, but could you remove
> them
> > from reads.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10805050/diff/5001/chrome/browser/perf...
> File chrome/browser/performance_monitor/database_unittest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10805050/diff/5001/chrome/browser/perf...
> chrome/browser/performance_monitor/database_unittest.cc:105: void PopulateDB()
{
> Actually I just added a couple into mine.
> Done.
> 
>
https://chromiumcodereview.appspot.com/10805050/diff/5001/chrome/browser/perf...
> chrome/browser/performance_monitor/database_unittest.cc:308: // stats =
> db_->GetStatsForActivityAndMetric(METRIC_NUMBER_OF_METRICS);
> On 2012/07/21 02:11:40, Matt Tytel wrote:
> > Because I'm careless. When Mitchell's patch goes through I'll fix this up.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10805050/diff/5001/chrome/browser/perf...
> File chrome/browser/performance_monitor/metric_details.h (right):
> 
>
https://chromiumcodereview.appspot.com/10805050/diff/5001/chrome/browser/perf...
> chrome/browser/performance_monitor/metric_details.h:14: METRIC_SAMPLE,
> On 2012/07/21 02:11:40, Matt Tytel wrote:
> > In Mitchell's CL:
> > http://codereview.chromium.org/10656052/
> 
> Done.

lgtm

[Matt Tytel, 2012-07-23 23:02:24]

koz: I got another one for you.

estade: Could I get a webui review?

thanks in advance.

[koz (OOO until 15th September), 2012-07-24 01:05:59]

lgtm

[Evan Stade, 2012-07-24 18:53:04]

lgtm

https://chromiumcodereview.appspot.com/10805050/diff/9001/chrome/browser/ui/w...
File chrome/browser/ui/webui/performance_monitor/web_ui_handler.cc (right):

https://chromiumcodereview.appspot.com/10805050/diff/9001/chrome/browser/ui/w...
chrome/browser/ui/webui/performance_monitor/web_ui_handler.cc:61:
Database::MetricVectorMap metric_vector_map = db->GetStatsForMetricByActivity(
I think it would be easier to read if you line break right after the = operator

[Matt Tytel, 2012-07-24 20:11:20]

https://chromiumcodereview.appspot.com/10805050/diff/9001/chrome/browser/ui/w...
File chrome/browser/ui/webui/performance_monitor/web_ui_handler.cc (right):

https://chromiumcodereview.appspot.com/10805050/diff/9001/chrome/browser/ui/w...
chrome/browser/ui/webui/performance_monitor/web_ui_handler.cc:61:
Database::MetricVectorMap metric_vector_map = db->GetStatsForMetricByActivity(
On 2012/07/24 18:53:04, Evan Stade wrote:
> I think it would be easier to read if you line break right after the =
operator

Done.

[commit-bot: I haz the power, 2012-07-24 20:11:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtytel@chromium.org/10805050/9003

[commit-bot: I haz the power, 2012-07-24 23:38:20]

Change committed as 148248