Implement ubertracing.

Implement ubertracing.

This is based on Nat's patch: https://codereview.chromium.org/441873007/

Move timeline and importers to use telemetry.timeline.TraceData

This (admittedly massive) patch actually gives us an ubertrace object: the
TraceData. The key way to understand this CL is to read timeline/trace_data.py file,
then read importer.py. The rest is cosmetic fallout from the signature changes.

A TraceData contains the data from many tracers, and is always serializable. A
TimelineModel is constructed from a single TraceData. Internally, a
TraceData is an in memory version of json of the following form:
  { traceEvents: ...
    inspectorTimelineEvnets: ...
    tabIds: ...
  }

Since a TraceData has all the tracer data, the model and importer system was
updated so that instead of there being 1 importer per value, the importers
instead indicate which part of the TraceData they can handle, if any.

BUG=356763
Committed: https://crrev.com/fdb146cf261d909613a1d1b11e70d0ba96280ffe
Cr-Commit-Position: refs/heads/master@{#309489}

[sullivan, 2014-12-12 19:01:17]

I did a first pass at the conversion, and got the unit tests mostly running.
Lots of questions came up as I work on greening the remaining 15 tests. I'll
post them here as I work on the tests:


TraceDataBuilder is meant to get data with AddEventsTo(TraceDataPart, list)
In inspector_backend, StopTimelineRecording does this:
  data = self._timeline.Stop()
  builder.AddEventsTo(trace_data_module.INSPECTOR_TRACE_PART, data)
In inspector_timeline, Stop() returns trace_data_module.TraceData(raw_events)

So we are adding TraceData to TraceDataValue instead of a list. I'm not sure
which API is returning the wrong type here?

[chromium-reviews, 2014-12-12 19:01:51]

Ooops, this is not ready for review, should be titled "WIP"

On Fri, Dec 12, 2014 at 2:01 PM, <sullivan@chromium.org> wrote:

> Reviewers: ,
>
> Message:
> I did a first pass at the conversion, and got the unit tests mostly
> running.
> Lots of questions came up as I work on greening the remaining 15 tests.
> I'll
> post them here as I work on the tests:
>
>
> TraceDataBuilder is meant to get data with AddEventsTo(TraceDataPart, list)
> In inspector_backend, StopTimelineRecording does this:
>   data = self._timeline.Stop()
>   builder.AddEventsTo(trace_data_module.INSPECTOR_TRACE_PART, data)
> In inspector_timeline, Stop() returns trace_data_module.TraceData(
> raw_events)
>
> So we are adding TraceData to TraceDataValue instead of a list. I'm not
> sure
> which API is returning the wrong type here?
>
> Description:
> Implement ubertracing.
>
> This is based on Nat's patch: https://codereview.chromium.org/441873007/
>
>
> BUG=356763
>
> Please review this at https://codereview.chromium.org/805463002/
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+610, -670 lines):
>   M tools/telemetry/telemetry/core/backends/browser_backend.py
>   M tools/telemetry/telemetry/core/backends/chrome/chrome_
> browser_backend.py
>   M tools/telemetry/telemetry/core/backends/chrome/tracing_backend.py
>   M tools/telemetry/telemetry/core/backends/chrome/tracing_
> backend_unittest.py
>   M tools/telemetry/telemetry/core/backends/chrome_
> inspector/inspector_backend.py
>   M tools/telemetry/telemetry/core/backends/chrome_
> inspector/inspector_network.py
>   M tools/telemetry/telemetry/core/backends/chrome_
> inspector/inspector_timeline.py
>   M tools/telemetry/telemetry/core/platform/profiler/
> android_systrace_profiler.py
>   M tools/telemetry/telemetry/core/platform/profiler/strace_profiler.py
>   M tools/telemetry/telemetry/core/platform/profiler/trace_profiler.py
>   M tools/telemetry/telemetry/core/platform/tracing_controller_backend.py
>   D tools/telemetry/telemetry/timeline/empty_timeline_data_importer.py
>   D tools/telemetry/telemetry/timeline/empty_timeline_data_
> importer_unittest.py
>   M tools/telemetry/telemetry/timeline/importer.py
>   M tools/telemetry/telemetry/timeline/inspector_importer.py
>   M tools/telemetry/telemetry/timeline/inspector_importer_unittest.py
>   D tools/telemetry/telemetry/timeline/inspector_timeline_data.py
>   M tools/telemetry/telemetry/timeline/model.py
>   M tools/telemetry/telemetry/timeline/model_unittest.py
>   A tools/telemetry/telemetry/timeline/tab_id_importer.py
>   A tools/telemetry/telemetry/timeline/tab_id_importer_unittest.py
>   D tools/telemetry/telemetry/timeline/timeline_data.py
>   A tools/telemetry/telemetry/timeline/trace_data.py
>   A tools/telemetry/telemetry/timeline/trace_data_unittest.py
>   M tools/telemetry/telemetry/timeline/trace_event_importer.py
>   M tools/telemetry/telemetry/timeline/trace_event_importer_unittest.py
>   D tools/telemetry/telemetry/timeline/tracing_timeline_data.py
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sullivan, 2014-12-12 19:05:10]

Unit test results at www/~sullivan/results.txt

[nednguyen, 2014-12-12 19:05:50]

nednguyen@google.com changed reviewers:
+ nednguyen@google.com

[nednguyen, 2014-12-12 19:30:19]

You will also need to modify value/trace.py to take in the trace_data instead of
tracing_timeline_data. For now, that trace value can just process in
ChromeTracePart, we can expand it to process other TracePart in subsequent
patches.

https://codereview.chromium.org/805463002/diff/1/tools/telemetry/telemetry/ti...
File tools/telemetry/telemetry/timeline/model.py (right):

https://codereview.chromium.org/805463002/diff/1/tools/telemetry/telemetry/ti...
tools/telemetry/telemetry/timeline/model.py:54: provided.
This line of documentation seems to describe an implementation detail. I would
follow python's style guide to describe trace_data, shift_world_to_zero
variable:
https://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Comments

