Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11415177:
Implements the storage free space watching functionality into storage info provider (Closed)
|
Created:
8 years ago by Hongbo Min Modified:
8 years ago CC:
chromium-reviews, Aaron Boodman, chromium-apps-reviews_chromium.org, benwells, jhawkins Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionImplements the storage free space watching functionality into
storage info provider in a way of polling the free space
changes at a regular time interval.
BUG=141229
TEST=unit_test --gtest_filter=StorageInfoProviderTest.*
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170667
Patch Set 1 #Patch Set 2 : unit test enhancement #
Total comments: 18
Patch Set 3 : update patch and test case enhancement #
Total comments: 4
Patch Set 4 : update #Patch Set 5 : avoid naming conflict in browser_tests #
Total comments: 14
Messages
Total messages: 20 (0 generated)
benwells@, pls help review the patch. Nico@, need your LGTM for gyp change.
https://codereview.chromium.org/11415177/diff/8001/chrome/browser/extensions/... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc (right): https://codereview.chromium.org/11415177/diff/8001/chrome/browser/extensions/... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:122: if (watching_ids_map_.size() == 0) Please remove the early return. As having size == 0 is an error condition, and having this early exit is an optimization (i.e. the rest of the code will work otherwise). https://codereview.chromium.org/11415177/diff/8001/chrome/browser/extensions/... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:145: void StorageInfoProvider::TerminateWatch() { Oh, this just stops the timer. Ah. This should be called StopWatchTimer or just StopTimer. https://codereview.chromium.org/11415177/diff/8001/chrome/browser/extensions/... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.h (right): https://codereview.chromium.org/11415177/diff/8001/chrome/browser/extensions/... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:45: virtual void OnStorageFreeSpaceChanged(const std::string& id, Will you be adding other notifications? If not this should be abstract. Also, will this be called on the file thread? If so would be good to mention it. https://codereview.chromium.org/11415177/diff/8001/chrome/browser/extensions/... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:56: void AddObserver(Observer* obs); Would be good to mention can be called on any thread. https://codereview.chromium.org/11415177/diff/8001/chrome/browser/extensions/... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:73: typedef std::map<std::string, double> WatchingIDMap; The name should make it clear what the map is to as well as from. E.g. IDToSizeMap. https://codereview.chromium.org/11415177/diff/8001/chrome/browser/extensions/... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:89: void DoWatch(); Nit: a better name might be CheckWatchedStorages. https://codereview.chromium.org/11415177/diff/8001/chrome/browser/extensions/... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:107: unsigned int polling_interval_; Is this field needed? Seems like it could be removed and the default constant used instead, as the field is private and has no accessors. https://codereview.chromium.org/11415177/diff/8001/chrome/browser/extensions/... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc (right): https://codereview.chromium.org/11415177/diff/8001/chrome/browser/extensions/... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:50: double, double)); Super nit: indent >> 2 https://codereview.chromium.org/11415177/diff/8001/chrome/browser/extensions/... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:242: } Nit: } // namespace extensions.
https://chromiumcodereview.appspot.com/11415177/diff/8001/chrome/browser/exte... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc (right): https://chromiumcodereview.appspot.com/11415177/diff/8001/chrome/browser/exte... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:122: if (watching_ids_map_.size() == 0) On 2012/11/29 05:12:47, benwells wrote: > Please remove the early return. > > As having size == 0 is an error condition, and having this early exit is an > optimization (i.e. the rest of the code will work otherwise). Done. https://chromiumcodereview.appspot.com/11415177/diff/8001/chrome/browser/exte... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:145: void StorageInfoProvider::TerminateWatch() { On 2012/11/29 05:12:47, benwells wrote: > Oh, this just stops the timer. Ah. This should be called StopWatchTimer or just > StopTimer. Rename DestroyWatchingTimer now. https://chromiumcodereview.appspot.com/11415177/diff/8001/chrome/browser/exte... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.h (right): https://chromiumcodereview.appspot.com/11415177/diff/8001/chrome/browser/exte... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:45: virtual void OnStorageFreeSpaceChanged(const std::string& id, On 2012/11/29 05:12:47, benwells wrote: > Will you be adding other notifications? If not this should be abstract. > > Also, will this be called on the file thread? If so would be good to mention it. Other notifications will be added in future, e.g. OnStorageAttached and OnStorageDetached. Done. https://chromiumcodereview.appspot.com/11415177/diff/8001/chrome/browser/exte... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:56: void AddObserver(Observer* obs); On 2012/11/29 05:12:47, benwells wrote: > Would be good to mention can be called on any thread. Done. https://chromiumcodereview.appspot.com/11415177/diff/8001/chrome/browser/exte... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:73: typedef std::map<std::string, double> WatchingIDMap; On 2012/11/29 05:12:47, benwells wrote: > The name should make it clear what the map is to as well as from. E.g. > IDToSizeMap. Rename it to StorageIDToSizeMap now. https://chromiumcodereview.appspot.com/11415177/diff/8001/chrome/browser/exte... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:89: void DoWatch(); On 2012/11/29 05:12:47, benwells wrote: > Nit: a better name might be CheckWatchedStorages. Done. https://chromiumcodereview.appspot.com/11415177/diff/8001/chrome/browser/exte... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:107: unsigned int polling_interval_; On 2012/11/29 05:12:47, benwells wrote: > Is this field needed? Seems like it could be removed and the default constant > used instead, as the field is private and has no accessors. Currently it is only used for unit tests. It allows unit tests to set the interval to be shorter for making builtbot happy:) In future, I am planing to add a method to allow its consumer (systemInfo.storage extension API) to customize the interval, e.g. systemInfo.storage.setInterval, to customize the event firing frequency. https://chromiumcodereview.appspot.com/11415177/diff/8001/chrome/browser/exte... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc (right): https://chromiumcodereview.appspot.com/11415177/diff/8001/chrome/browser/exte... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:50: double, double)); On 2012/11/29 05:12:47, benwells wrote: > Super nit: indent >> 2 Done. https://chromiumcodereview.appspot.com/11415177/diff/8001/chrome/browser/exte... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:242: } On 2012/11/29 05:12:47, benwells wrote: > Nit: } // namespace extensions. Done.
benwells@, the patch is updated. Pls review it again. Nico, need your LGTM for gypi changes. Thanks
On 2012/11/29 08:45:38, Hongbo Min wrote: > benwells@, the patch is updated. Pls review it again. > > Nico, need your LGTM for gypi changes. > > Thanks ping benwells@...
lgtm with nits https://codereview.chromium.org/11415177/diff/6/chrome/browser/extensions/api... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc (right): https://codereview.chromium.org/11415177/diff/6/chrome/browser/extensions/api... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:111: storage_id_to_size_map_.erase(id); Nit: blank line after early return. https://codereview.chromium.org/11415177/diff/6/chrome/browser/extensions/api... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.h (right): https://codereview.chromium.org/11415177/diff/6/chrome/browser/extensions/api... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:103: // The thread-safe observer list that observe the changes happened on the Nit: ...that observes any changes happening to the storages.
ping Nico@, need your LGTM for gyp change. Thanks. https://codereview.chromium.org/11415177/diff/6/chrome/browser/extensions/api... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc (right): https://codereview.chromium.org/11415177/diff/6/chrome/browser/extensions/api... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:111: storage_id_to_size_map_.erase(id); On 2012/12/01 02:46:25, benwells wrote: > Nit: blank line after early return. Done. https://codereview.chromium.org/11415177/diff/6/chrome/browser/extensions/api... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.h (right): https://codereview.chromium.org/11415177/diff/6/chrome/browser/extensions/api... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:103: // The thread-safe observer list that observe the changes happened on the On 2012/12/01 02:46:25, benwells wrote: > Nit: ...that observes any changes happening to the storages. Done.
ping Nico@, need your LGTM for gyp change. Thanks.
gypi lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hongbo.min@intel.com/11415177/4006
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hongbo.min@intel.com/11415177/12004
Retried try job too often on win_rel for step(s) browser_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hongbo.min@intel.com/11415177/12004
Message was sent while issue was closed.
Change committed as 170667
Message was sent while issue was closed.
Pardon the drive-by; I found this CL while chasing down [ http://crbug.com/163964 ]. I know this CL is already closed, but I'd strongly encourage you to create a follow-up CL to address my comments below -- a couple are minor stylistic issues; but the rest are IMO fairly important to address: https://chromiumcodereview.appspot.com/11415177/diff/12004/chrome/browser/ext... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc (right): https://chromiumcodereview.appspot.com/11415177/diff/12004/chrome/browser/ext... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:86: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); You might want to consider using the file worker pool, rather than using the file thread -- see Fred Akalin's reply on this thread: https://groups.google.com/a/chromium.org/forum/?fromgroups=#!topic/chromium-d... https://chromiumcodereview.appspot.com/11415177/diff/12004/chrome/browser/ext... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.h (right): https://chromiumcodereview.appspot.com/11415177/diff/12004/chrome/browser/ext... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:101: base::RepeatingTimer<StorageInfoProvider>* watching_timer_; Why isn't this just allocated as a direct member of the class, i.e. "base::RepeatingTimer<StorageInfoProvider> watching_timer_;", calling watching_timer_.Stop() where you would otherwise delete it? If you really need to use a pointer, you should at least ensure memory safety by using a scoped_ptr<> and calling reset() when you'd want to NULL it out. https://chromiumcodereview.appspot.com/11415177/diff/12004/chrome/browser/ext... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc (right): https://chromiumcodereview.appspot.com/11415177/diff/12004/chrome/browser/ext... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:40: // The watching interval for unit test is 100 milliseconds. Why is this set so high? Does something fail if you set the interval to something more like 1 millisecond? Setting this to 100 milliseconds, and waiting for 100's of milliseconds within the test forces each test case to take 100's of milliseconds to run. Given the number of unit tests that Chrome has, this is not sustainable; and it doesn't seem necessary for testing the functionality that you're testing here. https://chromiumcodereview.appspot.com/11415177/diff/12004/chrome/browser/ext... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:156: for (int i = 1; i <= kCallTimes; i++) { You should use "++i" rather than "i++" https://chromiumcodereview.appspot.com/11415177/diff/12004/chrome/browser/ext... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:168: RunMessageLoopUntilTimeout(500); You should *never* use timeouts in tests unless you really have no other recourse. If you're listening for a timeout, your test will almost always end up being flaky. You should wait for an explicit event instead. https://chromiumcodereview.appspot.com/11415177/diff/12004/chrome/browser/ext... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:178: for (size_t k = 1; k < arraysize(testing_data); ++k) { Why does this loop start at 1 rather than 0? If this is intentional, the rationale should be documented with a comment.
Message was sent while issue was closed.
Sherman, thanks for your reviewing comments. Once we have final agreement on them, I will submit a new CL to fix them. https://codereview.chromium.org/11415177/diff/12004/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc (right): https://codereview.chromium.org/11415177/diff/12004/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:86: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); On 2012/12/03 19:38:00, Ilya Sherman wrote: > You might want to consider using the file worker pool, rather than using the > file thread -- see Fred Akalin's reply on this thread: > https://groups.google.com/a/chromium.org/forum/?fromgroups=#%21topic/chromium... I ever used the file worker pool. The limitation of worker pool is that no ThreadTaskRunnerHandle instance is set for the timer usage. With file thread, we can a message loop which sets the ThreadTaskRunnerHandle instance. https://codereview.chromium.org/11415177/diff/12004/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.h (right): https://codereview.chromium.org/11415177/diff/12004/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:101: base::RepeatingTimer<StorageInfoProvider>* watching_timer_; On 2012/12/03 19:38:00, Ilya Sherman wrote: > Why isn't this just allocated as a direct member of the class, i.e. > "base::RepeatingTimer<StorageInfoProvider> watching_timer_;", calling > watching_timer_.Stop() where you would otherwise delete it? If you really need > to use a pointer, you should at least ensure memory safety by using a > scoped_ptr<> and calling reset() when you'd want to NULL it out. The reason is, the timer is designed to work on file thread since we want not to block UI thread when querying storage info via StorageInfoProvider. If making it become a direct member of StorageInfoProvider, we will violate the thread model required by Timer, which requires that the task stopping must occur on the same thread as the task starting. https://codereview.chromium.org/11415177/diff/12004/chrome/browser/extensions... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc (right): https://codereview.chromium.org/11415177/diff/12004/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:40: // The watching interval for unit test is 100 milliseconds. On 2012/12/03 19:38:00, Ilya Sherman wrote: > Why is this set so high? Does something fail if you set the interval to > something more like 1 millisecond? Setting this to 100 milliseconds, and > waiting for 100's of milliseconds within the test forces each test case to take > 100's of milliseconds to run. Given the number of unit tests that Chrome has, > this is not sustainable; and it doesn't seem necessary for testing the > functionality that you're testing here. No special consideration on the value. 10 milliseconds might be also fine. https://codereview.chromium.org/11415177/diff/12004/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:168: RunMessageLoopUntilTimeout(500); On 2012/12/03 19:38:00, Ilya Sherman wrote: > You should *never* use timeouts in tests unless you really have no other > recourse. If you're listening for a timeout, your test will almost always end > up being flaky. You should wait for an explicit event instead. Yes, I really have no other recourse. I just a post a delayed task to quit the message loop after a given time interval since there is no event received for quitting the message loop in my test. But I am not sure why the tests are always ending up being flaky. https://codereview.chromium.org/11415177/diff/12004/chrome/browser/extensions... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:178: for (size_t k = 1; k < arraysize(testing_data); ++k) { On 2012/12/03 19:38:00, Ilya Sherman wrote: > Why does this loop start at 1 rather than 0? If this is intentional, the > rationale should be documented with a comment. Since the change step of the first one is zero. I will update the comment.
Message was sent while issue was closed.
https://codereview.chromium.org/11415177/diff/12004/chrome/browser/extensions... > chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:86: > DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); > On 2012/12/03 19:38:00, Ilya Sherman wrote: > > You might want to consider using the file worker pool, rather than using the > > file thread -- see Fred Akalin's reply on this thread: > > > https://groups.google.com/a/chromium.org/forum/?fromgroups=#%2521topic/chromi... > > I ever used the file worker pool. The limitation of worker pool is that no > ThreadTaskRunnerHandle instance is set for the timer usage. With file thread, we > can a message loop which sets the ThreadTaskRunnerHandle instance. > file worker pool? I assume what you mentioned is the worker blocking pool since I can grep content::BrowserThread::GetBlockingPool rather than content::GetFileBlockingPool as mentioned by Fred Akalin in chromium-dev maillist.
Message was sent while issue was closed.
On 2012/12/04 08:17:06, Hongbo Min wrote: > https://codereview.chromium.org/11415177/diff/12004/chrome/browser/extensions... > > chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:86: > > DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); > > On 2012/12/03 19:38:00, Ilya Sherman wrote: > > > You might want to consider using the file worker pool, rather than using the > > > file thread -- see Fred Akalin's reply on this thread: > > > > > > https://groups.google.com/a/chromium.org/forum/?fromgroups=#%252521topic/chro... > > > > I ever used the file worker pool. The limitation of worker pool is that no > > ThreadTaskRunnerHandle instance is set for the timer usage. With file thread, > we > > can a message loop which sets the ThreadTaskRunnerHandle instance. > > > > file worker pool? I assume what you mentioned is the worker blocking pool since > I can grep content::BrowserThread::GetBlockingPool rather than > content::GetFileBlockingPool as mentioned by Fred Akalin in chromium-dev > maillist. Yes, sorry, content::browserThread::GetBlockingPool() is what I meant. https://chromiumcodereview.appspot.com/11415177/diff/12004/chrome/browser/ext... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc (right): https://chromiumcodereview.appspot.com/11415177/diff/12004/chrome/browser/ext... chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:86: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); On 2012/12/04 02:58:25, Hongbo Min wrote: > On 2012/12/03 19:38:00, Ilya Sherman wrote: > > You might want to consider using the file worker pool, rather than using the > > file thread -- see Fred Akalin's reply on this thread: > > > https://groups.google.com/a/chromium.org/forum/?fromgroups=#%2521topic/chromi... > > I ever used the file worker pool. The limitation of worker pool is that no > ThreadTaskRunnerHandle instance is set for the timer usage. With file thread, we > can a message loop which sets the ThreadTaskRunnerHandle instance. Again, you should be able to post a task from the UI thread to the worker pool. The disadvantage of using the FILE thread is that lots of slow operations often run on that thread, which means your tasks can end up getting blocked for lengthy periods of time. https://chromiumcodereview.appspot.com/11415177/diff/12004/chrome/browser/ext... File chrome/browser/extensions/api/system_info_storage/storage_info_provider.h (right): https://chromiumcodereview.appspot.com/11415177/diff/12004/chrome/browser/ext... chrome/browser/extensions/api/system_info_storage/storage_info_provider.h:101: base::RepeatingTimer<StorageInfoProvider>* watching_timer_; On 2012/12/04 02:58:25, Hongbo Min wrote: > On 2012/12/03 19:38:00, Ilya Sherman wrote: > > Why isn't this just allocated as a direct member of the class, i.e. > > "base::RepeatingTimer<StorageInfoProvider> watching_timer_;", calling > > watching_timer_.Stop() where you would otherwise delete it? If you really > need > > to use a pointer, you should at least ensure memory safety by using a > > scoped_ptr<> and calling reset() when you'd want to NULL it out. > > The reason is, the timer is designed to work on file thread since we want not to > block UI thread when querying storage info via StorageInfoProvider. If making it > become a direct member of StorageInfoProvider, we will violate the thread model > required by Timer, which requires that the task stopping must occur on the same > thread as the task starting. It's no more legal to allocate (new) and free (delete) the timer on different threads. The usual (and recommended) pattern is to perform Start() and Stop() operations on the thread on which your object lives, using task posting to accomplish this. So, you'll call Start() on the UI thread and post a task to the FILE thread when the timer fires. When the data comes back on the FILE thread indicating that the timer should be stopped, you'll post a task back to the UI thread that tells the timer to stop. https://chromiumcodereview.appspot.com/11415177/diff/12004/chrome/browser/ext... File chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc (right): https://chromiumcodereview.appspot.com/11415177/diff/12004/chrome/browser/ext... chrome/browser/extensions/api/system_info_storage/storage_info_provider_unittest.cc:168: RunMessageLoopUntilTimeout(500); On 2012/12/04 02:58:25, Hongbo Min wrote: > On 2012/12/03 19:38:00, Ilya Sherman wrote: > > You should *never* use timeouts in tests unless you really have no other > > recourse. If you're listening for a timeout, your test will almost always end > > up being flaky. You should wait for an explicit event instead. > > Yes, I really have no other recourse. I just a post a delayed task to quit the > message loop after a given time interval since there is no event received for > quitting the message loop in my test. But I am not sure why the tests are always > ending up being flaky. At least for the cases that expect an observer to be notified, there is an explicit event that you can wait for: the observer being notified. Waiting for that event might require using custom mocking rather than using gMock, but it's certainly feasible. For the cases where you're testing that observers *aren't* notified, it's a little trickier. One option is to add a call within StorageInfoProvider::CheckWatchedStorages() to a method named something like CheckWatchedStoragesFinishedForTesting(), which is only exposed to the test code. You could then wait until that method is called, and then return.
Message was sent while issue was closed.
sherman, Generally speaking, I understand your concerns about the Timer usage and FILE　thread. Actually, the querying operation doesn't require a fine-grained real-time, so it should be fine although there is a lengthy operation occurred on FILE thread. Anyway, you exposed a good question to the implementation, can we have an opportunity to optimize the implementation using worker blocking pool? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
