Don't make lazy transformers eager when an asset is requested.

Don't make lazy transformers eager when an asset is requested.

Instead, they'll only be active long enough to finish applying, then become lazy
again.

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

[Bob Nystrom, 2014-04-10 22:58:55]

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

https://codereview.chromium.org/233843002/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transform_node.dart:55: /// primary inputs that are
themselves lazy.
I think we should disambiguate these terms. How about "lazy" means "a
transformer that doesn't run until needed" and "deferred" is a transformation
that is either lazy or declared and downstream from something lazy?

https://codereview.chromium.org/233843002/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transform_node.dart:66: bool _awaitingForce;
This name is a bit confusing. How about "_deferred"?

https://codereview.chromium.org/233843002/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transform_node.dart:205: if (_state == _State.DECLARING ||
_awaitingForce) return;
Add some documentation here about what the DECLARING check is for.

https://codereview.chromium.org/233843002/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transform_node.dart:211: _state = _State.DECLARED;
This could use some docs here. It isn't obvious why it transitions from APPLIED
to DECLARED. (I assume the answer is because the declaration is only based on
the primary ID now, so won't change and doesn't need to be re-run.)

https://codereview.chromium.org/233843002/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transform_node.dart:351: if (state.isDirty) node.force();
So if an input to this transform changes to a dirty state, this transform forces
it to apply?

Document what's going on here.

https://codereview.chromium.org/233843002/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transform_node.dart:519: static final DECLARED = const
_State._("declared");
"DEFERRED"?

[nweiz, 2014-04-11 00:47:59]

While working on your comments, I noticed a couple bugs lurking, so I fixed them
and added tests as well.

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

https://codereview.chromium.org/233843002/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transform_node.dart:55: /// primary inputs that are
themselves lazy.
On 2014/04/10 22:58:56, Bob Nystrom wrote:
> I think we should disambiguate these terms. How about "lazy" means "a
> transformer that doesn't run until needed" and "deferred" is a transformation
> that is either lazy or declared and downstream from something lazy?

Done.

https://codereview.chromium.org/233843002/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transform_node.dart:66: bool _awaitingForce;
On 2014/04/10 22:58:56, Bob Nystrom wrote:
> This name is a bit confusing. How about "_deferred"?

That doesn't match your previous suggestion. You suggested that "deferred" means
"lazy or downstream from something lazy", but this variable isn't that; it
indicates whether the transformer has emitted lazy outputs that have yet to be
materialized.

https://codereview.chromium.org/233843002/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transform_node.dart:205: if (_state == _State.DECLARING ||
_awaitingForce) return;
On 2014/04/10 22:58:56, Bob Nystrom wrote:
> Add some documentation here about what the DECLARING check is for.

Done.

https://codereview.chromium.org/233843002/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transform_node.dart:211: _state = _State.DECLARED;
On 2014/04/10 22:58:56, Bob Nystrom wrote:
> This could use some docs here. It isn't obvious why it transitions from
APPLIED
> to DECLARED. (I assume the answer is because the declaration is only based on
> the primary ID now, so won't change and doesn't need to be re-run.)

Done.

https://codereview.chromium.org/233843002/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transform_node.dart:351: if (state.isDirty) node.force();
On 2014/04/10 22:58:56, Bob Nystrom wrote:
> So if an input to this transform changes to a dirty state, this transform
forces
> it to apply?
> 
> Document what's going on here.

Removed, as per discussion.

https://codereview.chromium.org/233843002/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transform_node.dart:519: static final DECLARED = const
_State._("declared");
On 2014/04/10 22:58:56, Bob Nystrom wrote:
> "DEFERRED"?

That would be confusing in light of your terminology suggestion above, since the
state of being lazy or transitively beneath a lazy transformer is static whereas
a transform can move into and out of this state.

[Bob Nystrom, 2014-04-14 19:20:25]

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

