Highlight all the associate flow events for an input event

Highlight all the associate flow events for an input event

BUG=#948
R=nduca@chromium.org

Committed: https://chromium.googlesource.com/external/trace-viewer/+/202afe717e19c4cda9614bf79a6d83c3d4e2a5ba

[yuhaoz, 2015-05-27 01:19:56]

For now, I just highlighted all the flow events (i.e., the arrows). But I would
assume that we want to highlight all the related slices as well?

[nduca, 2015-05-27 01:26:24]

unit test plox

https://codereview.appspot.com/237520043/diff/1/trace_viewer/extras/chrome/cc...
File trace_viewer/extras/chrome/cc/input_latency_async_slice.html (right):

https://codereview.appspot.com/237520043/diff/1/trace_viewer/extras/chrome/cc...
trace_viewer/extras/chrome/cc/input_latency_async_slice.html:29: var
associatedEvents = [];
should we remember the result so we don't do this computation over and over?

https://codereview.appspot.com/237520043/diff/1/trace_viewer/extras/chrome/cc...
trace_viewer/extras/chrome/cc/input_latency_async_slice.html:33: if
(flowEvent.id == asyncSliceId) associatedEvents.push(flowEvent);
Should we have a model.getFlowEventsWithId(...)?

The performance of this is pretty scary otherwise...

https://codereview.appspot.com/237520043/diff/1/trace_viewer/extras/chrome/cc...
trace_viewer/extras/chrome/cc/input_latency_async_slice.html:33: if
(flowEvent.id == asyncSliceId) associatedEvents.push(flowEvent);
we put newlines after oneline ifs

if (condition)
  statement;

[yuhaoz, 2015-05-27 15:09:19]

https://codereview.appspot.com/237520043/diff/1/trace_viewer/extras/chrome/cc...
File trace_viewer/extras/chrome/cc/input_latency_async_slice.html (right):

https://codereview.appspot.com/237520043/diff/1/trace_viewer/extras/chrome/cc...
trace_viewer/extras/chrome/cc/input_latency_async_slice.html:29: var
associatedEvents = [];
On 2015/05/27 01:26:24, nduca wrote:
> should we remember the result so we don't do this computation over and over?

Done.

https://codereview.appspot.com/237520043/diff/1/trace_viewer/extras/chrome/cc...
trace_viewer/extras/chrome/cc/input_latency_async_slice.html:33: if
(flowEvent.id == asyncSliceId) associatedEvents.push(flowEvent);
On 2015/05/27 01:26:24, nduca wrote:
> we put newlines after oneline ifs
> 
> if (condition)
>   statement;

Done.

https://codereview.appspot.com/237520043/diff/1/trace_viewer/extras/chrome/cc...
trace_viewer/extras/chrome/cc/input_latency_async_slice.html:33: if
(flowEvent.id == asyncSliceId) associatedEvents.push(flowEvent);
On 2015/05/27 01:26:24, nduca wrote:
> Should we have a model.getFlowEventsWithId(...)?
> 
> The performance of this is pretty scary otherwise..

Can I just edit trace_model itself? I am afraid of conflict, etc.

[yuhaoz, 2015-05-27 15:10:03]

On 2015/05/27 15:09:19, yuhaoz wrote:

Now I also highlight all the events along the flow path.

[nduca, 2015-05-27 17:44:45]

> Can I just edit trace_model itself? I am afraid of conflict, etc.
yes, we can.

probably you want a new step right before "run audits" in trace_model importer
that calls buildIndices_() function, then have that build indices. happy to chat
in person.

[yuhaoz, 2015-05-27 21:03:36]

On 2015/05/27 17:44:45, nduca wrote:

Now flowEventsIndexMap is just a local variable in trace_model. Do you think it
makes more sense to create a standalone class for it, and then new it in
trace_model, just like how flowIntervalTree is handled now? If so, do you think
it should be a base class or just within the trace_model directory?

[nduca, 2015-05-28 00:10:32]

https://codereview.appspot.com/237520043/diff/60001/trace_viewer/core/trace_m...
File trace_viewer/core/trace_model/flow_event_index.html (right):

