Move Barback to a more thoroughly push-based model.

Move Barback to a more thoroughly push-based model.

This had a couple smaller ramifications:

* Instead of onDirty streams, most nodes now emit onDone streams that indicate
  when they have no more processing to do.

* AssetCascade was made more similar to other nodes; the BuildResult aggregation
  is now done entirely in PackageGraph.

R=rnystrom@google.com
BUG=

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

[Bob Nystrom, 2014-03-05 22:13:25]

Looks nicer!

https://codereview.chromium.org/187263003/diff/1/pkg/barback/lib/src/asset_ca...
File pkg/barback/lib/src/asset_cascade.dart (right):

https://codereview.chromium.org/187263003/diff/1/pkg/barback/lib/src/asset_ca...
pkg/barback/lib/src/asset_cascade.dart:75: final _onDoneController = new
StreamController<bool>.broadcast(sync: true);
Ditch the <bool>.

https://codereview.chromium.org/187263003/diff/1/pkg/barback/lib/src/asset_ca...
pkg/barback/lib/src/asset_cascade.dart:134: var completers =
_pendingAssetRequests.putIfAbsent(id, () => new Queue());
Why do you need a queue here? Can you just reuse the previous completer for this
asset?

https://codereview.chromium.org/187263003/diff/1/pkg/barback/lib/src/asset_ca...
pkg/barback/lib/src/asset_cascade.dart:254: if (asset.state.isAvailable) {
This seems weird to me. Doesn't it make more sense to hoist this out of the
loop? It's the same asset for every request. Then you'd:

1. If the asset is available, complete all of the requests.
2. Otherwise, wait for the state to change and retry.

https://codereview.chromium.org/187263003/diff/1/pkg/barback/lib/src/group_ru...
File pkg/barback/lib/src/group_runner.dart (right):

https://codereview.chromium.org/187263003/diff/1/pkg/barback/lib/src/group_ru...
pkg/barback/lib/src/group_runner.dart:66: phase.onDone.listen((_) {
Do we need to listen to this on all phases, or just the last one?

https://codereview.chromium.org/187263003/diff/1/pkg/barback/lib/src/package_...
File pkg/barback/lib/src/package_graph.dart (right):

https://codereview.chromium.org/187263003/diff/1/pkg/barback/lib/src/package_...
pkg/barback/lib/src/package_graph.dart:223: /// produce only a single
[BuildResult].
"produces".

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

https://codereview.chromium.org/187263003/diff/1/pkg/barback/lib/src/phase.da...
pkg/barback/lib/src/phase.dart:159: if (!isDirty)
_onDoneController.add(isDirty);
.add(null);

https://codereview.chromium.org/187263003/diff/1/pkg/barback/lib/src/phase_in...
File pkg/barback/lib/src/phase_input.dart (right):

https://codereview.chromium.org/187263003/diff/1/pkg/barback/lib/src/phase_in...
pkg/barback/lib/src/phase_input.dart:86: bool _isAdjustingTransformers = false;
Instead of flags, maybe a state enum? Are all permutations of these flag values
valid?

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

https://codereview.chromium.org/187263003/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transform_node.dart:45: // TODO(nweiz): remove this and move
isPrimary computation into TransformNode.
"remove" -> "Remove".

https://codereview.chromium.org/187263003/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transform_node.dart:57: var _isApplying = false;
I think a state enum would be a better fit than a few flags.

[nweiz, 2014-03-06 00:29:08]

https://codereview.chromium.org/187263003/diff/1/pkg/barback/lib/src/asset_ca...
File pkg/barback/lib/src/asset_cascade.dart (right):

https://codereview.chromium.org/187263003/diff/1/pkg/barback/lib/src/asset_ca...
pkg/barback/lib/src/asset_cascade.dart:75: final _onDoneController = new
StreamController<bool>.broadcast(sync: true);
On 2014/03/05 22:13:25, Bob Nystrom wrote:
> Ditch the <bool>.

Done.

https://codereview.chromium.org/187263003/diff/1/pkg/barback/lib/src/asset_ca...
pkg/barback/lib/src/asset_cascade.dart:134: var completers =
_pendingAssetRequests.putIfAbsent(id, () => new Queue());
On 2014/03/05 22:13:25, Bob Nystrom wrote:
> Why do you need a queue here? Can you just reuse the previous completer for
this
> asset?

Done.

https://codereview.chromium.org/187263003/diff/1/pkg/barback/lib/src/asset_ca...
pkg/barback/lib/src/asset_cascade.dart:254: if (asset.state.isAvailable) {
On 2014/03/05 22:13:25, Bob Nystrom wrote:
> This seems weird to me. Doesn't it make more sense to hoist this out of the
> loop? It's the same asset for every request. Then you'd:
> 
> 1. If the asset is available, complete all of the requests.
> 2. Otherwise, wait for the state to change and retry.

N/A since I got rid of the queue.

https://codereview.chromium.org/187263003/diff/1/pkg/barback/lib/src/group_ru...
File pkg/barback/lib/src/group_runner.dart (right):

https://codereview.chromium.org/187263003/diff/1/pkg/barback/lib/src/group_ru...
pkg/barback/lib/src/group_runner.dart:66: phase.onDone.listen((_) {
On 2014/03/05 22:13:25, Bob Nystrom wrote:
> Do we need to listen to this on all phases, or just the last one?

It's possible for a middle phase to go dirty and then back to done without
affecting anything else. For example, suppose the first phase had a transform
that just consumed an input and did nothing else. If that input changes, the
phase will go dirty until it finishes re-running [apply]. If it continues to do
nothing but consume the input, it will go clean again without affecting any
future phases.

Also, it's a pain to track which phase is the last one all the time, so even if
this wasn't relevant for other phases it would probably still be cleaner this
way.

https://codereview.chromium.org/187263003/diff/1/pkg/barback/lib/src/package_...
File pkg/barback/lib/src/package_graph.dart (right):

https://codereview.chromium.org/187263003/diff/1/pkg/barback/lib/src/package_...
pkg/barback/lib/src/package_graph.dart:223: /// produce only a single
[BuildResult].
On 2014/03/05 22:13:25, Bob Nystrom wrote:
> "produces".

Done.

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

https://codereview.chromium.org/187263003/diff/1/pkg/barback/lib/src/phase.da...
pkg/barback/lib/src/phase.dart:159: if (!isDirty)
_onDoneController.add(isDirty);
On 2014/03/05 22:13:25, Bob Nystrom wrote:
> .add(null);

Done.

https://codereview.chromium.org/187263003/diff/1/pkg/barback/lib/src/phase_in...
File pkg/barback/lib/src/phase_input.dart (right):

https://codereview.chromium.org/187263003/diff/1/pkg/barback/lib/src/phase_in...
pkg/barback/lib/src/phase_input.dart:86: bool _isAdjustingTransformers = false;
On 2014/03/05 22:13:25, Bob Nystrom wrote:
> Instead of flags, maybe a state enum? Are all permutations of these flag
values
> valid?

I considered a state enum, but [_hasBecomeDirty] and [_isAdjustingTransformers]
get adjusted separately, and it felt more obscure to write things like

    if (state == ADJUSTING_TRANSFORMERS) {
      state = DIRTY_AND_ADJUSTING_TRANFORMERS;
    }

instead of just treating them separately. I think they have clear enough
individual semantics that it's nicer this way.

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

https://codereview.chromium.org/187263003/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transform_node.dart:45: // TODO(nweiz): remove this and move
isPrimary computation into TransformNode.
On 2014/03/05 22:13:25, Bob Nystrom wrote:
> "remove" -> "Remove".

Done.

https://codereview.chromium.org/187263003/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transform_node.dart:57: var _isApplying = false;
On 2014/03/05 22:13:25, Bob Nystrom wrote:
> I think a state enum would be a better fit than a few flags.

See previous comment.

[Bob Nystrom, 2014-03-06 17:21:01]

LGTM!

https://codereview.chromium.org/187263003/diff/1/pkg/barback/lib/src/phase_in...
File pkg/barback/lib/src/phase_input.dart (right):

https://codereview.chromium.org/187263003/diff/1/pkg/barback/lib/src/phase_in...
pkg/barback/lib/src/phase_input.dart:86: bool _isAdjustingTransformers = false;
On 2014/03/06 00:29:08, nweiz wrote:
> On 2014/03/05 22:13:25, Bob Nystrom wrote:
> > Instead of flags, maybe a state enum? Are all permutations of these flag
> values
> > valid?
> 
> I considered a state enum, but [_hasBecomeDirty] and
[_isAdjustingTransformers]
> get adjusted separately, and it felt more obscure to write things like
> 
>     if (state == ADJUSTING_TRANSFORMERS) {
>       state = DIRTY_AND_ADJUSTING_TRANFORMERS;
>     }
> 
> instead of just treating them separately. I think they have clear enough
> individual semantics that it's nicer this way.

SGTM, though if you later find yourself adding another bool, it will probably be
a good point to switch over to an explicit state machine at that point.

[nweiz, 2014-03-06 19:32:17]

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