Move isPrimary computation from PhaseInput into TransformNode.

Move isPrimary computation from PhaseInput into TransformNode.

In addition to cleaning up the code, this makes transforms more parallel and
paves the way for properly supporting declaring transformers.

R=rnystrom@google.com
BUG=17401
Committed: https://code.google.com/p/dart/source/detail?r=33626

[nweiz, 2014-03-12 02:36:45]

It looks like this also fixes
https://code.google.com/p/dart/issues/detail?id=17401.

[Bob Nystrom, 2014-03-12 17:42:57]

https://codereview.chromium.org/196273003/diff/1/pkg/barback/lib/src/phase_fo...
File pkg/barback/lib/src/phase_forwarder.dart (right):

https://codereview.chromium.org/196273003/diff/1/pkg/barback/lib/src/phase_fo...
pkg/barback/lib/src/phase_forwarder.dart:48: int get _realNumChannels =>
_numChannels + 1;
This is pretty gross. How about we encapsulate the concept of channels within
PhaseForwarder. I'm thinking:

1. Remove numChannels and replace it with updateTransformers(int
numTransformers, int numGroups)
2. Use the same arguments for the constructor.
3. Renamed _realNumChannels -> _numChannels.

This way, Phase doesn't have any idea of "channels": it just tells
PhaseForwarder what it knows and its PhaseForwarder's job to know how to keep
track of when all of the intermediates have arrived.

WDYT?

https://codereview.chromium.org/196273003/diff/1/pkg/barback/lib/src/phase_fo...
pkg/barback/lib/src/phase_forwarder.dart:66: final _onAssetController = new
StreamController<AssetNode>.broadcast(sync: true);
Long line.

https://codereview.chromium.org/196273003/diff/1/pkg/barback/lib/src/transfor...
File pkg/barback/lib/src/transform_node.dart (right):

https://codereview.chromium.org/196273003/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transform_node.dart:61: /// null if the asset is not being
passed through.
"null null" -> "null"

https://codereview.chromium.org/196273003/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transform_node.dart:173: }
It might be worth moving this conditional logic into the state itself:

_state = _state._dirty(primaryChanged: primaryChanged);

https://codereview.chromium.org/196273003/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transform_node.dart:210: }
This might be another chunk of conditional logic you could push into the state:

_state = _state.finishIsPrimary(this, isPrimary);

https://codereview.chromium.org/196273003/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transform_node.dart:254: }
Ditto.

https://codereview.chromium.org/196273003/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transform_node.dart:477: /// Whether [this] is
[IS_NOT_PRIMARY].
IS_NOT_PRIMARY -> NOT_PRIMARY.

[nweiz, 2014-03-12 21:44:46]

https://codereview.chromium.org/196273003/diff/1/pkg/barback/lib/src/phase_fo...
File pkg/barback/lib/src/phase_forwarder.dart (right):

https://codereview.chromium.org/196273003/diff/1/pkg/barback/lib/src/phase_fo...
pkg/barback/lib/src/phase_forwarder.dart:48: int get _realNumChannels =>
_numChannels + 1;
On 2014/03/12 17:42:58, Bob Nystrom wrote:
> This is pretty gross. How about we encapsulate the concept of channels within
> PhaseForwarder. I'm thinking:
> 
> 1. Remove numChannels and replace it with updateTransformers(int
> numTransformers, int numGroups)
> 2. Use the same arguments for the constructor.
> 3. Renamed _realNumChannels -> _numChannels.
> 
> This way, Phase doesn't have any idea of "channels": it just tells
> PhaseForwarder what it knows and its PhaseForwarder's job to know how to keep
> track of when all of the intermediates have arrived.
> 
> WDYT?

SGTM, done.

https://codereview.chromium.org/196273003/diff/1/pkg/barback/lib/src/phase_fo...
pkg/barback/lib/src/phase_forwarder.dart:66: final _onAssetController = new
StreamController<AssetNode>.broadcast(sync: true);
On 2014/03/12 17:42:58, Bob Nystrom wrote:
> Long line.

Done.

https://codereview.chromium.org/196273003/diff/1/pkg/barback/lib/src/transfor...
File pkg/barback/lib/src/transform_node.dart (right):

https://codereview.chromium.org/196273003/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transform_node.dart:61: /// null if the asset is not being
passed through.
On 2014/03/12 17:42:58, Bob Nystrom wrote:
> "null null" -> "null"

Done.

https://codereview.chromium.org/196273003/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transform_node.dart:173: }
On 2014/03/12 17:42:58, Bob Nystrom wrote:
> It might be worth moving this conditional logic into the state itself:
> 
> _state = _state._dirty(primaryChanged: primaryChanged);

I tried this out, but I ended up finding it more confusing than keeping all the
logic in TransformNode. The interaction between setting the state and doing a
bunch of other actions to the transform node got more twisted when I moved some
of them out into the state class.

https://codereview.chromium.org/196273003/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transform_node.dart:477: /// Whether [this] is
[IS_NOT_PRIMARY].
On 2014/03/12 17:42:58, Bob Nystrom wrote:
> IS_NOT_PRIMARY -> NOT_PRIMARY.

Done.

[Bob Nystrom, 2014-03-12 23:13:10]

lgtm

[nweiz, 2014-03-12 23:20:33]

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