https://codereview.chromium.org/805463002/diff/1/tools/telemetry/telemetry/ti...
File tools/telemetry/telemetry/timeline/trace_data.py (right):

https://codereview.chromium.org/805463002/diff/1/tools/telemetry/telemetry/ti...
tools/telemetry/telemetry/timeline/trace_data.py:9: # TODO(sullivan): is None
valid?
Yes, None is valid. But depends on whether it's key or value (see my comment
below).

https://codereview.chromium.org/805463002/diff/1/tools/telemetry/telemetry/ti...
tools/telemetry/telemetry/timeline/trace_data.py:23: _ValidateRawData(v)
The notion of RawData is based on json serializable. So I am pretty sure that
you will need to validate key & value differently.

https://codereview.chromium.org/805463002/diff/1/tools/telemetry/telemetry/ti...
tools/telemetry/telemetry/timeline/trace_data.py:37: self.raw_field_name =
raw_field_name
Can you make this "immutable" by setting self._raw_field_name = raw_file_name
and define @property raw_field_name?

https://codereview.chromium.org/805463002/diff/1/tools/telemetry/telemetry/ti...
tools/telemetry/telemetry/timeline/trace_data.py:50: class
RawTraceDataHelpersMixin(object):
I think we don't need this superclass because TraceDataBuilder API doesn't need
to have active_parts, HasEventsFor... methods.

[sullivan, 2014-12-12 21:38:35]

I'm still working on whittling the unit tests down, but addressed the review
comments.

https://codereview.chromium.org/805463002/diff/1/tools/telemetry/telemetry/ti...
File tools/telemetry/telemetry/timeline/model.py (right):

https://codereview.chromium.org/805463002/diff/1/tools/telemetry/telemetry/ti...
tools/telemetry/telemetry/timeline/model.py:54: provided.
On 2014/12/12 19:30:18, nednguyen wrote:
> This line of documentation seems to describe an implementation detail. I would
> follow python's style guide to describe trace_data, shift_world_to_zero
> variable:
> https://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Comments

Done.

https://codereview.chromium.org/805463002/diff/1/tools/telemetry/telemetry/ti...
File tools/telemetry/telemetry/timeline/trace_data.py (right):

https://codereview.chromium.org/805463002/diff/1/tools/telemetry/telemetry/ti...
tools/telemetry/telemetry/timeline/trace_data.py:23: _ValidateRawData(v)
On 2014/12/12 19:30:19, nednguyen wrote:
> The notion of RawData is based on json serializable. So I am pretty sure that
> you will need to validate key & value differently.

That's a really good point. I started looking at the specifics of json, and it
gets a little complicated. I switched it to just actually use json
serialization. Any reason not to do that? (The original CL did not).

https://codereview.chromium.org/805463002/diff/1/tools/telemetry/telemetry/ti...
tools/telemetry/telemetry/timeline/trace_data.py:50: class
RawTraceDataHelpersMixin(object):
On 2014/12/12 19:30:19, nednguyen wrote:
> I think we don't need this superclass because TraceDataBuilder API doesn't
need
> to have active_parts, HasEventsFor... methods. 

Done.

[nednguyen, 2014-12-12 21:48:52]

Thanks Sullivan, I think the patch is in the good shape overall.

On 2014/12/12 21:38:35, sullivan wrote:
> I'm still working on whittling the unit tests down, but addressed the review
> comments.
> 
>
https://codereview.chromium.org/805463002/diff/1/tools/telemetry/telemetry/ti...
> File tools/telemetry/telemetry/timeline/model.py (right):
> 
>
https://codereview.chromium.org/805463002/diff/1/tools/telemetry/telemetry/ti...
> tools/telemetry/telemetry/timeline/model.py:54: provided.
> On 2014/12/12 19:30:18, nednguyen wrote:
> > This line of documentation seems to describe an implementation detail. I
would
> > follow python's style guide to describe trace_data, shift_world_to_zero
> > variable:
> > https://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Comments
> 
> Done.
> 
>
https://codereview.chromium.org/805463002/diff/1/tools/telemetry/telemetry/ti...
> File tools/telemetry/telemetry/timeline/trace_data.py (right):
> 
>
https://codereview.chromium.org/805463002/diff/1/tools/telemetry/telemetry/ti...
> tools/telemetry/telemetry/timeline/trace_data.py:23: _ValidateRawData(v)
> On 2014/12/12 19:30:19, nednguyen wrote:
> > The notion of RawData is based on json serializable. So I am pretty sure
that
> > you will need to validate key & value differently.
> 
> That's a really good point. I started looking at the specifics of json, and it
> gets a little complicated. I switched it to just actually use json
> serialization. Any reason not to do that? (The original CL did not).

Maybe to improving debuggability when the data is not not json serializable? I
am not sure, you may want to chat with Nat.
 
> 
>
https://codereview.chromium.org/805463002/diff/1/tools/telemetry/telemetry/ti...
> tools/telemetry/telemetry/timeline/trace_data.py:50: class
> RawTraceDataHelpersMixin(object):
> On 2014/12/12 19:30:19, nednguyen wrote:
> > I think we don't need this superclass because TraceDataBuilder API doesn't
> need
> > to have active_parts, HasEventsFor... methods. 
> 
> Done.

[nednguyen, 2014-12-12 21:49:01]

https://codereview.chromium.org/805463002/diff/40001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/timeline/trace_data.py (right):

https://codereview.chromium.org/805463002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/timeline/trace_data.py:9: # TODO(sullivan): is None
valid?
None works for json.dumps :)

[nednguyen, 2014-12-12 22:42:25]

On 2014/12/12 19:01:17, sullivan wrote:
> I did a first pass at the conversion, and got the unit tests mostly running.
> Lots of questions came up as I work on greening the remaining 15 tests. I'll
> post them here as I work on the tests:
> 
> 
> TraceDataBuilder is meant to get data with AddEventsTo(TraceDataPart, list)
> In inspector_backend, StopTimelineRecording does this:
>   data = self._timeline.Stop()
>   builder.AddEventsTo(trace_data_module.INSPECTOR_TRACE_PART, data)
> In inspector_timeline, Stop() returns trace_data_module.TraceData(raw_events)
> 
> So we are adding TraceData to TraceDataValue instead of a list. I'm not sure
> which API is returning the wrong type here?

