Telemetry: Sanitize a key and add description to speedindex metrics

tools/perf/metrics/speedindex.py

 # 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.
 
 import collections
 
 from metrics import Metric
 from telemetry.core import bitmap
 from telemetry.value import scalar
 
@@ skipping 35 matching lines @@
     self._impl.Stop(tab)
 
   # Optional argument chart_name is not in base class Metric.
   # pylint: disable=W0221
   def AddResults(self, tab, results, chart_name=None):
     """Calculate the speed index and add it to the results."""
     index = self._impl.CalculateSpeedIndex(tab)
     # Release the tab so that it can be disconnected.
     self._impl = None
     results.AddValue(scalar.ScalarValue(
-        results.current_page, '%s.speed_index' % chart_name, 'ms', index))
+        results.current_page, '%s_speed_index' % chart_name, 'ms', index,

[qyearsley, 2014/10/23 17:01:39]

This change from %s.speed_index to %s_speed_index was intentional, right?

[shimazu, 2014/10/27 02:33:12]

Yes, I saw the result of run_benchmark as 'yourChartName: speed_index' format
though 'yourChartName_speed_index: url' is our expectation. This is caused by
the dot, so I changed the dot to underscore.

+        description='Speed Index. This focuses on a time when visible parts of '

[qyearsley, 2014/10/23 17:01:39]

a time -> time

[shimazu, 2014/10/27 02:33:12]

Done.

+                    'page are displayed, and this shows the time when the '
+                    'first look is "almost" composed. If you want to get more '

[qyearsley, 2014/10/23 17:01:39]

I'm not sure if this description is accurate. Speed index doesn't indicate the
time when any particular event happened -- it's just supposed to be a measure of
load time when is lower when most of the page loads sooner.

[shimazu, 2014/10/27 02:33:11]

I tried to add such explanation. I'm not sure how detail I have to write. Is it
okay or a bit too redundant?

[qyearsley, 2014/10/27 02:55:46]

I personally think it might be a bit redundant, because that link
(http://goo.gl/Rw3d5d) has more details.

+                    'detail, please refer to http://goo.gl/Rw3d5d . Currently '

[qyearsley, 2014/10/23 17:01:39]

No need for the space between the URL and the dot.

[shimazu, 2014/10/27 02:33:12]

Done.

+                    'there are two implementation: for Android and for '

[qyearsley, 2014/10/23 17:01:39]

implementation -> implementations

[shimazu, 2014/10/27 02:33:11]

Done.

+                    'Desktop. Android version (using video capture) is good '
+                    'index to show the advantage of your patch, but Desktop '
+                    'one (using paint events) is not because of extra overhead '
+                    'to catch paint events.'))

[qyearsley, 2014/10/23 17:01:39]

I wouldn't say the desktop version is "not good", but it is worth noting that it
it has extra overhead.

I would phrase this sentence as:

"The Android version uses video capture; the Desktop one uses paint events and
has extra overhead to catch paint events.

[shimazu, 2014/10/27 02:33:12]

Done.

 
   def IsFinished(self, tab):
     """Decide whether the timeline recording should be stopped.
 
     When the timeline recording is stopped determines which paint events
     are used in the speed index metric calculation. In general, the recording
     should continue if there has just been some data received, because
     this suggests that painting may continue.
 
     A page may repeatedly request resources in an infinite loop; a timeout
@@ skipping 231 matching lines @@
     frame = paint_event.args['frameId']
     return (frame,) + GetBox(paint_event.args['data']['clip'])
 
   def _GroupEventByRectangle(self, paint_events):
     """Group all paint events according to the rectangle that they update."""
     result = collections.defaultdict(list)
     for event in paint_events:
       assert event.name == 'Paint'
       result[self._GetRectangle(event)].append(event)
     return result