Better handling of asset collisions in barback.

Better handling of asset collisions in barback.

R=rnystrom@google.com

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

[Bob Nystrom, 2013-08-09 22:44:42]

https://codereview.chromium.org/22685006/diff/1/pkg/barback/lib/src/phase.dart
File pkg/barback/lib/src/phase.dart (right):

https://codereview.chromium.org/22685006/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:75: /// possible for multiple transformers to
output an asset with the same id. In
"transformers" -> "transformers in this phase"

https://codereview.chromium.org/22685006/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:76: /// that case, the first output emitted is
passed forward. We keep track of
"first" -> "chronologically first"

https://codereview.chromium.org/22685006/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:335: collisions.sort((a, b) =>
a.toString().compareTo(b.toString()));
This presumes AssetNode.toString() does something well-defined, which is a bit
sketchy.

Add a TODO that it should implement Comparable explicitly.

https://codereview.chromium.org/22685006/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:339: if (_outputs[collision].length < 2) return;
"< 2" -> "<= 1"
"return" -> "continue"

https://codereview.chromium.org/22685006/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:360: _outputs.remove(output.id);
How about handling this case first and then exiting the function? So:

output.whenRemoved.then((_) {
  var assets = _outputs[output.id];
  if (assets.length == 1) {
    _outputs.remove(output.id);
   return;
  }

  // Now handle collision edge cases...
});

This is the most common case, so it would be nice to see it first.

https://codereview.chromium.org/22685006/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:360: _outputs.remove(output.id);
Also, do we need to tell the next phase the input was removed, or is that
handled elsewhere?

https://codereview.chromium.org/22685006/diff/1/pkg/barback/test/package_grap...
File pkg/barback/test/package_graph/errors_test.dart (right):

https://codereview.chromium.org/22685006/diff/1/pkg/barback/test/package_grap...
pkg/barback/test/package_graph/errors_test.dart:281: });
Great tests!

[nweiz, 2013-08-12 21:15:44]

https://codereview.chromium.org/22685006/diff/1/pkg/barback/lib/src/phase.dart
File pkg/barback/lib/src/phase.dart (right):

https://codereview.chromium.org/22685006/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:75: /// possible for multiple transformers to
output an asset with the same id. In
On 2013/08/09 22:44:43, Bob Nystrom wrote:
> "transformers" -> "transformers in this phase"

Done.

https://codereview.chromium.org/22685006/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:76: /// that case, the first output emitted is
passed forward. We keep track of
On 2013/08/09 22:44:43, Bob Nystrom wrote:
> "first" -> "chronologically first"

Done.

https://codereview.chromium.org/22685006/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:335: collisions.sort((a, b) =>
a.toString().compareTo(b.toString()));
On 2013/08/09 22:44:43, Bob Nystrom wrote:
> This presumes AssetNode.toString() does something well-defined, which is a bit
> sketchy.
> 
> Add a TODO that it should implement Comparable explicitly.

I just went ahead and implemented comparable.

https://codereview.chromium.org/22685006/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:339: if (_outputs[collision].length < 2) return;
On 2013/08/09 22:44:43, Bob Nystrom wrote:
> "< 2" -> "<= 1"
> "return" -> "continue"

Done.

https://codereview.chromium.org/22685006/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:360: _outputs.remove(output.id);
On 2013/08/09 22:44:43, Bob Nystrom wrote:
> How about handling this case first and then exiting the function? So:
> 
> output.whenRemoved.then((_) {
>   var assets = _outputs[output.id];
>   if (assets.length == 1) {
>     _outputs.remove(output.id);
>    return;
>   }
> 
>   // Now handle collision edge cases...
> });
> 
> This is the most common case, so it would be nice to see it first.

Done.

> Also, do we need to tell the next phase the input was removed, or is that
> handled elsewhere?

The next phase has subscribed to the input's [onStateChange] feed and will be
notified that way.

https://codereview.chromium.org/22685006/diff/1/pkg/barback/test/package_grap...
File pkg/barback/test/package_graph/errors_test.dart (right):

https://codereview.chromium.org/22685006/diff/1/pkg/barback/test/package_grap...
pkg/barback/test/package_graph/errors_test.dart:281: });
On 2013/08/09 22:44:43, Bob Nystrom wrote:
> Great tests!

Thanks!

[Bob Nystrom, 2013-08-12 22:19:40]

LGTM!

[nweiz, 2013-08-12 22:25:11]

Committed patchset #2 manually as r26042 (presubmit successful).