Polymer transformer logs now show on the frontend for pub serve.

Polymer transformers write to a *._buildLogs file and inject an element in that dom to show these elements when not in release mode (pub serve).

R=sigmund@google.com

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

[Siggi Cherem (dart-lang), 2014-08-01 21:31:53]

looking nice!

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/add_log_injector.dart (right):

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/add_log_injector.dart:22: 
nit: remove empty line

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/add_log_injector.dart:24: Future<bool>
isPrimary(idOrAsset) {
let's remove the type (since we return bool or Future<bool> below), or make it
bool and return synchronously in line 27 too?

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/add_log_injector.dart:34:
src="packages/polymer/src/build/log_injector.dart"></script>
this line is not necessary since this file is imported explicitly in the
_bootstrap.dart file that you generate in script_compactor.

I'm not sure, but this could bring us some trouble today because we have the
rule that there should be only 1 script tag per page. If you have an entrypoint
that contains a script tag, this might fail or produce an extra warning later
on?

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/add_log_injector.dart:36:
href="packages/polymer/src/build/log_injector.css">
since this is the only thing left to add on this phase, I'm thinking it might be
fine to add this also in script compactor and get rid of this extra phase
entirely.

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/add_log_injector.dart:37:
${document.body.innerHtml}''';
let's use append, that way we don't serialize and deserialize the html twice.

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/import_inliner.dart (right):

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/import_inliner.dart:402: uriToAssetId(sourceId, uri,
logger, span, errorOnAbsolute: false);
minor nit: let's keep the original wrapping

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/linter.dart (right):

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/linter.dart:26: 
nit: remove

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/linter.dart:474: const ['auto-binding-dart',
'polymer-element'];
not sure what the diff means here, is your editor by any chance removing the
trailing \n?

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/log_combiner.dart (right):

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/log_combiner.dart:15: class LogCombiner extends
Transformer with PolymerTransformer {
(a) nit: let's add a Dart doc here too. Might be the same that you have in the
library.

(b) maybe rename to BuildLogCombiner? or MessagesCombiner, or MessagesMerger?

The main thing is that 'log' has the same name for runtime and build-time logs.
I commonly think of 'log' of the runtime logs, and not the build-time warning
and error messages. So it would be nice to make the distinction.

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/log_combiner.dart:16: 
nit: remove empty line

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/log_combiner.dart:22: Future<bool>
isPrimary(idOrAsset) {
same here: change to 'bool', and remove new Future.value() below?

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/log_combiner.dart:31: 
nit: remove empty line

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/log_injector.dart (right):

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/log_injector.dart:1: /// This library provides a
single function called injectLogs which when called
+ copyright notice above this

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/log_injector.dart:11: injectLogs() {
Consider adding a note in a comment here to indicate that we are not using
polymer here to make sure this page works even if the app fails for some other
reason. I hope in the future, when we can have multiple Dart scripts on a page
that are independent of each other, we might be able to do this with Polymer
instead.

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/log_injector.dart:19: });
maybe return here if there are no messages?

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/log_injector.dart:22: var wrapperDiv = new
DivElement()..classes.add('build-logs');
or wrapperDiv = new Element.html('<div class="build-logs"></div>');

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/log_injector.dart:34: ..appendHtml('$level <span
class="num">(${logs.length})</span>');
how about:
menuItem = new Element.html(
    '<div class="$levelClassName">'
    '$level <span class="num">(...)</span>'
    '</div>');

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/log_injector.dart:42:
wrapperDiv.querySelectorAll('.menu > div, .content > div')
alternatively:
 - save a list of items/contents? so we don't do a selector all every time?
 - save the lastActiveItem and remove the 'active' class directly just on those
two nodes?

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/log_injector.dart:51: logs.forEach((log) {
for long blocks, we tend to use a normal loop instead:

for (var log in logs) {
  ...
}

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/log_injector.dart:72: var logElement = new
DivElement()
How about at the beginning do:
logHtml.write('<div class="log">);

and here do
logHtml.write('</div>);
contentItemp.append(new Element.html(logHtml.toString());

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/script_compactor.dart (right):

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/script_compactor.dart:437: code.writeln(
guard this also with !options.releaseMode?

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/script_compactor.dart:486: '<script
type="application/dart" src="$srcUrl"></script>'));
if we get rid of the 'add' phase, here we could add the style to document.head
in a similar way:

document.head.nodes.add(parseFragment(
  '<link ...>'));

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/wrapped_logger.dart (right):

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/wrapped_logger.dart:52: // figures out the next # to
use.
include a TODO that this is a workaround because of a bug in aggregate
transformers?  maybe include a link to the actual bug?

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/wrapped_logger.dart:58: return
getNextLogAssetPath(++nextNumber);
interesting ... another idea, but I'm not convinced if what I'm suggesting is
any better, so totally up to you.

we could have an enum of number IDs we expect to use (one per phase). Then the
name would be fixed:

  import_inliner  always generates file._buildLog.3

so you wouldn't need to find what is the next available log. 

Below, combine would have to check every fixed Id, rather than iterating until a
number doesn't exist.

[Siggi Cherem (dart-lang), 2014-08-01 21:48:30]

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/linter.dart (right):

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/linter.dart:474: const ['auto-binding-dart',
'polymer-element'];
On 2014/08/01 21:31:52, Siggi Cherem (dart-lang) wrote:
> not sure what the diff means here, is your editor by any chance removing the
> trailing \n?

If there isn't one, and this is adding it back, then that's great :)

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/wrapped_logger.dart (right):

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/wrapped_logger.dart:1: library
polymer.src.build.wrapped_logger;
+ copyright here too

[jakemac, 2014-08-04 19:49:59]

Updated everything, I still need to write some tests as well.

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/add_log_injector.dart (right):

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/add_log_injector.dart:22: 
On 2014/08/01 21:31:51, Siggi Cherem (dart-lang) wrote:
> nit: remove empty line

Done.

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/add_log_injector.dart:24: Future<bool>
isPrimary(idOrAsset) {
On 2014/08/01 21:31:51, Siggi Cherem (dart-lang) wrote:
> let's remove the type (since we return bool or Future<bool> below), or make it
> bool and return synchronously in line 27 too?

Done, good call.

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/add_log_injector.dart:34:
src="packages/polymer/src/build/log_injector.dart"></script>
On 2014/08/01 21:31:51, Siggi Cherem (dart-lang) wrote:
> this line is not necessary since this file is imported explicitly in the
> _bootstrap.dart file that you generate in script_compactor.
> 
> I'm not sure, but this could bring us some trouble today because we have the
> rule that there should be only 1 script tag per page. If you have an
entrypoint
> that contains a script tag, this might fail or produce an extra warning later
> on?

Removed it here, previously it was working because I was doing this before
inlining.

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/add_log_injector.dart:36:
href="packages/polymer/src/build/log_injector.css">
On 2014/08/01 21:31:51, Siggi Cherem (dart-lang) wrote:
> since this is the only thing left to add on this phase, I'm thinking it might
be
> fine to add this also in script compactor and get rid of this extra phase
> entirely.

Added it to the log_combiner phase since that one is more related to this.

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/add_log_injector.dart:37:
${document.body.innerHtml}''';
On 2014/08/01 21:31:51, Siggi Cherem (dart-lang) wrote:
> let's use append, that way we don't serialize and deserialize the html twice.

Done.

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/import_inliner.dart (right):

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/import_inliner.dart:402: uriToAssetId(sourceId, uri,
logger, span, errorOnAbsolute: false);
On 2014/08/01 21:31:51, Siggi Cherem (dart-lang) wrote:
> minor nit: let's keep the original wrapping

Done.

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/linter.dart (right):

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/linter.dart:26: 
On 2014/08/01 21:31:51, Siggi Cherem (dart-lang) wrote:
> nit: remove

Done.

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/linter.dart:474: const ['auto-binding-dart',
'polymer-element'];
On 2014/08/01 21:48:29, Siggi Cherem (dart-lang) wrote:
> On 2014/08/01 21:31:52, Siggi Cherem (dart-lang) wrote:
> > not sure what the diff means here, is your editor by any chance removing the
> > trailing \n?
> 
> If there isn't one, and this is adding it back, then that's great :)

