Adding checkerboard measurements to telemetry.

Previously, approximated_pixel_percentage would combine the amount of low-res tiles along with checkerboarded tiles.

A separate CL added a new checkerboard-only metric to Chrome.

This CL adds support for that checkerboard metric to telemetry.

BUG=466867
Committed: https://crrev.com/7fcac8edc816818380269a6507ff8295475ab762
Cr-Commit-Position: refs/heads/master@{#324709}

[cblume, 2015-03-26 01:00:17]

cblume@chromium.org changed reviewers:
+ nednguyen@google.com

[nednguyen, 2015-03-26 01:01:30]

Actually for these metrics change, I think you probably don't need to check for
chrome branch number. If you want to go great length, we probably add that info
to chrome tracing but using None value to say it fails to find the data is good
enough.

[nednguyen, 2015-04-07 01:30:56]

https://codereview.chromium.org/1033733010/diff/1/tools/telemetry/telemetry/w...
File tools/telemetry/telemetry/web_perf/metrics/rendering_stats.py (right):

https://codereview.chromium.org/1033733010/diff/1/tools/telemetry/telemetry/w...
tools/telemetry/telemetry/web_perf/metrics/rendering_stats.py:247:
round(float(data['checkerboarded_visible_content_area']) /
if 'checkerboarded_visible_content_area' & 'visible_content_area' are not in
data, we should fail more gracefully here.
RenderingStats class has a self.errors member, you can check whether these data
are available and add the error to self.error if they aren't.

https://codereview.chromium.org/1033733010/diff/1/tools/telemetry/telemetry/w...
tools/telemetry/telemetry/web_perf/metrics/rendering_stats.py:248:
float(data['visible_content_area']) * 100.0, 3))
Also check for division by zero error.

https://codereview.chromium.org/1033733010/diff/1/tools/telemetry/telemetry/w...
File tools/telemetry/telemetry/web_perf/metrics/smoothness.py (right):

https://codereview.chromium.org/1033733010/diff/1/tools/telemetry/telemetry/w...
tools/telemetry/telemetry/web_perf/metrics/smoothness.py:302:
FlattenList(stats.checkerboarded_pixel_percentages)), 3)
when this has enough frames but still no checkerboarded_pixel_percentages data
due to 'checkerboarded_visible_content_area' field and the other one is missing,
you can set the none_value_reason to s.t like "missing checkerboarded... data in
the trace"

[cblume, 2015-04-08 23:57:46]

Please double check that my changes are satisfactory. Thanks, Ned :)

[nednguyen, 2015-04-09 17:59:23]

https://codereview.chromium.org/1033733010/diff/20001/tools/telemetry/telemet...
File tools/telemetry/telemetry/web_perf/metrics/rendering_stats.py (right):

https://codereview.chromium.org/1033733010/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/rendering_stats.py:242:
visible_content_area = data.get('visible_content_area', 0)
This does not distinguish the case from visible_content_area is missing vs its
value is 0. Can you handle the two cases separately?

https://codereview.chromium.org/1033733010/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/rendering_stats.py:251: else:
To avoid nested if else, you can return whenever there is error.

https://codereview.chromium.org/1033733010/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/rendering_stats.py:259: if
'checkerboarded_visible_content_area2' in data:
'checkerboarded_visible_content_area2' typo. Probably should use an enum string
for this field to.

https://codereview.chromium.org/1033733010/diff/20001/tools/telemetry/telemet...
File tools/telemetry/telemetry/web_perf/metrics/smoothness.py (right):

https://codereview.chromium.org/1033733010/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/smoothness.py:301: if
'checkerboarded_pixel_percentages' in stats.errors:
You can make a "enum" CHECKER_BOARDED_ERROR='checker_boarded_error' for this
check

https://codereview.chromium.org/1033733010/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/smoothness.py:304:
'checkerboarded_pixel_percentages data.')
How about none_value_reason = stats.errors[CHECKER_BOARDED_ERROR]

https://codereview.chromium.org/1033733010/diff/20001/tools/telemetry/telemet...
File tools/telemetry/telemetry/web_perf/metrics/smoothness_unittest.py (right):

https://codereview.chromium.org/1033733010/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/smoothness_unittest.py:252: 
Can you add a simple missing data case?

[cblume, 2015-04-10 01:52:16]

I appreciate your thorough reviews, Ned. Glad to have you on board. :)

