Support passing barback assets through phases in which they're unused.

Support passing barback assets through phases in which they're unused.

R=rnystrom@google.com

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

[Bob Nystrom, 2013-08-14 22:43:01]

A couple of suggestions but otherwise LGTM. Very nice. :)

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

https://codereview.chromium.org/23199004/diff/1/pkg/barback/lib/src/errors.da...
pkg/barback/lib/src/errors.dart:33: /// This could only contain a single
transform, which indicates that a
This took me a few reads to grok. How about "If this only contains a single
transform, that indicates..."

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

https://codereview.chromium.org/23199004/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:286: /// whether it's consumed by other
transforms.
"transforms" -> "transforms in this phase".

https://codereview.chromium.org/23199004/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:294: var controller =
_passThroughControllers[node.id];
How about just using putIfAbsent() here?

https://codereview.chromium.org/23199004/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:304: controller.setRemoved();
Nit, but I think it's a little cleaner to just do:

if (controller != null) controller.setRemoved();

https://codereview.chromium.org/23199004/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:331: return
!_outputs[output.id].contains(output);
Took me a while to parse this. Maybe just:

return !_outputs.containsKey(...) || ...

Or maybe go DeMorgan on it?

https://codereview.chromium.org/23199004/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:378: Set<AssetId> _passThroughAssets(Set<AssetId>
newPassThroughs) {
How about "_passAssetsThrough"? I read it as "pass-through assets" at first.

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

https://codereview.chromium.org/23199004/diff/1/pkg/barback/test/package_grap...
pkg/barback/test/package_graph/transform_test.dart:829: test("doesn't pass an
asset through a phase in which a transform consumes it",
Long line.

[nweiz, 2013-08-15 21:06:46]

Committed patchset #3 manually as r26220 (presubmit successful).

[nweiz, 2013-08-15 21:07:01]

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

https://codereview.chromium.org/23199004/diff/1/pkg/barback/lib/src/errors.da...
pkg/barback/lib/src/errors.dart:33: /// This could only contain a single
transform, which indicates that a
On 2013/08/14 22:43:01, Bob Nystrom wrote:
> This took me a few reads to grok. How about "If this only contains a single
> transform, that indicates..."

Done.

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

https://codereview.chromium.org/23199004/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:286: /// whether it's consumed by other
transforms.
On 2013/08/14 22:43:01, Bob Nystrom wrote:
> "transforms" -> "transforms in this phase".

Done.

https://codereview.chromium.org/23199004/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:294: var controller =
_passThroughControllers[node.id];
On 2013/08/14 22:43:01, Bob Nystrom wrote:
> How about just using putIfAbsent() here?

That doesn't work well when we need to do something different if it does exist
(set it available in this case).

https://codereview.chromium.org/23199004/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:304: controller.setRemoved();
On 2013/08/14 22:43:01, Bob Nystrom wrote:
> Nit, but I think it's a little cleaner to just do:
> 
> if (controller != null) controller.setRemoved();

Done.

https://codereview.chromium.org/23199004/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:331: return
!_outputs[output.id].contains(output);
On 2013/08/14 22:43:01, Bob Nystrom wrote:
> Took me a while to parse this. Maybe just:
> 
> return !_outputs.containsKey(...) || ...
> 
> Or maybe go DeMorgan on it?

Done.

https://codereview.chromium.org/23199004/diff/1/pkg/barback/lib/src/phase.dar...
pkg/barback/lib/src/phase.dart:378: Set<AssetId> _passThroughAssets(Set<AssetId>
newPassThroughs) {
On 2013/08/14 22:43:01, Bob Nystrom wrote:
> How about "_passAssetsThrough"? I read it as "pass-through assets" at first.

Done.

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

https://codereview.chromium.org/23199004/diff/1/pkg/barback/test/package_grap...
pkg/barback/test/package_graph/transform_test.dart:829: test("doesn't pass an
asset through a phase in which a transform consumes it",
On 2013/08/14 22:43:01, Bob Nystrom wrote:
> Long line.

Done.