DevTools: Remove files in the navigator when they are deleted externally

DevTools: Remove files in the navigator when they are deleted externally

This splits up the filesChanged event to specify whether each was added,
removed, or had content changed.

BUG=679016, 679017
Review-Url: https://codereview.chromium.org/2651043006
Cr-Commit-Position: refs/heads/master@{#448473}
Committed: https://chromium.googlesource.com/chromium/src/+/56bb5978cf646941f9df3ec22bd83c0bd5a4c403

[einbinder, 2017-01-25 01:47:12]

einbinder@chromium.org changed reviewers:
+ lushnikov@chromium.org

[einbinder, 2017-01-25 01:47:13]

ptal

[einbinder, 2017-01-25 01:47:21]

The CQ bit was checked by einbinder@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 01:48:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lushnikov, 2017-01-25 02:23:11]

!! good one !!

A few questions:
1. There's a list of files cached in the IsolatedFileSystem - it would be nice
to update it as well
2. Are the folder paths reported as well?

https://codereview.chromium.org/2651043006/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/devtools_compatibility.js
(right):

https://codereview.chromium.org/2651043006/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/devtools_compatibility.js:186:
'fileSystemFilesChanged',
[changedPaths.concat(addedPaths).concat(removedPaths)]);
nit: maybe use .pushAll to avoid multiple array copying?

https://codereview.chromium.org/2651043006/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/persistence/FileSystemWorkspaceBinding.js
(right):

https://codereview.chromium.org/2651043006/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/persistence/FileSystemWorkspaceBinding.js:155:
forEachFile.call(this, paths.changed, (path, file) => file._fileChanged(path));
file => fileSystem?

