[webkit-reviews] review denied: [Bug 31372] WebInspector: Adds Timer record tests for Timeline agent records : [Attachment 43001] WebInspector: Adds unit test for Timer records from the Timeline agent.

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Eric Ayers
<zundel at google.com>'s request for review:
Bug 31372: WebInspector: Adds Timer record tests for Timeline agent records
https://bugs.webkit.org/show_bug.cgi?id=31372

Attachment 43001: WebInspector: Adds unit test for Timer records from the
Timeline agent. 
https://bugs.webkit.org/attachment.cgi?id=43001&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
> +// Look for the TimerFire event that correlates to the 
> +// to the <script> tag below.

to the is used twice


> +   function analyzeTimerFire(record) 

I think I've seen this pattern before. Can we extract something receiving
timelineMark and a printcallback or something?

> +	   var numChildren = record.children ? record.children.length : 0;
> +	   // Look for a MarkTimeline as a direct child.
> +	   for (var i = 0; i < numChildren; ++i) {
> +	       var child = record.children[i];
> +	       if (child.type === timelineAgentRecordType.MarkTimeline &&
child.data.message === timelineMark) {
> +		   return record;
> +	       }
> +	   }

This snippet should really be reused.


> +function analyzeTimerFire(record) 
> +{
> +    var numChildren = record.children ? record.children.length : 0;
> +    for (var i = 0; i < numChildren; ++i) {
> +	   var child = record.children[i];
> +	   if (child.type === timelineAgentRecordType.MarkTimeline &&
child.data.message === timelineMark) {
> +	       printTimelineRecordProperties(record);
> +	       return true;
> +	   }
> +    }
> +    return false;

ditto


> +    // Look for the install that corresponds to the timer fire.
> +    for (var i = 0 ; i < numRecords; ++i) {
> +	   var record = timelineRecords[i];
> +	   found = findTimerRecord(timelineAgentRecordType.TimerInstall,
record, timerId);
> +	   if (found)
> +	       break;

This pattern is also happening all the time.

JavaScript is a dynamic language allowing you to pass functions as parameters,
etc. 
- Implementing forEachRecord would do some code reuse for you.
- findRecord(type, goldenData) where golden is a set of properties that should
be set in data would also cover lots of stuff.
like the snipet above would convert into
findRecord(timelineAgentRecordType.MarkTimeline, { message : timelineMark }); 
or printTimelineRecordProperties(timelineAgentRecordType.MarkTimeline, {
message : timelineMark }) to reuse even more.
- the approach above would convert findTimerInstall into
findRecort(timelineAgentRecordType.TimerInstall, { timerId : timerId }); and
should work for eval as well.

In other words, please extract some methods. As you create more tests we get
more and more code duplication.