Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2285393003:
Add API to get a folder's size. (Closed)
|
Created:
4 years, 3 months ago by harukam Modified:
4 years, 3 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, asvitkine+watch_chromium.org, rginda+watch_chromium.org, jlklein+watch-closure_chromium.org, oshima+watch_chromium.org, vitalyp+closure_chromium.org, chromium-apps-reviews_chromium.org, davemoore+watch_chromium.org, dbeam+watch-closure_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd API to get a folder's size.
BUG=474318
TEST=None
We are attempting to add a feature in Files app which shows a folder's size.
It's possible to implement it with JavaScript, but it's too slow. So, We've decided to add a new C++ API in this CL.
Committed: https://crrev.com/18d04665c3e4aaf6bdfbb5330ca103f5b5513261
Cr-Commit-Position: refs/heads/master@{#416293}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Add API to get a folder's size. #
Total comments: 2
Patch Set 3 : Add API to get a folder's size. #
Total comments: 10
Patch Set 4 : Add API to get a folder's size. #
Total comments: 8
Patch Set 5 : Add API to get a folder's size. #Messages
Total messages: 33 (12 generated)
harukam@google.com changed reviewers: + fukino@chromium.org
PTAL
https://codereview.chromium.org/2285393003/diff/1/chrome/browser/chromeos/ext... File chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc (right): https://codereview.chromium.org/2285393003/diff/1/chrome/browser/chromeos/ext... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:974: const base::FilePath root_path = file_manager::util::GetLocalPathFromURL( Please check if the root_path is empty or not. https://codereview.chromium.org/2285393003/diff/1/chrome/browser/chromeos/ext... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:975: render_frame_host(), chrome_details_.GetProfile(), GURL(params->url)); chrome_details_ looks unnecessary. Simply GetProfile() should work. https://codereview.chromium.org/2285393003/diff/1/chrome/browser/chromeos/ext... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:976: int64_t size = base::ComputeDirectorySize(root_path); You should not access file system on this thread not to block the UI. BlockingPool is the preferred thread for such purpose. You can refer to FileManagerPrivateInternalValidatePathNameLengthFunction::RunAsync() to touch file system on blocking pool. https://codereview.chromium.org/2285393003/diff/1/chrome/common/extensions/ap... File chrome/common/extensions/api/file_manager_private.idl (right): https://codereview.chromium.org/2285393003/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/file_manager_private.idl:978: SimpleCallback callback); We need to receive the directory size, so please define a callback type for this method instead of using SimpleCallback. https://codereview.chromium.org/2285393003/diff/1/third_party/closure_compile... File third_party/closure_compiler/externs/file_manager_private.js (right): https://codereview.chromium.org/2285393003/diff/1/third_party/closure_compile... third_party/closure_compiler/externs/file_manager_private.js:1: // Copyright 2016 The Chromium Authors. All rights reserved. Revert the change of this file (updating whole this will will cause some errors) and then add the annotation for getDirecdtorySize for now.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
PTAL https://codereview.chromium.org/2285393003/diff/1/chrome/browser/chromeos/ext... File chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc (right): https://codereview.chromium.org/2285393003/diff/1/chrome/browser/chromeos/ext... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:974: const base::FilePath root_path = file_manager::util::GetLocalPathFromURL( On 2016/08/30 05:17:06, fukino wrote: > Please check if the root_path is empty or not. Done. https://codereview.chromium.org/2285393003/diff/1/chrome/browser/chromeos/ext... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:975: render_frame_host(), chrome_details_.GetProfile(), GURL(params->url)); On 2016/08/30 05:17:06, fukino wrote: > chrome_details_ looks unnecessary. Simply GetProfile() should work. Acknowledged. https://codereview.chromium.org/2285393003/diff/1/chrome/browser/chromeos/ext... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:976: int64_t size = base::ComputeDirectorySize(root_path); On 2016/08/30 05:17:06, fukino wrote: > You should not access file system on this thread not to block the UI. > BlockingPool is the preferred thread for such purpose. > > You can refer to > FileManagerPrivateInternalValidatePathNameLengthFunction::RunAsync() to touch > file system on blocking pool. Acknowledged. https://codereview.chromium.org/2285393003/diff/1/chrome/common/extensions/ap... File chrome/common/extensions/api/file_manager_private.idl (right): https://codereview.chromium.org/2285393003/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/file_manager_private.idl:978: SimpleCallback callback); On 2016/08/30 05:17:06, fukino wrote: > We need to receive the directory size, so please define a callback type for this > method instead of using SimpleCallback. Acknowledged. https://codereview.chromium.org/2285393003/diff/1/third_party/closure_compile... File third_party/closure_compiler/externs/file_manager_private.js (right): https://codereview.chromium.org/2285393003/diff/1/third_party/closure_compile... third_party/closure_compiler/externs/file_manager_private.js:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/08/30 05:17:06, fukino wrote: > Revert the change of this file (updating whole this will will cause some errors) > and then add the annotation for getDirecdtorySize for now. Done.
lgtm with a nit. https://codereview.chromium.org/2285393003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc (right): https://codereview.chromium.org/2285393003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:981: OnDirectorySizeRetrieved, this)); This line's indent level looks incorrect. Could you try git cl format?
harukam@google.com changed reviewers: + holte@chromium.org, mtomasz@chromium.org
Please take a look. +mtomasz@ for the files under chrome/ +holte@ for extensions/browser/extension_function_histogram_value.h, tools/metrics/histograms/histograms.xml https://codereview.chromium.org/2285393003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc (right): https://codereview.chromium.org/2285393003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:981: OnDirectorySizeRetrieved, this)); On 2016/08/30 06:55:49, fukino wrote: > This line's indent level looks incorrect. Could you try git cl format? Thanks! Done.
Description was changed from ========== Add API to get a folder's size. BUG=474318 TEST=None ========== to ========== Add API to get a folder's size. BUG=474318 TEST=None We are attempting to add a feature in Files app which shows a folder's size. It's possible to implement it with JavaScript, but it's too slow. So, We've decided to add a new C++ API in this CL. ==========
histograms lgtm
Friendly ping -> mtomasz@
Sorry for late. https://codereview.chromium.org/2285393003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc (right): https://codereview.chromium.org/2285393003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:966: SetError("File URL must be provided"); nit: period missing. https://codereview.chromium.org/2285393003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:973: if (root_path.empty()) nit: Maybe we should set an error message? https://codereview.chromium.org/2285393003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:978: base::Bind(&base::ComputeDirectorySize, root_path), Does it work on non native mount points, eg. on Drive or on Dropbox (via FSP API)? If it's not intended to work, then can we add a comment? https://codereview.chromium.org/2285393003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/private_api_file_system.h (right): https://codereview.chromium.org/2285393003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.h:339: // Implements the chrome.fileManagerPrivate.getDirectorySize mothod. typo: method
Thanks. Please take another look. https://codereview.chromium.org/2285393003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc (right): https://codereview.chromium.org/2285393003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:966: SetError("File URL must be provided"); On 2016/09/01 07:49:49, mtomasz wrote: > nit: period missing. You can also find line 804 missing period. Should I correct it as well as this? https://codereview.chromium.org/2285393003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:973: if (root_path.empty()) On 2016/09/01 07:49:48, mtomasz wrote: > nit: Maybe we should set an error message? Acknowledged. https://codereview.chromium.org/2285393003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:978: base::Bind(&base::ComputeDirectorySize, root_path), On 2016/09/01 07:49:49, mtomasz wrote: > Does it work on non native mount points, eg. on Drive or on Dropbox (via FSP > API)? If it's not intended to work, then can we add a comment? It works only on local directories, not on Drive. We will get Drive directories' size in another way. To check whether the url is local, I added several lines above. https://codereview.chromium.org/2285393003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/private_api_file_system.h (right): https://codereview.chromium.org/2285393003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.h:339: // Implements the chrome.fileManagerPrivate.getDirectorySize mothod. On 2016/09/01 07:49:49, mtomasz wrote: > typo: method Sorry, thanks.
lgtm! https://codereview.chromium.org/2285393003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc (right): https://codereview.chromium.org/2285393003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:966: SetError("File URL must be provided"); On 2016/09/02 01:33:32, harukam wrote: > On 2016/09/01 07:49:49, mtomasz wrote: > > nit: period missing. > > You can also find line 804 missing period. > Should I correct it as well as this? Up to you. It's a minor thing, but if possible we should try to be consistent.
https://codereview.chromium.org/2285393003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc (right): https://codereview.chromium.org/2285393003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:980: file_system_url.type() != storage::kFileSystemTypeRestrictedNativeLocal) { Should we add both file systems (kFileSystemTypeNativeLocal/kFileSystemTypeRestrictedNativeLocal)? https://codereview.chromium.org/2285393003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:980: file_system_url.type() != storage::kFileSystemTypeRestrictedNativeLocal) { I think external media like USB memory, SD card, etc... should be supported in this API, too. Are they covered in this if condition?
https://codereview.chromium.org/2285393003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc (right): https://codereview.chromium.org/2285393003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:980: file_system_url.type() != storage::kFileSystemTypeRestrictedNativeLocal) { On 2016/09/02 01:40:25, fukino wrote: > I think external media like USB memory, SD card, etc... should be supported in > this API, too. > Are they covered in this if condition? It seems USB memory, SD card, etc... are also NativeLocal. https://codereview.chromium.org/2285393003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:980: file_system_url.type() != storage::kFileSystemTypeRestrictedNativeLocal) { On 2016/09/02 01:40:25, fukino wrote: > Should we add both file systems > (kFileSystemTypeNativeLocal/kFileSystemTypeRestrictedNativeLocal)? Maybe we can drop kFileSystemTypeRestrictedNativeLocal?
Thanks, please take one more look. https://codereview.chromium.org/2285393003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc (right): https://codereview.chromium.org/2285393003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:966: SetError("File URL must be provided"); On 2016/09/02 01:36:37, mtomasz wrote: > On 2016/09/02 01:33:32, harukam wrote: > > On 2016/09/01 07:49:49, mtomasz wrote: > > > nit: period missing. > > > > You can also find line 804 missing period. > > Should I correct it as well as this? > > Up to you. It's a minor thing, but if possible we should try to be consistent. Thanks, I've added a period 804 line. https://codereview.chromium.org/2285393003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc (right): https://codereview.chromium.org/2285393003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:980: file_system_url.type() != storage::kFileSystemTypeRestrictedNativeLocal) { On 2016/09/02 01:40:25, fukino wrote: > I think external media like USB memory, SD card, etc... should be supported in > this API, too. > Are they covered in this if condition? Acknowledged. https://codereview.chromium.org/2285393003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:980: file_system_url.type() != storage::kFileSystemTypeRestrictedNativeLocal) { On 2016/09/02 01:40:25, fukino wrote: > Should we add both file systems > (kFileSystemTypeNativeLocal/kFileSystemTypeRestrictedNativeLocal)? Acknowledged. https://codereview.chromium.org/2285393003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:980: file_system_url.type() != storage::kFileSystemTypeRestrictedNativeLocal) { On 2016/09/02 02:33:53, fukino wrote: > On 2016/09/02 01:40:25, fukino wrote: > > I think external media like USB memory, SD card, etc... should be supported in > > this API, too. > > Are they covered in this if condition? > > It seems USB memory, SD card, etc... are also NativeLocal. Acknowledged. https://codereview.chromium.org/2285393003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:980: file_system_url.type() != storage::kFileSystemTypeRestrictedNativeLocal) { On 2016/09/02 02:33:53, fukino wrote: > On 2016/09/02 01:40:25, fukino wrote: > > Should we add both file systems > > (kFileSystemTypeNativeLocal/kFileSystemTypeRestrictedNativeLocal)? > > Maybe we can drop kFileSystemTypeRestrictedNativeLocal? Thanks for checking.
lgtm
The CQ bit was checked by harukam@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from holte@chromium.org, mtomasz@chromium.org Link to the patchset: https://codereview.chromium.org/2285393003/#ps120001 (title: "Add API to get a folder's size.")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
harukam@google.com changed reviewers: + isherman@chromium.org
isherman@, please take a look at 'extensions/browser/extension_function_histogram_value.h'.
extensions/browser/extension_function_histogram_value.h lgtm
The CQ bit was checked by isherman@chromium.org
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 ========== Add API to get a folder's size. BUG=474318 TEST=None We are attempting to add a feature in Files app which shows a folder's size. It's possible to implement it with JavaScript, but it's too slow. So, We've decided to add a new C++ API in this CL. ========== to ========== Add API to get a folder's size. BUG=474318 TEST=None We are attempting to add a feature in Files app which shows a folder's size. It's possible to implement it with JavaScript, but it's too slow. So, We've decided to add a new C++ API in this CL. ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add API to get a folder's size. BUG=474318 TEST=None We are attempting to add a feature in Files app which shows a folder's size. It's possible to implement it with JavaScript, but it's too slow. So, We've decided to add a new C++ API in this CL. ========== to ========== Add API to get a folder's size. BUG=474318 TEST=None We are attempting to add a feature in Files app which shows a folder's size. It's possible to implement it with JavaScript, but it's too slow. So, We've decided to add a new C++ API in this CL. Committed: https://crrev.com/18d04665c3e4aaf6bdfbb5330ca103f5b5513261 Cr-Commit-Position: refs/heads/master@{#416293} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/18d04665c3e4aaf6bdfbb5330ca103f5b5513261 Cr-Commit-Position: refs/heads/master@{#416293} |
