Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2714433003:
mediaview: IPC definitions to watch for document changes. (Closed)
|
Created:
3 years, 10 months ago by Shuhei Takahashi Modified:
3 years, 10 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, viettrungluu+watch_chromium.org, hidehiko+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, lhchavez+watch_chromium.org, darin (slow to review), qsr+mojo_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmediaview: IPC definitions to watch for document changes.
New IPC methods will allow Chrome to watch for changes in Android
documents provider.
This patch contains just mojom changes and minimal changes to
FakeFileSystemInstance. Implementation of the fake is coming in the
next patch.
BUG=chromium:684233
TEST=trybot
Review-Url: https://codereview.chromium.org/2714433003
Cr-Commit-Position: refs/heads/master@{#452775}
Committed: https://chromium.googlesource.com/chromium/src/+/533aed06060aba3689e57e04bb352f14cfc7ca25
Patch Set 1 #
Total comments: 4
Patch Set 2 : Review ready. #Patch Set 3 : Addressed lhchavez's comments. #Patch Set 4 : Make it clear watchers can be installed only on directories. #
Total comments: 2
Patch Set 5 : Rebased to ToT. #
Total comments: 1
Patch Set 6 : Added a comment. #
Total comments: 2
Patch Set 7 : Add more fool-proof comment as per hidehiko's request in another CL. #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 31 (17 generated)
Description was changed from ========== mediaview: IPC definitions to watch for document changes. New IPC methods will allow Chrome to watch for changes in Android documents provider. BUG=chromium:684233 TEST=trybot ========== to ========== mediaview: IPC definitions to watch for document changes. New IPC methods will allow Chrome to watch for changes in Android documents provider. This patch contains just mojom changes and minimal changes to FakeFileSystemInstance. Implementation of the fake is coming in the next patch. BUG=chromium:684233 TEST=trybot ==========
The CQ bit was checked by nya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
nya@chromium.org changed reviewers: + dcheng@chromium.org, hidehiko@chromium.org
hidehiko: PTAL dcheng: PTAL for security review
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
drive-by https://codereview.chromium.org/2714433003/diff/1/components/arc/common/file_... File components/arc/common/file_system.mojom (right): https://codereview.chromium.org/2714433003/diff/1/components/arc/common/file_... components/arc/common/file_system.mojom:37: [MinVersion=3] OnDocumentChanged@0(int64 watcher_id, bool deleted); I'd prefer to replace |bool deleted| with an enum indicating the nature of the change. Or a struct with even more information, but that depends on whether the implementation will eagerly refresh metadata (struct would be preferred) or do it lazily thus coalescing multiple changed notifications (enum is sufficient). https://codereview.chromium.org/2714433003/diff/1/components/arc/common/file_... components/arc/common/file_system.mojom:95: // Uninstalls a document watcher. Please document the semantics of uninstalling a document watcher that has been deleted, given the wording of the comment above OnDocumentChanged() ("does not need to explicitly call").
The CQ bit was checked by nya@chromium.org to run a CQ dry run
Thanks Luis for comments! https://codereview.chromium.org/2714433003/diff/1/components/arc/common/file_... File components/arc/common/file_system.mojom (right): https://codereview.chromium.org/2714433003/diff/1/components/arc/common/file_... components/arc/common/file_system.mojom:37: [MinVersion=3] OnDocumentChanged@0(int64 watcher_id, bool deleted); On 2017/02/22 17:29:31, Luis Héctor Chávez wrote: > I'd prefer to replace |bool deleted| with an enum indicating the nature of the > change. Or a struct with even more information, but that depends on whether the > implementation will eagerly refresh metadata (struct would be preferred) or do > it lazily thus coalescing multiple changed notifications (enum is sufficient). Sure, let's use enum instead. We are not going to do eager refreshing because the expected caller (storage::WatcherManager) does not support such protocol. https://codereview.chromium.org/2714433003/diff/1/components/arc/common/file_... components/arc/common/file_system.mojom:95: // Uninstalls a document watcher. On 2017/02/22 17:29:31, Luis Héctor Chávez wrote: > Please document the semantics of uninstalling a document watcher that has been > deleted, given the wording of the comment above OnDocumentChanged() ("does not > need to explicitly call"). I changed the comment of OnDocumentChanged(): Before: the host does not need to explicitly call RemoveWatcher() After: the host should not call RemoveWatcher() And updated the comments around here to be more explicit about semantics.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Oh, BTW, I remembered I should make it clear that watchers can be installed only on directories. Updated the comments.
The CQ bit was checked by nya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2714433003/diff/60001/components/arc/common/f... File components/arc/common/file_system.mojom (right): https://codereview.chromium.org/2714433003/diff/60001/components/arc/common/f... components/arc/common/file_system.mojom:118: // (notified by OnDocumentChanged() with |type| = DELETED). My understanding is; until this callback is called on Chrome, OnDocumentChanged() still can be called even after RemoveWatcher(), right? If so (or not), could you comment the timing explicitly?
https://codereview.chromium.org/2714433003/diff/80001/components/arc/common/f... File components/arc/common/file_system.mojom (right): https://codereview.chromium.org/2714433003/diff/80001/components/arc/common/f... components/arc/common/file_system.mojom:108: [MinVersion=3] Init@5(FileSystemHost host_ptr); Btw, I know this is the pattern in ARC++, but for other code like this, we've been moving towards a factory model where we ensure a 1:1 binding by using a factory interface, e.g. interface ArcInterfaceFactory { CreateFileSystemInstance(FileSystemHost&) => (FileSystemInstance); } Which guarantees: - 1:1 binding - that Init() won't be called multiple times - easier to understand (probably) Is that something we could investigate for ARC++?
On 2017/02/23 19:07:07, dcheng wrote: > https://codereview.chromium.org/2714433003/diff/80001/components/arc/common/f... > File components/arc/common/file_system.mojom (right): > > https://codereview.chromium.org/2714433003/diff/80001/components/arc/common/f... > components/arc/common/file_system.mojom:108: [MinVersion=3] > Init@5(FileSystemHost host_ptr); > Btw, I know this is the pattern in ARC++, but for other code like this, we've > been moving towards a factory model where we ensure a 1:1 binding by using a > factory interface, e.g. > > interface ArcInterfaceFactory { > CreateFileSystemInstance(FileSystemHost&) => (FileSystemInstance); > } > > Which guarantees: > - 1:1 binding > - that Init() won't be called multiple times > - easier to understand (probably) > > Is that something we could investigate for ARC++? I'm planning to do some changes to the plumbing for ARC++ that might align us a bit better with the overall servicification effort, but that's going to take some time :(
On 2017/02/23 23:06:58, Luis Héctor Chávez wrote: > On 2017/02/23 19:07:07, dcheng wrote: > > > https://codereview.chromium.org/2714433003/diff/80001/components/arc/common/f... > > File components/arc/common/file_system.mojom (right): > > > > > https://codereview.chromium.org/2714433003/diff/80001/components/arc/common/f... > > components/arc/common/file_system.mojom:108: [MinVersion=3] > > Init@5(FileSystemHost host_ptr); > > Btw, I know this is the pattern in ARC++, but for other code like this, we've > > been moving towards a factory model where we ensure a 1:1 binding by using a > > factory interface, e.g. > > > > interface ArcInterfaceFactory { > > CreateFileSystemInstance(FileSystemHost&) => (FileSystemInstance); > > } > > > > Which guarantees: > > - 1:1 binding > > - that Init() won't be called multiple times > > - easier to understand (probably) > > > > Is that something we could investigate for ARC++? > > I'm planning to do some changes to the plumbing for ARC++ that might align us a > bit better with the overall servicification effort, but that's going to take > some time :( I agree 1:1 binding style is better. I'm happy to help once the team is ready to migrate.
PTAL https://codereview.chromium.org/2714433003/diff/60001/components/arc/common/f... File components/arc/common/file_system.mojom (right): https://codereview.chromium.org/2714433003/diff/60001/components/arc/common/f... components/arc/common/file_system.mojom:118: // (notified by OnDocumentChanged() with |type| = DELETED). On 2017/02/23 11:19:58, hidehiko wrote: > My understanding is; until this callback is called on Chrome, > OnDocumentChanged() still can be called even after RemoveWatcher(), right? If so > (or not), could you comment the timing explicitly? I added a comment to guarantee OnDocumentChanged() will be never called after this method returns.
LGTM. Defer to other reviewers.
mojo lgtm https://codereview.chromium.org/2714433003/diff/100001/components/arc/common/... File components/arc/common/file_system.mojom (right): https://codereview.chromium.org/2714433003/diff/100001/components/arc/common/... components/arc/common/file_system.mojom:53: [MinVersion=3] OnDocumentChanged@0(int64 watcher_id, ChangeType type); Btw, I usually prefer that the actual host be landed with this CL, but I guess testing might be hard, since the ARC++ side is not landed. Please add an IPC security reviewer on the followup that implements this. Thanks!
Thanks for reviewing! https://codereview.chromium.org/2714433003/diff/100001/components/arc/common/... File components/arc/common/file_system.mojom (right): https://codereview.chromium.org/2714433003/diff/100001/components/arc/common/... components/arc/common/file_system.mojom:53: [MinVersion=3] OnDocumentChanged@0(int64 watcher_id, ChangeType type); On 2017/02/24 06:00:42, dcheng wrote: > Btw, I usually prefer that the actual host be landed with this CL, but I guess > testing might be hard, since the ARC++ side is not landed. Please add an IPC > security reviewer on the followup that implements this. Thanks! Sure, I'll add you to relevant CLs. Thanks!
The CQ bit was checked by nya@chromium.org
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/2714433003/#ps120001 (title: "Add more fool-proof comment as per hidehiko's request in another CL.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1487916345386020,
"parent_rev": "616d2f430dc084729f71b275408df8ae9555f629", "commit_rev":
"533aed06060aba3689e57e04bb352f14cfc7ca25"}
Message was sent while issue was closed.
Description was changed from ========== mediaview: IPC definitions to watch for document changes. New IPC methods will allow Chrome to watch for changes in Android documents provider. This patch contains just mojom changes and minimal changes to FakeFileSystemInstance. Implementation of the fake is coming in the next patch. BUG=chromium:684233 TEST=trybot ========== to ========== mediaview: IPC definitions to watch for document changes. New IPC methods will allow Chrome to watch for changes in Android documents provider. This patch contains just mojom changes and minimal changes to FakeFileSystemInstance. Implementation of the fake is coming in the next patch. BUG=chromium:684233 TEST=trybot Review-Url: https://codereview.chromium.org/2714433003 Cr-Commit-Position: refs/heads/master@{#452775} Committed: https://chromium.googlesource.com/chromium/src/+/533aed06060aba3689e57e04bb35... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/533aed06060aba3689e57e04bb35... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