I think inspector_timeline should return the raw data, since the
inspector_backend does the task of  combining the data from inspector_timeline &
inspector_network. My mental model is TraceDataBuilder is for creating TraceData
that come from multiple sources. Eventually, the StopTimelineRecording logic
should be moved to tracing_controller.

[sullivan, 2014-12-16 20:48:16]

On 2014/12/12 22:42:25, nednguyen wrote:
> On 2014/12/12 19:01:17, sullivan wrote:
> > I did a first pass at the conversion, and got the unit tests mostly running.
> > Lots of questions came up as I work on greening the remaining 15 tests. I'll
> > post them here as I work on the tests:
> > 
> > 
> > TraceDataBuilder is meant to get data with AddEventsTo(TraceDataPart, list)
> > In inspector_backend, StopTimelineRecording does this:
> >   data = self._timeline.Stop()
> >   builder.AddEventsTo(trace_data_module.INSPECTOR_TRACE_PART, data)
> > In inspector_timeline, Stop() returns
trace_data_module.TraceData(raw_events)
> > 
> > So we are adding TraceData to TraceDataValue instead of a list. I'm not sure
> > which API is returning the wrong type here?
> 
> I think inspector_timeline should return the raw data, since the
> inspector_backend does the task of  combining the data from inspector_timeline
&
> inspector_network. My mental model is TraceDataBuilder is for creating
TraceData
> that come from multiple sources. Eventually, the StopTimelineRecording logic
> should be moved to tracing_controller.

Thanks! I've updated it to do that, and fixed all but 2 of the unit tests.
Full results of a unit test run are in
https://x20web.corp.google.com/~sullivan/results.txt

1) PageRunEndToEndTests.testRunPageWithProfilingFlag fails in a stack of files I
didn't modify at all:

  Traceback (most recent call last):
    File
"/usr/local/google/code/chromium/src/tools/telemetry/telemetry/page/page_run_end_to_end_unittest.py",
line 478, in testRunPageWithProfilingFlag
      user_story_runner.Run(Measurement(), ps, expectations, options, results)
    File
"/usr/local/google/code/chromium/src/tools/telemetry/telemetry/user_story/user_story_runner.py",
line 250, in Run
      test, expectations, user_story, results, state)
    File
"/usr/local/google/code/chromium/src/tools/telemetry/telemetry/user_story/user_story_runner.py",
line 91, in _RunUserStoryAndProcessErrorIfNeeded
      state.WillRunUserStory(user_story)
    File
"/usr/local/google/code/chromium/src/tools/telemetry/telemetry/page/shared_page_state.py",
line 183, in WillRunUserStory
      self._StartProfiling(self._current_page)
    File
"/usr/local/google/code/chromium/src/tools/telemetry/telemetry/page/shared_page_state.py",
line 257, in _StartProfiling
      self._finder_options.profiler, output_file)
    File
"/usr/local/google/code/chromium/src/tools/telemetry/telemetry/core/platform/profiling_controller.py",
line 12, in Start
      profiler_name, base_output_file)
    File
"/usr/local/google/code/chromium/src/tools/telemetry/telemetry/core/platform/profiling_controller_backend.py",
line 24, in Start
      raise NotImplementedError()
  NotImplementedError

I can repro this failure locally sometimes on my master branch, but it fails
100% of the time on my ubertracing branch. Any ideas what could cause this?


2) ActionRunnerInteractionTest.testIssuingMultipleMeasurementInteractionRecords
fails first with a timeout:


  Traceback (most recent call last):
    File
"/usr/local/google/code/chromium/src/tools/telemetry/telemetry/decorators.py",
line 55, in wrapper
      func(*args, **kwargs)
    File
"/usr/local/google/code/chromium/src/tools/telemetry/telemetry/page/actions/action_runner_unittest.py",
line 55, in testIssuingMultipleMeasurementInteractionRecords
      self.VerifyIssuingInteractionRecords(is_responsive=True)
    File
"/usr/local/google/code/chromium/src/tools/telemetry/telemetry/page/actions/action_runner_unittest.py",
line 40, in VerifyIssuingInteractionRecords
      trace_data = self._browser.platform.tracing_controller.Stop()
    File
"/usr/local/google/code/chromium/src/tools/telemetry/telemetry/core/platform/tracing_controller.py",
line 32, in Stop
      return self._tracing_controller_backend.Stop()
    File
"/usr/local/google/code/chromium/src/tools/telemetry/telemetry/core/platform/tracing_controller_backend.py",
line 42, in Stop
      browser_backend.StopTracing(trace_data_builder)
    File
"/usr/local/google/code/chromium/src/tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py",
line 322, in StopTracing
      self._tracing_backend.StopTracing(trace_data_builder)
    File
"/usr/local/google/code/chromium/src/tools/telemetry/telemetry/core/backends/chrome/tracing_backend.py",
line 95, in StopTracing
      'time out amount.' % err.elapsed_time)
  TracingTimeoutException: Trace data was not fully received due to timeout
after 30.0010008812 seconds. If the trace data is big, you may want to increase
the time out amount.

Then subsequently fails with _tracing_backend being None in
self._tracing_backend.StopTracing(trace_data_builder) in
chrome_browser_backend.py:

  Traceback (most recent call last):
    File
"/usr/local/google/code/chromium/src/tools/telemetry/telemetry/decorators.py",
line 55, in wrapper
      func(*args, **kwargs)
    File
"/usr/local/google/code/chromium/src/tools/telemetry/telemetry/page/actions/action_runner_unittest.py",
line 54, in testIssuingMultipleMeasurementInteractionRecords
      self.VerifyIssuingInteractionRecords(is_fast=True)
    File
