Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2572683004:
mediaview: Introduce ArcDocumentsProviderRoot. (Closed)
|
Created:
4 years ago by Shuhei Takahashi Modified:
4 years 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: Introduce ArcDocumentsProviderRoot.
On file system operations, ArcDocumentsProviderAsyncFileUtil
will look up a corresponding ArcDocumentsProviderRoot instance
from ArcDocumentsProviderRootMap and delegates operations to
it.
ArcDocumentsProviderRootMap is owned by
ArcDocumentsProviderBackendDelegate because the delegate will
also use the root map to implement its methods soon.
BUG=chromium:671511
TEST=unit_tests --gtest_filter='Arc*'
TEST=trybot
Committed: https://crrev.com/29942ba33818a9db2fc90a441b80d56cbdbc1f1f
Cr-Commit-Position: refs/heads/master@{#439090}
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : Remove unused functions. #
Total comments: 19
Patch Set 4 : Addressed lhchavez's comments. #
Total comments: 22
Patch Set 5 : Addressed hashimoto's comments. #Patch Set 6 : Removed WeakPtrFactory. #Patch Set 7 : Assume cancel-on-delete contract. #Patch Set 8 : Fix failing unit tests. #
Total comments: 10
Patch Set 9 : Addressed lhchavez's comments. #
Total comments: 4
Patch Set 10 : Address hashimoto's comment. #Patch Set 11 : Constructors are called on the UI thread. #Depends on Patchset: Dependent Patchsets: Messages
Total messages: 54 (32 generated)
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.
nya@chromium.org changed reviewers: + hashimoto@chromium.org, yusukes@chromium.org
yusukes, hashimoto: PTAL
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: + lhchavez@chromium.org - yusukes@chromium.org
Oops, I meant lhchavez. lhchavez, hashimoto: PTAL yusukes: FYI
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2572683004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc (right): https://codereview.chromium.org/2572683004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc:110: base::Unretained(this), base::Owned(context.release()), Who will be responsible of deleting |context|, then? IIUC it is currently leaking since you're not even giving the pointer to the callback. Shouldn't you at least store |context| somewhere in |this| (say, in a std::vector or std::unordered_map) and then remove it from there if |has_more| = false in OnReadDirectory? https://codereview.chromium.org/2572683004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.h (right): https://codereview.chromium.org/2572683004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.h:107: ArcDocumentsProviderRootMap* roots_; Please document who owns this pointer. Also, it seems to be possible to make this ArcDocumentsProviderRootMap* const roots_; https://codereview.chromium.org/2572683004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_map.cc (right): https://codereview.chromium.org/2572683004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_map.cc:42: auto iter = map_.find(std::make_pair(authority, root_document_id)); nit: does it work with s/std::make_pair/Key/? https://codereview.chromium.org/2572683004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_map.cc:43: if (iter == map_.end()) { nit: elide braces. Same in L54. https://codereview.chromium.org/2572683004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc (right): https://codereview.chromium.org/2572683004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc:25: if (url.type() != storage::kFileSystemTypeArcDocumentsProvider) { nit: elide braces https://codereview.chromium.org/2572683004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc:34: if (components.size() < 5 || components[0] != FILE_PATH_LITERAL("/") || re: components[0] != FILE_PATH_LITERAL("/"). Does it work if you check if url.path().IsAbsolute() instead? https://codereview.chromium.org/2572683004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc:44: DCHECK(!path->IsAbsolute()); nit: compute the path in a temporary and move the DCHECK before L40, so you only modify the parameters in the success case. https://codereview.chromium.org/2572683004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.h (right): https://codereview.chromium.org/2572683004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.h:21: constexpr char kDocumentsProviderMountPointName[] = "arc-documents-provider"; The convention is to declare string constants as "extern const char kXxx[];" in the headers and then define them in the .cc file: https://cs.chromium.org/chromium/src/components/arc/intent_helper/intent_cons... https://codereview.chromium.org/2572683004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util_unittest.cc (right): https://codereview.chromium.org/2572683004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util_unittest.cc:87: base::FilePath("/special/something-else/cats/root/home/calico.jpg")), Can you add some paths with Japanese to exercise the UTF-8 conversion of the code? (Maybe I'm being paranoid, but the "Unsafe" UTF8 conversion scared me).
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...
https://codereview.chromium.org/2572683004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc (right): https://codereview.chromium.org/2572683004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc:110: base::Unretained(this), base::Owned(context.release()), On 2016/12/14 20:53:50, Luis Héctor Chávez wrote: > Who will be responsible of deleting |context|, then? IIUC it is currently > leaking since you're not even giving the pointer to the callback. > > Shouldn't you at least store |context| somewhere in |this| (say, in a > std::vector or std::unordered_map) and then remove it from there if |has_more| = > false in OnReadDirectory? Hmm, maybe. I've changed the code to explicitly reset a unique_ptr. In this way, even in the worst case, deleting the last callback instance will release the context. https://codereview.chromium.org/2572683004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.h (right): https://codereview.chromium.org/2572683004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.h:107: ArcDocumentsProviderRootMap* roots_; On 2016/12/14 20:53:50, Luis Héctor Chávez wrote: > Please document who owns this pointer. Also, it seems to be possible to make > this > > ArcDocumentsProviderRootMap* const roots_; Done. https://codereview.chromium.org/2572683004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_map.cc (right): https://codereview.chromium.org/2572683004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_map.cc:42: auto iter = map_.find(std::make_pair(authority, root_document_id)); On 2016/12/14 20:53:50, Luis Héctor Chávez wrote: > nit: does it work with s/std::make_pair/Key/? Done. https://codereview.chromium.org/2572683004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_map.cc:43: if (iter == map_.end()) { On 2016/12/14 20:53:50, Luis Héctor Chávez wrote: > nit: elide braces. Same in L54. Done. https://codereview.chromium.org/2572683004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc (right): https://codereview.chromium.org/2572683004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc:25: if (url.type() != storage::kFileSystemTypeArcDocumentsProvider) { On 2016/12/14 20:53:50, Luis Héctor Chávez wrote: > nit: elide braces Done. https://codereview.chromium.org/2572683004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc:34: if (components.size() < 5 || components[0] != FILE_PATH_LITERAL("/") || On 2016/12/14 20:53:50, Luis Héctor Chávez wrote: > re: components[0] != FILE_PATH_LITERAL("/"). Does it work if you check if > url.path().IsAbsolute() instead? Strictly, those two conditions are different. If the path is "//foo/bar", components[0] is "//", while IsAbsolute() = true. https://cs.chromium.org/chromium/src/base/files/file_path_unittest.cc?type=cs... https://cs.chromium.org/chromium/src/base/files/file_path_unittest.cc?type=cs... https://codereview.chromium.org/2572683004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc:44: DCHECK(!path->IsAbsolute()); On 2016/12/14 20:53:50, Luis Héctor Chávez wrote: > nit: compute the path in a temporary and move the DCHECK before L40, so you only > modify the parameters in the success case. Hmm, if we return false in the failure case it makes sense to defer assigning to |path|, but this is DCHECK() for sanity check. It may crash chrome on failure, but then no one cares if |path| is assigned or not at this point. https://codereview.chromium.org/2572683004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.h (right): https://codereview.chromium.org/2572683004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.h:21: constexpr char kDocumentsProviderMountPointName[] = "arc-documents-provider"; On 2016/12/14 20:53:51, Luis Héctor Chávez wrote: > The convention is to declare string constants as "extern const char kXxx[];" in > the headers and then define them in the .cc file: > https://cs.chromium.org/chromium/src/components/arc/intent_helper/intent_cons... I think it's rather C++03 convention, but I'm fine with it anyway. https://codereview.chromium.org/2572683004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util_unittest.cc (right): https://codereview.chromium.org/2572683004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util_unittest.cc:87: base::FilePath("/special/something-else/cats/root/home/calico.jpg")), On 2016/12/14 20:53:51, Luis Héctor Chávez wrote: > Can you add some paths with Japanese to exercise the UTF-8 conversion of the > code? (Maybe I'm being paranoid, but the "Unsafe" UTF8 conversion scared me). Sure, done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc (right): https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc:83: // It is guaranteed that |this| outlives |context|, so keep > "It is guaranteed that |this| outlives |context|" I don't understand why this becomes the reason to keep the context object alive. Why do you need to do this? https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc:85: base::Unretained(this), base::Passed(std::move(context)), Why don't you use the newly added WeakPtrFactory here? https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc:107: // It is guaranteed that |this| outlives |context|, so keep ditto. https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc:111: base::Unretained(this), ditto. https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc (right): https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:16: const std::string& root_document_id) {} = default; https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_map.h (right): https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_map.h:36: const std::string& root_document_id); const? https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_map.h:42: base::FilePath* path); const? https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc (right): https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc:46: Components(components.begin() + 5, components.end()), "/")); How about using FilePath::AppendRelativePath? https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util_unittest.cc (right): https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util_unittest.cc:24: "/special/arc-documents-provider/cats/root/home/calico.jpg")), 1. Why don't you use kDocumentsProviderMountPointPath? 2. This should be FILE_PATH_LITERAL (the same goes for other places). https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util_unittest.cc:74: "special/arc-documents-provider/cats/root/home/calico.jpg")), Please add comments to make it clear what makes each ParseDocumentsProviderUrl() call return false.
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...
https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc (right): https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc:83: // It is guaranteed that |this| outlives |context|, so keep On 2016/12/15 03:53:22, hashimoto wrote: > > "It is guaranteed that |this| outlives |context|" > I don't understand why this becomes the reason to keep the context object alive. > Why do you need to do this? From AsyncFileUtil comment: // As far as an instance of this class is owned by a FileSystemBackend // (which is owned by FileSystemContext), it's guaranteed that this instance's // alive while FileSystemOperationContext given to each operation is kept // alive. (Note that this instance might be freed on different thread // from the thread it is created.) https://cs.chromium.org/chromium/src/storage/browser/fileapi/async_file_util.... So the comment guarantees that, as long as we keep |context| alive, |this| is also kept alive. Other than that it says nothing; in particular, about canceling operations. So, assuming this comment is the only specification we can rely on, it is *wrong* to use WeakPtr here because we are not explicitly allowed to cancel calling |callback| after |this| is deleted, and the only way to do the business correctly is to keep |context| (and |this|) alive until the operation finishes and invoke |callback| all the time. If you know any other explicit contract we can rely on, please let me know. https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc:85: base::Unretained(this), base::Passed(std::move(context)), On 2016/12/15 03:53:22, hashimoto wrote: > Why don't you use the newly added WeakPtrFactory here? Ditto. https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc:107: // It is guaranteed that |this| outlives |context|, so keep On 2016/12/15 03:53:21, hashimoto wrote: > ditto. Ditto. https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc:111: base::Unretained(this), On 2016/12/15 03:53:22, hashimoto wrote: > ditto. Ditto. https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc (right): https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:16: const std::string& root_document_id) {} On 2016/12/15 03:53:22, hashimoto wrote: > = default; A constructor with arguments can't be defaulted. https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_map.h (right): https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_map.h:36: const std::string& root_document_id); On 2016/12/15 03:53:22, hashimoto wrote: > const? Done. https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_map.h:42: base::FilePath* path); On 2016/12/15 03:53:22, hashimoto wrote: > const? Done. https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc (right): https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc:46: Components(components.begin() + 5, components.end()), "/")); On 2016/12/15 03:53:22, hashimoto wrote: > How about using FilePath::AppendRelativePath? Thanks, that's what I wanted. https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util_unittest.cc (right): https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util_unittest.cc:24: "/special/arc-documents-provider/cats/root/home/calico.jpg")), On 2016/12/15 03:53:22, hashimoto wrote: > 1. Why don't you use kDocumentsProviderMountPointPath? > 2. This should be FILE_PATH_LITERAL (the same goes for other places). Done. https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util_unittest.cc:74: "special/arc-documents-provider/cats/root/home/calico.jpg")), On 2016/12/15 03:53:22, hashimoto wrote: > Please add comments to make it clear what makes each ParseDocumentsProviderUrl() > call return false. Done.
https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc (right): https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc:83: // It is guaranteed that |this| outlives |context|, so keep On 2016/12/15 05:21:53, Shuhei Takahashi wrote: > On 2016/12/15 03:53:22, hashimoto wrote: > > > "It is guaranteed that |this| outlives |context|" > > I don't understand why this becomes the reason to keep the context object > alive. > > Why do you need to do this? > > From AsyncFileUtil comment: > > // As far as an instance of this class is owned by a FileSystemBackend > // (which is owned by FileSystemContext), it's guaranteed that this instance's > // alive while FileSystemOperationContext given to each operation is kept > // alive. (Note that this instance might be freed on different thread > // from the thread it is created.) > > https://cs.chromium.org/chromium/src/storage/browser/fileapi/async_file_util.... > > > So the comment guarantees that, as long as we keep |context| alive, |this| is > also kept alive. Other than that it says nothing; in particular, about canceling > operations. So, assuming this comment is the only specification we can rely on, > it is *wrong* to use WeakPtr here because we are not explicitly allowed to > cancel calling |callback| after |this| is deleted, and the only way to do the > business correctly is to keep |context| (and |this|) alive until the operation > finishes and invoke |callback| all the time. > > If you know any other explicit contract we can rely on, please let me know. Oh, BTW, you're right that weak_ptr_factory_ is useless. I removed it.
https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc (right): https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc:83: // It is guaranteed that |this| outlives |context|, so keep On 2016/12/15 05:26:15, Shuhei Takahashi wrote: > On 2016/12/15 05:21:53, Shuhei Takahashi wrote: > > On 2016/12/15 03:53:22, hashimoto wrote: > > > > "It is guaranteed that |this| outlives |context|" > > > I don't understand why this becomes the reason to keep the context object > > alive. > > > Why do you need to do this? > > > > From AsyncFileUtil comment: > > > > // As far as an instance of this class is owned by a FileSystemBackend > > // (which is owned by FileSystemContext), it's guaranteed that this > instance's > > // alive while FileSystemOperationContext given to each operation is kept > > // alive. (Note that this instance might be freed on different thread > > // from the thread it is created.) > > > > > https://cs.chromium.org/chromium/src/storage/browser/fileapi/async_file_util.... Sorry, I still don't understand why the said comment becomes the reason to keep FileSystemOperationContext alive. Existing implementations don't keep FileSystemOperationContext alive if unnecessary. The said comment was added in https://chromiumcodereview.appspot.com/16413007. What it says is just the fact that AsyncFileUtil doesn't die as long as FileSystemOperationContext is alive because FileSystemOperationContext has a scoped_refptr of FileSystemContext which owns FileSystemBackend. > > > > > > So the comment guarantees that, as long as we keep |context| alive, |this| is > > also kept alive. Other than that it says nothing; in particular, about > canceling > > operations. So, assuming this comment is the only specification we can rely > on, > > it is *wrong* to use WeakPtr here because we are not explicitly allowed to > > cancel calling |callback| after |this| is deleted, and the only way to do the > > business correctly is to keep |context| (and |this|) alive until the operation > > finishes and invoke |callback| all the time. > > > > If you know any other explicit contract we can rely on, please let me know. In Chrome, when destroying an object, usually you shouldn't expect the callback to get called for any in-flight operations. > > Oh, BTW, you're right that weak_ptr_factory_ is useless. I removed it.
On 2016/12/15 10:34:58, hashimoto wrote: > https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... > File > chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc > (right): > > https://codereview.chromium.org/2572683004/diff/60001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc:83: > // It is guaranteed that |this| outlives |context|, so keep > On 2016/12/15 05:26:15, Shuhei Takahashi wrote: > > On 2016/12/15 05:21:53, Shuhei Takahashi wrote: > > > On 2016/12/15 03:53:22, hashimoto wrote: > > > > > "It is guaranteed that |this| outlives |context|" > > > > I don't understand why this becomes the reason to keep the context object > > > alive. > > > > Why do you need to do this? > > > > > > From AsyncFileUtil comment: > > > > > > // As far as an instance of this class is owned by a FileSystemBackend > > > // (which is owned by FileSystemContext), it's guaranteed that this > > instance's > > > // alive while FileSystemOperationContext given to each operation is kept > > > // alive. (Note that this instance might be freed on different thread > > > // from the thread it is created.) > > > > > > > > > https://cs.chromium.org/chromium/src/storage/browser/fileapi/async_file_util.... > Sorry, I still don't understand why the said comment becomes the reason to keep > FileSystemOperationContext alive. > Existing implementations don't keep FileSystemOperationContext alive if > unnecessary. > > The said comment was added in https://chromiumcodereview.appspot.com/16413007. > What it says is just the fact that AsyncFileUtil doesn't die as long as > FileSystemOperationContext is alive because FileSystemOperationContext has a > scoped_refptr of FileSystemContext which owns FileSystemBackend. > > > > > > > > > So the comment guarantees that, as long as we keep |context| alive, |this| > is > > > also kept alive. Other than that it says nothing; in particular, about > > canceling > > > operations. So, assuming this comment is the only specification we can rely > > on, > > > it is *wrong* to use WeakPtr here because we are not explicitly allowed to > > > cancel calling |callback| after |this| is deleted, and the only way to do > the > > > business correctly is to keep |context| (and |this|) alive until the > operation > > > finishes and invoke |callback| all the time. > > > > > > If you know any other explicit contract we can rely on, please let me know. > In Chrome, when destroying an object, usually you shouldn't expect the callback > to get called for any in-flight operations. OK, I will trust you rather than code comments. Would you mind pointing me to some document describing the generic contact you mentioned? > > > > Oh, BTW, you're right that weak_ptr_factory_ is useless. I removed it.
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 Now ArcDPAsyncFileUtil uses WeakPtrFactory.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
Fixed unit tests which failed due to FilePath::AppendRelativePath.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with nits https://codereview.chromium.org/2572683004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc (right): https://codereview.chromium.org/2572683004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc:34: if (components.size() < 5 || components[0] != FILE_PATH_LITERAL("/") || On 2016/12/15 02:37:55, Shuhei Takahashi wrote: > On 2016/12/14 20:53:50, Luis Héctor Chávez wrote: > > re: components[0] != FILE_PATH_LITERAL("/"). Does it work if you check if > > url.path().IsAbsolute() instead? > > Strictly, those two conditions are different. If the path is "//foo/bar", > components[0] is "//", while IsAbsolute() = true. > > https://cs.chromium.org/chromium/src/base/files/file_path_unittest.cc?type=cs... > https://cs.chromium.org/chromium/src/base/files/file_path_unittest.cc?type=cs... Got it, thanks for your explanation. https://codereview.chromium.org/2572683004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc (right): https://codereview.chromium.org/2572683004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc:9: #include "base/bind.h" This seems to be unused. https://codereview.chromium.org/2572683004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc:10: #include "base/callback.h" This is already added in the .h https://codereview.chromium.org/2572683004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc (right): https://codereview.chromium.org/2572683004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:16: const std::string& root_document_id) {} nit: = default (or DCHECK_CURRENTLY_ON(BrowserThread::IO); ) https://codereview.chromium.org/2572683004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_map.h (right): https://codereview.chromium.org/2572683004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_map.h:35: ArcDocumentsProviderRoot* Lookup(const std::string& authority, I couldn't find any calls for this function from anywhere outside this class in either this or the next CL. I don't exactly have strong opinions on this, but how about you move it to the anonymous namespace and pass in |map_| as const-reference? (or just move this implementation into ParseAndLookup). https://codereview.chromium.org/2572683004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc (right): https://codereview.chromium.org/2572683004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc:16: const base::FilePath::CharType kSpecialDirectory[] = nit: can this be constexpr?
Thanks Luis! https://codereview.chromium.org/2572683004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc (right): https://codereview.chromium.org/2572683004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc:9: #include "base/bind.h" On 2016/12/15 21:26:10, Luis Héctor Chávez wrote: > This seems to be unused. Done. https://codereview.chromium.org/2572683004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc:10: #include "base/callback.h" On 2016/12/15 21:26:10, Luis Héctor Chávez wrote: > This is already added in the .h Done. https://codereview.chromium.org/2572683004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc (right): https://codereview.chromium.org/2572683004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:16: const std::string& root_document_id) {} On 2016/12/15 21:26:10, Luis Héctor Chávez wrote: > nit: = default > > (or DCHECK_CURRENTLY_ON(BrowserThread::IO); ) Added DCHECK. FYI, a constructor with arguments can't be marked default. https://codereview.chromium.org/2572683004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_map.h (right): https://codereview.chromium.org/2572683004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_map.h:35: ArcDocumentsProviderRoot* Lookup(const std::string& authority, On 2016/12/15 21:26:10, Luis Héctor Chávez wrote: > I couldn't find any calls for this function from anywhere outside this class in > either this or the next CL. I don't exactly have strong opinions on this, but > how about you move it to the anonymous namespace and pass in |map_| as > const-reference? (or just move this implementation into ParseAndLookup). Makes sense. I've just moved the implementation into ParseAndLoopup. Thanks! https://codereview.chromium.org/2572683004/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc (right): https://codereview.chromium.org/2572683004/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc:16: const base::FilePath::CharType kSpecialDirectory[] = On 2016/12/15 21:26:10, Luis Héctor Chávez wrote: > nit: can this be constexpr? Of course.
lgtm https://codereview.chromium.org/2572683004/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.h (right): https://codereview.chromium.org/2572683004/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.h:10: #include "base/memory/weak_ptr.h" weak_ptr_factory_ looks unused. https://codereview.chromium.org/2572683004/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc (right): https://codereview.chromium.org/2572683004/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc:38: components[2] != kDocumentsProviderMountPointName) { How about using base::FilePath::IsParent() instead of checking components[0..2]?
https://codereview.chromium.org/2572683004/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.h (right): https://codereview.chromium.org/2572683004/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.h:10: #include "base/memory/weak_ptr.h" On 2016/12/16 09:03:52, hashimoto wrote: > weak_ptr_factory_ looks unused. Done. https://codereview.chromium.org/2572683004/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc (right): https://codereview.chromium.org/2572683004/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.cc:38: components[2] != kDocumentsProviderMountPointName) { On 2016/12/16 09:03:52, hashimoto wrote: > How about using base::FilePath::IsParent() instead of checking components[0..2]? That sounds better!
The CQ bit was checked by nya@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, hashimoto@chromium.org Link to the patchset: https://codereview.chromium.org/2572683004/#ps180001 (title: "Address hashimoto's comment.")
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
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/12/16 10:37:40, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux > (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) It seems constructors are called on the UI thread. I removed DCHECK.
The CQ bit was checked by nya@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, hashimoto@chromium.org Link to the patchset: https://codereview.chromium.org/2572683004/#ps200001 (title: "Constructors are called on the UI thread.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
For the record, it seems out backend delegates are constructed on UI thread. Check failed: ::content::BrowserThread::CurrentlyOn(BrowserThread::IO). Must be called on Chrome_IOThread; actually called on CrBrowserMain. #0 0x0000032fb83e base::debug::StackTrace::StackTrace() #1 0x000003312cba logging::LogMessage::~LogMessage() #2 0x000001796b96 arc::ArcDocumentsProviderRoot::ArcDocumentsProviderRoot() #3 0x000001797138 _ZN4base10MakeUniqueIN3arc24ArcDocumentsProviderRootEJRPKcS5_EEENS_8internal16MakeUniqueResultIT_E6ScalarEDpOT0_ #4 0x000001797016 arc::ArcDocumentsProviderRootMap::ArcDocumentsProviderRootMap() #5 0x0000017963dd arc::ArcDocumentsProviderBackendDelegate::ArcDocumentsProviderBackendDelegate() #6 0x00000340a20a ChromeContentBrowserClient::GetAdditionalFileSystemBackends() #7 0x00000299951a content::CreateFileSystemContext() #8 0x000002c6291c content::StoragePartitionImpl::Create() #9 0x000002c66e1f content::StoragePartitionImplMap::Get() #10 0x0000028ca1ce content::BrowserContext::GetStoragePartition() #11 0x0000034f0389 ProfileImpl::DoFinalInit() #12 0x0000034f1f63 ProfileImpl::OnLocaleReady() #13 0x0000034ef9e9 ProfileImpl::OnPrefsLoaded() #14 0x0000034ef803 ProfileImpl::ProfileImpl() #15 0x0000034ede9d Profile::CreateProfile() #16 0x00000350c289 ProfileManager::CreateProfileHelper() #17 0x000003505feb ProfileManager::CreateAndInitializeProfile() #18 0x000003505d31 ProfileManager::GetProfile() #19 0x000003505aa9 ProfileManager::GetActiveUserOrOffTheRecordProfileFromPath() #20 0x000003505c3c ProfileManager::GetActiveUserProfile() #21 0x0000033ffc1e ChromeBrowserMainParts::PreMainMessageLoopRunImpl() #22 0x0000033ff2f1 ChromeBrowserMainParts::PreMainMessageLoopRun() #23 0x0000017c924a chromeos::ChromeBrowserMainPartsChromeos::PreMainMessageLoopRun() #24 0x0000028d0df5 content::BrowserMainLoop::PreMainMessageLoopRun() #25 0x000002c618f6 content::StartupTaskRunner::RunAllTasksNow() #26 0x0000028cec7b content::BrowserMainLoop::CreateStartupTasks() #27 0x0000028d3b3a content::BrowserMainRunnerImpl::Initialize() #28 0x0000028cc1e8 content::BrowserMain() #29 0x000003299f1c content::ContentMainRunnerImpl::Run() #30 0x000003298a10 content::ContentMain() #31 0x0000039806b7 content::BrowserTestBase::SetUp() #32 0x0000033c2be9 InProcessBrowserTest::SetUp() #33 0x0000040f8ee8 testing::Test::Run() #34 0x0000040f9df0 testing::TestInfo::Run() #35 0x0000040fa287 testing::TestCase::Run() #36 0x000004101547 testing::internal::UnitTestImpl::RunAllTests() #37 0x00000410115a testing::UnitTest::Run() #38 0x0000033d1291 base::TestSuite::Run() #39 0x0000032ee6ba (anonymous namespace)::MashTestLauncherDelegate::RunTestSuite() #40 0x0000039ad549 content::LaunchTests() #41 0x0000032ee1c7 RunMashBrowserTests() #42 0x0000032edf83 main #43 0x7f93054f3f45 __libc_start_main #44 0x000000626811 <unknown> https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1481885767274600,
"parent_rev": "ef04064b81b615a18e87e48bad75398a331922e0", "commit_rev":
"f40be051b3f319b35876ef1ebdfd458d4cdf99d9"}
Message was sent while issue was closed.
Description was changed from ========== mediaview: Introduce ArcDocumentsProviderRoot. On file system operations, ArcDocumentsProviderAsyncFileUtil will look up a corresponding ArcDocumentsProviderRoot instance from ArcDocumentsProviderRootMap and delegates operations to it. ArcDocumentsProviderRootMap is owned by ArcDocumentsProviderBackendDelegate because the delegate will also use the root map to implement its methods soon. BUG=chromium:671511 TEST=unit_tests --gtest_filter='Arc*' TEST=trybot ========== to ========== mediaview: Introduce ArcDocumentsProviderRoot. On file system operations, ArcDocumentsProviderAsyncFileUtil will look up a corresponding ArcDocumentsProviderRoot instance from ArcDocumentsProviderRootMap and delegates operations to it. ArcDocumentsProviderRootMap is owned by ArcDocumentsProviderBackendDelegate because the delegate will also use the root map to implement its methods soon. BUG=chromium:671511 TEST=unit_tests --gtest_filter='Arc*' TEST=trybot Review-Url: https://codereview.chromium.org/2572683004 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== mediaview: Introduce ArcDocumentsProviderRoot. On file system operations, ArcDocumentsProviderAsyncFileUtil will look up a corresponding ArcDocumentsProviderRoot instance from ArcDocumentsProviderRootMap and delegates operations to it. ArcDocumentsProviderRootMap is owned by ArcDocumentsProviderBackendDelegate because the delegate will also use the root map to implement its methods soon. BUG=chromium:671511 TEST=unit_tests --gtest_filter='Arc*' TEST=trybot Review-Url: https://codereview.chromium.org/2572683004 ========== to ========== mediaview: Introduce ArcDocumentsProviderRoot. On file system operations, ArcDocumentsProviderAsyncFileUtil will look up a corresponding ArcDocumentsProviderRoot instance from ArcDocumentsProviderRootMap and delegates operations to it. ArcDocumentsProviderRootMap is owned by ArcDocumentsProviderBackendDelegate because the delegate will also use the root map to implement its methods soon. BUG=chromium:671511 TEST=unit_tests --gtest_filter='Arc*' TEST=trybot Committed: https://crrev.com/29942ba33818a9db2fc90a441b80d56cbdbc1f1f Cr-Commit-Position: refs/heads/master@{#439090} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/29942ba33818a9db2fc90a441b80d56cbdbc1f1f Cr-Commit-Position: refs/heads/master@{#439090} |
