Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2574173002:
mediaview: Implement 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: Implement ArcDocumentsProviderRoot.
BUG=chromium:671511
TEST=unit_tests --gtest_filter='Arc*'
TEST=trybot
Committed: https://crrev.com/787e1c859432963c8efe3525a7ad1cc340fa66a5
Cr-Commit-Position: refs/heads/master@{#440020}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Address lhchavez's comments. #
Total comments: 18
Patch Set 3 : Address lhchavez's comments. #Patch Set 4 : Constructors are called on the UI thread. #Patch Set 5 : ThreadChecker failure fix + misc. #Patch Set 6 : Rebased to ToT. #
Total comments: 18
Patch Set 7 : WIP: Remove cache layer. #Patch Set 8 : Revive unit tests & fixed build breakage. #
Total comments: 25
Patch Set 9 : Avoid long ifs. #Patch Set 10 : Addressed hashimoto's comments. #Patch Set 11 : Missing diffs in PS10... #
Total comments: 2
Patch Set 12 : Addressed hashimoto's comments. #Messages
Total messages: 52 (37 generated)
Description was changed from ========== mediaview: hoge BUG=chromium:671511 TEST=unit_tests --gtest_filter='Arc*' TEST=trybot ========== to ========== mediaview: Implement ArcDocumentsProviderRoot. BUG=chromium:671511 TEST=unit_tests --gtest_filter='Arc*' 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: + hashimoto@chromium.org, lhchavez@chromium.org
lhchavez, hashimoto: PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
round 1, i'll finish tomorrow. https://codereview.chromium.org/2574173002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.cc (right): https://codereview.chromium.org/2574173002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.cc:35: if (path.empty()) { nit: elide braces for all the one-line if blocks. https://codereview.chromium.org/2574173002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.cc:61: std::set<mojom::Document*> processed; How large do you think these maps/sets are going to get? Would it make sense to use unordered_{set,map}? https://codereview.chromium.org/2574173002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.cc:71: if (iter != reverse_old_map.end()) { nit: if (iter == reverse_old_map.end()) continue; https://codereview.chromium.org/2574173002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.cc:79: std::map<base::FilePath::StringType, int> name_counters; Can you also wrap this in a block to ensure |name_counters| is destroyed as soon as it's not needed? (similar to what you did for |reverse_old_map|). https://codereview.chromium.org/2574173002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.cc:115: ++suffix_counter; Why not move this above L114? That way you don't need L110-L112. https://codereview.chromium.org/2574173002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.h (right): https://codereview.chromium.org/2574173002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.h:26: // WARNING: This class is thread-unsafe. Must be accessed only from a single Can you add one of these? https://cs.chromium.org/chromium/src/base/threading/thread_checker.h?type=cs&... https://codereview.chromium.org/2574173002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.h:41: const std::string& document_id() { return document_id_; } You can make all of the inlined accessors const: const std::string& document_id() const { ... }
https://codereview.chromium.org/2574173002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.cc (right): https://codereview.chromium.org/2574173002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.cc:35: if (path.empty()) { On 2016/12/15 02:40:31, Luis Héctor Chávez wrote: > nit: elide braces for all the one-line if blocks. Done. https://codereview.chromium.org/2574173002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.cc:61: std::set<mojom::Document*> processed; On 2016/12/15 02:40:31, Luis Héctor Chávez wrote: > How large do you think these maps/sets are going to get? Would it make sense to > use unordered_{set,map}? It is the number of files in a single directory, so it will be typically not very large. BTW is unordered_set efficient in terms of memory? https://codereview.chromium.org/2574173002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.cc:71: if (iter != reverse_old_map.end()) { On 2016/12/15 02:40:31, Luis Héctor Chávez wrote: > nit: > > if (iter == reverse_old_map.end()) > continue; Done. https://codereview.chromium.org/2574173002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.cc:79: std::map<base::FilePath::StringType, int> name_counters; On 2016/12/15 02:40:31, Luis Héctor Chávez wrote: > Can you also wrap this in a block to ensure |name_counters| is destroyed as soon > as it's not needed? (similar to what you did for |reverse_old_map|). Done. https://codereview.chromium.org/2574173002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.cc:115: ++suffix_counter; On 2016/12/15 02:40:31, Luis Héctor Chávez wrote: > Why not move this above L114? That way you don't need L110-L112. Sounds like a good idea. https://codereview.chromium.org/2574173002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.h (right): https://codereview.chromium.org/2574173002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.h:26: // WARNING: This class is thread-unsafe. Must be accessed only from a single On 2016/12/15 02:40:31, Luis Héctor Chávez wrote: > Can you add one of these? > https://cs.chromium.org/chromium/src/base/threading/thread_checker.h?type=cs&... Sure. https://codereview.chromium.org/2574173002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.h:41: const std::string& document_id() { return document_id_; } On 2016/12/15 02:40:31, Luis Héctor Chávez wrote: > You can make all of the inlined accessors const: > > const std::string& document_id() const { ... } Done.
https://codereview.chromium.org/2574173002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.cc (right): https://codereview.chromium.org/2574173002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.cc:61: std::set<mojom::Document*> processed; On 2016/12/15 06:50:57, Shuhei Takahashi wrote: > On 2016/12/15 02:40:31, Luis Héctor Chávez wrote: > > How large do you think these maps/sets are going to get? Would it make sense > to > > use unordered_{set,map}? > > It is the number of files in a single directory, so it will be typically not > very large. > > BTW is unordered_set efficient in terms of memory? sort of. depends on the load factor. On 64-bit machines, overall it uses roughly the same amount of memory as a std::set, but for some sizes it uses less, for some it uses more. On 32-bit machines where the pointer overhead is smaller, the proportion of time it uses more memory might be larger, though. https://codereview.chromium.org/2574173002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.cc (right): https://codereview.chromium.org/2574173002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.cc:85: std::map<base::FilePath::StringType, int> name_counters; nit: wrap in a block for consistency? https://codereview.chromium.org/2574173002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.cc:174: base::StringPrintf("%s.%s", filename.c_str(), new_extension.c_str()); Given that you're already converting |filename| to a base::FilePath (in L160), can you use base::FilePath::ReplaceExtension() instead? https://codereview.chromium.org/2574173002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.h (right): https://codereview.chromium.org/2574173002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.h:63: static base::FilePath::StringType GetFileNameForDocument( nit: can this be in the anonymous namespace? https://codereview.chromium.org/2574173002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document_unittest.cc (right): https://codereview.chromium.org/2574173002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document_unittest.cc:27: root_directory_.reset(new ArcDocumentsProviderDocument("root", true)); nit: s/.reset(new X(...))/ = base::MakeUnique<X>(...)/ https://codereview.chromium.org/2574173002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc (right): https://codereview.chromium.org/2574173002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:62: base::Bind(&ArcDocumentsProviderRoot::GetFileInfoAfterCacheUpdate, nit: #include "base/bind.h" https://codereview.chromium.org/2574173002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:109: base::Time::FromDoubleT(document->last_modified / 1000.0); nit: base::Time::FromJavaTime(document->last_modified); ? https://codereview.chromium.org/2574173002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc (right): https://codereview.chromium.org/2574173002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc:133: root_->GetFileInfo(base::FilePath(FILE_PATH_LITERAL("pet/cat.png")), Won't all of these DCHECK since you're running them in the UI thread? https://codereview.chromium.org/2574173002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.h (right): https://codereview.chromium.org/2574173002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.h:28: constexpr char kAndroidDirectoryMimeType[] = "vnd.android.document/directory"; nit: extern const char. https://codereview.chromium.org/2574173002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_file_system_instance_util.cc (right): https://codereview.chromium.org/2574173002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_instance_util.cc:45: void CallOnIOThread(const base::Callback<void(T)>& callback, T result) { nit: PostToIOThread? You seem to be using OnXxxThread() to mean that you intend that function to only be called from the Xxx thread.
https://codereview.chromium.org/2574173002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.cc (right): https://codereview.chromium.org/2574173002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.cc:85: std::map<base::FilePath::StringType, int> name_counters; On 2016/12/15 23:54:26, Luis Héctor Chávez wrote: > nit: wrap in a block for consistency? Done. https://codereview.chromium.org/2574173002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.cc:174: base::StringPrintf("%s.%s", filename.c_str(), new_extension.c_str()); On 2016/12/15 23:54:26, Luis Héctor Chávez wrote: > Given that you're already converting |filename| to a base::FilePath (in L160), > can you use base::FilePath::ReplaceExtension() instead? Using base::FilePath sounds good, thanks for the pointer. BTW I used AddExtension() instead of ReplaceExtension() to avoid accidentally replacing meaningful part of filename. For example, music files are presented by titles, so I don't want to rewrite "Let.It.Be" with "Let.It.mp3". https://codereview.chromium.org/2574173002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.h (right): https://codereview.chromium.org/2574173002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.h:63: static base::FilePath::StringType GetFileNameForDocument( On 2016/12/15 23:54:26, Luis Héctor Chávez wrote: > nit: can this be in the anonymous namespace? Ah yes, I first put this function here for unit tests, but now I test it indirectly via UpdateWithChildDocuments(), so we can make this an anonymous function. https://codereview.chromium.org/2574173002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document_unittest.cc (right): https://codereview.chromium.org/2574173002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document_unittest.cc:27: root_directory_.reset(new ArcDocumentsProviderDocument("root", true)); On 2016/12/15 23:54:26, Luis Héctor Chávez wrote: > nit: s/.reset(new X(...))/ = base::MakeUnique<X>(...)/ Done. https://codereview.chromium.org/2574173002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc (right): https://codereview.chromium.org/2574173002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:62: base::Bind(&ArcDocumentsProviderRoot::GetFileInfoAfterCacheUpdate, On 2016/12/15 23:54:26, Luis Héctor Chávez wrote: > nit: #include "base/bind.h" Done. https://codereview.chromium.org/2574173002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:109: base::Time::FromDoubleT(document->last_modified / 1000.0); On 2016/12/15 23:54:26, Luis Héctor Chávez wrote: > nit: base::Time::FromJavaTime(document->last_modified); ? Thanks, that should be better. https://codereview.chromium.org/2574173002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc (right): https://codereview.chromium.org/2574173002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc:133: root_->GetFileInfo(base::FilePath(FILE_PATH_LITERAL("pet/cat.png")), On 2016/12/15 23:54:26, Luis Héctor Chávez wrote: > Won't all of these DCHECK since you're running them in the UI thread? TestBrowserThreadBundle runs UI/IO/etc. threads as a single shared thread, so DCHECK_CURRENT_ON() always succeeds. https://cs.chromium.org/chromium/src/content/public/test/test_browser_thread_... https://codereview.chromium.org/2574173002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.h (right): https://codereview.chromium.org/2574173002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_util.h:28: constexpr char kAndroidDirectoryMimeType[] = "vnd.android.document/directory"; On 2016/12/15 23:54:26, Luis Héctor Chávez wrote: > nit: extern const char. Done. https://codereview.chromium.org/2574173002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/fileapi/arc_file_system_instance_util.cc (right): https://codereview.chromium.org/2574173002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/fileapi/arc_file_system_instance_util.cc:45: void CallOnIOThread(const base::Callback<void(T)>& callback, T result) { On 2016/12/15 23:54:26, Luis Héctor Chávez wrote: > nit: PostToIOThread? You seem to be using OnXxxThread() to mean that you intend > that function to only be called from the Xxx thread. Renamed to PostToIOThread.
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: 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...
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 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.
Sorry, this CL is too big for me and I could just take a quick glance. Random comments. https://codereview.chromium.org/2574173002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.cc (right): https://codereview.chromium.org/2574173002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.cc:30: filename = "dot"; "dot" looks unfriendly for users who don't speak English. FYI: To display Google Drive files, we use drive::util::NormalizeFileName(). https://cs.chromium.org/chromium/src/components/drive/file_system_core_util.c... https://codereview.chromium.org/2574173002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.h (right): https://codereview.chromium.org/2574173002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.h:60: const bool is_directory_; Why don't you just remember the mime type? https://codereview.chromium.org/2574173002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc (right): https://codereview.chromium.org/2574173002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:27: // base::FilePath::DirName() returns kCurrentDirectory if the path does not Why don't you give this class absolute paths? This way, it looks more natural (the root's path is "/", not "") and, you don't have to have tricks like this. https://codereview.chromium.org/2574173002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:54: // Skip a cache update if possible. Caching should consist of two parts: remembering data and forgetting it when necessary. I see the former part, but no latter part. How are we going to handle when documents disappear? It'd be nice to have a design doc for the cache system if we are going to have one. https://codereview.chromium.org/2574173002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.h (right): https://codereview.chromium.org/2574173002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.h:59: void UpdateDirectoryCacheUpTo(const base::FilePath& path, Please add a comment to describe what this method does. What does "cache" mean? What does "up to" mean? https://codereview.chromium.org/2574173002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.h:61: void UpdateDirectoryCacheIter(const base::FilePath& parent_directory_path, What does "iter" mean? Generally speaking, you should avoid abbreviation https://google.github.io/styleguide/cppguide.html#General_Naming_Rules https://codereview.chromium.org/2574173002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc (right): https://codereview.chromium.org/2574173002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc:41: constexpr DocumentSpec kPetDocumentSpec = { I know you really love animals, but please give these constants more readable names. https://codereview.chromium.org/2574173002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_file_system_instance_util.cc (right): https://codereview.chromium.org/2574173002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_file_system_instance_util.cc:25: constexpr uint32_t kContentFileSystemVersion = 1; Could you keep constants for each method? It'd be convenient when we happen to add new ones. (kContentFileSystemNewerMethodsVersion should look awkward, and I don't want to waste time to come up with an appropriate name for the constant)
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 After offline discussions with hashimoto@, we've decided to remove the cache layer to simplify the code and avoid possible bugs with caching. Let's see how this simple implementation performs. I'm updating the unit tests now, but meanwhile please take a look as the patch changed quite a bit. https://codereview.chromium.org/2574173002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.cc (right): https://codereview.chromium.org/2574173002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.cc:30: filename = "dot"; On 2016/12/19 06:40:48, hashimoto wrote: > "dot" looks unfriendly for users who don't speak English. > > FYI: To display Google Drive files, we use drive::util::NormalizeFileName(). > https://cs.chromium.org/chromium/src/components/drive/file_system_core_util.c... Thanks for the pointer, I'll follow it. https://codereview.chromium.org/2574173002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.h (right): https://codereview.chromium.org/2574173002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_document.h:60: const bool is_directory_; On 2016/12/19 06:40:48, hashimoto wrote: > Why don't you just remember the mime type? Because we only need is_directory, not mime type. https://codereview.chromium.org/2574173002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc (right): https://codereview.chromium.org/2574173002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:27: // base::FilePath::DirName() returns kCurrentDirectory if the path does not On 2016/12/19 06:40:48, hashimoto wrote: > Why don't you give this class absolute paths? > > This way, it looks more natural (the root's path is "/", not "") and, you don't > have to have tricks like this. N/A https://codereview.chromium.org/2574173002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:54: // Skip a cache update if possible. On 2016/12/19 06:40:48, hashimoto wrote: > Caching should consist of two parts: remembering data and forgetting it when > necessary. > I see the former part, but no latter part. > How are we going to handle when documents disappear? > It'd be nice to have a design doc for the cache system if we are going to have > one. N/A https://codereview.chromium.org/2574173002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.h (right): https://codereview.chromium.org/2574173002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.h:59: void UpdateDirectoryCacheUpTo(const base::FilePath& path, On 2016/12/19 06:40:48, hashimoto wrote: > Please add a comment to describe what this method does. > What does "cache" mean? > What does "up to" mean? N/A https://codereview.chromium.org/2574173002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.h:61: void UpdateDirectoryCacheIter(const base::FilePath& parent_directory_path, On 2016/12/19 06:40:48, hashimoto wrote: > What does "iter" mean? > Generally speaking, you should avoid abbreviation > https://google.github.io/styleguide/cppguide.html#General_Naming_Rules N/A https://codereview.chromium.org/2574173002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_file_system_instance_util.cc (right): https://codereview.chromium.org/2574173002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_file_system_instance_util.cc:25: constexpr uint32_t kContentFileSystemVersion = 1; On 2016/12/19 06:40:48, hashimoto wrote: > Could you keep constants for each method? > It'd be convenient when we happen to add new ones. > (kContentFileSystemNewerMethodsVersion should look awkward, and I don't want to > waste time to come up with an appropriate name for the constant) Done.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2574173002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc (right): https://codereview.chromium.org/2574173002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:27: // base::FilePath::DirName() returns kCurrentDirectory if the path does not On 2016/12/19 10:01:56, Shuhei Takahashi wrote: > On 2016/12/19 06:40:48, hashimoto wrote: > > Why don't you give this class absolute paths? > > > > This way, it looks more natural (the root's path is "/", not "") and, you > don't > > have to have tricks like this. > > N/A ping? https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc (right): https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:57: std::string new_extension; This should be base::FilePath::StringType. https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:213: callback.Run(NameMapping()); ReadDirectoryInternal cannot return errors when the specified directory doesn't exist. Doesn't this cause any problems? If so, it'd be nice to have a comment to describe the justification. https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:235: std::string suffix = base::StringPrintf(" %d", suffix_counter); Drive code uses "(n)" instead of "n" as suffix. It'd be nice to be consistent. https://cs.chromium.org/chromium/src/components/drive/chromeos/resource_metad... Probably we should make a change later to share the same code (as well as file name escape code above). https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.h (right): https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.h:12: #include "base/callback.h" nit: Can't be replaced with callback_forward.h? https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.h:53: using NameMapping = std::map<base::FilePath::StringType, ThinDocument>; nit: "NameMapping" sounds like a mapping from names to names. How about something like NameToDocumentMap? https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc (right): https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc:52: kAndroidDirectoryMimeType, 0, 11}; The Android side code returns -1 if the size is unknown. Shouldn't the size value be -1 here too? https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc:76: class FakeFileSystemInstanceImpl : public FakeFileSystemInstance { "FakeFileSystemInstanceImpl" sounds weird, as "Fake" here already means it's a fake implementation. https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc:132: EXPECT_EQ(strcmp(spec.mime_type, kAndroidDirectoryMimeType) == 0, How about something like this to make it more readable, and for better output on failures? if (is_directory) { EXPECT_EQ(spec.mime_type, kAndroidDirectoryMimeType); } else { EXPECT_NE(spec.mime_type, kAndroidDirectoryMimeType); } https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc:134: EXPECT_EQ(false, info.is_symbolic_link); EXPECT_FALSE https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc:146: arc_service_manager_ = base::MakeUnique<ArcServiceManager>(nullptr); Can't this be done in the initializer list? https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc:149: root_ = base::MakeUnique<ArcDocumentsProviderRoot>(kAuthority, ditto.
PTAL https://codereview.chromium.org/2574173002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc (right): https://codereview.chromium.org/2574173002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:27: // base::FilePath::DirName() returns kCurrentDirectory if the path does not On 2016/12/20 04:55:07, hashimoto wrote: > On 2016/12/19 10:01:56, Shuhei Takahashi wrote: > > On 2016/12/19 06:40:48, hashimoto wrote: > > > Why don't you give this class absolute paths? > > > > > > This way, it looks more natural (the root's path is "/", not "") and, you > > don't > > > have to have tricks like this. > > > > N/A > > ping? Oh, since StripBaseName() is removed in the latest patch set I thought this comment is no longer applicable. Actually I don't have an idea of how making |path| absolute will make this code simpler, so could you elaborate? The only part in this file manipulating |path| now is ResolveToDocumentId() and its sibling methods, and it needs to iterate through the hierarchy of a file path. In ResolveToDocumentIdRecursivelyWithNameToThinDocumentMap(), I need to strip the top-level part of a file path (e.g. foo/bar/baz -> bar/baz). I thought one of the most simple ways to do this is to split the path with GetComponents() and treat the path as a vector. Is it simple enough right? If we make |path| absolute, GetComponents() will just add "/" as a first component, which is just garbage for us. Maybe you mean stop using GetComponents() and use other methods to do the business? I looked through methods in base::FilePath again but I could not find a good method I can compute /bar/baz from /foo/bar/baz. Would you mind telling me how this can be simply done? https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc (right): https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:57: std::string new_extension; On 2016/12/20 04:55:07, hashimoto wrote: > This should be base::FilePath::StringType. Done. https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:213: callback.Run(NameMapping()); On 2016/12/20 04:55:07, hashimoto wrote: > ReadDirectoryInternal cannot return errors when the specified directory doesn't > exist. > Doesn't this cause any problems? > If so, it'd be nice to have a comment to describe the justification. Makes sense, let's return errors from ReadDirectoryInternal. https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:235: std::string suffix = base::StringPrintf(" %d", suffix_counter); On 2016/12/20 04:55:07, hashimoto wrote: > Drive code uses "(n)" instead of "n" as suffix. > It'd be nice to be consistent. > https://cs.chromium.org/chromium/src/components/drive/chromeos/resource_metad... > > Probably we should make a change later to share the same code (as well as file > name escape code above). Changed to (n). Yes, it would be good if we can consolidate those codes. If you know a good place to put the shared code please let me know. https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.h (right): https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.h:12: #include "base/callback.h" On 2016/12/20 04:55:07, hashimoto wrote: > nit: Can't be replaced with callback_forward.h? Done. https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.h:53: using NameMapping = std::map<base::FilePath::StringType, ThinDocument>; On 2016/12/20 04:55:07, hashimoto wrote: > nit: "NameMapping" sounds like a mapping from names to names. > How about something like NameToDocumentMap? Done. https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc (right): https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc:52: kAndroidDirectoryMimeType, 0, 11}; On 2016/12/20 04:55:07, hashimoto wrote: > The Android side code returns -1 if the size is unknown. > Shouldn't the size value be -1 here too? Done. https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc:76: class FakeFileSystemInstanceImpl : public FakeFileSystemInstance { On 2016/12/20 04:55:07, hashimoto wrote: > "FakeFileSystemInstanceImpl" sounds weird, as "Fake" here already means it's a > fake implementation. Renamed to FileSystemInstanceTestImpl. Does it sound good to you? https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc:132: EXPECT_EQ(strcmp(spec.mime_type, kAndroidDirectoryMimeType) == 0, On 2016/12/20 04:55:07, hashimoto wrote: > How about something like this to make it more readable, and for better output on > failures? > > if (is_directory) { > EXPECT_EQ(spec.mime_type, kAndroidDirectoryMimeType); > } else { > EXPECT_NE(spec.mime_type, kAndroidDirectoryMimeType); > } Done. https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc:134: EXPECT_EQ(false, info.is_symbolic_link); On 2016/12/20 04:55:07, hashimoto wrote: > EXPECT_FALSE Done. https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc:146: arc_service_manager_ = base::MakeUnique<ArcServiceManager>(nullptr); On 2016/12/20 04:55:07, hashimoto wrote: > Can't this be done in the initializer list? Done. https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc:149: root_ = base::MakeUnique<ArcDocumentsProviderRoot>(kAuthority, On 2016/12/20 04:55:07, hashimoto wrote: > ditto. Done.
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 https://codereview.chromium.org/2574173002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc (right): https://codereview.chromium.org/2574173002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:27: // base::FilePath::DirName() returns kCurrentDirectory if the path does not On 2016/12/20 06:08:24, Shuhei Takahashi wrote: > On 2016/12/20 04:55:07, hashimoto wrote: > > On 2016/12/19 10:01:56, Shuhei Takahashi wrote: > > > On 2016/12/19 06:40:48, hashimoto wrote: > > > > Why don't you give this class absolute paths? > > > > > > > > This way, it looks more natural (the root's path is "/", not "") and, you > > > don't > > > > have to have tricks like this. > > > > > > N/A > > > > ping? > > Oh, since StripBaseName() is removed in the latest patch set I thought this > comment is no longer applicable. Actually I don't have an idea of how making > |path| absolute will make this code simpler, so could you elaborate? > > The only part in this file manipulating |path| now is ResolveToDocumentId() and > its sibling methods, and it needs to iterate through the hierarchy of a file > path. In ResolveToDocumentIdRecursivelyWithNameToThinDocumentMap(), I need to > strip the top-level part of a file path (e.g. foo/bar/baz -> bar/baz). I thought > one of the most simple ways to do this is to split the path with GetComponents() > and treat the path as a vector. Is it simple enough right? > > If we make |path| absolute, GetComponents() will just add "/" as a first > component, which is just garbage for us. Maybe you mean stop using > GetComponents() and use other methods to do the business? I looked through > methods in base::FilePath again but I could not find a good method I can compute > /bar/baz from /foo/bar/baz. Would you mind telling me how this can be simply > done? Thanks for the explanation. sg https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc (right): https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:235: std::string suffix = base::StringPrintf(" %d", suffix_counter); On 2016/12/20 06:08:24, Shuhei Takahashi wrote: > On 2016/12/20 04:55:07, hashimoto wrote: > > Drive code uses "(n)" instead of "n" as suffix. > > It'd be nice to be consistent. > > > https://cs.chromium.org/chromium/src/components/drive/chromeos/resource_metad... > > > > Probably we should make a change later to share the same code (as well as file > > name escape code above). > > Changed to (n). Yes, it would be good if we can consolidate those codes. If you > know a good place to put the shared code please let me know. Somewhere under components (probably a new directory) or in storage/browser/fileapi? https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc (right): https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc:76: class FakeFileSystemInstanceImpl : public FakeFileSystemInstance { On 2016/12/20 06:08:24, Shuhei Takahashi wrote: > On 2016/12/20 04:55:07, hashimoto wrote: > > "FakeFileSystemInstanceImpl" sounds weird, as "Fake" here already means it's a > > fake implementation. > > Renamed to FileSystemInstanceTestImpl. Does it sound good to you? sg https://codereview.chromium.org/2574173002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc (right): https://codereview.chromium.org/2574173002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc:110: for (auto spec : kAllSpecs) { "const auto&"?
Thanks for reviewing! https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc (right): https://codereview.chromium.org/2574173002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:235: std::string suffix = base::StringPrintf(" %d", suffix_counter); On 2016/12/20 06:24:18, hashimoto wrote: > On 2016/12/20 06:08:24, Shuhei Takahashi wrote: > > On 2016/12/20 04:55:07, hashimoto wrote: > > > Drive code uses "(n)" instead of "n" as suffix. > > > It'd be nice to be consistent. > > > > > > https://cs.chromium.org/chromium/src/components/drive/chromeos/resource_metad... > > > > > > Probably we should make a change later to share the same code (as well as > file > > > name escape code above). > > > > Changed to (n). Yes, it would be good if we can consolidate those codes. If > you > > know a good place to put the shared code please let me know. > Somewhere under components (probably a new directory) or in > storage/browser/fileapi? Thanks, filed crbug.com/675868. https://codereview.chromium.org/2574173002/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc (right): https://codereview.chromium.org/2574173002/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc:110: for (auto spec : kAllSpecs) { On 2016/12/20 06:24:18, hashimoto wrote: > "const auto&"? Done.
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
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/2574173002/#ps220001 (title: "Addressed hashimoto's comments.")
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": 220001, "attempt_start_ts": 1482295501402740,
"parent_rev": "4a85e4adab768737c1c2e2655ad109791515e991", "commit_rev":
"ce62f89ef4db3ae1c3b4a01503e6ccc7b837a480"}
Message was sent while issue was closed.
Description was changed from ========== mediaview: Implement ArcDocumentsProviderRoot. BUG=chromium:671511 TEST=unit_tests --gtest_filter='Arc*' TEST=trybot ========== to ========== mediaview: Implement ArcDocumentsProviderRoot. BUG=chromium:671511 TEST=unit_tests --gtest_filter='Arc*' TEST=trybot Review-Url: https://codereview.chromium.org/2574173002 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== mediaview: Implement ArcDocumentsProviderRoot. BUG=chromium:671511 TEST=unit_tests --gtest_filter='Arc*' TEST=trybot Review-Url: https://codereview.chromium.org/2574173002 ========== to ========== mediaview: Implement ArcDocumentsProviderRoot. BUG=chromium:671511 TEST=unit_tests --gtest_filter='Arc*' TEST=trybot Committed: https://crrev.com/787e1c859432963c8efe3525a7ad1cc340fa66a5 Cr-Commit-Position: refs/heads/master@{#440020} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/787e1c859432963c8efe3525a7ad1cc340fa66a5 Cr-Commit-Position: refs/heads/master@{#440020} |
