Dartest CL - Please review

Code for Dartest which consists of two parts:
1. Test runner library located in client/testing/
2. Instrumentation code for adding coverage probes

Added some fixes to the AST Writer and changes to highlight instrumented nodes.
Committed: https://code.google.com/p/dart/source/detail?r=2549

[shauvik, 2011-12-13 21:46:31]

Hi,

This is the work behind my internship. I'll be sending out a demo video shortly
to dart-team.

Kindly review it and let me know if you have any feedback.

Thanks,
Shauvik

[Bob Nystrom, 2011-12-14 00:15:30]

Just reviewed the changes under client/testing/unittest.

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/shar...
File client/testing/unittest/shared.dart (right):

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/shar...
client/testing/unittest/shared.dart:14: /** Test Runner Function */
This comment doesn't really add any value. How about "Callback used to run
tests. Entrypoints can replace this with their own if they want."

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/shar...
client/testing/unittest/shared.dart:15: Function _testRunner;
Rename "_runTests".

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/shar...
client/testing/unittest/shared.dart:244: //Set _nextBatch() as default test
runner
Obvious comment is obvious. Remove it. :)

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/shar...
client/testing/unittest/shared.dart:338: Date startDate;
Rename "startTime".

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/shar...
client/testing/unittest/shared.dart:340: Duration timeDuration;
Rename "runningTime".

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/shar...
client/testing/unittest/shared.dart:344: set startingDate(Date startDate) =>
this.startDate = startDate;
Remove this.

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/shar...
client/testing/unittest/shared.dart:346: set runningTime(Duration timeDuration)
=> this.timeDuration = timeDuration;
And this.

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/unit...
File client/testing/unittest/unittest_dartest.dart (right):

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/unit...
client/testing/unittest/unittest_dartest.dart:6: * A simple unit test library
for running tests in a browser.
Update this comment to describe what this entrypoint is for.

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/unit...
client/testing/unittest/unittest_dartest.dart:14: /** Getter for getting tests
*/
Obvious. Remove or make it say something I don't already know. :)

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/unit...
client/testing/unittest/unittest_dartest.dart:17: int testsRun = 0, testsFailed
= 0, testsErrors = 0;
We don't usually declare multiple variables on one line. I'd split this up.

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/unit...
client/testing/unittest/unittest_dartest.dart:24: Function uiUpdator = null;
"updateUI".

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/unit...
client/testing/unittest/unittest_dartest.dart:25: set uiUpdateFunc(Function
updateFunc) => uiUpdator = updateFunc;
Remove, you can just do "updateUI = ..."

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/unit...
client/testing/unittest/unittest_dartest.dart:28: set
dartestLoggingFunc(Function logFunc) => dartestLogger = logFunc;
Remove.

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/unit...
client/testing/unittest/unittest_dartest.dart:50: runDARTests(){
Dart shouldn't be all caps. "runDartests" or "runDartTests".

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/unit...
client/testing/unittest/unittest_dartest.dart:51: if(previousAsyncTest){
Space between "if" and "(". Also between ")" and "{". Here and elsewhere.

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/unit...
client/testing/unittest/unittest_dartest.dart:52:
updateTestStats(_tests[_currentTest-1]);
Space around "-".

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/unit...
client/testing/unittest/unittest_dartest.dart:57: dartestLogger('Running
test:'+testCase.description);
Space around "+".

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/unit...
client/testing/unittest/unittest_dartest.dart:71: //Support for VM and command
line coming soon!
Make this a TODO comment:

// TODO(shauvik): Support for VM and command line coming soon!

That way it's easy to grep for it.

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/unit...
client/testing/unittest/unittest_dartest.dart:76: //Do nothing
Space after "//".

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/unit...
client/testing/unittest/unittest_dartest.dart:83: String getTestResultsCSV() {
"getTestResultsCsv".

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/unit...
client/testing/unittest/unittest_dartest.dart:84: StringBuffer out = new
StringBuffer();
Just use "final" instead of a type annotation here.

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/unit...
client/testing/unittest/unittest_dartest.dart:85: for(TestCase test in _tests) {
Space after "for".

http://codereview.chromium.org/8905021/diff/6001/client/tests/client/samples/...
File client/tests/client/samples/total/total_tests.dart (right):

http://codereview.chromium.org/8905021/diff/6001/client/tests/client/samples/...
client/tests/client/samples/total/total_tests.dart:16: <<<<<<< .mine
Merge conflict marker. Fix.

[pdr, 2011-12-14 21:02:36]

General comments (many are the same as Bob's).

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/css.dart
File client/testing/dartest/css.dart (right):

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/css.d...
client/testing/dartest/css.dart:3: // BSD-style license that can be found in the
LICENSE file.
Could you test this on FF and IE as well? Several of the defs here (and
elsewhere) are clearly WebKit-specific and it may be simple to bring these to
the other browsers.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
File client/testing/dartest/dartest.dart (right):

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:7: #import('dart:dom');
I would strongly recommend using dart:html. It's generally way easier and more
concise.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:13: class DARTest{
Under the names section of the style guide
(http://www.dartlang.org/articles/style-guide) it suggests capitalizing
abbreviations like words (i.e., DartTest).

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:14: Map<String, HTMLElement> inAppElements,
fullAppElements, appElements;
I don't think these are necessary (if they are, can you explain?)

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:34: void log(String message){
Log calls should generally be behind a flag at worst, or removed entirely.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:70: HTMLTableRowElement details =
domWin.document.getElementById('dt-detail-${t.id}');
Line > 100 chars

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:93: /** Get test results as table cells */
This can be private so you can rename it using the underscore notation:
_getTestDetails(TestCase t). (Here, and elsewhere)

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:94: String getTestDetails(TestCase t) {
Instead of building up this table by strings, I would suggest building up from
DOM nodes which has the added benefit that you don't have to worry about
escaping since you don't use innerHTML.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:94: String getTestDetails(TestCase t) {
Without escaping and innerHTML-based DOM manipulation, you can shorten this
function to something like:
String get _testDetails(TestCase t) => "${t.id} - ${t.result == null ? 'none' :
t.result}";

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:104: String getTestStats(TestCase t) {
See comments in getTestDetails

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:112: return "<td colspan='3'> $message took
${printDuration(t.timeDuration)}</td>";
Line > 100 cols

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:115: /** Update the UI after running test */
Nit: make sure to use a period in your comments :)

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:117: // Update table
unnecessary comment

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:137: var pPass = ((numTestsRun -
numTestsFailed - numTestsErrors) / tests.length) * 100;
Line > 100 cols (here and elsewhere).

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:145: elements['testsRunElem'].innerText =
'$numTestsRun';
There's no need to do this for your strings. Just do:
elements['testsRunElem'].innerText = numTestsRun;

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:377: void showHide(String toShow, String
toHide) {
I'd split this into show and hide.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:397: log('Window:'+win.toString());
I would remove these logging calls, or put them behind a flag.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:409: void populateResults() {
I think this can be:
tests.forEach(TestCase t) => updateDartTestUi(t);

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/resou...
File client/testing/dartest/resources/img2base64.py (right):

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/resou...
client/testing/dartest/resources/img2base64.py:1: #!/usr/bin/python
There's a small effort to rewrite these types of scripts in Dart. Not required
but it might be a fun exercise as a followup CL.

[Kelly Norton, 2011-12-15 18:35:31]

I stacked up quite a few comments in a couple of files. Some of the comments
apply to all files. Could you make a pass fixing the following global issues:

1 - Adjust spacing according to typical style. I think Bob Nystrom put together
a style guide and where that doesn't have proper info, you can usually lean on
the Java style guide.

2 - There are far too many properties and methods that are public. Can you make
a pass moving unintentional public methods to private, using the _ prefix.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/css.dart
File client/testing/dartest/css.dart (right):

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/css.d...
client/testing/dartest/css.dart:24: .dt-hide{
space before {

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/css.d...
client/testing/dartest/css.dart:28: .dt-show{
space before {

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/css.d...
client/testing/dartest/css.dart:32: .dt-tab ul{
space before {

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/css.d...
client/testing/dartest/css.dart:39: .dt-tab li{
space before {

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/css.d...
client/testing/dartest/css.dart:48: .dt-tab li:hover{
space before {

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/css.d...
client/testing/dartest/css.dart:52: .dt-tab-selected{
space before {

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/css.d...
client/testing/dartest/css.dart:56: .dt-results{
space before {

(lots more below)

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/css.d...
client/testing/dartest/css.dart:285: background:#FCFCFC;
space between property and value.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/css.d...
client/testing/dartest/css.dart:305: static String _getInAppStyleSheet(){
You can use => form here ... and this could just be a getter.

Also, you need a space before your opening { here. There are many instances of
that in this file.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
File client/testing/dartest/dartest.dart (right):

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:15: bool inAppMode;
I suspect you want these to be private _inAppMode?

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:16: DOMWindow win, appWindow;
private?

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:19: win = window;
It seems unnecessary to store a reference to window, do you do this for when the
runner is not running inApp?

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:26: DARTestCss.inject(win.document, true);
document should also be available as a top-level getter, so you don't have to
reference it via win.document.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:38: void addChildToElem(String key,
HTMLElement child) {
This seems like a sizeable public API. Are all of these methods supposed to be
public?

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:47: HTMLTableElement table =
win.document.createElement('table');
I'm really confused by all the references to different windows. I could see you
needing to keep a window reference for when the runner is not inApp. However, if
that's the case here are some other things I would expect:

I would expect there to be a single reference to window, probably called
runnerWindow.

I would also expect that runnWindow != window would remove the need to have an
inApp bool field and instead inApp would be a getter.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:107: message = t.message + '<br>';
This looks problematic. where does t.message get html-escaped?

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:110: message +=
'<pre>${t.stackTrace}</pre>';
Similar here, I don't see where t.stackTrace is escaped.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:116: void updateDARTestUI(TestCase test){
space before {

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:135: void updateStatusProgress(Map<String,
HTMLElement> elements){
space before {

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:138: elements['green'].setAttribute('style',
'width:$pPass%');
Why do you use a map to store references to things like the 'green' element?
Would it make more sense to just have these be fields in this class?

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:145: elements['testsRunElem'].innerText =
'$numTestsRun';
You want to use textContent instead of innerText as innerText is not standard
and unsupported on mozilla browsers. Plus, textContent is faster and more
predictable.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:445: String urlencode(String s) {
How is the different from the native encodeURI?

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:459: if(s.length<n) {
s.length < n

Lots of other space problems in this file.

[shauvik, 2011-12-16 07:19:27]

Hi All,

Thanks for all your comments and support! 
I have tried to address them all.

Please let me know if there are more things you'd like for me to fix. Today is
my last day of my Google internship. I'll try my best to incorporate all issues
before I leave.

Thanks,
Shauvik

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/css.dart
File client/testing/dartest/css.dart (right):

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/css.d...
client/testing/dartest/css.dart:3: // BSD-style license that can be found in the
LICENSE file.
I will test and fix on FF. For IE, I don't have access to a windows machine now.
Will test it from my home machine later.

On 2011/12/14 21:02:36, pdr wrote:
> Could you test this on FF and IE as well? Several of the defs here (and
> elsewhere) are clearly WebKit-specific and it may be simple to bring these to
> the other browsers.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/css.d...
client/testing/dartest/css.dart:24: .dt-hide{
On 2011/12/15 18:35:31, Kelly Norton wrote:
> space before {

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/css.d...
client/testing/dartest/css.dart:28: .dt-show{
On 2011/12/15 18:35:31, Kelly Norton wrote:
> space before {

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/css.d...
client/testing/dartest/css.dart:32: .dt-tab ul{
On 2011/12/15 18:35:31, Kelly Norton wrote:
> space before {

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/css.d...
client/testing/dartest/css.dart:39: .dt-tab li{
On 2011/12/15 18:35:31, Kelly Norton wrote:
> space before {

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/css.d...
client/testing/dartest/css.dart:48: .dt-tab li:hover{
On 2011/12/15 18:35:31, Kelly Norton wrote:
> space before {

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/css.d...
client/testing/dartest/css.dart:52: .dt-tab-selected{
On 2011/12/15 18:35:31, Kelly Norton wrote:
> space before {

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/css.d...
client/testing/dartest/css.dart:56: .dt-results{
On 2011/12/15 18:35:31, Kelly Norton wrote:
> space before {
> 
> (lots more below)

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/css.d...
client/testing/dartest/css.dart:285: background:#FCFCFC;
On 2011/12/15 18:35:31, Kelly Norton wrote:
> space between property and value.

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/css.d...
client/testing/dartest/css.dart:305: static String _getInAppStyleSheet(){
Removed this function and accessed directly from the private variables.

On 2011/12/15 18:35:31, Kelly Norton wrote:
> You can use => form here ... and this could just be a getter.
> 
> Also, you need a space before your opening { here. There are many instances of
> that in this file.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
File client/testing/dartest/dartest.dart (right):

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:7: #import('dart:dom');
When I started, you couldn't do many things using dart:html. Maybe they fixed it
now. I'll migrate to dart:html after I fix all of the current issues. 

On 2011/12/14 21:02:36, pdr wrote:
> I would strongly recommend using dart:html. It's generally way easier and more
> concise.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:13: class DARTest{
Kelly and I decided to have the same name across all places. The name of the
tool is DARTest - we reuse the T; so I think it isn't suitable for camel casing.

On 2011/12/14 21:02:36, pdr wrote:
> Under the names section of the style guide
> (http://www.dartlang.org/articles/style-guide) it suggests capitalizing
> abbreviations like words (i.e., DartTest).

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:14: Map<String, HTMLElement> inAppElements,
fullAppElements, appElements;
I spoke to Kelly and Joel and its difficult and sometimes not possible (buggy)
to move DOM elements across windows.
https://developer.mozilla.org/en/DOM/document.importNode

inAppElements is a map of elements in the same window - which is never
destroyed; only hidden and shown.

fullAppElements is a map of elements always created for a new popup window.

appElements always points to one of the above.

On 2011/12/14 21:02:36, pdr wrote:
> I don't think these are necessary (if they are, can you explain?)

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:15: bool inAppMode;
On 2011/12/15 18:35:31, Kelly Norton wrote:
> I suspect you want these to be private _inAppMode?

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:16: DOMWindow win, appWindow;
On 2011/12/15 18:35:31, Kelly Norton wrote:
> private?

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:19: win = window;
On 2011/12/15 18:35:31, Kelly Norton wrote:
> It seems unnecessary to store a reference to window, do you do this for when
the
> runner is not running inApp?

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:26: DARTestCss.inject(win.document, true);
On 2011/12/15 18:35:31, Kelly Norton wrote:
> document should also be available as a top-level getter, so you don't have to
> reference it via win.document.

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:34: void log(String message){
Whole of dartest is behind a compiler flag. In discussion with Kelly, we
discussed that the logging calls are helpful for the developer and that we
wanted to have them.

I have tried to minimize the things that we log to not overwhelm the developer
with logging messages.

On 2011/12/14 21:02:36, pdr wrote:
> Log calls should generally be behind a flag at worst, or removed entirely.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:38: void addChildToElem(String key,
HTMLElement child) {
Made it private.

On 2011/12/15 18:35:31, Kelly Norton wrote:
> This seems like a sizeable public API. Are all of these methods supposed to be
> public?

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:47: HTMLTableElement table =
win.document.createElement('table');
On 2011/12/15 18:35:31, Kelly Norton wrote:
> I'm really confused by all the references to different windows. I could see
you
> needing to keep a window reference for when the runner is not inApp. However,
if
> that's the case here are some other things I would expect:
> 
> I would expect there to be a single reference to window, probably called
> runnerWindow.
> 
> I would also expect that runnWindow != window would remove the need to have an
> inApp bool field and instead inApp would be a getter.

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:70: HTMLTableRowElement details =
domWin.document.getElementById('dt-detail-${t.id}');
On 2011/12/14 21:02:36, pdr wrote:
> Line > 100 chars

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:93: /** Get test results as table cells */
On 2011/12/14 21:02:36, pdr wrote:
> This can be private so you can rename it using the underscore notation:
> _getTestDetails(TestCase t). (Here, and elsewhere)

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:94: String getTestDetails(TestCase t) {
On 2011/12/14 21:02:36, pdr wrote:
> Instead of building up this table by strings, I would suggest building up from
> DOM nodes which has the added benefit that you don't have to worry about
> escaping since you don't use innerHTML.

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:94: String getTestDetails(TestCase t) {
Actually I need that value in the class too. So, can't use this and have changed
it accordingly.

On 2011/12/14 21:02:36, pdr wrote:
> Without escaping and innerHTML-based DOM manipulation, you can shorten this
> function to something like:
> String get _testDetails(TestCase t) => "${t.id} - ${t.result == null ? 'none'
:
> t.result}";

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:104: String getTestStats(TestCase t) {
On 2011/12/14 21:02:36, pdr wrote:
> See comments in getTestDetails

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:107: message = t.message + '<br>';
On 2011/12/15 18:35:31, Kelly Norton wrote:
> This looks problematic. where does t.message get html-escaped?

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:110: message +=
'<pre>${t.stackTrace}</pre>';
On 2011/12/15 18:35:31, Kelly Norton wrote:
> Similar here, I don't see where t.stackTrace is escaped.

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:112: return "<td colspan='3'> $message took
${printDuration(t.timeDuration)}</td>";
On 2011/12/14 21:02:36, pdr wrote:
> Line > 100 cols

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:115: /** Update the UI after running test */
On 2011/12/14 21:02:36, pdr wrote:
> Nit: make sure to use a period in your comments :)

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:116: void updateDARTestUI(TestCase test){
On 2011/12/15 18:35:31, Kelly Norton wrote:
> space before {

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:117: // Update table
On 2011/12/14 21:02:36, pdr wrote:
> unnecessary comment

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:135: void updateStatusProgress(Map<String,
HTMLElement> elements){
On 2011/12/15 18:35:31, Kelly Norton wrote:
> space before {

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:137: var pPass = ((numTestsRun -
numTestsFailed - numTestsErrors) / tests.length) * 100;
On 2011/12/14 21:02:36, pdr wrote:
> Line > 100 cols (here and elsewhere).

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:138: elements['green'].setAttribute('style',
'width:$pPass%');
Actually everything is stored in maps. We need to update elements in the
corresponding map - inAppElements or fullAppElements. 

On 2011/12/15 18:35:31, Kelly Norton wrote:
> Why do you use a map to store references to things like the 'green' element?
> Would it make more sense to just have these be fields in this class?

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:145: elements['testsRunElem'].innerText =
'$numTestsRun';
Yes, I did that to get rid of int-to-string warnings. Now changed to:

elements['testsRunElem'].innerText = numTestsRun.toString();

On 2011/12/14 21:02:36, pdr wrote:
> There's no need to do this for your strings. Just do:
> elements['testsRunElem'].innerText = numTestsRun;

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:145: elements['testsRunElem'].innerText =
'$numTestsRun';
Cool, Thanks for the tip. I didn't know innerText wasn't supported in Firefox.

On 2011/12/15 18:35:31, Kelly Norton wrote:
> You want to use textContent instead of innerText as innerText is not standard
> and unsupported on mozilla browsers. Plus, textContent is faster and more
> predictable.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:377: void showHide(String toShow, String
toHide) {
On 2011/12/14 21:02:36, pdr wrote:
> I'd split this into show and hide.

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:397: log('Window:'+win.toString());
On 2011/12/14 21:02:36, pdr wrote:
> I would remove these logging calls, or put them behind a flag.

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:409: void populateResults() {
Thanks. Its slightly different that what you proposed.

tests.forEach((TestCase t) => _updateDARTestUI(t));

On 2011/12/14 21:02:36, pdr wrote:
> I think this can be:
> tests.forEach(TestCase t) => updateDartTestUi(t);

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:445: String urlencode(String s) {
Actually I think encodeURI isn't supported in dart:dom or dart:html -- or
atleast I couldn't find it. So, I wrote this one by myself.

On 2011/12/15 18:35:31, Kelly Norton wrote:
> How is the different from the native encodeURI?

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:459: if(s.length<n) {
On 2011/12/15 18:35:31, Kelly Norton wrote:
> s.length < n
> 
> Lots of other space problems in this file.

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/resou...
File client/testing/dartest/resources/img2base64.py (right):

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/resou...
client/testing/dartest/resources/img2base64.py:1: #!/usr/bin/python
Okay - that would be the next exercise after I get this CL in! :)

On 2011/12/14 21:02:36, pdr wrote:
> There's a small effort to rewrite these types of scripts in Dart. Not required
> but it might be a fun exercise as a followup CL.

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/shar...
File client/testing/unittest/shared.dart (right):

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/shar...
client/testing/unittest/shared.dart:14: /** Test Runner Function */
On 2011/12/14 00:15:30, Bob Nystrom wrote:
> This comment doesn't really add any value. How about "Callback used to run
> tests. Entrypoints can replace this with their own if they want."

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/shar...
client/testing/unittest/shared.dart:15: Function _testRunner;
There is already a function called "_runTests". Can't have two members with the
same name. Leaving this as it is.

On 2011/12/14 00:15:30, Bob Nystrom wrote:
> Rename "_runTests".

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/shar...
client/testing/unittest/shared.dart:244: //Set _nextBatch() as default test
runner
On 2011/12/14 00:15:30, Bob Nystrom wrote:
> Obvious comment is obvious. Remove it. :)

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/shar...
client/testing/unittest/shared.dart:338: Date startDate;
On 2011/12/14 00:15:30, Bob Nystrom wrote:
> Rename "startTime".

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/shar...
client/testing/unittest/shared.dart:340: Duration timeDuration;
On 2011/12/14 00:15:30, Bob Nystrom wrote:
> Rename "runningTime".

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/shar...
client/testing/unittest/shared.dart:344: set startingDate(Date startDate) =>
this.startDate = startDate;
On 2011/12/14 00:15:30, Bob Nystrom wrote:
> Remove this.

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/shar...
client/testing/unittest/shared.dart:346: set runningTime(Duration timeDuration)
=> this.timeDuration = timeDuration;
On 2011/12/14 00:15:30, Bob Nystrom wrote:
> And this.

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/unit...
File client/testing/unittest/unittest_dartest.dart (right):

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/unit...
client/testing/unittest/unittest_dartest.dart:6: * A simple unit test library
for running tests in a browser.
On 2011/12/14 00:15:30, Bob Nystrom wrote:
> Update this comment to describe what this entrypoint is for.

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/unit...
client/testing/unittest/unittest_dartest.dart:14: /** Getter for getting tests
*/
Ok - added more detail :)

On 2011/12/14 00:15:30, Bob Nystrom wrote:
> Obvious. Remove or make it say something I don't already know. :)

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/unit...
client/testing/unittest/unittest_dartest.dart:17: int testsRun = 0, testsFailed
= 0, testsErrors = 0;
On 2011/12/14 00:15:30, Bob Nystrom wrote:
> We don't usually declare multiple variables on one line. I'd split this up.

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/unit...
client/testing/unittest/unittest_dartest.dart:24: Function uiUpdator = null;
On 2011/12/14 00:15:30, Bob Nystrom wrote:
> "updateUI".

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/unit...
client/testing/unittest/unittest_dartest.dart:25: set uiUpdateFunc(Function
updateFunc) => uiUpdator = updateFunc;
On 2011/12/14 00:15:30, Bob Nystrom wrote:
> Remove, you can just do "updateUI = ..."

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/unit...
client/testing/unittest/unittest_dartest.dart:28: set
dartestLoggingFunc(Function logFunc) => dartestLogger = logFunc;
On 2011/12/14 00:15:30, Bob Nystrom wrote:
> Remove.

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/unit...
client/testing/unittest/unittest_dartest.dart:50: runDARTests(){
On 2011/12/14 00:15:30, Bob Nystrom wrote:
> Dart shouldn't be all caps. "runDartests" or "runDartTests".

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/unit...
client/testing/unittest/unittest_dartest.dart:51: if(previousAsyncTest){
On 2011/12/14 00:15:30, Bob Nystrom wrote:
> Space between "if" and "(". Also between ")" and "{". Here and elsewhere.

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/unit...
client/testing/unittest/unittest_dartest.dart:52:
updateTestStats(_tests[_currentTest-1]);
On 2011/12/14 00:15:30, Bob Nystrom wrote:
> Space around "-".

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/unit...
client/testing/unittest/unittest_dartest.dart:57: dartestLogger('Running
test:'+testCase.description);
On 2011/12/14 00:15:30, Bob Nystrom wrote:
> Space around "+".

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/unit...
client/testing/unittest/unittest_dartest.dart:71: //Support for VM and command
line coming soon!
On 2011/12/14 00:15:30, Bob Nystrom wrote:
> Make this a TODO comment:
> 
> // TODO(shauvik): Support for VM and command line coming soon!
> 
> That way it's easy to grep for it.

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/unit...
client/testing/unittest/unittest_dartest.dart:76: //Do nothing
On 2011/12/14 00:15:30, Bob Nystrom wrote:
> Space after "//".

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/unit...
client/testing/unittest/unittest_dartest.dart:83: String getTestResultsCSV() {
On 2011/12/14 00:15:30, Bob Nystrom wrote:
> "getTestResultsCsv".

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/unit...
client/testing/unittest/unittest_dartest.dart:84: StringBuffer out = new
StringBuffer();
On 2011/12/14 00:15:30, Bob Nystrom wrote:
> Just use "final" instead of a type annotation here.

Done.

http://codereview.chromium.org/8905021/diff/6001/client/testing/unittest/unit...
client/testing/unittest/unittest_dartest.dart:85: for(TestCase test in _tests) {
On 2011/12/14 00:15:30, Bob Nystrom wrote:
> Space after "for".

Done.

http://codereview.chromium.org/8905021/diff/6001/client/tests/client/samples/...
File client/tests/client/samples/total/total_tests.dart (right):

http://codereview.chromium.org/8905021/diff/6001/client/tests/client/samples/...
client/tests/client/samples/total/total_tests.dart:16: <<<<<<< .mine
On 2011/12/14 00:15:30, Bob Nystrom wrote:
> Merge conflict marker. Fix.

Done.

[pdr, 2011-12-16 17:00:49]

http://codereview.chromium.org/8905021/diff/17001/client/testing/dartest/dart...
File client/testing/dartest/dartest.dart (right):

http://codereview.chromium.org/8905021/diff/17001/client/testing/dartest/dart...
client/testing/dartest/dartest.dart:100: 
Extra space

http://codereview.chromium.org/8905021/diff/17001/client/testing/dartest/dart...
client/testing/dartest/dartest.dart:211: headDiv.innerHTML = '<b>DARTest: In-App
View</b>';
I think you should use font-weight: bold in your CSS and headDiv.innerText here.
This way your presentation is separated. (This applies to the HTML in
_showTestControls(), below).

http://codereview.chromium.org/8905021/diff/17001/client/testing/dartest/dart...
client/testing/dartest/dartest.dart:395: covTable.appendChild(covTableBody);
indentation

http://codereview.chromium.org/8905021/diff/17001/client/testing/dartest/dart...
client/testing/dartest/dartest.dart:429: _runnerWindow = window.open('',
'dartest-window', 'width=600,height=750');
Optional nit: I like moving these kinds of static presentation settings
(width/height) into your CSS Dart file and then reference them here.

http://codereview.chromium.org/8905021/diff/17001/client/testing/dartest/dart...
client/testing/dartest/dartest.dart:473: 
Make these private and static

http://codereview.chromium.org/8905021/diff/17001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/CommandLineOptions.java (right):

http://codereview.chromium.org/8905021/diff/17001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/CommandLineOptions.java:81: usage = "Add
instrumentation probes for collecting coverage. Supported types include
function, statement and branch")
Note: tabs

http://codereview.chromium.org/8905021/diff/17001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/ast/CoverageInstrumenter.java
(right):

http://codereview.chromium.org/8905021/diff/17001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/CoverageInstrumenter.java:13: import
com.google.dart.compiler.CommandLineOptions.CompilerOptions;
Alphabetize

http://codereview.chromium.org/8905021/diff/17001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/CoverageInstrumenter.java:19: */
This file is formatted with 8-space tabs, can you reformat?

http://codereview.chromium.org/8905021/diff/17001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/CoverageInstrumenter.java:164:
protected static Map<String, Integer> numFunctionsMap = new HashMap<String,
Integer>(),
Line > 100 cols (here, and below).

http://codereview.chromium.org/8905021/diff/17001/compiler/lib/coverage.dart
File compiler/lib/coverage.dart (right):

http://codereview.chromium.org/8905021/diff/17001/compiler/lib/coverage.dart#...
compiler/lib/coverage.dart:22: totalBranches = new HashMap<String, int>();
Indentation

http://codereview.chromium.org/8905021/diff/17001/compiler/lib/coverage.dart#...
compiler/lib/coverage.dart:132: }
Indentation

http://codereview.chromium.org/8905021/diff/17001/compiler/lib/coverage.dart#...
compiler/lib/coverage.dart:186: coverageDetails.add('<tr> <td>$unit <td>
$covFunc/$totFunc <td> $covStmt/$totStmt <td> $covBr/$totBr\n');
Line > 100 cols

[shauvik, 2011-12-16 17:57:05]

Hi,

I have fixed the issues raised by Philip.
Please let me know if it lgty. :)

Cheers,
Shauvik

http://codereview.chromium.org/8905021/diff/17001/client/testing/dartest/dart...
File client/testing/dartest/dartest.dart (right):

http://codereview.chromium.org/8905021/diff/17001/client/testing/dartest/dart...
client/testing/dartest/dartest.dart:100: 
On 2011/12/16 17:00:50, pdr wrote:
> Extra space

Done.

http://codereview.chromium.org/8905021/diff/17001/client/testing/dartest/dart...
client/testing/dartest/dartest.dart:395: covTable.appendChild(covTableBody);
On 2011/12/16 17:00:50, pdr wrote:
> indentation

Done.

http://codereview.chromium.org/8905021/diff/17001/client/testing/dartest/dart...
client/testing/dartest/dartest.dart:429: _runnerWindow = window.open('',
'dartest-window', 'width=600,height=750');
On 2011/12/16 17:00:50, pdr wrote:
> Optional nit: I like moving these kinds of static presentation settings
> (width/height) into your CSS Dart file and then reference them here.

Done.

http://codereview.chromium.org/8905021/diff/17001/client/testing/dartest/dart...
client/testing/dartest/dartest.dart:473: 
On 2011/12/16 17:00:50, pdr wrote:
> Make these private and static

Done.

http://codereview.chromium.org/8905021/diff/17001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/CommandLineOptions.java (right):

http://codereview.chromium.org/8905021/diff/17001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/CommandLineOptions.java:81: usage = "Add
instrumentation probes for collecting coverage. Supported types include
function, statement and branch")
On 2011/12/16 17:00:50, pdr wrote:
> Note: tabs

Done.

http://codereview.chromium.org/8905021/diff/17001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/ast/CoverageInstrumenter.java
(right):

http://codereview.chromium.org/8905021/diff/17001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/CoverageInstrumenter.java:13: import
com.google.dart.compiler.CommandLineOptions.CompilerOptions;
On 2011/12/16 17:00:50, pdr wrote:
> Alphabetize

Done.

http://codereview.chromium.org/8905021/diff/17001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/CoverageInstrumenter.java:19: */
On 2011/12/16 17:00:50, pdr wrote:
> This file is formatted with 8-space tabs, can you reformat?

Done.

http://codereview.chromium.org/8905021/diff/17001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/CoverageInstrumenter.java:164:
protected static Map<String, Integer> numFunctionsMap = new HashMap<String,
Integer>(),
On 2011/12/16 17:00:50, pdr wrote:
> Line > 100 cols (here, and below).

Done.

http://codereview.chromium.org/8905021/diff/17001/compiler/lib/coverage.dart
File compiler/lib/coverage.dart (right):

http://codereview.chromium.org/8905021/diff/17001/compiler/lib/coverage.dart#...
compiler/lib/coverage.dart:22: totalBranches = new HashMap<String, int>();
On 2011/12/16 17:00:50, pdr wrote:
> Indentation

Done.

http://codereview.chromium.org/8905021/diff/17001/compiler/lib/coverage.dart#...
compiler/lib/coverage.dart:132: }
On 2011/12/16 17:00:50, pdr wrote:
> Indentation

Done.

http://codereview.chromium.org/8905021/diff/17001/compiler/lib/coverage.dart#...
compiler/lib/coverage.dart:186: coverageDetails.add('<tr> <td>$unit <td>
$covFunc/$totFunc <td> $covStmt/$totStmt <td> $covBr/$totBr\n');
On 2011/12/16 17:00:50, pdr wrote:
> Line > 100 cols

Done.

[Bob Nystrom, 2011-12-16 19:22:08]

The stuff under client/testing/unittest LGTM!

[Kelly Norton, 2011-12-16 19:48:12]

Still looking at the review, but I think this is one thing you want to take a
look at real quick.

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
File client/testing/dartest/dartest.dart (right):

http://codereview.chromium.org/8905021/diff/6001/client/testing/dartest/darte...
client/testing/dartest/dartest.dart:138: elements['green'].setAttribute('style',
'width:$pPass%');
I'm just saying that a map is definitely not the best thing to hold these. Now
you are essentially using a map for a standard structure. The classic static way
to do this would be:

interface Elements {
  Element get green();
  Element get red();
  Element get orange();
}

class InAppElements {
  Element red;
  Element green;
  Element orange;
  Element other;
}

class OutAppElements extends Elements {
  Element red;
  Element green;
  Element orange;
  Element otherother;
}

void updateTheView(Elements elements) {
  elements.green.textContent = 'boo';
}

You could also so it with class inheritance too. A simpler alternative is to
just pass in the references as parameters.

void updateTheView(Element green, Element red, Element orange) {
}

I think there are a lot of solutions here that would beat using a map as a
struct.

[Kelly Norton, 2011-12-16 20:01:17]

besides the map issue and a few little nits, this lgtm.

Can you bug one of the dartc guys about reviewing the compiler changes?

http://codereview.chromium.org/8905021/diff/23026/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/ast/viz/ConsoleWriter.java (right):

http://codereview.chromium.org/8905021/diff/23026/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/viz/ConsoleWriter.java:37:
out.flush();
tab.

http://codereview.chromium.org/8905021/diff/23026/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/ast/viz/DotWriter.java (right):

http://codereview.chromium.org/8905021/diff/23026/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/viz/DotWriter.java:75: String nodeId
= node.getObjectIdentifier();
tabs.

[zundel, 2011-12-16 20:49:05]

lgtm, there is nothing I feel strongly about here

http://codereview.chromium.org/8905021/diff/23026/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/CommandLineOptions.java (right):

http://codereview.chromium.org/8905021/diff/23026/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/CommandLineOptions.java:81: usage = "Add
instrumentation probes for collecting coverage. Supported types include
function, statement and branch")
line > 100 chars

http://codereview.chromium.org/8905021/diff/23026/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/DartCompiler.java (right):

http://codereview.chromium.org/8905021/diff/23026/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/DartCompiler.java:689:
coverageInstrumenter.process(libraries);
nit.  you have an internal loop that duplicates the nested for loops below.  You
could move process() into the for loop and have it accept a unit as input.

Or you could make the instrumenter a part of the Normalizer.

http://codereview.chromium.org/8905021/diff/23026/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/ast/CoverageInstrumenter.java
(right):

http://codereview.chromium.org/8905021/diff/23026/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/CoverageInstrumenter.java:87:
System.out.println("Instrumenting " + unit.getSourceName() + ", lib:"
this looks like debugging...  for a real dart program you going to get a lot of
output here.

http://codereview.chromium.org/8905021/diff/23026/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/CoverageInstrumenter.java:138: if
("all".equals(covType)) {
instead of inlined strings, I'd make constants out of these values.

http://codereview.chromium.org/8905021/diff/23026/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/CoverageInstrumenter.java:144:
instrumenters.add(new StatementInstrumenter());
is it going to cause problems if you add the same instrumenter more than once? 
Maybe instrumenters should be a Set?

http://codereview.chromium.org/8905021/diff/23026/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/CoverageInstrumenter.java:160:
private static class BaseInstrumenter extends DartModVisitor {
FYI, we've been trying to replace instances of DartModVisitor with
DartNodeTraverser.  You probably don't have time to fix it, add a TODO

http://codereview.chromium.org/8905021/diff/23026/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/CoverageInstrumenter.java:165: new
HashMap<String, Integer>(),
style nit: don't use the comma for followon declarations like this, use a
separate statement for each one.

http://codereview.chromium.org/8905021/diff/23026/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/CoverageInstrumenter.java:302: *
myFunction(){ coverFunction('unit.dart', 'myFunction'); stmts; ... }
You aren't covering closures here. Is that intentional?  I would think they
might be useful. You might create a name out of the unit name and line number
since naming closure expressions is optional.

http://codereview.chromium.org/8905021/diff/23026/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/CoverageInstrumenter.java:369: public
boolean visit(DartBlock x, DartContext ctx) {
it would be nice to add a big fat comment like you did for visitDartFunction
above about what gets added to the statement.

http://codereview.chromium.org/8905021/diff/23026/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/CoverageInstrumenter.java:416: public
void endVisit(DartIfStatement x, DartContext ctx) {
same comment as above - I'd appreciate seeing how the statmement is
instrumented.

http://codereview.chromium.org/8905021/diff/23026/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/CoverageInstrumenter.java:477: *  
case 'b': do1(); break;
I didn't think you needed a break stmt in dart.

http://codereview.chromium.org/8905021/diff/23026/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java (right):

http://codereview.chromium.org/8905021/diff/23026/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/viz/BaseASTWriter.java:89:
"observable", "layout.dart", "unittest", "dartest"};
use spaces instead of tabs

http://codereview.chromium.org/8905021/diff/23026/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/ast/viz/DotWriter.java (right):

http://codereview.chromium.org/8905021/diff/23026/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/viz/DotWriter.java:96: data =
data.replaceAll("\"", "'");
odd indentation

http://codereview.chromium.org/8905021/diff/23026/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/viz/DotWriter.java:98: data =
data.replaceAll("\r", "\\r");
this is a bit optimisitc.  If you already have a statement \\r or \\n I don't
think this is going to work the way you want it to.

you could use a regular expression something like: s/[^\]?\n/\\n/

http://codereview.chromium.org/8905021/diff/23026/compiler/lib/coverage.dart
File compiler/lib/coverage.dart (right):

http://codereview.chromium.org/8905021/diff/23026/compiler/lib/coverage.dart#...
compiler/lib/coverage.dart:5: //#library("coverage");
Why the comment? does this line need to be removed?