Refactor BenchmarkResult.Add to take enum for data_type

Refactor BenchmarkResult.Add to take an enum for data_type

BUG=168754

[tonyg, 2013-08-12 21:51:59]

dtu should take a look at this.

[dtu, 2013-08-12 23:29:25]

https://codereview.chromium.org/22754003/diff/13001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/page/results_data_type.py (right):

https://codereview.chromium.org/22754003/diff/13001/tools/telemetry/telemetry...
tools/telemetry/telemetry/page/results_data_type.py:5: class DataType:
Seems wordy. Let's just get rid of the wrapping class and just keep everything
at the module level. (This is never done in Java, etc., but is common in
Python.) Hopefully it can be done with just a sed :)

https://codereview.chromium.org/22754003/diff/13001/tools/telemetry/telemetry...
tools/telemetry/telemetry/page/results_data_type.py:6: DEFAULT = 1
Keep them as strings instead of integers, so we can print them to the console.
(Idea taken from nduca)

[nduca, 2013-08-12 23:30:55]

An example of a ptyhon-not-an-enum-but-kinda is in timeline_metrics, iirw

[rschoen, 2013-08-16 00:23:21]

On 2013/08/12 23:29:25, Dave Tu wrote:
>
https://codereview.chromium.org/22754003/diff/13001/tools/telemetry/telemetry...
> File tools/telemetry/telemetry/page/results_data_type.py (right):
> 
>
https://codereview.chromium.org/22754003/diff/13001/tools/telemetry/telemetry...
> tools/telemetry/telemetry/page/results_data_type.py:5: class DataType:
> Seems wordy. Let's just get rid of the wrapping class and just keep everything
> at the module level. (This is never done in Java, etc., but is common in
> Python.) Hopefully it can be done with just a sed :)
> 
>
https://codereview.chromium.org/22754003/diff/13001/tools/telemetry/telemetry...
> tools/telemetry/telemetry/page/results_data_type.py:6: DEFAULT = 1
> Keep them as strings instead of integers, so we can print them to the console.
> (Idea taken from nduca)

Done, PTAL.

[dtu, 2013-08-16 00:40:01]

Cool, lgtm with nits.

https://codereview.chromium.org/22754003/diff/29001/build/android/pylib/perf_...
File build/android/pylib/perf_tests_helper.py (right):

https://codereview.chromium.org/22754003/diff/29001/build/android/pylib/perf_...
build/android/pylib/perf_tests_helper.py:108: assert
result_data_type.IsValidType(result_type), \
nit: use parentheses instead of \ (Google style guide)

https://codereview.chromium.org/22754003/diff/29001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/page/result_data_type.py (right):

https://codereview.chromium.org/22754003/diff/29001/tools/telemetry/telemetry...
tools/telemetry/telemetry/page/result_data_type.py:5: DEFAULT = "default"
nit: single quotes

https://codereview.chromium.org/22754003/diff/29001/tools/telemetry/telemetry...
tools/telemetry/telemetry/page/result_data_type.py:12: INFORMATIONAL]
nit: align indent

[commit-bot: I haz the power, 2013-08-16 00:48:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rschoen@chromium.org/22754003/41001

[commit-bot: I haz the power, 2013-08-16 15:23:42]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-08-16 17:14:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rschoen@chromium.org/22754003/41001

[commit-bot: I haz the power, 2013-08-16 17:14:49]

Failed to apply patch for tools/perf/metrics/histogram.py:
While running patch -p0 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 5
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: tools/perf/metrics/histogram.py
  |===================================================================
  |--- tools/perf/metrics/histogram.py	(revision 217866)
  |+++ tools/perf/metrics/histogram.py	(working copy)
  --------------------------
  No file to patch.  Skipping patch.
  2 out of 2 hunks ignored

Patch:       tools/perf/metrics/histogram.py
Index: tools/perf/metrics/histogram.py
===================================================================
--- tools/perf/metrics/histogram.py	(revision 217866)
+++ tools/perf/metrics/histogram.py	(working copy)
@@ -1,7 +1,9 @@
 # Copyright 2013 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
+
 from metrics import histogram_util
+from telemetry.page import result_data_type
 
 BROWSER_HISTOGRAM = 'browser_histogram'
 RENDERER_HISTOGRAM = 'renderer_histogram'
@@ -27,7 +29,7 @@
     new_histogram = histogram_util.SubtractHistogram(
         data, self._start_values[page.url + self.name])
     results.Add(self.name, self.units, new_histogram,
-                data_type='unimportant-histogram')
+                data_type=result_data_type.UNIMPORTANT_HISTOGRAM)
 
   @property
   def histogram_function(self):

