[ew] Add basic classes.

[ew] Add basic classes.

This patch adds a EntryWatcherService, which is a bridge between extensions and
fileapi. Also, WatcherManager interface has been created to let backends
implement their own watching logic.

Note, that EntryWatcherService is not wired up to File System API yet. Also,
a lot of features are missing.

TBR=noamsml, jcivelli
TEST=unit_tests: *EntryWatcherService*
BUG=261491
Committed: https://crrev.com/b75244fdf1d75e67485649e6a96a69c50676bbc3
Cr-Commit-Position: refs/heads/master@{#292327}

[mtomasz, 2014-08-12 03:14:58]

@benwells: PTAL at */file_system/*.
@kinaba: PTAL at /c/b/chromeos/fileapi/*, *drive*, *mtp*.
@tzik: PTAL at *webkit*, *fileapi*.

Thanks. Let me know if you have any questions.

[mtomasz, 2014-08-12 04:49:58]

On 2014/08/12 03:14:58, mtomasz wrote:
> @benwells: PTAL at */file_system/*.
> @kinaba: PTAL at /c/b/chromeos/fileapi/*, *drive*, *mtp*.
> @tzik: PTAL at *webkit*, *fileapi*.
> 
> Thanks. Let me know if you have any questions.

This document may be helpful in understanding the approach:
https://docs.google.com/a/google.com/document/d/1phS6zdQNr24wTcaBSE3DEBKimqww...

[kinaba, 2014-08-12 09:15:37]

Stamp LGTM for files I own.

https://codereview.chromium.org/452043003/diff/40001/webkit/browser/fileapi/w...
File webkit/browser/fileapi/watcher_manager.h (right):

https://codereview.chromium.org/452043003/diff/40001/webkit/browser/fileapi/w...
webkit/browser/fileapi/watcher_manager.h:58: // already watched, or setting up
the watcher fails, then |callback|
I have some questions on the "if already watched => error" case.

Is the restriction also posed on JS-level user code? If so, it sounds a bit
inconvenient that you cannot set two ore more listeners on a single entry.

Does it have to be checked on each WatcherManager instance? What about rejecting
such a case in EntryWatcherService and allow WatcherManager to assume there's no
duplication?

[mtomasz, 2014-08-12 09:31:37]

https://codereview.chromium.org/452043003/diff/40001/webkit/browser/fileapi/w...
File webkit/browser/fileapi/watcher_manager.h (right):

https://codereview.chromium.org/452043003/diff/40001/webkit/browser/fileapi/w...
webkit/browser/fileapi/watcher_manager.h:58: // already watched, or setting up
the watcher fails, then |callback|
On 2014/08/12 09:15:37, kinaba wrote:
> I have some questions on the "if already watched => error" case.
> 
> Is the restriction also posed on JS-level user code? If so, it sounds a bit
> inconvenient that you cannot set two ore more listeners on a single entry.

I was thinking about it a lot, and I couldn't find a better solution.
Basically, I wanted notifications to be sent using events instead of
callback, since callbacks are killed suspending an event page.

Since we have events, then there is no way to create a watcher on the same
event multiple times. If we allowed that, then we still would get one event
invocation per change.

Note, that we can have multiple listeners on these events.

> Does it have to be checked on each WatcherManager instance? What about
rejecting
> such a case in EntryWatcherService and allow WatcherManager to assume there's
no
> duplication?

I tried it, but the code became very complicated. Basically, WatcherManager's
methods are asynchronous, so setting or removing a watcher may require some
rounds
of PostTasks.

If we checked for duplication in EntryWatcherService *before creating*, then we
could still end up on creating a duplicate.

Eg.
1. Request creating a watcher on /A.
2. Request creating a watcher on /A. Not created yet, so go ahead.
3. Watcher on /A created.
4. Watcher on /A created (duplicate).

To avoid that we could create some vector of watchers being created, but not
created yet. That wouldn't work though, since even if a watcher is being
created,
its creation may fail. So the second request can't be discarded just because
the same watcher is being created, because it may succeed this time.

Another solution would be to use a SequencedTaskRunner and enqueue all watcher
creation calls. However, this makes code complicated and slower if setting
watchers
is slow (eg. for File System Provider API it may require some HTTP call).

So, I thought that just putting responsibility on backends is the simplest
solution.

WDYT?

[tzik, 2014-08-12 10:40:46]

Not yet look all of them, let me revisit later.

https://codereview.chromium.org/452043003/diff/40001/chrome/browser/file_syst...
File chrome/browser/file_system/entry_watcher_service.cc (right):

https://codereview.chromium.org/452043003/diff/40001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.cc:73:
base::MessageLoopProxy::current()->PostTask(
MessageLoopProxy seems deprecated ThreadTaskRunnerHandle::Get()?

https://codereview.chromium.org/452043003/diff/40001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.cc:230: if
(!watchers_[gurl].size())
.empty() is more readable.

https://codereview.chromium.org/452043003/diff/40001/chrome/browser/file_syst...
File chrome/browser/file_system/entry_watcher_service.h (right):

https://codereview.chromium.org/452043003/diff/40001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.h:78: public:
not needed?

https://codereview.chromium.org/452043003/diff/40001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.h:109: Profile* profile_;
All usage of this |profile_| is as a BrowserContext. Can we change the type to
it?

[mtomasz, 2014-08-13 03:08:44]

https://codereview.chromium.org/452043003/diff/40001/chrome/browser/file_syst...
File chrome/browser/file_system/entry_watcher_service.cc (right):

https://codereview.chromium.org/452043003/diff/40001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.cc:73:
base::MessageLoopProxy::current()->PostTask(
On 2014/08/12 10:40:46, tzik wrote:
> MessageLoopProxy seems deprecated ThreadTaskRunnerHandle::Get()?

Done.

https://codereview.chromium.org/452043003/diff/40001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.cc:230: if
(!watchers_[gurl].size())
On 2014/08/12 10:40:46, tzik wrote:
> .empty() is more readable.

Done.

https://codereview.chromium.org/452043003/diff/40001/chrome/browser/file_syst...
File chrome/browser/file_system/entry_watcher_service.h (right):

https://codereview.chromium.org/452043003/diff/40001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.h:78: public:
On 2014/08/12 10:40:46, tzik wrote:
> not needed?

Done.

https://codereview.chromium.org/452043003/diff/40001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.h:109: Profile* profile_;
On 2014/08/12 10:40:46, tzik wrote:
> All usage of this |profile_| is as a BrowserContext. Can we change the type to
> it?

Done.

[benwells, 2014-08-13 05:50:48]

Mostly questions...

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.cc:8: #include
<string>
As <string> is in the .h now you shouldn't need it here.

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
File chrome/browser/file_system/OWNERS (right):

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/OWNERS:1: benwells@chromium.org
Why are you creating a new chrome/browser/file_system folder?

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
File chrome/browser/file_system/entry_watcher_service.cc (right):

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.cc:25: if
(!event_router->ExtensionHasEventListener(extension_id, event->event_name))
Is it necessary to check this and return a bool?

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.cc:88:
base::ThreadTaskRunnerHandle::Get()->PostTask(
Why do you post this and not just run it immediately?

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.cc:89: FROM_HERE,
base::Bind(callback, base::File::FILE_ERROR_SECURITY));
Why is this a security error?

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.cc:188:
DCHECK_EQ(base::File::FILE_OK, result);
Is this DCHECK necessary?

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.cc:197:
callback.Run(base::File::FILE_ERROR_SECURITY);
Given that you should have already successfully gotten the watcher manager
before calling this, is this a programming error (i.e. NOT_REACHED?).

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.cc:239:
fileapi::WatcherManager* const watcher_manager =
Why do you get the watcher manager here?

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.cc:298: return true;
Could you add a comment about why this is needed? I assume it is so you can
restore persistent watchers as soon as the profile is loaded.

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
File chrome/browser/file_system/entry_watcher_service.h (right):

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.h:9: #include <set>
Nit: <set> doesn't appear to be used.

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.h:13: #include
"base/memory/singleton.h"
Super nit: you can forward declare "template <typename T> struct
DefaultSingletonTraits;" instead of including this.

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.h:25: namespace extensions {
If this isn't in the extensions folder it shouldn't be in the extensions
namespace.

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.h:79: // Holds an information
about an entry watcher.
Nit: Just "Holds information"

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.h:87: fileapi::FileSystemURL
url;
Nit: Could these fields all be const? Or do you change them after creating the
struct?

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.h:112: WatcherMap watchers_;
Nit: Could you comment (here or at the typedef) what the GURL and string are
for?

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.h:122: class
EntryWatcherServiceFactory : public BrowserContextKeyedServiceFactory {
This isn't always done, but often factories are kept in a separate file their
services. This means things that just need the factory don't also depend on the
service and visa versa, and IMHO is cleaner. Could you do that?

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.h:124: // Returns a service
instance singleton, after creating it (if necessary).
Nit: it isn't a singleton.

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
File chrome/browser/file_system/entry_watcher_service_unittest.cc (right):

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service_unittest.cc:123: ASSERT_LE(1u,
dispatch_event_log_targets_.size());
What causes the entrychanged and entryremoved events to happen?

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/local_dis...
File chrome/browser/local_discovery/storage/privet_filesystem_backend.h (right):

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/local_dis...
chrome/browser/local_discovery/storage/privet_filesystem_backend.h:5: #ifndef
CHROME_BROWSER_LOCAL_DISCOVERY_STORAGE_PRIVET_FILESYSTEM_BACKEND_H_
Is this meant to be "privet"?

[mtomasz, 2014-08-13 07:02:10]

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.cc:8: #include
<string>
On 2014/08/13 05:50:46, benwells wrote:
> As <string> is in the .h now you shouldn't need it here.

Done.

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
File chrome/browser/file_system/OWNERS (right):

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/OWNERS:1: benwells@chromium.org
On 2014/08/13 05:50:47, benwells wrote:
> Why are you creating a new chrome/browser/file_system folder?

We used to put just api method implementations in /c/b/extensions/api, while
meat such as services in /c/b.

See:
/c/b/chromeos/extensions/file_manager vs. /c/b/chromeos/file_manager
/c/b/chromeos/extensions/file_system_provider vs.
/c/b/chromeos/file_system_provider

However if you think that it should go to /c/b/extensions/api, then I'm fine
too.
On the other hand SavedFilesService is very deep in /apps.

I'm not sure what's the best place for this service. WDYT?

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
File chrome/browser/file_system/entry_watcher_service.cc (right):

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.cc:25: if
(!event_router->ExtensionHasEventListener(extension_id, event->event_name))
On 2014/08/13 05:50:47, benwells wrote:
> Is it necessary to check this and return a bool?

Right. Removed.

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.cc:88:
base::ThreadTaskRunnerHandle::Get()->PostTask(
On 2014/08/13 05:50:47, benwells wrote:
> Why do you post this and not just run it immediately?

Several times during reviews I was asked to post callbacks instead of calling
them directly with some exceptions. The reason is that a caller usually expects
it to be called asynchronously, eg.

void callback() {
  LOG(ERROR) << "Watcher creation finished.";
}

1. Call WatchDirectory(callback)
2. WatchDirectory executes PostTask(callback);
2. LOG(ERROR) << "Watcher creation started.";
3. callback is executed and prints "Watcher creation finished.".

If we called the callback directly, instead of posting it, we would get on the
screen:
Watcher creation finished.
Watcher creation started.

Another reason is that we may have different behavior depending if the callback
is called synchronously or asynchronously, hence if we always post it, then at
least we avoid one more possible bug.

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.cc:89: FROM_HERE,
base::Bind(callback, base::File::FILE_ERROR_SECURITY));
On 2014/08/13 05:50:47, benwells wrote:
> Why is this a security error?

Changed to base::File::FILE_ERROR_INVALID_OPERATION.

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.cc:188:
DCHECK_EQ(base::File::FILE_OK, result);
On 2014/08/13 05:50:47, benwells wrote:
> Is this DCHECK necessary?

Removed.

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.cc:197:
callback.Run(base::File::FILE_ERROR_SECURITY);
On 2014/08/13 05:50:47, benwells wrote:
> Given that you should have already successfully gotten the watcher manager
> before calling this, is this a programming error (i.e. NOT_REACHED?).

GetWatcherManager() may return NULL at any point. There is no restriction on it,
so I think we can't assume it's returned value doesn't change. WDYT?

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.cc:239:
fileapi::WatcherManager* const watcher_manager =
On 2014/08/13 05:50:47, benwells wrote:
> Why do you get the watcher manager here?

Not needed. Removed.

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.cc:298: return true;
On 2014/08/13 05:50:47, benwells wrote:
> Could you add a comment about why this is needed? I assume it is so you can
> restore persistent watchers as soon as the profile is loaded.

Done.

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
File chrome/browser/file_system/entry_watcher_service.h (right):

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.h:9: #include <set>
On 2014/08/13 05:50:47, benwells wrote:
> Nit: <set> doesn't appear to be used.

Done.

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.h:13: #include
"base/memory/singleton.h"
On 2014/08/13 05:50:47, benwells wrote:
> Super nit: you can forward declare "template <typename T> struct
> DefaultSingletonTraits;" instead of including this.

Our style guide says that for class templates, #includes are preferred. Not sure
if struct are considered as classes though.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Forwar...

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.h:25: namespace extensions {
On 2014/08/13 05:50:47, benwells wrote:
> If this isn't in the extensions folder it shouldn't be in the extensions
> namespace.

Good point. It was there in the past, but I moved it later. Removed.

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.h:79: // Holds an information
about an entry watcher.
On 2014/08/13 05:50:47, benwells wrote:
> Nit: Just "Holds information"

Done.

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.h:87: fileapi::FileSystemURL
url;
On 2014/08/13 05:50:47, benwells wrote:
> Nit: Could these fields all be const? Or do you change them after creating the
> struct?

We don't modify them directly, but with const they can't be used in stl
containers.

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.h:112: WatcherMap watchers_;
On 2014/08/13 05:50:47, benwells wrote:
> Nit: Could you comment (here or at the typedef) what the GURL and string are
> for?

Done.

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.h:122: class
EntryWatcherServiceFactory : public BrowserContextKeyedServiceFactory {
On 2014/08/13 05:50:47, benwells wrote:
> This isn't always done, but often factories are kept in a separate file their
> services. This means things that just need the factory don't also depend on
the
> service and visa versa, and IMHO is cleaner. Could you do that?

Done.

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.h:124: // Returns a service
instance singleton, after creating it (if necessary).
On 2014/08/13 05:50:47, benwells wrote:
> Nit: it isn't a singleton.

Singleton per context?

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
File chrome/browser/file_system/entry_watcher_service_unittest.cc (right):

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service_unittest.cc:123: ASSERT_LE(1u,
dispatch_event_log_targets_.size());
On 2014/08/13 05:50:47, benwells wrote:
> What causes the entrychanged and entryremoved events to happen?

It's part of the testing file system backend implementation.

https://codereview.chromium.org/452043003/diff/60001/content/public/test/test...

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/local_dis...
File chrome/browser/local_discovery/storage/privet_filesystem_backend.h (right):

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/local_dis...
chrome/browser/local_discovery/storage/privet_filesystem_backend.h:5: #ifndef
CHROME_BROWSER_LOCAL_DISCOVERY_STORAGE_PRIVET_FILESYSTEM_BACKEND_H_
On 2014/08/13 05:50:47, benwells wrote:
> Is this meant to be "privet"?

Yep, it's the name of the protocol.
https://developers.google.com/cloud-print/docs/privet

[tzik, 2014-08-13 09:21:49]

LGTM for my part.

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
File chrome/browser/file_system/entry_watcher_service.h (right):

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.h:44:
GetWatchedEntriesCallback;
Not used? / Can this argument be const ref?

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.h:92: typedef std::map<GURL,
std::map<std::string, EntryWatcher> > WatcherMap;
Can this be FileSystemURL with FileSystemURL::Comparator?

[benwells, 2014-08-13 23:57:02]

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/file_system/file_system_api.cc:8: #include
<string>
On 2014/08/13 07:02:08, mtomasz wrote:
> On 2014/08/13 05:50:46, benwells wrote:
> > As <string> is in the .h now you shouldn't need it here.
> 
> Done.

Is there meant to be a new patch set?

[mtomasz, 2014-08-14 00:27:37]

On 2014/08/13 23:57:02, benwells wrote:
>
https://codereview.chromium.org/452043003/diff/60001/chrome/browser/extension...
> File chrome/browser/extensions/api/file_system/file_system_api.cc (right):
> 
>
https://codereview.chromium.org/452043003/diff/60001/chrome/browser/extension...
> chrome/browser/extensions/api/file_system/file_system_api.cc:8: #include
> <string>
> On 2014/08/13 07:02:08, mtomasz wrote:
> > On 2014/08/13 05:50:46, benwells wrote:
> > > As <string> is in the .h now you shouldn't need it here.
> > 
> > Done.
> 
> Is there meant to be a new patch set?

Oops. Uploaded.

[benwells, 2014-08-14 05:16:32]

lgtm

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
File chrome/browser/file_system/OWNERS (right):

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/OWNERS:1: benwells@chromium.org
On 2014/08/13 07:02:08, mtomasz wrote:
> On 2014/08/13 05:50:47, benwells wrote:
> > Why are you creating a new chrome/browser/file_system folder?
> 
> We used to put just api method implementations in /c/b/extensions/api, while
> meat such as services in /c/b.
> 
> See:
> /c/b/chromeos/extensions/file_manager vs. /c/b/chromeos/file_manager
> /c/b/chromeos/extensions/file_system_provider vs.
> /c/b/chromeos/file_system_provider
> 
> However if you think that it should go to /c/b/extensions/api, then I'm fine
> too.
> On the other hand SavedFilesService is very deep in /apps.
> 
> I'm not sure what's the best place for this service. WDYT?

I would start in c/b/extensionns/api. We put SavedFilesService deep in apps as
it is used by multiple APIs - the file system and also the runtime api, I think.

If this isn't needed outside of the c/b/extensions/api/file_system i'd leave it
there.

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
File chrome/browser/file_system/entry_watcher_service.cc (right):

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.cc:88:
base::ThreadTaskRunnerHandle::Get()->PostTask(
On 2014/08/13 07:02:09, mtomasz wrote:
> On 2014/08/13 05:50:47, benwells wrote:
> > Why do you post this and not just run it immediately?
> 
> Several times during reviews I was asked to post callbacks instead of calling
> them directly with some exceptions. The reason is that a caller usually
expects
> it to be called asynchronously, eg.
> 
> void callback() {
>   LOG(ERROR) << "Watcher creation finished.";
> }
> 
> 1. Call WatchDirectory(callback)
> 2. WatchDirectory executes PostTask(callback);
> 2. LOG(ERROR) << "Watcher creation started.";
> 3. callback is executed and prints "Watcher creation finished.".
> 
> If we called the callback directly, instead of posting it, we would get on the
> screen:
> Watcher creation finished.
> Watcher creation started.
> 
> Another reason is that we may have different behavior depending if the
callback
> is called synchronously or asynchronously, hence if we always post it, then at
> least we avoid one more possible bug.

OK, makes sense. Could you add a comment explaining that you're posting back to
keep the order of operations consistent in all cases?

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.cc:197:
callback.Run(base::File::FILE_ERROR_SECURITY);
On 2014/08/13 07:02:09, mtomasz wrote:
> On 2014/08/13 05:50:47, benwells wrote:
> > Given that you should have already successfully gotten the watcher manager
> > before calling this, is this a programming error (i.e. NOT_REACHED?).
> 
> GetWatcherManager() may return NULL at any point. There is no restriction on
it,
> so I think we can't assume it's returned value doesn't change. WDYT?

That seems odd, are you thinking about the cases where the provider is providing
the watcher, and we have no control over it?

Could we return a new error 'provider doing weird things' or something? Invalid
operation will communicate to an app that they have done something wrong, but
this seems like it would be an error on the providers part. Or maybe I'm missing
something....

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
File chrome/browser/file_system/entry_watcher_service.h (right):

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.h:13: #include
"base/memory/singleton.h"
On 2014/08/13 07:02:09, mtomasz wrote:
> On 2014/08/13 05:50:47, benwells wrote:
> > Super nit: you can forward declare "template <typename T> struct
> > DefaultSingletonTraits;" instead of including this.
> 
> Our style guide says that for class templates, #includes are preferred. Not
sure
> if struct are considered as classes though.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Forwar...

Ah, good to know.

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
File chrome/browser/file_system/entry_watcher_service_unittest.cc (right):

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service_unittest.cc:123: ASSERT_LE(1u,
dispatch_event_log_targets_.size());
On 2014/08/13 07:02:09, mtomasz wrote:
> On 2014/08/13 05:50:47, benwells wrote:
> > What causes the entrychanged and entryremoved events to happen?
> 
> It's part of the testing file system backend implementation.
> 
>
https://codereview.chromium.org/452043003/diff/60001/content/public/test/test...

Right, OK. It seems a bit magical - could you add a comment here to help out
anyone who has to debug this test later on? Ideally you would directly cause
these things to happen with some function calls here but I can see that might
make the test structure yuck.

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/local_dis...
File chrome/browser/local_discovery/storage/privet_filesystem_backend.h (right):

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/local_dis...
chrome/browser/local_discovery/storage/privet_filesystem_backend.h:5: #ifndef
CHROME_BROWSER_LOCAL_DISCOVERY_STORAGE_PRIVET_FILESYSTEM_BACKEND_H_
On 2014/08/13 07:02:09, mtomasz wrote:
> On 2014/08/13 05:50:47, benwells wrote:
> > Is this meant to be "privet"?
> 
> Yep, it's the name of the protocol.
> https://developers.google.com/cloud-print/docs/privet

Huh, there you go.

https://codereview.chromium.org/452043003/diff/80001/chrome/browser/file_syst...
File chrome/browser/file_system/entry_watcher_service.cc (right):

https://codereview.chromium.org/452043003/diff/80001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.cc:219:
DCHECK_EQ(base::File::FILE_OK, result);
This DCHECK can be removed as well.

[benwells, 2014-08-14 05:17:48]

On 2014/08/14 05:16:32, benwells wrote:
> lgtm
> 
>
https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
> File chrome/browser/file_system/OWNERS (right):
> 
>
https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
> chrome/browser/file_system/OWNERS:1: mailto:benwells@chromium.org
> On 2014/08/13 07:02:08, mtomasz wrote:
> > On 2014/08/13 05:50:47, benwells wrote:
> > > Why are you creating a new chrome/browser/file_system folder?
> > 
> > We used to put just api method implementations in /c/b/extensions/api, while
> > meat such as services in /c/b.
> > 
> > See:
> > /c/b/chromeos/extensions/file_manager vs. /c/b/chromeos/file_manager
> > /c/b/chromeos/extensions/file_system_provider vs.
> > /c/b/chromeos/file_system_provider
> > 
> > However if you think that it should go to /c/b/extensions/api, then I'm fine
> > too.
> > On the other hand SavedFilesService is very deep in /apps.
> > 
> > I'm not sure what's the best place for this service. WDYT?
> 
> I would start in c/b/extensionns/api. We put SavedFilesService deep in apps as
> it is used by multiple APIs - the file system and also the runtime api, I
think.
> 
> If this isn't needed outside of the c/b/extensions/api/file_system i'd leave
it
> there.
> 
>
https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
> File chrome/browser/file_system/entry_watcher_service.cc (right):
> 
>
https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
> chrome/browser/file_system/entry_watcher_service.cc:88:
> base::ThreadTaskRunnerHandle::Get()->PostTask(
> On 2014/08/13 07:02:09, mtomasz wrote:
> > On 2014/08/13 05:50:47, benwells wrote:
> > > Why do you post this and not just run it immediately?
> > 
> > Several times during reviews I was asked to post callbacks instead of
calling
> > them directly with some exceptions. The reason is that a caller usually
> expects
> > it to be called asynchronously, eg.
> > 
> > void callback() {
> >   LOG(ERROR) << "Watcher creation finished.";
> > }
> > 
> > 1. Call WatchDirectory(callback)
> > 2. WatchDirectory executes PostTask(callback);
> > 2. LOG(ERROR) << "Watcher creation started.";
> > 3. callback is executed and prints "Watcher creation finished.".
> > 
> > If we called the callback directly, instead of posting it, we would get on
the
> > screen:
> > Watcher creation finished.
> > Watcher creation started.
> > 
> > Another reason is that we may have different behavior depending if the
> callback
> > is called synchronously or asynchronously, hence if we always post it, then
at
> > least we avoid one more possible bug.
> 
> OK, makes sense. Could you add a comment explaining that you're posting back
to
> keep the order of operations consistent in all cases?
> 
>
https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
> chrome/browser/file_system/entry_watcher_service.cc:197:
> callback.Run(base::File::FILE_ERROR_SECURITY);
> On 2014/08/13 07:02:09, mtomasz wrote:
> > On 2014/08/13 05:50:47, benwells wrote:
> > > Given that you should have already successfully gotten the watcher manager
> > > before calling this, is this a programming error (i.e. NOT_REACHED?).
> > 
> > GetWatcherManager() may return NULL at any point. There is no restriction on
> it,
> > so I think we can't assume it's returned value doesn't change. WDYT?
> 
> That seems odd, are you thinking about the cases where the provider is
providing
> the watcher, and we have no control over it?
> 
> Could we return a new error 'provider doing weird things' or something?
Invalid
> operation will communicate to an app that they have done something wrong, but
> this seems like it would be an error on the providers part. Or maybe I'm
missing
> something....
> 
>
https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
> File chrome/browser/file_system/entry_watcher_service.h (right):
> 
>
https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
> chrome/browser/file_system/entry_watcher_service.h:13: #include
> "base/memory/singleton.h"
> On 2014/08/13 07:02:09, mtomasz wrote:
> > On 2014/08/13 05:50:47, benwells wrote:
> > > Super nit: you can forward declare "template <typename T> struct
> > > DefaultSingletonTraits;" instead of including this.
> > 
> > Our style guide says that for class templates, #includes are preferred. Not
> sure
> > if struct are considered as classes though.
> > 
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Forwar...
> 
> Ah, good to know.
> 
>
https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
> File chrome/browser/file_system/entry_watcher_service_unittest.cc (right):
> 
>
https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
> chrome/browser/file_system/entry_watcher_service_unittest.cc:123:
ASSERT_LE(1u,
> dispatch_event_log_targets_.size());
> On 2014/08/13 07:02:09, mtomasz wrote:
> > On 2014/08/13 05:50:47, benwells wrote:
> > > What causes the entrychanged and entryremoved events to happen?
> > 
> > It's part of the testing file system backend implementation.
> > 
> >
>
https://codereview.chromium.org/452043003/diff/60001/content/public/test/test...
> 
> Right, OK. It seems a bit magical - could you add a comment here to help out
> anyone who has to debug this test later on? Ideally you would directly cause
> these things to happen with some function calls here but I can see that might
> make the test structure yuck.
> 
>
https://codereview.chromium.org/452043003/diff/60001/chrome/browser/local_dis...
> File chrome/browser/local_discovery/storage/privet_filesystem_backend.h
(right):
> 
>
https://codereview.chromium.org/452043003/diff/60001/chrome/browser/local_dis...
> chrome/browser/local_discovery/storage/privet_filesystem_backend.h:5: #ifndef
> CHROME_BROWSER_LOCAL_DISCOVERY_STORAGE_PRIVET_FILESYSTEM_BACKEND_H_
> On 2014/08/13 07:02:09, mtomasz wrote:
> > On 2014/08/13 05:50:47, benwells wrote:
> > > Is this meant to be "privet"?
> > 
> > Yep, it's the name of the protocol.
> > https://developers.google.com/cloud-print/docs/privet
> 
> Huh, there you go.
> 
>
https://codereview.chromium.org/452043003/diff/80001/chrome/browser/file_syst...
> File chrome/browser/file_system/entry_watcher_service.cc (right):
> 
>
https://codereview.chromium.org/452043003/diff/80001/chrome/browser/file_syst...
> chrome/browser/file_system/entry_watcher_service.cc:219:
> DCHECK_EQ(base::File::FILE_OK, result);
> This DCHECK can be removed as well.

Oops I somehow clicked the wrong button, didn't mean to lg just yet. not lgtm to
cancel that. I'm pretty much happy though, just need to clear up where the code
lives.

[kinaba, 2014-08-14 05:22:50]

lgtm still

https://codereview.chromium.org/452043003/diff/40001/webkit/browser/fileapi/w...
File webkit/browser/fileapi/watcher_manager.h (right):

https://codereview.chromium.org/452043003/diff/40001/webkit/browser/fileapi/w...
webkit/browser/fileapi/watcher_manager.h:58: // already watched, or setting up
the watcher fails, then |callback|
> > Is the restriction also posed on JS-level user code?
> I was thinking about it a lot, ... (snip) ...

Thanks for the explanation. Made sense to me.


> > What about rejecting such a case in EntryWatcherService and allow
> > WatcherManager to assume there's no duplication?
> ...(snip)...
> So, I thought that just putting responsibility on backends is the simplest
> solution.

I'm still not convinced. Wouldn't it be just shifting the difficulty from
EntryWatcherService to each backend? In the very same way that
EntryWatcherService
has to manage asynchronous tasks in a backend nicely, the backend has to manage
asynchronous work in its implementation (e.g., extensions, or ultimately,
some cloud API in case of FSP) if the backend has to check duplication.

I think the first "solution" is acceptable. In other words, it is ok to reject
the second request if some request is already in-flight for the same URL.

Well, however, either way, I don't have strong opinion for now, since we
anyhow impose the no-dup restriction. Let's see how it goes in the backend
implementation, and later come back here if needed.

[mtomasz, 2014-08-15 05:35:57]

https://codereview.chromium.org/452043003/diff/40001/webkit/browser/fileapi/w...
File webkit/browser/fileapi/watcher_manager.h (right):

https://codereview.chromium.org/452043003/diff/40001/webkit/browser/fileapi/w...
webkit/browser/fileapi/watcher_manager.h:58: // already watched, or setting up
the watcher fails, then |callback|
> I'm still not convinced. Wouldn't it be just shifting the difficulty from
> EntryWatcherService to each backend? In the very same way that
> EntryWatcherService
> has to manage asynchronous tasks in a backend nicely, the backend has to
manage
> asynchronous work in its implementation (e.g., extensions, or ultimately,
> some cloud API in case of FSP) if the backend has to check duplication.

How about doing it in the service, but with a SequencedWorkerPool created per
unique [url, extension_id] pair? That would be efficient and race-less. Also,
not
so complicated in implementation. WDYT?

> 
> I think the first "solution" is acceptable. In other words, it is ok to reject
> the second request if some request is already in-flight for the same URL.
> 
> Well, however, either way, I don't have strong opinion for now, since we
> anyhow impose the no-dup restriction. Let's see how it goes in the backend
> implementation, and later come back here if needed.

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
File chrome/browser/file_system/entry_watcher_service.cc (right):

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.cc:88:
base::ThreadTaskRunnerHandle::Get()->PostTask(
On 2014/08/14 05:16:32, benwells wrote:
> On 2014/08/13 07:02:09, mtomasz wrote:
> > On 2014/08/13 05:50:47, benwells wrote:
> > > Why do you post this and not just run it immediately?
> > 
> > Several times during reviews I was asked to post callbacks instead of
calling
> > them directly with some exceptions. The reason is that a caller usually
> expects
> > it to be called asynchronously, eg.
> > 
> > void callback() {
> >   LOG(ERROR) << "Watcher creation finished.";
> > }
> > 
> > 1. Call WatchDirectory(callback)
> > 2. WatchDirectory executes PostTask(callback);
> > 2. LOG(ERROR) << "Watcher creation started.";
> > 3. callback is executed and prints "Watcher creation finished.".
> > 
> > If we called the callback directly, instead of posting it, we would get on
the
> > screen:
> > Watcher creation finished.
> > Watcher creation started.
> > 
> > Another reason is that we may have different behavior depending if the
> callback
> > is called synchronously or asynchronously, hence if we always post it, then
at
> > least we avoid one more possible bug.
> 
> OK, makes sense. Could you add a comment explaining that you're posting back
to
> keep the order of operations consistent in all cases?

Done.

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.cc:197:
callback.Run(base::File::FILE_ERROR_SECURITY);
On 2014/08/14 05:16:32, benwells wrote:
> On 2014/08/13 07:02:09, mtomasz wrote:
> > On 2014/08/13 05:50:47, benwells wrote:
> > > Given that you should have already successfully gotten the watcher manager
> > > before calling this, is this a programming error (i.e. NOT_REACHED?).
> > 
> > GetWatcherManager() may return NULL at any point. There is no restriction on
> it,
> > so I think we can't assume it's returned value doesn't change. WDYT?
> 
> That seems odd, are you thinking about the cases where the provider is
providing
> the watcher, and we have no control over it?
> 
> Could we return a new error 'provider doing weird things' or something?
Invalid
> operation will communicate to an app that they have done something wrong, but
> this seems like it would be an error on the providers part. Or maybe I'm
missing
> something....

I reorganized the code. How about now?

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.cc:197:
callback.Run(base::File::FILE_ERROR_SECURITY);
On 2014/08/14 05:16:32, benwells wrote:
> On 2014/08/13 07:02:09, mtomasz wrote:
> > On 2014/08/13 05:50:47, benwells wrote:
> > > Given that you should have already successfully gotten the watcher manager
> > > before calling this, is this a programming error (i.e. NOT_REACHED?).
> > 
> > GetWatcherManager() may return NULL at any point. There is no restriction on
> it,
> > so I think we can't assume it's returned value doesn't change. WDYT?
> 
> That seems odd, are you thinking about the cases where the provider is
providing
> the watcher, and we have no control over it?
> 
> Could we return a new error 'provider doing weird things' or something?
Invalid
> operation will communicate to an app that they have done something wrong, but
> this seems like it would be an error on the providers part. Or maybe I'm
missing
> something....

I don't think it's weird. I can imagine a backend supporting some file system
type that is disabled while setting a watcher, and then GetWatcherManager()
returns NULL for that file type. It's unlikely to happen, but still I think any
backend can return NULL if it doesn't support a watcher anymore for any reason.

However, I tried to simplify the code and made following changes:
- Add a restriction that a pointer returned by GetWatcherManager() is valid
until shutdown.
- Bind the raw pointer to the EntryWatcher instance in WatchDirectory() to
OnWatchDirectoryCompleted.
Since UI message loop is the first thing to be stopped during shutdown, it
should safe.
Also, I believe KeyedServices are deleted *before* FileSystemContext, so it
should be all right.

WDYT?

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
File chrome/browser/file_system/entry_watcher_service_unittest.cc (right):

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service_unittest.cc:123: ASSERT_LE(1u,
dispatch_event_log_targets_.size());
On 2014/08/14 05:16:32, benwells wrote:
> On 2014/08/13 07:02:09, mtomasz wrote:
> > On 2014/08/13 05:50:47, benwells wrote:
> > > What causes the entrychanged and entryremoved events to happen?
> > 
> > It's part of the testing file system backend implementation.
> > 
> >
>
https://codereview.chromium.org/452043003/diff/60001/content/public/test/test...
> 
> Right, OK. It seems a bit magical - could you add a comment here to help out
> anyone who has to debug this test later on? Ideally you would directly cause
> these things to happen with some function calls here but I can see that might
> make the test structure yuck.

I added a comment. The TestWatcherManager class is in an anonymous namespace.
Even if we moved it out, we would need an ugly static cast. How about keeping it
as it is for now (with the comment)?

https://codereview.chromium.org/452043003/diff/80001/chrome/browser/file_syst...
File chrome/browser/file_system/entry_watcher_service.cc (right):

https://codereview.chromium.org/452043003/diff/80001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.cc:219:
DCHECK_EQ(base::File::FILE_OK, result);
On 2014/08/14 05:16:32, benwells wrote:
> This DCHECK can be removed as well.

Done.

[kinaba, 2014-08-18 02:32:15]

https://codereview.chromium.org/452043003/diff/40001/webkit/browser/fileapi/w...
File webkit/browser/fileapi/watcher_manager.h (right):

https://codereview.chromium.org/452043003/diff/40001/webkit/browser/fileapi/w...
webkit/browser/fileapi/watcher_manager.h:58: // already watched, or setting up
the watcher fails, then |callback|
On 2014/08/15 05:35:56, mtomasz wrote:
> > I'm still not convinced. Wouldn't it be just shifting the difficulty from
> > EntryWatcherService to each backend? In the very same way that
> > EntryWatcherService
> > has to manage asynchronous tasks in a backend nicely, the backend has to
> manage
> > asynchronous work in its implementation (e.g., extensions, or ultimately,
> > some cloud API in case of FSP) if the backend has to check duplication.
> 
> How about doing it in the service, but with a SequencedWorkerPool created per
> unique [url, extension_id] pair? That would be efficient and race-less. Also,
> not
> so complicated in implementation. WDYT?
> 
> > 
> > I think the first "solution" is acceptable. In other words, it is ok to
reject
> > the second request if some request is already in-flight for the same URL.
> > 
> > Well, however, either way, I don't have strong opinion for now, since we
> > anyhow impose the no-dup restriction. Let's see how it goes in the backend
> > implementation, and later come back here if needed.
> 

Sounds good.

[mtomasz, 2014-08-18 08:22:32]

On 2014/08/18 02:32:15, kinaba wrote:
>
https://codereview.chromium.org/452043003/diff/40001/webkit/browser/fileapi/w...
> File webkit/browser/fileapi/watcher_manager.h (right):
> 
>
https://codereview.chromium.org/452043003/diff/40001/webkit/browser/fileapi/w...
> webkit/browser/fileapi/watcher_manager.h:58: // already watched, or setting up
> the watcher fails, then |callback|
> On 2014/08/15 05:35:56, mtomasz wrote:
> > > I'm still not convinced. Wouldn't it be just shifting the difficulty from
> > > EntryWatcherService to each backend? In the very same way that
> > > EntryWatcherService
> > > has to manage asynchronous tasks in a backend nicely, the backend has to
> > manage
> > > asynchronous work in its implementation (e.g., extensions, or ultimately,
> > > some cloud API in case of FSP) if the backend has to check duplication.
> > 
> > How about doing it in the service, but with a SequencedWorkerPool created
per
> > unique [url, extension_id] pair? That would be efficient and race-less.
Also,
> > not
> > so complicated in implementation. WDYT?
> > 
> > > 
> > > I think the first "solution" is acceptable. In other words, it is ok to
> reject
> > > the second request if some request is already in-flight for the same URL.
> > > 
> > > Well, however, either way, I don't have strong opinion for now, since we
> > > anyhow impose the no-dup restriction. Let's see how it goes in the backend
> > > implementation, and later come back here if needed.
> > 
> 
> Sounds good.

@kinaba: I tried to do that by creating a SequencedTaskRunner with a named token
per url, and calling WatcherManager methods via
SequencedTaskRunner::PostNonNestableTask(). However, that wouldn't work since
implementations of WatcherManager may be asynchronous, and jump between threads.
I incorrectly assumed that PostNonNestableTask would block from posting a
nestable task until the callback is called, but it is of course impossible.

I don't see an easy way to avoid overlapping asynchronous requests in the C++
layer. How about keeping it as it is for now, and possibly refining in the
future?

[benwells, 2014-08-18 22:58:33]

lgtm with some nits (including one from tzik)

I think having you as an owner in the file_system api folder makes sense.

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
File chrome/browser/file_system/entry_watcher_service.cc (right):

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.cc:197:
callback.Run(base::File::FILE_ERROR_SECURITY);
On 2014/08/15 05:35:57, mtomasz wrote:
> On 2014/08/14 05:16:32, benwells wrote:
> > On 2014/08/13 07:02:09, mtomasz wrote:
> > > On 2014/08/13 05:50:47, benwells wrote:
> > > > Given that you should have already successfully gotten the watcher
manager
> > > > before calling this, is this a programming error (i.e. NOT_REACHED?).
> > > 
> > > GetWatcherManager() may return NULL at any point. There is no restriction
on
> > it,
> > > so I think we can't assume it's returned value doesn't change. WDYT?
> > 
> > That seems odd, are you thinking about the cases where the provider is
> providing
> > the watcher, and we have no control over it?
> > 
> > Could we return a new error 'provider doing weird things' or something?
> Invalid
> > operation will communicate to an app that they have done something wrong,
but
> > this seems like it would be an error on the providers part. Or maybe I'm
> missing
> > something....
> 
> I reorganized the code. How about now?

I think that is better, but the lifetime issues around shutdown are slightly
concerning. Could you add a comment explaining why it is safe?

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
File chrome/browser/file_system/entry_watcher_service.h (right):

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.h:44:
GetWatchedEntriesCallback;
On 2014/08/13 09:21:49, tzik wrote:
> Not used? / Can this argument be const ref?

^^^

https://codereview.chromium.org/452043003/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/entry_watcher_service_unittest.cc
(right):

https://codereview.chromium.org/452043003/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/entry_watcher_service_unittest.cc:122:
// The testing WatcherManager implementation emits a fake notification as soon
I think it it emits two. A comment line "The testing WatcherMananger
implementation emits hard coded notifications as soon as the watcher is set
properly. See Class::Function for details.

It will help any sheriff who ever has to debug these tests.

[mtomasz, 2014-08-19 06:50:36]

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
File chrome/browser/file_system/entry_watcher_service.cc (right):

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.cc:197:
callback.Run(base::File::FILE_ERROR_SECURITY);
On 2014/08/18 22:58:32, benwells wrote:
> On 2014/08/15 05:35:57, mtomasz wrote:
> > On 2014/08/14 05:16:32, benwells wrote:
> > > On 2014/08/13 07:02:09, mtomasz wrote:
> > > > On 2014/08/13 05:50:47, benwells wrote:
> > > > > Given that you should have already successfully gotten the watcher
> manager
> > > > > before calling this, is this a programming error (i.e. NOT_REACHED?).
> > > > 
> > > > GetWatcherManager() may return NULL at any point. There is no
restriction
> on
> > > it,
> > > > so I think we can't assume it's returned value doesn't change. WDYT?
> > > 
> > > That seems odd, are you thinking about the cases where the provider is
> > providing
> > > the watcher, and we have no control over it?
> > > 
> > > Could we return a new error 'provider doing weird things' or something?
> > Invalid
> > > operation will communicate to an app that they have done something wrong,
> but
> > > this seems like it would be an error on the providers part. Or maybe I'm
> > missing
> > > something....
> > 
> > I reorganized the code. How about now?
> 
> I think that is better, but the lifetime issues around shutdown are slightly
> concerning. Could you add a comment explaining why it is safe?

Done.

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
File chrome/browser/file_system/entry_watcher_service.h (right):

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.h:44:
GetWatchedEntriesCallback;
On 2014/08/18 22:58:32, benwells wrote:
> On 2014/08/13 09:21:49, tzik wrote:
> > Not used? / Can this argument be const ref?
> 
> ^^^

Done. Missed this one.

https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
chrome/browser/file_system/entry_watcher_service.h:92: typedef std::map<GURL,
std::map<std::string, EntryWatcher> > WatcherMap;
On 2014/08/13 09:21:49, tzik wrote:
> Can this be FileSystemURL with FileSystemURL::Comparator?

Good point. Done.

https://codereview.chromium.org/452043003/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/entry_watcher_service_unittest.cc
(right):

https://codereview.chromium.org/452043003/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/entry_watcher_service_unittest.cc:122:
// The testing WatcherManager implementation emits a fake notification as soon
On 2014/08/18 22:58:32, benwells wrote:
> I think it it emits two. A comment line "The testing WatcherMananger
> implementation emits hard coded notifications as soon as the watcher is set
> properly. See Class::Function for details.
> 
> It will help any sheriff who ever has to debug these tests. 

Done.

[mtomasz, 2014-08-25 05:39:56]

On 2014/08/19 06:50:36, mtomasz wrote:
>
https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
> File chrome/browser/file_system/entry_watcher_service.cc (right):
> 
>
https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
> chrome/browser/file_system/entry_watcher_service.cc:197:
> callback.Run(base::File::FILE_ERROR_SECURITY);
> On 2014/08/18 22:58:32, benwells wrote:
> > On 2014/08/15 05:35:57, mtomasz wrote:
> > > On 2014/08/14 05:16:32, benwells wrote:
> > > > On 2014/08/13 07:02:09, mtomasz wrote:
> > > > > On 2014/08/13 05:50:47, benwells wrote:
> > > > > > Given that you should have already successfully gotten the watcher
> > manager
> > > > > > before calling this, is this a programming error (i.e.
NOT_REACHED?).
> > > > > 
> > > > > GetWatcherManager() may return NULL at any point. There is no
> restriction
> > on
> > > > it,
> > > > > so I think we can't assume it's returned value doesn't change. WDYT?
> > > > 
> > > > That seems odd, are you thinking about the cases where the provider is
> > > providing
> > > > the watcher, and we have no control over it?
> > > > 
> > > > Could we return a new error 'provider doing weird things' or something?
> > > Invalid
> > > > operation will communicate to an app that they have done something
wrong,
> > but
> > > > this seems like it would be an error on the providers part. Or maybe I'm
> > > missing
> > > > something....
> > > 
> > > I reorganized the code. How about now?
> > 
> > I think that is better, but the lifetime issues around shutdown are slightly
> > concerning. Could you add a comment explaining why it is safe?
> 
> Done.
> 
>
https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
> File chrome/browser/file_system/entry_watcher_service.h (right):
> 
>
https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
> chrome/browser/file_system/entry_watcher_service.h:44:
> GetWatchedEntriesCallback;
> On 2014/08/18 22:58:32, benwells wrote:
> > On 2014/08/13 09:21:49, tzik wrote:
> > > Not used? / Can this argument be const ref?
> > 
> > ^^^
> 
> Done. Missed this one.
> 
>
https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
> chrome/browser/file_system/entry_watcher_service.h:92: typedef std::map<GURL,
> std::map<std::string, EntryWatcher> > WatcherMap;
> On 2014/08/13 09:21:49, tzik wrote:
> > Can this be FileSystemURL with FileSystemURL::Comparator?
> 
> Good point. Done.
> 
>
https://codereview.chromium.org/452043003/diff/120001/chrome/browser/extensio...
> File
chrome/browser/extensions/api/file_system/entry_watcher_service_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/452043003/diff/120001/chrome/browser/extensio...
>
chrome/browser/extensions/api/file_system/entry_watcher_service_unittest.cc:122:
> // The testing WatcherManager implementation emits a fake notification as soon
> On 2014/08/18 22:58:32, benwells wrote:
> > I think it it emits two. A comment line "The testing WatcherMananger
> > implementation emits hard coded notifications as soon as the watcher is set
> > properly. See Class::Function for details.
> > 
> > It will help any sheriff who ever has to debug these tests. 
> 
> Done.

@kinaba: Friendly ping as for https://codereview.chromium.org/452043003/#msg18.
Thanks.

[kinaba, 2014-08-25 06:04:24]

On 2014/08/25 05:39:56, mtomasz wrote:
> On 2014/08/19 06:50:36, mtomasz wrote:
> >
>
https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
> > File chrome/browser/file_system/entry_watcher_service.cc (right):
> > 
> >
>
https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
> > chrome/browser/file_system/entry_watcher_service.cc:197:
> > callback.Run(base::File::FILE_ERROR_SECURITY);
> > On 2014/08/18 22:58:32, benwells wrote:
> > > On 2014/08/15 05:35:57, mtomasz wrote:
> > > > On 2014/08/14 05:16:32, benwells wrote:
> > > > > On 2014/08/13 07:02:09, mtomasz wrote:
> > > > > > On 2014/08/13 05:50:47, benwells wrote:
> > > > > > > Given that you should have already successfully gotten the watcher
> > > manager
> > > > > > > before calling this, is this a programming error (i.e.
> NOT_REACHED?).
> > > > > > 
> > > > > > GetWatcherManager() may return NULL at any point. There is no
> > restriction
> > > on
> > > > > it,
> > > > > > so I think we can't assume it's returned value doesn't change. WDYT?
> > > > > 
> > > > > That seems odd, are you thinking about the cases where the provider is
> > > > providing
> > > > > the watcher, and we have no control over it?
> > > > > 
> > > > > Could we return a new error 'provider doing weird things' or
something?
> > > > Invalid
> > > > > operation will communicate to an app that they have done something
> wrong,
> > > but
> > > > > this seems like it would be an error on the providers part. Or maybe
I'm
> > > > missing
> > > > > something....
> > > > 
> > > > I reorganized the code. How about now?
> > > 
> > > I think that is better, but the lifetime issues around shutdown are
slightly
> > > concerning. Could you add a comment explaining why it is safe?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
> > File chrome/browser/file_system/entry_watcher_service.h (right):
> > 
> >
>
https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
> > chrome/browser/file_system/entry_watcher_service.h:44:
> > GetWatchedEntriesCallback;
> > On 2014/08/18 22:58:32, benwells wrote:
> > > On 2014/08/13 09:21:49, tzik wrote:
> > > > Not used? / Can this argument be const ref?
> > > 
> > > ^^^
> > 
> > Done. Missed this one.
> > 
> >
>
https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
> > chrome/browser/file_system/entry_watcher_service.h:92: typedef
std::map<GURL,
> > std::map<std::string, EntryWatcher> > WatcherMap;
> > On 2014/08/13 09:21:49, tzik wrote:
> > > Can this be FileSystemURL with FileSystemURL::Comparator?
> > 
> > Good point. Done.
> > 
> >
>
https://codereview.chromium.org/452043003/diff/120001/chrome/browser/extensio...
> > File
> chrome/browser/extensions/api/file_system/entry_watcher_service_unittest.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/452043003/diff/120001/chrome/browser/extensio...
> >
>
chrome/browser/extensions/api/file_system/entry_watcher_service_unittest.cc:122:
> > // The testing WatcherManager implementation emits a fake notification as
soon
> > On 2014/08/18 22:58:32, benwells wrote:
> > > I think it it emits two. A comment line "The testing WatcherMananger
> > > implementation emits hard coded notifications as soon as the watcher is
set
> > > properly. See Class::Function for details.
> > > 
> > > It will help any sheriff who ever has to debug these tests. 
> > 
> > Done.
> 
> @kinaba: Friendly ping as for
https://codereview.chromium.org/452043003/#msg18.
> Thanks.

Oops, sorry I've missed the mention.

Still think it's ok to land this patch in the current form (and later think
about it when needed.)

FYI my recommendation is still to overly prohibiting overlapping registrations:
https://codereview.chromium.org/452043003/#msg14

[mtomasz, 2014-08-25 08:38:18]

On 2014/08/25 06:04:24, kinaba wrote:
> On 2014/08/25 05:39:56, mtomasz wrote:
> > On 2014/08/19 06:50:36, mtomasz wrote:
> > >
> >
>
https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
> > > File chrome/browser/file_system/entry_watcher_service.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
> > > chrome/browser/file_system/entry_watcher_service.cc:197:
> > > callback.Run(base::File::FILE_ERROR_SECURITY);
> > > On 2014/08/18 22:58:32, benwells wrote:
> > > > On 2014/08/15 05:35:57, mtomasz wrote:
> > > > > On 2014/08/14 05:16:32, benwells wrote:
> > > > > > On 2014/08/13 07:02:09, mtomasz wrote:
> > > > > > > On 2014/08/13 05:50:47, benwells wrote:
> > > > > > > > Given that you should have already successfully gotten the
watcher
> > > > manager
> > > > > > > > before calling this, is this a programming error (i.e.
> > NOT_REACHED?).
> > > > > > > 
> > > > > > > GetWatcherManager() may return NULL at any point. There is no
> > > restriction
> > > > on
> > > > > > it,
> > > > > > > so I think we can't assume it's returned value doesn't change.
WDYT?
> > > > > > 
> > > > > > That seems odd, are you thinking about the cases where the provider
is
> > > > > providing
> > > > > > the watcher, and we have no control over it?
> > > > > > 
> > > > > > Could we return a new error 'provider doing weird things' or
> something?
> > > > > Invalid
> > > > > > operation will communicate to an app that they have done something
> > wrong,
> > > > but
> > > > > > this seems like it would be an error on the providers part. Or maybe
> I'm
> > > > > missing
> > > > > > something....
> > > > > 
> > > > > I reorganized the code. How about now?
> > > > 
> > > > I think that is better, but the lifetime issues around shutdown are
> slightly
> > > > concerning. Could you add a comment explaining why it is safe?
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
> > > File chrome/browser/file_system/entry_watcher_service.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
> > > chrome/browser/file_system/entry_watcher_service.h:44:
> > > GetWatchedEntriesCallback;
> > > On 2014/08/18 22:58:32, benwells wrote:
> > > > On 2014/08/13 09:21:49, tzik wrote:
> > > > > Not used? / Can this argument be const ref?
> > > > 
> > > > ^^^
> > > 
> > > Done. Missed this one.
> > > 
> > >
> >
>
https://codereview.chromium.org/452043003/diff/60001/chrome/browser/file_syst...
> > > chrome/browser/file_system/entry_watcher_service.h:92: typedef
> std::map<GURL,
> > > std::map<std::string, EntryWatcher> > WatcherMap;
> > > On 2014/08/13 09:21:49, tzik wrote:
> > > > Can this be FileSystemURL with FileSystemURL::Comparator?
> > > 
> > > Good point. Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/452043003/diff/120001/chrome/browser/extensio...
> > > File
> > chrome/browser/extensions/api/file_system/entry_watcher_service_unittest.cc
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/452043003/diff/120001/chrome/browser/extensio...
> > >
> >
>
chrome/browser/extensions/api/file_system/entry_watcher_service_unittest.cc:122:
> > > // The testing WatcherManager implementation emits a fake notification as
> soon
> > > On 2014/08/18 22:58:32, benwells wrote:
> > > > I think it it emits two. A comment line "The testing WatcherMananger
> > > > implementation emits hard coded notifications as soon as the watcher is
> set
> > > > properly. See Class::Function for details.
> > > > 
> > > > It will help any sheriff who ever has to debug these tests. 
> > > 
> > > Done.
> > 
> > @kinaba: Friendly ping as for
> https://codereview.chromium.org/452043003/#msg18.
> > Thanks.
> 
> Oops, sorry I've missed the mention.
> 
> Still think it's ok to land this patch in the current form (and later think
> about it when needed.)
> 
> FYI my recommendation is still to overly prohibiting overlapping
registrations:
> https://codereview.chromium.org/452043003/#msg14

The solution from msg#14 has some edge cases which need careful analysis. Eg.
calling RemoveWatcher just after AddWatcher. If we reject the RemoveWatcher()
request, then users may end up on having stale watchers. Each client would have
to call watcher methods in a queue. It would be nice if we could prevent that.

I have an idea for solving this problem, but it is quite large and tricky, so
I'd like to work on it in a separate CL.

[mtomasz, 2014-08-25 08:38:24]

The CQ bit was checked by mtomasz@chromium.org

[mtomasz, 2014-08-25 08:38:27]

The CQ bit was unchecked by mtomasz@chromium.org

[mtomasz, 2014-08-25 08:39:01]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-08-25 08:39:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mtomasz@chromium.org/452043003/160001

[commit-bot: I haz the power, 2014-08-25 09:21:55]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/45737)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/50937)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

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

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

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

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mtomasz, 2014-08-26 00:02:11]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-08-26 00:03:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mtomasz@chromium.org/452043003/160001

[mtomasz, 2014-08-26 00:49:14]

The CQ bit was unchecked by mtomasz@chromium.org

[mtomasz, 2014-08-26 01:11:13]

mtomasz@chromium.org changed reviewers:
+ jcivelli@chromium.org, noamsml@chromium.org

[mtomasz, 2014-08-26 01:12:42]

mtomasz@chromium.org changed reviewers:
- jcivelli@chromium.org

[mtomasz, 2014-08-26 01:12:42]

@noamsml: TBR for trivial change in privet_filesystem_backend.cc.

[mtomasz, 2014-08-26 01:13:23]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-08-26 01:15:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mtomasz@chromium.org/452043003/200001

[mtomasz, 2014-08-26 01:46:36]

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

[mtomasz, 2014-08-26 01:47:27]

On 2014/08/26 01:46:36, mtomasz wrote:
> mailto:mtomasz@chromium.org changed reviewers:
> + mailto:sky@chromium.org

@sky: PTAL at content/public/test/test_file_system_backend.*.

[sky, 2014-08-26 16:45:27]

https://codereview.chromium.org/452043003/diff/220001/content/public/test/tes...
File content/public/test/test_file_system_backend.cc (right):

https://codereview.chromium.org/452043003/diff/220001/content/public/test/tes...
content/public/test/test_file_system_backend.cc:102:
base::ThreadTaskRunnerHandle::Get()->PostTask(
Is base::ThreadTaskRunnerHandle::Get() a separate thread than current? Might
there be race conditions in accessing watched_urls_ from the two threads?

[Noam Samuel, 2014-08-26 18:57:03]

lgtm

[mtomasz, 2014-08-26 23:04:53]

https://codereview.chromium.org/452043003/diff/220001/content/public/test/tes...
File content/public/test/test_file_system_backend.cc (right):

https://codereview.chromium.org/452043003/diff/220001/content/public/test/tes...
content/public/test/test_file_system_backend.cc:102:
base::ThreadTaskRunnerHandle::Get()->PostTask(
On 2014/08/26 16:45:27, sky wrote:
> Is base::ThreadTaskRunnerHandle::Get() a separate thread than current? Might
> there be race conditions in accessing watched_urls_ from the two threads?

IIUC ThreadTaskRunnerHandle::Get basically returns the current thread, so here
it would be the main UI thread. It is used to post a task on the current thread.

[sky, 2014-08-26 23:51:09]

https://codereview.chromium.org/452043003/diff/220001/content/public/test/tes...
File content/public/test/test_file_system_backend.cc (right):

https://codereview.chromium.org/452043003/diff/220001/content/public/test/tes...
content/public/test/test_file_system_backend.cc:102:
base::ThreadTaskRunnerHandle::Get()->PostTask(
On 2014/08/26 23:04:53, mtomasz wrote:
> On 2014/08/26 16:45:27, sky wrote:
> > Is base::ThreadTaskRunnerHandle::Get() a separate thread than current? Might
> > there be race conditions in accessing watched_urls_ from the two threads?
> 
> IIUC ThreadTaskRunnerHandle::Get basically returns the current thread, so here
> it would be the main UI thread. It is used to post a task on the current
thread.

Is there a reason you don't do MessageLoop::current()->PostTask then?

[mtomasz, 2014-08-27 00:57:30]

On 2014/08/26 23:51:09, sky wrote:
>
https://codereview.chromium.org/452043003/diff/220001/content/public/test/tes...
> File content/public/test/test_file_system_backend.cc (right):
> 
>
https://codereview.chromium.org/452043003/diff/220001/content/public/test/tes...
> content/public/test/test_file_system_backend.cc:102:
> base::ThreadTaskRunnerHandle::Get()->PostTask(
> On 2014/08/26 23:04:53, mtomasz wrote:
> > On 2014/08/26 16:45:27, sky wrote:
> > > Is base::ThreadTaskRunnerHandle::Get() a separate thread than current?
Might
> > > there be race conditions in accessing watched_urls_ from the two threads?
> > 
> > IIUC ThreadTaskRunnerHandle::Get basically returns the current thread, so
here
> > it would be the main UI thread. It is used to post a task on the current
> thread.
> 
> Is there a reason you don't do MessageLoop::current()->PostTask then?

I'm following the instructions from "[chromium-dev] Time to retiring
MessageLoopProxy".

"All code expecting a MessageLoopProxy *should* work the same with a
SingleThreadTaskRunner (which MessageLoopProxy is). All code wanting the current
SingleThreadTaskRunner should use the ThreadTaskRunnerHandle::Get() to get it."

[sky, 2014-08-27 14:35:01]

Fair enough, LGTM

[mtomasz, 2014-08-27 22:48:34]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-08-27 22:53:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mtomasz@chromium.org/452043003/220001

[commit-bot: I haz the power, 2014-08-28 00:25:52]

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

[commit-bot: I haz the power, 2014-08-28 00:25:54]

Failed to commit the patch.

[mtomasz, 2014-08-28 00:28:33]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-08-28 00:31:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mtomasz@chromium.org/452043003/220001

[commit-bot: I haz the power, 2014-08-28 01:02:02]

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

[commit-bot: I haz the power, 2014-08-28 01:02:03]

Failed to commit the patch.

[mtomasz, 2014-08-28 05:31:03]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-08-28 05:31:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mtomasz@chromium.org/452043003/220001

[commit-bot: I haz the power, 2014-08-28 05:37:52]

Committed patchset #11 (id:220001) as b15956493ebd3cf823761ea6f34815fefbe6ec79

[commit-bot: I haz the power, 2014-09-10 02:57:48]

Patchset 11 (id:??) landed as
https://crrev.com/b75244fdf1d75e67485649e6a96a69c50676bbc3
Cr-Commit-Position: refs/heads/master@{#292327}