Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16707002:
[SystemInfo API] Rewrite storage info provider using storage monitor impl. (Closed)
|
Created:
7 years, 6 months ago by Haojian Wu Modified:
7 years, 5 months ago CC:
chromium-reviews, Aaron Boodman, sail+watch_chromium.org, chromium-apps-reviews_chromium.org, vandebo (ex-Chrome) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionRewrite storage info provider using storage monitor impl.
Make use of StorageMonitor implementation to reuse the common code as
possible.
BUG=177605
TEST=browser_tests --gtest_filter=SystemInfoStorageApiTest.*
TEST=unit_tests --gtest_filter=StorageInfoProviderTest.*
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=210169
Patch Set 1 #
Total comments: 9
Patch Set 2 : Updated #Patch Set 3 : Add some comments about kTestingData #Patch Set 4 : update #
Total comments: 1
Patch Set 5 : Remove storage_info_provider_linux_unittest.cc in chrome_tests_unit.gypi #
Total comments: 42
Patch Set 6 : update according to Greg's comments #Patch Set 7 : Add TODO in SystemInfoProvider #
Total comments: 48
Patch Set 8 : Update according to Billock and Lei's comments #
Total comments: 2
Patch Set 9 : Fix a little comment #Patch Set 10 : Change field "location" to "name" in SystemInfoStorage IDL definition file. #
Total comments: 38
Patch Set 11 : Rebase and Update #Patch Set 12 : Add comment about thread-safe issue for |info_| #Patch Set 13 : Fix a spelling mistake #Patch Set 14 : Fix some rename comment nits. #Patch Set 15 : Use a new way to avoid waiting for WatchingNoChangedStorage unit test to pass. #
Total comments: 8
Patch Set 16 : Update #
Total comments: 72
Patch Set 17 : Fix comments of jyasskin #
Total comments: 9
Patch Set 18 : Update based on jyasskin's comments #Patch Set 19 : Make QueryOnWorkerPool private #Patch Set 20 : Add explicit destructors in UnitTestStorageInfoProvider to avoid build error. #Messages
Total messages: 49 (0 generated)
This CL will replace the CL https://codereview.chromium.org/15896007/. I do not change systemInfo.storage api definition right now. It should get landed after the CLs https://codereview.chromium.org/15988011/ and https://codereview.chromium.org/16472007/ hongbo@ for initial review. vandebo@, lei@ for the whole patch review. Thanks.
Lei or I will take a look after hongbo is happy.
Haojian, thanks for your follow-up. A major comment here is, it should be fine to add |location| field in storage api IDL definition since we are reusing StorageMonitor to impl storage info provider. For other changes in storage api IDL, we can submit another new CLs to do them, e.g. add human readable name, modify change event etc. https://codereview.chromium.org/16707002/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc (right): https://codereview.chromium.org/16707002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:81: base::FilePath::FromUTF8Unsafe((*it)->id)); The |id| usage here is indeed a bit confused, not straightforward. Of course, it has nothing with CL. I prefer to add |location| field into storage IDL and use a persistent |device_id| as storage ID in this CL because the purpose of this CL is to reuse StorageMonitor to impl storage info provider. https://codereview.chromium.org/16707002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:183: #if defined(OS_POSIX) No indents for #if directives? https://codereview.chromium.org/16707002/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.h (right): https://codereview.chromium.org/16707002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:68: void StartQueryInfoImpl() OVERRIDE; nits: add 'virtual' keyword here. https://codereview.chromium.org/16707002/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc (right): https://codereview.chromium.org/16707002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:31: {"device:001", "C:", systeminfo::kStorageTypeUnknown, 1000, 10-0, 0}, Could you please explain why to minus the delta change step?
Sorry for the delayed review since I am on vocation these 2 days:)
https://codereview.chromium.org/16707002/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc (right): https://codereview.chromium.org/16707002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:81: base::FilePath::FromUTF8Unsafe((*it)->id)); On 2013/06/12 02:20:57, Hongbo Min wrote: > The |id| usage here is indeed a bit confused, not straightforward. Of course, it > has nothing with CL. > > I prefer to add |location| field into storage IDL and use a persistent > |device_id| as storage ID in this CL because the purpose of this CL is to reuse > StorageMonitor to impl storage info provider. Done. Add |location| field. https://codereview.chromium.org/16707002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:183: #if defined(OS_POSIX) On 2013/06/12 02:20:57, Hongbo Min wrote: > No indents for #if directives? Done. https://codereview.chromium.org/16707002/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.h (right): https://codereview.chromium.org/16707002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:68: void StartQueryInfoImpl() OVERRIDE; On 2013/06/12 02:20:57, Hongbo Min wrote: > nits: add 'virtual' keyword here. Done. https://codereview.chromium.org/16707002/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc (right): https://codereview.chromium.org/16707002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:31: {"device:001", "C:", systeminfo::kStorageTypeUnknown, 1000, 10-0, 0}, On 2013/06/12 02:20:57, Hongbo Min wrote: > Could you please explain why to minus the delta change step? It's about onAvailableCapacityChanged event implementation. In TestStorageInfoProvider class, we override GetStorageFreeSpace method to make all mock storge devices' available capacity change a given change_step. When onAvailableCapacityChanged.AddListener api is invoked in js. AddWatchedStorageOnBlockingPool function in StorageInfoProvider will be invoked, and in the function body, GetStorageFreeSpace will be invoked first time to check whether origin mount_path is valid. If so, then start watching timer to check the available capacity of the storage device; Otherwise the watching timer will not start. That means GetStorageFreeSpace has been inovoked one time before the watching timer get started. We should eliminate this function call for one time, that's why we minus delta change_step here.
LGTM. Thanks. https://codereview.chromium.org/16707002/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc (right): https://codereview.chromium.org/16707002/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:31: {"device:001", "C:", systeminfo::kStorageTypeUnknown, 1000, 10-0, 0}, On 2013/06/13 14:01:33, Haojian Wu wrote: > On 2013/06/12 02:20:57, Hongbo Min wrote: > > Could you please explain why to minus the delta change step? > > It's about onAvailableCapacityChanged event implementation. > > In TestStorageInfoProvider class, we override GetStorageFreeSpace method to make > all mock storge devices' available capacity change a given change_step. > > When onAvailableCapacityChanged.AddListener api is invoked in js. > AddWatchedStorageOnBlockingPool function in StorageInfoProvider will be invoked, > and in the function body, GetStorageFreeSpace will be invoked first time to > check whether origin mount_path is valid. If so, then start watching timer to > check the available capacity of the storage device; Otherwise the watching timer > will not start. That means GetStorageFreeSpace has been inovoked one time before > the watching timer get started. > > We should eliminate this function call for one time, that's why we minus delta > change_step here. Fair enough! You can add some comments into the code.
On 2013/06/14 13:22:42, Hongbo Min wrote: > LGTM. Thanks. > > https://codereview.chromium.org/16707002/diff/1/chrome/browser/extensions/api... > File > chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc > (right): > > https://codereview.chromium.org/16707002/diff/1/chrome/browser/extensions/api... > chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:31: > {"device:001", "C:", systeminfo::kStorageTypeUnknown, 1000, 10-0, 0}, > On 2013/06/13 14:01:33, Haojian Wu wrote: > > On 2013/06/12 02:20:57, Hongbo Min wrote: > > > Could you please explain why to minus the delta change step? > > > > It's about onAvailableCapacityChanged event implementation. > > > > In TestStorageInfoProvider class, we override GetStorageFreeSpace method to > make > > all mock storge devices' available capacity change a given change_step. > > > > When onAvailableCapacityChanged.AddListener api is invoked in js. > > AddWatchedStorageOnBlockingPool function in StorageInfoProvider will be > invoked, > > and in the function body, GetStorageFreeSpace will be invoked first time to > > check whether origin mount_path is valid. If so, then start watching timer to > > check the available capacity of the storage device; Otherwise the watching > timer > > will not start. That means GetStorageFreeSpace has been inovoked one time > before > > the watching timer get started. > > > > We should eliminate this function call for one time, that's why we minus delta > > change_step here. > > Fair enough! You can add some comments into the code. Done. Add some comments in apitest and unittest(see system_info_storage_apitest.cc and storage_info_provider_unittest.cc).
You should be deleting storage_info_provider_linux_unittest.cc from chrome_test_unit.gypi in the CL, but I don't see storage_info_provider_linux_unittest.cc in your copy of chrome_test_unit.gypi here at all. Something's a bit off in your git workflow?
https://codereview.chromium.org/16707002/diff/18001/chrome/chrome_tests_unit.... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/16707002/diff/18001/chrome/chrome_tests_unit.... chrome/chrome_tests_unit.gypi:769: 'browser/extensions/api/system_info_storage/storage_info_provider_linux_unittest.cc', Oh, here it is. Find-in-page failed me. Please remove since you are deleting the file.
On 2013/06/17 20:35:45, Lei Zhang wrote: > https://codereview.chromium.org/16707002/diff/18001/chrome/chrome_tests_unit.... > File chrome/chrome_tests_unit.gypi (right): > > https://codereview.chromium.org/16707002/diff/18001/chrome/chrome_tests_unit.... > chrome/chrome_tests_unit.gypi:769: > 'browser/extensions/api/system_info_storage/storage_info_provider_linux_unittest.cc', > Oh, here it is. Find-in-page failed me. > > Please remove since you are deleting the file. Done.
There's a few overall comments here you probably don't want to address in this change. Just learning the code... Lets add TODOs to remember the trajectory we want, though, if you agree with them. https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info/system_info_api.cc (right): https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/system_info/system_info_api.cc:227: base::PostTaskAndReplyWithResult( Shall we change storage monitor to attempt to gather this data as the device is attached? https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info/system_info_provider.h (right): https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/system_info/system_info_provider.h:32: template<class T> I'm not a fan of templates here. Given that we need specific implementation functions anyway to fetch each type, a code format that would be more chromium-like would be a superclass with all the functions needed, and then if there are platform-specific functions, there'd be subclasses to implement those methods for each type of data. https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/system_info/system_info_provider.h:60: void StartQueryInfo(const QueryInfoCompletionCallback& callback) { This can go in a .cc file, along with non-templated methods. https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/system_info/system_info_provider.h:101: virtual void QueryOnWorkerPool() { Instead of trampolines on both sides, could we use PostBlockingPoolTaskAndReply? Or does the template parameter foul that up? Could that be avoided, since we have a "T info_" member, and just not pass the parameter to QueryInfo()? I think this is another argument for just having the known types of system info explicitly referenced instead of using templates. https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.h (right): https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:64: void SetWatchingIntervalForTesting(size_t ms) { watching_interval_ = ms; } Can you add a todo to put this in a testing subclass rather than the api? https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:95: void AddWatchedStorageOnBlockingPool(const base::FilePath path); Change the param name to mount_path to be consistent throughout the api. (and below) https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:97: void RemoveWatchedStorageOnBlockingPool(const base::FilePath path); here https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.h (right): https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.h:23: TestStorageInfoProvider(const struct TestUnitInfo testing_data[], size_t n); Would it be easier to pass a vector to the constructor? Or can we use literals this way? https://codereview.chromium.org/16707002/diff/24001/chrome/common/extensions/... File chrome/common/extensions/api/experimental_system_info_storage.idl (right): https://codereview.chromium.org/16707002/diff/24001/chrome/common/extensions/... chrome/common/extensions/api/experimental_system_info_storage.idl:20: DOMString location; Does this get exposed to apps? That seems like a potential problem. There's no reason to expose this, right? https://codereview.chromium.org/16707002/diff/24001/chrome/common/extensions/... chrome/common/extensions/api/experimental_system_info_storage.idl:23: // The total amount of the storage space, in bytes. Document what the default values are in case we can't establish the capacity or available capacity. https://codereview.chromium.org/16707002/diff/24001/chrome/common/extensions/... chrome/common/extensions/api/experimental_system_info_storage.idl:29: dictionary StorageChangeInfo { How about "StorageCapacityChangeInfo"? https://codereview.chromium.org/16707002/diff/24001/chrome/common/extensions/... chrome/common/extensions/api/experimental_system_info_storage.idl:30: // The uniue id of the storage unit already changed. "unique" https://codereview.chromium.org/16707002/diff/24001/chrome/common/extensions/... chrome/common/extensions/api/experimental_system_info_storage.idl:55: static void addWatch(DOMString id, AddWatchCallback callback); Should attachment/detachment notifications be spliced in here? The StorageUnitInfo already has the type information. Should attachment/detachment notifications follow this same API path?
https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info/system_info_api.cc (right): https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/system_info/system_info_api.cc:227: base::PostTaskAndReplyWithResult( On 2013/06/18 18:24:52, Greg Billock wrote: > Shall we change storage monitor to attempt to gather this data as the device is > attached? If we gather available capacity in storage monitor(add available capacity field in chrome::StorageInfo), then we should do more work in the storage monitor(monitor free space change event for each attached storage device), right? Should we really need to add available capacity field in storage monitor? https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info/system_info_provider.h (right): https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/system_info/system_info_provider.h:32: template<class T> On 2013/06/18 18:24:52, Greg Billock wrote: > I'm not a fan of templates here. Given that we need specific implementation > functions anyway to fetch each type, a code format that would be more > chromium-like would be a superclass with all the functions needed, and then if > there are platform-specific functions, there'd be subclasses to implement those > methods for each type of data. hongbo@, how about your idea? Should we refractor the whole SystemInfo API skeleton? https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/system_info/system_info_provider.h:60: void StartQueryInfo(const QueryInfoCompletionCallback& callback) { On 2013/06/18 18:24:52, Greg Billock wrote: > This can go in a .cc file, along with non-templated methods. I will create a new CL to separate the declaration and definition of SystemInfoProvider. https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/system_info/system_info_provider.h:101: virtual void QueryOnWorkerPool() { On 2013/06/18 18:24:52, Greg Billock wrote: > Instead of trampolines on both sides, could we use PostBlockingPoolTaskAndReply? > Or does the template parameter foul that up? Could that be avoided, since we > have a "T info_" member, and just not pass the parameter to QueryInfo()? > > I think this is another argument for just having the known types of system info > explicitly referenced instead of using templates. hongbo@, your idea? https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.h (right): https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:64: void SetWatchingIntervalForTesting(size_t ms) { watching_interval_ = ms; } On 2013/06/18 18:24:52, Greg Billock wrote: > Can you add a todo to put this in a testing subclass rather than the api? Done. https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:95: void AddWatchedStorageOnBlockingPool(const base::FilePath path); On 2013/06/18 18:24:52, Greg Billock wrote: > Change the param name to mount_path to be consistent throughout the api. (and > below) Done. https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:97: void RemoveWatchedStorageOnBlockingPool(const base::FilePath path); On 2013/06/18 18:24:52, Greg Billock wrote: > here Done. https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.h (right): https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.h:23: TestStorageInfoProvider(const struct TestUnitInfo testing_data[], size_t n); On 2013/06/18 18:24:52, Greg Billock wrote: > Would it be easier to pass a vector to the constructor? Or can we use literals > this way? I suppose not. Since we define testing_data as a global array in apitest and unittest. https://codereview.chromium.org/16707002/diff/24001/chrome/common/extensions/... File chrome/common/extensions/api/experimental_system_info_storage.idl (right): https://codereview.chromium.org/16707002/diff/24001/chrome/common/extensions/... chrome/common/extensions/api/experimental_system_info_storage.idl:20: DOMString location; On 2013/06/18 18:24:52, Greg Billock wrote: > Does this get exposed to apps? That seems like a potential problem. There's no > reason to expose this, right? Currently I'm not sure. The systemInfo.storage api definition is still pending and we need to communicate with extension team. This CL is only for rewriting StorageInfoProvider rather than changing the definition. According to hongbo@ review comments, I add location field here. https://codereview.chromium.org/16707002/diff/24001/chrome/common/extensions/... chrome/common/extensions/api/experimental_system_info_storage.idl:23: // The total amount of the storage space, in bytes. On 2013/06/18 18:24:52, Greg Billock wrote: > Document what the default values are in case we can't establish the capacity or > available capacity. Done. https://codereview.chromium.org/16707002/diff/24001/chrome/common/extensions/... chrome/common/extensions/api/experimental_system_info_storage.idl:29: dictionary StorageChangeInfo { On 2013/06/18 18:24:52, Greg Billock wrote: > How about "StorageCapacityChangeInfo"? Done. https://codereview.chromium.org/16707002/diff/24001/chrome/common/extensions/... chrome/common/extensions/api/experimental_system_info_storage.idl:30: // The uniue id of the storage unit already changed. On 2013/06/18 18:24:52, Greg Billock wrote: > "unique" Done. https://codereview.chromium.org/16707002/diff/24001/chrome/common/extensions/... chrome/common/extensions/api/experimental_system_info_storage.idl:55: static void addWatch(DOMString id, AddWatchCallback callback); On 2013/06/18 18:24:52, Greg Billock wrote: > Should attachment/detachment notifications be spliced in here? The > StorageUnitInfo already has the type information. Should attachment/detachment > notifications follow this same API path? Here we keep the systemInfo storage api definition the same as media_galleries_private.
Billock and Haojian, please see my comments below. https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info/system_info_provider.h (right): https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/system_info/system_info_provider.h:32: template<class T> On 2013/06/18 18:24:52, Greg Billock wrote: > I'm not a fan of templates here. Given that we need specific implementation > functions anyway to fetch each type, a code format that would be more > chromium-like would be a superclass with all the functions needed, and then if > there are platform-specific functions, there'd be subclasses to implement those > methods for each type of data. Original thought on type T is, it is a type generated by IDL parser based on a dictionary declaration in IDL file. In this way, we won't need to build a new class hierarchy for system information categories. https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/system_info/system_info_provider.h:101: virtual void QueryOnWorkerPool() { On 2013/06/21 05:49:04, Haojian Wu wrote: > On 2013/06/18 18:24:52, Greg Billock wrote: > > Instead of trampolines on both sides, could we use > PostBlockingPoolTaskAndReply? > > Or does the template parameter foul that up? Could that be avoided, since we > > have a "T info_" member, and just not pass the parameter to QueryInfo()? > > > > I think this is another argument for just having the known types of system > info > > explicitly referenced instead of using templates. > > hongbo@, your idea? Thanks for Billock to raise these improvements. I spent a little time to see why QueryInfo is designed to accept a T-typed pointer as paramter, it is mainly for querying free space for storages when checking the free space change, but right now this condition for that is now not valid since StorageInfoProvider::QueryUnitInfo is added. I agree with Billock's suggestions: 1) No need a T-typed pointer in QueryInfo methods; 2) Could use PostBlockingPoolTaskAndReply to avoid unnecessary trampolines trip. It is OK to me if doing this in another CL since it will touch all SystemInfo related code. https://codereview.chromium.org/16707002/diff/24001/chrome/common/extensions/... File chrome/common/extensions/api/experimental_system_info_storage.idl (right): https://codereview.chromium.org/16707002/diff/24001/chrome/common/extensions/... chrome/common/extensions/api/experimental_system_info_storage.idl:20: DOMString location; On 2013/06/21 05:49:04, Haojian Wu wrote: > On 2013/06/18 18:24:52, Greg Billock wrote: > > Does this get exposed to apps? That seems like a potential problem. There's no > > reason to expose this, right? > > Currently I'm not sure. The systemInfo.storage api definition is still pending > and we need to communicate with extension team. This CL is only for rewriting > StorageInfoProvider rather than changing the definition. According to hongbo@ > review comments, I add location field here. The reason why it should be exposed is, it could tell user which storage is mounted, on desktop, user might go back the underlying OS to check it. https://codereview.chromium.org/16707002/diff/24001/chrome/common/extensions/... chrome/common/extensions/api/experimental_system_info_storage.idl:55: static void addWatch(DOMString id, AddWatchCallback callback); On 2013/06/21 05:49:04, Haojian Wu wrote: > On 2013/06/18 18:24:52, Greg Billock wrote: > > Should attachment/detachment notifications be spliced in here? The > > StorageUnitInfo already has the type information. Should attachment/detachment > > notifications follow this same API path? > > Here we keep the systemInfo storage api definition the same as > media_galleries_private. Haojian, please have a look at https://code.google.com/p/chromium/issues/detail?id=252994
https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info/system_info_provider.h (right): https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/system_info/system_info_provider.h:101: virtual void QueryOnWorkerPool() { On 2013/06/22 09:52:28, Hongbo Min wrote: > On 2013/06/21 05:49:04, Haojian Wu wrote: > > On 2013/06/18 18:24:52, Greg Billock wrote: > > > Instead of trampolines on both sides, could we use > > PostBlockingPoolTaskAndReply? > > > Or does the template parameter foul that up? Could that be avoided, since we > > > have a "T info_" member, and just not pass the parameter to QueryInfo()? > > > > > > I think this is another argument for just having the known types of system > > info > > > explicitly referenced instead of using templates. > > > > hongbo@, your idea? > > Thanks for Billock to raise these improvements. > > I spent a little time to see why QueryInfo is designed to accept a T-typed > pointer as paramter, it is mainly for querying free space for storages when > checking the free space change, but right now this condition for that is now not > valid since StorageInfoProvider::QueryUnitInfo is added. > > I agree with Billock's suggestions: > 1) No need a T-typed pointer in QueryInfo methods; > 2) Could use PostBlockingPoolTaskAndReply to avoid unnecessary trampolines trip. > > It is OK to me if doing this in another CL since it will touch all SystemInfo > related code. Done. Add TODO comments. https://codereview.chromium.org/16707002/diff/24001/chrome/common/extensions/... File chrome/common/extensions/api/experimental_system_info_storage.idl (right): https://codereview.chromium.org/16707002/diff/24001/chrome/common/extensions/... chrome/common/extensions/api/experimental_system_info_storage.idl:29: dictionary StorageChangeInfo { On 2013/06/21 05:49:04, Haojian Wu wrote: > On 2013/06/18 18:24:52, Greg Billock wrote: > > How about "StorageCapacityChangeInfo"? > > Done. Now I doubt whether we should change the name here according to the https://code.google.com/p/chromium/issues/detail?id=252994
https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info/system_info_api.cc (right): https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/system_info/system_info_api.cc:227: base::PostTaskAndReplyWithResult( On 2013/06/21 05:49:04, Haojian Wu wrote: > On 2013/06/18 18:24:52, Greg Billock wrote: > > Shall we change storage monitor to attempt to gather this data as the device > is > > attached? > > If we gather available capacity in storage monitor(add available capacity field > in chrome::StorageInfo), then we should do more work in the storage > monitor(monitor free space change event for each attached storage device), > right? > > Should we really need to add available capacity field in storage monitor? I think we've decided to keep that experimental, so let's just leave it as is for now. If we end up taking that out of experimental, we may want to put that work into StorageMonitor at that point. https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info/system_info_provider.h (right): https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/system_info/system_info_provider.h:32: template<class T> On 2013/06/22 09:52:28, Hongbo Min wrote: > On 2013/06/18 18:24:52, Greg Billock wrote: > > I'm not a fan of templates here. Given that we need specific implementation > > functions anyway to fetch each type, a code format that would be more > > chromium-like would be a superclass with all the functions needed, and then if > > there are platform-specific functions, there'd be subclasses to implement > those > > methods for each type of data. > > Original thought on type T is, it is a type generated by IDL parser based on a > dictionary declaration in IDL file. In this way, we won't need to build a new > class hierarchy for system information categories. I see. Do you foresee enough specializations to make that important? I think that you've got a pretty complete set, so that wouldn't be a big saver of complexity. https://codereview.chromium.org/16707002/diff/24001/chrome/browser/extensions... chrome/browser/extensions/api/system_info/system_info_provider.h:60: void StartQueryInfo(const QueryInfoCompletionCallback& callback) { On 2013/06/21 05:49:04, Haojian Wu wrote: > On 2013/06/18 18:24:52, Greg Billock wrote: > > This can go in a .cc file, along with non-templated methods. > > I will create a new CL to separate the declaration and definition of > SystemInfoProvider. Sounds good. https://codereview.chromium.org/16707002/diff/24001/chrome/common/extensions/... File chrome/common/extensions/api/experimental_system_info_storage.idl (right): https://codereview.chromium.org/16707002/diff/24001/chrome/common/extensions/... chrome/common/extensions/api/experimental_system_info_storage.idl:20: DOMString location; On 2013/06/22 09:52:28, Hongbo Min wrote: > On 2013/06/21 05:49:04, Haojian Wu wrote: > > On 2013/06/18 18:24:52, Greg Billock wrote: > > > Does this get exposed to apps? That seems like a potential problem. There's > no > > > reason to expose this, right? > > > > Currently I'm not sure. The systemInfo.storage api definition is still pending > > and we need to communicate with extension team. This CL is only for rewriting > > StorageInfoProvider rather than changing the definition. According to hongbo@ > > review comments, I add location field here. > > The reason why it should be exposed is, it could tell user which storage is > mounted, on desktop, user might go back the underlying OS to check it. That's sensible. I think we should work to make the name more informative, though. Especially as that's going to be more useful cross-platform -- there may not really be a user-helpful location for some storages (i.e. MTP devices). How about passing the name instead of the location? https://codereview.chromium.org/16707002/diff/24001/chrome/common/extensions/... chrome/common/extensions/api/experimental_system_info_storage.idl:29: dictionary StorageChangeInfo { On 2013/06/23 07:59:16, Haojian Wu wrote: > On 2013/06/21 05:49:04, Haojian Wu wrote: > > On 2013/06/18 18:24:52, Greg Billock wrote: > > > How about "StorageCapacityChangeInfo"? > > > > Done. > > Now I doubt whether we should change the name here according to the > https://code.google.com/p/chromium/issues/detail?id=252994 I think the new name is fine -- we can update the docs as we move things out of experimental. https://codereview.chromium.org/16707002/diff/24001/chrome/common/extensions/... chrome/common/extensions/api/experimental_system_info_storage.idl:55: static void addWatch(DOMString id, AddWatchCallback callback); On 2013/06/22 09:52:28, Hongbo Min wrote: > On 2013/06/21 05:49:04, Haojian Wu wrote: > > On 2013/06/18 18:24:52, Greg Billock wrote: > > > Should attachment/detachment notifications be spliced in here? The > > > StorageUnitInfo already has the type information. Should > attachment/detachment > > > notifications follow this same API path? > > > > Here we keep the systemInfo storage api definition the same as > > media_galleries_private. > > Haojian, please have a look at > https://code.google.com/p/chromium/issues/detail?id=252994 > Yeah, these are going to split as most of the API moves out of experimental, so it should stay as is. https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.h (right): https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:35: typedef std::vector<linked_ptr< Can you add a TODO to see if we can get by without linked_ptrs here? These objects are pretty light -- we can probably just have a copy constructor and copy them when needed. Should be rare. https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc (right): https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:30: // Available capacity in TestUnitInfo should minus the detla change step once "minus" --> "subtract" "delta" https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:31: // for onAvailableCapacityChanged event. "for every onAvailableCapacityChanged event." https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:32: // In the onAvailableCapacityChanged event implementation(StorageInfoProvider:: I didn't follow this. Is it a TODO to eliminate the call? Or are you saying the override eliminates this? If the latter, I don't think you need to add a comment here about it. Perhaps in that override itself. https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:60: const int kMaxChangeTimes = 10; How about "kMaxSpaceChanges" https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:92: change_times_ = 0; Should this be here? How about an ASSERT that change_times_ <= kMaxChangeTimes https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:160: // Quit the message loop even if no free space change happens after 500ms This should never happen right? If the test breaks, we want it to hang and be killed by the machinery. https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/system_info_storage_apitest.cc (right): https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/system_info_storage_apitest.cc:28: // Available capacity in TestUnitInfo should minus the detla change step once. See comment about similar comment in the unit test.
Billock, thanks for your careful review. As to SystemInfoProvider refactoring (e.g. move some impl into .cc file, template parameter T usage etc), I suggest to open a separate CL to do it. In this CL, let's focus on the issue of reusing StorageMonitor to implement StorageInfoProvider. Otherwise, we might get the things mixed together. Does it make sense? https://codereview.chromium.org/16707002/diff/24001/chrome/common/extensions/... File chrome/common/extensions/api/experimental_system_info_storage.idl (right): https://codereview.chromium.org/16707002/diff/24001/chrome/common/extensions/... chrome/common/extensions/api/experimental_system_info_storage.idl:20: DOMString location; On 2013/06/24 16:55:38, Greg Billock wrote: > On 2013/06/22 09:52:28, Hongbo Min wrote: > > On 2013/06/21 05:49:04, Haojian Wu wrote: > > > On 2013/06/18 18:24:52, Greg Billock wrote: > > > > Does this get exposed to apps? That seems like a potential problem. > There's > > no > > > > reason to expose this, right? > > > > > > Currently I'm not sure. The systemInfo.storage api definition is still > pending > > > and we need to communicate with extension team. This CL is only for > rewriting > > > StorageInfoProvider rather than changing the definition. According to > hongbo@ > > > review comments, I add location field here. > > > > The reason why it should be exposed is, it could tell user which storage is > > mounted, on desktop, user might go back the underlying OS to check it. > > That's sensible. I think we should work to make the name more informative, > though. Especially as that's going to be more useful cross-platform -- there may > not really be a user-helpful location for some storages (i.e. MTP devices). How > about passing the name instead of the location? Agree! https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.h (right): https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:35: typedef std::vector<linked_ptr< On 2013/06/24 16:55:38, Greg Billock wrote: > Can you add a TODO to see if we can get by without linked_ptrs here? These > objects are pretty light -- we can probably just have a copy constructor and > copy them when needed. Should be rare. Since the type T generated by IDL parser disallows copy constructor and assignment. So use linked_ptr instead here. https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc (right): https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:160: // Quit the message loop even if no free space change happens after 500ms On 2013/06/24 16:55:38, Greg Billock wrote: > This should never happen right? > > If the test breaks, we want it to hang and be killed by the machinery. Our expectation is no free space change happens. This is a way to quit message, otherwise, the test will hang although it is in our expectation.
I mostly have nits to pick, but there's also a couple real concerns. https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info/system_info_api.cc (right): https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info/system_info_api.cc:35: using api::experimental_system_info_storage::StorageCapacityChangeInfo; nit: alphabetical order https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info/system_info_api.cc:120: DCHECK(chrome::StorageMonitor::GetInstance()); I wouldn't bother with this DCHECK(). If the next line crashes will a NULL pointer dereference, it's pretty obvious what happened. https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info/system_info_provider.h (right): https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info/system_info_provider.h:102: // TODO(Haojian): use PostBlockingPoolTaskAndReply to avoid unnecessay nit: typo (unnecessay) https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc (right): https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:188: if (!ContainsKey(storage_location_to_size_map_, mount_path)) You don't need this check. Just call map::erase() below and see what the returned value is to determine whether to return or not. https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:231: double old_available_bytes = storage_location_to_size_map_[mount_path]; Erm, you could have already erased the entry for |mount_path| on line 223. If that happens, this line will add a new entry back for |mount_path| as a side effect. https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:237: id, /* storage id */ The comments are not necessary. The variable names are already very descriptive. https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:242: } nit: spacing https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.h (right): https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:39: : public SystemInfoProvider<StorageInfo> { fits on previous line https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:75: // pool, including fixed and removable.. nit: double periods https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:80: void QueryInfoImplOnUIThread(); Add a comment. "Run the QueryInfo() implementation after the StorageMonitor has been initialized." https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/system_info_storage_apitest.cc (right): https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/system_info_storage_apitest.cc:28: // Available capacity in TestUnitInfo should minus the detla change step once. nit: typo (detla) https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/system_info_storage_apitest.cc:66: size_t len = sizeof(kRemovableStorageData) / sizeof(TestUnitInfo); Use arraysize() https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.cc (right): https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.cc:48: for (size_t i = 0; i < testing_data_.size(); i++) { ++i, same for line 72. Be consistent with line 64. https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.h (right): https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.h:15: double capacity; Mention these are in bytes, or whatever unit. https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.h:17: // The change step of free space. I don't understand what exactly this means. If it means how fast free space is becoming available, for example, what's the unit? Megabytes / second? Bytes per hour? https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.h:23: TestStorageInfoProvider(const struct TestUnitInfo testing_data[], size_t n); I think you can just use a const struct TestUnitInfo* for the first parameter. https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.h:35: std::vector<struct TestUnitInfo> testing_data_; #include <vector>
Sorry for the delayed update since I'm busy with my graduation things these days. https://codereview.chromium.org/16707002/diff/24001/chrome/common/extensions/... File chrome/common/extensions/api/experimental_system_info_storage.idl (right): https://codereview.chromium.org/16707002/diff/24001/chrome/common/extensions/... chrome/common/extensions/api/experimental_system_info_storage.idl:20: DOMString location; On 2013/06/25 01:55:07, Hongbo Min wrote: > On 2013/06/24 16:55:38, Greg Billock wrote: > > On 2013/06/22 09:52:28, Hongbo Min wrote: > > > On 2013/06/21 05:49:04, Haojian Wu wrote: > > > > On 2013/06/18 18:24:52, Greg Billock wrote: > > > > > Does this get exposed to apps? That seems like a potential problem. > > There's > > > no > > > > > reason to expose this, right? > > > > > > > > Currently I'm not sure. The systemInfo.storage api definition is still > > pending > > > > and we need to communicate with extension team. This CL is only for > > rewriting > > > > StorageInfoProvider rather than changing the definition. According to > > hongbo@ > > > > review comments, I add location field here. > > > > > > The reason why it should be exposed is, it could tell user which storage is > > > mounted, on desktop, user might go back the underlying OS to check it. > > > > That's sensible. I think we should work to make the name more informative, > > though. Especially as that's going to be more useful cross-platform -- there > may > > not really be a user-helpful location for some storages (i.e. MTP devices). > How > > about passing the name instead of the location? > > Agree! Does this means changing the IDL name "location" to "name" and retrieve the value through "StorageMonitor::name()" instead of "StorageMonitor::location()"? https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info/system_info_api.cc (right): https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info/system_info_api.cc:35: using api::experimental_system_info_storage::StorageCapacityChangeInfo; On 2013/06/25 04:49:55, Lei Zhang wrote: > nit: alphabetical order Done. https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info/system_info_api.cc:120: DCHECK(chrome::StorageMonitor::GetInstance()); On 2013/06/25 04:49:55, Lei Zhang wrote: > I wouldn't bother with this DCHECK(). If the next line crashes will a NULL > pointer dereference, it's pretty obvious what happened. Done. https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info/system_info_provider.h (right): https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info/system_info_provider.h:102: // TODO(Haojian): use PostBlockingPoolTaskAndReply to avoid unnecessay On 2013/06/25 04:49:55, Lei Zhang wrote: > nit: typo (unnecessay) Done. https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc (right): https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:188: if (!ContainsKey(storage_location_to_size_map_, mount_path)) On 2013/06/25 04:49:55, Lei Zhang wrote: > You don't need this check. Just call map::erase() below and see what the > returned value is to determine whether to return or not. Done. https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:231: double old_available_bytes = storage_location_to_size_map_[mount_path]; On 2013/06/25 04:49:55, Lei Zhang wrote: > Erm, you could have already erased the entry for |mount_path| on line 223. If > that happens, this line will add a new entry back for |mount_path| as a side > effect. Done. It should return directly if the case on line 223 happens. https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:237: id, /* storage id */ On 2013/06/25 04:49:55, Lei Zhang wrote: > The comments are not necessary. The variable names are already very descriptive. Done. https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:242: } On 2013/06/25 04:49:55, Lei Zhang wrote: > nit: spacing Done. https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.h (right): https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:35: typedef std::vector<linked_ptr< On 2013/06/25 01:55:07, Hongbo Min wrote: > On 2013/06/24 16:55:38, Greg Billock wrote: > > Can you add a TODO to see if we can get by without linked_ptrs here? These > > objects are pretty light -- we can probably just have a copy constructor and > > copy them when needed. Should be rare. > > Since the type T generated by IDL parser disallows copy constructor and > assignment. So use linked_ptr instead here. As hongbo@ said, the StorageUnitInfo class which is auto generated from IDL definition file is *DISALLOW_COPY_AND_ASSIGN*. https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:39: : public SystemInfoProvider<StorageInfo> { On 2013/06/25 04:49:55, Lei Zhang wrote: > fits on previous line Done. https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:75: // pool, including fixed and removable.. On 2013/06/25 04:49:55, Lei Zhang wrote: > nit: double periods Done. https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:80: void QueryInfoImplOnUIThread(); On 2013/06/25 04:49:55, Lei Zhang wrote: > Add a comment. "Run the QueryInfo() implementation after the StorageMonitor has > been initialized." Done. https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc (right): https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:32: // In the onAvailableCapacityChanged event implementation(StorageInfoProvider:: On 2013/06/24 16:55:38, Greg Billock wrote: > I didn't follow this. Is it a TODO to eliminate the call? Or are you saying the > override eliminates this? If the latter, I don't think you need to add a comment > here about it. Perhaps in that override itself. I have adjusted the code of TestStorageInfoProvider::GetStorageFreeSpace(hongbo@, please review this function). Now there is no need to subtract the delta change step. Remove the comment. https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:60: const int kMaxChangeTimes = 10; On 2013/06/24 16:55:38, Greg Billock wrote: > How about "kMaxSpaceChanges" Done. https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:92: change_times_ = 0; On 2013/06/24 16:55:38, Greg Billock wrote: > Should this be here? How about an ASSERT that change_times_ <= kMaxChangeTimes From my point of view, it should be here. If change_times >= kMaxChangeTimes, we should quit the message loop(post a QuitClosure) and give UI thread a chance to verify the result. But the QuitClosure task is not processed synchronously. OnFreeSpaceChanged function may be called after post QuitClosure to UI thread. https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/system_info_storage_apitest.cc (right): https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/system_info_storage_apitest.cc:28: // Available capacity in TestUnitInfo should minus the detla change step once. On 2013/06/25 04:49:55, Lei Zhang wrote: > nit: typo (detla) Remove the comment now. See comment in the unit test. https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/system_info_storage_apitest.cc:66: size_t len = sizeof(kRemovableStorageData) / sizeof(TestUnitInfo); On 2013/06/25 04:49:55, Lei Zhang wrote: > Use arraysize() Done. https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.cc (right): https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.cc:48: for (size_t i = 0; i < testing_data_.size(); i++) { On 2013/06/25 04:49:55, Lei Zhang wrote: > ++i, same for line 72. Be consistent with line 64. Done. https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.h (right): https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.h:15: double capacity; On 2013/06/25 04:49:55, Lei Zhang wrote: > Mention these are in bytes, or whatever unit. Done. https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.h:17: // The change step of free space. On 2013/06/25 04:49:55, Lei Zhang wrote: > I don't understand what exactly this means. If it means how fast free space is > becoming available, for example, what's the unit? Megabytes / second? Bytes per > hour? The change_step here is used for simulating the free space change.Increasing the |available_capacity| with a fixed change_step. I have updated the comment now. https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.h:23: TestStorageInfoProvider(const struct TestUnitInfo testing_data[], size_t n); On 2013/06/25 04:49:55, Lei Zhang wrote: > I think you can just use a const struct TestUnitInfo* for the first parameter. Done. https://codereview.chromium.org/16707002/diff/35001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.h:35: std::vector<struct TestUnitInfo> testing_data_; On 2013/06/25 04:49:55, Lei Zhang wrote: > #include <vector> Done.
Thanks for addressing my nits and congratulations on finishing school. https://codereview.chromium.org/16707002/diff/56001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc (right): https://codereview.chromium.org/16707002/diff/56001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:222: return; This addresses the issue from line 231, but now you are skipping the if block on line 225.
https://codereview.chromium.org/16707002/diff/56001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc (right): https://codereview.chromium.org/16707002/diff/56001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:222: return; On 2013/06/26 03:30:38, Lei Zhang wrote: > This addresses the issue from line 231, but now you are skipping the if block on > line 225. Done. Thanks for the careful review. I aslo notice that on line 228 we should also return directly since storage_location_to_size_map_ is empty.
https://codereview.chromium.org/16707002/diff/24001/chrome/common/extensions/... File chrome/common/extensions/api/experimental_system_info_storage.idl (right): https://codereview.chromium.org/16707002/diff/24001/chrome/common/extensions/... chrome/common/extensions/api/experimental_system_info_storage.idl:20: DOMString location; Yes. That sounds right. I saw a note from Steve that he supports this change as well. On 2013/06/26 03:22:39, Haojian Wu wrote: > On 2013/06/25 01:55:07, Hongbo Min wrote: > > On 2013/06/24 16:55:38, Greg Billock wrote: > > > On 2013/06/22 09:52:28, Hongbo Min wrote: > > > > On 2013/06/21 05:49:04, Haojian Wu wrote: > > > > > On 2013/06/18 18:24:52, Greg Billock wrote: > > > > > > Does this get exposed to apps? That seems like a potential problem. > > > There's > > > > no > > > > > > reason to expose this, right? > > > > > > > > > > Currently I'm not sure. The systemInfo.storage api definition is still > > > pending > > > > > and we need to communicate with extension team. This CL is only for > > > rewriting > > > > > StorageInfoProvider rather than changing the definition. According to > > > hongbo@ > > > > > review comments, I add location field here. > > > > > > > > The reason why it should be exposed is, it could tell user which storage > is > > > > mounted, on desktop, user might go back the underlying OS to check it. > > > > > > That's sensible. I think we should work to make the name more informative, > > > though. Especially as that's going to be more useful cross-platform -- there > > may > > > not really be a user-helpful location for some storages (i.e. MTP devices). > > How > > > about passing the name instead of the location? > > > > Agree! > > Does this means changing the IDL name "location" to "name" and retrieve the > value through "StorageMonitor::name()" instead of "StorageMonitor::location()"?
I have renamed 'location' to 'name'. Also I combine GetStorageFreeSpace and GetStoragePathFromId functions into GetStorageFreeSpaceFromId for simplify. hongbo@, please review it again. Thanks. https://codereview.chromium.org/16707002/diff/24001/chrome/common/extensions/... File chrome/common/extensions/api/experimental_system_info_storage.idl (right): https://codereview.chromium.org/16707002/diff/24001/chrome/common/extensions/... chrome/common/extensions/api/experimental_system_info_storage.idl:20: DOMString location; On 2013/06/26 17:32:48, Greg Billock wrote: > Yes. That sounds right. I saw a note from Steve that he supports this change as > well. > > On 2013/06/26 03:22:39, Haojian Wu wrote: > > On 2013/06/25 01:55:07, Hongbo Min wrote: > > > On 2013/06/24 16:55:38, Greg Billock wrote: > > > > On 2013/06/22 09:52:28, Hongbo Min wrote: > > > > > On 2013/06/21 05:49:04, Haojian Wu wrote: > > > > > > On 2013/06/18 18:24:52, Greg Billock wrote: > > > > > > > Does this get exposed to apps? That seems like a potential problem. > > > > There's > > > > > no > > > > > > > reason to expose this, right? > > > > > > > > > > > > Currently I'm not sure. The systemInfo.storage api definition is still > > > > pending > > > > > > and we need to communicate with extension team. This CL is only for > > > > rewriting > > > > > > StorageInfoProvider rather than changing the definition. According to > > > > hongbo@ > > > > > > review comments, I add location field here. > > > > > > > > > > The reason why it should be exposed is, it could tell user which storage > > is > > > > > mounted, on desktop, user might go back the underlying OS to check it. > > > > > > > > That's sensible. I think we should work to make the name more informative, > > > > though. Especially as that's going to be more useful cross-platform -- > there > > > may > > > > not really be a user-helpful location for some storages (i.e. MTP > devices). > > > How > > > > about passing the name instead of the location? > > > > > > Agree! > > > > Does this means changing the IDL name "location" to "name" and retrieve the > > value through "StorageMonitor::name()" instead of > "StorageMonitor::location()"? > Done.
Haojian, some nits are added, you are getting there close. https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc (right): https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:57: StorageMonitor::GetInstance()->Initialize( Just a reminder: you need to use EnsureInitialized since https://codereview.chromium.org/16472007/ gets landed. https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:135: storage_list.begin(); nit: Please adjust your statement to keep a clean indent, reference line 92 https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:200: double available_bytes = GetStorageFreeSpaceFromId(id); since GetStorageFreeSpaceFromId will return -1 in case of |id| is not found in |storage_id_to_size_map_|, it should be safe to remove line 196~198, right? https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:212: if (available_bytes < 0) You can avoid this if-clause to make the code compact: if (available_bytes < 0) { storage_id_to_size_map_.erase(id); if (storage_id_to_size_map_.size() == 0) { ... } return; } https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.h (right): https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:56: // Get the amount of storage free space from |id|, nits: no need 2 lines. append line 57 to line 56. https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.h (right): https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.h:22: // Increasing |available_capacity| with a fixed change_step. nit: Each querying operation will increase the |available_capacity| with this value.
https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info/system_info_provider.h (right): https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info/system_info_provider.h:76: virtual void StartQueryInfoImpl() { Is this OK for the non-storage SystemInfoProvider implementations as well? https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc (right): https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:30: unit->id = info.device_id(); (!) Needs to use the Transient IDs call here. https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:31: unit->name = WideToUTF8(info.name()); UTF16ToUTF8 https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:74: for (StorageInfo::iterator it = info_.begin(); it != info_.end(); ++it) { (!) I don't think it is safe to use info_ across threads like this. Instead pass curried parameters to the other threads, and then swap them in on the UI thread only. That'll make sure that multiple calls don't end up colliding on the un-mutexed variable. https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:89: bool StorageInfoProvider::QueryInfo(StorageInfo* info) { can this be const? https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:130: int64 StorageInfoProvider::GetStorageFreeSpaceFromId(const std::string& id) { can this be const? https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:179: void StorageInfoProvider::CheckWatchedStorages() { can this be const? https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:212: if (available_bytes < 0) On 2013/06/27 12:35:26, Hongbo Min wrote: > You can avoid this if-clause to make the code compact: > > if (available_bytes < 0) { > storage_id_to_size_map_.erase(id); > if (storage_id_to_size_map_.size() == 0) { > ... > } > return; > } I think an early return is better. In fact, I'd suggest replacing the stanza below with an early return as well: if (old_available_bytes == storage_id_to_size_map_[id]) return; .... <--- the rest can be unindented. https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc (right): https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:156: MessageLoop::current()->PostDelayedTask(FROM_HERE, (!) I think we discussed this before, but I don't see the reason for this. If this is an expected thing -- no more changes -- then we need to figure that out without waiting 500ms. If its an unexpected thing, where is the check for that? I don't think we need to wait 500ms here. https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.cc (right): https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.cc:33: UTF8ToWide(unit.name), UTF8ToUTF16 https://codereview.chromium.org/16707002/diff/68001/chrome/common/extensions/... File chrome/common/extensions/api/experimental_system_info_storage.idl (right): https://codereview.chromium.org/16707002/diff/68001/chrome/common/extensions/... chrome/common/extensions/api/experimental_system_info_storage.idl:17: // The unique storage id. It is persistent between storage attachments. (!) This shouldn't be documented as persistent -- it'll use the transient ID.
https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc (right): https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:74: for (StorageInfo::iterator it = info_.begin(); it != info_.end(); ++it) { On 2013/06/27 17:12:22, Greg Billock wrote: > (!) I don't think it is safe to use info_ across threads like this. Instead pass > curried parameters to the other threads, and then swap them in on the UI thread > only. That'll make sure that multiple calls don't end up colliding on the > un-mutexed variable. I ever considered the thread-safe issue for |info_| access across threads. It is actually safe to access |info_| cross threads in such a usage scenario. The code might be a little obscure but it does. You can reference its base class SystemInfoProvider which maintains a variable |is_waiting_for_completion_| on UI thread. The |is_waiting_for_completion_| is used to ensure that only one thread is accessing the |info_| member at the same time. In case of |is_waiting_for_completion_| is true, the callback will be queued and so no entry will be access |info_| until SystemInfoProvider::OnQueryCompleted is called on UI thread. And StorageInfoProvider is also running in such an assumption, only OnQueryCompleted gets called on UI thread which resets |is_waiting_for_completion_| to false, means that the |info_| could be accessed on UI thread again. https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:89: bool StorageInfoProvider::QueryInfo(StorageInfo* info) { On 2013/06/27 17:12:22, Greg Billock wrote: > can this be const? No. It is derived from SystemInfoProvider base class. The virtual QueryInfo is not defined as const since I don't want to pose too much assumptions to the concrete implementation of QueryInfo. https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc (right): https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:156: MessageLoop::current()->PostDelayedTask(FROM_HERE, On 2013/06/27 17:12:22, Greg Billock wrote: > (!) I think we discussed this before, but I don't see the reason for this. If > this is an expected thing -- no more changes -- then we need to figure that out > without waiting 500ms. If its an unexpected thing, where is the check for that? > I don't think we need to wait 500ms here. You might miss the explanation. The expectation is, if OnFreeSpaceChanged is not called during 500ms interval, it passed. Otherwise, the EXPECT_CALL could catch the failure since its times is set to 0. Of course, 100ms or less is also fine. It is just a testing time window.
https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc (right): https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:156: MessageLoop::current()->PostDelayedTask(FROM_HERE, Right. Testing time windows are bad. We have several thousand tests -- if we get in the habit of waiting half a second for them to pass, it makes the unit tests run unacceptably slow. Let's find a better way to signal this affirmatively. On 2013/06/28 02:29:54, Hongbo Min wrote: > On 2013/06/27 17:12:22, Greg Billock wrote: > > (!) I think we discussed this before, but I don't see the reason for this. If > > this is an expected thing -- no more changes -- then we need to figure that > out > > without waiting 500ms. If its an unexpected thing, where is the check for > that? > > I don't think we need to wait 500ms here. > > You might miss the explanation. The expectation is, if OnFreeSpaceChanged is not > called during 500ms interval, it passed. Otherwise, the EXPECT_CALL could catch > the failure since its times is set to 0. > > Of course, 100ms or less is also fine. It is just a testing time window. >
Now using the transient id as |id| field in StorageUnitInfo. And I also rename responding 'id' to 'transient_id' for better understanding. Please review it again. Thanks. https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info/system_info_provider.h (right): https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info/system_info_provider.h:76: virtual void StartQueryInfoImpl() { On 2013/06/27 17:12:22, Greg Billock wrote: > Is this OK for the non-storage SystemInfoProvider implementations as well? Yes. https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc (right): https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:30: unit->id = info.device_id(); On 2013/06/27 17:12:22, Greg Billock wrote: > (!) Needs to use the Transient IDs call here. Done. https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:31: unit->name = WideToUTF8(info.name()); On 2013/06/27 17:12:22, Greg Billock wrote: > UTF16ToUTF8 Done. https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:57: StorageMonitor::GetInstance()->Initialize( On 2013/06/27 12:35:26, Hongbo Min wrote: > Just a reminder: you need to use EnsureInitialized since > https://codereview.chromium.org/16472007/ gets landed. Done. https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:130: int64 StorageInfoProvider::GetStorageFreeSpaceFromId(const std::string& id) { On 2013/06/27 17:12:22, Greg Billock wrote: > can this be const? I suppose not. Since TestStorageInfoProvider will override this method and in the implementaion it will change its class member |testing_data_|. https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:135: storage_list.begin(); On 2013/06/27 12:35:26, Hongbo Min wrote: > nit: Please adjust your statement to keep a clean indent, reference line 92 Done. https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:179: void StorageInfoProvider::CheckWatchedStorages() { On 2013/06/27 17:12:22, Greg Billock wrote: > can this be const? I suppose not. Since watching_timer_ will invoke CheckWatchedStorages repeatly(see storage_info_provider.cc on line 231). With the const keyword, it will break complie. https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:200: double available_bytes = GetStorageFreeSpaceFromId(id); On 2013/06/27 12:35:26, Hongbo Min wrote: > since GetStorageFreeSpaceFromId will return -1 in case of |id| is not found in > |storage_id_to_size_map_|, it should be safe to remove line 196~198, right? I suppose not since GetStorageFreeSpaceFromId will invoke GetAllStorage function to get all the storage devices and filter with |id| field which is no related to |storage_id_to_size_map_|. https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:212: if (available_bytes < 0) On 2013/06/27 12:35:26, Hongbo Min wrote: > You can avoid this if-clause to make the code compact: > > if (available_bytes < 0) { > storage_id_to_size_map_.erase(id); > if (storage_id_to_size_map_.size() == 0) { > ... > } > return; > } Done. https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:212: if (available_bytes < 0) On 2013/06/27 17:12:22, Greg Billock wrote: > On 2013/06/27 12:35:26, Hongbo Min wrote: > > You can avoid this if-clause to make the code compact: > > > > if (available_bytes < 0) { > > storage_id_to_size_map_.erase(id); > > if (storage_id_to_size_map_.size() == 0) { > > ... > > } > > return; > > } > > I think an early return is better. In fact, I'd suggest replacing the stanza > below with an early return as well: > > if (old_available_bytes == storage_id_to_size_map_[id]) > return; > > .... <--- the rest can be unindented. Done. https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.h (right): https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:56: // Get the amount of storage free space from |id|, On 2013/06/27 12:35:26, Hongbo Min wrote: > nits: no need 2 lines. append line 57 to line 56. Done. https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.cc (right): https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.cc:33: UTF8ToWide(unit.name), On 2013/06/27 17:12:22, Greg Billock wrote: > UTF8ToUTF16 Done. https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.h (right): https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.h:22: // Increasing |available_capacity| with a fixed change_step. On 2013/06/27 12:35:26, Hongbo Min wrote: > nit: Each querying operation will increase the |available_capacity| with this > value. Done. https://codereview.chromium.org/16707002/diff/68001/chrome/common/extensions/... File chrome/common/extensions/api/experimental_system_info_storage.idl (right): https://codereview.chromium.org/16707002/diff/68001/chrome/common/extensions/... chrome/common/extensions/api/experimental_system_info_storage.idl:17: // The unique storage id. It is persistent between storage attachments. On 2013/06/27 17:12:22, Greg Billock wrote: > (!) This shouldn't be documented as persistent -- it'll use the transient ID. Done.
https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc (right): https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:74: for (StorageInfo::iterator it = info_.begin(); it != info_.end(); ++it) { Ah, ok. Let's note that in the header (that this variable is accessed on multiple threads, but that the whole class is being guarded by SystemInfoProvider). On 2013/06/28 02:29:54, Hongbo Min wrote: > On 2013/06/27 17:12:22, Greg Billock wrote: > > (!) I don't think it is safe to use info_ across threads like this. Instead > pass > > curried parameters to the other threads, and then swap them in on the UI > thread > > only. That'll make sure that multiple calls don't end up colliding on the > > un-mutexed variable. > > I ever considered the thread-safe issue for |info_| access across threads. It is > actually safe to access |info_| cross threads in such a usage scenario. The code > might be a little obscure but it does. > > You can reference its base class SystemInfoProvider which maintains a variable > |is_waiting_for_completion_| on UI thread. The |is_waiting_for_completion_| is > used to ensure that only one thread is accessing the |info_| member at the same > time. In case of |is_waiting_for_completion_| is true, the callback will be > queued and so no entry will be access |info_| until > SystemInfoProvider::OnQueryCompleted is called on UI thread. > > And StorageInfoProvider is also running in such an assumption, only > OnQueryCompleted gets called on UI thread which resets > |is_waiting_for_completion_| to false, means that the |info_| could be accessed > on UI thread again.
https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc (right): https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:74: for (StorageInfo::iterator it = info_.begin(); it != info_.end(); ++it) { On 2013/06/28 14:51:34, Greg Billock wrote: > Ah, ok. Let's note that in the header (that this variable is accessed on > multiple threads, > but that the whole class is being guarded by SystemInfoProvider). > > On 2013/06/28 02:29:54, Hongbo Min wrote: > > On 2013/06/27 17:12:22, Greg Billock wrote: > > > (!) I don't think it is safe to use info_ across threads like this. Instead > > pass > > > curried parameters to the other threads, and then swap them in on the UI > > thread > > > only. That'll make sure that multiple calls don't end up colliding on the > > > un-mutexed variable. > > > > I ever considered the thread-safe issue for |info_| access across threads. It > is > > actually safe to access |info_| cross threads in such a usage scenario. The > code > > might be a little obscure but it does. > > > > You can reference its base class SystemInfoProvider which maintains a variable > > |is_waiting_for_completion_| on UI thread. The |is_waiting_for_completion_| is > > used to ensure that only one thread is accessing the |info_| member at the > same > > time. In case of |is_waiting_for_completion_| is true, the callback will be > > queued and so no entry will be access |info_| until > > SystemInfoProvider::OnQueryCompleted is called on UI thread. > > > > And StorageInfoProvider is also running in such an assumption, only > > OnQueryCompleted gets called on UI thread which resets > > |is_waiting_for_completion_| to false, means that the |info_| could be > accessed > > on UI thread again. > Done.
Now I reuse the OnCheckWatchedStoragesFinishedForTesting function as a new way to avoid wait a half second for StorageInfoProvider 'WatchingNoChangedStorage' unit test to pass. hongbo@, please review this new way. Thanks. https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc (right): https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:156: MessageLoop::current()->PostDelayedTask(FROM_HERE, On 2013/06/28 14:46:32, Greg Billock wrote: > Right. Testing time windows are bad. We have several thousand tests -- if we get > in the habit of waiting half a second for them to pass, it makes the unit tests > run unacceptably slow. Let's find a better way to signal this affirmatively. > > On 2013/06/28 02:29:54, Hongbo Min wrote: > > On 2013/06/27 17:12:22, Greg Billock wrote: > > > (!) I think we discussed this before, but I don't see the reason for this. > If > > > this is an expected thing -- no more changes -- then we need to figure that > > out > > > without waiting 500ms. If its an unexpected thing, where is the check for > > that? > > > I don't think we need to wait 500ms here. > > > > You might miss the explanation. The expectation is, if OnFreeSpaceChanged is > not > > called during 500ms interval, it passed. Otherwise, the EXPECT_CALL could > catch > > the failure since its times is set to 0. > > > > Of course, 100ms or less is also fine. It is just a testing time window. > > > Done.
On 2013/06/29 03:15:14, Haojian Wu wrote: > Now I reuse the OnCheckWatchedStoragesFinishedForTesting function as a new way > to avoid wait a half second for StorageInfoProvider 'WatchingNoChangedStorage' > unit test to pass. > > hongbo@, please review this new way. Thanks. > LGTM
Very nearly there! Just a couple changes. https://codereview.chromium.org/16707002/diff/101001/chrome/browser/extension... File chrome/browser/extensions/api/system_info/system_info_api.cc (right): https://codereview.chromium.org/16707002/diff/101001/chrome/browser/extension... chrome/browser/extensions/api/system_info/system_info_api.cc:212: const std::string& transisent_id, double new_value, double old_value) { transient https://codereview.chromium.org/16707002/diff/101001/chrome/browser/extension... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc (right): https://codereview.chromium.org/16707002/diff/101001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:227: storage_transient_id_to_size_map_[transient_id] = new_available_bytes; (!) I think this needs to go outside the if statement. https://codereview.chromium.org/16707002/diff/101001/chrome/browser/extension... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc (right): https://codereview.chromium.org/16707002/diff/101001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:58: const int kMaxCheckWatchStroageTimes = 10; Storage https://codereview.chromium.org/16707002/diff/101001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:114: int check_watch_stroage_times_; storage
https://codereview.chromium.org/16707002/diff/101001/chrome/browser/extension... File chrome/browser/extensions/api/system_info/system_info_api.cc (right): https://codereview.chromium.org/16707002/diff/101001/chrome/browser/extension... chrome/browser/extensions/api/system_info/system_info_api.cc:212: const std::string& transisent_id, double new_value, double old_value) { On 2013/07/01 23:26:13, Greg Billock wrote: > transient Done. https://codereview.chromium.org/16707002/diff/101001/chrome/browser/extension... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc (right): https://codereview.chromium.org/16707002/diff/101001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:227: storage_transient_id_to_size_map_[transient_id] = new_available_bytes; On 2013/07/01 23:26:13, Greg Billock wrote: > (!) I think this needs to go outside the if statement. Done. https://codereview.chromium.org/16707002/diff/101001/chrome/browser/extension... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc (right): https://codereview.chromium.org/16707002/diff/101001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:58: const int kMaxCheckWatchStroageTimes = 10; On 2013/07/01 23:26:13, Greg Billock wrote: > Storage Done. https://codereview.chromium.org/16707002/diff/101001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:114: int check_watch_stroage_times_; On 2013/07/01 23:26:13, Greg Billock wrote: > storage Done.
lgtm thanks!
Looks like you need an extensions owner. Adding jyasskin.
https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... File chrome/browser/extensions/api/system_info/system_info_api.cc (right): https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info/system_info_api.cc:125: if (chrome::StorageMonitor::GetInstance()) You can avoid calling GetInstance twice by writing: if (chrome::StorageMonitor* storage_monitor = chrome::StorageMonitor::GetInstance()) storage_monitor->RemoveObserver(this); https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info/system_info_api.cc:234: base::Unretained(this), info)); What makes sure that *this is still alive when the reply comes back? Comment that. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... File chrome/browser/extensions/api/system_info/system_info_provider.h (right): https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info/system_info_provider.h:34: // class is being guarded by SystemInfoProvider. What does this mean? Normally I'd expect a mutex to guard an object accessed from multiple threads, but SystemInfoProvider isn't a mutex. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info/system_info_provider.h:78: // Default implementation of querying system information. Virtual functions need a comment saying how to override them, not just what the base class implementation does. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info/system_info_provider.h:85: // Post a task to blocking pool for information querying. "for information querying" is too vague. What does the query_callback need to do? https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info/system_info_provider.h:102: // TODO(Haojian): Remove the parameter T-typed pointer. And replace it with what? https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info/system_info_provider.h:105: // TODO(Haojian): Use PostBlockingPoolTaskAndReply to avoid unnecessary Why can't you do this now? https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info/system_info_provider.h:107: virtual void QueryOnWorkerPool() { This virtual function also needs a description of how to override it. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... File chrome/browser/extensions/api/system_info_storage/storage_free_space_observer.h (right): https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_free_space_observer.h:11: // Observes the storage free space changes. Comment what class this is used with. i.e. which class takes a StorageFreeSpaceObserver* and calls its methods? https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_free_space_observer.h:15: virtual void OnFreeSpaceChanged(const std::string& device_id, Comment where the device_id is defined. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_free_space_observer.h:16: double old_value, Comment what units this double has. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc (right): https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:58: StorageMonitor::GetInstance()->EnsureInitialized( This doesn't look like a good reason to add this customization point to SystemInfoProvider. Instead I'd suggest a method that's not directly in the query path that describes how to initialize the subclass. Give the base class states like "initializing" and "initialized" in addition to "is_waiting_for_completion_" (probably represented as a single enum) so that you can call the subclass's initialization method only once and have its completion advance the state. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:81: // Notify UI thread that the querying operation is already completed. Engligh nit: "already" doesn't have the right connotations here. It would mean that the operations was complete even before QueryAvailableCapacityOnBlockingPool was called. You'd get the right connotation from "... the querying operation has completed." or "... is complete." https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:212: storage_transient_id_to_size_map_.erase(transient_id); Comment why. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:221: // Ignore free space change event if the old available capacity is 0. Why? That seems like one of the particularly useful times to notice a change. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.h (right): https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:54: virtual std::vector<chrome::StorageInfo> GetAllStorages() const; Why so many virtual methods? This doesn't look like a class that's designed for subclassing. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:60: virtual std::string GetTransientIdForDeviceId( Find somewhere to comment why transient IDs are different from device IDs, or link to such an explanation. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc (right): https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:32: 1000, 10, 0}, You have special cases about 0 and negative sizes. Test those. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:103: virtual void OnCheckWatchedStorageOnBlockingPoolForTesting() OVERRIDE { You already have to override GetStorageFreeSpaceFromTransientId() in order to avoid calling out to the actual system. I'd just use that to determine that it's getting called. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:106: // Post a quit task to UI thread for test result verfication. sp: verfication https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:110: check_watch_stroage_times_ = 0; sp: stroage https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... File chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.cc (right): https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.cc:43: extensions::StorageInfo results; Unless there's a reason I'm missing, call info->clear(), and then push directly into *info. (which you could rename to results) https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.cc:72: // We simulate free space change by increasing the |available_capacity| But free space can never shrink? That seems like an odd test, especially with your logic about the old size being 0. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... File chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.h (right): https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.h:13: struct TestUnitInfo { You can't put a name this general into the global namespace. Put all your new names into the extensions namespace, and then make this one have something to do with storage. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.h:30: TestStorageInfoProvider(const struct TestUnitInfo* testing_data, size_t n); Document what this class does. https://codereview.chromium.org/16707002/diff/123001/chrome/common/extensions... File chrome/common/extensions/api/experimental_system_info_storage.idl (right): https://codereview.chromium.org/16707002/diff/123001/chrome/common/extensions... chrome/common/extensions/api/experimental_system_info_storage.idl:25: // The total amount of the storage space, in bytes, default value is 0. What does "default" mean here? Storage units don't have default sizes, they just have sizes. https://codereview.chromium.org/16707002/diff/123001/chrome/common/extensions... chrome/common/extensions/api/experimental_system_info_storage.idl:31: dictionary StorageCapacityChangeInfo { It's not the capacity that's changing with this event, it's the available space. So "StorageAvailabilityChangeInfo" or "StorageFreeSpaceChangeInfo"? https://codereview.chromium.org/16707002/diff/123001/chrome/common/extensions... chrome/common/extensions/api/experimental_system_info_storage.idl:32: // The unique id of the storage unit already changed. s/already/that/ I think.
https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc (right): https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:58: StorageMonitor::GetInstance()->EnsureInitialized( On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > This doesn't look like a good reason to add this customization point to > SystemInfoProvider. Instead I'd suggest a method that's not directly in the > query path that describes how to initialize the subclass. Give the base class > states like "initializing" and "initialized" in addition to > "is_waiting_for_completion_" (probably represented as a single enum) so that you > can call the subclass's initialization method only once and have its completion > advance the state. Jeffrey@, here EnsureInitialized is called on StorageMonitor instance, not for StorageInfoProvider. See StorageMonitor::EnsureInitialized comments.
https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc (right): https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:58: StorageMonitor::GetInstance()->EnsureInitialized( On 2013/07/03 01:27:28, Hongbo Min wrote: > On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > > This doesn't look like a good reason to add this customization point to > > SystemInfoProvider. Instead I'd suggest a method that's not directly in the > > query path that describes how to initialize the subclass. Give the base class > > states like "initializing" and "initialized" in addition to > > "is_waiting_for_completion_" (probably represented as a single enum) so that > you > > can call the subclass's initialization method only once and have its > completion > > advance the state. > > Jeffrey@, here EnsureInitialized is called on StorageMonitor instance, not for > StorageInfoProvider. See StorageMonitor::EnsureInitialized comments. Yeah, I saw that: https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/st.... The need to call EnsureInitialized is driving complexity in SystemInfoProvider<> and forcing you to make extra methods protected, since you have to allow StorageInfoProvider to inject in the middle of a callback chain. If possible, I'd rather have SystemInfoProvider call a dedicated virtual function InitializeProvider(callback) that initializes the subsystem and then calls |callback|. So for StorageInfoProvider, the implementation would be virtual void InitializeProvider(const Closure& done) { StorageMonitor::GetInstance()->EnsureInitialized(done); } It would be possible for the SystemInfoProvider to call that on every query, but it'd probably be more efficient to cache the fact that initialization is done and skip the calls on subsequent queries.
https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... File chrome/browser/extensions/api/system_info/system_info_api.cc (right): https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info/system_info_api.cc:234: base::Unretained(this), info)); On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > What makes sure that *this is still alive when the reply comes back? Comment > that. OK. It is because SystemInfoProvider instance is a global lazy instance. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... File chrome/browser/extensions/api/system_info/system_info_provider.h (right): https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info/system_info_provider.h:34: // class is being guarded by SystemInfoProvider. On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > What does this mean? Normally I'd expect a mutex to guard an object accessed > from multiple threads, but SystemInfoProvider isn't a mutex. You might miss some background of this class implementation. We try to avoid using mutex to keep thread-safe to access |info_|. You might notice that the StartQueryInfo will put a new callback into a queue, once the information is queried and put into |info_|, then the callbacks in queue will be called one by one in UI thread. The bool |is_waiting_for_completion_| serves as such a purpose. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info/system_info_provider.h:105: // TODO(Haojian): Use PostBlockingPoolTaskAndReply to avoid unnecessary On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > Why can't you do this now? After discussion with Billock@ and Lei@, we agree to do that in a separate CL. This CL we only focus on StorageMonitor reusing work. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc (right): https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:58: StorageMonitor::GetInstance()->EnsureInitialized( On 2013/07/03 02:26:48, Jeffrey Yasskin wrote: > On 2013/07/03 01:27:28, Hongbo Min wrote: > > On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > > > This doesn't look like a good reason to add this customization point to > > > SystemInfoProvider. Instead I'd suggest a method that's not directly in the > > > query path that describes how to initialize the subclass. Give the base > class > > > states like "initializing" and "initialized" in addition to > > > "is_waiting_for_completion_" (probably represented as a single enum) so that > > you > > > can call the subclass's initialization method only once and have its > > completion > > > advance the state. > > > > Jeffrey@, here EnsureInitialized is called on StorageMonitor instance, not for > > StorageInfoProvider. See StorageMonitor::EnsureInitialized comments. > > Yeah, I saw that: > https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/st.... > The need to call EnsureInitialized is driving complexity in SystemInfoProvider<> > and forcing you to make extra methods protected, since you have to allow > StorageInfoProvider to inject in the middle of a callback chain. If possible, > I'd rather have SystemInfoProvider call a dedicated virtual function > InitializeProvider(callback) that initializes the subsystem and then calls > |callback|. So for StorageInfoProvider, the implementation would be > > virtual void InitializeProvider(const Closure& done) { > StorageMonitor::GetInstance()->EnsureInitialized(done); > } > > It would be possible for the SystemInfoProvider to call that on every query, but > it'd probably be more efficient to cache the fact that initialization is done > and skip the calls on subsequent queries. Using cache might be problematic since StorageMonitor will also update its data if a storage is detached/attached. In that sense, we have to ensure the data cached is synced to the StorageMonitor. It will also increase the complexity without significant improvement. Also, the call of EnsureInitialized on every query is still light-weight since in EnsureInitialized impl, the callback will get called immediately if StorageMonitor is already initialized. Did I catch up your points correctly?
I will add more test cases(special cases about 0 and negative sizes) in unit_tests and browser_tests of StorageInfoProvider later. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... File chrome/browser/extensions/api/system_info/system_info_api.cc (right): https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info/system_info_api.cc:125: if (chrome::StorageMonitor::GetInstance()) On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > You can avoid calling GetInstance twice by writing: > > if (chrome::StorageMonitor* storage_monitor = > chrome::StorageMonitor::GetInstance()) > storage_monitor->RemoveObserver(this); Done. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... File chrome/browser/extensions/api/system_info/system_info_provider.h (right): https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info/system_info_provider.h:34: // class is being guarded by SystemInfoProvider. On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > What does this mean? Normally I'd expect a mutex to guard an object accessed > from multiple threads, but SystemInfoProvider isn't a mutex. Please see hongbo's explanation at here:https://codereview.chromium.org/16707002/diff/68001/chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc#newcode74 I have updated the comment for more details. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info/system_info_provider.h:78: // Default implementation of querying system information. On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > Virtual functions need a comment saying how to override them, not just what the > base class implementation does. Done. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info/system_info_provider.h:85: // Post a task to blocking pool for information querying. On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > "for information querying" is too vague. What does the query_callback need to > do? Done. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info/system_info_provider.h:102: // TODO(Haojian): Remove the parameter T-typed pointer. On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > And replace it with what? Replace with bool QueryInfo() instead. Update the comment. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info/system_info_provider.h:105: // TODO(Haojian): Use PostBlockingPoolTaskAndReply to avoid unnecessary On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > Why can't you do this now? Since this work will make a big change of SystemInfo API code. It will be better to separate a new CL to do this. Please see this CL: https://codereview.chromium.org/18290002/ https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info/system_info_provider.h:107: virtual void QueryOnWorkerPool() { On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > This virtual function also needs a description of how to override it. This function can be non-virtual and replace it with non-virtual. Also update the comment. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... File chrome/browser/extensions/api/system_info_storage/storage_free_space_observer.h (right): https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_free_space_observer.h:11: // Observes the storage free space changes. On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > Comment what class this is used with. i.e. which class takes a > StorageFreeSpaceObserver* and calls its methods? Done. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_free_space_observer.h:15: virtual void OnFreeSpaceChanged(const std::string& device_id, On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > Comment where the device_id is defined. Done. Indeed, the device_id here is transient_id. I have renamed it. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_free_space_observer.h:16: double old_value, On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > Comment what units this double has. Done. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc (right): https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:81: // Notify UI thread that the querying operation is already completed. On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > Engligh nit: "already" doesn't have the right connotations here. It would mean > that the operations was complete even before > QueryAvailableCapacityOnBlockingPool was called. You'd get the right connotation > from "... the querying operation has completed." or "... is complete." Done. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:212: storage_transient_id_to_size_map_.erase(transient_id); On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > Comment why. Done. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:221: // Ignore free space change event if the old available capacity is 0. On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > Why? That seems like one of the particularly useful times to notice a change. hongbo@, you may have more thoughts here. I just follow your initialized patch. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.h (right): https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:54: virtual std::vector<chrome::StorageInfo> GetAllStorages() const; On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > Why so many virtual methods? This doesn't look like a class that's designed for > subclassing. Design for test. please refer to TestStorageInfoProvider class. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc (right): https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:103: virtual void OnCheckWatchedStorageOnBlockingPoolForTesting() OVERRIDE { On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > You already have to override GetStorageFreeSpaceFromTransientId() in order to > avoid calling out to the actual system. I'd just use that to determine that it's > getting called. Done. And remove OnCheckWatchedStorageOnBlockingPoolForTesting function. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:106: // Post a quit task to UI thread for test result verfication. On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > sp: verfication Done. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:110: check_watch_stroage_times_ = 0; On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > sp: stroage Done. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... File chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.cc (right): https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.cc:43: extensions::StorageInfo results; On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > Unless there's a reason I'm missing, call info->clear(), and then push directly > into *info. (which you could rename to results) Done. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.cc:72: // We simulate free space change by increasing the |available_capacity| On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > But free space can never shrink? That seems like an odd test, especially with > your logic about the old size being 0. There is one test data which the change_step is 0. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... File chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.h (right): https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.h:13: struct TestUnitInfo { On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > You can't put a name this general into the global namespace. Put all your new > names into the extensions namespace, and then make this one have something to do > with storage. Done. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.h:30: TestStorageInfoProvider(const struct TestUnitInfo* testing_data, size_t n); On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > Document what this class does. Done. https://codereview.chromium.org/16707002/diff/123001/chrome/common/extensions... File chrome/common/extensions/api/experimental_system_info_storage.idl (right): https://codereview.chromium.org/16707002/diff/123001/chrome/common/extensions... chrome/common/extensions/api/experimental_system_info_storage.idl:25: // The total amount of the storage space, in bytes, default value is 0. On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > What does "default" mean here? Storage units don't have default sizes, they just > have sizes. If we can't establish the capacity or available capacity, these variables will be set to the default value. https://codereview.chromium.org/16707002/diff/123001/chrome/common/extensions... chrome/common/extensions/api/experimental_system_info_storage.idl:31: dictionary StorageCapacityChangeInfo { On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > It's not the capacity that's changing with this event, it's the available space. > So "StorageAvailabilityChangeInfo" or "StorageFreeSpaceChangeInfo"? Done. But the current name "StorageCapacityChangeInfo" is based on Billock's suggestions. https://codereview.chromium.org/16707002/diff/123001/chrome/common/extensions... chrome/common/extensions/api/experimental_system_info_storage.idl:32: // The unique id of the storage unit already changed. On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > s/already/that/ I think. Done.
https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc (right): https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:221: // Ignore free space change event if the old available capacity is 0. On 2013/07/03 16:23:49, Haojian Wu wrote: > On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > > Why? That seems like one of the particularly useful times to notice a change. > > hongbo@, you may have more thoughts here. I just follow your initialized patch. I'm guessing the reason is that if we don't know the capacity at some point, if we start knowing it we ought to wait until things change to notify? Agreed this seems like an interesting notification. Shall we just pass it through? Perhaps add a comment on the notification that this is the semantics. https://codereview.chromium.org/16707002/diff/123001/chrome/common/extensions... File chrome/common/extensions/api/experimental_system_info_storage.idl (right): https://codereview.chromium.org/16707002/diff/123001/chrome/common/extensions... chrome/common/extensions/api/experimental_system_info_storage.idl:25: // The total amount of the storage space, in bytes, default value is 0. On 2013/07/03 16:23:49, Haojian Wu wrote: > On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > > What does "default" mean here? Storage units don't have default sizes, they > just > > have sizes. > > If we can't establish the capacity or available capacity, these variables will > be set to the default value. Maybe the comment should be "If the storage size cannot be determined, it will have a 0 value." That's ambiguous for availableCapacity though. :-( https://codereview.chromium.org/16707002/diff/123001/chrome/common/extensions... chrome/common/extensions/api/experimental_system_info_storage.idl:31: dictionary StorageCapacityChangeInfo { On 2013/07/03 16:23:49, Haojian Wu wrote: > On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > > It's not the capacity that's changing with this event, it's the available > space. > > So "StorageAvailabilityChangeInfo" or "StorageFreeSpaceChangeInfo"? > > Done. But the current name "StorageCapacityChangeInfo" is based on Billock's > suggestions. Either way is fine with me. I think "StorageAvailabilityChangeInfo" is misleading, though -- sounds like whether the storage is avilable or not, not about available capacity.
Greg says this is just the first step of a long sequence of refactorings, so I'll lgtm, but please address these comments in future changes. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... File chrome/browser/extensions/api/system_info/system_info_api.cc (right): https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info/system_info_api.cc:234: base::Unretained(this), info)); On 2013/07/03 04:59:40, Hongbo Min wrote: > On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > > What makes sure that *this is still alive when the reply comes back? Comment > > that. > > OK. It is because SystemInfoProvider instance is a global lazy instance. > Please comment that. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... File chrome/browser/extensions/api/system_info/system_info_provider.h (right): https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info/system_info_provider.h:102: // TODO(Haojian): Remove the parameter T-typed pointer. On 2013/07/03 16:23:49, Haojian Wu wrote: > On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > > And replace it with what? > > Replace with bool QueryInfo() instead. Update the comment. No, this signature gives the subclass access to a T*. Where will the subclass get that T* once you've removed the parameter? https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info/system_info_provider.h:105: // TODO(Haojian): Use PostBlockingPoolTaskAndReply to avoid unnecessary On 2013/07/03 16:23:49, Haojian Wu wrote: > On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > > Why can't you do this now? > > Since this work will make a big change of SystemInfo API code. It will be better > to separate a new CL to do this. Please see this CL: > https://codereview.chromium.org/18290002/ In https://codereview.chromium.org/18290002/ I see only 2 uses of PostBlockingPoolTaskAndReply, both of which replace code you're adding in this CL. I see lots of *other* changes, which make sense to split out, but the PostBlockingPoolTaskAndReply change doesn't look like it implies changes anywhere else. Actually, you should use content::BrowserThread::GetBlockingPool()->GetSequencedTaskRunnerWithShutdownBehavior( content::BrowserThread::GetBlockingPool()->GetSequenceToken(), CONTINUE_ON_SHUTDOWN) to initialize a scoped_refptr<SequencedTaskRunner> (replacing |worker_pool_token_|) and then use base::PostTaskAndReplyWithResult() to get |success| passed from the worker thread back to the UI thread. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc (right): https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:58: StorageMonitor::GetInstance()->EnsureInitialized( On 2013/07/03 04:59:40, Hongbo Min wrote: > On 2013/07/03 02:26:48, Jeffrey Yasskin wrote: > > On 2013/07/03 01:27:28, Hongbo Min wrote: > > > On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > > > > This doesn't look like a good reason to add this customization point to > > > > SystemInfoProvider. Instead I'd suggest a method that's not directly in > the > > > > query path that describes how to initialize the subclass. Give the base > > class > > > > states like "initializing" and "initialized" in addition to > > > > "is_waiting_for_completion_" (probably represented as a single enum) so > that > > > you > > > > can call the subclass's initialization method only once and have its > > > completion > > > > advance the state. > > > > > > Jeffrey@, here EnsureInitialized is called on StorageMonitor instance, not > for > > > StorageInfoProvider. See StorageMonitor::EnsureInitialized comments. > > > > Yeah, I saw that: > > > https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/st.... > > The need to call EnsureInitialized is driving complexity in > SystemInfoProvider<> > > and forcing you to make extra methods protected, since you have to allow > > StorageInfoProvider to inject in the middle of a callback chain. If possible, > > I'd rather have SystemInfoProvider call a dedicated virtual function > > InitializeProvider(callback) that initializes the subsystem and then calls > > |callback|. So for StorageInfoProvider, the implementation would be > > > > virtual void InitializeProvider(const Closure& done) { > > StorageMonitor::GetInstance()->EnsureInitialized(done); > > } > > > > It would be possible for the SystemInfoProvider to call that on every query, > but > > it'd probably be more efficient to cache the fact that initialization is done > > and skip the calls on subsequent queries. > > Using cache might be problematic since StorageMonitor will also update its data > if a storage is detached/attached. In that sense, we have to ensure the data > cached is synced to the StorageMonitor. It will also increase the complexity > without significant improvement. I said you could cache the fact that initialization is finished, not to cache any data. > Also, the call of EnsureInitialized on every query is still light-weight since > in EnsureInitialized impl, the callback will get called immediately if > StorageMonitor is already initialized. Ok, if you want to call EnsureInitialized on every query, you can still make StartQueryInfoImpl() private and non-virtual: StartQueryInfo(const QueryInfoCompletionCallback& callback) { ... EnsureInitialized(base::Bind(&SystemInfoProvider<T>::StartQueryInfoPostInitialization, this)) } https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... File chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.cc (right): https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.cc:72: // We simulate free space change by increasing the |available_capacity| On 2013/07/03 16:23:49, Haojian Wu wrote: > On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > > But free space can never shrink? That seems like an odd test, especially with > > your logic about the old size being 0. > > There is one test data which the change_step is 0. change_step==0 just means the size doesn't change, not that the old size is 0. I think your whole testing strategy is probably going to need to change: I'd expect an interface that represents the system storage functions, with a googlemock implementation that lets you specify the exact order of results. That'll let you remove your little programming language in TestUnitInfo and nearly all of the virtual functions in StorageInfoProvider. However, that's a bigger change than you need to do in this CL. https://codereview.chromium.org/16707002/diff/140001/chrome/browser/extension... File chrome/browser/extensions/api/system_info/system_info_provider.h (right): https://codereview.chromium.org/16707002/diff/140001/chrome/browser/extension... chrome/browser/extensions/api/system_info/system_info_provider.h:36: // SystemInfoProvider maintains a variable |is_waiting_for_completion_| on UI To summarize this long paragraph, would it be correct to just say "info_ is accessed on the UI thread while |is_waiting_for_completion_| is false and on the BlockingPool under worker_pool_token_ while |is_waiting_for_completion_| is true."? If that's correct, just replace the previous paragraph with it and remove this one. https://codereview.chromium.org/16707002/diff/140001/chrome/browser/extension... chrome/browser/extensions/api/system_info/system_info_provider.h:87: // Before overriding this function, you should know what you intend to do This sentence doesn't convey any information, so please remove it. https://codereview.chromium.org/16707002/diff/140001/chrome/browser/extension... File chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.h (right): https://codereview.chromium.org/16707002/diff/140001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.h:15: struct TestUnitInfo { extensions::TestUnitInfo is still too general. Make the name have something to do with "storage". https://codereview.chromium.org/16707002/diff/140001/chrome/common/extensions... File chrome/common/extensions/api/experimental_system_info_storage.idl (right): https://codereview.chromium.org/16707002/diff/140001/chrome/common/extensions... chrome/common/extensions/api/experimental_system_info_storage.idl:26: // Default value is 0 if query operation failes. sp: failes->fails https://codereview.chromium.org/16707002/diff/140001/chrome/common/extensions... chrome/common/extensions/api/experimental_system_info_storage.idl:34: // The transient id of the storage unit already changed. Comments here end up in user-visible documentation (http://developer.chrome.com/dev/extensions/experimental.systemInfo.storage.ht...), so "transient" isn't quite right. I'd leave this comment alone until someone has time to actually rewrite it.
https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... File chrome/browser/extensions/api/system_info/system_info_api.cc (right): https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info/system_info_api.cc:234: base::Unretained(this), info)); On 2013/07/03 21:44:48, Jeffrey Yasskin wrote: > On 2013/07/03 04:59:40, Hongbo Min wrote: > > On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > > > What makes sure that *this is still alive when the reply comes back? Comment > > > that. > > > > OK. It is because SystemInfoProvider instance is a global lazy instance. > > > > Please comment that. Done. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... File chrome/browser/extensions/api/system_info/system_info_provider.h (right): https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info/system_info_provider.h:102: // TODO(Haojian): Remove the parameter T-typed pointer. On 2013/07/03 21:44:48, Jeffrey Yasskin wrote: > On 2013/07/03 16:23:49, Haojian Wu wrote: > > On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > > > And replace it with what? > > > > Replace with bool QueryInfo() instead. Update the comment. > > No, this signature gives the subclass access to a T*. Where will the subclass > get that T* once you've removed the parameter? Actually the subclass can access T-typed info_ directly since info_ is a protected member in SystemInfoProvider(see system_info_provider.h line 139). https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info/system_info_provider.h:105: // TODO(Haojian): Use PostBlockingPoolTaskAndReply to avoid unnecessary On 2013/07/03 21:44:48, Jeffrey Yasskin wrote: > On 2013/07/03 16:23:49, Haojian Wu wrote: > > On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > > > Why can't you do this now? > > > > Since this work will make a big change of SystemInfo API code. It will be > better > > to separate a new CL to do this. Please see this CL: > > https://codereview.chromium.org/18290002/ > > In https://codereview.chromium.org/18290002/ I see only 2 uses of > PostBlockingPoolTaskAndReply, both of which replace code you're adding in this > CL. I see lots of *other* changes, which make sense to split out, but the > PostBlockingPoolTaskAndReply change doesn't look like it implies changes > anywhere else. > > Actually, you should use > content::BrowserThread::GetBlockingPool()->GetSequencedTaskRunnerWithShutdownBehavior( > content::BrowserThread::GetBlockingPool()->GetSequenceToken(), > CONTINUE_ON_SHUTDOWN) > to initialize a scoped_refptr<SequencedTaskRunner> (replacing > |worker_pool_token_|) and then use base::PostTaskAndReplyWithResult() to get > |success| passed from the worker thread back to the UI thread. Thanks for the useful suggestions. I will update that CL soon. https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... File chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.cc (right): https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.cc:72: // We simulate free space change by increasing the |available_capacity| On 2013/07/03 21:44:48, Jeffrey Yasskin wrote: > On 2013/07/03 16:23:49, Haojian Wu wrote: > > On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > > > But free space can never shrink? That seems like an odd test, especially > with > > > your logic about the old size being 0. > > > > There is one test data which the change_step is 0. > > change_step==0 just means the size doesn't change, not that the old size is 0. > > I think your whole testing strategy is probably going to need to change: I'd > expect an interface that represents the system storage functions, with a > googlemock implementation that lets you specify the exact order of results. > That'll let you remove your little programming language in TestUnitInfo and > nearly all of the virtual functions in StorageInfoProvider. However, that's a > bigger change than you need to do in this CL. Add TODO in TestStorageInfoProvider header. https://codereview.chromium.org/16707002/diff/123001/chrome/common/extensions... File chrome/common/extensions/api/experimental_system_info_storage.idl (right): https://codereview.chromium.org/16707002/diff/123001/chrome/common/extensions... chrome/common/extensions/api/experimental_system_info_storage.idl:31: dictionary StorageCapacityChangeInfo { On 2013/07/03 17:33:57, Greg Billock wrote: > On 2013/07/03 16:23:49, Haojian Wu wrote: > > On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > > > It's not the capacity that's changing with this event, it's the available > > space. > > > So "StorageAvailabilityChangeInfo" or "StorageFreeSpaceChangeInfo"? > > > > Done. But the current name "StorageCapacityChangeInfo" is based on Billock's > > suggestions. > > Either way is fine with me. I think "StorageAvailabilityChangeInfo" is > misleading, though -- sounds like whether the storage is avilable or not, not > about available capacity. Now rename to StorageFreeSpaceChangeInfo. https://codereview.chromium.org/16707002/diff/140001/chrome/browser/extension... File chrome/browser/extensions/api/system_info/system_info_provider.h (right): https://codereview.chromium.org/16707002/diff/140001/chrome/browser/extension... chrome/browser/extensions/api/system_info/system_info_provider.h:36: // SystemInfoProvider maintains a variable |is_waiting_for_completion_| on UI On 2013/07/03 21:44:49, Jeffrey Yasskin wrote: > To summarize this long paragraph, would it be correct to just say "info_ is > accessed on the UI thread while |is_waiting_for_completion_| is false and on the > BlockingPool under worker_pool_token_ while |is_waiting_for_completion_| is > true."? If that's correct, just replace the previous paragraph with it and > remove this one. I think so. Done. https://codereview.chromium.org/16707002/diff/140001/chrome/browser/extension... chrome/browser/extensions/api/system_info/system_info_provider.h:87: // Before overriding this function, you should know what you intend to do On 2013/07/03 21:44:49, Jeffrey Yasskin wrote: > This sentence doesn't convey any information, so please remove it. Done. https://codereview.chromium.org/16707002/diff/140001/chrome/browser/extension... File chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.h (right): https://codereview.chromium.org/16707002/diff/140001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/test_storage_info_provider.h:15: struct TestUnitInfo { On 2013/07/03 21:44:49, Jeffrey Yasskin wrote: > extensions::TestUnitInfo is still too general. Make the name have something to > do with "storage". Done. https://codereview.chromium.org/16707002/diff/140001/chrome/common/extensions... File chrome/common/extensions/api/experimental_system_info_storage.idl (right): https://codereview.chromium.org/16707002/diff/140001/chrome/common/extensions... chrome/common/extensions/api/experimental_system_info_storage.idl:26: // Default value is 0 if query operation failes. On 2013/07/03 21:44:49, Jeffrey Yasskin wrote: > sp: failes->fails Done.
https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc (right): https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:58: StorageMonitor::GetInstance()->EnsureInitialized( On 2013/07/03 21:44:48, Jeffrey Yasskin wrote: > On 2013/07/03 04:59:40, Hongbo Min wrote: > > On 2013/07/03 02:26:48, Jeffrey Yasskin wrote: > > > On 2013/07/03 01:27:28, Hongbo Min wrote: > > > > On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > > > > > This doesn't look like a good reason to add this customization point to > > > > > SystemInfoProvider. Instead I'd suggest a method that's not directly in > > the > > > > > query path that describes how to initialize the subclass. Give the base > > > class > > > > > states like "initializing" and "initialized" in addition to > > > > > "is_waiting_for_completion_" (probably represented as a single enum) so > > that > > > > you > > > > > can call the subclass's initialization method only once and have its > > > > completion > > > > > advance the state. > > > > > > > > Jeffrey@, here EnsureInitialized is called on StorageMonitor instance, not > > for > > > > StorageInfoProvider. See StorageMonitor::EnsureInitialized comments. > > > > > > Yeah, I saw that: > > > > > > https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/st.... > > > The need to call EnsureInitialized is driving complexity in > > SystemInfoProvider<> > > > and forcing you to make extra methods protected, since you have to allow > > > StorageInfoProvider to inject in the middle of a callback chain. If > possible, > > > I'd rather have SystemInfoProvider call a dedicated virtual function > > > InitializeProvider(callback) that initializes the subsystem and then calls > > > |callback|. So for StorageInfoProvider, the implementation would be > > > > > > virtual void InitializeProvider(const Closure& done) { > > > StorageMonitor::GetInstance()->EnsureInitialized(done); > > > } > > > > > > It would be possible for the SystemInfoProvider to call that on every query, > > but > > > it'd probably be more efficient to cache the fact that initialization is > done > > > and skip the calls on subsequent queries. > > > > Using cache might be problematic since StorageMonitor will also update its > data > > if a storage is detached/attached. In that sense, we have to ensure the data > > cached is synced to the StorageMonitor. It will also increase the complexity > > without significant improvement. > > I said you could cache the fact that initialization is finished, not to cache > any data. > > > Also, the call of EnsureInitialized on every query is still light-weight since > > in EnsureInitialized impl, the callback will get called immediately if > > StorageMonitor is already initialized. > > Ok, if you want to call EnsureInitialized on every query, you can still make > StartQueryInfoImpl() private and non-virtual: > > StartQueryInfo(const QueryInfoCompletionCallback& callback) { > ... > > EnsureInitialized(base::Bind(&SystemInfoProvider<T>::StartQueryInfoPostInitialization, > this)) > } I agree with hongbo@. Jeffrey@, you might be misleading by the name StorageMonitor::EnsureInitialized. StorageInfoProvider at here have already initialized and start to query the system information. The custom callback function binding here is the query task which should be processed after StorageMonitor has been initialized. StorageMonitor::EnsureInitialized will invoke the callback instantly if StorageMonitor is initialized.
On 2013/07/03 21:44:48, Jeffrey Yasskin wrote: > Greg says this is just the first step of a long sequence of refactorings, so > I'll lgtm, but please address these comments in future changes. > > https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension... > chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:58: > StorageMonitor::GetInstance()->EnsureInitialized( > On 2013/07/03 04:59:40, Hongbo Min wrote: > > On 2013/07/03 02:26:48, Jeffrey Yasskin wrote: > > > On 2013/07/03 01:27:28, Hongbo Min wrote: > > > > On 2013/07/02 23:00:50, Jeffrey Yasskin wrote: > > > > > This doesn't look like a good reason to add this customization point to > > > > > SystemInfoProvider. Instead I'd suggest a method that's not directly in > > the > > > > > query path that describes how to initialize the subclass. Give the base > > > class > > > > > states like "initializing" and "initialized" in addition to > > > > > "is_waiting_for_completion_" (probably represented as a single enum) so > > that > > > > you > > > > > can call the subclass's initialization method only once and have its > > > > completion > > > > > advance the state. > > > > > > > > Jeffrey@, here EnsureInitialized is called on StorageMonitor instance, not > > for > > > > StorageInfoProvider. See StorageMonitor::EnsureInitialized comments. > > > > > > Yeah, I saw that: > > > > > > https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/st.... > > > The need to call EnsureInitialized is driving complexity in > > SystemInfoProvider<> > > > and forcing you to make extra methods protected, since you have to allow > > > StorageInfoProvider to inject in the middle of a callback chain. If > possible, > > > I'd rather have SystemInfoProvider call a dedicated virtual function > > > InitializeProvider(callback) that initializes the subsystem and then calls > > > |callback|. So for StorageInfoProvider, the implementation would be > > > > > > virtual void InitializeProvider(const Closure& done) { > > > StorageMonitor::GetInstance()->EnsureInitialized(done); > > > } > > > > > > It would be possible for the SystemInfoProvider to call that on every query, > > but > > > it'd probably be more efficient to cache the fact that initialization is > done > > > and skip the calls on subsequent queries. > > > > Using cache might be problematic since StorageMonitor will also update its > data > > if a storage is detached/attached. In that sense, we have to ensure the data > > cached is synced to the StorageMonitor. It will also increase the complexity > > without significant improvement. > > I said you could cache the fact that initialization is finished, not to cache > any data. > > > Also, the call of EnsureInitialized on every query is still light-weight since > > in EnsureInitialized impl, the callback will get called immediately if > > StorageMonitor is already initialized. > > Ok, if you want to call EnsureInitialized on every query, you can still make > StartQueryInfoImpl() private and non-virtual: > > StartQueryInfo(const QueryInfoCompletionCallback& callback) { > ... > > EnsureInitialized(base::Bind(&SystemInfoProvider<T>::StartQueryInfoPostInitialization, > this)) > } > In this sense, what is the significant improvement by introducing another new virtual method StartQueryInfoPostInitialization? We still need to make SystemInfoProvider::StartQueryInfoPostInitialization as protected and virtual, right? Again, EnsureInitialized is specific to StorageInfoProvider, not all SystemInfoProvider subclasses.
Still lgtm so you can fix things in followup changes. On Wed, Jul 3, 2013 at 6:59 PM, <hongbo.min@intel.com> wrote: > On 2013/07/03 21:44:48, Jeffrey Yasskin wrote: >> Ok, if you want to call EnsureInitialized on every query, you can still >> make >> StartQueryInfoImpl() private and non-virtual: > >> StartQueryInfo(const QueryInfoCompletionCallback& callback) { >> ... >> EnsureInitialized(base::Bind(&SystemInfoProvider<T>::StartQueryInfoPostInitialization, >> this)) >> } > > > In this sense, what is the significant improvement by introducing another > new > virtual method StartQueryInfoPostInitialization? We still need to make > SystemInfoProvider::StartQueryInfoPostInitialization as protected and > virtual, > right? Again, EnsureInitialized is specific to StorageInfoProvider, not all > SystemInfoProvider subclasses. You would introduce another virtual method with a fairly simple contract, and that would allow you to make StartQueryInfoImpl(), PostQueryTaskToBlockingPool(), and OnQueryCompleted() non-virtual and private. (It looks like QueryOnWorkerPool() could already be private; please make it so.) > https://codereview.chromium.org/16707002/
On 2013/07/04 02:17:48, Jeffrey Yasskin wrote:
> Still lgtm so you can fix things in followup changes.
>
> On Wed, Jul 3, 2013 at 6:59 PM, <mailto:hongbo.min@intel.com> wrote:
> > On 2013/07/03 21:44:48, Jeffrey Yasskin wrote:
> >> Ok, if you want to call EnsureInitialized on every query, you can still
> >> make
> >> StartQueryInfoImpl() private and non-virtual:
> >
> >> StartQueryInfo(const QueryInfoCompletionCallback& callback) {
> >> ...
> >>
>
EnsureInitialized(base::Bind(&SystemInfoProvider<T>::StartQueryInfoPostInitialization,
> >> this))
> >> }
> >
> >
> > In this sense, what is the significant improvement by introducing another
> > new
> > virtual method StartQueryInfoPostInitialization? We still need to make
> > SystemInfoProvider::StartQueryInfoPostInitialization as protected and
> > virtual,
> > right? Again, EnsureInitialized is specific to StorageInfoProvider, not all
> > SystemInfoProvider subclasses.
>
> You would introduce another virtual method with a fairly simple
> contract, and that would allow you to make StartQueryInfoImpl(),
> PostQueryTaskToBlockingPool(), and OnQueryCompleted() non-virtual and
> private. (It looks like QueryOnWorkerPool() could already be private;
> please make it so.)
>
> > https://codereview.chromium.org/16707002/
Update the patch.
Now:
PostQueryTaskToBlockingPool, OnQueryCompleted are protected and non-virtual
QueryOnWorkerPool are private and non-virtual
I think it not easy to make PostQueryTaskToBlockingPooland OnQueryCompleted
private even through a new virtual method since the custom callback task in
StorageInfoProvider need to call these two functions explictly.
Also as hongbo@ said, EnsureInitialized is specific to StorageInfoProvider, not
all SystemInfoProvider subclasses.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/Hokein.Wu@gmail.com/16707002/169001
Message was sent while issue was closed.
Change committed as 210169 |
