Suppress get_drt/dartium download if an executable is provided via flag

tools/testing/dart/drt_updater.dart

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #library("drt_updater");
 
 #import("dart:io");
 #import("dart:builtin");
 
 class _DartiumUpdater {
@@ skipping 35 matching lines @@
 
   void _onUpdatedHandler(int exit_code) {
     print('$name updated ($exit_code)');
     for (var callback in onUpdated ) callback();
   }
 }
 
 _DartiumUpdater _dumpRenderTreeUpdater;
 _DartiumUpdater _dartiumUpdater;
 
-_DartiumUpdater runtimeUpdater(String runtime) {
-  if (runtime == 'drt') {
+_DartiumUpdater runtimeUpdater(Map configuration) {
+  String runtime = configuration['runtime'];
+  if (!configuration.containsKey(runtime) || configuration[runtime] != '') {

[Bill Hesse, 2012/04/30 13:46:10]

This seems totally wrong.  I think configuration['runtime'] always exists, and
it is never the empty string.  I don't see any place where it is changed. 
Shouldn't you be checking
configuration['drt'] instead?

I would think the condition would be just 
if (runtime == 'drt' && configuration['drt'] == null) {
(return updater) }
All valid keys should always be in the configuration, with their default values
- every key has a default.

[vsm, 2012/04/30 14:00:33]

I'm checking what you suggest.  It's runtime the variable ('drt' or 'dartium'),
not 'runtime' the string.

Isn't the default to configuration['drt'] the empty string and not null?

On 2012/04/30 13:46:10, Bill Hesse wrote:

[Bill Hesse, 2012/05/01 08:03:33]

Oh, I see.  That is really tricky, and easy to overlook.

I would either put a comment in,  pointing out that it is not
configuration['runtime'], but configuration[runtime], or I would just add 
&& configuration['drt'] == ''
to the runtime == 'drt' clause, and similarly for the dartium case.  

Also, is the last 
else {
  return null;
}
in line 75 now dead code?


On 2012/04/30 14:00:33, vsm wrote:

+    // Either the runtime executable does not require updating or an
+    // explicit path is provided via --drt or --dartium.
+    return null;
+  } else if (runtime == 'drt') {
     if (_dumpRenderTreeUpdater === null) {
       _dumpRenderTreeUpdater = new _DartiumUpdater('DumpRenderTree',
                                                    'get_drt.py');
     }
     return _dumpRenderTreeUpdater;
   } else if (runtime == 'dartium') {
     if (_dartiumUpdater === null) {
       _dartiumUpdater = new _DartiumUpdater('Dartium Chrome', 'get_drt.py',
                                             '--dartium');
     }
     return _dartiumUpdater;
   } else {
     return null;
   }
 }