[Webkit-unassigned] [Bug 117941] code coverage report

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=117941


Joseph Pecoraro <joepeck at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #205327|review?                     |review+
               Flag|                            |




--- Comment #6 from Joseph Pecoraro <joepeck at webkit.org>  2013-06-25 11:58:13 PST ---
(From update of attachment 205327)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.