[commit-bot: I haz the power, 2013-08-16 17:57:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rschoen@chromium.org/22754003/66001

[commit-bot: I haz the power, 2013-08-16 17:58:17]

Failed to apply patch for tools/perf/metrics/histogram.py:
While running patch -p0 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 5
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: tools/perf/metrics/histogram.py
  |===================================================================
  |--- tools/perf/metrics/histogram.py	(revision 217866)
  |+++ tools/perf/metrics/histogram.py	(working copy)
  --------------------------
  No file to patch.  Skipping patch.
  2 out of 2 hunks ignored

Patch:       tools/perf/metrics/histogram.py
Index: tools/perf/metrics/histogram.py
===================================================================
--- tools/perf/metrics/histogram.py	(revision 217866)
+++ tools/perf/metrics/histogram.py	(working copy)
@@ -1,7 +1,9 @@
 # Copyright 2013 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
+
 from metrics import histogram_util
+from telemetry.page import result_data_type
 
 BROWSER_HISTOGRAM = 'browser_histogram'
 RENDERER_HISTOGRAM = 'renderer_histogram'
@@ -27,7 +29,7 @@
     new_histogram = histogram_util.SubtractHistogram(
         data, self._start_values[page.url + self.name])
     results.Add(self.name, self.units, new_histogram,
-                data_type='unimportant-histogram')
+                data_type=result_data_type.UNIMPORTANT_HISTOGRAM)
 
   @property
   def histogram_function(self):

[commit-bot: I haz the power, 2013-08-16 20:01:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rschoen@chromium.org/22754003/83001

[rschoen, 2013-08-16 23:49:08]

Hey Marcus, would you mind taking a look at the android file?

Also, any idea why it didn't warn me that I didn't have OWNERS approval? I
didn't realize it until I was investigating the trybot failure (which was
happening for another reason).

[bulach, 2013-08-19 08:37:57]

hey ryan! thanks for doing this! whilst I fully understand my suggestion is not
totally related to your patch, it'd help solve one long standing strange
dependency we have :)
please let me your thoughts:

(no idea why it didn't complain about missing OWNERS review...)

https://codereview.chromium.org/22754003/diff/101001/build/android/pylib/perf...
File build/android/pylib/perf_tests_helper.py (right):

https://codereview.chromium.org/22754003/diff/101001/build/android/pylib/perf...
build/android/pylib/perf_tests_helper.py:17: 'tools', 'telemetry')))
hmm, this is dodgy :)

tools/telemetry already depends on android/pylib, this circular dependency would
make things worse.. :(

any chance of moving result_data_type here? or maybe even better, let's get this
out of build/android and into build/util/lib, so that both android and telemetry
can depend one way on it, and it'd be self-contained there, wdyt?

probably best to do a separate patch just copying this file out first: android
depends on it downstream, so we'll need to have a copy, then I can change
downstream, and later on I'll be happy to nuke it from orbit.

[bulach, 2013-08-19 08:43:00]

https://codereview.chromium.org/22754003/diff/101001/build/android/pylib/perf...
File build/android/pylib/perf_tests_helper.py (right):

https://codereview.chromium.org/22754003/diff/101001/build/android/pylib/perf...
build/android/pylib/perf_tests_helper.py:8: import android_commands
forgot to mention, I think this is leftover from previous refactorings, so it
should be reasonably straightforward to put this and result_data_type (probably
as perf_result_data_type) under build/util/lib...