https://codereview.chromium.org/233843002/diff/1/pkg/barback/lib/src/transfor...
pkg/barback/lib/src/transform_node.dart:66: bool _awaitingForce;
On 2014/04/11 00:47:59, nweiz wrote:
> On 2014/04/10 22:58:56, Bob Nystrom wrote:
> > This name is a bit confusing. How about "_deferred"?
> 
> That doesn't match your previous suggestion. You suggested that "deferred"
means
> "lazy or downstream from something lazy", but this variable isn't that; it
> indicates whether the transformer has emitted lazy outputs that have yet to be
> materialized. 

Ah, good point.

https://codereview.chromium.org/233843002/diff/20001/pkg/barback/lib/src/tran...
File pkg/barback/lib/src/transform_node.dart (right):

https://codereview.chromium.org/233843002/diff/20001/pkg/barback/lib/src/tran...
pkg/barback/lib/src/transform_node.dart:139: primary.transform.deferred) {
Instead of checking for a null transform here, what do you think of adding
.deferred to AssetNode that encapsulates that check?

https://codereview.chromium.org/233843002/diff/20001/pkg/barback/lib/src/tran...
pkg/barback/lib/src/transform_node.dart:208: // If we're in the process of
running [isPrimary] or [declareOutputs], we're
"we're" -> "we"

https://codereview.chromium.org/233843002/diff/20001/pkg/barback/lib/src/tran...
pkg/barback/lib/src/transform_node.dart:213: // If we're waiting until [force]
is called to run [apply], we don't to run
"don't to" -> "don't want to".

https://codereview.chromium.org/233843002/diff/20001/pkg/barback/lib/src/tran...
pkg/barback/lib/src/transform_node.dart:235: if (!controller.node.state.isDirty)
controller.setDirty();
Can this check be moved into .setDirty()?

https://codereview.chromium.org/233843002/diff/20001/pkg/barback/lib/src/tran...
pkg/barback/lib/src/transform_node.dart:525: /// primary, [DECLARED] if the
transform is deferred, and [APPLYING] otherwise.
Long line.

[nweiz, 2014-04-14 21:55:07]

I realized there was another test here that had only been passing because
DeclaringRewriteTransformer was falsely declared as lazy. I disabled it and
filed an issue to track the feature it tested.

https://codereview.chromium.org/233843002/diff/20001/pkg/barback/lib/src/tran...
File pkg/barback/lib/src/transform_node.dart (right):

https://codereview.chromium.org/233843002/diff/20001/pkg/barback/lib/src/tran...
pkg/barback/lib/src/transform_node.dart:139: primary.transform.deferred) {
On 2014/04/14 19:20:25, Bob Nystrom wrote:
> Instead of checking for a null transform here, what do you think of adding
> .deferred to AssetNode that encapsulates that check?

Done.

https://codereview.chromium.org/233843002/diff/20001/pkg/barback/lib/src/tran...
pkg/barback/lib/src/transform_node.dart:208: // If we're in the process of
running [isPrimary] or [declareOutputs], we're
On 2014/04/14 19:20:25, Bob Nystrom wrote:
> "we're" -> "we"

Done.

https://codereview.chromium.org/233843002/diff/20001/pkg/barback/lib/src/tran...
pkg/barback/lib/src/transform_node.dart:213: // If we're waiting until [force]
is called to run [apply], we don't to run
On 2014/04/14 19:20:25, Bob Nystrom wrote:
> "don't to" -> "don't want to".

Done.

https://codereview.chromium.org/233843002/diff/20001/pkg/barback/lib/src/tran...
pkg/barback/lib/src/transform_node.dart:235: if (!controller.node.state.isDirty)
controller.setDirty();
On 2014/04/14 19:20:25, Bob Nystrom wrote:
> Can this check be moved into .setDirty()?

Done.

https://codereview.chromium.org/233843002/diff/20001/pkg/barback/lib/src/tran...
pkg/barback/lib/src/transform_node.dart:525: /// primary, [DECLARED] if the
transform is deferred, and [APPLYING] otherwise.
On 2014/04/14 19:20:25, Bob Nystrom wrote:
> Long line.

Done.

[Bob Nystrom, 2014-04-15 00:11:28]

lgtm

[nweiz, 2014-04-15 01:08:47]

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