Clean up various ways we find paths in pub.

Clean up various ways we find paths in pub.

This makes it so that it's never necessary to pass in DART_SDK.

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

[Bob Nystrom, 2013-04-23 22:46:42]

https://codereview.chromium.org/14449004/diff/3001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/utils.dart (right):

https://codereview.chromium.org/14449004/diff/3001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/utils.dart:366: String libraryPath(Symbol
libraryName) =>
How about making this just take a string and wrap it in a Symbol here?

https://codereview.chromium.org/14449004/diff/3001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/utils.dart:367:
fileUriToPath(currentMirrorSystem().libraries[libraryName].uri);
Are you sure we want to rely on mirrors for this?

https://codereview.chromium.org/14449004/diff/3001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/utils.dart:375: return uri.path.replaceFirst("/",
"").replaceAll("/", "\\");
Document that the first replace is for "C:/" -> "C:".

Does this work with network paths?

[nweiz, 2013-04-23 23:14:03]

cc-ing Andrei because this also changes fileUriToPath and pathToFileUri in
dartdoc.

https://codereview.chromium.org/14449004/diff/3001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/utils.dart (right):

https://codereview.chromium.org/14449004/diff/3001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/utils.dart:366: String libraryPath(Symbol
libraryName) =>
On 2013/04/23 22:46:42, Bob Nystrom wrote:
> How about making this just take a string and wrap it in a Symbol here?

I thought constructing a symbol with a literal string told the VM that it just
needed to keep track of that symbol. That seems pretty relevant if we're moving
to snapshotting. I'm not 100% confident that that's accurate, though.

https://codereview.chromium.org/14449004/diff/3001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/utils.dart:367:
fileUriToPath(currentMirrorSystem().libraries[libraryName].uri);
On 2013/04/23 22:46:42, Bob Nystrom wrote:
> Are you sure we want to rely on mirrors for this?

They seem way less hacky than basing everything on Options().script. They'll
also make it possible in the future for other apps to import pub libraries.

https://codereview.chromium.org/14449004/diff/3001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/utils.dart:375: return uri.path.replaceFirst("/",
"").replaceAll("/", "\\");
On 2013/04/23 22:46:42, Bob Nystrom wrote:
> Document that the first replace is for "C:/" -> "C:".

Done.

> Does this work with network paths?

No. I'll fix that.

[Bob Nystrom, 2013-04-23 23:32:43]

LGTM.

https://codereview.chromium.org/14449004/diff/3001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/utils.dart (right):

https://codereview.chromium.org/14449004/diff/3001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/utils.dart:366: String libraryPath(Symbol
libraryName) =>
On 2013/04/23 23:14:03, nweiz wrote:
> On 2013/04/23 22:46:42, Bob Nystrom wrote:
> > How about making this just take a string and wrap it in a Symbol here?
> 
> I thought constructing a symbol with a literal string told the VM that it just
> needed to keep track of that symbol. That seems pretty relevant if we're
moving
> to snapshotting. I'm not 100% confident that that's accurate, though.

I don't think it matters. I believe this stuff is only relevant for dart2js
where they want to minify names. On the VM, we should be fine.

https://codereview.chromium.org/14449004/diff/3001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/utils.dart:367:
fileUriToPath(currentMirrorSystem().libraries[libraryName].uri);
On 2013/04/23 23:14:03, nweiz wrote:
> On 2013/04/23 22:46:42, Bob Nystrom wrote:
> > Are you sure we want to rely on mirrors for this?
> 
> They seem way less hacky than basing everything on Options().script. They'll
> also make it possible in the future for other apps to import pub libraries.

Well, alright, but I'm never excited about taking on a new dependency on a
still-being-developed "dart:" library.

[Andrei Mouravski, 2013-04-23 23:54:15]

lgtm

1 comment.

https://codereview.chromium.org/14449004/diff/7001/sdk/lib/_internal/dartdoc/...
File sdk/lib/_internal/dartdoc/lib/src/dartdoc/utils.dart (right):

https://codereview.chromium.org/14449004/diff/7001/sdk/lib/_internal/dartdoc/...
sdk/lib/_internal/dartdoc/lib/src/dartdoc/utils.dart:115: /// Converts a `file:`
[Uri] to a local path string.
Please put this back to the original comment style to match the rest of this
file.

[nweiz, 2013-04-23 23:55:36]

https://codereview.chromium.org/14449004/diff/3001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/utils.dart (right):

https://codereview.chromium.org/14449004/diff/3001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/utils.dart:366: String libraryPath(Symbol
libraryName) =>
On 2013/04/23 23:32:43, Bob Nystrom wrote:
> On 2013/04/23 23:14:03, nweiz wrote:
> > On 2013/04/23 22:46:42, Bob Nystrom wrote:
> > > How about making this just take a string and wrap it in a Symbol here?
> > 
> > I thought constructing a symbol with a literal string told the VM that it
just
> > needed to keep track of that symbol. That seems pretty relevant if we're
> moving
> > to snapshotting. I'm not 100% confident that that's accurate, though.
> 
> I don't think it matters. I believe this stuff is only relevant for dart2js
> where they want to minify names. On the VM, we should be fine.

Done.

https://codereview.chromium.org/14449004/diff/7001/sdk/lib/_internal/dartdoc/...
File sdk/lib/_internal/dartdoc/lib/src/dartdoc/utils.dart (right):

https://codereview.chromium.org/14449004/diff/7001/sdk/lib/_internal/dartdoc/...
sdk/lib/_internal/dartdoc/lib/src/dartdoc/utils.dart:115: /// Converts a `file:`
[Uri] to a local path string.
On 2013/04/23 23:54:15, Andrei Mouravski wrote:
> Please put this back to the original comment style to match the rest of this
> file.

Done.

[nweiz, 2013-04-23 23:55:54]

Committed patchset #4 manually as r21914 (presubmit successful).