The script generate-coverage-data has no way to view its results.
Created attachment 205325 [details] Patch
Created attachment 205327 [details] Patch
Comment on attachment 205327 [details] Patch Attachment 205327 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/953201 New failing tests: fast/media/mq-transform-03.html fast/forms/search-event-delay.html fast/media/mq-transform-02.html platform/win/accessibility/multiple-select-element-role.html
Created attachment 205350 [details] Archive of layout-test-results from APPLE-EWS-3 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: APPLE-EWS-3 Port: win-future Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit
There is no way this caused any failures because the only changes were to tools that nobody uses right now.
Comment on attachment 205327 [details] Patch 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.
> 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. I also want a cross-platform way to open a url, but system "open '$url'" removes the query, which I use to locate the data file.
Created attachment 205424 [details] Patch
> There is a lot of makeGraphs going on. Could some of this be done lazily like when you click an item? The calls to makeGraphs don't delay loading significantly, and they are hidden in groups when all subdirectories are collapsed. I think this would be an unnecessary design change.
Comment on attachment 205424 [details] Patch Attachment 205424 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/925124
Comment on attachment 205424 [details] Patch Attachment 205424 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/944171
Comment on attachment 205424 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205424&action=review > Tools/CodeCoverage/report.html:73 > + // This is the contents of the images left of directories. Why don't we use inline svg instead? > Tools/CodeCoverage/report.html:353 > + var dataFilename = '../../WebKitBuild/Coverage/' + window.location.search.substring(1) + '.json'; I'm not sure if it's a good idea to hard code the path like this. Presumably, there are situations in which you want to copy files between different computers, etc... and still want to be able to open it on the viewer. > Tools/CodeCoverage/report.html:355 > + xhr.open('GET', dataFilename, true); > + xhr.send(); This is not going to work unless the person explicitly disables local file restriction. I would have preferred if we had used JSONP or generated a html file that contained this script & JSON.
Created attachment 205428 [details] Patch
Comment on attachment 205428 [details] Patch Attachment 205428 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/912737
Comment on attachment 205428 [details] Patch Attachment 205428 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/848548
Created attachment 205512 [details] Patch
Comment on attachment 205512 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205512&action=review > Tools/CodeCoverage/results-template.html:356 > + <script id="json" type="application/json">%PeformanceTestsResultsJSON%</script> We probably shouldn't call this PeformanceTestsResultsJSON.
(In reply to comment #17) > (From update of attachment 205512 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=205512&action=review > > > Tools/CodeCoverage/results-template.html:356 > > + <script id="json" type="application/json">%PeformanceTestsResultsJSON%</script> > > We probably shouldn't call this PeformanceTestsResultsJSON. That's what I get for copying code and only checking to see that it works without actually reading it :)
Created attachment 205513 [details] Patch
Comment on attachment 205428 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=205428&action=review Looks good to me > Tools/CodeCoverage/results-template.html:8 > + padding: 0px; Suggestion: Everywhere you have 0, you can drop the units "px", "%", etc. But I'll leave that decision up to you. > Tools/CodeCoverage/results-template.html:159 > + xhr.onreadystatechange=function() Style: '=' => ' = ' > Tools/CodeCoverage/results-template.html:161 > + if (xhr.readyState === 4) { Suggestion: 4 => XMLHttpRequest.DONE > Tools/CodeCoverage/results-template.html:197 > + li.className='file'; Style: '=' => ' = ' > Tools/CodeCoverage/results-template.html:234 > + children.style.display=''; > + } else { > + img.src = rightArrow; > + children.style.display='none'; Style: '=' => ' = ' > Tools/CodeCoverage/results-template.html:356 > + <script id="json" type="application/json">%PeformanceTestsResultsJSON%</script> Style: This is the only place where you use double quotes intend of single quotes. I'd change it for consistency. > Tools/Scripts/generate-coverage-data:78 > +$templateHtml =~ s/%PeformanceTestsResultsJSON%/$jsonData/g; The /g shouldn't be necessary. There will probably only ever be one %PerformanceTestsResultsJSON%, right? Its like a 9MB string. > Tools/Scripts/generate-coverage-data:85 > +system "open $url"; I'd recommend quoting the path in case anyone has spaces in their path leading up to WebKitBuild. system "open '$url'"; Of course using perl exec APIs there is probably a safer way that handles this. Probably: system "open", $url;
Comment on attachment 205513 [details] Patch r+ with the comments from the earlier patch.
Created attachment 205515 [details] Patch
Can we see a sample output? (Attach it on the bugzilla)
Created attachment 205517 [details] Patch
Created attachment 205519 [details] This is an example report. The data for the example report is from a release build which has some quirks right now (so entire directories seem to be uncovered when they are not), and the links to the files won't work unless you have the same generated files that I do.
Comment on attachment 205517 [details] Patch Clearing flags on attachment: 205517 Committed r152012: <http://trac.webkit.org/changeset/152012>
All reviewed patches have been landed. Closing bug.