[webkit-reviews] review granted: [Bug 117941] code coverage report : [Attachment 205327] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 117941: code coverage report
https://bugs.webkit.org/show_bug.cgi?id=117941

Attachment 205327: Patch
https://bugs.webkit.org/attachment.cgi?id=205327&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=205327&action=review


Overall this looks good. I'd like to see a second patch with more style
consistency and a few of the other comments addressed, but the logic looks good
so r+

> Tools/CodeCoverage/report.html:77
> +		   if(hits == -1)
> +		       return 'white'; // Non-code lines
> +		   else if(hits == 0)
> +		       return 'orange'; // Unexecuted lines

Style: "if(" => "if ("

Suggestion: For JS in the Web Inspector we prefer to use === wherever possible.
These and others could be made strict equality.

> Tools/CodeCoverage/report.html:79
> +		       // Executed lines are between red and green

Nit: Comments should be sentences and end with a period. Here and throughout
the patch.

> Tools/CodeCoverage/report.html:81
> +		       return 'rgb('+ relativeHeat + ',' + (255 - relativeHeat)
+ ', 0)';

Style: space between substrings, so between 'rob(' and the +.

> Tools/CodeCoverage/report.html:88
> +		   return 'rgb('+ (255 - value) + ',' + value + ', 0)';

Ditto.

> Tools/CodeCoverage/report.html:93
> +		   var children =
this.parentNode.childNodes[this.parentNode.childNodes.length - 1];

Suggestion: this.parentNode.lastChild

> Tools/CodeCoverage/report.html:114
> +		   for (var i = 0; i < fileData.hitLines.length; i++)

Suggestion: Newlines before/after these for loops would make the code a little
more readable.

> Tools/CodeCoverage/report.html:133
> +		       textColumn.style.background =
getHeatBackgroundColor(hits[i],fileData.maxHeat);

Style: space after comma

> Tools/CodeCoverage/report.html:135
> +		       textColumn.style.whiteSpace = 'nowrap';
> +		       textColumn.style.fontFamily = 'Courier, monospace';

How about giving the textColumn a className and putting these styles in CSS?

> Tools/CodeCoverage/report.html:146
> +	       function fileClicked()

I'd prefer to see an "event" parameter to this click handler instead of
depending on window.event below. Being explicit makes code easier to follow.

> Tools/CodeCoverage/report.html:154
> +		   var xmlhttp=new XMLHttpRequest();
> +		   xmlhttp.onreadystatechange=function()
> +		   {
> +		       if (xmlhttp.readyState==4) {
> +			   var codeviewer =
document.getElementById('codeviewer');
> +			  
codeviewer.replaceChild(processFile(xmlhttp.fileData,xmlhttp.responseText),
codeviewer.childNodes[0]);
> +		       }

Style: spaces around =, ==, and comma. This comment applies here and throughout
the code.

"xmlhttp" is a weird name, how about "xhr"?

> Tools/CodeCoverage/report.html:190
> +		   a.href = 'javascript:void(0)';

We try to avoid javascript: whenever possible. This could just be:

    a.href = '#';

And inside the event handler (fileClicked) you can call:

    event.preventDefault();

> Tools/CodeCoverage/report.html:191
> +		   a.onclick = fileClicked;

I'd prefer to see addEventListener. How about:

    a.addEventListener('click', fileClicked.bind(a));

Or just "fileClicked" and instead of using "this" in the handler you could use
"event.target".

> Tools/CodeCoverage/report.html:211
> +		   var subdirNames = [];
> +		   var fileNames = [];
> +		   for (var subdir in dir.subdirs)
> +		       subdirNames.push(subdir);
> +		   for (var file in dir.files)
> +		       fileNames.push(file);
> +		   subdirNames.sort();
> +		   fileNames.sort();

Seems like this could be:

    var subdirNames = Object.keys(dir.subdirs).sort();
    var fileNames = Object.keys(dir.files).sort();

> Tools/CodeCoverage/report.html:222
> +		   img.onclick = expandClicked;

Style: addEventListener. You may need to bind so that "this" makes sense in the
handler.

> Tools/CodeCoverage/report.html:235
> +		   li.appendChild(makeGraphs(dir));

There is a lot of makeGraphs going on. Could some of this be done lazily like
when you click an item?

> Tools/CodeCoverage/report.html:291
> +		   directory.coverage = directory.totalHitLines /
directory.totalLines;

Is it possible that directory.totalLines is 0? If so then coverage would be
Infinity. You should 0 check like you do with branchCoverage just in case.

> Tools/CodeCoverage/report.html:314
> +		   var directory = {};

How about naming this rootDirectory, or something more descriptive?

> Tools/CodeCoverage/report.html:336
> +		   directories.replaceChild(report, directories.childNodes[0]);


Suggestion: directories.childNodes[0] => directories.firstChild

> Tools/Scripts/generate-coverage-data:80
> +# Open report
> +my $url = 'file://' . sourceDir() . '/Tools/CodeCoverage/report.html?' .
$resultName;
> +system "open -g -a Safari";
> +system "osascript -e \"tell application \\\"Safari\\\" to make new document
with properties {URL:\\\"" . $url . "\\\"} at front\"";

Why not just do:

    system "open '$url'";

Does the Apple Script (Mac only by the way) do anything special?

It might also be nice to have a way to open a URL that supports other OSs, like
linux. I see there are some Perl modules that do that, but nothing built-in.