DevTools: improve DevTools file watcher to send added and removed paths

DevTools: improve DevTools file watcher to send added and removed paths

Currently, DevTools file watcher reports both added and changed paths
as "changed" and doesn't report removed paths.

This patch improves DevTools file watcher so that it starts reporting
changed, added and removed paths.

BUG=none
R=dgozman, pfeldman

Committed: https://crrev.com/2eaab0b3ed3ccdb072e3f60a0ebcf0298841d01d
Cr-Commit-Position: refs/heads/master@{#422838}

[lushnikov, 2016-10-04 00:00:33]

please, take a look.

I also need an advise on backwards-compatibility of the event. Should I instead
of adding parameters to the existing event, introduce two new events?

[lushnikov, 2016-10-04 00:00:38]

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

[commit-bot: I haz the power, 2016-10-04 00:01:27]

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

[dgozman, 2016-10-04 00:17:02]

https://codereview.chromium.org/2384343002/diff/1/chrome/browser/devtools/dev...
File chrome/browser/devtools/devtools_file_watcher.cc (right):

https://codereview.chromium.org/2384343002/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/devtools_file_watcher.cc:44: FilePathTimesMap&
file_path_times);
Out parameters must be passed by pointer.

https://codereview.chromium.org/2384343002/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/devtools_file_watcher.cc:141: iterator !=
file_path_times_.end();) {
Looks to be expensive to check all files on any single update.

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

https://codereview.chromium.org/2384343002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/devtools.js:197:
fileSystemFilesChanged: function(changedPaths, addedPaths, removedPaths)
Let's add JSDoc.

https://codereview.chromium.org/2384343002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/devtools.js:199:
this._dispatchOnInspectorFrontendAPI("fileSystemFilesChanged", [changedPaths,
addedPaths, removedPaths]);
I'd rename the method to fileSystemPathsChanged to avoid confusion with old one,
and add the following code to this place. Everything else should work with new
method name.

// Support for legacy (<M55) frontends.
var api = window["InspectorFrontendAPI"];
if (api["fileSystemPathsChanged"])
    api["fileSystemPathsChanged"].call(api, changedPaths, addedPaths,
removedPaths)
else
    api["fileSystemFilesChanged"].call(api, <concat all paths>);

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

https://codereview.chromium.org/2384343002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/workspace/IsolatedFileSystemManager.js:37:
WebInspector.Object.call(this);
Whoa!

https://codereview.chromium.org/2384343002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/workspace/IsolatedFileSystemManager.js:207:
var paths = changedPaths.concat(addedPaths, removedPaths);
Let's use it in the same patch?

[lushnikov, 2016-10-04 01:04:19]

So now this patch starts sending removed file paths among changed files.

https://codereview.chromium.org/2384343002/diff/1/chrome/browser/devtools/dev...
File chrome/browser/devtools/devtools_file_watcher.cc (right):

https://codereview.chromium.org/2384343002/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/devtools_file_watcher.cc:44: FilePathTimesMap&
file_path_times);
On 2016/10/04 00:17:02, dgozman wrote:
> Out parameters must be passed by pointer.

Done.

