Apply several phases only to entrypoint files (which currently are identified as

Apply several phases only to entrypoint files (which currently are identified as
those inside the web/ or test/ directory), and hide doctype warnings when
processing non entrypoint files.

R=jmesserly@google.com

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

[Jennifer Messerly, 2013-09-04 02:35:04]

LGTM other than some minor comments.

https://codereview.chromium.org/23478015/diff/5001/pkg/polymer/lib/src/transf...
File pkg/polymer/lib/src/transform/common.dart (right):

https://codereview.chromium.org/23478015/diff/5001/pkg/polymer/lib/src/transf...
pkg/polymer/lib/src/transform/common.dart:76: Future<bool>
isHtmlInWebOrTest(AssetId id) =>
wondering if we could name this to more reflect what it does (instead of how it
is implemented). Perhaps "isEntryPointHtml" or "isPrimaryHtml" or
"canBePrimaryHtml" (or "nonImportedHtml", "deployedHtml", "publicHtml" ... ? )

https://codereview.chromium.org/23478015/diff/5001/pkg/polymer/lib/src/transf...
pkg/polymer/lib/src/transform/common.dart:77: new Future.value(id.extension ==
'.html' &&
hmm. Perhaps add a comment that these are always a package-rooted paths. also
might be good in a public doc comment to mention the convention we are using:
"web" and "test" HTML documents are assumed to be ones that can be loaded
standalone in the browser, and therefore should be deployed.

I was worried about getting tests to run if the structure doesn't match a Dart
package. But since you got the content_shell tests working, it must be okay for
that :)


Unrelated note: do you know why `id.extension` includes the dot?

https://codereview.chromium.org/23478015/diff/5001/pkg/polymer/lib/src/transf...
File pkg/polymer/lib/src/transform/import_inliner.dart (right):

https://codereview.chromium.org/23478015/diff/5001/pkg/polymer/lib/src/transf...
pkg/polymer/lib/src/transform/import_inliner.dart:20: /** Only run on entrypoint
.html files under web or test. */
nit: space between entry and point

https://codereview.chromium.org/23478015/diff/5001/pkg/polymer/lib/src/transf...
pkg/polymer/lib/src/transform/import_inliner.dart:87: var document =
parseHtml(content, id.path, transform.logger);
it's not a big deal but i'm not sure about flipping this default. In my mind,
entry point HTML documents are the classic, normal ones; imported HTML documents
are the new fancy ones :)

[Siggi Cherem (dart-lang), 2013-09-04 16:30:15]

thanks!

https://codereview.chromium.org/23478015/diff/5001/pkg/polymer/lib/src/transf...
File pkg/polymer/lib/src/transform/common.dart (right):

https://codereview.chromium.org/23478015/diff/5001/pkg/polymer/lib/src/transf...
pkg/polymer/lib/src/transform/common.dart:76: Future<bool>
isHtmlInWebOrTest(AssetId id) =>
On 2013/09/04 02:35:04, John Messerly wrote:
> wondering if we could name this to more reflect what it does (instead of how
it
> is implemented). Perhaps "isEntryPointHtml" or "isPrimaryHtml" or
> "canBePrimaryHtml" (or "nonImportedHtml", "deployedHtml", "publicHtml" ... ? )

Done. Went with isPrimaryHtml

https://codereview.chromium.org/23478015/diff/5001/pkg/polymer/lib/src/transf...
pkg/polymer/lib/src/transform/common.dart:77: new Future.value(id.extension ==
'.html' &&
On 2013/09/04 02:35:04, John Messerly wrote:
> hmm. Perhaps add a comment that these are always a package-rooted paths.

Done

> also
> might be good in a public doc comment to mention the convention we are using:
> "web" and "test" HTML documents are assumed to be ones that can be loaded
> standalone in the browser, and therefore should be deployed.

Done (added to deploy.dart for now). Note that by default we don't deploy what's
under 'test/' (we don't list it in deploy.dart, only our test script will do so)

> 
> I was worried about getting tests to run if the structure doesn't match a Dart
> package. But since you got the content_shell tests working, it must be okay
for
> that :)

I got tests with 'web' directories working, but I have not played with the test/
folder yet. I actually included 'test/' above after I saw your change that adds
support for it. I'm happy to wait and submit this after your CL, that way I
don't add anything that would hold your change any longer =)

> 
> 
> Unrelated note: do you know why `id.extension` includes the dot?

I think this is to match what 'package:path' does.

https://codereview.chromium.org/23478015/diff/5001/pkg/polymer/lib/src/transf...
File pkg/polymer/lib/src/transform/import_inliner.dart (right):

https://codereview.chromium.org/23478015/diff/5001/pkg/polymer/lib/src/transf...
pkg/polymer/lib/src/transform/import_inliner.dart:20: /** Only run on entrypoint
.html files under web or test. */
On 2013/09/04 02:35:04, John Messerly wrote:
> nit: space between entry and point

Done.

https://codereview.chromium.org/23478015/diff/5001/pkg/polymer/lib/src/transf...
pkg/polymer/lib/src/transform/import_inliner.dart:87: var document =
parseHtml(content, id.path, transform.logger);
On 2013/09/04 02:35:04, John Messerly wrote:
> it's not a big deal but i'm not sure about flipping this default. In my mind,
> entry point HTML documents are the classic, normal ones; imported HTML
documents
> are the new fancy ones :)

:) - good point, I flipped it back.

[Siggi Cherem (dart-lang), 2013-09-04 19:49:58]

Committed patchset #5 manually as r27142 (presubmit successful).