fix imports link rel=stylesheet

fix imports link rel=stylesheet
fixes https://code.google.com/p/dart/issues/detail?id=16648

also:
* adds a few DOM APIs to html5lib that it was missing
* removes a broken test from html5lib
* fixes polymer's dependency on analyzer

R=sigmund@google.com

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

[Jennifer Messerly, 2014-02-12 23:35:50]

This fixes https://code.google.com/p/dart/issues/detail?id=16648

[Siggi Cherem (dart-lang), 2014-02-13 00:08:16]

lgtm!

Very nice.

https://codereview.chromium.org/162093002/diff/110001/pkg/polymer/lib/src/bui...
File pkg/polymer/lib/src/build/common.dart (right):

https://codereview.chromium.org/162093002/diff/110001/pkg/polymer/lib/src/bui...
pkg/polymer/lib/src/build/common.dart:115: Future<String> readAsString(AssetId
id, Transform transform) {
is this method needed? seems that the first few values are not used, so this is
just doing the same as:
  transform.readInputAsString(id)

https://codereview.chromium.org/162093002/diff/110001/pkg/polymer/lib/src/bui...
pkg/polymer/lib/src/build/common.dart:156: }
we might want to resolve more precisely paths that start with '/' but not a
different domain. For now I think this is fine, in the future I think we will
want to treat

/packages/foo/bar

similar to 

packages/foo/bar

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