"/usr/local/google/code/chromium/src/tools/telemetry/telemetry/page/actions/action_runner_unittest.py",
line 40, in VerifyIssuingInteractionRecords
      trace_data = self._browser.platform.tracing_controller.Stop()
    File
"/usr/local/google/code/chromium/src/tools/telemetry/telemetry/core/platform/tracing_controller.py",
line 32, in Stop
      return self._tracing_controller_backend.Stop()
    File
"/usr/local/google/code/chromium/src/tools/telemetry/telemetry/core/platform/tracing_controller_backend.py",
line 42, in Stop
      browser_backend.StopTracing(trace_data_builder)
    File
"/usr/local/google/code/chromium/src/tools/telemetry/telemetry/core/backends/chrome/chrome_browser_backend.py",
line 322, in StopTracing
      self._tracing_backend.StopTracing(trace_data_builder)
  AttributeError: 'NoneType' object has no attribute 'StopTracing'

Any ideas how my CL could cause a timeout, or why it always fails differently on
subsequent test runs?

[nednguyen, 2014-12-17 02:45:47]

I think the fix below changes the error you are seeing with the profiler test
also.

https://codereview.chromium.org/805463002/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/backends/chrome/tracing_backend.py (right):

https://codereview.chromium.org/805463002/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/tracing_backend.py:95: 'time out
amount.' % err.elapsed_time)
self._is_tracing_running = False below will help you pass
ActionRunnerInteractionTest.testIssuingMultipleMeasurementInteractionRecords

[sullivan, 2014-12-17 16:11:48]

Fixing the missing line passed
ActionRunnerInteractionTest.testIssuingMultipleMeasurementInteractionRecords.

But now PageRunEndToEndTests.testRunPageWithProfilingFlag is failing with a
different error every time. Do you know what causes tests to fail differently on
successive runs like that? Full output:
https://x20web.corp.google.com/~sullivan/results.txt

https://codereview.chromium.org/805463002/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/backends/chrome/tracing_backend.py (right):

https://codereview.chromium.org/805463002/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/backends/chrome/tracing_backend.py:95: 'time out
amount.' % err.elapsed_time)
On 2014/12/17 02:45:47, nednguyen wrote:
> self._is_tracing_running = False below will help you pass
> ActionRunnerInteractionTest.testIssuingMultipleMeasurementInteractionRecords

Oops, I can't believe I missed that line. Sorry about that!

[nednguyen, 2014-12-17 16:32:11]

On 2014/12/17 16:11:48, sullivan wrote:
> Fixing the missing line passed
> ActionRunnerInteractionTest.testIssuingMultipleMeasurementInteractionRecords.
> 
> But now PageRunEndToEndTests.testRunPageWithProfilingFlag is failing with a
> different error every time. Do you know what causes tests to fail differently
on
> successive runs like that? Full output:
> https://x20web.corp.google.com/~sullivan/results.txt
> 
>
https://codereview.chromium.org/805463002/diff/100001/tools/telemetry/telemet...
> File tools/telemetry/telemetry/core/backends/chrome/tracing_backend.py
(right):
> 
>
https://codereview.chromium.org/805463002/diff/100001/tools/telemetry/telemet...
> tools/telemetry/telemetry/core/backends/chrome/tracing_backend.py:95: 'time
out
> amount.' % err.elapsed_time)
> On 2014/12/17 02:45:47, nednguyen wrote:
> > self._is_tracing_running = False below will help you pass
> > ActionRunnerInteractionTest.testIssuingMultipleMeasurementInteractionRecords
> 
> Oops, I can't believe I missed that line. Sorry about that!

It's so amusing to look at the these different failures, such an extreme cases
of heisenbug :P.  I will look more into this when I have time today.

[sullivan, 2014-12-17 16:49:15]

On 2014/12/17 16:32:11, nednguyen wrote:
> On 2014/12/17 16:11:48, sullivan wrote:
> > Fixing the missing line passed
> >
ActionRunnerInteractionTest.testIssuingMultipleMeasurementInteractionRecords.
> > 
> > But now PageRunEndToEndTests.testRunPageWithProfilingFlag is failing with a
> > different error every time. Do you know what causes tests to fail
differently
> on
> > successive runs like that? Full output:
> > https://x20web.corp.google.com/~sullivan/results.txt
> > 
> >
>
https://codereview.chromium.org/805463002/diff/100001/tools/telemetry/telemet...
> > File tools/telemetry/telemetry/core/backends/chrome/tracing_backend.py
> (right):
> > 
> >
>
https://codereview.chromium.org/805463002/diff/100001/tools/telemetry/telemet...
> > tools/telemetry/telemetry/core/backends/chrome/tracing_backend.py:95: 'time
> out
> > amount.' % err.elapsed_time)
> > On 2014/12/17 02:45:47, nednguyen wrote:
> > > self._is_tracing_running = False below will help you pass
> > >
ActionRunnerInteractionTest.testIssuingMultipleMeasurementInteractionRecords
> > 
> > Oops, I can't believe I missed that line. Sorry about that!
> 
> It's so amusing to look at the these different failures, such an extreme cases
> of heisenbug :P.  I will look more into this when I have time today.

Sounds good. I'm going to branch this and try to merge to pick up ernstm's
surfaceflinger changes, which should keep me busy for a while.

[sullivan, 2014-12-17 23:11:10]

+ernstm

I merged this with ernstm's surfaceflinger cleanup cl. All the tests pass except
the one I mentioned above, but I'm concerned about test coverage because after
the tests passed and I uploaded my cl, the linter told me I had a typo in
AndroidPlatformBackend.StopDisplayTracing() that would have prevented it from
running at all (missing trace_data_module import). Can anyone suggest good test
cases for the surfaceflinger code?

[ernstm, 2014-12-18 18:06:36]

