combine script extractor and import inliner

[polymer] combine script extractor and import inliner
The goal is to ensure we aren't processing HTML files that aren't reachable from the entry point(s).

R=sigmund@google.com

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

[Jennifer Messerly, 2014-02-26 02:38:55]

This is mainly to avoid ~300 HTML files being processed unnecessarily.

(we may also want to skip linting, but that's a separate issue)

[Jennifer Messerly, 2014-02-26 02:43:01]

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

https://codereview.chromium.org/180933002/diff/20001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/import_inliner.dart:157: bool _extractScripts(Document
document, {bool htmlImport: false}) {
Meshing together these two functions didn't work quite as well as I had hoped.
It started clean, but the main HTML page needs the script extraction without the
script removal. I'm going to try splitting it and see if it helps.

[Jennifer Messerly, 2014-02-26 03:04:45]

Okay, done changing. Read for you take a look :)

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

https://codereview.chromium.org/180933002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/import_inliner.dart:173: bool
_extractScripts(List<Element> scripts) {
the refactored version

[Siggi Cherem (dart-lang), 2014-02-26 22:27:09]

very nice. Minor comments below...

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

https://codereview.chromium.org/180933002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/import_inliner.dart:163: // others are already removed
by _extractInlineScripts
_extarctInlineScripts => _extractScripts

https://codereview.chromium.org/180933002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/import_inliner.dart:201: script.attributes['src'] =
src = '$filename.$count.dart';
I never bothered fixing this, but since there is only 1 script per page, the
$count here is not necessary. Feel free to remove it altogether.

https://codereview.chromium.org/180933002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/import_inliner.dart:319: 
nit: remove empty line

https://codereview.chromium.org/180933002/diff/40001/pkg/polymer/test/build/a...
File pkg/polymer/test/build/all_phases_test.dart (right):

https://codereview.chromium.org/180933002/diff/40001/pkg/polymer/test/build/a...
pkg/polymer/test/build/all_phases_test.dart:122: 'a|web/a.dart':
_sampleObservableOutput('A', 'foo'),
consider providing also the expectedMessages argument in this call to testPhases
to check that we complain about the multiple script tags?

https://codereview.chromium.org/180933002/diff/40001/pkg/polymer/test/build/c...
File pkg/polymer/test/build/code_extractor.dart (right):

https://codereview.chromium.org/180933002/diff/40001/pkg/polymer/test/build/c...
pkg/polymer/test/build/code_extractor.dart:6: // Keeping here for now so it's
easier to see the diff.
nice, thank you :)