https://codereview.chromium.org/162093002/diff/110001/pkg/polymer/lib/src/bui...
pkg/polymer/lib/src/build/import_inliner.dart:81: (id.package == 'polymer' &&
id.path == 'lib/init.html')) return null;
80 :-(

https://codereview.chromium.org/162093002/diff/110001/pkg/polymer/lib/src/bui...
pkg/polymer/lib/src/build/import_inliner.dart:106: Future
_inlineStylesheet(AssetId id, Element link) =>
the formatting mixing => + then below is a bit odd, I might prefer switching to
the normal method form.

https://codereview.chromium.org/162093002/diff/110001/pkg/polymer/lib/src/loa...
File pkg/polymer/lib/src/loader.dart (right):

https://codereview.chromium.org/162093002/diff/110001/pkg/polymer/lib/src/loa...
pkg/polymer/lib/src/loader.dart:92: bool _useDirtyChecking = true;
we could delete this flag and use _deployMode for this purpose too

https://codereview.chromium.org/162093002/diff/110001/pkg/polymer/lib/src/loa...
pkg/polymer/lib/src/loader.dart:95: bool _deployMode = false;
FYI - I intend to later on make this a `const` and have a transformer that
replaces it. That way deploy mode is easier to optimize by dart2js.

[Jennifer Messerly, 2014-02-13 06:11:22]

thanks Siggi, PTAL

https://codereview.chromium.org/162093002/diff/110001/pkg/polymer/lib/src/bui...
File pkg/polymer/lib/src/build/common.dart (right):

https://codereview.chromium.org/162093002/diff/110001/pkg/polymer/lib/src/bui...
pkg/polymer/lib/src/build/common.dart:115: Future<String> readAsString(AssetId
id, Transform transform) {
On 2014/02/13 00:08:18, Siggi Cherem (dart-lang) wrote:
> is this method needed? seems that the first few values are not used, so this
is
> just doing the same as:
>   transform.readInputAsString(id)

good catch. Out of curiosity, what is the  `var url = samePackage ? id.path ...`
business for? Should I be using it when I construct a source span -- since it
seems to be passed to the _parseHtml function?

It seems like a lot of complexity compared to just doing an id.path, just to
improve the error message... Does id.path not point at the correct path for a
user to understand a warning?

anyway I moved the URL computation into the place where I'm building the source
spans, so it matches the HTML code path as closely as possible.

https://codereview.chromium.org/162093002/diff/110001/pkg/polymer/lib/src/bui...
pkg/polymer/lib/src/build/common.dart:156: }
On 2014/02/13 00:08:18, Siggi Cherem (dart-lang) wrote:
> we might want to resolve more precisely paths that start with '/' but not a
> different domain. For now I think this is fine, in the future I think we will
> want to treat
> 
> /packages/foo/bar
> 
> similar to 
> 
> packages/foo/bar

yeah agree

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

https://codereview.chromium.org/162093002/diff/110001/pkg/polymer/lib/src/bui...
pkg/polymer/lib/src/build/import_inliner.dart:106: Future
_inlineStylesheet(AssetId id, Element link) =>
On 2014/02/13 00:08:18, Siggi Cherem (dart-lang) wrote:
> the formatting mixing => + then below is a bit odd, I might prefer switching
to
> the normal method form.

okay, I'll change this one. But, I'm think we should continue experimenting. I'm
desperate to avoid the plague of  increasingly nested return { return { return
... I find that totally unreadable. And sometimes it takes a while to get used
to a different style, even if it ultimately ends up better :)

https://codereview.chromium.org/162093002/diff/110001/pkg/polymer/lib/src/loa...
File pkg/polymer/lib/src/loader.dart (right):

https://codereview.chromium.org/162093002/diff/110001/pkg/polymer/lib/src/loa...
pkg/polymer/lib/src/loader.dart:95: bool _deployMode = false;
On 2014/02/13 00:08:18, Siggi Cherem (dart-lang) wrote:
> FYI - I intend to later on make this a `const` and have a transformer that
> replaces it. That way deploy mode is easier to optimize by dart2js.

SGTM

[Jennifer Messerly, 2014-02-13 06:12:52]

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

https://codereview.chromium.org/162093002/diff/110001/pkg/polymer/lib/src/bui...
pkg/polymer/lib/src/build/import_inliner.dart:81: (id.package == 'polymer' &&
id.path == 'lib/init.html')) return null;
On 2014/02/13 00:08:18, Siggi Cherem (dart-lang) wrote:
> 80 :-(

oops, missed this. fixed.

out of curiosity, why the special case for lib/init.html?

[Siggi Cherem (dart-lang), 2014-02-13 17:46:32]

https://codereview.chromium.org/162093002/diff/110001/pkg/polymer/lib/src/bui...
File pkg/polymer/lib/src/build/common.dart (right):

https://codereview.chromium.org/162093002/diff/110001/pkg/polymer/lib/src/bui...
pkg/polymer/lib/src/build/common.dart:115: Future<String> readAsString(AssetId
id, Transform transform) {
On 2014/02/13 06:11:22, John Messerly wrote:
> On 2014/02/13 00:08:18, Siggi Cherem (dart-lang) wrote:
> > is this method needed? seems that the first few values are not used, so this
> is
> > just doing the same as:
> >   transform.readInputAsString(id)
> 
> good catch. Out of curiosity, what is the  `var url = samePackage ? id.path
...`
> business for? 

It's basically for better error messages.

> Should I be using it when I construct a source span -- since it
> seems to be passed to the _parseHtml function?

Exactly. 

> 
> It seems like a lot of complexity compared to just doing an id.path, just to
> improve the error message... Does id.path not point at the correct path for a
> user to understand a warning?

Well, only if you are in the same package. 'id.path' is a relative path from a
package's home dir, so if you have 'lib/foo/bar.dart' in 2 packages, their
id.path is the same.

The basic principle I followed was:
 - if it's on the same package, then use the path (id.path), the user will know
that a path means a file in their own code
 - if it's on another package, then use [assetUrlFor], this will likely be a
"package:" url to the file in the other package, which is enough for users to
identify where the error is.

Unfortunately, [assetUrlFor] doesn't work for the same package because we have
files in 'web' and other folders that don't have a "package:" url. [assetUrlFor]
will produce a relative URL instead.

> 
> anyway I moved the URL computation into the place where I'm building the
source
> spans, so it matches the HTML code path as closely as possible.

Cool

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

https://codereview.chromium.org/162093002/diff/110001/pkg/polymer/lib/src/bui...
pkg/polymer/lib/src/build/import_inliner.dart:81: (id.package == 'polymer' &&
id.path == 'lib/init.html')) return null;
On 2014/02/13 06:12:52, John Messerly wrote:
> On 2014/02/13 00:08:18, Siggi Cherem (dart-lang) wrote:
> > 80 :-(
> 
> oops, missed this. fixed.
> 
> out of curiosity, why the special case for lib/init.html?

Oh, this was from some old bootstrap we used to have. It's doesn't even exist
anymore. We should just delete this.

https://codereview.chromium.org/162093002/diff/320001/pkg/polymer/lib/src/bui...
File pkg/polymer/lib/src/build/common.dart (right):

https://codereview.chromium.org/162093002/diff/320001/pkg/polymer/lib/src/bui...
pkg/polymer/lib/src/build/common.dart:106: bool samePackage = id.package ==
primaryId.package;
use getAssetUrl here too

https://codereview.chromium.org/162093002/diff/320001/pkg/polymer/lib/src/bui...
pkg/polymer/lib/src/build/common.dart:115: String getAssetUrl(AssetId id,
Transform transform) {
I find it a bit confusing to have both getAssetUrl and assetUrlFor.

How about renaming this to something else. Some ideas:
 - spanUrlFor
 - getUrlForErrorMessages
 - packageAbsoluteUrlFor

Let's also add a small dartdoc for it.

[Jennifer Messerly, 2014-02-13 20:43:45]

PTAL!

https://codereview.chromium.org/162093002/diff/110001/pkg/polymer/lib/src/bui...
File pkg/polymer/lib/src/build/common.dart (right):

https://codereview.chromium.org/162093002/diff/110001/pkg/polymer/lib/src/bui...
pkg/polymer/lib/src/build/common.dart:115: Future<String> readAsString(AssetId
id, Transform transform) {
On 2014/02/13 17:46:33, Siggi Cherem (dart-lang) wrote:
> On 2014/02/13 06:11:22, John Messerly wrote:
> > On 2014/02/13 00:08:18, Siggi Cherem (dart-lang) wrote:
> > > is this method needed? seems that the first few values are not used, so
this
> > is
> > > just doing the same as:
> > >   transform.readInputAsString(id)
> > 
> > good catch. Out of curiosity, what is the  `var url = samePackage ? id.path
> ...`
> > business for? 
> 
> It's basically for better error messages.
> 
> > Should I be using it when I construct a source span -- since it
> > seems to be passed to the _parseHtml function?
> 
> Exactly. 
> 
> > 
> > It seems like a lot of complexity compared to just doing an id.path, just to
> > improve the error message... Does id.path not point at the correct path for
a
> > user to understand a warning?
> 
> Well, only if you are in the same package. 'id.path' is a relative path from a
> package's home dir, so if you have 'lib/foo/bar.dart' in 2 packages, their
> id.path is the same.
> 
> The basic principle I followed was:
>  - if it's on the same package, then use the path (id.path), the user will
know
> that a path means a file in their own code
>  - if it's on another package, then use [assetUrlFor], this will likely be a
> "package:" url to the file in the other package, which is enough for users to
> identify where the error is.
> 
> Unfortunately, [assetUrlFor] doesn't work for the same package because we have
> files in 'web' and other folders that don't have a "package:" url.
[assetUrlFor]
> will produce a relative URL instead.
> 
> > 
> > anyway I moved the URL computation into the place where I'm building the
> source
> > spans, so it matches the HTML code path as closely as possible.
> 
> Cool

Lovely explanation! Thank you :)

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

https://codereview.chromium.org/162093002/diff/110001/pkg/polymer/lib/src/bui...
pkg/polymer/lib/src/build/import_inliner.dart:81: (id.package == 'polymer' &&
id.path == 'lib/init.html')) return null;
On 2014/02/13 17:46:33, Siggi Cherem (dart-lang) wrote:
> On 2014/02/13 06:12:52, John Messerly wrote:
> > On 2014/02/13 00:08:18, Siggi Cherem (dart-lang) wrote:
> > > 80 :-(
> > 
> > oops, missed this. fixed.
> > 
> > out of curiosity, why the special case for lib/init.html?
> 
> Oh, this was from some old bootstrap we used to have. It's doesn't even exist
> anymore. We should just delete this.

awesome. done!

https://codereview.chromium.org/162093002/diff/320001/pkg/polymer/lib/src/bui...
File pkg/polymer/lib/src/build/common.dart (right):

https://codereview.chromium.org/162093002/diff/320001/pkg/polymer/lib/src/bui...
pkg/polymer/lib/src/build/common.dart:106: bool samePackage = id.package ==
primaryId.package;
On 2014/02/13 17:46:33, Siggi Cherem (dart-lang) wrote:
> use getAssetUrl here too

Done.

https://codereview.chromium.org/162093002/diff/320001/pkg/polymer/lib/src/bui...
pkg/polymer/lib/src/build/common.dart:115: String getAssetUrl(AssetId id,
Transform transform) {
On 2014/02/13 17:46:33, Siggi Cherem (dart-lang) wrote:
> I find it a bit confusing to have both getAssetUrl and assetUrlFor.
> 
> How about renaming this to something else. Some ideas:
>  - spanUrlFor
>  - getUrlForErrorMessages
>  - packageAbsoluteUrlFor
> 
> Let's also add a small dartdoc for it.

done! based Dartdoc on your extremely helpful explanation! :)

[Siggi Cherem (dart-lang), 2014-02-13 23:11:35]

:) awesome, thanks John!

LGTM

[Jennifer Messerly, 2014-02-13 23:23:31]

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