https://codereview.chromium.org/2384343002/diff/1/chrome/browser/devtools/dev...
chrome/browser/devtools/devtools_file_watcher.cc:141: iterator !=
file_path_times_.end();) {
On 2016/10/04 00:17:02, dgozman wrote:
> Looks to be expensive to check all files on any single update.

This method is actually throttled, so should be fine (see
DevToolsFileWatcher::SharedFileWatcher::DirectoryChanged)

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

https://codereview.chromium.org/2384343002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/devtools.js:197:
fileSystemFilesChanged: function(changedPaths, addedPaths, removedPaths)
On 2016/10/04 00:17:02, dgozman wrote:
> Let's add JSDoc.

Done.

https://codereview.chromium.org/2384343002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/devtools.js:199:
this._dispatchOnInspectorFrontendAPI("fileSystemFilesChanged", [changedPaths,
addedPaths, removedPaths]);
On 2016/10/04 00:17:02, dgozman wrote:
> I'd rename the method to fileSystemPathsChanged to avoid confusion with old
one,
> and add the following code to this place. Everything else should work with new
> method name.
> 
> // Support for legacy (<M55) frontends.
> var api = window["InspectorFrontendAPI"];
> if (api["fileSystemPathsChanged"])
>     api["fileSystemPathsChanged"].call(api, changedPaths, addedPaths,
> removedPaths)
> else
>     api["fileSystemFilesChanged"].call(api, <concat all paths>);
> 

Nice, thanks!

As per offline discussion, I've refrained from changing API for now altogether.

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

https://codereview.chromium.org/2384343002/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/workspace/IsolatedFileSystemManager.js:207:
var paths = changedPaths.concat(addedPaths, removedPaths);
On 2016/10/04 00:17:02, dgozman wrote:
> Let's use it in the same patch?

Reverted this.

[lushnikov, 2016-10-04 01:04:26]

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

[commit-bot: I haz the power, 2016-10-04 01:05:10]

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

[lushnikov, 2016-10-04 01:43:27]

please, take another look

[lushnikov, 2016-10-04 01:43:32]

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

[commit-bot: I haz the power, 2016-10-04 01:43:58]

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

[dgozman, 2016-10-04 02:26:41]

https://codereview.chromium.org/2384343002/diff/60001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_file_watcher.cc (right):

https://codereview.chromium.org/2384343002/diff/60001/chrome/browser/devtools...
chrome/browser/devtools/devtools_file_watcher.cc:91: GetModificationTimes(path,
&times_map);
One line, avoids copying:

GetModificationTimes(path, &file_path_times_[path])

https://codereview.chromium.org/2384343002/diff/60001/chrome/browser/devtools...
chrome/browser/devtools/devtools_file_watcher.cc:146: for (auto it =
current_times.begin(); it != current_times.end(); ++it) {
for (const auto& pair : current_times)

https://codereview.chromium.org/2384343002/diff/60001/chrome/browser/devtools...
chrome/browser/devtools/devtools_file_watcher.cc:156: for (auto it =
old_times.begin(); it != old_times.end();) {
ditto

https://codereview.chromium.org/2384343002/diff/60001/chrome/browser/devtools...
chrome/browser/devtools/devtools_file_watcher.cc:160: old_times.erase(it++);
Don't do this. Just |old_times.swap(current_times)| at the end.

https://codereview.chromium.org/2384343002/diff/60001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_ui_bindings.cc (right):

https://codereview.chromium.org/2384343002/diff/60001/chrome/browser/devtools...
chrome/browser/devtools/devtools_ui_bindings.cc:991:
CallClientFunction("DevToolsAPI.fileSystemFilesChanged", &changed, &added,
Let's pass single parameter here and not change devtools.js.

[lushnikov, 2016-10-04 02:51:59]

https://codereview.chromium.org/2384343002/diff/60001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_file_watcher.cc (right):

https://codereview.chromium.org/2384343002/diff/60001/chrome/browser/devtools...
chrome/browser/devtools/devtools_file_watcher.cc:91: GetModificationTimes(path,
&times_map);
On 2016/10/04 02:26:41, dgozman wrote:
> One line, avoids copying:
> 
> GetModificationTimes(path, &file_path_times_[path])

Done.

https://codereview.chromium.org/2384343002/diff/60001/chrome/browser/devtools...
chrome/browser/devtools/devtools_file_watcher.cc:146: for (auto it =
current_times.begin(); it != current_times.end(); ++it) {
On 2016/10/04 02:26:41, dgozman wrote:
> for (const auto& pair : current_times)

Done.

https://codereview.chromium.org/2384343002/diff/60001/chrome/browser/devtools...
chrome/browser/devtools/devtools_file_watcher.cc:156: for (auto it =
old_times.begin(); it != old_times.end();) {
On 2016/10/04 02:26:41, dgozman wrote:
> ditto

Done.

https://codereview.chromium.org/2384343002/diff/60001/chrome/browser/devtools...
chrome/browser/devtools/devtools_file_watcher.cc:160: old_times.erase(it++);
On 2016/10/04 02:26:41, dgozman wrote:
> Don't do this. Just |old_times.swap(current_times)| at the end.

Done.

https://codereview.chromium.org/2384343002/diff/60001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_ui_bindings.cc (right):

https://codereview.chromium.org/2384343002/diff/60001/chrome/browser/devtools...
chrome/browser/devtools/devtools_ui_bindings.cc:991:
CallClientFunction("DevToolsAPI.fileSystemFilesChanged", &changed, &added,
On 2016/10/04 02:26:41, dgozman wrote:
> Let's pass single parameter here and not change devtools.js.

Done.

[lushnikov, 2016-10-04 02:52:57]

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

[commit-bot: I haz the power, 2016-10-04 02:53:23]

Dry run: 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, 2016-10-04 04:35:53]

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

[commit-bot: I haz the power, 2016-10-04 04:35:54]

Dry run: This issue passed the CQ dry run.

[dgozman, 2016-10-04 04:41:01]

lgtm

https://codereview.chromium.org/2384343002/diff/80001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_file_watcher.cc (right):

https://codereview.chromium.org/2384343002/diff/80001/chrome/browser/devtools...
chrome/browser/devtools/devtools_file_watcher.cc:144: for (const auto& it :
current_times) {
I usually call it |pair| or |path_time|.

https://codereview.chromium.org/2384343002/diff/80001/chrome/browser/devtools...
chrome/browser/devtools/devtools_file_watcher.cc:147: old_times[it.first] =
it.second;
Don't modify, you do swap below.

https://codereview.chromium.org/2384343002/diff/80001/chrome/browser/devtools...
chrome/browser/devtools/devtools_file_watcher.cc:150: old_timestamp->second =
it.second;
ditto

https://codereview.chromium.org/2384343002/diff/80001/chrome/browser/devtools...
chrome/browser/devtools/devtools_file_watcher.cc:154: for (const auto& it :
old_times) {
I'd rather do:

for (const auto& path_time : old_times) {
  base::FilePath& path = path_time.first;
  if (current_times.find(path) == current_times.end())
    removed_paths.push_back(path.AsUTF8Unsafe());
}

[lushnikov, 2016-10-04 15:56:35]

https://codereview.chromium.org/2384343002/diff/80001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_file_watcher.cc (right):

https://codereview.chromium.org/2384343002/diff/80001/chrome/browser/devtools...
chrome/browser/devtools/devtools_file_watcher.cc:144: for (const auto& it :
current_times) {
On 2016/10/04 04:41:01, dgozman wrote:
> I usually call it |pair| or |path_time|.

|path_time| is really nice, thanks

https://codereview.chromium.org/2384343002/diff/80001/chrome/browser/devtools...
chrome/browser/devtools/devtools_file_watcher.cc:147: old_times[it.first] =
it.second;
On 2016/10/04 04:41:01, dgozman wrote:
> Don't modify, you do swap below.

My bad. Done.

https://codereview.chromium.org/2384343002/diff/80001/chrome/browser/devtools...
chrome/browser/devtools/devtools_file_watcher.cc:150: old_timestamp->second =
it.second;
On 2016/10/04 04:41:01, dgozman wrote:
> ditto

Done.

https://codereview.chromium.org/2384343002/diff/80001/chrome/browser/devtools...
chrome/browser/devtools/devtools_file_watcher.cc:154: for (const auto& it :
old_times) {
On 2016/10/04 04:41:01, dgozman wrote:
> I'd rather do:
> 
> for (const auto& path_time : old_times) {
>   base::FilePath& path = path_time.first;
>   if (current_times.find(path) == current_times.end())
>     removed_paths.push_back(path.AsUTF8Unsafe());
> }

This is much more clear, thanks

[lushnikov, 2016-10-04 15:56:43]

The CQ bit was checked by lushnikov@chromium.org

[lushnikov, 2016-10-04 15:56:43]

The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/2384343002/#ps100001
(title: "honing skills.")

[commit-bot: I haz the power, 2016-10-04 15:57:00]

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, 2016-10-04 17:18:17]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-10-04 17:22:00]

Description was changed from

==========
DevTools: improve DevTools file watcher to send added and removed paths

Currently, DevTools file watcher reports both added and changed paths
as "changed" and doesn't report removed paths.

This patch improves DevTools file watcher so that it starts reporting
changed, added and removed paths.

BUG=none
R=dgozman, pfeldman
==========

to

==========
DevTools: improve DevTools file watcher to send added and removed paths

Currently, DevTools file watcher reports both added and changed paths
as "changed" and doesn't report removed paths.

This patch improves DevTools file watcher so that it starts reporting
changed, added and removed paths.

BUG=none
R=dgozman, pfeldman

Committed: https://crrev.com/2eaab0b3ed3ccdb072e3f60a0ebcf0298841d01d
Cr-Commit-Position: refs/heads/master@{#422838}
==========

[commit-bot: I haz the power, 2016-10-04 17:22:01]

Patchset 6 (id:??) landed as
https://crrev.com/2eaab0b3ed3ccdb072e3f60a0ebcf0298841d01d
Cr-Commit-Position: refs/heads/master@{#422838}