Update user model to identify WebGL animations.

Update user model to identify WebGL animations.

BUG=catapult:#2446

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

[alexandermont, 2016-07-21 17:52:26]

alexandermont@chromium.org changed reviewers:
+ charliea@chromium.org, eakuefner@chromium.org, kbr@chromium.org

[Ken Russell (switch to Gerrit), 2016-07-21 19:14:36]

Very nice. I don't really know this code but it makes sense. LGTM

https://codereview.chromium.org/2169963002/diff/1/tracing/tracing/importer/fi...
File tracing/tracing/importer/find_input_expectations.html (right):

https://codereview.chromium.org/2169963002/diff/1/tracing/tracing/importer/fi...
tracing/tracing/importer/find_input_expectations.html:179:
handleWebGlAnimations,
Should this be "handleWebGLAnimations"? (capital L)

https://codereview.chromium.org/2169963002/diff/1/tracing/tracing/importer/fi...
tracing/tracing/importer/find_input_expectations.html:823:
scriptedAnimationEvents.sort(compareEvents);
What's the comparator -- time on the timeline? (Sorry, I don't know this code)

[alexandermont, 2016-07-22 18:40:50]

https://codereview.chromium.org/2169963002/diff/1/tracing/tracing/importer/fi...
File tracing/tracing/importer/find_input_expectations.html (right):

https://codereview.chromium.org/2169963002/diff/1/tracing/tracing/importer/fi...
tracing/tracing/importer/find_input_expectations.html:179:
handleWebGlAnimations,
On 2016/07/21 at 19:14:35, Ken Russell wrote:
> Should this be "handleWebGLAnimations"? (capital L)

Done

https://codereview.chromium.org/2169963002/diff/1/tracing/tracing/importer/fi...
tracing/tracing/importer/find_input_expectations.html:823:
scriptedAnimationEvents.sort(compareEvents);
On 2016/07/21 at 19:14:36, Ken Russell wrote:
> What's the comparator -- time on the timeline? (Sorry, I don't know this code)

The comparator is compareEvents - defined near the top of this file - which is
the start time (but uses end time and GUID as tiebreakers)

[charliea (OOO until 10-5), 2016-07-22 20:06:09]

https://codereview.chromium.org/2169963002/diff/1/tracing/tracing/importer/fi...
File tracing/tracing/importer/find_input_expectations.html (right):

https://codereview.chromium.org/2169963002/diff/1/tracing/tracing/importer/fi...
tracing/tracing/importer/find_input_expectations.html:179:
handleWebGlAnimations,
On 2016/07/21 at 19:14:35, Ken Russell wrote:
> Should this be "handleWebGLAnimations"? (capital L)

+1

