Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2715473003:
mediaview: Support watchers in ArcDocumentsProviderRoot. (Closed)
|
Created:
3 years, 10 months ago by Shuhei Takahashi Modified:
3 years, 9 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, tzik, hidehiko+watch_chromium.org, nhiroki, lhchavez+watch_chromium.org, oshima+watch_chromium.org, kinuko+fileapi, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmediaview: Support watchers in ArcDocumentsProviderRoot.
Watchers are not always correct by design. See the long comments at
AddWatcher().
BUG=chromium:684233
TEST=trybot
Review-Url: https://codereview.chromium.org/2715473003
Cr-Commit-Position: refs/heads/master@{#454590}
Committed: https://chromium.googlesource.com/chromium/src/+/7049abb88b1e21acdd6345400183cd1df7762a84
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : Review ready. #
Total comments: 4
Patch Set 4 : . #Patch Set 5 : . #Patch Set 6 : . #Patch Set 7 : Addressed hidehiko's comments. #Patch Set 8 : Rebased on latest patch of https://codereview.chromium.org/2715493002/. #
Total comments: 10
Patch Set 9 : Addressed dcheng's comments. #
Depends on Patchset: Messages
Total messages: 39 (28 generated)
Description was changed from ========== wip BUG= ========== to ========== mediaview: Support watchers in ArcDocumentsProviderRoot. 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...
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...
Description was changed from ========== mediaview: Support watchers in ArcDocumentsProviderRoot. BUG=chromium:684233 TEST=trybot ========== to ========== mediaview: Support watchers in ArcDocumentsProviderRoot. TODO: Write CL description BUG=chromium:684233 TEST=trybot ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== mediaview: Support watchers in ArcDocumentsProviderRoot. TODO: Write CL description BUG=chromium:684233 TEST=trybot ========== to ========== mediaview: Support watchers in ArcDocumentsProviderRoot. Watchers are not always correct by design. See the long comments at AddWatcher(). BUG=chromium:684233 TEST=trybot ==========
nya@chromium.org changed reviewers: + hidehiko@chromium.org
hidehiko: PTAL
https://codereview.chromium.org/2715473003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.h (right): https://codereview.chromium.org/2715473003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.h:68: // - Racy file system operations. Could you add a simple racy operations as an example? https://codereview.chromium.org/2715473003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc (right): https://codereview.chromium.org/2715473003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc:362: mojom::ChangeType::CHANGED); DELETED?
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.
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...
PTAL https://codereview.chromium.org/2715473003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.h (right): https://codereview.chromium.org/2715473003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.h:68: // - Racy file system operations. On 2017/02/23 12:10:54, hidehiko wrote: > Could you add a simple racy operations as an example? Sure, added more examples. Also I added mention of a notable case about directory moves (I forgot to add this once). https://codereview.chromium.org/2715473003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc (right): https://codereview.chromium.org/2715473003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc:362: mojom::ChangeType::CHANGED); On 2017/02/23 12:10:54, hidehiko wrote: > DELETED? Ugh, I somehow failed to run unit tests for this CL. Sorry about that.
nya@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng for security review. Sorry for not adding you initially. It seems https://codereview.chromium.org/2715493002/ is getting less and less self-contained.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2715473003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc (right): https://codereview.chromium.org/2715473003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:106: callback.Run(base::File::FILE_ERROR_FAILED); This is called by... Chromium code? Would it make sense to DCHECK this instead? https://codereview.chromium.org/2715473003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:120: if (iter == path_to_watcher_id_.end()) { Ditto: if this is something called by code within Chromium, I think it should probably be a DCHECK? https://codereview.chromium.org/2715473003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:152: iter != path_to_watcher_id_.end(); ++iter) { Nit: for (auto& it : path_to_watcher_id_) it->second = -1; https://codereview.chromium.org/2715473003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:241: if (document_id.empty()) { DCHECK if this is only called by Chromium code? https://codereview.chromium.org/2715473003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.h (right): https://codereview.chromium.org/2715473003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.h:85: // at X will always success. Nit: success => succeed
https://codereview.chromium.org/2715473003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc (right): https://codereview.chromium.org/2715473003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:106: callback.Run(base::File::FILE_ERROR_FAILED); On 2017/03/03 09:40:56, dcheng wrote: > This is called by... Chromium code? Would it make sense to DCHECK this instead? It is called by Chromium code, but the upstream explicitly requires us to return errors when it was attempted to install multiple watchers to the same path. So we can't DCHECK here unfortunately. https://cs.chromium.org/chromium/src/storage/browser/fileapi/watcher_manager.... https://codereview.chromium.org/2715473003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:120: if (iter == path_to_watcher_id_.end()) { On 2017/03/03 09:40:56, dcheng wrote: > Ditto: if this is something called by code within Chromium, I think it should > probably be a DCHECK? Same as above. https://codereview.chromium.org/2715473003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:152: iter != path_to_watcher_id_.end(); ++iter) { On 2017/03/03 09:40:56, dcheng wrote: > Nit: > for (auto& it : path_to_watcher_id_) > it->second = -1; Ah yes, we can do that. Thanks! https://codereview.chromium.org/2715473003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:241: if (document_id.empty()) { On 2017/03/03 09:40:56, dcheng wrote: > DCHECK if this is only called by Chromium code? This can happen when AddWatcher() is called to non-existent files, which can happen in race conditions. https://codereview.chromium.org/2715473003/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.h (right): https://codereview.chromium.org/2715473003/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.h:85: // at X will always success. On 2017/03/03 09:40:56, dcheng wrote: > Nit: success => succeed Oops, I always make this mistake :)
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...
LGTM, thanks for the explanations
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by nya@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2715493002 Patch 240001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by nya@chromium.org
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": 160001, "attempt_start_ts": 1488555730169140,
"parent_rev": "1cf2cd5fd31329f2cb7de2cd2df919648b5e331b", "commit_rev":
"7049abb88b1e21acdd6345400183cd1df7762a84"}
Message was sent while issue was closed.
Description was changed from ========== mediaview: Support watchers in ArcDocumentsProviderRoot. Watchers are not always correct by design. See the long comments at AddWatcher(). BUG=chromium:684233 TEST=trybot ========== to ========== mediaview: Support watchers in ArcDocumentsProviderRoot. Watchers are not always correct by design. See the long comments at AddWatcher(). BUG=chromium:684233 TEST=trybot Review-Url: https://codereview.chromium.org/2715473003 Cr-Commit-Position: refs/heads/master@{#454590} Committed: https://chromium.googlesource.com/chromium/src/+/7049abb88b1e21acdd6345400183... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/7049abb88b1e21acdd6345400183... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
