delete debug only files from output in release mode

delete debug only files from output in release mode

Note: This adds a couple hundred milliseconds to the build for my sample application. However, that should remain fairly constant even if we add more files later since we already paid the price of spinning up the transformers.

BUG= http://dartbug.com/19521
R=sigmund@google.com

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

[jakemac, 2014-09-02 21:25:52]

jakemac@google.com changed reviewers:
+ sigmund@google.com

[Siggi Cherem (dart-lang), 2014-09-03 16:56:02]

https://codereview.chromium.org/531943002/diff/60001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/build_filter.dart (right):

https://codereview.chromium.org/531943002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/build_filter.dart:25: return new Future.value(
given our current dependency on barback 0.14.2 we can now remove the future here
and return a boolean directly, yay! :)

https://codereview.chromium.org/531943002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/build_filter.dart:30: (isBuildArtifact ||
id.path.startsWith('web/')) &&
I think we no longer generate build artifacts except on the entrypoint directory

https://codereview.chromium.org/531943002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/build_filter.dart:30: (isBuildArtifact ||
id.path.startsWith('web/')) &&
maybe we should check `!id.path.startsWith('lib/')` that way we include
'example' and other entry point directories

https://codereview.chromium.org/531943002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/build_filter.dart:52: const List<String>
_BUILD_ARTIFACTS = const ['.scriptUrls', '._data'];
I think scriptUrls no longer exist, so we can change it back to just ._data

https://codereview.chromium.org/531943002/diff/60001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/delete_file.dart (right):

https://codereview.chromium.org/531943002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/delete_file.dart:19: return new
Future.value(settings.mode == BarbackMode.RELEASE);
same here: 
  bool isPrimary(_) => settings.mode == BarbackMode.RELEASE;

https://codereview.chromium.org/531943002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/delete_file.dart:24: return new Future(() {});
I wonder if this is needed by barback, or can we just return nothing.

https://codereview.chromium.org/531943002/diff/60001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/remove_sourcemap_comment.dart (right):

https://codereview.chromium.org/531943002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/remove_sourcemap_comment.dart:20: return new
Future.value(settings.mode == BarbackMode.RELEASE);
same here

https://codereview.chromium.org/531943002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/remove_sourcemap_comment.dart:24:
transform.consumePrimary();
I think this works also if you don't consume it, instead just do the addOutput
only when you'll modify it.

[jakemac, 2014-09-03 17:45:43]

https://codereview.chromium.org/531943002/diff/60001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/build_filter.dart (right):

https://codereview.chromium.org/531943002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/build_filter.dart:25: return new Future.value(
On 2014/09/03 16:56:01, Siggi Cherem (dart-lang) wrote:
> given our current dependency on barback 0.14.2 we can now remove the future
here
> and return a boolean directly, yay! :)

Done.

https://codereview.chromium.org/531943002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/build_filter.dart:30: (isBuildArtifact ||
id.path.startsWith('web/')) &&
Good call, done

On 2014/09/03 16:56:01, Siggi Cherem (dart-lang) wrote:
> maybe we should check `!id.path.startsWith('lib/')` that way we include
> 'example' and other entry point directories

https://codereview.chromium.org/531943002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/build_filter.dart:30: (isBuildArtifact ||
id.path.startsWith('web/')) &&
On 2014/09/03 16:56:01, Siggi Cherem (dart-lang) wrote:
> I think we no longer generate build artifacts except on the entrypoint
directory

Done.

https://codereview.chromium.org/531943002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/build_filter.dart:52: const List<String>
_BUILD_ARTIFACTS = const ['.scriptUrls', '._data'];
On 2014/09/03 16:56:01, Siggi Cherem (dart-lang) wrote:
> I think scriptUrls no longer exist, so we can change it back to just ._data

Done.

https://codereview.chromium.org/531943002/diff/60001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/delete_file.dart (right):

https://codereview.chromium.org/531943002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/delete_file.dart:19: return new
Future.value(settings.mode == BarbackMode.RELEASE);
On 2014/09/03 16:56:02, Siggi Cherem (dart-lang) wrote:
> same here: 
>   bool isPrimary(_) => settings.mode == BarbackMode.RELEASE;

Done.

https://codereview.chromium.org/531943002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/delete_file.dart:24: return new Future(() {});
On 2014/09/03 16:56:02, Siggi Cherem (dart-lang) wrote:
> I wonder if this is needed by barback, or can we just return nothing.

Done.

https://codereview.chromium.org/531943002/diff/60001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/remove_sourcemap_comment.dart (right):

https://codereview.chromium.org/531943002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/remove_sourcemap_comment.dart:20: return new
Future.value(settings.mode == BarbackMode.RELEASE);
On 2014/09/03 16:56:02, Siggi Cherem (dart-lang) wrote:
> same here 

Done.

https://codereview.chromium.org/531943002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/remove_sourcemap_comment.dart:24:
transform.consumePrimary();
On 2014/09/03 16:56:02, Siggi Cherem (dart-lang) wrote:
> I think this works also if you don't consume it, instead just do the addOutput
> only when you'll modify it.

Done.

[Siggi Cherem (dart-lang), 2014-09-03 19:20:30]

lgtm

https://codereview.chromium.org/531943002/diff/80001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/build_filter.dart (right):

https://codereview.chromium.org/531943002/diff/80001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/build_filter.dart:22: // nothing is filtered in debug
mode
minor nit: maybe move this up and put the first expression be together with the
return?

https://codereview.chromium.org/531943002/diff/80001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/remove_sourcemap_comment.dart (right):

https://codereview.chromium.org/531943002/diff/80001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/remove_sourcemap_comment.dart:26:
transform.addOutput(transform.primaryInput);
oh, I meant that by removing the `consume` we also don't need the body of this
branch. This can just be:
if (file.contains(_...)) {
  transform.addOutput(... replaceAll...);
}

[jakemac, 2014-09-03 19:52:48]

https://codereview.chromium.org/531943002/diff/80001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/build_filter.dart (right):

https://codereview.chromium.org/531943002/diff/80001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/build_filter.dart:22: // nothing is filtered in debug
mode
On 2014/09/03 19:20:30, Siggi Cherem (dart-lang) wrote:
> minor nit: maybe move this up and put the first expression be together with
the
> return?

Done.

https://codereview.chromium.org/531943002/diff/80001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/remove_sourcemap_comment.dart (right):

https://codereview.chromium.org/531943002/diff/80001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/remove_sourcemap_comment.dart:26:
transform.addOutput(transform.primaryInput);
On 2014/09/03 19:20:30, Siggi Cherem (dart-lang) wrote:
> oh, I meant that by removing the `consume` we also don't need the body of this
> branch. This can just be:
> if (file.contains(_...)) {
>   transform.addOutput(... replaceAll...);
> }

Done.

[jakemac, 2014-09-03 20:01:54]

Committed patchset #6 (id:100001) manually as 39828 (presubmit successful).