https://codereview.chromium.org/2169963002/diff/1/tracing/tracing/importer/fi...
tracing/tracing/importer/find_input_expectations.html:810: {
nit: { goes on same line as for()

https://codereview.chromium.org/2169963002/diff/1/tracing/tracing/importer/fi...
tracing/tracing/importer/find_input_expectations.html:818: // close by. These
will be called the webGlEvents.
s/webGl/webGL (here and elsewhere)

https://codereview.chromium.org/2169963002/diff/1/tracing/tracing/importer/fi...
tracing/tracing/importer/find_input_expectations.html:824: var
scriptedAnimationIterator = scriptedAnimationEvents[Symbol.iterator]();
I think that keeping track of the scriptedAnimations via iterators is a little
unwieldy here, and it might be a little cleaner to just use an index. That would
also give us the ability to do what I suggested in the loop below

https://codereview.chromium.org/2169963002/diff/1/tracing/tracing/importer/fi...
tracing/tracing/importer/find_input_expectations.html:830: (scriptEvent.start <
(event.start - ANIMATION_MERGE_THRESHOLD_MS)))
nit: line continuations are indented by 4 spaces, not 2

https://codereview.chromium.org/2169963002/diff/1/tracing/tracing/importer/fi...
tracing/tracing/importer/find_input_expectations.html:1091: // Don't remove them
if it's a WebGL animation
Is the above any less true for webGL animations?

https://codereview.chromium.org/2169963002/diff/20001/tracing/tracing/importe...
File tracing/tracing/importer/find_input_expectations.html (right):

https://codereview.chromium.org/2169963002/diff/20001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:807: function
handleWebGLAnimations(modelHelper, sortedInputEvents) {
High level thought: I think that this function could be a lot more readable by
breaking it up into smaller functions. Conceptually, the whole thing is:

function handleWebGLAnimations(modelHelper, sortedInputEvents) {
  var prepareMailboxEvents = [];
  var scriptedAnimationEvents = [];

  findWebGLEvents(prepareMailboxEvents, scriptedAnimationEvents);

  var webGLMailboxEvents = findMailboxEventsAfterAnimationEvents(
      prepareMailboxEvents, scriptedAnimationEvents);

  return createWebGLProtoExpectations(webGLMailboxEvents);
}

This is functionally equivalent to the function you have written below, but it's
easier for someone scanning the function to see "here are the high level things
that the function is doing, and I can go to each of those functions if I'm
interested in a specific part".

This generally aligns with the rule "if you want to write a comment describing
what the code is doing, just pull it out and write a new function. try to
reserve comments for describing WHY the code is doing what it's doing (e.g.
describing magical constants in code, like ANIMATION_MERGE_THRESHOLD_MS)

This is especially true because this function is pretty long (60 lines), and has
a lot going on in it.

https://codereview.chromium.org/2169963002/diff/20001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:813: {
nit: { goes on same line as for()

https://codereview.chromium.org/2169963002/diff/20001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:821: // close by. These
will be called the webGlEvents.
s/webGl/webGL (here and everywhere)

https://codereview.chromium.org/2169963002/diff/20001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:833: (scriptEvent.start <
(event.start - ANIMATION_MERGE_THRESHOLD_MS)))
nit: line continuations are indented by 4 spaces, not 2

https://codereview.chromium.org/2169963002/diff/20001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:849: if (!currentPE) {
I think that there's a bug here where you push a proto expectation once for
every mailbox event that goes into it. I think this would be fixed if you only
pushed the proto expectation when it was created, and this would allow you to
simplify the code, too:

function shouldMerge(currentPE, event) {
  return currentPE !== undefined && 
      currentPE.isNear(event, ANIMATION_MERGE_THRESHOLD_MS);
}

if (shouldMerge(currentPE, event)) {
  currentPE.pushEvent(event);
} else {
  currentPE = new ProtoExpectation(
      ProtoExpectation.ANIMATION_TYPE, WEBGL_IR_NAME);
  currentPE.pushEvent(event);
  protoExpectations.push(currentPE);
}

Also, there should definitely be unit tests that exercise:

  1) The case where there are no mailbox events
  2) The case where there are mailbox events, but none of them are near script
events
  3) The case where there are proto expectations containing single mailbox
events
  4) The case where there are proto expectations containing multiple mailbox
events

https://codereview.chromium.org/2169963002/diff/20001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:856: if
(currentPE.isNear(event, ANIMATION_MERGE_THRESHOLD_MS))
supernit: if the else branch has braces, the if branch also needs them

https://codereview.chromium.org/2169963002/diff/20001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:1094: // Don't remove them
if it's a WebGL animation
Shouldn't there still be frames when a WebGL animation is active?

[alexandermont, 2016-07-22 23:39:07]

alexandermont@chromium.org changed reviewers:
+ nduca@chromium.org

[alexandermont, 2016-07-22 23:39:08]

https://codereview.chromium.org/2169963002/diff/20001/tracing/tracing/importe...
File tracing/tracing/importer/find_input_expectations.html (right):

