mediaview: Change the contract of watcher callbacks.

mediaview: Change the contract of watcher callbacks.

In initial contract, watchers could be automatically uninstalled without
explicit RemoveWatcher() calls if watched directories are deleted.
However it does not align with the contract of storage::WatcherManager
(the primary user of watchers) which leaves watchers even after watched
directories are removed. Using different contracts may make it slightly
difficult to understand, so this patch will update the Mojo IPC contract
to align with WatcherManager's.

Usually we should not update the contract of existing Mojo IPC methods,
but it is okay in this case because we have not implemented those
methods in both Chrome and Android yet. One exception is
https://codereview.chromium.org/2709613006/ which implemented a fake of
FileSystemInstance, but it (unexpectedly) follows the new contract, so
we do not need to make changes to it.

BUG=chromium:684233
TEST=trybot

Review-Url: https://codereview.chromium.org/2718303002
Cr-Commit-Position: refs/heads/master@{#454285}
Committed: https://chromium.googlesource.com/chromium/src/+/6cbbd4f60f2285cf33ed2a47658ab3e3e7380eaa

[Shuhei Takahashi, 2017-02-28 04:50:32]

Description was changed from

==========
mediaview: Change the contract of watcher callbacks.

Initial contract was that watchers can be automatically uninstalled
without RemoveWatcher() calls if watched directories are deleted.
However it does not align with the contract of storage::WatcherManager
(the primary user of watchers). Using different contracts may make it
slightly difficult to understand, so this patch will update the Mojo IPC
contract to align with WatcherManager's.

BUG=chromium:684233
TEST=trybot
==========

to

==========
mediaview: Change the contract of watcher callbacks.

Initial contract was that watchers can be automatically uninstalled
without RemoveWatcher() calls if watched directories are deleted.
However it does not align with the contract of storage::WatcherManager
(the primary user of watchers). Using different contracts may make it
slightly difficult to understand, so this patch will update the Mojo IPC
contract to align with WatcherManager's.

Usually we should not update the contract of existing Mojo IPC methods,
but it is okay in this case because we have not implemented those
methods in both Chrome and Android yet. One exception is
https://codereview.chromium.org/2709613006/ which implemented a fake of
FileSystemInstance, but it (unexpectedly) follows the new contract, so
we do not need to make changes to the fake.

BUG=chromium:684233
TEST=trybot
==========

[Shuhei Takahashi, 2017-02-28 04:50:45]

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

[commit-bot: I haz the power, 2017-02-28 04:51:24]

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

[Shuhei Takahashi, 2017-02-28 04:52:58]

Description was changed from

==========
mediaview: Change the contract of watcher callbacks.

Initial contract was that watchers can be automatically uninstalled
without RemoveWatcher() calls if watched directories are deleted.
However it does not align with the contract of storage::WatcherManager
(the primary user of watchers). Using different contracts may make it
slightly difficult to understand, so this patch will update the Mojo IPC
contract to align with WatcherManager's.

Usually we should not update the contract of existing Mojo IPC methods,
but it is okay in this case because we have not implemented those
methods in both Chrome and Android yet. One exception is
https://codereview.chromium.org/2709613006/ which implemented a fake of
FileSystemInstance, but it (unexpectedly) follows the new contract, so
we do not need to make changes to the fake.

BUG=chromium:684233
TEST=trybot
==========

to

==========
mediaview: Change the contract of watcher callbacks.

In initial contract, watchers could be automatically uninstalled without
explicit RemoveWatcher() calls if watched directories are deleted.
However it does not align with the contract of storage::WatcherManager
(the primary user of watchers) which leaves watchers even after watched
directories are removed. Using different contracts may make it slightly
difficult to understand, so this patch will update the Mojo IPC contract
to align with WatcherManager's.

Usually we should not update the contract of existing Mojo IPC methods,
but it is okay in this case because we have not implemented those
methods in both Chrome and Android yet. One exception is
https://codereview.chromium.org/2709613006/ which implemented a fake of
FileSystemInstance, but it (unexpectedly) follows the new contract, so
we do not need to make changes to it.

BUG=chromium:684233
TEST=trybot
==========

[Shuhei Takahashi, 2017-02-28 04:53:56]

nya@chromium.org changed reviewers:
+ dcheng@chromium.org, hidehiko@chromium.org

[Shuhei Takahashi, 2017-02-28 04:53:56]

hidehiko: PTAL

dcheng: PTAL for security review

[commit-bot: I haz the power, 2017-02-28 05:37:55]

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

[commit-bot: I haz the power, 2017-02-28 05:37:56]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-02-28 07:27:59]

