Change call-sites in trace viewer to use generators instead of iteration functions.

Change call-sites in trace viewer to use generators instead of iteration functions.

Remove the iteration functions (iterateAllEventContainers, etc.) from the trace
viewer and instead use the versions that iterate through the generators
(childEvents, descendantEvents, childEventContainers, descendantEventContainers)/.

BUG=catapult:#2407

Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/403dec7cf147396400a292e2904e5c325f06c1ee

[alexandermont, 2016-06-21 23:58:25]

alexandermont@chromium.org changed reviewers:
+ charliea@chromium.org, eakuefner@chromium.org, nednguyen@google.com

[alexandermont, 2016-06-21 23:58:45]

Description was changed from

==========
Change call-sites to use generators instead of iteration functions.

Remove the iteration functions (iterateAllEventContainers, etc.) from the trace
viewer and instead use the versions that iterate through the generators
(childEvents, descendantEvents, childEventContainers,
descendantEventContainers)/.

BUG=catapult:#2407
==========

to

==========
Change call-sites in trace viewer to use generators instead of iteration
functions.

Remove the iteration functions (iterateAllEventContainers, etc.) from the trace
viewer and instead use the versions that iterate through the generators
(childEvents, descendantEvents, childEventContainers,
descendantEventContainers)/.

BUG=catapult:#2407
==========

[nednguyen, 2016-06-21 23:59:00]

The CQ bit was checked by nednguyen@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-21 23:59:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2083213002/1

[alexandermont, 2016-06-21 23:59:05]

The CQ bit was unchecked by alexandermont@chromium.org

[nednguyen, 2016-06-22 00:05:33]

The CQ bit was checked by nednguyen@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-22 00:05:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2083213002/1

[nednguyen, 2016-06-22 00:05:48]

On 2016/06/21 23:59:08, alexandermont wrote:

I enabled CQ (dry run) so we can see which tests are failing

[commit-bot: I haz the power, 2016-06-22 00:10:46]

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

[commit-bot: I haz the power, 2016-06-22 00:10:47]

Dry run: Try jobs failed on following builders:
  Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)

[charliea (OOO until 10-5), 2016-06-22 13:56:56]

(will take a look once the presubmits are passing)

[alexandermont, 2016-06-22 18:10:37]

The problem was that perf_insights used the iterateAllEvents function which is
now removed, and I hadn't changed the call-sites there yet. I have now changed
these and ran the tests locally, and they pass.

[nednguyen, 2016-06-22 18:15:11]

The CQ bit was checked by nednguyen@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-22 18:15:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2083213002/20001

[commit-bot: I haz the power, 2016-06-22 18:32:47]

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

[commit-bot: I haz the power, 2016-06-22 18:32:48]

Dry run: Try jobs failed on following builders:
  Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)

[alexandermont, 2016-06-22 20:43:53]

The CQ bit was checked by alexandermont@chromium.org

[commit-bot: I haz the power, 2016-06-22 20:43:57]

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

[commit-bot: I haz the power, 2016-06-22 20:43:58]

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

[commit-bot: I haz the power, 2016-06-22 20:43:59]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
Committers are members of the group "project-catapult-committers".
Note that this has nothing to do with OWNERS files.

[alexandermont, 2016-06-22 20:45:14]

The CQ bit was checked by alexandermont@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-22 20:45:26]

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

[commit-bot: I haz the power, 2016-06-22 21:22:12]

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

[commit-bot: I haz the power, 2016-06-22 21:22:14]

Dry run: This issue passed the CQ dry run.

[alexandermont, 2016-06-22 21:57:54]

Dry run working now. Problem was fixed

[nednguyen, 2016-06-22 21:58:40]

lgtm but please wait for Charlie

[charliea (OOO until 10-5), 2016-06-29 18:08:45]

https://codereview.chromium.org/2083213002/diff/60001/perf_insights/perf_insi...
File perf_insights/perf_insights/mappers/scheduling/map_input_blockers.html
(right):

https://codereview.chromium.org/2083213002/diff/60001/perf_insights/perf_insi...
perf_insights/perf_insights/mappers/scheduling/map_input_blockers.html:52: for
(var ievent of mainThread.descendantEvents()) {
Not a big fan of the name "ievent" for two reasons:

  1) It's not at all clear what the name means without looking up to how it's
calculated
  2) It doesn't _really_ follow camelcase conventions (iEvent would be more
common)

How about: "intersectingEvent"? Seems like we should be able to keep everything
on the same number of lines even with that