On 2014/12/17 23:11:10, sullivan wrote:
> +ernstm
> 
> I merged this with ernstm's surfaceflinger cleanup cl. All the tests pass
except
> the one I mentioned above, but I'm concerned about test coverage because after
> the tests passed and I uploaded my cl, the linter told me I had a typo in
> AndroidPlatformBackend.StopDisplayTracing() that would have prevented it from
> running at all (missing trace_data_module import). Can anyone suggest good
test
> cases for the surfaceflinger code?

SurfaceFlinger is Android only. Did you run the tests on Android? The smoke
tests for smoothness do exercise the full code path. But we should add an
Android-only test to tools/perf/smoothness_unittest.py that checks if
SurfaceFlinger metrics (avg_surface_fps, jank_count, etc.) have been calculated.
I forgot that in my CL.

[nednguyen, 2014-12-19 02:15:09]

https://codereview.chromium.org/805463002/diff/140001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/profiler/trace_profiler.py (right):

https://codereview.chromium.org/805463002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/profiler/trace_profiler.py:42:
trace_result = self._browser_backend.StopTracing()
You want to remove this line.

[sullivan, 2014-12-19 21:46:35]

On 2014/12/18 18:06:36, ernstm wrote:
> On 2014/12/17 23:11:10, sullivan wrote:
> > +ernstm
> > 
> > I merged this with ernstm's surfaceflinger cleanup cl. All the tests pass
> except
> > the one I mentioned above, but I'm concerned about test coverage because
after
> > the tests passed and I uploaded my cl, the linter told me I had a typo in
> > AndroidPlatformBackend.StopDisplayTracing() that would have prevented it
from
> > running at all (missing trace_data_module import). Can anyone suggest good
> test
> > cases for the surfaceflinger code?
> 
> SurfaceFlinger is Android only. Did you run the tests on Android? The smoke
> tests for smoothness do exercise the full code path. But we should add an
> Android-only test to tools/perf/smoothness_unittest.py that checks if
> SurfaceFlinger metrics (avg_surface_fps, jank_count, etc.) have been
calculated.
> I forgot that in my CL.

Oops, was not running on Android, found some issues and fixed them. I'll work on
a test that checks if surfaceflinger metrics have been calculated in a separate
patch.

[sullivan, 2014-12-19 21:47:36]

I think this is ready for review. I merged with all the recent refactors, and
all tests pass locally.

I'll work on a separate patch for surfaceflinger metrics, and make sure that
passes before submitting.

https://codereview.chromium.org/805463002/diff/140001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/profiler/trace_profiler.py (right):

https://codereview.chromium.org/805463002/diff/140001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/profiler/trace_profiler.py:42:
trace_result = self._browser_backend.StopTracing()
On 2014/12/19 02:15:08, nednguyen wrote:
> You want to remove this line.

Done.

[nednguyen, 2014-12-20 02:07:38]

LGTM overall, let's land and iterate.

[nednguyen, 2014-12-20 02:56:09]

On 2014/12/20 02:07:38, nednguyen wrote:
> LGTM overall, let's land and iterate.

Before landing this, can you try a local smoothness run with --upload-trace and
make sure that nothing crashes?

[slamm, 2014-12-22 17:12:30]

slamm@chromium.org changed reviewers:
+ slamm@chromium.org

[slamm, 2014-12-22 17:12:33]

Here are some style comments that can be done as a follow-up. (A big change like
this is hard enough to land.)

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
File tools/telemetry/telemetry/timeline/trace_data.py (right):

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/trace_data.py:7: 
It is good practice for modules to raise their own exceptions instead of a
generic Exception.

class TraceDataSerializationError(Exception):
  """Raised when raw trace data cannot be serialized to TraceData."""
  pass

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/trace_data.py:14: raise Exception('TraceData
cannot be serialized: %s' % e)
except (TypeError, ValueError) as e:
  raise NonSerializableTraceData('TraceData cannot be serialized: %s' % e)

Is this message correct? It happens while initializing a TraceData object.

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/trace_data.py:30: return self._raw_field_name
indent 2.

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/trace_data.py:38: ALL_TRACE_PARTS =
set([CHROME_TRACE_PART, INSPECTOR_TRACE_PART, TAB_ID_PART])
Use set literal?

ALL_TRACE_PARTS = {CHROME_TRACE_PART, INSPECTOR_TRACE_PART, TAB_ID_PART}

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/trace_data.py:51: NOTE: raw data must NOT
include any non-primitive objects!
NOTE: raw data must only include primitive objects!

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/trace_data.py:56: Raw data can be any of the
three standard trace_event formats:
Raw data can be one of three standard trace_event formats:

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/trace_data.py:113: if p.raw_field_name in
self._raw_data])
Use a set comprehension?

  return {p for p in ALL_TRACE_PARTS if p.raw_field_name in self._raw_data}

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/trace_data.py:117: part_field_names =
set([p.raw_field_name for p in ALL_TRACE_PARTS])
part_field_names = {p.raw_field_name for p in ALL_TRACE_PARTS}

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/trace_data.py:118: for k,v in
self._raw_data.iteritems():
A space after the comma.

for k, v in self._raw_data.iteritems():

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/trace_data.py:123: 'value' :
self._raw_data[v]
No spaces before the colons.

'name': k,
'value':  self._raw_data[v]

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/trace_data.py:138: Always writes in the trace
container format."""
Closing quotes on a new line.

"""Serializes the trace result to a file-like object.

