DevTools: use inotify to pick changes on the filesystem.

DevTools: use inotify to pick changes on the filesystem.

Committed: https://crrev.com/7d01b8735bba35b97f9cea3ebceaf8af769b7610
Cr-Commit-Position: refs/heads/master@{#358015}

BUG=316106

[pfeldman, 2015-11-03 23:50:05]

Description was changed from

==========
DevTools: [straw man] use inotify to pick changes on the filesystem.
==========

to

==========
DevTools: use inotify to pick changes on the filesystem.
==========

[pfeldman, 2015-11-03 23:50:14]

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

[dgozman, 2015-11-04 20:11:29]

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

https://codereview.chromium.org/1422463007/diff/40001/chrome/browser/devtools...
chrome/browser/devtools/devtools_file_watcher.cc:21: typedef
base::Callback<void(const base::FilePath&, int)> WatchCallback;
There is exactly same WatchCallback in DevToolsFileWatcher.

https://codereview.chromium.org/1422463007/diff/40001/chrome/browser/devtools...
chrome/browser/devtools/devtools_file_watcher.cc:38: typedef
std::map<base::FilePath, base::Time> FilePathTimesMap;
using

https://codereview.chromium.org/1422463007/diff/40001/chrome/browser/devtools...
chrome/browser/devtools/devtools_file_watcher.cc:73: base::FileEnumerator
enumerator(path, true, base::FileEnumerator::FILES);
How does this perform on large workspaces?

https://codereview.chromium.org/1422463007/diff/40001/chrome/browser/devtools...
chrome/browser/devtools/devtools_file_watcher.cc:99:
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
I think we'll need a batch and progress indicator in frontend, while it re-reads
file contents. Imagine you do a "gclient sync", and DevTools become
unresponsive.

https://codereview.chromium.org/1422463007/diff/40001/chrome/browser/devtools...
chrome/browser/devtools/devtools_file_watcher.cc:125:
shared_watcher_->RemoveListener(this);
Why refcounting if you remove listener in destructor?

https://codereview.chromium.org/1422463007/diff/40001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_file_watcher.h (right):

https://codereview.chromium.org/1422463007/diff/40001/chrome/browser/devtools...
chrome/browser/devtools/devtools_file_watcher.h:20: typedef
base::Callback<void(const base::FilePath&, int)> WatchCallback;
using

https://codereview.chromium.org/1422463007/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/devtools.js (right):

https://codereview.chromium.org/1422463007/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/devtools.js:179:
fileSystemFileChanged: function(path)
JSDoc please

[pfeldman, 2015-11-05 00:18:42]

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

https://codereview.chromium.org/1422463007/diff/40001/chrome/browser/devtools...
chrome/browser/devtools/devtools_file_watcher.cc:21: typedef
base::Callback<void(const base::FilePath&, int)> WatchCallback;
On 2015/11/04 20:11:29, dgozman wrote:
> There is exactly same WatchCallback in DevToolsFileWatcher.

Done.

https://codereview.chromium.org/1422463007/diff/40001/chrome/browser/devtools...
chrome/browser/devtools/devtools_file_watcher.cc:38: typedef
std::map<base::FilePath, base::Time> FilePathTimesMap;
On 2015/11/04 20:11:29, dgozman wrote:
> using

Done.

https://codereview.chromium.org/1422463007/diff/40001/chrome/browser/devtools...
chrome/browser/devtools/devtools_file_watcher.cc:73: base::FileEnumerator
enumerator(path, true, base::FileEnumerator::FILES);
On 2015/11/04 20:11:29, dgozman wrote:
> How does this perform on large workspaces?

MBP, SSD, 25K files
cold: 1532
hot: 210

All FILE thread, so seems reasonable.

https://codereview.chromium.org/1422463007/diff/40001/chrome/browser/devtools...
chrome/browser/devtools/devtools_file_watcher.cc:99:
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
On 2015/11/04 20:11:29, dgozman wrote:
> I think we'll need a batch and progress indicator in frontend, while it
re-reads
> file contents. Imagine you do a "gclient sync", and DevTools become
> unresponsive.

Let me instead
1) throttle it so that i was not re-enumerating the fs in browser
2) prevent checkContent from happening if content has not been loaded

