[fsp] Allow to create multiple observers for a directory, up to one per origin.

[fsp] Allow to create multiple observers for a directory, up to one per origin.

This patch allows to create multiple subscribers for observed entry change
notifications, up to one per origin for the same entry. Eg. multiple extensions
may want to observe the same directory.

Also, the persistent argument is introduced. Persistent subscriber mean, that the
origin is watching the entry persistently, despite reboots. It will be used by
cache only.

Extensions should never use persistent origins. Such subscribers will be removed
on reboot.

TEST=unit_tests: *FileSystemProvider*Registry*,
     *FileSystemProvider*ProvidedFileSystem*
BUG=248427
Committed: https://crrev.com/70fb5591df0082b01e07f77b62da97de1fa4d416
Cr-Commit-Position: refs/heads/master@{#301586}

[mtomasz, 2014-10-21 09:32:51]

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

[mtomasz, 2014-10-21 09:33:48]

@hirono: PTAL. It seems to be the last big patches for entry watching logic in
FSP.

[hirono, 2014-10-22 06:00:36]

https://codereview.chromium.org/642023003/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_system_provider/observed_entry.h (right):

https://codereview.chromium.org/642023003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/observed_entry.h:9: #include <set>
Is this used?

https://codereview.chromium.org/642023003/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_system_provider/provided_file_system.cc
(right):

https://codereview.chromium.org/642023003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/provided_file_system.cc:397: if
(!recursive || it->second.recursive) {
If extension A requests with recursive=false and then extension B requests with
recursive=true, it looks extension A obtains the change events for children of
the directory.

[mtomasz, 2014-10-24 05:48:42]

https://codereview.chromium.org/642023003/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_system_provider/observed_entry.h (right):

https://codereview.chromium.org/642023003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/observed_entry.h:9: #include <set>
On 2014/10/22 06:00:35, hirono wrote:
> Is this used?

Done.

https://codereview.chromium.org/642023003/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_system_provider/provided_file_system.cc
(right):

https://codereview.chromium.org/642023003/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/provided_file_system.cc:397: if
(!recursive || it->second.recursive) {
On 2014/10/22 06:00:35, hirono wrote:
> If extension A requests with recursive=false and then extension B requests
with
> recursive=true, it looks extension A obtains the change events for children of
> the directory.

After offline discussion we decided to split recursive and non-recursive
observers into separate ones. This is done in 679573002.

[hirono, 2014-10-27 02:20:18]

https://codereview.chromium.org/642023003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/registry.cc (right):

https://codereview.chromium.org/642023003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/registry.cc:208: (!last_tag.empty()
|| persistent_origins->GetSize()))) {
Is this indent correct?

https://codereview.chromium.org/642023003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/registry.cc:227: GURL
origin_as_gurl(origin);
nit: const

[mtomasz, 2014-10-27 04:05:57]

https://codereview.chromium.org/642023003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/registry.cc (right):

https://codereview.chromium.org/642023003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/registry.cc:208: (!last_tag.empty()
|| persistent_origins->GetSize()))) {
On 2014/10/27 02:20:17, hirono wrote:
> Is this indent correct?

git cl format forces this formatting.

https://codereview.chromium.org/642023003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/registry.cc:227: GURL
origin_as_gurl(origin);
On 2014/10/27 02:20:17, hirono wrote:
> nit: const

Done.

[hirono, 2014-10-27 04:50:32]

https://codereview.chromium.org/642023003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/observed_entry.h (right):

https://codereview.chromium.org/642023003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/observed_entry.h:49: bool
persistent;
Could you add a comment about what persistent means?

https://codereview.chromium.org/642023003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/provided_file_system.cc
(right):

https://codereview.chromium.org/642023003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system.cc:397: if (it
!= observed_entries_.end()) {
it != observed_entries_.end() is always false because of the check at #387.
Should we remove lines 387 - 395?

[mtomasz, 2014-10-27 05:01:18]

https://codereview.chromium.org/642023003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/observed_entry.h (right):

https://codereview.chromium.org/642023003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/observed_entry.h:49: bool
persistent;
On 2014/10/27 04:50:31, hirono wrote:
> Could you add a comment about what persistent means?

Done.

https://codereview.chromium.org/642023003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/provided_file_system.cc
(right):

https://codereview.chromium.org/642023003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system.cc:397: if (it
!= observed_entries_.end()) {
On 2014/10/27 04:50:31, hirono wrote:
> it != observed_entries_.end() is always false because of the check at #387.
> Should we remove lines 387 - 395?

Right. Done.

[hirono, 2014-10-27 08:15:15]

lgtm with nits!

https://codereview.chromium.org/642023003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/provided_file_system.cc
(right):

https://codereview.chromium.org/642023003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system.cc:388: if
(it->second.subscribers.find(origin) != it->second.subscribers.end()) {
nit:

if (it != observed_entries_.end() &&
    it->second.subscribers.find(origin) != it->second.subscribers.end()) {
}

for less indent?

https://codereview.chromium.org/642023003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/registry.cc (right):

https://codereview.chromium.org/642023003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/registry.cc:218: for
(base::ListValue::const_iterator persistent_origin_it =
nit: for (const auto& it : persistent_origins) for consistency?

[mtomasz, 2014-10-28 06:09:18]

https://codereview.chromium.org/642023003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/provided_file_system.cc
(right):

https://codereview.chromium.org/642023003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system.cc:388: if
(it->second.subscribers.find(origin) != it->second.subscribers.end()) {
On 2014/10/27 08:15:15, hirono wrote:
> nit:
> 
> if (it != observed_entries_.end() &&
>     it->second.subscribers.find(origin) != it->second.subscribers.end()) {
> }
> 
> for less indent?

Done + fixed a bug.

https://codereview.chromium.org/642023003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/registry.cc (right):

https://codereview.chromium.org/642023003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/registry.cc:218: for
(base::ListValue::const_iterator persistent_origin_it =
On 2014/10/27 08:15:15, hirono wrote:
> nit: for (const auto& it : persistent_origins) for consistency?

Done.

[mtomasz, 2014-10-28 06:40:11]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-10-28 06:40:51]

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

[commit-bot: I haz the power, 2014-10-28 07:23:54]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-10-28 07:24:33]

Patchset 5 (id:??) landed as
https://crrev.com/70fb5591df0082b01e07f77b62da97de1fa4d416
Cr-Commit-Position: refs/heads/master@{#301586}