https://codereview.appspot.com/237520043/diff/60001/trace_viewer/core/trace_m...
trace_viewer/core/trace_model/flow_event_index.html:23: function
FlowEventIndexMap() {
TraceModelIndices

https://codereview.appspot.com/237520043/diff/60001/trace_viewer/core/trace_m...
trace_viewer/core/trace_model/flow_event_index.html:23: function
FlowEventIndexMap() {
I think we should just construct the indices with the model, e.g.

TraceModelIndicates(model) {
  model.flowEvents.forEach(function(fe) {
    if (fe.id === undefined)
      continue;
    this.flowEventsById_[fe.id] = fe;
  }, this);
}

https://codereview.appspot.com/237520043/diff/60001/trace_viewer/core/trace_m...
trace_viewer/core/trace_model/flow_event_index.html:24: this.map_ = {};
flowEventsById_ = {}

https://codereview.appspot.com/237520043/diff/60001/trace_viewer/core/trace_m...
trace_viewer/core/trace_model/flow_event_index.html:35: getFlowEventsWithId:
function(id) {
if its not in the map return []

https://codereview.appspot.com/237520043/diff/60001/trace_viewer/core/trace_m...
File trace_viewer/core/trace_model/trace_model.html (right):

https://codereview.appspot.com/237520043/diff/60001/trace_viewer/core/trace_m...
trace_viewer/core/trace_model/trace_model.html:647:
progressMeter.update('Building flow event indices...');
once you move the index construction to the helper class this just becomes

this.indices = new TraceModelIndices()

https://codereview.appspot.com/237520043/diff/60001/trace_viewer/extras/chrom...
File trace_viewer/extras/chrome/cc/input_latency_async_slice.html (right):

https://codereview.appspot.com/237520043/diff/60001/trace_viewer/extras/chrom...
trace_viewer/extras/chrome/cc/input_latency_async_slice.html:39: if
(matchedFlowEvents) {
this can be simplified once you make getFlowEventsWithId() return []

[nduca, 2015-05-28 03:05:35]

aaalmost :)

https://codereview.appspot.com/237520043/diff/80001/trace_viewer/core/trace_m...
File trace_viewer/core/trace_model/trace_model.html (right):

https://codereview.appspot.com/237520043/diff/80001/trace_viewer/core/trace_m...
trace_viewer/core/trace_model/trace_model.html:121: this.flowEventsIndices = {};
why is this {}? it should be undefined.

https://codereview.appspot.com/237520043/diff/80001/trace_viewer/core/trace_m...
trace_viewer/core/trace_model/trace_model.html:140: 
put the indices field here. the actual events and container objects are lines
114-136 [with the flow interval tree being misplaced. whereas, import warnings
etc are just supporting data. the indices field is supporting data, so should be
in this cluste.r

might be a cool separate patch to organize these fields with a comment block so
its clearer to contirbutors what each cluster of fields is about.

https://codereview.appspot.com/237520043/diff/80001/trace_viewer/core/trace_m...
trace_viewer/core/trace_model/trace_model.html:648: this.flowEventsIndices = new
tv.c.trace_model.TraceModelIndices(this);
the model's field name should be traceModelIndices

[nduca, 2015-05-28 03:10:56]

https://codereview.appspot.com/237520043/diff/80001/trace_viewer/core/trace_m...
File trace_viewer/core/trace_model/trace_model.html (left):

https://codereview.appspot.com/237520043/diff/80001/trace_viewer/core/trace_m...
trace_viewer/core/trace_model/trace_model.html:15: <link rel="import"
href="/core/importer/empty_importer.html">
you should modify an existing set of basic test in trace_model_test to assert
that model.indices is not undefined.

https://codereview.appspot.com/237520043/diff/80001/trace_viewer/core/trace_m...
File trace_viewer/core/trace_model/trace_model_indices.html (right):

https://codereview.appspot.com/237520043/diff/80001/trace_viewer/core/trace_m...
trace_viewer/core/trace_model/trace_model_indices.html:32: }, this);
this file should have a _test file that makes a model with a pair of flow events
with the same id.

test1: tests that the getFlowEventsWithId returns the right thing when you ask
it for that flow event id
test2: tests that getFlowEventsWithId returns [] when you ask for an unindexed
id

https://codereview.appspot.com/237520043/diff/80001/trace_viewer/core/trace_m...
trace_viewer/core/trace_model/trace_model_indices.html:44: if
(!this.flowEventsById_.hasOwnProperty(id))
we prefer this style:
if (x)
 return y
return z

where the else clause simply isn't indented. this way the z case doesn't
indented, so when you start chaining if statements you don't burn your 80col
limit too fast.

https://codereview.appspot.com/237520043/diff/80001/trace_viewer/extras/chrom...
File trace_viewer/extras/chrome/cc/input_latency_async_slice_test.html (right):

https://codereview.appspot.com/237520043/diff/80001/trace_viewer/extras/chrom...
trace_viewer/extras/chrome/cc/input_latency_async_slice_test.html:103:
m.flowEventsIndices = new tv.c.trace_model.TraceModelIndices(m);
you should not be populating the index directly.

you should be doing

var m = test_utils.newModel(function(m) {
    m.a = create async slice with same id and add to model
    m.flow1 = create flow event with same id and add to model
});

assertEquals(m.a.associatedEvents, [m.flow1])

for instance. this way the events and model are created as they are done in
production, via the importEvents flow. the way you are doing it, you are
creating events in potentially different way than the import flow does it. this
means that if we change the way we do the import flow, this test could break.

[nduca, 2015-05-29 00:07:33]

lgtm

https://codereview.appspot.com/237520043/diff/120001/trace_viewer/extras/chro...
File trace_viewer/extras/chrome/cc/input_latency_async_slice.html (right):

https://codereview.appspot.com/237520043/diff/120001/trace_viewer/extras/chro...
trace_viewer/extras/chrome/cc/input_latency_async_slice.html:36: 
var guidAddedToEvents = {}

https://codereview.appspot.com/237520043/diff/120001/trace_viewer/extras/chro...
trace_viewer/extras/chrome/cc/input_latency_async_slice.html:39: if
(flowEvent.startSlice &&
if (flowEvent.startSlice && guidAddedToEvents[flowEvent.startSlice.guid]) {
   push
   add to map
}

...

[nduca, 2015-05-29 00:12:24]

lgtm

[yuhaoz, 2015-05-29 01:42:18]

Committed patchset #7 (id:120001) manually as
202afe717e19c4cda9614bf79a6d83c3d4e2a5ba (presubmit successful).