https://codereview.chromium.org/1422463007/diff/40001/chrome/browser/devtools...
chrome/browser/devtools/devtools_file_watcher.cc:125:
shared_watcher_->RemoveListener(this);
On 2015/11/04 20:11:29, dgozman wrote:
> Why refcounting if you remove listener in destructor?

Done.

https://codereview.chromium.org/1422463007/diff/40001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_file_watcher.h (right):

https://codereview.chromium.org/1422463007/diff/40001/chrome/browser/devtools...
chrome/browser/devtools/devtools_file_watcher.h:20: typedef
base::Callback<void(const base::FilePath&, int)> WatchCallback;
On 2015/11/04 20:11:29, dgozman wrote:
> using

Done.

[dgozman, 2015-11-05 00:46:42]

https://codereview.chromium.org/1422463007/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/devtools.js (right):

https://codereview.chromium.org/1422463007/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/devtools.js:179:
fileSystemFilesChanged: function(path)
JSDoc please. Also, you pass paths instead of path now.

[dgozman, 2015-11-05 01:13:51]

lgtm

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

https://codereview.chromium.org/1422463007/diff/80001/chrome/browser/devtools...
chrome/browser/devtools/devtools_file_watcher.cc:94: watchers_.erase(path);
Should we remove subfolders of path from pending_paths_ to ensure consistency?

https://codereview.chromium.org/1422463007/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/bindings/FileSystemWorkspaceBinding.js
(right):

https://codereview.chromium.org/1422463007/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/bindings/FileSystemWorkspaceBinding.js:647:
this.dispatchEventToListeners(WebInspector.ProjectDelegate.Events.FileChanged,
path);
If you already have uiSourceCode here, why issue an event and not update right
here?

[pfeldman, 2015-11-05 01:31:06]

On 2015/11/05 01:13:51, dgozman wrote:
> lgtm
> 
>
https://codereview.chromium.org/1422463007/diff/80001/chrome/browser/devtools...
> File chrome/browser/devtools/devtools_file_watcher.cc (right):
> 
>
https://codereview.chromium.org/1422463007/diff/80001/chrome/browser/devtools...
> chrome/browser/devtools/devtools_file_watcher.cc:94: watchers_.erase(path);
> Should we remove subfolders of path from pending_paths_ to ensure consistency?
> 
>
https://codereview.chromium.org/1422463007/diff/80001/third_party/WebKit/Sour...
> File
>
third_party/WebKit/Source/devtools/front_end/bindings/FileSystemWorkspaceBinding.js
> (right):
> 
>
https://codereview.chromium.org/1422463007/diff/80001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/devtools/front_end/bindings/FileSystemWorkspaceBinding.js:647:
> this.dispatchEventToListeners(WebInspector.ProjectDelegate.Events.FileChanged,
> path);
> If you already have uiSourceCode here, why issue an event and not update right
> here?

i'll follow up separately. first is actually a larger problem of the whole
workspace system

[pfeldman, 2015-11-05 01:31:11]

The CQ bit was checked by pfeldman@chromium.org

[commit-bot: I haz the power, 2015-11-05 01:33:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1422463007/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1422463007/80001

[pfeldman, 2015-11-05 01:33:35]

The CQ bit was unchecked by pfeldman@chromium.org

[pfeldman, 2015-11-05 01:49:03]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2015-11-05 01:49:04]

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/1422463007/#ps100001
(title: "same w/ default throttle")

[commit-bot: I haz the power, 2015-11-05 01:49:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1422463007/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1422463007/100001

[commit-bot: I haz the power, 2015-11-05 02:04:04]

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

[commit-bot: I haz the power, 2015-11-05 02:04:04]

Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[pfeldman, 2015-11-05 02:51:30]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2015-11-05 02:51:30]

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/1422463007/#ps120001
(title: "same w/ gn fixed")

[commit-bot: I haz the power, 2015-11-05 02:52:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1422463007/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1422463007/120001

[commit-bot: I haz the power, 2015-11-05 02:58:00]

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

[commit-bot: I haz the power, 2015-11-05 02:58:01]

Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pfeldman, 2015-11-05 06:17:54]

