Fixed a bug that the list still shows deleted directory.

Fixed a bug that the list still shows deleted directory.

BUG=421407
TEST=Open Files.app (WindowA); Create a directory; Open a new window of Files.app, and open the directory which is created in the previous step; Delete the directory in WindowA.

Committed: https://crrev.com/416b5284a7ab72dcf6ee673e51f29317eafae265
Cr-Commit-Position: refs/heads/master@{#299076}

[yawano, 2014-10-09 05:18:34]

yawano@chromium.org changed reviewers:
+ mtomasz@chromium.org

[yawano, 2014-10-09 05:18:34]

PTAL.

[mtomasz, 2014-10-09 07:23:47]

https://codereview.chromium.org/639273002/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/ui/directory_tree.js (right):

https://codereview.chromium.org/639273002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/directory_tree.js:228: * code will
navigate to another directory.
Please update the comment.

https://codereview.chromium.org/639273002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/directory_tree.js:233:
DirectoryItem.prototype.remove = function(child) {
Does it work for Downloads? We have a directory tree for Drive only.

https://codereview.chromium.org/639273002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/directory_tree.js:238: // If
currentDirectory is removed,
nit: Please fill the first line of comment before going to another.

[yawano, 2014-10-09 08:59:05]

PTAL. Thanks.

https://codereview.chromium.org/639273002/diff/1/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/ui/directory_tree.js (right):

https://codereview.chromium.org/639273002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/directory_tree.js:228: * code will
navigate to another directory.
On 2014/10/09 07:23:47, mtomasz wrote:
> Please update the comment.

Acknowledged.

https://codereview.chromium.org/639273002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/directory_tree.js:233:
DirectoryItem.prototype.remove = function(child) {
On 2014/10/09 07:23:47, mtomasz wrote:
> Does it work for Downloads? We have a directory tree for Drive only.

Done.

https://codereview.chromium.org/639273002/diff/1/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/ui/directory_tree.js:238: // If
currentDirectory is removed,
On 2014/10/09 07:23:47, mtomasz wrote:
> nit: Please fill the first line of comment before going to another.

Acknowledged.

[mtomasz, 2014-10-10 00:34:53]

https://codereview.chromium.org/639273002/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/directory_model.js (right):

https://codereview.chromium.org/639273002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/directory_model.js:177:
directoryEntry.getDirectory(directoryEntry.fullPath, {create: false},
Does it work? The value of directoryEntry is eg. '/Downloads/test', which got
deleted. This code looks for a directory '/Downloads/test/test'. This obviously
will fail, because /Downloads/test dir is removed.

Please check:
1. If it changes the dir to parent *only if* the dir is removed.
2. If it works for subdirectories like /Downloads/1/2, where '2' is removed.

[yawano, 2014-10-10 06:29:10]

Changed FileWatcher.onDirectoryChanged_ to fire watcher-directory-changed event
when the watched directory is deleted.
While checking the directory is deleted or not in onDirectoryChanged_ causes
double check of deletion(we also checks the folder is deleted or not in
DirectoryModel), I wanted to avoid firing unnecessary events.

https://codereview.chromium.org/639273002/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/directory_model.js (right):

https://codereview.chromium.org/639273002/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/directory_model.js:177:
directoryEntry.getDirectory(directoryEntry.fullPath, {create: false},
On 2014/10/10 00:34:52, mtomasz wrote:
> Does it work? The value of directoryEntry is eg. '/Downloads/test', which got
> deleted. This code looks for a directory '/Downloads/test/test'. This
obviously
> will fail, because /Downloads/test dir is removed.
> 
> Please check:
> 1. If it changes the dir to parent *only if* the dir is removed.
> 2. If it works for subdirectories like /Downloads/1/2, where '2' is removed.

Yes, this works.
Since this checks whether the watched directory exists or not by calling
getDirectory with the full path of watched directory.

[mtomasz, 2014-10-10 06:36:50]

https://codereview.chromium.org/639273002/diff/60001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/directory_model.js (right):

https://codereview.chromium.org/639273002/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/directory_model.js:180: var info =
this.volumeManager_.getVolumeInfo(directoryEntry);
nit: info -> volumeInfo for consistency with other places.

https://codereview.chromium.org/639273002/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/directory_model.js:180: var info =
this.volumeManager_.getVolumeInfo(directoryEntry);
getVolumeInfo may return null. Please add:
if (volumeInfo) ...

https://codereview.chromium.org/639273002/diff/60001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/file_watcher.js (right):

https://codereview.chromium.org/639273002/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/file_watcher.js:53: if
(e.changedFiles)
e.changedFiles or changedFiles in this if?

[yawano, 2014-10-10 06:54:17]

Thank you for the review. PTAL.

https://codereview.chromium.org/639273002/diff/60001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/directory_model.js (right):

https://codereview.chromium.org/639273002/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/directory_model.js:180: var info =
this.volumeManager_.getVolumeInfo(directoryEntry);
On 2014/10/10 06:36:50, mtomasz wrote:
> nit: info -> volumeInfo for consistency with other places.

Done.

https://codereview.chromium.org/639273002/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/directory_model.js:180: var info =
this.volumeManager_.getVolumeInfo(directoryEntry);
On 2014/10/10 06:36:50, mtomasz wrote:
> getVolumeInfo may return null. Please add:
> if (volumeInfo) ... 

Done.

https://codereview.chromium.org/639273002/diff/60001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/file_watcher.js (right):

https://codereview.chromium.org/639273002/diff/60001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/file_watcher.js:53: if
(e.changedFiles)
On 2014/10/10 06:36:50, mtomasz wrote:
> e.changedFiles or changedFiles in this if?

Done.

[mtomasz, 2014-10-10 07:41:19]

lgtm

[yawano, 2014-10-10 07:44:33]

The CQ bit was checked by yawano@chromium.org

[commit-bot: I haz the power, 2014-10-10 07:45:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/639273002/130001

[commit-bot: I haz the power, 2014-10-10 08:23:38]

Committed patchset #4 (id:130001)

[commit-bot: I haz the power, 2014-10-10 08:24:25]

Patchset 4 (id:??) landed as
https://crrev.com/416b5284a7ab72dcf6ee673e51f29317eafae265
Cr-Commit-Position: refs/heads/master@{#299076}