Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10831353:
Add a generic template for system info provider (Closed)
|
Created:
8 years, 4 months ago by Hongbo Min Modified:
8 years, 4 months ago CC:
chromium-reviews, Aaron Boodman, mihaip-chromium-reviews_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdd a generic template for system info provider and also provides mock cpu info provider based on it.
BUG=136519
TEST=browser_tests --gtest_filter=SystemInfoCpuApiTest.*
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152950
Patch Set 1 : Simplify the SystemInfoProvider template code #
Total comments: 20
Patch Set 2 : Updated patch #Patch Set 3 : Expose QueryInfo interface publicly #
Total comments: 11
Patch Set 4 : Code polishment #Messages
Total messages: 23 (0 generated)
Hi, reviewers, This patch is to add a generic template for system information. I found the CpuInfoProvider and StorageInfoProvider shares lots of common features except the information type differs. So I abstracts away them and write a generic template SystemInfoProvider<T> for all system information used in systemInfo API. In addition, each SystemInfoProvider is designed to a single shared instance which is created only when needed and destroyed if nobody is using it. Pls have a review. Thanks.
The gypi changes lgtm. James is on vacation as far as I know, so it looks like the main review is for Mihai :-) (I don't know the history here, but it looks like you're using the file thread. I think we generally prefer using the blocking pool for background work these days.)
On 2012/08/16 14:58:06, Nico wrote: > The gypi changes lgtm. James is on vacation as far as I know, so it looks like > the main review is for Mihai :-) > > (I don't know the history here, but it looks like you're using the file thread. > I think we generally prefer using the blocking pool for background work these > days.) Thanks for your information. Could you point me where use the blocking pool currently in chromium? The purpose of using FILE thread is to avoid potential blocking to UI, it need to query some information from system, which might be a time-consuming operation.
On Thu, Aug 16, 2012 at 4:29 PM, <hongbo.min@intel.com> wrote: > On 2012/08/16 14:58:06, Nico wrote: >> >> The gypi changes lgtm. James is on vacation as far as I know, so it looks >> like >> the main review is for Mihai :-) > > >> (I don't know the history here, but it looks like you're using the file > > thread. >> >> I think we generally prefer using the blocking pool for background work >> these >> days.) > > > Thanks for your information. > > Could you point me where use the blocking pool currently in chromium? The > purpose of using FILE thread is to avoid potential blocking to UI, it need > to > query some information from system, which might be a time-consuming > operation. Look for `BrowserThread::GetBlockingPool()->PostSequencedWorkerTask`. That's the same motivation as the FILE thread, but backed with more threads. See a post to chromium-dev by brettw from a while ago for more information
On 2012/08/16 23:47:15, Nico wrote: > On Thu, Aug 16, 2012 at 4:29 PM, <mailto:hongbo.min@intel.com> wrote: > > On 2012/08/16 14:58:06, Nico wrote: > >> > >> The gypi changes lgtm. James is on vacation as far as I know, so it looks > >> like > >> the main review is for Mihai :-) > > > > > >> (I don't know the history here, but it looks like you're using the file > > > > thread. > >> > >> I think we generally prefer using the blocking pool for background work > >> these > >> days.) > > > > > > Thanks for your information. > > > > Could you point me where use the blocking pool currently in chromium? The > > purpose of using FILE thread is to avoid potential blocking to UI, it need > > to > > query some information from system, which might be a time-consuming > > operation. > > Look for `BrowserThread::GetBlockingPool()->PostSequencedWorkerTask`. > That's the same motivation as the FILE thread, but backed with more > threads. See a post to chromium-dev by brettw from a while ago for > more information Yes, I did find the post in chromium-dev. Thanks a lot.
On 2012/08/16 13:36:25, Hongbo wrote: > Hi, reviewers, > > This patch is to add a generic template for system information. > > I found the CpuInfoProvider and StorageInfoProvider shares lots of common > features except the information type differs. So I abstracts away them and write > a generic template SystemInfoProvider<T> for all system information used in > systemInfo API. > > In addition, each SystemInfoProvider is designed to a single shared instance > which is created only when needed and destroyed if nobody is using it. > > Pls have a review. Thanks. If this patch gets landed, the next step is to add platform specific code for cpu info provider and storage info provider.
This generally seems fine, but how expensive do you envision it being to have these providers live indefinitely? That would simplify the code quite a bit if, once created, they were reused for all Get() calls after that (no more ref counting, no more weak ptrs). http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/a... File chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc (right): http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc:12: typedef SystemInfoProvider<CpuInfo> CpuInfoProvider; Having to repeat this typedef in all implementation files seem awkward. Might as well put it in a cpu_info_provider.h header that gets included everywhere. http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc:17: CpuInfoProviderLinux() {} Nit: these lines are indented by one space too many (applies to all files). http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/a... File chrome/browser/extensions/api/system_info_cpu/system_info_cpu_api.cc (right): http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/system_info_cpu_api.cc:37: SetError("error in querying cpu information"); Capitalize error message and add a period. http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/a... File chrome/browser/extensions/api/system_info_cpu/system_info_cpu_apitest.cc (right): http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/system_info_cpu_apitest.cc:19: // Clear the this.info_ firstly Redundant comment, please delete. http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/system_info_cpu_apitest.cc:40: // Use mock implementation Ditto. http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/s... File chrome/browser/extensions/system_info_provider.h (right): http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:19: // of SystemInfProvider is a single shared instance. It is created if needed, Typo (SystemInfProvider). http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:20: // but won't be kept if nobody is using it. This is done by LazyInstance and s/done by/done via/. http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:23: // The SystemInfoProvider is designed to query system information on FILE "on the FILE thread" is more correct. http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:24: // thread. It also maintains a callback queue which is waiting for the "It maintains a queue of callbacks which are waiting for" http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:25: // completion of system information querying. It could avoid query Instead of "It could avoid" it's more correct to say "It avoids" http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:28: // These callbacks are created on UI thread, and run also on UI thread. "the UI thread" http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:58: // If |is_waiting_for_completion_| is true, nothing to do. This comment is redundant with the code. It would be even more obvious if you changed the code to do an early return instead. http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:63: content::BrowserThread::PostTask(content::BrowserThread::FILE, FROM_HERE, Are you going to switch this to use PostSequencedWorkerTask? http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:83: virtual bool GetInfo() = 0; This should be called FetchInfo or LoadInfo or ProvideInfo. The Get* name makes it sound like it actually returns the info. http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:90: // Travers the queue to invoke the pending callbacks Typo: traverse. http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:93: callback.Run(info_, success); You still pass back the info_, even if the fetching failed? Are there any guarantees about it contents at that point? http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:102: // destroy it if nobody is using it, so use a WeakPtr with LazInstance. This isn't quite right, you don't destroy it per se, but it will get destroyed automatically due to reference counting. Typo (LazInstance). http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:104: base::WeakPtr<SystemInfoProvider<T> > >::Leaky g_shared_provider; This isn't actually a global variable, it doesn't need a g_ prefix. http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:117: // The weak pointer factory for generating "this" pointer which might This, and most other members, should be private. http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:126: // sychronous flag for UI thread and FILE thread. You only use this variable on the UI thread.
On 2012/08/17 23:40:30, Mihai Parparita wrote: > This generally seems fine, but how expensive do you envision it being to have > these providers live indefinitely? That would simplify the code quite a bit if, > once created, they were reused for all Get() calls after that (no more ref Yes, if a systemInfo.cpu.get method is called repeatedly, the shared instance will be created indefinitely, however it can reduce the memory footprint if only call systemInfo.cpu.get one time. If the performance is more important than memory footprint, once it is created, let it there and all Get() shared the same one info provider. I will update the patch according to comment. Thanks.
Thanks for your comments. I have updated the patch. The single shared SystemInfoProvider instance is now simplified: no ref counting and weak pointer. Th InitializeForTesting method is added into SystemInfoProvider for testing purpose. http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/a... File chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc (right): http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc:12: typedef SystemInfoProvider<CpuInfo> CpuInfoProvider; On 2012/08/17 23:40:30, Mihai Parparita wrote: > Having to repeat this typedef in all implementation files seem awkward. Might as > well put it in a cpu_info_provider.h header that gets included everywhere. Add a common header file cpu_info_provider.h Done! http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/cpu_info_provider_linux.cc:17: CpuInfoProviderLinux() {} On 2012/08/17 23:40:30, Mihai Parparita wrote: > Nit: these lines are indented by one space too many (applies to all files). Done. http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/a... File chrome/browser/extensions/api/system_info_cpu/system_info_cpu_api.cc (right): http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/system_info_cpu_api.cc:37: SetError("error in querying cpu information"); On 2012/08/17 23:40:30, Mihai Parparita wrote: > Capitalize error message and add a period. Done. http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/a... File chrome/browser/extensions/api/system_info_cpu/system_info_cpu_apitest.cc (right): http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/system_info_cpu_apitest.cc:19: // Clear the this.info_ firstly On 2012/08/17 23:40:30, Mihai Parparita wrote: > Redundant comment, please delete. Done. http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/system_info_cpu_apitest.cc:40: // Use mock implementation On 2012/08/17 23:40:30, Mihai Parparita wrote: > Ditto. Done. http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/s... File chrome/browser/extensions/system_info_provider.h (right): http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:19: // of SystemInfProvider is a single shared instance. It is created if needed, On 2012/08/17 23:40:30, Mihai Parparita wrote: > Typo (SystemInfProvider). Done. http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:20: // but won't be kept if nobody is using it. This is done by LazyInstance and On 2012/08/17 23:40:30, Mihai Parparita wrote: > s/done by/done via/. Done. http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:23: // The SystemInfoProvider is designed to query system information on FILE On 2012/08/17 23:40:30, Mihai Parparita wrote: > "on the FILE thread" is more correct. Done. http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:24: // thread. It also maintains a callback queue which is waiting for the On 2012/08/17 23:40:30, Mihai Parparita wrote: > "It maintains a queue of callbacks which are waiting for" Done. http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:25: // completion of system information querying. It could avoid query On 2012/08/17 23:40:30, Mihai Parparita wrote: > Instead of "It could avoid" it's more correct to say "It avoids" Done. http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:28: // These callbacks are created on UI thread, and run also on UI thread. On 2012/08/17 23:40:30, Mihai Parparita wrote: > "the UI thread" Done. http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:58: // If |is_waiting_for_completion_| is true, nothing to do. On 2012/08/17 23:40:30, Mihai Parparita wrote: > This comment is redundant with the code. It would be even more obvious if you > changed the code to do an early return instead. Done. http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:63: content::BrowserThread::PostTask(content::BrowserThread::FILE, FROM_HERE, On 2012/08/17 23:40:30, Mihai Parparita wrote: > Are you going to switch this to use PostSequencedWorkerTask? Done. http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:83: virtual bool GetInfo() = 0; On 2012/08/17 23:40:30, Mihai Parparita wrote: > This should be called FetchInfo or LoadInfo or ProvideInfo. The Get* name makes > it sound like it actually returns the info. I rename QueryInfo instead. Does it make sense? http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:90: // Travers the queue to invoke the pending callbacks On 2012/08/17 23:40:30, Mihai Parparita wrote: > Typo: traverse. Done. http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:93: callback.Run(info_, success); On 2012/08/17 23:40:30, Mihai Parparita wrote: > You still pass back the info_, even if the fetching failed? Are there any > guarantees about it contents at that point? The info contents are guaranteed by the |success| parameter, if it is false, the contents may be malformed. http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:104: base::WeakPtr<SystemInfoProvider<T> > >::Leaky g_shared_provider; On 2012/08/17 23:40:30, Mihai Parparita wrote: > This isn't actually a global variable, it doesn't need a g_ prefix. Rename it to "single_shared_provider_". http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:117: // The weak pointer factory for generating "this" pointer which might On 2012/08/17 23:40:30, Mihai Parparita wrote: > This, and most other members, should be private. Done. http://codereview.chromium.org/10831353/diff/5001/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:126: // sychronous flag for UI thread and FILE thread. On 2012/08/17 23:40:30, Mihai Parparita wrote: > You only use this variable on the UI thread. Already update the code comments.
Sorry, I deleted the origin patch set by mistake. Is there any way to restore it again? Thanks.
Thanks for the simplification, I think this is getting closer to landing. http://codereview.chromium.org/10831353/diff/1027/chrome/browser/extensions/a... File chrome/browser/extensions/api/system_info_cpu/system_info_cpu_api.cc (right): http://codereview.chromium.org/10831353/diff/1027/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/system_info_cpu_api.cc:30: SetError("Error occurs when querying cpu information."); Nit: "Error occurred when querying CPU information." is more correct. http://codereview.chromium.org/10831353/diff/1027/chrome/browser/extensions/a... File chrome/browser/extensions/api/system_info_cpu/system_info_cpu_apitest.cc (right): http://codereview.chromium.org/10831353/diff/1027/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/system_info_cpu_apitest.cc:19: virtual bool QueryInfo() { Add OVERRIDE annotation. http://codereview.chromium.org/10831353/diff/1027/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/system_info_cpu_apitest.cc:50: CpuInfoProvider* provider_; Does this need to be a member? You don't actually use it outside of SetUpInProcessBrowserTestFixture. If it does, then it should have a comment indicating that it's owned by single_shared_provider_ http://codereview.chromium.org/10831353/diff/1027/chrome/browser/extensions/s... File chrome/browser/extensions/system_info_provider.h (right): http://codereview.chromium.org/10831353/diff/1027/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:73: // Post the query task to the worker pool with CONTINUE_ON_SHUTDOWN This comment just says what the code says. Either change it to explain why CONTINUE_ON_SHUTDOWN is the appropriate choice, or delete it. http://codereview.chromium.org/10831353/diff/1027/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:86: // Query the information on worker pool. Another redundant comment. http://codereview.chromium.org/10831353/diff/1027/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:89: // Notify UI thread once it is completed. Ditto. http://codereview.chromium.org/10831353/diff/1027/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:106: // Traverse the queue to invoke the pending callbacks. Ditto. http://codereview.chromium.org/10831353/diff/1027/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:113: // Reset waiting status. Ditto. http://codereview.chromium.org/10831353/diff/1027/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:131: // an argument for base::Bind. How is the base::Bind discussion relevant? http://codereview.chromium.org/10831353/diff/1027/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:155: typename base::LazyInstance<scoped_ptr<SystemInfoProvider<T> > > Do you actually need to use scoped_ptr here? From the LazyInstance docs: "When the instance is constructed it is registered with AtExitManager. The destructor will be called on program exit."
Will update it later. Thanks. http://codereview.chromium.org/10831353/diff/1027/chrome/browser/extensions/s... File chrome/browser/extensions/system_info_provider.h (right): http://codereview.chromium.org/10831353/diff/1027/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:155: typename base::LazyInstance<scoped_ptr<SystemInfoProvider<T> > > On 2012/08/21 01:14:52, Mihai Parparita wrote: > Do you actually need to use scoped_ptr here? From the LazyInstance docs: "When > the instance is constructed it is registered with AtExitManager. The destructor > will be called on program exit." The reason I use scoped_ptr is, the SystemInfoProvider instance is constructed only if needed. Otherwise, the LazyInstance will allocate a memory buffer for the SystemInfoProvider instance even if nobody is using it, e.g. no any extension calls systemInfo API. So it can reduce the memory footprint in case of the systemInfo API is not used at all.
The patch is now updated. http://codereview.chromium.org/10831353/diff/1027/chrome/browser/extensions/a... File chrome/browser/extensions/api/system_info_cpu/system_info_cpu_api.cc (right): http://codereview.chromium.org/10831353/diff/1027/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/system_info_cpu_api.cc:30: SetError("Error occurs when querying cpu information."); On 2012/08/21 01:14:52, Mihai Parparita wrote: > Nit: "Error occurred when querying CPU information." is more correct. Done. http://codereview.chromium.org/10831353/diff/1027/chrome/browser/extensions/a... File chrome/browser/extensions/api/system_info_cpu/system_info_cpu_apitest.cc (right): http://codereview.chromium.org/10831353/diff/1027/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/system_info_cpu_apitest.cc:19: virtual bool QueryInfo() { On 2012/08/21 01:14:52, Mihai Parparita wrote: > Add OVERRIDE annotation. Done. http://codereview.chromium.org/10831353/diff/1027/chrome/browser/extensions/a... chrome/browser/extensions/api/system_info_cpu/system_info_cpu_apitest.cc:50: CpuInfoProvider* provider_; On 2012/08/21 01:14:52, Mihai Parparita wrote: > Does this need to be a member? You don't actually use it outside of > SetUpInProcessBrowserTestFixture. > > If it does, then it should have a comment indicating that it's owned by > single_shared_provider_ Done. http://codereview.chromium.org/10831353/diff/1027/chrome/browser/extensions/s... File chrome/browser/extensions/system_info_provider.h (right): http://codereview.chromium.org/10831353/diff/1027/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:73: // Post the query task to the worker pool with CONTINUE_ON_SHUTDOWN On 2012/08/21 01:14:52, Mihai Parparita wrote: > This comment just says what the code says. Either change it to explain why > CONTINUE_ON_SHUTDOWN is the appropriate choice, or delete it. Done. http://codereview.chromium.org/10831353/diff/1027/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:86: // Query the information on worker pool. On 2012/08/21 01:14:52, Mihai Parparita wrote: > Another redundant comment. Done. http://codereview.chromium.org/10831353/diff/1027/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:89: // Notify UI thread once it is completed. On 2012/08/21 01:14:52, Mihai Parparita wrote: > Ditto. Done. http://codereview.chromium.org/10831353/diff/1027/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:106: // Traverse the queue to invoke the pending callbacks. On 2012/08/21 01:14:52, Mihai Parparita wrote: > Ditto. Done. http://codereview.chromium.org/10831353/diff/1027/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:131: // an argument for base::Bind. On 2012/08/21 01:14:52, Mihai Parparita wrote: > How is the base::Bind discussion relevant? Removed it now. The origin implementation is to allocate T object on stack in QueryOnWorkerPool, but since the T is disallowed to copy constructor, it can be used to base::Bind as a callback parameter.
http://codereview.chromium.org/10831353/diff/1027/chrome/browser/extensions/s... File chrome/browser/extensions/system_info_provider.h (right): http://codereview.chromium.org/10831353/diff/1027/chrome/browser/extensions/s... chrome/browser/extensions/system_info_provider.h:131: // an argument for base::Bind. On 2012/08/21 03:07:11, Hongbo Min wrote: > On 2012/08/21 01:14:52, Mihai Parparita wrote: > > How is the base::Bind discussion relevant? > > Removed it now. > > The origin implementation is to allocate T object on stack in QueryOnWorkerPool, > but since the T is disallowed to copy constructor, it can be used to base::Bind > as a callback parameter. Correct: It can not be used as a callback parameter in base::Bind.
Some updates. Pls review it. Thanks. http://codereview.chromium.org/10831353/diff/18001/chrome/browser/extensions/... File chrome/browser/extensions/system_info_provider.h (right): http://codereview.chromium.org/10831353/diff/18001/chrome/browser/extensions/... chrome/browser/extensions/system_info_provider.h:62: virtual void StartQueryInfo(const QueryInfoCompletionCallback& callback) { StartGetInfo is renamed to StartQueryInfo for consistency. http://codereview.chromium.org/10831353/diff/18001/chrome/browser/extensions/... chrome/browser/extensions/system_info_provider.h:89: virtual bool QueryInfo(T* info) = 0; When I am writing the code for watching storage free space change event, I found the QueryInfo should be exposed as pubic. StartQueryInfo function is only allowed to run on the UI thread, it is fine for 'get' extension API. However, if we are watching the storage space change on the worker pool, and get a space change notification, then we will query the storage information for getting the available capacity on the worker pool. QueryInfo function helps us avoid querying the info via StartQueryInfo from the UI thread. Does it make sense? Thanks.
Mihai, Together with issue http://codereview.chromium.org/10836341/, both of these two patches provide a general framework for systemInfo API. If these 2 patches get landed, the development for implementation on different platforms could be in parallel, and it can accelerate the whole progress. I also have the prototype code for cpu.get implementation on Windows and Linux, and wait for this patch gets landed. Regarding to system info watcher for monitoring device change event, I found there are some similar stuffs in chromium code base, such as DeviceMonitorLinux, SystemMessageWindow etc. However, the scattering device watcher functionalities is hard to reusable. Do you think that it is possible to provide a unify framework for device monitor to detect system hardware changes, such as device arrival/removal etc?
LGTM http://codereview.chromium.org/10831353/diff/18001/chrome/browser/extensions/... File chrome/browser/extensions/system_info_provider.h (right): http://codereview.chromium.org/10831353/diff/18001/chrome/browser/extensions/... chrome/browser/extensions/system_info_provider.h:18: // A generic template for all kinds of system information provider. Each kind "providers" is more grammatically correct. http://codereview.chromium.org/10831353/diff/18001/chrome/browser/extensions/... chrome/browser/extensions/system_info_provider.h:27: // of query requests are arrived, e.g. calling systemInfo.cpu.get repeatedly "are arrived" can be removed. http://codereview.chromium.org/10831353/diff/18001/chrome/browser/extensions/... chrome/browser/extensions/system_info_provider.h:28: // in renderer. instead of a "in renderer", "in an extension process" is more correct. http://codereview.chromium.org/10831353/diff/18001/chrome/browser/extensions/... chrome/browser/extensions/system_info_provider.h:92: virtual void QueryOnWorkerPool() { Does this need to be virtual? http://codereview.chromium.org/10831353/diff/18001/chrome/browser/extensions/... chrome/browser/extensions/system_info_provider.h:101: virtual void OnQueryCompleted(bool success) { Ditto. http://codereview.chromium.org/10831353/diff/18001/chrome/browser/extensions/... chrome/browser/extensions/system_info_provider.h:126: // type generated by IDL parser. Ah, you mean that because T doesn't have a copy constructor, it can't be directory passed as a parameter to callbacks, therefore we need a member instance, and pass around a pointer to it? Also, now that QueryInfo takes a pointer to it, I think this can be a private member.
On Wed, Aug 22, 2012 at 7:14 AM, <hongbo.min@intel.com> wrote: > Regarding to system info watcher for monitoring device change event, I > found > there are some similar stuffs in chromium code base, such as > DeviceMonitorLinux, > SystemMessageWindow etc. However, the scattering device watcher > functionalities > is hard to reusable. > > Do you think that it is possible to provide a unify framework for device > monitor > to detect system hardware changes, such as device arrival/removal etc? > It looks like base/system_monitor tries to encompass such functioanlity. I'm not that familiar with it, but if it's not meeting your needs, then perhaps you can propose improvements to willchan@ and/or brettw@ (recent reviewers of that code). Feel free to cc me on those reviews. Mihai
http://codereview.chromium.org/10831353/diff/18001/chrome/browser/extensions/... File chrome/browser/extensions/system_info_provider.h (right): http://codereview.chromium.org/10831353/diff/18001/chrome/browser/extensions/... chrome/browser/extensions/system_info_provider.h:92: virtual void QueryOnWorkerPool() { On 2012/08/23 00:24:52, Mihai Parparita wrote: > Does this need to be virtual? Platform specific implementation may have different strategy for posting task to the worker pool. For example, on Linux, if reading CPU usage info from /proc/stat, the time interval is required between twice times reading /proc/stat. http://codereview.chromium.org/10831353/diff/18001/chrome/browser/extensions/... chrome/browser/extensions/system_info_provider.h:101: virtual void OnQueryCompleted(bool success) { On 2012/08/23 00:24:52, Mihai Parparita wrote: > Ditto. Same as the above. http://codereview.chromium.org/10831353/diff/18001/chrome/browser/extensions/... chrome/browser/extensions/system_info_provider.h:126: // type generated by IDL parser. On 2012/08/23 00:24:52, Mihai Parparita wrote: > Ah, you mean that because T doesn't have a copy constructor, it can't be > directory passed as a parameter to callbacks, therefore we need a member > instance, and pass around a pointer to it? > > Also, now that QueryInfo takes a pointer to it, I think this can be a private > member. Correct. It is protected due to the same reason as above.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hongbo.min@intel.com/10831353/22003
On 2012/08/23 00:31:46, Mihai Parparita wrote: > On Wed, Aug 22, 2012 at 7:14 AM, <mailto:hongbo.min@intel.com> wrote: > > > Regarding to system info watcher for monitoring device change event, I > > found > > there are some similar stuffs in chromium code base, such as > > DeviceMonitorLinux, > > SystemMessageWindow etc. However, the scattering device watcher > > functionalities > > is hard to reusable. > > > > Do you think that it is possible to provide a unify framework for device > > monitor > > to detect system hardware changes, such as device arrival/removal etc? > > > > It looks like base/system_monitor tries to encompass such functioanlity. > I'm not that familiar with it, but if it's not meeting your needs, then > perhaps you can propose improvements to willchan@ and/or brettw@ (recent > reviewers of that code). Feel free to cc me on those reviews. > > Mihai Thanks for your suggestion. I am in the process of figuring out the gaps in system monitor for systemInfo API.
Change committed as 152950 |