Always writes in the trace container format.
"""

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
File tools/telemetry/telemetry/timeline/trace_data_unittest.py (right):

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/trace_data_unittest.py:20: assert
'traceEvents' in d
self.assertIn('traceEvents', d)

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/trace_data_unittest.py:21: assert '[1, 2, 3]'
in d
self.assertIn('[1, 2, 3]', d)

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/trace_data_unittest.py:25: def
testValidate(self):
Split into 2 tests.

def testValidateWithNonPrimativeRaises(self):
  ...

def testValidateWithCircularReferenceRaises(self):
  ...

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/trace_data_unittest.py:30: ({'hello':
TraceDataTest}))
With a module-defined exception, this check is simpler:

with self.assertRaises(trace_data.NonSerializableTraceDataError):
  trace_data.TraceData({'hello': TraceDataTest})

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/trace_data_unittest.py:37: 'TraceData cannot
be serialized: Circular reference detected',
I did not see this exception raised in the code.

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/trace_data_unittest.py:44: assert
d.HasEventsFor(trace_data.CHROME_TRACE_PART) == False
self.assertFalse(d.HasEventsFor(trace_data.CHROME_TRACE_PART))

and fix-up other asserts below similarly.

[sullivan, 2014-12-22 19:28:38]

I actually had to make some changes to this CL to merge and submit, so I went
ahead and addressed slamm's comments. Thanks for the feedback!

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
File tools/telemetry/telemetry/timeline/trace_data.py (right):

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/trace_data.py:14: raise Exception('TraceData
cannot be serialized: %s' % e)
On 2014/12/22 17:12:31, slamm wrote:
> except (TypeError, ValueError) as e:
>   raise NonSerializableTraceData('TraceData cannot be serialized: %s' % e)
> 
> Is this message correct? It happens while initializing a TraceData object.

TraceData should only contain json-serializable data. The idea here is to
validate at construction time. Ideas for a clearer message welcome!

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/trace_data.py:30: return self._raw_field_name
On 2014/12/22 17:12:32, slamm wrote:
> indent 2.

Done.

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/trace_data.py:38: ALL_TRACE_PARTS =
set([CHROME_TRACE_PART, INSPECTOR_TRACE_PART, TAB_ID_PART])
On 2014/12/22 17:12:32, slamm wrote:
> Use set literal?
> 
> ALL_TRACE_PARTS = {CHROME_TRACE_PART, INSPECTOR_TRACE_PART, TAB_ID_PART}

Done.

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/trace_data.py:51: NOTE: raw data must NOT
include any non-primitive objects!
On 2014/12/22 17:12:32, slamm wrote:
> NOTE: raw data must only include primitive objects!

Done.

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/trace_data.py:56: Raw data can be any of the
three standard trace_event formats:
On 2014/12/22 17:12:32, slamm wrote:
> Raw data can be one of three standard trace_event formats:

Done.

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/trace_data.py:113: if p.raw_field_name in
self._raw_data])
On 2014/12/22 17:12:31, slamm wrote:
> Use a set comprehension?
> 
>   return {p for p in ALL_TRACE_PARTS if p.raw_field_name in self._raw_data}

Done.

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/trace_data.py:117: part_field_names =
set([p.raw_field_name for p in ALL_TRACE_PARTS])
On 2014/12/22 17:12:32, slamm wrote:
> part_field_names = {p.raw_field_name for p in ALL_TRACE_PARTS}

Done.

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/trace_data.py:118: for k,v in
self._raw_data.iteritems():
On 2014/12/22 17:12:32, slamm wrote:
> A space after the comma.
> 
> for k, v in self._raw_data.iteritems():

Done.

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/trace_data.py:123: 'value' :
self._raw_data[v]
On 2014/12/22 17:12:32, slamm wrote:
> No spaces before the colons.
> 
> 'name': k,
> 'value':  self._raw_data[v]

Done.

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/trace_data.py:138: Always writes in the trace
container format."""
On 2014/12/22 17:12:32, slamm wrote:
> Closing quotes on a new line.
> 
> """Serializes the trace result to a file-like object.
> 
> Always writes in the trace container format.
> """

Done.

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
File tools/telemetry/telemetry/timeline/trace_data_unittest.py (right):

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/trace_data_unittest.py:20: assert
'traceEvents' in d
On 2014/12/22 17:12:32, slamm wrote:
> self.assertIn('traceEvents', d)

Done.

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/trace_data_unittest.py:21: assert '[1, 2, 3]'
in d
On 2014/12/22 17:12:32, slamm wrote:
> self.assertIn('[1, 2, 3]', d)

Done.

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/trace_data_unittest.py:25: def
testValidate(self):
On 2014/12/22 17:12:32, slamm wrote:
> Split into 2 tests.
> 
> def testValidateWithNonPrimativeRaises(self):
>   ...
> 
> def testValidateWithCircularReferenceRaises(self):
>   ...

Done.

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/trace_data_unittest.py:30: ({'hello':
TraceDataTest}))
On 2014/12/22 17:12:32, slamm wrote:
> With a module-defined exception, this check is simpler:
> 
> with self.assertRaises(trace_data.NonSerializableTraceDataError):
>   trace_data.TraceData({'hello': TraceDataTest})

Done.

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/trace_data_unittest.py:37: 'TraceData cannot
be serialized: Circular reference detected',
On 2014/12/22 17:12:32, slamm wrote:
> I did not see this exception raised in the code.

Raised by json parsing.

https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/trace_data_unittest.py:44: assert
d.HasEventsFor(trace_data.CHROME_TRACE_PART) == False
On 2014/12/22 17:12:32, slamm wrote:
> self.assertFalse(d.HasEventsFor(trace_data.CHROME_TRACE_PART))
> 
> and fix-up other asserts below similarly.

Done.

[sullivan, 2014-12-22 19:48:14]

The CQ bit was checked by sullivan@chromium.org

[commit-bot: I haz the power, 2014-12-22 19:49:26]

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

[slamm, 2014-12-22 20:22:13]

