Refactoring IO classes, async & offline storage

Refactoring IO classes, async & offline storage

Created classes File, Directory and FileSystem in wrap/io_wrap.dart which mirror chrome.FileEntry, chrome.DirectoryEntry and chrome.FileSystem. Using
io.dart in Dartium should be simple as only io_wrap.dart
would need to be changed.

Created a PathRep class which is basically a String with
some helper methods from the path library. This class
has been propogated throughout the existing code replacing
many Strings.

Rewrote io.dart to supply asynchronous functions. This
change propogated out to entrypoint.dart and
source/hosted.dart.

Renamed wrap/blawrap.dart to wrap/bla_wrap.dart. I wouldn't
mind reducing it to simply wrap/bla.dart...

Stored the working directory in offline storage, so that
now the user is only prompted for a directory on the
first opening of the app.

Symlink code was commented out as I don't know how to
approach them at all.

R=tapted@chromium.org

Committed: https://github.com/tapted/bleeding_edge/commit/65d21e3

[pajamallama, 2013-12-24 05:22:50]

Merry Christmas haha

[tapted, 2013-12-28 05:42:14]

Awesome work!

On 2013/12/24 05:22:50, pajamallama wrote:
> Renamed wrap/blawrap.dart to wrap/bla_wrap.dart. I wouldn't
> mind reducing it to simply wrap/bla.dart... 

I'm down with that. One cool thing to do would be to separate the files into
things like:

libwrap/io.dart  // wraps dart:io
pkgwrap/path.dart  // wraps package:path

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/lib/src/entrypoint.dart (right):

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/entrypoint.dart:79: // TODO(pajamallama):
Uncomment this.
I would perhaps try renaming the function e.g. to `getNative` and provide your
own `get` that returns null -- I think that will be enough to hide it from
dart2js for now, and might even make sense when trying to merge upstream.