The CQ bit was checked by pfeldman@chromium.org

[pfeldman, 2015-11-05 06:17:54]

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/1422463007/#ps140001
(title: "build fix")

[commit-bot: I haz the power, 2015-11-05 06:18:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1422463007/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1422463007/140001

[commit-bot: I haz the power, 2015-11-05 08:24:43]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-11-05 08:25:11]

Failed to apply patch for
third_party/WebKit/Source/devtools/front_end/bindings/WorkspaceController.js:
While running git rm
third_party/WebKit/Source/devtools/front_end/bindings/WorkspaceController.js;
  fatal: pathspec
'third_party/WebKit/Source/devtools/front_end/bindings/WorkspaceController.js'
did not match any files

Patch:  D   
third_party/WebKit/Source/devtools/front_end/bindings/WorkspaceController.js
Index:
third_party/WebKit/Source/devtools/front_end/bindings/WorkspaceController.js
diff --git
a/third_party/WebKit/Source/devtools/front_end/bindings/WorkspaceController.js
b/third_party/WebKit/Source/devtools/front_end/bindings/WorkspaceController.js
deleted file mode 100644
index
495aee650f9e57040f478c44d5abbb96725aa424..0000000000000000000000000000000000000000
---
a/third_party/WebKit/Source/devtools/front_end/bindings/WorkspaceController.js
+++ /dev/null
@@ -1,63 +0,0 @@
-/*
- * Copyright (C) 2013 Google Inc. All rights reserved.
- *
- * Redistribution and use in source and binary forms, with or without
- * modification, are permitted provided that the following conditions are
- * met:
- *
- *     * Redistributions of source code must retain the above copyright
- * notice, this list of conditions and the following disclaimer.
- *     * Redistributions in binary form must reproduce the above
- * copyright notice, this list of conditions and the following disclaimer
- * in the documentation and/or other materials provided with the
- * distribution.
- *     * Neither the name of Google Inc. nor the names of its
- * contributors may be used to endorse or promote products derived from
- * this software without specific prior written permission.
- *
- * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
- * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
- * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
- * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
- * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
- * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
- * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
- * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
- * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
- * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
- * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
- */
-
-/**
- * @constructor
- * @suppressGlobalPropertiesCheck
- */
-WebInspector.WorkspaceController = function(workspace)
-{
-    this._workspace = workspace;
-    // Only for main window.
-    window.addEventListener("focus", this._windowFocused.bind(this), false);
-    this._fileSystemRefreshThrottler = new WebInspector.Throttler(1000);
-}
-
-WebInspector.WorkspaceController.prototype = {
-    /**
-     * @param {!Event} event
-     */
-    _windowFocused: function(event)
-    {
-       
this._fileSystemRefreshThrottler.schedule(refreshFileSystems.bind(this));
-
-        /**
-         * @this {WebInspector.WorkspaceController}
-         */
-        function refreshFileSystems()
-        {
-            var barrier = new CallbackBarrier();
-            var projects = this._workspace.projects();
-            for (var i = 0; i < projects.length; ++i)
-                projects[i].refresh("/", barrier.createCallback());
-            return barrier.donePromise();
-        }
-    }
-}

[commit-bot: I haz the power, 2015-11-05 08:25:30]

Patchset 8 (id:??) landed as
https://crrev.com/7d01b8735bba35b97f9cea3ebceaf8af769b7610
Cr-Commit-Position: refs/heads/master@{#358015}

[paulirish, 2015-11-15 19:48:19]

Description was changed from

==========
DevTools: use inotify to pick changes on the filesystem.

Committed: https://crrev.com/7d01b8735bba35b97f9cea3ebceaf8af769b7610
Cr-Commit-Position: refs/heads/master@{#358015}
==========

to

==========
DevTools: use inotify to pick changes on the filesystem.

Committed: https://crrev.com/7d01b8735bba35b97f9cea3ebceaf8af769b7610
Cr-Commit-Position: refs/heads/master@{#358015}

BUG=316106
==========