On 2014/12/22 19:28:38, sullivan wrote:
> I actually had to make some changes to this CL to merge and submit, so I went
> ahead and addressed slamm's comments. Thanks for the feedback!
> 
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> File tools/telemetry/telemetry/timeline/trace_data.py (right):
> 
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> tools/telemetry/telemetry/timeline/trace_data.py:14: raise
Exception('TraceData
> cannot be serialized: %s' % e)
> On 2014/12/22 17:12:31, slamm wrote:
> > except (TypeError, ValueError) as e:
> >   raise NonSerializableTraceData('TraceData cannot be serialized: %s' % e)
> > 
> > Is this message correct? It happens while initializing a TraceData object.
> 
> TraceData should only contain json-serializable data. The idea here is to
> validate at construction time. Ideas for a clearer message welcome!
> 
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> tools/telemetry/telemetry/timeline/trace_data.py:30: return
self._raw_field_name
> On 2014/12/22 17:12:32, slamm wrote:
> > indent 2.
> 
> Done.
> 
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> tools/telemetry/telemetry/timeline/trace_data.py:38: ALL_TRACE_PARTS =
> set([CHROME_TRACE_PART, INSPECTOR_TRACE_PART, TAB_ID_PART])
> On 2014/12/22 17:12:32, slamm wrote:
> > Use set literal?
> > 
> > ALL_TRACE_PARTS = {CHROME_TRACE_PART, INSPECTOR_TRACE_PART, TAB_ID_PART}
> 
> Done.
> 
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> tools/telemetry/telemetry/timeline/trace_data.py:51: NOTE: raw data must NOT
> include any non-primitive objects!
> On 2014/12/22 17:12:32, slamm wrote:
> > NOTE: raw data must only include primitive objects!
> 
> Done.
> 
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> tools/telemetry/telemetry/timeline/trace_data.py:56: Raw data can be any of
the
> three standard trace_event formats:
> On 2014/12/22 17:12:32, slamm wrote:
> > Raw data can be one of three standard trace_event formats:
> 
> Done.
> 
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> tools/telemetry/telemetry/timeline/trace_data.py:113: if p.raw_field_name in
> self._raw_data])
> On 2014/12/22 17:12:31, slamm wrote:
> > Use a set comprehension?
> > 
> >   return {p for p in ALL_TRACE_PARTS if p.raw_field_name in self._raw_data}
> 
> Done.
> 
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> tools/telemetry/telemetry/timeline/trace_data.py:117: part_field_names =
> set([p.raw_field_name for p in ALL_TRACE_PARTS])
> On 2014/12/22 17:12:32, slamm wrote:
> > part_field_names = {p.raw_field_name for p in ALL_TRACE_PARTS}
> 
> Done.
> 
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> tools/telemetry/telemetry/timeline/trace_data.py:118: for k,v in
> self._raw_data.iteritems():
> On 2014/12/22 17:12:32, slamm wrote:
> > A space after the comma.
> > 
> > for k, v in self._raw_data.iteritems():
> 
> Done.
> 
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> tools/telemetry/telemetry/timeline/trace_data.py:123: 'value' :
> self._raw_data[v]
> On 2014/12/22 17:12:32, slamm wrote:
> > No spaces before the colons.
> > 
> > 'name': k,
> > 'value':  self._raw_data[v]
> 
> Done.
> 
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> tools/telemetry/telemetry/timeline/trace_data.py:138: Always writes in the
trace
> container format."""
> On 2014/12/22 17:12:32, slamm wrote:
> > Closing quotes on a new line.
> > 
> > """Serializes the trace result to a file-like object.
> > 
> > Always writes in the trace container format.
> > """
> 
> Done.
> 
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> File tools/telemetry/telemetry/timeline/trace_data_unittest.py (right):
> 
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> tools/telemetry/telemetry/timeline/trace_data_unittest.py:20: assert
> 'traceEvents' in d
> On 2014/12/22 17:12:32, slamm wrote:
> > self.assertIn('traceEvents', d)
> 
> Done.
> 
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> tools/telemetry/telemetry/timeline/trace_data_unittest.py:21: assert '[1, 2,
3]'
> in d
> On 2014/12/22 17:12:32, slamm wrote:
> > self.assertIn('[1, 2, 3]', d)
> 
> Done.
> 
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> tools/telemetry/telemetry/timeline/trace_data_unittest.py:25: def
> testValidate(self):
> On 2014/12/22 17:12:32, slamm wrote:
> > Split into 2 tests.
> > 
> > def testValidateWithNonPrimativeRaises(self):
> >   ...
> > 
> > def testValidateWithCircularReferenceRaises(self):
> >   ...
> 
> Done.
> 
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> tools/telemetry/telemetry/timeline/trace_data_unittest.py:30: ({'hello':
> TraceDataTest}))
> On 2014/12/22 17:12:32, slamm wrote:
> > With a module-defined exception, this check is simpler:
> > 
> > with self.assertRaises(trace_data.NonSerializableTraceDataError):
> >   trace_data.TraceData({'hello': TraceDataTest})
> 
> Done.
> 
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> tools/telemetry/telemetry/timeline/trace_data_unittest.py:37: 'TraceData
cannot
> be serialized: Circular reference detected',
> On 2014/12/22 17:12:32, slamm wrote:
> > I did not see this exception raised in the code.
> 
> Raised by json parsing.
> 
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> tools/telemetry/telemetry/timeline/trace_data_unittest.py:44: assert
> d.HasEventsFor(trace_data.CHROME_TRACE_PART) == False
> On 2014/12/22 17:12:32, slamm wrote:
> > self.assertFalse(d.HasEventsFor(trace_data.CHROME_TRACE_PART))
> > 
> > and fix-up other asserts below similarly.
> 
> Done.

The patch the CQ is trying is the same one I commented on.  Do you have changes
you need to upload?

[sullivan, 2014-12-22 20:28:11]

The CQ bit was unchecked by sullivan@chromium.org

[sullivan, 2014-12-22 20:36:43]

On 2014/12/22 20:22:13, slamm wrote:
> On 2014/12/22 19:28:38, sullivan wrote:
> > I actually had to make some changes to this CL to merge and submit, so I
went
> > ahead and addressed slamm's comments. Thanks for the feedback!
> > 
> >
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> > File tools/telemetry/telemetry/timeline/trace_data.py (right):
> > 
> >
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> > tools/telemetry/telemetry/timeline/trace_data.py:14: raise
> Exception('TraceData
> > cannot be serialized: %s' % e)
> > On 2014/12/22 17:12:31, slamm wrote:
> > > except (TypeError, ValueError) as e:
> > >   raise NonSerializableTraceData('TraceData cannot be serialized: %s' % e)
> > > 
> > > Is this message correct? It happens while initializing a TraceData object.
> > 
> > TraceData should only contain json-serializable data. The idea here is to
> > validate at construction time. Ideas for a clearer message welcome!
> > 
> >
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> > tools/telemetry/telemetry/timeline/trace_data.py:30: return
> self._raw_field_name
> > On 2014/12/22 17:12:32, slamm wrote:
> > > indent 2.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> > tools/telemetry/telemetry/timeline/trace_data.py:38: ALL_TRACE_PARTS =
> > set([CHROME_TRACE_PART, INSPECTOR_TRACE_PART, TAB_ID_PART])
> > On 2014/12/22 17:12:32, slamm wrote:
> > > Use set literal?
> > > 
> > > ALL_TRACE_PARTS = {CHROME_TRACE_PART, INSPECTOR_TRACE_PART, TAB_ID_PART}
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> > tools/telemetry/telemetry/timeline/trace_data.py:51: NOTE: raw data must NOT
> > include any non-primitive objects!
> > On 2014/12/22 17:12:32, slamm wrote:
> > > NOTE: raw data must only include primitive objects!
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> > tools/telemetry/telemetry/timeline/trace_data.py:56: Raw data can be any of
> the
> > three standard trace_event formats:
> > On 2014/12/22 17:12:32, slamm wrote:
> > > Raw data can be one of three standard trace_event formats:
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> > tools/telemetry/telemetry/timeline/trace_data.py:113: if p.raw_field_name in
> > self._raw_data])
> > On 2014/12/22 17:12:31, slamm wrote:
> > > Use a set comprehension?
> > > 
> > >   return {p for p in ALL_TRACE_PARTS if p.raw_field_name in
self._raw_data}
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> > tools/telemetry/telemetry/timeline/trace_data.py:117: part_field_names =
> > set([p.raw_field_name for p in ALL_TRACE_PARTS])
> > On 2014/12/22 17:12:32, slamm wrote:
> > > part_field_names = {p.raw_field_name for p in ALL_TRACE_PARTS}
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> > tools/telemetry/telemetry/timeline/trace_data.py:118: for k,v in
> > self._raw_data.iteritems():
> > On 2014/12/22 17:12:32, slamm wrote:
> > > A space after the comma.
> > > 
> > > for k, v in self._raw_data.iteritems():
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> > tools/telemetry/telemetry/timeline/trace_data.py:123: 'value' :
> > self._raw_data[v]
> > On 2014/12/22 17:12:32, slamm wrote:
> > > No spaces before the colons.
> > > 
> > > 'name': k,
> > > 'value':  self._raw_data[v]
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> > tools/telemetry/telemetry/timeline/trace_data.py:138: Always writes in the
> trace
> > container format."""
> > On 2014/12/22 17:12:32, slamm wrote:
> > > Closing quotes on a new line.
> > > 
> > > """Serializes the trace result to a file-like object.
> > > 
> > > Always writes in the trace container format.
> > > """
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> > File tools/telemetry/telemetry/timeline/trace_data_unittest.py (right):
> > 
> >
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> > tools/telemetry/telemetry/timeline/trace_data_unittest.py:20: assert
> > 'traceEvents' in d
> > On 2014/12/22 17:12:32, slamm wrote:
> > > self.assertIn('traceEvents', d)
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> > tools/telemetry/telemetry/timeline/trace_data_unittest.py:21: assert '[1, 2,
> 3]'
> > in d
> > On 2014/12/22 17:12:32, slamm wrote:
> > > self.assertIn('[1, 2, 3]', d)
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> > tools/telemetry/telemetry/timeline/trace_data_unittest.py:25: def
> > testValidate(self):
> > On 2014/12/22 17:12:32, slamm wrote:
> > > Split into 2 tests.
> > > 
> > > def testValidateWithNonPrimativeRaises(self):
> > >   ...
> > > 
> > > def testValidateWithCircularReferenceRaises(self):
> > >   ...
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> > tools/telemetry/telemetry/timeline/trace_data_unittest.py:30: ({'hello':
> > TraceDataTest}))
> > On 2014/12/22 17:12:32, slamm wrote:
> > > With a module-defined exception, this check is simpler:
> > > 
> > > with self.assertRaises(trace_data.NonSerializableTraceDataError):
> > >   trace_data.TraceData({'hello': TraceDataTest})
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> > tools/telemetry/telemetry/timeline/trace_data_unittest.py:37: 'TraceData
> cannot
> > be serialized: Circular reference detected',
> > On 2014/12/22 17:12:32, slamm wrote:
> > > I did not see this exception raised in the code.
> > 
> > Raised by json parsing.
> > 
> >
>
https://codereview.chromium.org/805463002/diff/220001/tools/telemetry/telemet...
> > tools/telemetry/telemetry/timeline/trace_data_unittest.py:44: assert
> > d.HasEventsFor(trace_data.CHROME_TRACE_PART) == False
> > On 2014/12/22 17:12:32, slamm wrote:
> > > self.assertFalse(d.HasEventsFor(trace_data.CHROME_TRACE_PART))
> > > 
> > > and fix-up other asserts below similarly.
> > 
> > Done.
> 
> The patch the CQ is trying is the same one I commented on.  Do you have
changes
> you need to upload?

Whoops! Uploaded to the wrong issue id. Fixed now.

[sullivan, 2014-12-22 20:36:50]

The CQ bit was checked by sullivan@chromium.org

[commit-bot: I haz the power, 2014-12-22 20:38:20]

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

[commit-bot: I haz the power, 2014-12-22 22:35:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-22 22:35:54]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[sullivan, 2014-12-22 22:44:32]

The CQ bit was checked by sullivan@chromium.org

[commit-bot: I haz the power, 2014-12-22 22:45:57]

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

[commit-bot: I haz the power, 2014-12-22 23:46:36]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2014-12-22 23:47:28]

Patchset 13 (id:??) landed as
https://crrev.com/fdb146cf261d909613a1d1b11e70d0ba96280ffe
Cr-Commit-Position: refs/heads/master@{#309489}