https://codereview.chromium.org/2169963002/diff/20001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:807: function
handleWebGLAnimations(modelHelper, sortedInputEvents) {
On 2016/07/22 at 20:06:09, charliea wrote:
> High level thought: I think that this function could be a lot more readable by
breaking it up into smaller functions. Conceptually, the whole thing is:
> 
> function handleWebGLAnimations(modelHelper, sortedInputEvents) {
>   var prepareMailboxEvents = [];
>   var scriptedAnimationEvents = [];
> 
>   findWebGLEvents(prepareMailboxEvents, scriptedAnimationEvents);
> 
>   var webGLMailboxEvents = findMailboxEventsAfterAnimationEvents(
>       prepareMailboxEvents, scriptedAnimationEvents);
> 
>   return createWebGLProtoExpectations(webGLMailboxEvents);
> }
> 
> This is functionally equivalent to the function you have written below, but
it's easier for someone scanning the function to see "here are the high level
things that the function is doing, and I can go to each of those functions if
I'm interested in a specific part".
> 
> This generally aligns with the rule "if you want to write a comment describing
what the code is doing, just pull it out and write a new function. try to
reserve comments for describing WHY the code is doing what it's doing (e.g.
describing magical constants in code, like ANIMATION_MERGE_THRESHOLD_MS)
> 
> This is especially true because this function is pretty long (60 lines), and
has a lot going on in it.

Done

https://codereview.chromium.org/2169963002/diff/20001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:813: {
On 2016/07/22 at 20:06:08, charliea wrote:
> nit: { goes on same line as for()

Done

https://codereview.chromium.org/2169963002/diff/20001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:821: // close by. These
will be called the webGlEvents.
On 2016/07/22 at 20:06:08, charliea wrote:
> s/webGl/webGL (here and everywhere)

Done

https://codereview.chromium.org/2169963002/diff/20001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:833: (scriptEvent.start <
(event.start - ANIMATION_MERGE_THRESHOLD_MS)))
On 2016/07/22 at 20:06:08, charliea wrote:
> nit: line continuations are indented by 4 spaces, not 2

Done

https://codereview.chromium.org/2169963002/diff/20001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:849: if (!currentPE) {
On 2016/07/22 at 20:06:09, charliea wrote:
> I think that there's a bug here where you push a proto expectation once for
every mailbox event that goes into it. I think this would be fixed if you only
pushed the proto expectation when it was created, and this would allow you to
simplify the code, too:
> 
> function shouldMerge(currentPE, event) {
>   return currentPE !== undefined && 
>       currentPE.isNear(event, ANIMATION_MERGE_THRESHOLD_MS);
> }
> 
> if (shouldMerge(currentPE, event)) {
>   currentPE.pushEvent(event);
> } else {
>   currentPE = new ProtoExpectation(
>       ProtoExpectation.ANIMATION_TYPE, WEBGL_IR_NAME);
>   currentPE.pushEvent(event);
>   protoExpectations.push(currentPE);
> }
> 
> Also, there should definitely be unit tests that exercise:
> 
>   1) The case where there are no mailbox events
>   2) The case where there are mailbox events, but none of them are near script
events
>   3) The case where there are proto expectations containing single mailbox
events
>   4) The case where there are proto expectations containing multiple mailbox
events