https://codereview.chromium.org/2651043006/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/persistence/FileSystemWorkspaceBinding.js:164:
function forEachFile(filePaths, fn) {
fn => callback

https://codereview.chromium.org/2651043006/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/persistence/FileSystemWorkspaceBinding.js:165:
for (var path of filePaths) {
path => filePath

https://codereview.chromium.org/2651043006/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/persistence/FileSystemWorkspaceBinding.js:166:
for (var key of this._boundFileSystems.keys()) {
key => fileSystemPath

[commit-bot: I haz the power, 2017-01-25 05:35:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 05:35:52]

Dry run: This issue passed the CQ dry run.

[einbinder, 2017-01-25 19:53:13]

The list of files in IsolatedFileSystem is only used by
FileSystemWorkspaceBinding. It seems awkward for FileSystemWorkspaceBinding to
maintain a list of files just for itself in IsolatedFileSystem. Maybe _filePaths
should be _initialFilePaths, or returned differently.

When I create or remove a folder, I get an empty notification with no file paths
specified.

I got lost trying to follow what gitFolders in IsolatedFileSystem is used for.

https://codereview.chromium.org/2651043006/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/devtools_compatibility.js
(right):

https://codereview.chromium.org/2651043006/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/devtools_compatibility.js:186:
'fileSystemFilesChanged',
[changedPaths.concat(addedPaths).concat(removedPaths)]);
On 2017/01/25 at 02:23:10, lushnikov wrote:
> nit: maybe use .pushAll to avoid multiple array copying?

I don't think devtools_compatibility should rely on platform. I'm not really
worried about the performance of the legacy path.

https://codereview.chromium.org/2651043006/diff/1/third_party/WebKit/Source/d...
File
third_party/WebKit/Source/devtools/front_end/persistence/FileSystemWorkspaceBinding.js
(right):

https://codereview.chromium.org/2651043006/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/persistence/FileSystemWorkspaceBinding.js:155:
forEachFile.call(this, paths.changed, (path, file) => file._fileChanged(path));
On 2017/01/25 at 02:23:10, lushnikov wrote:
> file => fileSystem?

Done.

https://codereview.chromium.org/2651043006/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/persistence/FileSystemWorkspaceBinding.js:164:
function forEachFile(filePaths, fn) {
On 2017/01/25 at 02:23:10, lushnikov wrote:
> fn => callback

Done.

https://codereview.chromium.org/2651043006/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/persistence/FileSystemWorkspaceBinding.js:165:
for (var path of filePaths) {
On 2017/01/25 at 02:23:10, lushnikov wrote:
> path => filePath

Done.

https://codereview.chromium.org/2651043006/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/persistence/FileSystemWorkspaceBinding.js:166:
for (var key of this._boundFileSystems.keys()) {
On 2017/01/25 at 02:23:10, lushnikov wrote:
> key => fileSystemPath

Done.

[lushnikov, 2017-01-26 01:10:45]

On 2017/01/25 19:53:13, einbinder wrote:
> The list of files in IsolatedFileSystem is only used by
> FileSystemWorkspaceBinding. It seems awkward for FileSystemWorkspaceBinding to
> maintain a list of files just for itself in IsolatedFileSystem. Maybe
_filePaths
> should be _initialFilePaths, or returned differently.

The motivation behind this is that FileSystemWorkspaceBinding has no notion of
folders, whereas IsolatedFileSystem has.
So gitFolders could be extracted only in IFS.
Also, one way to teach Navigator to show folders is to move it over to
IsolatedFileSystem

As per the patch: let's rename filePaths into initialFilePaths, and gitFolders
into initialGitFolders, this should
remove confusion.

> 
> When I create or remove a folder, I get an empty notification with no file
paths
> specified.
> 
> I got lost trying to follow what gitFolders in IsolatedFileSystem is used for.
> 
>
https://codereview.chromium.org/2651043006/diff/1/third_party/WebKit/Source/d...
> File third_party/WebKit/Source/devtools/front_end/devtools_compatibility.js
> (right):
> 
>
https://codereview.chromium.org/2651043006/diff/1/third_party/WebKit/Source/d...
> third_party/WebKit/Source/devtools/front_end/devtools_compatibility.js:186:
> 'fileSystemFilesChanged',
> [changedPaths.concat(addedPaths).concat(removedPaths)]);
> On 2017/01/25 at 02:23:10, lushnikov wrote:
> > nit: maybe use .pushAll to avoid multiple array copying?
> 
> I don't think devtools_compatibility should rely on platform. I'm not really
> worried about the performance of the legacy path.
> 
>
https://codereview.chromium.org/2651043006/diff/1/third_party/WebKit/Source/d...
> File
>
third_party/WebKit/Source/devtools/front_end/persistence/FileSystemWorkspaceBinding.js
> (right):
> 
>
https://codereview.chromium.org/2651043006/diff/1/third_party/WebKit/Source/d...
>
third_party/WebKit/Source/devtools/front_end/persistence/FileSystemWorkspaceBinding.js:155:
> forEachFile.call(this, paths.changed, (path, file) =>
file._fileChanged(path));
> On 2017/01/25 at 02:23:10, lushnikov wrote:
> > file => fileSystem?
> 
> Done.
> 
>
https://codereview.chromium.org/2651043006/diff/1/third_party/WebKit/Source/d...
>
third_party/WebKit/Source/devtools/front_end/persistence/FileSystemWorkspaceBinding.js:164:
> function forEachFile(filePaths, fn) {
> On 2017/01/25 at 02:23:10, lushnikov wrote:
> > fn => callback
> 
> Done.
> 
>
https://codereview.chromium.org/2651043006/diff/1/third_party/WebKit/Source/d...
>
third_party/WebKit/Source/devtools/front_end/persistence/FileSystemWorkspaceBinding.js:165:
> for (var path of filePaths) {
> On 2017/01/25 at 02:23:10, lushnikov wrote:
> > path => filePath
> 
> Done.
> 
>
https://codereview.chromium.org/2651043006/diff/1/third_party/WebKit/Source/d...
>
third_party/WebKit/Source/devtools/front_end/persistence/FileSystemWorkspaceBinding.js:166:
> for (var key of this._boundFileSystems.keys()) {
> On 2017/01/25 at 02:23:10, lushnikov wrote:
> > key => fileSystemPath
> 
> Done.

[lushnikov, 2017-01-26 01:10:57]

https://codereview.chromium.org/2651043006/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/workspace/IsolatedFileSystemManager.js
(right):

https://codereview.chromium.org/2651043006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/workspace/IsolatedFileSystemManager.js:171:
for (var pathType in paths)
let's write this explicitly

var changedPaths = paths.changed.map(...)
var addedPaths = ..
var removedPaths = ..

Only one line more, but so much easier to read

[einbinder, 2017-01-26 22:06:14]

Moved to initialGitFolders and initialFilePaths

https://codereview.chromium.org/2651043006/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/workspace/IsolatedFileSystemManager.js
(right):

https://codereview.chromium.org/2651043006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/workspace/IsolatedFileSystemManager.js:171:
for (var pathType in paths)
On 2017/01/26 at 01:10:57, lushnikov wrote:
> let's write this explicitly
> 
> var changedPaths = paths.changed.map(...)
> var addedPaths = ..
> var removedPaths = ..
> 
> Only one line more, but so much easier to read

Done.

[lushnikov, 2017-01-26 23:18:59]

Could you please annotate this CL with bugs it fixes? There should be at least
two

lgtm, thanks! We now need @dgozman's blessing for this

[einbinder, 2017-01-27 00:07:32]

Description was changed from

==========
DevTools: Remove files in the navigator when they are deleted externally

This splits up the filesChanged event to specify whether each was added,
removed, or had content changed.

BUG=none
==========

to

==========
DevTools: Remove files in the navigator when they are deleted externally

This splits up the filesChanged event to specify whether each was added,
removed, or had content changed.

BUG=679016, 679017
==========

[einbinder, 2017-01-27 00:07:46]

einbinder@chromium.org changed reviewers:
+ dgozman@chromium.org

[pfeldman, 2017-02-06 23:07:41]

pfeldman@chromium.org changed reviewers:
+ pfeldman@chromium.org

[pfeldman, 2017-02-06 23:07:42]

lgtm

[einbinder, 2017-02-06 23:08:12]

The CQ bit was checked by einbinder@chromium.org

[commit-bot: I haz the power, 2017-02-06 23:09:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 00:48:38]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486422492686070,
"parent_rev": "4091f2f04a14767d816857bd480088c3c62d25d8", "commit_rev":
"56bb5978cf646941f9df3ec22bd83c0bd5a4c403"}

[commit-bot: I haz the power, 2017-02-07 00:49:07]

Description was changed from

==========
DevTools: Remove files in the navigator when they are deleted externally

This splits up the filesChanged event to specify whether each was added,
removed, or had content changed.

BUG=679016, 679017
==========

to

==========
DevTools: Remove files in the navigator when they are deleted externally

This splits up the filesChanged event to specify whether each was added,
removed, or had content changed.

BUG=679016, 679017

Review-Url: https://codereview.chromium.org/2651043006
Cr-Commit-Position: refs/heads/master@{#448473}
Committed:
https://chromium.googlesource.com/chromium/src/+/56bb5978cf646941f9df3ec22bd8...
==========

[commit-bot: I haz the power, 2017-02-07 00:49:08]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/56bb5978cf646941f9df3ec22bd8...