Support for parsing all CSS and producing one CSS file

Support for parsing all CSS and producing one CSS file


BUG=

Committed: https://github.com/dart-lang/web-ui/commit/92b680a

[terry, 2013-03-07 17:16:39]

Handle parsing all local stylesheets and emitting one style sheet per app.

[Siggi Cherem (dart-lang), 2013-03-07 22:14:20]

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/analyzer.dart
File lib/src/analyzer.dart (right):

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/analyzer.d...
lib/src/analyzer.dart:730: if (rel != 'components' && rel != 'stylesheet')
return;
there seems to be a bad merge here. this should also include the singular
'component' as well as the plural 'components'

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/analyzer.d...
lib/src/analyzer.dart:753: case 'components':
given that there are only two options (since you returned earlier on anything
else), a simple if should do:

if (rel == 'component' || rel == 'components') {
  _fileInfo.componentLinks.add(path);
} else {
  ...
}

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/compiler.dart
File lib/src/compiler.dart (right):

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/compiler.d...
lib/src/compiler.dart:461: _processStylesheet(uriVisitor, fileInfo, options:
options);
seems like something is broken here? the parameters don't match for
_processStylesheet and this call.

Also, you seem to be running the uri visitor on the _parseCSS too. Do we need to
run it in all these places, or can we remove the uriVisitor argument here?

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/compiler.d...
lib/src/compiler.dart:474: _processStylesheet(uriVisitor, fileInfo, options:
options);
remove the uri visitor here?

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/compiler.d...
lib/src/compiler.dart:665: StyleSheet _parseCSS(String src, String content,
UriVisitor uriVisitor,
style nit: change to _parseCss (3 letter acronym can't be all CAPS)

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/emitters.dart
File lib/src/emitters.dart (right):

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/emitters.d...
lib/src/emitters.dart:534: if (rel == 'components') {
do you mind adding rel == 'component' too, I think we had fixed the analyzer,
but we didn't fix it here.

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/emitters.d...
lib/src/emitters.dart:536: } else if (rel == 'stylesheet' &&
!href.startsWith('http')) {
let's check also that && cssPolyfill is true, otherwise we don't want to modify
this either.

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/emitters.d...
lib/src/emitters.dart:543: var linkElem = new Element.html('<link
rel="stylesheet" type="text/css"'
likewise - we should only add this new link if cssPolyfill is true.

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/emitters.d...
lib/src/emitters.dart:545: document.head.insertBefore(linkElem, null);
should this be done as the first child so that the css is downloaded as soon as
possible? For instance:
head.insertBefore(linkElem, head.hasChildNodes() ?  head.nodes.first : null);

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/files.dart
File lib/src/files.dart (right):

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/files.dart...
lib/src/files.dart:18: final int fileType;
how about simply 'type' (since we are inside SourceFile, the 'file' feels a bit
redundant to me)

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/files.dart...
lib/src/files.dart:22: String linkedCssContent;
I think I prefer to simply reuse 'code' for this too. Basically if type ==
STYLESHEET, code == the contents. 

Alternatively, we could rename 'code' to 'contents' or start using subclasses
for source file.

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/html_css_f...
File lib/src/html_css_fixup.dart (right):

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/html_css_f...
lib/src/html_css_fixup.dart:32: // For components only 1 stylesheet allowed.
let's emit a warning instead of silently ignoring when length > 1

Let's do that in analyzer and not here in this method, though.

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/html_css_f...
lib/src/html_css_fixup.dart:183: final Path _cssFilePath;
I'm having trouble understanding these fields. What do they represent? we might
need to rename them a bit?

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/html_css_f...
lib/src/html_css_fixup.dart:188: _lastCssFilePathSegment = paths.length > 0 ?
paths.last : "";
I'm confused about this... if the idea is to transform URLs, then we need the
entire path, not just the last segment of the path.

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/html_css_f...
lib/src/html_css_fixup.dart:188: _lastCssFilePathSegment = paths.length > 0 ?
paths.last : "";
seems that you can instead write:
  _lastCssFilePathSegment = _cssFilePath.directoryPath.filename;

this should be much cleaner once we move to pathos

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/html_css_f...
lib/src/html_css_fixup.dart:192: Path uriPath;
nit: use var instead of the explicit type.

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/html_css_f...
lib/src/html_css_fixup.dart:194: _cssFilePath.directoryPath.toString()) {
_mainFilePath and _cssFilePath are final, how about instead of storing them,
just store the result of this comparison in the constructor.

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/html_css_f...
lib/src/html_css_fixup.dart:197: uriPath = new Path(node.text);
switch to just use String (you seem to create a Path and immediately convert it
back to a String in line 199)

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/html_css_f...
lib/src/html_css_fixup.dart:199: String newHRef =
_pathInfo.transformUrl(_mainFilePath, uriPath.toString());
nit: remove this variable, do the assignment direclty below.

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/info.dart
File lib/src/info.dart (right):

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/info.dart#...
lib/src/info.dart:150: Path transformUrlToPath(Path src, String target) {
seems that you no longer need this part of the change (I didn't find anywhere
where you use transformUrlPath), let's revert the changes here (lines 150..160)

https://chromiumcodereview.appspot.com/12474002/diff/17001/test/emitter_test....
File test/emitter_test.dart (right):

https://chromiumcodereview.appspot.com/12474002/diff/17001/test/emitter_test....
test/emitter_test.dart:328: '<link href="/a.css" rel="stylesheet">'
we shouldn't be deleting this entry in the output. We should make sure to only
remove a css file if we have access to it in a local path and we can rewrite it.
We should also have another test that checks the old functionality when css
polyfill is turned off.

https://chromiumcodereview.appspot.com/12474002/diff/17001/test/emitter_test....
test/emitter_test.dart:349: group('transform css urls', () {
the purpose of this test was to check that transform_url was working correctly
when we were not modifying existing stylesheets, can we split this in 2 tests?

one test that did the same as before (for that we need to turn off csspolyfill)

another that checks what you have.

[terry, 2013-03-08 20:11:24]

Thanks Siggi, PTAL

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/analyzer.dart
File lib/src/analyzer.dart (right):

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/analyzer.d...
lib/src/analyzer.dart:730: if (rel != 'components' && rel != 'stylesheet')
return;
Nice catch yeah it was a bad merge.

On 2013/03/07 22:14:20, Siggi Cherem (dart-lang) wrote:
> there seems to be a bad merge here. this should also include the singular
> 'component' as well as the plural 'components'

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/analyzer.d...
lib/src/analyzer.dart:753: case 'components':
On 2013/03/07 22:14:20, Siggi Cherem (dart-lang) wrote:
> given that there are only two options (since you returned earlier on anything
> else), a simple if should do:
> 
> if (rel == 'component' || rel == 'components') {
>   _fileInfo.componentLinks.add(path);
> } else {
>   ...
> }

Done.

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/compiler.dart
File lib/src/compiler.dart (right):

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/compiler.d...
lib/src/compiler.dart:461: _processStylesheet(uriVisitor, fileInfo, options:
options);
The name in processStylesheet was pathInfo (it's should be uriVisitor) but the
type is right.

This is no longer necessary once I separated out the linked stylesheets as
separate files that are processed. See _processStyleSheetFile.

On 2013/03/07 22:14:20, Siggi Cherem (dart-lang) wrote:
> seems like something is broken here? the parameters don't match for
> _processStylesheet and this call.
> 
> Also, you seem to be running the uri visitor on the _parseCSS too. Do we need
to
> run it in all these places, or can we remove the uriVisitor argument here?

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/compiler.d...
lib/src/compiler.dart:474: _processStylesheet(uriVisitor, fileInfo, options:
options);
This is the UriVisitor for each component to handle <style scoped> tags.
On 2013/03/07 22:14:20, Siggi Cherem (dart-lang) wrote:
> remove the uri visitor here?

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/compiler.d...
lib/src/compiler.dart:665: StyleSheet _parseCSS(String src, String content,
UriVisitor uriVisitor,
On 2013/03/07 22:14:20, Siggi Cherem (dart-lang) wrote:
> style nit: change to _parseCss (3 letter acronym can't be all CAPS)

Done.

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/emitters.dart
File lib/src/emitters.dart (right):

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/emitters.d...
lib/src/emitters.dart:534: if (rel == 'components') {
On 2013/03/07 22:14:20, Siggi Cherem (dart-lang) wrote:
> do you mind adding rel == 'component' too, I think we had fixed the analyzer,
> but we didn't fix it here.

Done.

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/emitters.d...
lib/src/emitters.dart:536: } else if (rel == 'stylesheet' &&
!href.startsWith('http')) {
I didn't think of this as polyfill but more of how to get the Uri's fixed up but
now that you mention it I think it makes since to make this a part polyfill for
now it's easier to remember that nothing happens if cssPolyfill is off.
On 2013/03/07 22:14:20, Siggi Cherem (dart-lang) wrote:
> let's check also that && cssPolyfill is true, otherwise we don't want to
modify
> this either.

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/emitters.d...
lib/src/emitters.dart:543: var linkElem = new Element.html('<link
rel="stylesheet" type="text/css"'
On 2013/03/07 22:14:20, Siggi Cherem (dart-lang) wrote:
> likewise - we should only add this new link if cssPolyfill is true.

Done.

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/emitters.d...
lib/src/emitters.dart:545: document.head.insertBefore(linkElem, null);
Good idea to make it first.
On 2013/03/07 22:14:20, Siggi Cherem (dart-lang) wrote:
> should this be done as the first child so that the css is downloaded as soon
as
> possible? For instance:
> head.insertBefore(linkElem, head.hasChildNodes() ?  head.nodes.first : null);

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/files.dart
File lib/src/files.dart (right):

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/files.dart...
lib/src/files.dart:18: final int fileType;
On 2013/03/07 22:14:20, Siggi Cherem (dart-lang) wrote:
> how about simply 'type' (since we are inside SourceFile, the 'file' feels a
bit
> redundant to me)

Done.

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/files.dart...
lib/src/files.dart:22: String linkedCssContent;
On 2013/03/07 22:14:20, Siggi Cherem (dart-lang) wrote:
> I think I prefer to simply reuse 'code' for this too. Basically if type ==
> STYLESHEET, code == the contents. 
> 
> Alternatively, we could rename 'code' to 'contents' or start using subclasses
> for source file.

Done.

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/html_css_f...
File lib/src/html_css_fixup.dart (right):

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/html_css_f...
lib/src/html_css_fixup.dart:32: // For components only 1 stylesheet allowed.
On 2013/03/07 22:14:20, Siggi Cherem (dart-lang) wrote:
> let's emit a warning instead of silently ignoring when length > 1
> 
> Let's do that in analyzer and not here in this method, though.

Done.

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/html_css_f...
lib/src/html_css_fixup.dart:183: final Path _cssFilePath;
mainFile is the main application file (.HTML)
cssFile is the current CSS stylesheet.
On 2013/03/07 22:14:20, Siggi Cherem (dart-lang) wrote:
> I'm having trouble understanding these fields. What do they represent? we
might
> need to rename them a bit?

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/html_css_f...
lib/src/html_css_fixup.dart:188: _lastCssFilePathSegment = paths.length > 0 ?
paths.last : "";
On 2013/03/07 22:14:20, Siggi Cherem (dart-lang) wrote:
> seems that you can instead write:
>   _lastCssFilePathSegment = _cssFilePath.directoryPath.filename;
> 
> this should be much cleaner once we move to pathos

Done.

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/html_css_f...
lib/src/html_css_fixup.dart:192: Path uriPath;
On 2013/03/07 22:14:20, Siggi Cherem (dart-lang) wrote:
> nit: use var instead of the explicit type.

Done.

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/html_css_f...
lib/src/html_css_fixup.dart:194: _cssFilePath.directoryPath.toString()) {
Done.

On 2013/03/07 22:14:20, Siggi Cherem (dart-lang) wrote:
> _mainFilePath and _cssFilePath are final, how about instead of storing them,
> just store the result of this comparison in the constructor.

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/html_css_f...
lib/src/html_css_fixup.dart:197: uriPath = new Path(node.text);
On 2013/03/07 22:14:20, Siggi Cherem (dart-lang) wrote:
> switch to just use String (you seem to create a Path and immediately convert
it
> back to a String in line 199)
> 

Done.

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/html_css_f...
lib/src/html_css_fixup.dart:199: String newHRef =
_pathInfo.transformUrl(_mainFilePath, uriPath.toString());
On 2013/03/07 22:14:20, Siggi Cherem (dart-lang) wrote:
> nit: remove this variable, do the assignment direclty below.

Done.

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/info.dart
File lib/src/info.dart (right):

https://chromiumcodereview.appspot.com/12474002/diff/17001/lib/src/info.dart#...
lib/src/info.dart:150: Path transformUrlToPath(Path src, String target) {
On 2013/03/07 22:14:20, Siggi Cherem (dart-lang) wrote:
> seems that you no longer need this part of the change (I didn't find anywhere
> where you use transformUrlPath), let's revert the changes here (lines
150..160)

Done.

https://chromiumcodereview.appspot.com/12474002/diff/17001/test/emitter_test....
File test/emitter_test.dart (right):

https://chromiumcodereview.appspot.com/12474002/diff/17001/test/emitter_test....
test/emitter_test.dart:328: '<link href="/a.css" rel="stylesheet">'
On 2013/03/07 22:14:20, Siggi Cherem (dart-lang) wrote:
> we shouldn't be deleting this entry in the output. We should make sure to only
> remove a css file if we have access to it in a local path and we can rewrite
it.
> We should also have another test that checks the old functionality when css
> polyfill is turned off.

Yeah, I've reverted this to test w/o polyfill I'll add my own tests.
Done.

https://chromiumcodereview.appspot.com/12474002/diff/17001/test/emitter_test....
test/emitter_test.dart:349: group('transform css urls', () {
On 2013/03/07 22:14:20, Siggi Cherem (dart-lang) wrote:
> the purpose of this test was to check that transform_url was working correctly
> when we were not modifying existing stylesheets, can we split this in 2 tests?
> 
> one test that did the same as before (for that we need to turn off
csspolyfill)
> 
> another that checks what you have. 

Done.

[Siggi Cherem (dart-lang), 2013-03-08 21:57:52]

Thanks Terry, just a few more comments...

https://chromiumcodereview.appspot.com/12474002/diff/33001/lib/src/compiler.dart
File lib/src/compiler.dart (right):

https://chromiumcodereview.appspot.com/12474002/diff/33001/lib/src/compiler.d...
lib/src/compiler.dart:254: ..code = code.trim())
Maybe remove 'trim' here?

You are also trimming the content in _parseCss. Alternatively, keep it here,
remove it in _parseCss, and add it to the other call to _parseCss.

https://chromiumcodereview.appspot.com/12474002/diff/33001/lib/src/compiler.d...
lib/src/compiler.dart:451: var fileInfo = info[file.path];
delete this unused variable

https://chromiumcodereview.appspot.com/12474002/diff/33001/lib/src/compiler.d...
lib/src/compiler.dart:466: if (!fileInfo.isEntryPoint) {
&& options.processCss

https://chromiumcodereview.appspot.com/12474002/diff/33001/lib/src/compiler.d...
lib/src/compiler.dart:542: StringBuffer allCssBuff = new StringBuffer();
style nit: use var

https://chromiumcodereview.appspot.com/12474002/diff/33001/lib/src/compiler.d...
lib/src/compiler.dart:543: 
style nit, delete empty line

https://chromiumcodereview.appspot.com/12474002/diff/33001/lib/src/compiler.d...
lib/src/compiler.dart:564: var fileInfo = info[file.path];
move this down inside the next if

https://chromiumcodereview.appspot.com/12474002/diff/33001/lib/src/compiler.d...
lib/src/compiler.dart:565: if (!file.isStyleSheet) {
!file.isStylesheet => file.isHtml
(skip dart files too?)

https://chromiumcodereview.appspot.com/12474002/diff/33001/lib/src/compiler.d...
lib/src/compiler.dart:594: if (component.styleSheets.length > 1) {
&& options.processCss

https://chromiumcodereview.appspot.com/12474002/diff/33001/lib/src/compiler.d...
lib/src/compiler.dart:710: var content = node.nodes.single.value.toString();
do we need a 'toString'? I'm not sure what's the type of node.nodes.single, but
if the node is a Text node, I believe it's value is already a String.

https://chromiumcodereview.appspot.com/12474002/diff/33001/lib/src/html_css_f...
File lib/src/html_css_fixup.dart (right):

https://chromiumcodereview.appspot.com/12474002/diff/33001/lib/src/html_css_f...
lib/src/html_css_fixup.dart:201: '$_lastCssFilePathSegment/${node.text}' :
node.text;
mm... I'm still confused about this. Should we be joining only the last path
segment or not the entire path?

I thought you'd want to do something more like a relative path from main to css
and append the url to that.

For example, add a field like _urlBasePath or _cssRelativePath:

  final Path _cssRelativePath =
     _cssFilePath.directoryPath.relativeTo(_mainFilePath);

(so you can remove _pathEquals and _cssFilePath)

and then this method would simply do:

  node.text = _pathInfo.transformUrl(_mainFilePath,
    _cssRelativePath.join(node.text).toString());

[terry, 2013-03-08 22:42:23]

PTAL

https://chromiumcodereview.appspot.com/12474002/diff/33001/lib/src/compiler.dart
File lib/src/compiler.dart (right):

https://chromiumcodereview.appspot.com/12474002/diff/33001/lib/src/compiler.d...
lib/src/compiler.dart:254: ..code = code.trim())
I like it _parseCss only.
On 2013/03/08 21:57:52, Siggi Cherem (dart-lang) wrote:
> Maybe remove 'trim' here?
> 
> You are also trimming the content in _parseCss. Alternatively, keep it here,
> remove it in _parseCss, and add it to the other call to _parseCss.

https://chromiumcodereview.appspot.com/12474002/diff/33001/lib/src/compiler.d...
lib/src/compiler.dart:451: var fileInfo = info[file.path];
On 2013/03/08 21:57:52, Siggi Cherem (dart-lang) wrote:
> delete this unused variable

Done.

https://chromiumcodereview.appspot.com/12474002/diff/33001/lib/src/compiler.d...
lib/src/compiler.dart:466: if (!fileInfo.isEntryPoint) {
I'm not sure this is right.  Don't we want to support var, calc, mixin etc.
regardless of polyfill?

On 2013/03/08 21:57:52, Siggi Cherem (dart-lang) wrote:
> && options.processCss

https://chromiumcodereview.appspot.com/12474002/diff/33001/lib/src/compiler.d...
lib/src/compiler.dart:542: StringBuffer allCssBuff = new StringBuffer();
On 2013/03/08 21:57:52, Siggi Cherem (dart-lang) wrote:
> style nit: use var

Done.

https://chromiumcodereview.appspot.com/12474002/diff/33001/lib/src/compiler.d...
lib/src/compiler.dart:543: 
On 2013/03/08 21:57:52, Siggi Cherem (dart-lang) wrote:
> style nit, delete empty line

Done.

https://chromiumcodereview.appspot.com/12474002/diff/33001/lib/src/compiler.d...
lib/src/compiler.dart:564: var fileInfo = info[file.path];
On 2013/03/08 21:57:52, Siggi Cherem (dart-lang) wrote:
> move this down inside the next if

Done.

https://chromiumcodereview.appspot.com/12474002/diff/33001/lib/src/compiler.d...
lib/src/compiler.dart:565: if (!file.isStyleSheet) {
Right should only be isHtml (dart and stylesheets aren't processed here).
On 2013/03/08 21:57:52, Siggi Cherem (dart-lang) wrote:
> !file.isStylesheet => file.isHtml
> (skip dart files too?)

https://chromiumcodereview.appspot.com/12474002/diff/33001/lib/src/compiler.d...
lib/src/compiler.dart:594: if (component.styleSheets.length > 1) {
On 2013/03/08 21:57:52, Siggi Cherem (dart-lang) wrote:
> && options.processCss

Done.

https://chromiumcodereview.appspot.com/12474002/diff/33001/lib/src/compiler.d...
lib/src/compiler.dart:594: if (component.styleSheets.length > 1) {
On 2013/03/08 21:57:52, Siggi Cherem (dart-lang) wrote:
> && options.processCss

Done.

https://chromiumcodereview.appspot.com/12474002/diff/33001/lib/src/compiler.d...
lib/src/compiler.dart:710: var content = node.nodes.single.value.toString();
Yep only need .value

On 2013/03/08 21:57:52, Siggi Cherem (dart-lang) wrote:
> do we need a 'toString'? I'm not sure what's the type of node.nodes.single,
but
> if the node is a Text node, I believe it's value is already a String.

https://chromiumcodereview.appspot.com/12474002/diff/33001/lib/src/html_css_f...
File lib/src/html_css_fixup.dart (right):

https://chromiumcodereview.appspot.com/12474002/diff/33001/lib/src/html_css_f...
lib/src/html_css_fixup.dart:201: '$_lastCssFilePathSegment/${node.text}' :
node.text;
I think it can be simpler than that:

Path cssRelativePath = _cssFilePath.directoryPath.relativeTo(_mainFilePath);
node.text = '${cssRelativePath.toString()}/${node.text}';

Does that make sense?

On 2013/03/08 21:57:52, Siggi Cherem (dart-lang) wrote:
> mm... I'm still confused about this. Should we be joining only the last path
> segment or not the entire path?
> 
> I thought you'd want to do something more like a relative path from main to
css
> and append the url to that.
> 
> For example, add a field like _urlBasePath or _cssRelativePath:
> 
>   final Path _cssRelativePath =
>      _cssFilePath.directoryPath.relativeTo(_mainFilePath);
> 
> (so you can remove _pathEquals and _cssFilePath)
> 
> and then this method would simply do:
> 
>   node.text = _pathInfo.transformUrl(_mainFilePath,
>     _cssRelativePath.join(node.text).toString());
>

[Siggi Cherem (dart-lang), 2013-03-09 01:32:09]

I forgot to mention it in the last round - I like that we reverted the old tests
(to keep testing the existing functionality), but let's also add the new tests
for the css changes together with this CL

https://chromiumcodereview.appspot.com/12474002/diff/33001/lib/src/compiler.dart
File lib/src/compiler.dart (right):

https://chromiumcodereview.appspot.com/12474002/diff/33001/lib/src/compiler.d...
lib/src/compiler.dart:466: if (!fileInfo.isEntryPoint) {
On 2013/03/08 22:42:23, terry wrote:
> I'm not sure this is right.  Don't we want to support var, calc, mixin etc.
> regardless of polyfill?
> 
> On 2013/03/08 21:57:52, Siggi Cherem (dart-lang) wrote:
> > && options.processCss
> 

my understanding was that 'processCss' is not just about scroped-styles, but
about polyfilling all the futuristic features of css that are not available in
browsers today (like var, mixin, etc)

https://chromiumcodereview.appspot.com/12474002/diff/31002/lib/src/html_css_f...
File lib/src/html_css_fixup.dart (right):

https://chromiumcodereview.appspot.com/12474002/diff/31002/lib/src/html_css_f...
lib/src/html_css_fixup.dart:188: node.text =
'${_cssRelativePath.toString()}/${node.text}';
Looks great. Note that you can remove the toString(), it's done automatically by
doing '$cssRelativePath'.

Also, let's add a TODO here saying that this needs to be fixed further.

The problem is that '$path/$text' will not work in windows. That's why I was
initially suggesting we use something like append/join. Turns out this is also
broken in _rewritePackages in info.dart, so that's why now I lean towards
keeping it like you have and fixing it later when I switch web_ui to use pathos.

[terry, 2013-03-11 21:04:25]

https://chromiumcodereview.appspot.com/12474002/diff/31002/lib/src/html_css_f...
File lib/src/html_css_fixup.dart (right):

https://chromiumcodereview.appspot.com/12474002/diff/31002/lib/src/html_css_f...
lib/src/html_css_fixup.dart:188: node.text =
'${_cssRelativePath.toString()}/${node.text}';
On 2013/03/09 01:32:10, Siggi Cherem (dart-lang) wrote:
> Looks great. Note that you can remove the toString(), it's done automatically
by
> doing '$cssRelativePath'.
> 
> Also, let's add a TODO here saying that this needs to be fixed further.
> 
> The problem is that '$path/$text' will not work in windows. That's why I was
> initially suggesting we use something like append/join. Turns out this is also
> broken in _rewritePackages in info.dart, so that's why now I lean towards
> keeping it like you have and fixing it later when I switch web_ui to use
pathos.

Reworked this still wasn't quite right.

[Siggi Cherem (dart-lang), 2013-03-11 21:13:11]

Thanks Terry!

Looks great. I just have some minor nits and fixing something small in the test.

https://chromiumcodereview.appspot.com/12474002/diff/52003/lib/src/compiler.dart
File lib/src/compiler.dart (right):

https://chromiumcodereview.appspot.com/12474002/diff/52003/lib/src/compiler.d...
lib/src/compiler.dart:581: //    assert(mainFile != null);
remove commented code?

https://chromiumcodereview.appspot.com/12474002/diff/52003/lib/src/html_css_f...
File lib/src/html_css_fixup.dart (right):

https://chromiumcodereview.appspot.com/12474002/diff/52003/lib/src/html_css_f...
lib/src/html_css_fixup.dart:186: ).directoryPath;
oops - this was my fault. Can you fix the indentation/line-breaks here?

https://chromiumcodereview.appspot.com/12474002/diff/52003/test/data/input/cs...
File test/data/input/css_main_test.html (right):

https://chromiumcodereview.appspot.com/12474002/diff/52003/test/data/input/cs...
test/data/input/css_main_test.html:60: String baseDirectory =
'web_ui/test/data/input/';
remove the 'web_ui' segment. The 'web_ui' is not part of the project's
directory, so this would fail if you name your git clone directory something
else.

It seems that this test is assuming that this will be rewritten using absolute
URLs. That might be OK, but I wonder if we prefer to just check for the suffix
that is common to both relative and absolute paths? For example, maybe only
check for 'input/'? If we do that, I might consider removing the 'baseDirectory'
variable entirely (since the variable is even longer than just saying 'input/'
below)

[terry, 2013-03-11 22:09:13]

thx

https://chromiumcodereview.appspot.com/12474002/diff/52003/lib/src/html_css_f...
File lib/src/html_css_fixup.dart (right):

https://chromiumcodereview.appspot.com/12474002/diff/52003/lib/src/html_css_f...
lib/src/html_css_fixup.dart:186: ).directoryPath;
On 2013/03/11 21:13:11, Siggi Cherem (dart-lang) wrote:
> oops - this was my fault. Can you fix the indentation/line-breaks here?

Done.

https://chromiumcodereview.appspot.com/12474002/diff/52003/test/data/input/cs...
File test/data/input/css_main_test.html (right):

https://chromiumcodereview.appspot.com/12474002/diff/52003/test/data/input/cs...
test/data/input/css_main_test.html:60: String baseDirectory =
'web_ui/test/data/input/';
Okay, changed to /input/ and /input/resources/

On 2013/03/11 21:13:11, Siggi Cherem (dart-lang) wrote:
> remove the 'web_ui' segment. The 'web_ui' is not part of the project's
> directory, so this would fail if you name your git clone directory something
> else.
> 
> It seems that this test is assuming that this will be rewritten using absolute
> URLs. That might be OK, but I wonder if we prefer to just check for the suffix
> that is common to both relative and absolute paths? For example, maybe only
> check for 'input/'? If we do that, I might consider removing the
'baseDirectory'
> variable entirely (since the variable is even longer than just saying 'input/'
> below)

[Siggi Cherem (dart-lang), 2013-03-11 22:21:54]

lgtm

https://chromiumcodereview.appspot.com/12474002/diff/74001/pubspec.yaml
File pubspec.yaml (right):

https://chromiumcodereview.appspot.com/12474002/diff/74001/pubspec.yaml#newcode5
pubspec.yaml:5: version: 0.4.1+7
I'm inclined to keep the old version here. I don't think we need to publish a
new version right away (unless we have a bug fix to make). For now, I rather
keep this unreleased so we have a couple days to test it internally and release
it together with the next cut we make to fix breaking APIs.