Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2379323002:
arc: Exclude non-media files from Android media scanning. (Closed)
|
Created:
4 years, 2 months ago by Shuhei Takahashi Modified:
4 years, 2 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: Exclude non-media files from Android media scanning.
Currently ArcDownloadsWatcherService requests Android media provider to
scan every new/modified files under the Downloads directory. It causes
heavy load when many files are created/deleted there.
This change excludes files not supported by Android media scanner.
BUG=chromium:651731
TEST=touch a.txt # Not scanned
TEST=touch a.png # Scanned
TEST=unit_tests --gtest_filter='ArcDownloadsWatcherServiceTest.*'
Committed: https://crrev.com/748504fe82a72cd664eec781f844fbf086c906d1
Cr-Commit-Position: refs/heads/master@{#422723}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressed comments. #
Total comments: 6
Patch Set 3 : Add unittest. #Patch Set 4 : IWYU #
Total comments: 4
Patch Set 5 : Addressed comments from yusukes. #
Messages
Total messages: 24 (13 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...
nya@chromium.org changed reviewers: + yusukes@chromium.org
yusukes: PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mostly nits: https://codereview.chromium.org/2379323002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc (right): https://codereview.chromium.org/2379323002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:45: // Entries must be sorted by lexicographical order for binary search. and must be lower-case, I think. https://codereview.chromium.org/2379323002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:47: // frameworks/base/media/java/android/media/MediaScanner.java. What about adding a git hash to document the revision you used? https://codereview.chromium.org/2379323002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:131: bool HasMediaExtension(const base::FilePath& path) { Would you mind adding chrome/browser/chromeos/arc/arc_downloads_watcher_service_unittest.cc? I know this function is kind of trivial but I'd like to see some tests for the file... https://codereview.chromium.org/2379323002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:132: const std::string extension = base::ToLowerASCII(path.Extension()); ..and add DCHECK(std::is_sorted(...)); check here? https://codereview.chromium.org/2379323002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:136: [](const char* a, const std::string& b) { return a == b; }); nit: any reason to create a string every time? ... extension.c_str(), [](const char* a, const char* b) { return !strcmp(a, b); }); maybe?
Description was changed from ========== arc: Exclude non-media files from Android media scanning. Currently ArcDownloadsWatcherService requests Android media provider to scan every new/modified files under the Downloads directory. It causes heavy load when many files are created/deleted there. This change excludes files not supported by Android media scanner. BUG=chromium:651731 TEST=touch a.txt # Not scanned TEST=touch a.png # Scanned ========== to ========== arc: Exclude non-media files from Android media scanning. Currently ArcDownloadsWatcherService requests Android media provider to scan every new/modified files under the Downloads directory. It causes heavy load when many files are created/deleted there. This change excludes files not supported by Android media scanner. BUG=chromium:651731 TEST=touch a.txt # Not scanned TEST=touch a.png # Scanned TEST=unit_tests --gtest_filter='ArcDownloadsWatcherServiceTest.*' ==========
PTAL https://codereview.chromium.org/2379323002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc (right): https://codereview.chromium.org/2379323002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:45: // Entries must be sorted by lexicographical order for binary search. On 2016/09/30 22:51:23, Yusuke Sato wrote: > and must be lower-case, I think. Done. https://codereview.chromium.org/2379323002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:47: // frameworks/base/media/java/android/media/MediaScanner.java. On 2016/09/30 22:51:23, Yusuke Sato wrote: > What about adding a git hash to document the revision you used? Mentioned that this list from aosp-marshmallow. Not a specific commit, but I believe the list is fairly stable in aosp-marshmallow branch. https://codereview.chromium.org/2379323002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:131: bool HasMediaExtension(const base::FilePath& path) { On 2016/09/30 22:51:23, Yusuke Sato wrote: > Would you mind adding > chrome/browser/chromeos/arc/arc_downloads_watcher_service_unittest.cc? I know > this function is kind of trivial but I'd like to see some tests for the file... Yes that's good, and actually this function had a bug. My brain was exhausted at the time I wrote this code... https://codereview.chromium.org/2379323002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:132: const std::string extension = base::ToLowerASCII(path.Extension()); On 2016/09/30 22:51:23, Yusuke Sato wrote: > ..and add DCHECK(std::is_sorted(...)); check here? Done. https://codereview.chromium.org/2379323002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:136: [](const char* a, const std::string& b) { return a == b; }); On 2016/09/30 22:51:23, Yusuke Sato wrote: > nit: any reason to create a string every time? > > ... extension.c_str(), > [](const char* a, const char* b) { return !strcmp(a, b); }); > > maybe? Sounds good, done.
lg, but can you upload the test file and run try? https://codereview.chromium.org/2379323002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/BUILD.gn (right): https://codereview.chromium.org/2379323002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/BUILD.gn:1355: "arc/arc_downloads_watcher_service_unittest.cc", Can you run git-add for the file? https://codereview.chromium.org/2379323002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc (right): https://codereview.chromium.org/2379323002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:12: IWYU: #include <string.h> ? https://codereview.chromium.org/2379323002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:179: strcmp(*iter, extension.c_str()) == 0; Can you run git cl format if you haven't?
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: + satorux@chromium.org
satorux: PTAL for BUILD.gn yusukes: PTAL CQ dry-run is now in progress. https://codereview.chromium.org/2379323002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/BUILD.gn (right): https://codereview.chromium.org/2379323002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/BUILD.gn:1355: "arc/arc_downloads_watcher_service_unittest.cc", On 2016/10/03 16:54:53, Yusuke Sato wrote: > Can you run git-add for the file? Oops, sorry. https://codereview.chromium.org/2379323002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc (right): https://codereview.chromium.org/2379323002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:12: On 2016/10/03 16:54:54, Yusuke Sato wrote: > IWYU: #include <string.h> ? Done. https://codereview.chromium.org/2379323002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_downloads_watcher_service.cc:179: strcmp(*iter, extension.c_str()) == 0; On 2016/10/03 16:54:54, Yusuke Sato wrote: > Can you run git cl format if you haven't? Actually this is the result of git cl format.
lgtm https://codereview.chromium.org/2379323002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_downloads_watcher_service_unittest.cc (right): https://codereview.chromium.org/2379323002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_downloads_watcher_service_unittest.cc:7: #include "testing/gtest/include/gtest/gtest.h" IWYU: #include "base/files/file_path.h" for FILE_PATH_LITERAL and the constructor call. https://codereview.chromium.org/2379323002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_downloads_watcher_service_unittest.cc:13: TEST_F(ArcDownloadsWatcherServiceTest, HasAndroidSupportedMediaExtension) { This doesn't have to be TEST_F. If you agree, please change it to TEST and remove line 11.
Thanks for reviewing, Yusuke! https://codereview.chromium.org/2379323002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_downloads_watcher_service_unittest.cc (right): https://codereview.chromium.org/2379323002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_downloads_watcher_service_unittest.cc:7: #include "testing/gtest/include/gtest/gtest.h" On 2016/10/04 04:07:01, Yusuke Sato wrote: > IWYU: #include "base/files/file_path.h" for FILE_PATH_LITERAL and the > constructor call. Done. https://codereview.chromium.org/2379323002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_downloads_watcher_service_unittest.cc:13: TEST_F(ArcDownloadsWatcherServiceTest, HasAndroidSupportedMediaExtension) { On 2016/10/04 04:07:01, Yusuke Sato wrote: > This doesn't have to be TEST_F. If you agree, please change it to TEST and > remove line 11. Done.
BUILD.gn lgtm
The CQ bit was checked by nya@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yusukes@chromium.org Link to the patchset: https://codereview.chromium.org/2379323002/#ps80001 (title: "Addressed comments from yusukes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== arc: Exclude non-media files from Android media scanning. Currently ArcDownloadsWatcherService requests Android media provider to scan every new/modified files under the Downloads directory. It causes heavy load when many files are created/deleted there. This change excludes files not supported by Android media scanner. BUG=chromium:651731 TEST=touch a.txt # Not scanned TEST=touch a.png # Scanned TEST=unit_tests --gtest_filter='ArcDownloadsWatcherServiceTest.*' ========== to ========== arc: Exclude non-media files from Android media scanning. Currently ArcDownloadsWatcherService requests Android media provider to scan every new/modified files under the Downloads directory. It causes heavy load when many files are created/deleted there. This change excludes files not supported by Android media scanner. BUG=chromium:651731 TEST=touch a.txt # Not scanned TEST=touch a.png # Scanned TEST=unit_tests --gtest_filter='ArcDownloadsWatcherServiceTest.*' ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== arc: Exclude non-media files from Android media scanning. Currently ArcDownloadsWatcherService requests Android media provider to scan every new/modified files under the Downloads directory. It causes heavy load when many files are created/deleted there. This change excludes files not supported by Android media scanner. BUG=chromium:651731 TEST=touch a.txt # Not scanned TEST=touch a.png # Scanned TEST=unit_tests --gtest_filter='ArcDownloadsWatcherServiceTest.*' ========== to ========== arc: Exclude non-media files from Android media scanning. Currently ArcDownloadsWatcherService requests Android media provider to scan every new/modified files under the Downloads directory. It causes heavy load when many files are created/deleted there. This change excludes files not supported by Android media scanner. BUG=chromium:651731 TEST=touch a.txt # Not scanned TEST=touch a.png # Scanned TEST=unit_tests --gtest_filter='ArcDownloadsWatcherServiceTest.*' Committed: https://crrev.com/748504fe82a72cd664eec781f844fbf086c906d1 Cr-Commit-Position: refs/heads/master@{#422723} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/748504fe82a72cd664eec781f844fbf086c906d1 Cr-Commit-Position: refs/heads/master@{#422723} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