https://codereview.chromium.org/1033733010/diff/20001/tools/telemetry/telemet...
File tools/telemetry/telemetry/web_perf/metrics/rendering_stats.py (right):

https://codereview.chromium.org/1033733010/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/rendering_stats.py:242:
visible_content_area = data.get('visible_content_area', 0)
On 2015/04/09 17:59:22, nednguyen wrote:
> This does not distinguish the case from visible_content_area is missing vs its
> value is 0. Can you handle the two cases separately?

Done.

https://codereview.chromium.org/1033733010/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/rendering_stats.py:251: else:
On 2015/04/09 17:59:22, nednguyen wrote:
> To avoid nested if else, you can return whenever there is error.

Done.

https://codereview.chromium.org/1033733010/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/rendering_stats.py:259: if
'checkerboarded_visible_content_area2' in data:
On 2015/04/09 17:59:22, nednguyen wrote:
> 'checkerboarded_visible_content_area2' typo. Probably should use an enum
string
> for this field to.

Done.

https://codereview.chromium.org/1033733010/diff/20001/tools/telemetry/telemet...
File tools/telemetry/telemetry/web_perf/metrics/smoothness.py (right):

https://codereview.chromium.org/1033733010/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/smoothness.py:301: if
'checkerboarded_pixel_percentages' in stats.errors:
On 2015/04/09 17:59:22, nednguyen wrote:
> You can make a "enum" CHECKER_BOARDED_ERROR='checker_boarded_error' for this
> check

Done.

https://codereview.chromium.org/1033733010/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/smoothness.py:304:
'checkerboarded_pixel_percentages data.')
On 2015/04/09 17:59:22, nednguyen wrote:
> How about none_value_reason = stats.errors[CHECKER_BOARDED_ERROR]

Done.

https://codereview.chromium.org/1033733010/diff/20001/tools/telemetry/telemet...
File tools/telemetry/telemetry/web_perf/metrics/smoothness_unittest.py (right):

https://codereview.chromium.org/1033733010/diff/20001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/smoothness_unittest.py:252: 
On 2015/04/09 17:59:22, nednguyen wrote:
> Can you add a simple missing data case?

Done.

[nednguyen, 2015-04-10 16:27:09]

https://codereview.chromium.org/1033733010/diff/40001/tools/telemetry/telemet...
File tools/telemetry/telemetry/web_perf/metrics/rendering_stats.py (right):

https://codereview.chromium.org/1033733010/diff/40001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/rendering_stats.py:30: # These are
keys used in a data dictionary
nit: "." at the end of sentence. Also can you specify that data dictionary is
the 'data' field of 'BenchmarkInstrumentation::ImplThreadRenderingStats' event?

https://codereview.chromium.org/1033733010/diff/40001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/rendering_stats.py:259: # prevent a
divide-by-zero
Nit: comment should be "Prevent a divide-by-zero.", but I think the code here is
readable enough so you can remove the comment. :)

https://codereview.chromium.org/1033733010/diff/40001/tools/telemetry/telemet...
File tools/telemetry/telemetry/web_perf/metrics/smoothness.py (right):

https://codereview.chromium.org/1033733010/diff/40001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/smoothness.py:24:
CHECKERBOARDED_PIXEL_ERROR = 'checkerboarded_pixel_percentage'
Err, just use rendering_stats.CHECKERBOARDED_PIXEL_ERROR for the code below.
Again, the reason to reuse a string here is to reduce the chance of making
error. You are having a typo in this one as 's' at the end of
'checkerboarded_pixel_percentages' is missing.

https://codereview.chromium.org/1033733010/diff/40001/tools/telemetry/telemet...
File tools/telemetry/telemetry/web_perf/metrics/smoothness_unittest.py (right):

https://codereview.chromium.org/1033733010/diff/40001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/smoothness_unittest.py:267:
stats.errors[smoothness.CHECKERBOARDED_PIXEL_ERROR] = 'test error'
stats.errors[rendering_stats.CHECKERBOARDED_PIXEL_ERROR]

[nednguyen, 2015-04-10 20:45:02]

lgtm

[cblume, 2015-04-10 21:11:13]

The CQ bit was checked by cblume@chromium.org

[commit-bot: I haz the power, 2015-04-10 21:13:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1033733010/60001

[commit-bot: I haz the power, 2015-04-10 23:15:29]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-10 23:16:16]

Patchset 4 (id:??) landed as
https://crrev.com/7fcac8edc816818380269a6507ff8295475ab762
Cr-Commit-Position: refs/heads/master@{#324709}