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

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

Note, the Dartium win bot was hanging (I believe timing out) at this
point running the webdriver test even though it was providing its own
binary.  I had commented out the webdriver test - this CL should
enable it to work again.

Committed: https://code.google.com/p/dart/source/detail?r=7194

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

https://chromiumcodereview.appspot.com/10264005/diff/1/tools/testing/dart/drt...
File tools/testing/dart/drt_updater.dart (right):

https://chromiumcodereview.appspot.com/10264005/diff/1/tools/testing/dart/drt...
tools/testing/dart/drt_updater.dart:58: if (!configuration.containsKey(runtime)
|| configuration[runtime] != '') {
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]

https://chromiumcodereview.appspot.com/10264005/diff/1/tools/testing/dart/drt...
File tools/testing/dart/drt_updater.dart (right):

https://chromiumcodereview.appspot.com/10264005/diff/1/tools/testing/dart/drt...
tools/testing/dart/drt_updater.dart:58: if (!configuration.containsKey(runtime)
|| configuration[runtime] != '') {
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:
> 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 16:15:06]

PTAL.  Cleaned up the code to reduce confusion.

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

LGTM - I forgot to send you my earlier comment, but you read my mind and did
exactly what I suggested.

https://chromiumcodereview.appspot.com/10264005/diff/1/tools/testing/dart/drt...
File tools/testing/dart/drt_updater.dart (right):

https://chromiumcodereview.appspot.com/10264005/diff/1/tools/testing/dart/drt...
tools/testing/dart/drt_updater.dart:58: if (!configuration.containsKey(runtime)
|| configuration[runtime] != '') {
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:
> 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:
> > 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.
>