[polymer] fix import inliner to only rewrite URL attributes once per node

[polymer] fix import inliner to only rewrite URL attributes once per node

R=sigmund@google.com

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

[Jennifer Messerly, 2014-02-27 20:58:02]

https://codereview.chromium.org/178193007/diff/1/pkg/polymer/test/build/commo...
File pkg/polymer/test/build/common.dart (right):

https://codereview.chromium.org/178193007/diff/1/pkg/polymer/test/build/commo...
pkg/polymer/test/build/common.dart:126: [List<String> expectedMessages, bool
solo = false]) {
this can go, but has been helpful for debugging

https://codereview.chromium.org/178193007/diff/1/pkg/polymer/test/build/impor...
File pkg/polymer/test/build/import_inliner_test.dart (right):

https://codereview.chromium.org/178193007/diff/1/pkg/polymer/test/build/impor...
pkg/polymer/test/build/import_inliner_test.dart:646: testPhases('deep, inlines
css, multiple nesting', phases, {
this test was useful to help me debug, but it doesn't actually prevent
regressions. The extra work we were doing didn't cause any visible behavior
difference.

I could try to add some hooks to ImportInliner for testing, but seems overkill.
The line where we intentionally resolve against "docId" should hopefully prevent
regression.

[Siggi Cherem (dart-lang), 2014-02-27 21:14:06]

https://codereview.chromium.org/178193007/diff/1/pkg/polymer/lib/src/build/im...
File pkg/polymer/lib/src/build/import_inliner.dart (right):

https://codereview.chromium.org/178193007/diff/1/pkg/polymer/lib/src/build/im...
pkg/polymer/lib/src/build/import_inliner.dart:64: Future<bool>
_visitImports(Document document, AssetId sourceId) {
seems like sourceId is no longer needed now?

https://codereview.chromium.org/178193007/diff/1/pkg/polymer/lib/src/build/im...
pkg/polymer/lib/src/build/import_inliner.dart:127: 
nit: up to you, but it feels like this empty line is not necessary

https://codereview.chromium.org/178193007/diff/1/pkg/polymer/lib/src/build/im...
pkg/polymer/lib/src/build/import_inliner.dart:128: new _UrlNormalizer(transform,
id).visit(doc);
I'm a bit confused. This is still applied on every call to _inlineImport, so
technically we are normalizing documents more than once? It probably happens
that subsequent times are not changing urls, but we are still traversing the
doc?

https://codereview.chromium.org/178193007/diff/1/pkg/polymer/lib/src/build/im...
pkg/polymer/lib/src/build/import_inliner.dart:130: 
same here

[Siggi Cherem (dart-lang), 2014-02-27 21:19:01]

lgtm

https://codereview.chromium.org/178193007/diff/1/pkg/polymer/lib/src/build/im...
File pkg/polymer/lib/src/build/import_inliner.dart (right):

https://codereview.chromium.org/178193007/diff/1/pkg/polymer/lib/src/build/im...
pkg/polymer/lib/src/build/import_inliner.dart:128: new _UrlNormalizer(transform,
id).visit(doc);
On 2014/02/27 21:14:06, Siggi Cherem (dart-lang) wrote:
> I'm a bit confused. This is still applied on every call to _inlineImport, so
> technically we are normalizing documents more than once? It probably happens
> that subsequent times are not changing urls, but we are still traversing the
> doc?
> 

just talked offline, I missed that the swap in order here fixes changes the
recursion logic. I can see now that after the change you first process the file,
then recurse (vs first recurse, then process the file).

[Jennifer Messerly, 2014-02-27 21:23:18]

https://codereview.chromium.org/178193007/diff/1/pkg/polymer/lib/src/build/im...
File pkg/polymer/lib/src/build/import_inliner.dart (right):

https://codereview.chromium.org/178193007/diff/1/pkg/polymer/lib/src/build/im...
pkg/polymer/lib/src/build/import_inliner.dart:64: Future<bool>
_visitImports(Document document, AssetId sourceId) {
On 2014/02/27 21:14:06, Siggi Cherem (dart-lang) wrote:
> seems like sourceId is no longer needed now?

Oops! It was removed, and I added it back while debugging for prints, forgot to
remove it again :)

Fixed.

https://codereview.chromium.org/178193007/diff/1/pkg/polymer/lib/src/build/im...
pkg/polymer/lib/src/build/import_inliner.dart:127: 
On 2014/02/27 21:14:06, Siggi Cherem (dart-lang) wrote:
> nit: up to you, but it feels like this empty line is not necessary

yeah, personally I thought it was more readable with the newline, but that might
be a result of viewing it in the Editor which has colors .. "return new return
_extract" was somewhat hard to parse.

but anyway removed newlines.

[Jennifer Messerly, 2014-02-27 21:23:54]

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