LGTM

[hidehiko, 2017-03-02 14:20:28]

LGTM. Sorry for delay.

https://codereview.chromium.org/2718303002/diff/1/components/arc/common/file_...
File components/arc/common/file_system.mojom (right):

https://codereview.chromium.org/2718303002/diff/1/components/arc/common/file_...
components/arc/common/file_system.mojom:83: // case, different watcher IDs are
returned.
Optional: How about adding the lifetime of watcher? E.g.;

Watcher IDs are not persistent. I.e., on connection lost, including ARC crash,
all watcher IDs are invalidated.

[Shuhei Takahashi, 2017-03-02 16:13:16]

Thanks for reviewing!

https://codereview.chromium.org/2718303002/diff/1/components/arc/common/file_...
File components/arc/common/file_system.mojom (right):

https://codereview.chromium.org/2718303002/diff/1/components/arc/common/file_...
components/arc/common/file_system.mojom:83: // case, different watcher IDs are
returned.
On 2017/03/02 14:20:28, hidehiko wrote:
> Optional: How about adding the lifetime of watcher? E.g.;
> 
> Watcher IDs are not persistent. I.e., on connection lost, including ARC crash,
> all watcher IDs are invalidated.

Sounds great! Let's do so.

[Shuhei Takahashi, 2017-03-02 16:13:38]

The CQ bit was checked by nya@chromium.org

[Shuhei Takahashi, 2017-03-02 16:13:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
hidehiko@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2718303002/#ps20001
(title: "Added a comment about watcher persistence.")

[commit-bot: I haz the power, 2017-03-02 16:13:53]

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

[commit-bot: I haz the power, 2017-03-02 16:49:31]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488471218534080,
"parent_rev": "df47eddfc5482633bf4612576fd096b7e67cc496", "commit_rev":
"6cbbd4f60f2285cf33ed2a47658ab3e3e7380eaa"}

[commit-bot: I haz the power, 2017-03-02 16:50:09]

Description was changed from

==========
mediaview: Change the contract of watcher callbacks.

In initial contract, watchers could be automatically uninstalled without
explicit RemoveWatcher() calls if watched directories are deleted.
However it does not align with the contract of storage::WatcherManager
(the primary user of watchers) which leaves watchers even after watched
directories are removed. Using different contracts may make it slightly
difficult to understand, so this patch will update the Mojo IPC contract
to align with WatcherManager's.

Usually we should not update the contract of existing Mojo IPC methods,
but it is okay in this case because we have not implemented those
methods in both Chrome and Android yet. One exception is
https://codereview.chromium.org/2709613006/ which implemented a fake of
FileSystemInstance, but it (unexpectedly) follows the new contract, so
we do not need to make changes to it.

BUG=chromium:684233
TEST=trybot
==========

to

==========
mediaview: Change the contract of watcher callbacks.

In initial contract, watchers could be automatically uninstalled without
explicit RemoveWatcher() calls if watched directories are deleted.
However it does not align with the contract of storage::WatcherManager
(the primary user of watchers) which leaves watchers even after watched
directories are removed. Using different contracts may make it slightly
difficult to understand, so this patch will update the Mojo IPC contract
to align with WatcherManager's.

Usually we should not update the contract of existing Mojo IPC methods,
but it is okay in this case because we have not implemented those
methods in both Chrome and Android yet. One exception is
https://codereview.chromium.org/2709613006/ which implemented a fake of
FileSystemInstance, but it (unexpectedly) follows the new contract, so
we do not need to make changes to it.

BUG=chromium:684233
TEST=trybot

Review-Url: https://codereview.chromium.org/2718303002
Cr-Commit-Position: refs/heads/master@{#454285}
Committed:
https://chromium.googlesource.com/chromium/src/+/6cbbd4f60f2285cf33ed2a47658a...
==========

[commit-bot: I haz the power, 2017-03-02 16:50:10]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/6cbbd4f60f2285cf33ed2a47658a...