I don't think there actually was a bug: I *was* only pushing the
ProtoExpectation when it was created (the branch where you just add a new event
to the existing ProtoExpectation just pushes the event onto the
ProtoExpectation, not the ProtoExpectation onto the list of expectations. But I
did change this to avoid the duplication of code.

Additional test cases added.

https://codereview.chromium.org/2169963002/diff/20001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:856: if
(currentPE.isNear(event, ANIMATION_MERGE_THRESHOLD_MS))
On 2016/07/22 at 20:06:09, charliea wrote:
> supernit: if the else branch has braces, the if branch also needs them

Done

https://codereview.chromium.org/2169963002/diff/20001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:1094: // Don't remove them
if it's a WebGL animation
On 2016/07/22 at 20:06:09, charliea wrote:
> Shouldn't there still be frames when a WebGL animation is active?

I think there are still frames, but there aren't any
BenchmarkInstrumentation::ImplThreadRenderingStats messages generated, and
that's what this is checking for. I asked kbr@ and he said that he wasn't
familiar with that event type - I would guess that WebGL just doesn't generate
these kinds of events. It looks like nduca@ wrote most of the file
(chrome_model_helper.py) that has this check; I will ask him about that.

[charliea (OOO until 10-5), 2016-07-26 20:20:01]

https://codereview.chromium.org/2169963002/diff/20001/tracing/tracing/importe...
File tracing/tracing/importer/find_input_expectations.html (right):

https://codereview.chromium.org/2169963002/diff/20001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:1094: // Don't remove them
if it's a WebGL animation
On 2016/07/22 23:39:08, alexandermont wrote:
> On 2016/07/22 at 20:06:09, charliea wrote:
> > Shouldn't there still be frames when a WebGL animation is active?
> 
> I think there are still frames, but there aren't any
> BenchmarkInstrumentation::ImplThreadRenderingStats messages generated, and
> that's what this is checking for. I asked kbr@ and he said that he wasn't
> familiar with that event type - I would guess that WebGL just doesn't generate
> these kinds of events. It looks like nduca@ wrote most of the file
> (chrome_model_helper.py) that has this check; I will ask him about that.

My thought is that, if these frame events aren't generated for webGL frames,
we're probably hooked in at the wrong level of abstraction to understand what
frame events really look like. Probably okay to leave as-is for now though.

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
File tracing/tracing/importer/find_input_expectations.html (right):

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:804: // Get
WebGL-animation related events.
nit: function documentation is surrounded with 

/**
 * Function doc goes here...
 */

not //

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:804: // Get
WebGL-animation related events.
nit: this function doc could be a lot better. Given that this function has to
have kind of a funky interface for performance reasons (not iterating through
the events twice), a better JSdoc is in order.

Important things for the jsdoc to include are:

  - It's assumed that modelHelper, mailboxEvents, and animationEvents are all
defined, not null, and have specific types
  - mailboxEvents and animationEvents are both modified
  - It'd probably be useful to specify which events are getting added to
mailboxEvents and animationEvents ('DrawingBuffer::prepareMailbox',
'PageAnimator::serviceScriptedAnimations')

It's definitely worth spending some time reading through http://usejsdoc.org/

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:809: if (event.title ===
'PageAnimator::serviceScriptedAnimations')
nit: else if

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:814: // Keep only the
prepareMailboxEvents that have a scriptedAnimationEvent
wrong jsdoc form

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:816: function
findMailboxEventsNearAnimationEvents(mailboxEvents,
function findMailboxEventsNearAnimationEvents(
    mailboxEvents, animationEvents) {

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:823: var animationIterator
= animationEvents[Symbol.iterator]();
It seems like this logic might be clearer:

var animationEventIterator = animationEvents[Symbol.iterator]();
var mailboxEventIterator = mailboxEvents[Symbol.iterator]();

while (!mailboxEventIterator.done && !animationEventIterator.done) {
  var mailboxEvent = mailboxEventIterator.value;
  var animationEvent = animationEventIterator.value;

  // If the animation events are too far ahead of the mailbox
  // events, move to the next mailbox event.
  if (mailboxEvent.start < 
      (animationEvent.start - ANIMATION_MERGE_THRESHOLD_MS) {
    mailboxEventIterator.next();
    continue;
  }

  // If the mailbox events are too far ahead of the animation 
  // events, move to the next animation event.
  if (mailboxEvent.start > 
      (animationEvent.start + ANIMATION_MERGE_THRESHOLD_MS) {
    animationEventIterator.next()
    continue;
  }

  filteredEvents.push(mailboxEvent);

  // We only add each mailbox event once.
  mailboxEventIterator.next();
}

Things I like about this:

  - All of the loop conditions are expressed in the condition statement of the
loop

Things I don't like about this:

  - Your way of iterating makes it a little clearer that we only add each
mailbox event once.

Your call, though. Given how hard it is to make this code stupidly obvious what
it's doing, and the difficulty of creating functions to express the high-level
concepts of what's happening, it's probably worth adding some comments no matter
what. (I know I'm being hypocritical - I usually prefer comments as a last
resort, but I don't see any way around them here.)

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:824: var scriptEvent =
animationIterator.next().value;
nit: not crazy about the value of the animationIterator being "scriptEvent".
Seems like we should be consistent (e.g. animationIterator/animationEvent)

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:839: // Merge consecutive
mailbox events into a ProtoExpectation. Note. only
wrong jsdoc form

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:839: // Merge consecutive
mailbox events into a ProtoExpectation. Note. only
s/Note./Note:

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:869:
getWebGLEvents(modelHelper, prepareMailboxEvents, scriptedAnimationEvents);
supernit: what's the rationale behind this function starting with "get" but the
next function starting with "find"? (given that this file is called
"find_input_expectations", "find" makes more sense as the verb for both to me)

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:873: return
getProtoExpectationsFromMailboxEvents(webGLMailboxEvents);
suggestion: given that the proto expectations don't exist when this function is
called, "create..." makes more sense than "get" as the verb for this function

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:1101: // Don't remove them
if it's a WebGL animation
nit: add your comment to the one above it. Otherwise, it looks like that comment
isn't associated with this branch

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
File tracing/tracing/importer/user_model_builder_test.html (right):

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
tracing/tracing/importer/user_model_builder_test.html:422:
test('webGlAnimations_oneSequence', function() {
s/webGl/webGL everywhere

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
tracing/tracing/importer/user_model_builder_test.html:422:
test('webGlAnimations_oneSequence', function() {
maybe s/Sequence/Animation?

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
tracing/tracing/importer/user_model_builder_test.html:493:
test('webGlAnimations_oneWithAnimatorOneWithout', function() {
Are we calling these animators, or animation events, or script events? I don't
particularly care, but it seems like it makes sense to be consistent.

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
tracing/tracing/importer/user_model_builder_test.html:570: 
nit: delete line

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
tracing/tracing/importer/user_model_builder_test.html:571:
test('webGlAnimations_oneEvent', function() {
nit: what does this test that the other tests don't? Seems like it's basically
covered by others

[alexandermont, 2016-07-26 22:12:23]

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
File tracing/tracing/importer/find_input_expectations.html (right):

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:804: // Get
WebGL-animation related events.
On 2016/07/26 at 20:20:00, charliea wrote:
> nit: function documentation is surrounded with 
> 
> /**
>  * Function doc goes here...
>  */
> 
> not //

Done. Note that all the other functions in this file use // for the multi-line
comments. I suppose that in another CL, we should change all these comments into
/** ... */ comments?

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:814: // Keep only the
prepareMailboxEvents that have a scriptedAnimationEvent
On 2016/07/26 at 20:20:00, charliea wrote:
> wrong jsdoc form

Done

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:816: function
findMailboxEventsNearAnimationEvents(mailboxEvents,
On 2016/07/26 at 20:20:00, charliea wrote:
> function findMailboxEventsNearAnimationEvents(
>     mailboxEvents, animationEvents) {

Done

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:823: var animationIterator
= animationEvents[Symbol.iterator]();
On 2016/07/26 at 20:20:00, charliea wrote:
> It seems like this logic might be clearer:
> 
> var animationEventIterator = animationEvents[Symbol.iterator]();
> var mailboxEventIterator = mailboxEvents[Symbol.iterator]();
> 
> while (!mailboxEventIterator.done && !animationEventIterator.done) {
>   var mailboxEvent = mailboxEventIterator.value;
>   var animationEvent = animationEventIterator.value;
> 
>   // If the animation events are too far ahead of the mailbox
>   // events, move to the next mailbox event.
>   if (mailboxEvent.start < 
>       (animationEvent.start - ANIMATION_MERGE_THRESHOLD_MS) {
>     mailboxEventIterator.next();
>     continue;
>   }
> 
>   // If the mailbox events are too far ahead of the animation 
>   // events, move to the next animation event.
>   if (mailboxEvent.start > 
>       (animationEvent.start + ANIMATION_MERGE_THRESHOLD_MS) {
>     animationEventIterator.next()
>     continue;
>   }
> 
>   filteredEvents.push(mailboxEvent);
> 
>   // We only add each mailbox event once.
>   mailboxEventIterator.next();
> }
> 
> Things I like about this:
> 
>   - All of the loop conditions are expressed in the condition statement of the
loop
> 
> Things I don't like about this:
> 
>   - Your way of iterating makes it a little clearer that we only add each
mailbox event once.
> 
> Your call, though. Given how hard it is to make this code stupidly obvious
what it's doing, and the difficulty of creating functions to express the
high-level concepts of what's happening, it's probably worth adding some
comments no matter what. (I know I'm being hypocritical - I usually prefer
comments as a last resort, but I don't see any way around them here.)

Done

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:824: var scriptEvent =
animationIterator.next().value;
On 2016/07/26 at 20:20:00, charliea wrote:
> nit: not crazy about the value of the animationIterator being "scriptEvent".
Seems like we should be consistent (e.g. animationIterator/animationEvent)

Done

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:839: // Merge consecutive
mailbox events into a ProtoExpectation. Note. only
On 2016/07/26 at 20:20:01, charliea wrote:
> s/Note./Note:

Done

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:869:
getWebGLEvents(modelHelper, prepareMailboxEvents, scriptedAnimationEvents);
On 2016/07/26 at 20:20:00, charliea wrote:
> supernit: what's the rationale behind this function starting with "get" but
the next function starting with "find"? (given that this file is called
"find_input_expectations", "find" makes more sense as the verb for both to me)

Done

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:873: return
getProtoExpectationsFromMailboxEvents(webGLMailboxEvents);
On 2016/07/26 at 20:20:00, charliea wrote:
> suggestion: given that the proto expectations don't exist when this function
is called, "create..." makes more sense than "get" as the verb for this function

Done

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:1101: // Don't remove them
if it's a WebGL animation
On 2016/07/26 at 20:20:00, charliea wrote:
> nit: add your comment to the one above it. Otherwise, it looks like that
comment isn't associated with this branch

Done

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
File tracing/tracing/importer/user_model_builder_test.html (right):

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
tracing/tracing/importer/user_model_builder_test.html:422:
test('webGlAnimations_oneSequence', function() {
On 2016/07/26 at 20:20:01, charliea wrote:
> maybe s/Sequence/Animation?

Done

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
tracing/tracing/importer/user_model_builder_test.html:493:
test('webGlAnimations_oneWithAnimatorOneWithout', function() {
On 2016/07/26 at 20:20:01, charliea wrote:
> Are we calling these animators, or animation events, or script events? I don't
particularly care, but it seems like it makes sense to be consistent.

Done

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
tracing/tracing/importer/user_model_builder_test.html:570: 
On 2016/07/26 at 20:20:01, charliea wrote:
> nit: delete line

Done

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
tracing/tracing/importer/user_model_builder_test.html:571:
test('webGlAnimations_oneEvent', function() {
On 2016/07/26 at 20:20:01, charliea wrote:
> nit: what does this test that the other tests don't? Seems like it's basically
covered by others

This tests the case where there's only one mailbox event, rather than multiple
mailbox events.

[charliea (OOO until 10-5), 2016-07-27 19:56:31]

Looks great! lgtm

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
File tracing/tracing/importer/find_input_expectations.html (right):

https://codereview.chromium.org/2169963002/diff/40001/tracing/tracing/importe...
tracing/tracing/importer/find_input_expectations.html:804: // Get
WebGL-animation related events.
On 2016/07/26 22:12:22, alexandermont wrote:
> On 2016/07/26 at 20:20:00, charliea wrote:
> > nit: function documentation is surrounded with 
> > 
> > /**
> >  * Function doc goes here...
> >  */
> > 
> > not //
> 
> Done. Note that all the other functions in this file use // for the multi-line
> comments. I suppose that in another CL, we should change all these comments
into
> /** ... */ comments?

Yep! They should definitely be /** */... sorry that those made it in there in
the first place.

#NeedABetterJSLinter

[alexandermont1, 2016-07-27 20:41:48]

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

[alexandermont, 2016-07-28 21:18:42]

The CQ bit was checked by alexandermont@chromium.org

[alexandermont, 2016-07-28 21:18:42]

The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org
Link to the patchset: https://codereview.chromium.org/2169963002/#ps60001
(title: "fix whitespace")

[commit-bot: I haz the power, 2016-07-28 21:18:44]

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-07-28 22:02:56]

Description was changed from

==========
Update user model to identify WebGL animations.

BUG=catapult:#2446
==========

to

==========
Update user model to identify WebGL animations.

BUG=catapult:#2446

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

[commit-bot: I haz the power, 2016-07-28 22:02:57]

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