Failing that, pretty sure Dart supports /* .. */ block comments, which will help
keep the VC history sane.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/entrypoint.dart:135: useLatest: useLatest,
upgradeAll: upgradeAll))
nit: indenting isn't quite right here. I think this will be most readable with
each function argument on its own line, like

    return loadLockFile().then((lockFile) =>
        resolveVersions(cache.sources,
                        root,
                        lockFile: lockFile,
                        useLatest: useLatest,
                        upgradeAll: upgradeAll))
        .then((result)

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/entrypoint.dart:142: numChanged =
result.showReport(showAll: useLatest != null || upgradeAll);
nit: this needs to be re-wrapped to fit 80 chars -- probably break after the
opening `(`

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/entrypoint.dart:145: return
cleanDir(packagesDir).then((_) =>
Since cleanDir returns a future, I think you can just `return
cleanDir(packagesDir);`, and move the `.then` out to line up with
`.then((result)` and avoid a level of nesting

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/entrypoint.dart:287: void _linkSelf() {
Same deal - if it's too hard to import a fake/wrapped `createPackageSymlink`
implementation, it would be better to call this something like `_linkSelfNative`
and provide a new, dummy implementation.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/entrypoint.dart:313: return
Future.forEach(dirs, _linkSecondaryPackageDirsRecursively);
nice!

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/entrypoint.dart:335: // Recursively lists the
contents of [dir], excluding hidden `.DS_Store`
nit: Needs triple '///' since it's a function comment

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/lib/src/io.dart (right):

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:45: return
Future.wait([dirExists(path), fileExists(path), linkExists(path)]).then(
nit: needs to be wrapped. Maybe on the list elements, like

  return Future.wait([dirExists(path),
                      fileExists(path),
                      linkExists(path)]).then((results) {
    return results.any((elem) => elem);
  });

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:75: // TODO(pajamallama): deal with
symlinks later
canonicalizeNative(..), or (less preferred) block /* .. */ comment

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:158: // TODO(pajamallama): deal with
symlinks later
resolveLinkNative

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:168: /// Reads the contents of the
text file [file].
nit: comment needs to be updated: [file] -> at [path]. Or (for all of these) you
could just leave the argument as a `file` to suggest that it shouldn't be a
directory that gets passed.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:172: /// Reads the contents of the
binary file [file].
nit: [file] -> at [path]

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:182: 
nit: remove stray blank line

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:183: /// Creates [file] and writes
[contents] to it.
file -> path

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:197: /// Creates [file] and writes
[contents] to it.
file -> path

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:203: /// Writes [stream] to a new
file at path [file]. Will replace any file already
`at path [file]` -> `at [path]`

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:233: /// Creates a directory [dir].
dir -> path, or maybe just leave the argument called `dir` since it suggests it
should only take paths that are directories

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:241: /// Creates a temp directory in
[dir], whose name will be [prefix] with
dir/base

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:246: var name = prefix + (new
Random()).nextInt(10000).toString();
We should request a temporary HTML5 filesystem for temp folders (ideally). This
approach is OK for now, but maybe a `TODO(..): more robust tempdir creation.`

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:266: {bool recursive: false,
nit: might need to be indented an extra space? Unless you found a function that
already indents like this.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:307: /// Returns whether [dir] exists
on the file system. This will return `true` for
dir/path

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:333: // TODO(pajamallama): Do proper
error handling.
Maybe .catchError(..) abort()/log()? .. i.e. "something" rather than a silent
failure might be nice.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:348: Future createSymlink(PathRep
target, PathRep symlink,
createSymlinkNative, and a dummy function

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/lib/src/path_rep.dart (right):

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/path_rep.dart:5: class PathRep {
nit: needs a class comment. The public functions should be commented too, but I
guess that can wait.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/path_rep.dart:6: 
nit: there's not usually a blank line before the first member

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/path_rep.dart:15: bool get inPackages =>
path.split(_filename).contains("packages");
Not sure this makes sense as a getter -- feels more like an inferred status
rather than something that makes up what the object "is"

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/path_rep.dart:21: 
nit: usually no blank line after the last member either.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/lib/src/sdk.dart (right):

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/sdk.dart:35: // var revisionPath =
path.join(rootDirectory, "version");
Maybe put these lines in a `readVersionFile` function that isn't called here

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/lib/src/source/hosted.dart (right):

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/source/hosted.dart:95: 
nit: remove stray blank line

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart (right):

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:5: //import 'dart:html'
show ArrayBuffer;
nit: maybe just delete this line

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:12: class FileSystem {
nit: class comment

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:13: 
nit: remove blank line

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:29: // Wait until
chrome.storage check has finished
nit: aim to wrap comments closer to 80 chars

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:32: // Check if a working
directory has already been
nit: wrapping

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:34: if
(chrome.storage.available)
nit: if an `if` condition or body has more than a line each, we give it curly
braces

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:36: if (map != null)
nit: curlies for multi-line if

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:38: (entry) => _workingDir
= new Directory(entry));
nit: indent 4 spaces

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:42: if (dir != null) {
nit: doesn't need curlies

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:59: 
nit: remove extra blank line

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:61: class FileSystemEntity
{
class comment

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:62: 
nit: remove blank line

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:84: 
nit: remove extra blank line

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:97: 
nit: blank line -> class comment

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:127: class Directory
extends FileSystemEntity {
nit: class comment. remove next blank line

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:140: {Directory parent,
nit: indenting

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:164: return
_entry.createDirectory(dirname).then((entry) => new Directory(entry));
nit: wrapping

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:176: // TODO(pajamallama):
Test whether .readEntries is recursive or not.
nit: align comment with function

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:207: 
nit: blank line

[tapted, 2013-12-30 04:41:26]

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/lib/src/path_rep.dart (right):

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/path_rep.dart:15: bool get inPackages =>
path.split(_filename).contains("packages");
On 2013/12/28 05:42:14, tapted wrote:
> Not sure this makes sense as a getter -- feels more like an inferred status
> rather than something that makes up what the object "is"

Of course... if you're just copying this from the implementation in dart:io, it
should match! But a general comment to add to this file would be something to
that effect. E.g. "unless otherwise specified, class methods are identical to
the ones in dart:io", and then ensure that ones that *are* different are
commented separately.

[pajamallama, 2013-12-30 06:35:22]

Style fixes.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/lib/src/entrypoint.dart (right):

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/entrypoint.dart:135: useLatest: useLatest,
upgradeAll: upgradeAll))
On 2013/12/28 05:42:14, tapted wrote:
> nit: indenting isn't quite right here. I think this will be most readable with
> each function argument on its own line, like
> 
>     return loadLockFile().then((lockFile) =>
>         resolveVersions(cache.sources,
>                         root,
>                         lockFile: lockFile,
>                         useLatest: useLatest,
>                         upgradeAll: upgradeAll))
>         .then((result)

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/entrypoint.dart:142: numChanged =
result.showReport(showAll: useLatest != null || upgradeAll);
On 2013/12/28 05:42:14, tapted wrote:
> nit: this needs to be re-wrapped to fit 80 chars -- probably break after the
> opening `(`

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/entrypoint.dart:335: // Recursively lists the
contents of [dir], excluding hidden `.DS_Store`
On 2013/12/28 05:42:14, tapted wrote:
> nit: Needs triple '///' since it's a function comment

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/lib/src/io.dart (right):

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:45: return
Future.wait([dirExists(path), fileExists(path), linkExists(path)]).then(
On 2013/12/28 05:42:14, tapted wrote:
> nit: needs to be wrapped. Maybe on the list elements, like
> 
>   return Future.wait([dirExists(path),
>                       fileExists(path),
>                       linkExists(path)]).then((results) {
>     return results.any((elem) => elem);
>   });

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:168: /// Reads the contents of the
text file [file].
On 2013/12/28 05:42:14, tapted wrote:
> nit: comment needs to be updated: [file] -> at [path]. Or (for all of these)
you
> could just leave the argument as a `file` to suggest that it shouldn't be a
> directory that gets passed.

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:172: /// Reads the contents of the
binary file [file].
On 2013/12/28 05:42:14, tapted wrote:
> nit: [file] -> at [path]

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:182: 
On 2013/12/28 05:42:14, tapted wrote:
> nit: remove stray blank line

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:183: /// Creates [file] and writes
[contents] to it.
On 2013/12/28 05:42:14, tapted wrote:
> file -> path

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:203: /// Writes [stream] to a new
file at path [file]. Will replace any file already
On 2013/12/28 05:42:14, tapted wrote:
> `at path [file]` -> `at [path]`

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:233: /// Creates a directory [dir].
On 2013/12/28 05:42:14, tapted wrote:
> dir -> path, or maybe just leave the argument called `dir` since it suggests
it
> should only take paths that are directories

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:241: /// Creates a temp directory in
[dir], whose name will be [prefix] with
On 2013/12/28 05:42:14, tapted wrote:
> dir/base

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:246: var name = prefix + (new
Random()).nextInt(10000).toString();
On 2013/12/28 05:42:14, tapted wrote:
> We should request a temporary HTML5 filesystem for temp folders (ideally).
This
> approach is OK for now, but maybe a `TODO(..): more robust tempdir creation.`

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:266: {bool recursive: false,
On 2013/12/28 05:42:14, tapted wrote:
> nit: might need to be indented an extra space? Unless you found a function
that
> already indents like this.

Nope, I was just guessing what to do. Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:307: /// Returns whether [dir] exists
on the file system. This will return `true` for
On 2013/12/28 05:42:14, tapted wrote:
> dir/path

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:333: // TODO(pajamallama): Do proper
error handling.
On 2013/12/28 05:42:14, tapted wrote:
> Maybe .catchError(..) abort()/log()? .. i.e. "something" rather than a silent
> failure might be nice.

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/lib/src/path_rep.dart (right):

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/path_rep.dart:6: 
On 2013/12/28 05:42:14, tapted wrote:
> nit: there's not usually a blank line before the first member

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/path_rep.dart:21: 
On 2013/12/28 05:42:14, tapted wrote:
> nit: usually no blank line after the last member either.

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/lib/src/source/hosted.dart (right):

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/source/hosted.dart:95: 
On 2013/12/28 05:42:14, tapted wrote:
> nit: remove stray blank line

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart (right):

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:5: //import 'dart:html'
show ArrayBuffer;
On 2013/12/28 05:42:14, tapted wrote:
> nit: maybe just delete this line

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:13: 
On 2013/12/28 05:42:14, tapted wrote:
> nit: remove blank line

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:29: // Wait until
chrome.storage check has finished
On 2013/12/28 05:42:14, tapted wrote:
> nit: aim to wrap comments closer to 80 chars

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:32: // Check if a working
directory has already been
On 2013/12/28 05:42:14, tapted wrote:
> nit: wrapping

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:34: if
(chrome.storage.available)
On 2013/12/28 05:42:14, tapted wrote:
> nit: if an `if` condition or body has more than a line each, we give it curly
> braces

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:36: if (map != null)
On 2013/12/28 05:42:14, tapted wrote:
> nit: curlies for multi-line if

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:38: (entry) => _workingDir
= new Directory(entry));
On 2013/12/28 05:42:14, tapted wrote:
> nit: indent 4 spaces

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:42: if (dir != null) {
On 2013/12/28 05:42:14, tapted wrote:
> nit: doesn't need curlies

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:59: 
On 2013/12/28 05:42:14, tapted wrote:
> nit: remove extra blank line

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:62: 
On 2013/12/28 05:42:14, tapted wrote:
> nit: remove blank line

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:84: 
On 2013/12/28 05:42:14, tapted wrote:
> nit: remove extra blank line

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:140: {Directory parent,
On 2013/12/28 05:42:14, tapted wrote:
> nit: indenting

Done??

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:164: return
_entry.createDirectory(dirname).then((entry) => new Directory(entry));
On 2013/12/28 05:42:14, tapted wrote:
> nit: wrapping

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:176: // TODO(pajamallama):
Test whether .readEntries is recursive or not.
On 2013/12/28 05:42:14, tapted wrote:
> nit: align comment with function

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:207: 
On 2013/12/28 05:42:14, tapted wrote:
> nit: blank line

Done.

[pajamallama, 2014-01-02 05:03:38]

Fixed some bugs, added some comments. Also uncommented sections and re-added
deleted functions. Still have the symlink stuff remaining since I need to make a
plan about how to approach that.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/lib/src/entrypoint.dart (right):

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/entrypoint.dart:79: // TODO(pajamallama):
Uncomment this.
On 2013/12/28 05:42:14, tapted wrote:
> I would perhaps try renaming the function e.g. to `getNative` and provide your
> own `get` that returns null -- I think that will be enough to hide it from
> dart2js for now, and might even make sense when trying to merge upstream.
> 
> Failing that, pretty sure Dart supports /* .. */ block comments, which will
help
> keep the VC history sane.

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/entrypoint.dart:145: return
cleanDir(packagesDir).then((_) =>
On 2013/12/28 05:42:14, tapted wrote:
> Since cleanDir returns a future, I think you can just `return
> cleanDir(packagesDir);`, and move the `.then` out to line up with
> `.then((result)` and avoid a level of nesting

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/entrypoint.dart:313: return
Future.forEach(dirs, _linkSecondaryPackageDirsRecursively);
On 2013/12/28 05:42:14, tapted wrote:
> nice!

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/lib/src/io.dart (right):

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:197: /// Creates [file] and writes
[contents] to it.
On 2013/12/28 05:42:14, tapted wrote:
> file -> path

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/lib/src/path_rep.dart (right):

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/path_rep.dart:5: class PathRep {
On 2013/12/28 05:42:14, tapted wrote:
> nit: needs a class comment. The public functions should be commented too, but
I
> guess that can wait.

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/path_rep.dart:15: bool get inPackages =>
path.split(_filename).contains("packages");
On 2013/12/28 05:42:14, tapted wrote:
> Not sure this makes sense as a getter -- feels more like an inferred status
> rather than something that makes up what the object "is"

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/lib/src/sdk.dart (right):

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/sdk.dart:35: // var revisionPath =
path.join(rootDirectory, "version");
On 2013/12/28 05:42:14, tapted wrote:
> Maybe put these lines in a `readVersionFile` function that isn't called here

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart (right):

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:12: class FileSystem {
On 2013/12/28 05:42:14, tapted wrote:
> nit: class comment

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:61: class FileSystemEntity
{
On 2013/12/28 05:42:14, tapted wrote:
> class comment

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:97: 
On 2013/12/28 05:42:14, tapted wrote:
> nit: blank line -> class comment

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:127: class Directory
extends FileSystemEntity {
On 2013/12/28 05:42:14, tapted wrote:
> nit: class comment. remove next blank line

Done.

[tapted, 2014-01-02 12:06:42]

There's a lot of added functionality from the first patchset (maybe ~100 lines).
We don't normally do that unless it's requested by a reviewer -- makes following
the code in review really tricky. This CL is also pretty large to start with -
usually you'd be asked to split it up into separate CLs, each with a clear task.

I'm keen to get this one committed, but the large blocks of commented code still
need to come out of the diff before it's something that can just be fixed-up in
a cleanup patch post-commit.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/lib/src/io.dart (right):

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:183: /// Creates [file] and writes
[contents] to it.
On 2013/12/30 06:35:23, pajamallama wrote:
> On 2013/12/28 05:42:14, tapted wrote:
> > file -> path
> 
> Done.

No it's not :p

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart (right):

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:140: {Directory parent,
On 2013/12/30 06:35:23, pajamallama wrote:
> On 2013/12/28 05:42:14, tapted wrote:
> > nit: indenting
> 
> Done?? 

Doesn't matter any more - the map argument is gone.

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
File dart/sdk/lib/_internal/pub/lib/src/entrypoint.dart (right):

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/entrypoint.dart:229: "Your pubspec has
changed, so we need to update your lockfile:");
nit: wrapping. I'd probably do

        return lockFileExists.then((exists) {
          log.message(exists ?
              "Your pubspec has changed, so we need to update your lockfile:" :
              "You don't have a lockfile, so we need to generate that:");
          return false;
        });

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
File dart/sdk/lib/_internal/pub/lib/src/io.dart (right):

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/io.dart:78: // var seen = new Set<String>();
These line comments still need to go. More elsewhere.

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/io.dart:172: File.load(file).then((f) =>
f.readText());
nit: google style guide doesn't allow abbreviations like `f`, more below. Maybe
file, filePath, file2, ... or see what strategies are used elsewhere around
dart's codebase

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/io.dart:251: }).then((d) {
more abbreviations here. `dir` is OK since it's an accepted abbreviation for
`directory`, but generally only 'i' and 'j' get to be single-letters. Maybe 'd'
for a double.

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
File dart/sdk/lib/_internal/pub/lib/src/source.dart (right):

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/source.dart:193: if (exists)
nit: this is indented two spaces too many, since it's opening a scope.

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
File dart/sdk/lib/_internal/pub/lib/src/wrap/archive_wrap.dart (right):

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/wrap/archive_wrap.dart:3: /// A generic
archive File. In CLI this will use .tar.gz files, inside
nit: `, i` -> `. I`

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/wrap/archive_wrap.dart:6: 
nit: remove blank line.

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/wrap/archive_wrap.dart:9: throw new
UnimplementedError("Archive extraction not implemented yet.");
indenting/wrapping is off.

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
File dart/sdk/lib/_internal/pub/lib/src/wrap/http_wrap.dart (right):

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/wrap/http_wrap.dart:40: {Map<String, String>
headers,
nit: indent one more space

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
File dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart (right):

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:50: if (dir != null) return
dir;
btw - two lines is still OK ;). In C++/Chrome it would probably mandated to two
lines, but Dart goes both ways.

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:76: bool get isFile =>
_entry.isFile;
nit: some stray indents crept in here

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:82: return
FileSystem.workingDir._getFile(path).then((entry) =>
functions with leading underscores are "private" and shouldn't be called outside
that class. E.g. I should be able to see the implementation of `_getFile`
nearby, or it should be made public by removing the leading underscore.

[pajamallama, 2014-01-04 03:05:24]

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/lib/src/entrypoint.dart (right):

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/entrypoint.dart:287: void _linkSelf() {
On 2013/12/28 05:42:14, tapted wrote:
> Same deal - if it's too hard to import a fake/wrapped `createPackageSymlink`
> implementation, it would be better to call this something like
`_linkSelfNative`
> and provide a new, dummy implementation.

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/lib/src/io.dart (right):

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:75: // TODO(pajamallama): deal with
symlinks later
On 2013/12/28 05:42:14, tapted wrote:
> canonicalizeNative(..), or (less preferred) block /* .. */ comment

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:158: // TODO(pajamallama): deal with
symlinks later
On 2013/12/28 05:42:14, tapted wrote:
> resolveLinkNative

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:183: /// Creates [file] and writes
[contents] to it.
On 2014/01/02 12:06:43, tapted wrote:
> On 2013/12/30 06:35:23, pajamallama wrote:
> > On 2013/12/28 05:42:14, tapted wrote:
> > > file -> path
> > 
> > Done.
> 
> No it's not :p

Done (for real)

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/io.dart:348: Future createSymlink(PathRep
target, PathRep symlink,
On 2013/12/28 05:42:14, tapted wrote:
> createSymlinkNative, and a dummy function

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/lib/src/path_rep.dart (right):

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/path_rep.dart:15: bool get inPackages =>
path.split(_filename).contains("packages");
On 2014/01/02 05:03:39, pajamallama wrote:
> On 2013/12/28 05:42:14, tapted wrote:
> > Not sure this makes sense as a getter -- feels more like an inferred status
> > rather than something that makes up what the object "is"
> 
> Done.

Done.

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart (right):

https://codereview.appspot.com/45210043/diff/20001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:140: {Directory parent,
On 2014/01/02 12:06:43, tapted wrote:
> On 2013/12/30 06:35:23, pajamallama wrote:
> > On 2013/12/28 05:42:14, tapted wrote:
> > > nit: indenting
> > 
> > Done?? 
> 
> Doesn't matter any more - the map argument is gone.

Done.

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
File dart/sdk/lib/_internal/pub/lib/src/entrypoint.dart (right):

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/entrypoint.dart:229: "Your pubspec has
changed, so we need to update your lockfile:");
On 2014/01/02 12:06:43, tapted wrote:
> nit: wrapping. I'd probably do
> 
>         return lockFileExists.then((exists) {
>           log.message(exists ?
>               "Your pubspec has changed, so we need to update your lockfile:"
:
>               "You don't have a lockfile, so we need to generate that:");
>           return false;
>         });

Done.

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
File dart/sdk/lib/_internal/pub/lib/src/io.dart (right):

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/io.dart:78: // var seen = new Set<String>();
On 2014/01/02 12:06:43, tapted wrote:
> These line comments still need to go. More elsewhere.

Done.

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/io.dart:172: File.load(file).then((f) =>
f.readText());
On 2014/01/02 12:06:43, tapted wrote:
> nit: google style guide doesn't allow abbreviations like `f`, more below.
Maybe
> file, filePath, file2, ... or see what strategies are used elsewhere around
> dart's codebase

Done.

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/io.dart:251: }).then((d) {
On 2014/01/02 12:06:43, tapted wrote:
> more abbreviations here. `dir` is OK since it's an accepted abbreviation for
> `directory`, but generally only 'i' and 'j' get to be single-letters. Maybe
'd'
> for a double.

Done.

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
File dart/sdk/lib/_internal/pub/lib/src/source.dart (right):

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/source.dart:193: if (exists)
On 2014/01/02 12:06:43, tapted wrote:
> nit: this is indented two spaces too many, since it's opening a scope.

Done.

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
File dart/sdk/lib/_internal/pub/lib/src/wrap/archive_wrap.dart (right):

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/wrap/archive_wrap.dart:3: /// A generic
archive File. In CLI this will use .tar.gz files, inside
On 2014/01/02 12:06:43, tapted wrote:
> nit: `, i` -> `. I`

Done.

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/wrap/archive_wrap.dart:6: 
On 2014/01/02 12:06:43, tapted wrote:
> nit: remove blank line.

Done.

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/wrap/archive_wrap.dart:9: throw new
UnimplementedError("Archive extraction not implemented yet.");
On 2014/01/02 12:06:43, tapted wrote:
> indenting/wrapping is off.

Ah, new file so it used tabs instead of spaces. Done.

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
File dart/sdk/lib/_internal/pub/lib/src/wrap/http_wrap.dart (right):

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/wrap/http_wrap.dart:40: {Map<String, String>
headers,
On 2014/01/02 12:06:43, tapted wrote:
> nit: indent one more space

Done.

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
File dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart (right):

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:50: if (dir != null) return
dir;
On 2014/01/02 12:06:43, tapted wrote:
> btw - two lines is still OK ;). In C++/Chrome it would probably mandated to
two
> lines, but Dart goes both ways.

Yeah, the Dart style guide has most examples of (admittedly small) one-line if
statements written this way so thought I better follow along. Can easily change
if needed.

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:76: bool get isFile =>
_entry.isFile;
On 2014/01/02 12:06:43, tapted wrote:
> nit: some stray indents crept in here

Done.

https://codereview.appspot.com/45210043/diff/100001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:82: return
FileSystem.workingDir._getFile(path).then((entry) =>
On 2014/01/02 12:06:43, tapted wrote:
> functions with leading underscores are "private" and shouldn't be called
outside
> that class. E.g. I should be able to see the implementation of `_getFile`
> nearby, or it should be made public by removing the leading underscore.

Done.

[tapted, 2014-01-04 11:24:21]

lgtm with a few nits fixed.

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
File dart/sdk/lib/_internal/pub/lib/src/app/pubchrome.dart (right):

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/app/pubchrome.dart:9: import '../io.dart'
show FileSystem;
nit: sort alphabetically/lexicographically -- `i` before `p`

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/app/pubchrome.dart:25: var workingDir;
maybe not something to fix now, but I've noticed this pattern appearing in a few
places. "Pattern" = "using an uninitialized local variable to communicate data
between the first `.then` to a subsequent `.then`". I think it would be nicer to
instead enclose the data being passed in the `return`ed Future (possibly of a
new type, or a Pair) but haven't been commenting on it because I haven't used
Dart that much.

If there is precedent for this pattern elsewhere in Dart code you've seen, it's
probably fine. But it breaks a few coupling/cohesion guidelines from more
traditional programming, so make sure you're considering alternatives.

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
File dart/sdk/lib/_internal/pub/lib/src/entrypoint.dart (right):

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/entrypoint.dart:97: print("$id dowloaded,
create package symlink(${id.name}, ${pkg.dir}, $packageDir");
nit: fix wrapping to 80 chars

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/entrypoint.dart:99: });
nit: outdent 2 spaces

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/entrypoint.dart:109: _pendingGets[id] =
future;
nit: no need to delete the blank line after this (assumption is usually "those
before me knew what they were doing").

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/entrypoint.dart:128: bool upgradeAll: false})
{
nit: indent an extra space

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
File dart/sdk/lib/_internal/pub/lib/src/io.dart (right):

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/io.dart:23: import 'wrap/http_wrap.dart' show
ByteStream;
nit: alphabetise

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/io.dart:26: export 'wrap/http_wrap.dart' show
ByteStream;
nit: alphabetise

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/io.dart:74: return canonicalizeNative(path);
doh - I meant, you could probably keep the non-working definition by having it
in a function called cononicalizeNative, which isn't called, thereby preventing
dart2js from trying to compile it. But, this is actually better in the long term
:)

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/io.dart:96: {bool relative: false}) {
nit: indent arguments at the same min-indent like

 Future createSymlink(PathRep target, PathRep symlink,
                      {bool relative: false}) {

or

 Future createSymlink(
     PathRep target, PathRep symlink, {bool relative: false}) {

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/io.dart:132: return
File.create(file).then((file) => file..write(contents));
nit: does it still need the `..`?

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
File dart/sdk/lib/_internal/pub/lib/src/source.dart (right):

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/source.dart:89: if (id.source != name)
nit: keep the old curly braces here, since we're Dart n00bs

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
File dart/sdk/lib/_internal/pub/lib/src/wrap/http_wrap.dart (right):

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/wrap/http_wrap.dart:41: String responseType:
"text"}) {
nit: indent one more space, since it's in the arg dictionary part

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
File dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart (right):

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:285: {bool relative:
false}) {
nit: indent args at same indent

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
File dart/sdk/lib/_internal/pub/lib/src/wrap/system_cache_wrap.dart (right):

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/wrap/system_cache_wrap.dart:26:
SystemCache(this.rootDir) :
nit: in C++ we get a warning for `:` at the end of a line (when it's not a
ternary if-else), since it suggests a `label`. Initializers usually have their
`:` at the start of the line, indented. But tapted+Dart=??, so just make sure
it's consistent with the surrounding code.

[pajamallama, 2014-01-05 23:05:34]

Finished.

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
File dart/sdk/lib/_internal/pub/lib/src/app/pubchrome.dart (right):

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/app/pubchrome.dart:9: import '../io.dart'
show FileSystem;
On 2014/01/04 11:24:21, tapted wrote:
> nit: sort alphabetically/lexicographically -- `i` before `p`

Done.

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
File dart/sdk/lib/_internal/pub/lib/src/entrypoint.dart (right):

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/entrypoint.dart:97: print("$id dowloaded,
create package symlink(${id.name}, ${pkg.dir}, $packageDir");
On 2014/01/04 11:24:21, tapted wrote:
> nit: fix wrapping to 80 chars

Done.

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/entrypoint.dart:99: });
On 2014/01/04 11:24:21, tapted wrote:
> nit: outdent 2 spaces

Done.

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/entrypoint.dart:109: _pendingGets[id] =
future;
On 2014/01/04 11:24:21, tapted wrote:
> nit: no need to delete the blank line after this (assumption is usually "those
> before me knew what they were doing").

Done.

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/entrypoint.dart:128: bool upgradeAll: false})
{
On 2014/01/04 11:24:21, tapted wrote:
> nit: indent an extra space

Done.

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
File dart/sdk/lib/_internal/pub/lib/src/io.dart (right):

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/io.dart:23: import 'wrap/http_wrap.dart' show
ByteStream;
On 2014/01/04 11:24:21, tapted wrote:
> nit: alphabetise

Done.

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/io.dart:26: export 'wrap/http_wrap.dart' show
ByteStream;
On 2014/01/04 11:24:21, tapted wrote:
> nit: alphabetise

Done.

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/io.dart:96: {bool relative: false}) {
On 2014/01/04 11:24:21, tapted wrote:
> nit: indent arguments at the same min-indent like
> 
>  Future createSymlink(PathRep target, PathRep symlink,
>                       {bool relative: false}) {
> 
> or
> 
>  Future createSymlink(
>      PathRep target, PathRep symlink, {bool relative: false}) {

Done.

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/io.dart:132: return
File.create(file).then((file) => file..write(contents));
On 2014/01/04 11:24:21, tapted wrote:
> nit: does it still need the `..`?

Yes, since the interface wanted a File to be returned in the Future so I'm just
leaving that in (if it did file.write then a bool would be returned, which is
also fine by me).

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
File dart/sdk/lib/_internal/pub/lib/src/source.dart (right):

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/source.dart:89: if (id.source != name)
On 2014/01/04 11:24:21, tapted wrote:
> nit: keep the old curly braces here, since we're Dart n00bs

Done.

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
File dart/sdk/lib/_internal/pub/lib/src/wrap/http_wrap.dart (right):

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/wrap/http_wrap.dart:41: String responseType:
"text"}) {
On 2014/01/04 11:24:21, tapted wrote:
> nit: indent one more space, since it's in the arg dictionary part

Done.

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
File dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart (right):

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:285: {bool relative:
false}) {
On 2014/01/04 11:24:21, tapted wrote:
> nit: indent args at same indent

Done.

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
File dart/sdk/lib/_internal/pub/lib/src/wrap/system_cache_wrap.dart (right):

https://codereview.appspot.com/45210043/diff/220001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/wrap/system_cache_wrap.dart:26:
SystemCache(this.rootDir) :
On 2014/01/04 11:24:21, tapted wrote:
> nit: in C++ we get a warning for `:` at the end of a line (when it's not a
> ternary if-else), since it suggests a `label`. Initializers usually have their
> `:` at the start of the line, indented. But tapted+Dart=??, so just make sure
> it's consistent with the surrounding code.

Done.

[tapted, 2014-01-05 23:19:10]

https://codereview.appspot.com/45210043/diff/240001/dart/sdk/lib/_internal/pu...
File dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart (right):

https://codereview.appspot.com/45210043/diff/240001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:6: import 'dart:html' show
Blob;
nit: sort

https://codereview.appspot.com/45210043/diff/240001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:80: return
File.load(path).catchError((e) => Directory.load(path));
This looks kinda like it's using exceptions for control flow -- can we encode or
query information about whether it is a directory or file somewhere in PathRep,
and call the right version proactively?

a new member function PathRep::asFileSystemEnity() might be an alternative

https://codereview.appspot.com/45210043/diff/240001/dart/sdk/lib/_internal/pu...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:95:
/*chrome.ChromeFileEntry*/ var _entry;
nit: preferable would be:

var _entry;  // A chrome.ChromeFileEntry, or a ...

i.e. something to explain why it needs to be a `var`, since that's uncommon for
data members

[pajamallama, 2014-01-06 00:54:13]

Committed patchset #8 manually as r65d21e3.