https://codereview.chromium.org/180933002/diff/40001/pkg/polymer/test/build/c...
pkg/polymer/test/build/code_extractor.dart:90: testPhases('fixes
import/export/part URIs', phases, {
Could we add a similar test that includes files in lib/ and in other packages?

That would make sure that the import normalization works well when the .dart
code is moved across the "package:"-boundary (from web to lib) and between
packages.

[Jennifer Messerly, 2014-02-27 22:31:40]

PTAL

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

https://codereview.chromium.org/180933002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/import_inliner.dart:163: // others are already removed
by _extractInlineScripts
On 2014/02/26 22:27:09, Siggi Cherem (dart-lang) wrote:
> _extarctInlineScripts => _extractScripts

Done.

https://codereview.chromium.org/180933002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/import_inliner.dart:201: script.attributes['src'] =
src = '$filename.$count.dart';
On 2014/02/26 22:27:09, Siggi Cherem (dart-lang) wrote:
> I never bothered fixing this, but since there is only 1 script per page, the
> $count here is not necessary. Feel free to remove it altogether.

good catch! done.

https://codereview.chromium.org/180933002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/import_inliner.dart:319: 
On 2014/02/26 22:27:09, Siggi Cherem (dart-lang) wrote:
> nit: remove empty line

eeeeeek. that was ugly! fixed.

https://codereview.chromium.org/180933002/diff/40001/pkg/polymer/test/build/a...
File pkg/polymer/test/build/all_phases_test.dart (right):

https://codereview.chromium.org/180933002/diff/40001/pkg/polymer/test/build/a...
pkg/polymer/test/build/all_phases_test.dart:122: 'a|web/a.dart':
_sampleObservableOutput('A', 'foo'),
On 2014/02/26 22:27:09, Siggi Cherem (dart-lang) wrote:
> consider providing also the expectedMessages argument in this call to
testPhases
> to check that we complain about the multiple script tags?

done. All of our warnings are emitted too much (I noticed this running "pub
build" too) ... not sure what's up but something we should look at. Filed
https://code.google.com/p/dart/issues/detail?id=17197

https://codereview.chromium.org/180933002/diff/40001/pkg/polymer/test/build/c...
File pkg/polymer/test/build/code_extractor.dart (right):

https://codereview.chromium.org/180933002/diff/40001/pkg/polymer/test/build/c...
pkg/polymer/test/build/code_extractor.dart:90: testPhases('fixes
import/export/part URIs', phases, {
On 2014/02/26 22:27:09, Siggi Cherem (dart-lang) wrote:
> Could we add a similar test that includes files in lib/ and in other packages?
> 
> That would make sure that the import normalization works well when the .dart
> code is moved across the "package:"-boundary (from web to lib) and between
> packages.

GOOOOOOD catch! Cross pkg URLs were totally broken from a canonicalization
standpoint because they were emitted as HTML URLs: "packages/a/...". I changed
it in ImportInliner and added new tests.

[Jennifer Messerly, 2014-02-27 22:32:19]

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

https://codereview.chromium.org/180933002/diff/80001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/import_inliner.dart:309: var primaryId =
transform.primaryInput.id;
fyi -- this was the fix for URLs from pkgs

[Siggi Cherem (dart-lang), 2014-02-28 01:26:45]

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

https://codereview.chromium.org/180933002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/import_inliner.dart:201: script.attributes['src'] =
src = '$filename.$count.dart';
On 2014/02/27 22:31:40, John Messerly wrote:
> On 2014/02/26 22:27:09, Siggi Cherem (dart-lang) wrote:
> > I never bothered fixing this, but since there is only 1 script per page, the
> > $count here is not necessary. Feel free to remove it altogether.
> 
> good catch! done.

sigh - sorry, now that I reread things I think we need to revert this portion,
my bad. In particular, seems that now all files are named 'entrypoint.html.dart'
(it was unnecessary before when the file was next to the .html where it
originated from, but now I think we need the counter again (or we could create
folders to mimic the code structure where these .dart files came from, but the
could be fine).

https://codereview.chromium.org/180933002/diff/80001/pkg/polymer/test/build/c...
File pkg/polymer/test/build/code_extractor.dart (right):

https://codereview.chromium.org/180933002/diff/80001/pkg/polymer/test/build/c...
pkg/polymer/test/build/code_extractor.dart:88: });
to check the issue I broke with the .$counter thing, maybe we should add a test
that has 2 .html files with inline .dart code in them?

[Jennifer Messerly, 2014-02-28 02:21:49]

PTYAL

(Please Take Yet Another Look)

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

https://codereview.chromium.org/180933002/diff/40001/pkg/polymer/lib/src/buil...
pkg/polymer/lib/src/build/import_inliner.dart:201: script.attributes['src'] =
src = '$filename.$count.dart';
On 2014/02/28 01:26:45, Siggi Cherem (dart-lang) wrote:
> On 2014/02/27 22:31:40, John Messerly wrote:
> > On 2014/02/26 22:27:09, Siggi Cherem (dart-lang) wrote:
> > > I never bothered fixing this, but since there is only 1 script per page,
the
> > > $count here is not necessary. Feel free to remove it altogether.
> > 
> > good catch! done.
> 
> sigh - sorry, now that I reread things I think we need to revert this portion,
> my bad. In particular, seems that now all files are named
'entrypoint.html.dart'
> (it was unnecessary before when the file was next to the .html where it
> originated from, but now I think we need the counter again (or we could create
> folders to mimic the code structure where these .dart files came from, but the
> could be fine).

hah, yeah. Fixed now.

https://codereview.chromium.org/180933002/diff/80001/pkg/polymer/test/build/c...
File pkg/polymer/test/build/code_extractor.dart (right):

https://codereview.chromium.org/180933002/diff/80001/pkg/polymer/test/build/c...
pkg/polymer/test/build/code_extractor.dart:88: });
On 2014/02/28 01:26:45, Siggi Cherem (dart-lang) wrote:
> to check the issue I broke with the .$counter thing, maybe we should add a
test
> that has 2 .html files with inline .dart code in them?

done.

[Siggi Cherem (dart-lang), 2014-02-28 02:33:25]

L   GGG  TTT  M   M
L   G     T   MM MM
L   G G   T   M M M
LL  GGG   T   M   M

[Siggi Cherem (dart-lang), 2014-02-28 02:33:57]

(lgtm so rietveld can show the nice green color band too)

[Jennifer Messerly, 2014-02-28 02:50:10]

Committed patchset #6 manually as r33145 (presubmit successful).