Added working directory bar.

Added working directory bar.

This CL adds a button to the html allowing the working directory to be
changed after startup. The current working directory basename is shown
in a disabled text field next to it. Changing the working directory
persists the new entryID from chrome.fileSystem.retainEntry to local
storage.

R=tapted@chromium.org

Committed: https://github.com/tapted/bleeding_edge/commit/4058286

[tapted, 2014-01-13 07:59:39]

CL description needs some work

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

https://codereview.appspot.com/51260043/diff/1/dart/sdk/lib/_internal/pub/lib...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:56: static
Future<Directory> setWorkingDirectory([Directory dir]) {
Feels odd to have a function `setWorkingDirectory` call obtainDirectory and
prompt, and also to return a value (i.e. 'setters' should be void or return
success/failure, not return a possibly new value altogether). 

I'd maybe call the existing obtainWorkingDirectory
`getPersistedWorkingDirectory` and call this function
`persistWorkingDirectoryOrPrompt`. The control flow is still a bit confusing
though.

https://codereview.appspot.com/51260043/diff/1/dart/sdk/lib/_internal/pub/lib...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:64: String entryID =
chrome.fileSystem.retainEntry(dir.getEntry());
nit: indenting

https://codereview.appspot.com/51260043/diff/1/dart/sdk/lib/_internal/pub/pub...
File dart/sdk/lib/_internal/pub/pub.html (right):

https://codereview.appspot.com/51260043/diff/1/dart/sdk/lib/_internal/pub/pub...
dart/sdk/lib/_internal/pub/pub.html:11: <div id="working_dir_bar">
We need a solution that doesn't involve committing this to two files each time.
`packages/browser/dart.js` provides a potential solution (except we might need
to fork it to use the `precompiled` version).

Then it might need some additional copy steps in `make app`. Bonus points if it
checks if the destination copy was locally modified and breaks out of the make
step if it has.

[pajamallama, 2014-01-14 06:59:12]

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

https://codereview.appspot.com/51260043/diff/1/dart/sdk/lib/_internal/pub/lib...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:56: static
Future<Directory> setWorkingDirectory([Directory dir]) {
On 2014/01/13 07:59:40, tapted wrote:
> Feels odd to have a function `setWorkingDirectory` call obtainDirectory and
> prompt, and also to return a value (i.e. 'setters' should be void or return
> success/failure, not return a possibly new value altogether). 
> 
> I'd maybe call the existing obtainWorkingDirectory
> `getPersistedWorkingDirectory` and call this function
> `persistWorkingDirectoryOrPrompt`. The control flow is still a bit confusing
> though.

Done.

https://codereview.appspot.com/51260043/diff/1/dart/sdk/lib/_internal/pub/lib...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:64: String entryID =
chrome.fileSystem.retainEntry(dir.getEntry());
On 2014/01/13 07:59:40, tapted wrote:
> nit: indenting

Done.

https://codereview.appspot.com/51260043/diff/1/dart/sdk/lib/_internal/pub/pub...
File dart/sdk/lib/_internal/pub/pub.html (right):

https://codereview.appspot.com/51260043/diff/1/dart/sdk/lib/_internal/pub/pub...
dart/sdk/lib/_internal/pub/pub.html:11: <div id="working_dir_bar">
On 2014/01/13 07:59:40, tapted wrote:
> We need a solution that doesn't involve committing this to two files each
time.
> `packages/browser/dart.js` provides a potential solution (except we might need
> to fork it to use the `precompiled` version).
> 
> Then it might need some additional copy steps in `make app`. Bonus points if
it
> checks if the destination copy was locally modified and breaks out of the make
> step if it has.

Fixed up most of this in https://codereview.appspot.com/51210043/ 
I'll post a follow-up cl to remove the 2nd copies of pub.html, style.css and the
other duplicated.

[tapted, 2014-01-14 08:17:28]

https://codereview.appspot.com/51260043/diff/40001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/app/pub.html (right):

https://codereview.appspot.com/51260043/diff/40001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/app/pub.html:15: </div><br>
nit: You could probably put this in a <p> rather than a <div>, then drop the
<br>

https://codereview.appspot.com/51260043/diff/40001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/app/style.css (right):

https://codereview.appspot.com/51260043/diff/40001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/app/style.css:7: width: 80%;
nit: there is a CSS style guide, but I don't know it by heart

E.g. "sorting" applies too

http://google-styleguide.googlecode.com/svn/trunk/htmlcssguide.xml?showone=De...

But it's less common to get trickle-down effects with CSS nits the way you do in
code, so I'm fine with this being put off until later.

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

https://codereview.appspot.com/51260043/diff/40001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/app/pubchrome.dart:19: html.InputElement cwd;
nit: These should have leading underscores, since they're private.

https://codereview.appspot.com/51260043/diff/40001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/app/pubchrome.dart:25:
..onClick.listen(changeWorkingDirectory);
nit: I think these are indented 4 spaces. Should be consistent with line 23

https://codereview.appspot.com/51260043/diff/40001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/app/pubchrome.dart:43: ..catchError((e) =>
log.error("Could not create system cache", e))
nit: probably indent 4 spaces here

(but also not sure if you indended for these SystemCache changes to appear in
this CL.. -- you'll need to mention them in the CL description)

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

https://codereview.appspot.com/51260043/diff/40001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:37: /// return null (i.e.
if a working directory hasn't been set before).
nit: i.e. -> e.g.

https://codereview.appspot.com/51260043/diff/40001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:39: if
(chrome.storage.available) {
nit: we usually go for early-returns to avoid nesting. E.g. here you'd go with

if (!chrome.storage.available)
  return null;

(or return new Future.value(); ? although function comment says 'null' - make
sure they match)

https://codereview.appspot.com/51260043/diff/40001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:41: if (map[STORAGE_KEY] is
String) {
if (!(map[STORAGE_KEY] is String)
  return null;

[pajamallama, 2014-01-14 23:06:51]

Much cleaner

https://codereview.appspot.com/51260043/diff/40001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/app/pub.html (right):

https://codereview.appspot.com/51260043/diff/40001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/app/pub.html:15: </div><br>
On 2014/01/14 08:17:29, tapted wrote:
> nit: You could probably put this in a <p> rather than a <div>, then drop the
> <br>

Done.

https://codereview.appspot.com/51260043/diff/40001/dart/sdk/lib/_internal/pub...
File dart/sdk/lib/_internal/pub/app/style.css (right):

https://codereview.appspot.com/51260043/diff/40001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/app/style.css:7: width: 80%;
On 2014/01/14 08:17:29, tapted wrote:
> nit: there is a CSS style guide, but I don't know it by heart
> 
> E.g. "sorting" applies too
> 
>
http://google-styleguide.googlecode.com/svn/trunk/htmlcssguide.xml?showone=De...
> 
> But it's less common to get trickle-down effects with CSS nits the way you do
in
> code, so I'm fine with this being put off until later.

Sorted & removed trickle-down effect (added another class named 'log' for all
log output message).

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

https://codereview.appspot.com/51260043/diff/40001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/app/pubchrome.dart:19: html.InputElement cwd;
On 2014/01/14 08:17:29, tapted wrote:
> nit: These should have leading underscores, since they're private.

Done.

https://codereview.appspot.com/51260043/diff/40001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/app/pubchrome.dart:25:
..onClick.listen(changeWorkingDirectory);
On 2014/01/14 08:17:29, tapted wrote:
> nit: I think these are indented 4 spaces. Should be consistent with line 23

Done.

https://codereview.appspot.com/51260043/diff/40001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/app/pubchrome.dart:43: ..catchError((e) =>
log.error("Could not create system cache", e))
On 2014/01/14 08:17:29, tapted wrote:
> nit: probably indent 4 spaces here
> 
> (but also not sure if you indended for these SystemCache changes to appear in
> this CL.. -- you'll need to mention them in the CL description)

Done.

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

https://codereview.appspot.com/51260043/diff/40001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:37: /// return null (i.e.
if a working directory hasn't been set before).
On 2014/01/14 08:17:29, tapted wrote:
> nit: i.e. -> e.g.

Done.

https://codereview.appspot.com/51260043/diff/40001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:39: if
(chrome.storage.available) {
On 2014/01/14 08:17:29, tapted wrote:
> nit: we usually go for early-returns to avoid nesting. E.g. here you'd go with
> 
> if (!chrome.storage.available)
>   return null;
> 
> (or return new Future.value(); ? although function comment says 'null' - make
> sure they match)

Done.

https://codereview.appspot.com/51260043/diff/40001/dart/sdk/lib/_internal/pub...
dart/sdk/lib/_internal/pub/lib/src/wrap/io_wrap.dart:41: if (map[STORAGE_KEY] is
String) {
On 2014/01/14 08:17:29, tapted wrote:
> if (!(map[STORAGE_KEY] is String)
>   return null;

Done.

[tapted, 2014-01-14 23:31:04]

lgtm, except for the commit description. Needs to say stuff like

This CL adds a button to the html allowing the working directory to be changed
after startup. The current working directory basename is shown in a disabled
text field next to it. Changing the working directory persists the new entryID
from chrome.fileSystem.retainEntry to local storage.

[pajamallama, 2014-01-14 23:59:17]

Committed patchset #6 manually as r4058286 (presubmit successful).