Huh weird, I do have a newline so maybe thats what it was.

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/log_combiner.dart (right):

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/log_combiner.dart:15: class LogCombiner extends
Transformer with PolymerTransformer {
On 2014/08/01 21:31:52, Siggi Cherem (dart-lang) wrote:
> (a) nit: let's add a Dart doc here too. Might be the same that you have in the
> library.
> 
> (b) maybe rename to BuildLogCombiner? or MessagesCombiner, or MessagesMerger?
> 
> The main thing is that 'log' has the same name for runtime and build-time
logs.
> I commonly think of 'log' of the runtime logs, and not the build-time warning
> and error messages. So it would be nice to make the distinction.

Done.

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/log_combiner.dart:16: 
On 2014/08/01 21:31:52, Siggi Cherem (dart-lang) wrote:
> nit: remove empty line

Done.

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/log_combiner.dart:22: Future<bool>
isPrimary(idOrAsset) {
On 2014/08/01 21:31:52, Siggi Cherem (dart-lang) wrote:
> same here: change to 'bool', and remove new Future.value() below?

Done.

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/log_combiner.dart:31: 
On 2014/08/01 21:31:52, Siggi Cherem (dart-lang) wrote:
> nit: remove empty line

Done.

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/log_injector.dart (right):

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/log_injector.dart:1: /// This library provides a
single function called injectLogs which when called
On 2014/08/01 21:31:52, Siggi Cherem (dart-lang) wrote:
> + copyright notice above this

Done.

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/log_injector.dart:11: injectLogs() {
On 2014/08/01 21:31:52, Siggi Cherem (dart-lang) wrote:
> Consider adding a note in a comment here to indicate that we are not using
> polymer here to make sure this page works even if the app fails for some other
> reason. I hope in the future, when we can have multiple Dart scripts on a page
> that are independent of each other, we might be able to do this with Polymer
> instead.

Done.

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/log_injector.dart:19: });
On 2014/08/01 21:31:52, Siggi Cherem (dart-lang) wrote:
> maybe return here if there are no messages?

Done.

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/log_injector.dart:22: var wrapperDiv = new
DivElement()..classes.add('build-logs');
On 2014/08/01 21:31:52, Siggi Cherem (dart-lang) wrote:
> or wrapperDiv = new Element.html('<div class="build-logs"></div>');

But mine is shorter! (and a little faster?)

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/log_injector.dart:34: ..appendHtml('$level <span
class="num">(${logs.length})</span>');
On 2014/08/01 21:31:52, Siggi Cherem (dart-lang) wrote:
> how about:
> menuItem = new Element.html(
>     '<div class="$levelClassName">'
>     '$level <span class="num">(...)</span>'
>     '</div>');

Done.

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/log_injector.dart:42:
wrapperDiv.querySelectorAll('.menu > div, .content > div')
On 2014/08/01 21:31:52, Siggi Cherem (dart-lang) wrote:
> alternatively:
>  - save a list of items/contents? so we don't do a selector all every time?
>  - save the lastActiveItem and remove the 'active' class directly just on
those
> two nodes?

Done, keeping track of the currently selected items now.

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/log_injector.dart:51: logs.forEach((log) {
On 2014/08/01 21:31:52, Siggi Cherem (dart-lang) wrote:
> for long blocks, we tend to use a normal loop instead:
> 
> for (var log in logs) {
>   ...
> }

Done.

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/log_injector.dart:72: var logElement = new
DivElement()
On 2014/08/01 21:31:52, Siggi Cherem (dart-lang) wrote:
> How about at the beginning do:
> logHtml.write('<div class="log">);
> 
> and here do
> logHtml.write('</div>);
> contentItemp.append(new Element.html(logHtml.toString());

Done.

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/script_compactor.dart (right):

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/script_compactor.dart:437: code.writeln(
On 2014/08/01 21:31:52, Siggi Cherem (dart-lang) wrote:
> guard this also with !options.releaseMode?

Done.

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/wrapped_logger.dart (right):

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/wrapped_logger.dart:1: library
polymer.src.build.wrapped_logger;
On 2014/08/01 21:48:29, Siggi Cherem (dart-lang) wrote:
> + copyright here too

Done.

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/wrapped_logger.dart:52: // figures out the next # to
use.
On 2014/08/01 21:31:53, Siggi Cherem (dart-lang) wrote:
> include a TODO that this is a workaround because of a bug in aggregate
> transformers?  maybe include a link to the actual bug?

This just seemed easier to work with, I started going down the
AggregateTransformer route but I would have ended up making all our transforms
AggregateTransforms and was simpler to just keep the logic all within this
class. Technically I did have to create the extra transform at the end as well
though.... maybe we should talk offline about it.

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/wrapped_logger.dart:58: return
getNextLogAssetPath(++nextNumber);
On 2014/08/01 21:31:53, Siggi Cherem (dart-lang) wrote:
> interesting ... another idea, but I'm not convinced if what I'm suggesting is
> any better, so totally up to you.
> 
> we could have an enum of number IDs we expect to use (one per phase). Then the
> name would be fixed:
> 
>   import_inliner  always generates file._buildLog.3
> 
> so you wouldn't need to find what is the next available log. 
> 
> Below, combine would have to check every fixed Id, rather than iterating until
a
> number doesn't exist. 

I think I like just the incrementing number approach since it means we don't
have to update anything here to add this logger to different transforms. I
looked briefly and it doesn't look like calling hasInput actually attempts to
read a file from disk or anything, its just looking up an ID in a map so it
should be really quick.

[Siggi Cherem (dart-lang), 2014-08-04 20:38:37]

Thanks Jake!

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/add_log_injector.dart (right):

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/add_log_injector.dart:36:
href="packages/polymer/src/build/log_injector.css">
On 2014/08/04 19:49:57, jakemac wrote:
> On 2014/08/01 21:31:51, Siggi Cherem (dart-lang) wrote:
> > since this is the only thing left to add on this phase, I'm thinking it
might
> be
> > fine to add this also in script compactor and get rid of this extra phase
> > entirely.
> 
> Added it to the log_combiner phase since that one is more related to this.

makes sense, this might be premature optimization but - since script compactor
is already doing the other changes to include the .dart code for this, I think
it's fine to also add the .css there and save ourselves one more iteration of
parsing and reemiting the HTML file.

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/log_injector.dart (right):

https://codereview.chromium.org/427623002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/log_injector.dart:22: var wrapperDiv = new
DivElement()..classes.add('build-logs');
On 2014/08/04 19:49:58, jakemac wrote:
> On 2014/08/01 21:31:52, Siggi Cherem (dart-lang) wrote:
> > or wrapperDiv = new Element.html('<div class="build-logs"></div>');
> 
> But mine is shorter! (and a little faster?)

:-) let's keep it as you have it. 

As of speed goes, I'm not sure - in this case I don't think it matters, but more
generally browsers are optimized for innerHTML, so doing new Element.html('.....
') with a very long html is often faster than creating that HTML programatically
using the DOM APIs.

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

https://codereview.chromium.org/427623002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/build_log_combiner.dart:34:
document.body.append(parseFragment(
body => head

https://codereview.chromium.org/427623002/diff/60001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/log_injector.css (right):

https://codereview.chromium.org/427623002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/log_injector.css:1: .build-logs {
oops I missed this one too: + /** copyright ... */

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

https://codereview.chromium.org/427623002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/log_injector.dart:19: LogInjector created();
delete?

https://codereview.chromium.org/427623002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/log_injector.dart:21: // Gets the logs and builds the
html for the logs element, and injects that into
nit: 80 colums

https://codereview.chromium.org/427623002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/log_injector.dart:38: var wrapperDiv = new
DivElement()
nit: we could move this wrapper down, and have the 3 cascades together :)

[jakemac, 2014-08-04 22:21:58]

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

https://codereview.chromium.org/427623002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/build_log_combiner.dart:34:
document.body.append(parseFragment(
On 2014/08/04 20:38:37, Siggi Cherem (dart-lang) wrote:
> body => head

Done.

https://codereview.chromium.org/427623002/diff/60001/pkg/polymer/lib/src/buil...
File pkg/polymer/lib/src/build/log_injector.css (right):

https://codereview.chromium.org/427623002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/log_injector.css:1: .build-logs {
On 2014/08/04 20:38:37, Siggi Cherem (dart-lang) wrote:
> oops I missed this one too: + /** copyright ... */

Done.

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

https://codereview.chromium.org/427623002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/log_injector.dart:19: LogInjector created();
On 2014/08/04 20:38:37, Siggi Cherem (dart-lang) wrote:
> delete?

Done.

https://codereview.chromium.org/427623002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/log_injector.dart:21: // Gets the logs and builds the
html for the logs element, and injects that into
On 2014/08/04 20:38:37, Siggi Cherem (dart-lang) wrote:
> nit: 80 colums

Done.

https://codereview.chromium.org/427623002/diff/60001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/log_injector.dart:38: var wrapperDiv = new
DivElement()
On 2014/08/04 20:38:37, Siggi Cherem (dart-lang) wrote:
> nit: we could move this wrapper down, and have the 3 cascades together :)

Done.

[Siggi Cherem (dart-lang), 2014-08-04 22:30:26]

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

https://codereview.chromium.org/427623002/diff/80001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/build_log_combiner.dart:22: // Run only on entry point
html files in !releaseMode.
update comment now that you added injectBuildLogsInOutput

https://codereview.chromium.org/427623002/diff/80001/pkg/polymer/lib/transfor...
File pkg/polymer/lib/transformer.dart (right):

https://codereview.chromium.org/427623002/diff/80001/pkg/polymer/lib/transfor...
pkg/polymer/lib/transformer.dart:46: !releaseMode &&
args['inject_build_logs_in_output'] == true;
let's make it true by default:
 !releaseMode && args['inject_build_logs_in_output'] != false;

(I'd like to have this on by default, and make the flag available to opt-out
instead.)

[jakemac, 2014-08-05 16:17:58]

Added tests for everything but the element itself, working on those now

[jakemac, 2014-08-05 18:33:31]

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

https://codereview.chromium.org/427623002/diff/80001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/build_log_combiner.dart:22: // Run only on entry point
html files in !releaseMode.
On 2014/08/04 22:30:26, Siggi Cherem (dart-lang) wrote:
> update comment now that you added injectBuildLogsInOutput

Done.

https://codereview.chromium.org/427623002/diff/80001/pkg/polymer/lib/transfor...
File pkg/polymer/lib/transformer.dart (right):

https://codereview.chromium.org/427623002/diff/80001/pkg/polymer/lib/transfor...
pkg/polymer/lib/transformer.dart:46: !releaseMode &&
args['inject_build_logs_in_output'] == true;
On 2014/08/04 22:30:26, Siggi Cherem (dart-lang) wrote:
> let's make it true by default:
>  !releaseMode && args['inject_build_logs_in_output'] != false;
> 
> (I'd like to have this on by default, and make the flag available to opt-out
> instead.)

Done.

[Siggi Cherem (dart-lang), 2014-08-05 19:37:21]

https://codereview.chromium.org/427623002/diff/120001/pkg/polymer/lib/src/bui...
File pkg/polymer/lib/src/build/linter.dart (right):

https://codereview.chromium.org/427623002/diff/120001/pkg/polymer/lib/src/bui...
pkg/polymer/lib/src/build/linter.dart:41: var logger = new
WrappedLogger(transform, convertErrorsToWarnings: true);
should this be initialized conditionally, only if injectBuildLogsInOutput &&
!release mode are true?

Otherwise errors will not be errors in pub-build

https://codereview.chromium.org/427623002/diff/120001/pkg/polymer/test/build/...
File pkg/polymer/test/build/build_log_combiner_test.dart (right):

https://codereview.chromium.org/427623002/diff/120001/pkg/polymer/test/build/...
pkg/polymer/test/build/build_log_combiner_test.dart:19: group('combines logs',
logCombiningTests);
nit: I'd inline the tests below and get rid of the group. We can add groups
later if we need to.

https://codereview.chromium.org/427623002/diff/120001/pkg/polymer/test/build/...
pkg/polymer/test/build/build_log_combiner_test.dart:38: String
buildLogString(String level, String message) {
- I'd make this private, and being private, it's ok to have a shorter name for
it so you don't have to wrap above. For example, `_logEntry`.
- nit: use `=>` notation

https://codereview.chromium.org/427623002/diff/120001/pkg/polymer/test/build/...
File pkg/polymer/test/build/import_inliner_test.dart (right):

https://codereview.chromium.org/427623002/diff/120001/pkg/polymer/test/build/...
pkg/polymer/test/build/import_inliner_test.dart:679: // We are wrapping errors
and converting to warnings.
I think we should repeat this test two or three times with each combination of
options:
 - release mode
 - debug mode & inject logs
 - debug mode & ! inject logs

Probably we should do that for all tests that have a log output. You can see how
we wrote polyfill_injector_test.dart as an example of how we repeat tests based
on small variations to the options.

https://codereview.chromium.org/427623002/diff/120001/pkg/polymer/test/build/...
File pkg/polymer/test/build/linter_test.dart (right):

https://codereview.chromium.org/427623002/diff/120001/pkg/polymer/test/build/...
pkg/polymer/test/build/linter_test.dart:297: // We are wrapping this and
converting to a warning.
yeah, same here - I'm inclined to do that for all the linter tests.

https://codereview.chromium.org/427623002/diff/120001/pkg/polymer/test/build/...
File pkg/polymer/test/build/log_injector_test.html (right):

https://codereview.chromium.org/427623002/diff/120001/pkg/polymer/test/build/...
pkg/polymer/test/build/log_injector_test.html:8: <!--polymer-test: this comment
is needed for test_suite.dart-->
I don't think you need this file actually, since your test is just testing the
injector on its own.

This is only needed if you intend to run the polymer build, otherwise, the
test.py script will autogenerate an .html file for any test that says that it is
supposed to run in a browser. Alternatively you can provide an .html file, but
that would look more like this:

https://code.google.com/p/dart/source/browse/branches/bleeding_edge/dart/test...

[jakemac, 2014-08-05 22:58:24]

Updated

https://codereview.chromium.org/427623002/diff/120001/pkg/polymer/lib/src/bui...
File pkg/polymer/lib/src/build/linter.dart (right):

https://codereview.chromium.org/427623002/diff/120001/pkg/polymer/lib/src/bui...
pkg/polymer/lib/src/build/linter.dart:41: var logger = new
WrappedLogger(transform, convertErrorsToWarnings: true);
On 2014/08/05 19:37:21, Siggi Cherem (dart-lang) wrote:
> should this be initialized conditionally, only if injectBuildLogsInOutput &&
> !release mode are true?
> 
> Otherwise errors will not be errors in pub-build

Yes, good catch. This also allowed me to revert the tests back so they can
expect errors again (most of them run in release mode). I chose just
!releaseMode for now at least, this will allow pub serve to keep running even if
you have errors which should help with debugging/etc.

https://codereview.chromium.org/427623002/diff/120001/pkg/polymer/test/build/...
File pkg/polymer/test/build/build_log_combiner_test.dart (right):

https://codereview.chromium.org/427623002/diff/120001/pkg/polymer/test/build/...
pkg/polymer/test/build/build_log_combiner_test.dart:19: group('combines logs',
logCombiningTests);
On 2014/08/05 19:37:21, Siggi Cherem (dart-lang) wrote:
> nit: I'd inline the tests below and get rid of the group. We can add groups
> later if we need to.

Done.

https://codereview.chromium.org/427623002/diff/120001/pkg/polymer/test/build/...
pkg/polymer/test/build/build_log_combiner_test.dart:38: String
buildLogString(String level, String message) {
On 2014/08/05 19:37:21, Siggi Cherem (dart-lang) wrote:
> - I'd make this private, and being private, it's ok to have a shorter name for
> it so you don't have to wrap above. For example, `_logEntry`.
> - nit: use `=>` notation

Done.

https://codereview.chromium.org/427623002/diff/120001/pkg/polymer/test/build/...
File pkg/polymer/test/build/import_inliner_test.dart (right):

https://codereview.chromium.org/427623002/diff/120001/pkg/polymer/test/build/...
pkg/polymer/test/build/import_inliner_test.dart:679: // We are wrapping errors
and converting to warnings.
On 2014/08/05 19:37:21, Siggi Cherem (dart-lang) wrote:
> I think we should repeat this test two or three times with each combination of
> options:
>  - release mode
>  - debug mode & inject logs
>  - debug mode & ! inject logs
> 
> Probably we should do that for all tests that have a log output. You can see
how
> we wrote polyfill_injector_test.dart as an example of how we repeat tests
based
> on small variations to the options.

Done.

https://codereview.chromium.org/427623002/diff/120001/pkg/polymer/test/build/...
File pkg/polymer/test/build/linter_test.dart (right):

https://codereview.chromium.org/427623002/diff/120001/pkg/polymer/test/build/...
pkg/polymer/test/build/linter_test.dart:297: // We are wrapping this and
converting to a warning.
On 2014/08/05 19:37:21, Siggi Cherem (dart-lang) wrote:
> yeah, same here - I'm inclined to do that for all the linter tests.

Done.

https://codereview.chromium.org/427623002/diff/120001/pkg/polymer/test/build/...
File pkg/polymer/test/build/log_injector_test.html (right):

https://codereview.chromium.org/427623002/diff/120001/pkg/polymer/test/build/...
pkg/polymer/test/build/log_injector_test.html:8: <!--polymer-test: this comment
is needed for test_suite.dart-->
On 2014/08/05 19:37:21, Siggi Cherem (dart-lang) wrote:
> I don't think you need this file actually, since your test is just testing the
> injector on its own.
> 
> This is only needed if you intend to run the polymer build, otherwise, the
> test.py script will autogenerate an .html file for any test that says that it
is
> supposed to run in a browser. Alternatively you can provide an .html file, but
> that would look more like this:
> 
>
https://code.google.com/p/dart/source/browse/branches/bleeding_edge/dart/test...

Done, changed this up a bit so the http request is separated out and its easier
to test.

[Siggi Cherem (dart-lang), 2014-08-05 23:35:25]

LGTM!! Thanks Jake!

https://codereview.chromium.org/427623002/diff/140001/pkg/polymer/lib/src/bui...
File pkg/polymer/lib/src/build/import_inliner.dart (right):

https://codereview.chromium.org/427623002/diff/140001/pkg/polymer/lib/src/bui...
pkg/polymer/lib/src/build/import_inliner.dart:44: convertErrorsToWarnings:
!options.releaseMode),
alternatively, we could skip the wrapping in release mode entirely:

logger = options.releaseMode ? transform.logger : new WrapperLogger(transform);

WDYT?

https://codereview.chromium.org/427623002/diff/140001/pkg/polymer/test/build/...
File pkg/polymer/test/build/script_compactor_test.dart (right):

https://codereview.chromium.org/427623002/diff/140001/pkg/polymer/test/build/...
pkg/polymer/test/build/script_compactor_test.dart:163: 'invalid const expression
logs', {
nit: I think it's fine to combine this and the test below as a single test (it's
odd to read its output without the warning)

https://codereview.chromium.org/427623002/diff/140001/pkg/polymer/test/build/...
pkg/polymer/test/build/script_compactor_test.dart:489: testLogOutput(
same for these and below

[jakemac, 2014-08-06 14:11:35]

https://codereview.chromium.org/427623002/diff/140001/pkg/polymer/lib/src/bui...
File pkg/polymer/lib/src/build/import_inliner.dart (right):

https://codereview.chromium.org/427623002/diff/140001/pkg/polymer/lib/src/bui...
pkg/polymer/lib/src/build/import_inliner.dart:44: convertErrorsToWarnings:
!options.releaseMode),
On 2014/08/05 23:35:25, Siggi Cherem (dart-lang) wrote:
> alternatively, we could skip the wrapping in release mode entirely:
> 
> logger = options.releaseMode ? transform.logger : new
WrapperLogger(transform);
> 
> WDYT?

Ya, more efficient that way

https://codereview.chromium.org/427623002/diff/140001/pkg/polymer/test/build/...
File pkg/polymer/test/build/script_compactor_test.dart (right):

https://codereview.chromium.org/427623002/diff/140001/pkg/polymer/test/build/...
pkg/polymer/test/build/script_compactor_test.dart:163: 'invalid const expression
logs', {
On 2014/08/05 23:35:25, Siggi Cherem (dart-lang) wrote:
> nit: I think it's fine to combine this and the test below as a single test
(it's
> odd to read its output without the warning)

The problem is the output changes based on the different settings (it injects
extra code into the dart file). It would require a fair bit of extra work to
deal with that, so i just separated the tests. There is a single test at the
bottom that tests this functionality.

[Siggi Cherem (dart-lang), 2014-08-06 17:25:22]

lgtm!

https://codereview.chromium.org/427623002/diff/140001/pkg/polymer/test/build/...
File pkg/polymer/test/build/script_compactor_test.dart (right):

https://codereview.chromium.org/427623002/diff/140001/pkg/polymer/test/build/...
pkg/polymer/test/build/script_compactor_test.dart:163: 'invalid const expression
logs', {
On 2014/08/06 14:11:34, jakemac wrote:
> On 2014/08/05 23:35:25, Siggi Cherem (dart-lang) wrote:
> > nit: I think it's fine to combine this and the test below as a single test
> (it's
> > odd to read its output without the warning)
> 
> The problem is the output changes based on the different settings (it injects
> extra code into the dart file). It would require a fair bit of extra work to
> deal with that, so i just separated the tests. There is a single test at the
> bottom that tests this functionality.

Ah, got it. makes sense.

https://codereview.chromium.org/427623002/diff/160001/pkg/polymer/lib/src/bui...
File pkg/polymer/lib/src/build/import_inliner.dart (right):

https://codereview.chromium.org/427623002/diff/160001/pkg/polymer/lib/src/bui...
pkg/polymer/lib/src/build/import_inliner.dart:82: if
(options.injectBuildLogsInOutput && logger is WrappedLogger) {
I'm guessing our invariants guarantee that the second condition is always true?

[jakemac, 2014-08-06 19:41:22]

https://codereview.chromium.org/427623002/diff/160001/pkg/polymer/lib/src/bui...
File pkg/polymer/lib/src/build/import_inliner.dart (right):

https://codereview.chromium.org/427623002/diff/160001/pkg/polymer/lib/src/bui...
pkg/polymer/lib/src/build/import_inliner.dart:82: if
(options.injectBuildLogsInOutput && logger is WrappedLogger) {
On 2014/08/06 17:25:22, Siggi Cherem (dart-lang) wrote:
> I'm guessing our invariants guarantee that the second condition is always
true?

Technically not any more, when in release mode we don't wrap it any more,
regardless of the injectBuildLogsInOutput setting.

[Siggi Cherem (dart-lang), 2014-08-06 19:50:22]

lgtm

https://codereview.chromium.org/427623002/diff/160001/pkg/polymer/lib/src/bui...
File pkg/polymer/lib/src/build/import_inliner.dart (right):

https://codereview.chromium.org/427623002/diff/160001/pkg/polymer/lib/src/bui...
pkg/polymer/lib/src/build/import_inliner.dart:82: if
(options.injectBuildLogsInOutput && logger is WrappedLogger) {
On 2014/08/06 19:41:22, jakemac wrote:
> On 2014/08/06 17:25:22, Siggi Cherem (dart-lang) wrote:
> > I'm guessing our invariants guarantee that the second condition is always
> true?
> 
> Technically not any more, when in release mode we don't wrap it any more,
> regardless of the injectBuildLogsInOutput setting.

Ah ok - for some reason I thought we didn't let the option to be true in release
mode, that's why I assumed it was enough.

I'm ok leaving it as it is. In case you like it better, an alternative is to
change it above to only wrap when both release & inject are true, then here we
just need the instance-of check.

[jakemac, 2014-08-06 19:59:38]

https://codereview.chromium.org/427623002/diff/160001/pkg/polymer/lib/src/bui...
File pkg/polymer/lib/src/build/import_inliner.dart (right):

https://codereview.chromium.org/427623002/diff/160001/pkg/polymer/lib/src/bui...
pkg/polymer/lib/src/build/import_inliner.dart:82: if
(options.injectBuildLogsInOutput && logger is WrappedLogger) {
On 2014/08/06 19:50:22, Siggi Cherem (dart-lang) wrote:
> On 2014/08/06 19:41:22, jakemac wrote:
> > On 2014/08/06 17:25:22, Siggi Cherem (dart-lang) wrote:
> > > I'm guessing our invariants guarantee that the second condition is always
> > true?
> > 
> > Technically not any more, when in release mode we don't wrap it any more,
> > regardless of the injectBuildLogsInOutput setting.
> 
> Ah ok - for some reason I thought we didn't let the option to be true in
release
> mode, that's why I assumed it was enough.
> 
> I'm ok leaving it as it is. In case you like it better, an alternative is to
> change it above to only wrap when both release & inject are true, then here we
> just need the instance-of check.

I do want to keep the same semantics around converting errors to warnings though
when not in release mode, so unfortunately I think this is about as good as its
going to get :(.

[Siggi Cherem (dart-lang), 2014-08-06 20:01:44]

ah, good point. sounds good

[jakemac, 2014-08-06 20:27:12]

Committed patchset #9 manually as 38946 (presubmit successful).