https://codereview.chromium.org/2083213002/diff/60001/perf_insights/perf_insi...
File perf_insights/perf_insights/mappers/thread_grouping_test.html (right):

https://codereview.chromium.org/2083213002/diff/60001/perf_insights/perf_insi...
perf_insights/perf_insights/mappers/thread_grouping_test.html:44: for (var e of
m.descendantEvents()) {
nit: no need for braces here

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/metrics...
File tracing/tracing/metrics/system_health/first_paint_metric.html (right):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/first_paint_metric.html:47: if
(ev.category === 'blink.user_timing' &&
nit: I think the old way of writing this was more idiomatically Chrome (fast
exit for situations that the below code doesn't apply to)

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/first_paint_metric.html:47: if
(ev.category === 'blink.user_timing' &&
high level nit: big refactorings like this one aren't really the place to try to
simplify the code. I'd recommend sticking to as close to a direct translation of
the previous code as possible

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/first_paint_metric.html:54:
list.unshift(ev);
nit: bad indentation

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/first_paint_metric.html:67: for (var ev of
list)
same note (here and below): in this CL, please do a direct translation of the
old code into the new idiom. If you feel like you can simplify the code, that's
great, but do so in a later CL

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/a...
File tracing/tracing/model/async_slice.html (right):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/a...
tracing/tracing/model/async_slice.html:106:
findTopmostSlicesRelativeToThisSlice: function*(eventPredicate, opt_this) {
What a weird function...

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/c...
File tracing/tracing/model/cpu.html (right):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/c...
tracing/tracing/model/cpu.html:64: // Inherit these functions from
EventContainer, since this isn't
nit: this comment no longer applies, right?

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/d...
File tracing/tracing/model/device.html (right):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/d...
tracing/tracing/model/device.html:99: childEvents: function*() {
nit: can delete this if we provide a default ipml of childEvents

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/d...
File tracing/tracing/model/device_test.html (left):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/d...
tracing/tracing/model/device_test.html:53:
test('iterateAllChildEventContainers', function() {
Why delete this?

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/e...
File tracing/tracing/model/event_container.html (left):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/e...
tracing/tracing/model/event_container.html:142: throw new Error('Not
implemented.');
Did you mean to delete the not implemented error?

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/e...
File tracing/tracing/model/event_container.html (right):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/e...
tracing/tracing/model/event_container.html:75: * Gets list of all events in this
and descendant event containers.
s/Gets list/Returns an iterable

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/e...
tracing/tracing/model/event_container.html:77: descendantEvents: function*() {
I think that this should be getDescendantEvents() rather than
descendantEvents(). (Hopefully this isn't too hard to fix with a simple search
and replace)

I think that, ideally, we'd be able to make this a getter so we could do:

get* descendantEvents() {
  yield *this.childEvents;
  ... 
}

for (var event of this.descendantEvents)

... but apparently JS doesn't have good support for ES5 getters/setters +
generator functions yet :-(

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/e...
tracing/tracing/model/event_container.html:77: descendantEvents: function*() {
Is there no default implementation of childEvents and childEventContainers?
There probably should be (that just does nothing)

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/e...
tracing/tracing/model/event_container.html:84: * Yields this and all descendant
event containers.
s/Yields/Returns an iterable containing

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/e...
tracing/tracing/model/event_container.html:86: descendantEventContainers:
function*() {
same here (getDescendantEventContainers instead of descendantEventContainers)

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/h...
File tracing/tracing/model/helpers/chrome_browser_helper.html (right):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/h...
tracing/tracing/model/helpers/chrome_browser_helper.html:59: for (var thread of
this.modelHelper.model.getAllThreads())
see note earlier: in a CL this big, we should do a direct translation of the old
code (we can simplify in a follow-up CL)

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/i...
File tracing/tracing/model/ir_coverage.html (right):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/i...
tracing/tracing/model/ir_coverage.html:34: // the set of associated events.
nit: bad indentation

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/k...
File tracing/tracing/model/kernel.html (right):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/k...
tracing/tracing/model/kernel.html:130: yield *
ProcessBase.prototype.childEvents.call(this);
Looks like this is overriding the prototype's behavior to just call the
prototype? I think we can just get rid of this and the default will look up the
prototype chain anyhow.

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/o...
File tracing/tracing/model/object_collection.html (left):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/o...
tracing/tracing/model/object_collection.html:43:
findTopmostSlicesInThisContainer: function(eventPredicate, callback,
can this be deleted in a follow-up CL?

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/p...
File tracing/tracing/model/power_series.html (right):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/p...
tracing/tracing/model/power_series.html:121: childEventContainers: function*() {
This is default behavior, right?

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/p...
File tracing/tracing/model/power_series_test.html (left):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/p...
tracing/tracing/model/power_series_test.html:131: 
What's the reason for deleting this? It seems like it's testing that all of the
right stuff is being added in this.childEvents()

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/p...
File tracing/tracing/model/process_base.html (left):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/p...
tracing/tracing/model/process_base.html:55: findTopmostSlicesInThisContainer:
function(eventPredicate, callback,
nit: what's the reason for deleting this in this CL?

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/p...
File tracing/tracing/model/process_base.html (right):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/p...
tracing/tracing/model/process_base.html:62: childEvents: function*() {
same note: isn't this just the default impl?

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/t...
File tracing/tracing/model/thread.html (right):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/t...
tracing/tracing/model/thread.html:86: if (this.timeSlices &&
this.timeSlices.length)
you don't need "&& this.timeSlices.length" here, right? Otherwise, yield *
should just yield an empty array, which is the same as if we hadn't yielded
anything

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/u...
File tracing/tracing/model/user_model/user_model.html (left):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/u...
tracing/tracing/model/user_model/user_model.html:47:
findTopmostSlicesInThisContainer: function(eventPredicate, callback,
same

[alexandermont, 2016-06-29 20:47:12]

https://codereview.chromium.org/2083213002/diff/60001/perf_insights/perf_insi...
File perf_insights/perf_insights/mappers/scheduling/map_input_blockers.html
(right):

https://codereview.chromium.org/2083213002/diff/60001/perf_insights/perf_insi...
perf_insights/perf_insights/mappers/scheduling/map_input_blockers.html:52: for
(var ievent of mainThread.descendantEvents()) {
On 2016/06/29 at 18:08:43, charliea wrote:
> Not a big fan of the name "ievent" for two reasons:
> 
>   1) It's not at all clear what the name means without looking up to how it's
calculated
>   2) It doesn't _really_ follow camelcase conventions (iEvent would be more
common)
> 
> How about: "intersectingEvent"? Seems like we should be able to keep
everything on the same number of lines even with that

Done

https://codereview.chromium.org/2083213002/diff/60001/perf_insights/perf_insi...
File perf_insights/perf_insights/mappers/thread_grouping_test.html (right):

https://codereview.chromium.org/2083213002/diff/60001/perf_insights/perf_insi...
perf_insights/perf_insights/mappers/thread_grouping_test.html:44: for (var e of
m.descendantEvents()) {
On 2016/06/29 at 18:08:44, charliea wrote:
> nit: no need for braces here

Done

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/metrics...
File tracing/tracing/metrics/system_health/first_paint_metric.html (right):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/first_paint_metric.html:47: if
(ev.category === 'blink.user_timing' &&
On 2016/06/29 at 18:08:44, charliea wrote:
> nit: I think the old way of writing this was more idiomatically Chrome (fast
exit for situations that the below code doesn't apply to)

Done

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/first_paint_metric.html:54:
list.unshift(ev);
On 2016/06/29 at 18:08:44, charliea wrote:
> nit: bad indentation

Done

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/first_paint_metric.html:67: for (var ev of
list)
On 2016/06/29 at 18:08:44, charliea wrote:
> same note (here and below): in this CL, please do a direct translation of the
old code into the new idiom. If you feel like you can simplify the code, that's
great, but do so in a later CL

Done

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/a...
File tracing/tracing/model/async_slice.html (right):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/a...
tracing/tracing/model/async_slice.html:106:
findTopmostSlicesRelativeToThisSlice: function*(eventPredicate, opt_this) {
On 2016/06/29 at 18:08:44, charliea wrote:
> What a weird function...

Acknowledged, although I'm just translating the function that was there already.
This function (recursively) gets every element of the slice-subslice tree rooted
at this element, that both (a) satisfies eventPredicate, and (b) doesn't have an
ancestor that satisfies eventPredicate.

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/c...
File tracing/tracing/model/cpu.html (right):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/c...
tracing/tracing/model/cpu.html:64: // Inherit these functions from
EventContainer, since this isn't
On 2016/06/29 at 18:08:44, charliea wrote:
> nit: this comment no longer applies, right?

That is correct. Removed comment

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/d...
File tracing/tracing/model/device.html (right):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/d...
tracing/tracing/model/device.html:99: childEvents: function*() {
On 2016/06/29 at 18:08:44, charliea wrote:
> nit: can delete this if we provide a default ipml of childEvents

Done

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/d...
File tracing/tracing/model/device_test.html (left):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/d...
tracing/tracing/model/device_test.html:53:
test('iterateAllChildEventContainers', function() {
On 2016/06/29 at 18:08:44, charliea wrote:
> Why delete this?

I originally deleted it because it was testing a function,
iterateAllChildEventContainers, that doesn't exist anymore. But I do see now
that you can put this function back in, and have it test the new
childEventContainers functionality.

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/e...
File tracing/tracing/model/event_container.html (left):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/e...
tracing/tracing/model/event_container.html:142: throw new Error('Not
implemented.');
On 2016/06/29 at 18:08:44, charliea wrote:
> Did you mean to delete the not implemented error?
Yes. The blank function is now the default behavior, enabling is to remove other
copies of the blank function from elsewhere in the code.

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/e...
File tracing/tracing/model/event_container.html (right):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/e...
tracing/tracing/model/event_container.html:75: * Gets list of all events in this
and descendant event containers.
On 2016/06/29 at 18:08:44, charliea wrote:
> s/Gets list/Returns an iterable

Done

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/e...
tracing/tracing/model/event_container.html:77: descendantEvents: function*() {
On 2016/06/29 at 18:08:44, charliea wrote:
> Is there no default implementation of childEvents and childEventContainers?
There probably should be (that just does nothing)

Added default implementation

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/e...
tracing/tracing/model/event_container.html:84: * Yields this and all descendant
event containers.
On 2016/06/29 at 18:08:44, charliea wrote:
> s/Yields/Returns an iterable containing

Done

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/h...
File tracing/tracing/model/helpers/chrome_browser_helper.html (right):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/h...
tracing/tracing/model/helpers/chrome_browser_helper.html:59: for (var thread of
this.modelHelper.model.getAllThreads())
On 2016/06/29 at 18:08:44, charliea wrote:
> see note earlier: in a CL this big, we should do a direct translation of the
old code (we can simplify in a follow-up CL)

Done

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/i...
File tracing/tracing/model/ir_coverage.html (right):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/i...
tracing/tracing/model/ir_coverage.html:34: // the set of associated events.
On 2016/06/29 at 18:08:44, charliea wrote:
> nit: bad indentation

Done

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/k...
File tracing/tracing/model/kernel.html (right):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/k...
tracing/tracing/model/kernel.html:130: yield *
ProcessBase.prototype.childEvents.call(this);
On 2016/06/29 at 18:08:44, charliea wrote:
> Looks like this is overriding the prototype's behavior to just call the
prototype? I think we can just get rid of this and the default will look up the
prototype chain anyhow.

Done.

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/o...
File tracing/tracing/model/object_collection.html (left):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/o...
tracing/tracing/model/object_collection.html:43:
findTopmostSlicesInThisContainer: function(eventPredicate, callback,
On 2016/06/29 at 18:08:44, charliea wrote:
> can this be deleted in a follow-up CL?

Now default behavior

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/p...
File tracing/tracing/model/power_series.html (right):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/p...
tracing/tracing/model/power_series.html:121: childEventContainers: function*() {
On 2016/06/29 at 18:08:44, charliea wrote:
> This is default behavior, right?

Empty function removed now that empty function is default

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/p...
File tracing/tracing/model/power_series_test.html (left):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/p...
tracing/tracing/model/power_series_test.html:131: 
On 2016/06/29 at 18:08:45, charliea wrote:
> What's the reason for deleting this? It seems like it's testing that all of
the right stuff is being added in this.childEvents()

Put test back in

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/p...
File tracing/tracing/model/process_base.html (left):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/p...
tracing/tracing/model/process_base.html:55: findTopmostSlicesInThisContainer:
function(eventPredicate, callback,
On 2016/06/29 at 18:08:45, charliea wrote:
> nit: what's the reason for deleting this in this CL?

Blank function is now default

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/p...
File tracing/tracing/model/process_base.html (right):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/p...
tracing/tracing/model/process_base.html:62: childEvents: function*() {
On 2016/06/29 at 18:08:45, charliea wrote:
> same note: isn't this just the default impl?

Removed

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/t...
File tracing/tracing/model/thread.html (right):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/t...
tracing/tracing/model/thread.html:86: if (this.timeSlices &&
this.timeSlices.length)
On 2016/06/29 at 18:08:45, charliea wrote:
> you don't need "&& this.timeSlices.length" here, right? Otherwise, yield *
should just yield an empty array, which is the same as if we hadn't yielded
anything

Done

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/u...
File tracing/tracing/model/user_model/user_model.html (left):

https://codereview.chromium.org/2083213002/diff/60001/tracing/tracing/model/u...
tracing/tracing/model/user_model/user_model.html:47:
findTopmostSlicesInThisContainer: function(eventPredicate, callback,
On 2016/06/29 at 18:08:45, charliea wrote:
> same

Blank function is now the default behavior

[alexandermont, 2016-06-29 20:47:22]

The CQ bit was checked by alexandermont@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-29 20:47:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 20:51:01]

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

[commit-bot: I haz the power, 2016-06-29 20:51:03]

Dry run: Try jobs failed on following builders:
  Catapult Windows Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Wi...)

[charliea (OOO until 10-5), 2016-06-29 21:06:46]

lgtm w/ nits

https://codereview.chromium.org/2083213002/diff/80001/tracing/tracing/metrics...
File tracing/tracing/metrics/system_health/first_paint_metric.html (right):

https://codereview.chromium.org/2083213002/diff/80001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/first_paint_metric.html:55: }
nit: I believe this brace and the one below it should both be shifted to the
left by two spaces

https://codereview.chromium.org/2083213002/diff/80001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/first_paint_metric.html:166: }
nit: brace needs to go to the left by two spaces

https://codereview.chromium.org/2083213002/diff/80001/tracing/tracing/model/e...
File tracing/tracing/model/event_container.html (right):

https://codereview.chromium.org/2083213002/diff/80001/tracing/tracing/model/e...
tracing/tracing/model/event_container.html:75: /**
nit: /** needs to be moved one space to the left.

https://codereview.chromium.org/2083213002/diff/80001/tracing/tracing/model/e...
tracing/tracing/model/event_container.html:82: * Returns iterable of all events
in this and descendant event containers.
meganit: should probably say "Return an iterable" like all of the other comments

[alexandermont, 2016-06-29 21:22:32]

The CQ bit was checked by alexandermont@chromium.org

[alexandermont, 2016-06-29 21:22:33]

https://codereview.chromium.org/2083213002/diff/80001/tracing/tracing/metrics...
File tracing/tracing/metrics/system_health/first_paint_metric.html (right):

https://codereview.chromium.org/2083213002/diff/80001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/first_paint_metric.html:55: }
On 2016/06/29 at 21:06:46, charliea wrote:
> nit: I believe this brace and the one below it should both be shifted to the
left by two spaces

Done

https://codereview.chromium.org/2083213002/diff/80001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/first_paint_metric.html:166: }
On 2016/06/29 at 21:06:46, charliea wrote:
> nit: brace needs to go to the left by two spaces

Done

https://codereview.chromium.org/2083213002/diff/80001/tracing/tracing/model/e...
File tracing/tracing/model/event_container.html (right):

https://codereview.chromium.org/2083213002/diff/80001/tracing/tracing/model/e...
tracing/tracing/model/event_container.html:75: /**
On 2016/06/29 at 21:06:46, charliea wrote:
> nit: /** needs to be moved one space to the left.

Done

https://codereview.chromium.org/2083213002/diff/80001/tracing/tracing/model/e...
tracing/tracing/model/event_container.html:82: * Returns iterable of all events
in this and descendant event containers.
On 2016/06/29 at 21:06:46, charliea wrote:
> meganit: should probably say "Return an iterable" like all of the other
comments

Done

[alexandermont, 2016-06-29 21:22:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
nednguyen@google.com, charliea@chromium.org
Link to the patchset: https://codereview.chromium.org/2083213002/#ps100001
(title: "fix nits, rebase")

[commit-bot: I haz the power, 2016-06-29 21:22:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 21:55:35]

Description was changed from

==========
Change call-sites in trace viewer to use generators instead of iteration
functions.

Remove the iteration functions (iterateAllEventContainers, etc.) from the trace
viewer and instead use the versions that iterate through the generators
(childEvents, descendantEvents, childEventContainers,
descendantEventContainers)/.

BUG=catapult:#2407
==========

to

==========
Change call-sites in trace viewer to use generators instead of iteration
functions.

Remove the iteration functions (iterateAllEventContainers, etc.) from the trace
viewer and instead use the versions that iterate through the generators
(childEvents, descendantEvents, childEventContainers,
descendantEventContainers)/.

BUG=catapult:#2407

Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2016-06-29 21:55:36]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...