[SystemInfo API] Finish TODOs in SystemInfoProvider

chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc

Index: chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
diff --git a/chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc b/chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
index 581e02f509b9c360f2cdeeaf22fb103babe3dc53..0d72d7241e06a1c0a2449eda35f3f8da5b2f7dc3 100644
--- a/chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
+++ b/chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
@@ -50,6 +50,10 @@ StorageInfoProvider::StorageInfoProvider()
 StorageInfoProvider::~StorageInfoProvider() {
 }
 
+const StorageInfo& StorageInfoProvider::storage_info() const {
+  return info_;
+}
+
 void StorageInfoProvider::StartQueryInfoImpl() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   // Register a callback to notify UI thread that StorageMonitor finishes the
@@ -61,16 +65,17 @@ void StorageInfoProvider::StartQueryInfoImpl() {
 
 void StorageInfoProvider::QueryInfoImplOnUIThread() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  // At this point, we can call StorageMonitor::GetAllAvailableStorages to
-  // get the correct results.
-  QueryInfo(&info_);
-  // The amount of available capacity should be queried on blocking pool.
-  PostQueryTaskToBlockingPool(FROM_HERE,
+
+  // QueryInfo can be called on UI thread since in the implementation we use

[Jeffrey Yasskin, 2013/07/08 19:09:49]

s/can/must/, I think?

This explains why my suggestion to pull out EnsureInitialized() wasn't enough to
make StartQueryInfoImpl() non-virtual and private, since this subclass needs to
collect information on the UI thread before it can run a synchronous method on
the blocking pool to collect the actual result.

It's misleading to call the UI thread part of this subclass "QueryInfo()", since
that method's contract, established in the base class, is to run on the blocking
pool. I'd rename QueryInfo to GetAllStoragesIntoInfoList and rename
QueryAvailableCapacityOnBlockingPool to QueryInfo.

That rename, in turn, reveals another possible refactoring to avoid needing to
override the complex behavior of StartQueryInfoImpl: Create a default-empty
virtual method PrepareQueryOnUIThread(), which this class would override to call
GetAllStoragesIntoInfoList. Add that to the previous suggestion to provide an
InitializeProvider(callback), and I think you'll be able to de-virtual
StartQueryInfoImpl.

[Haojian Wu, 2013/07/09 13:45:04]

Rename Done.

I'm not sure whether I get you key point.Below are my understanding with your
refactoring suggestion.

class SystemInfoProvider {
  ...
 protected:
  virtual PrepareQueryOnUIThread() {}
  virtual InitializeProvider(callback) { callback.Run(); }

  StartQueryInfo() {
    ...
    InitilaizeProvider(Bind(&PrepareQueryOnUIThread));
    StartQueryInfoImpl().
  }

 private:
  StartQueryInfoImpl() { PostQueryTaskToBlockPool(&QueryInfo); }
}

class StorageInfoProvider {
 protected:
  PrepareQueryOnUIThread() {
   
StorageMonitor::Get()->EnsureInitialized(&StorageInfoProvider::GetAllStoragesInfoList);
  }
}

If I'm correct, I think this refactoring way also can not de-virtual
StartQueryInfoImpl.
In this way, we move the QueryInfo out of
StorageMonitor::EnsureInitialized(callback) callback function's body, which may
lead to  incorrect behavior. Because StorageMonitor::EnsureInitialized is a
asynchronous method which will post its storage init task to worker pool too.

StorageInfoProvider::QueryInfo should be called only after the StorageMonitor
gets initialized. And in the above way, QueryInfo is possible to be called
before StorageMonitor initialized, which requires StorageInfoProvider::QueryInfo
should be called in callback argument function of
StorageMonitor::EnsureInitialized(callback).

Also, I think the usage of EnsureInitialized in StorageInfoProvider is more like
StartQueryInfoImpl rather than initialization since we just want to
GetAllAvailableStorages to get the all storage devices.

  

[Greg Billock, 2013/07/09 16:44:52]

Here's my thought on this:

SystemInfoProvider should not be a singleton. It should be more like an
ExtensionFunction subclass that runs the QueryInfo virtual in the blocking pool.
There's no reason to have a singleton around to do things like cpu info
querying. That's a waste.

Modeling it as an ExtensionFunction subclass will let you avoid templating and
point the way to how to get the right delegation and separation of concerns --
the class should be purely concerned with executing a virtual in the right
thread context. That's it. Then you can do all the work in your implementation.

A couple of the sysinfo calls may not even need it. (Do the display info and
memory info calls need to run in the blocking pool? Seems like using the UI
thread would be fine there.)

I think this change is a step towards that, but I recommend we go that
direction, which means most of this class will be unrecognizable, and virtually
all the inheritance will disappear as well, and just be implemented directly in
the SystemInfo*Function classes directly. That'll erase hundreds of lines of
boilerplate as well as perhaps 10 files. :-)

Also, let's think about collapsing all the directories into .../api/system_info.

[Jeffrey Yasskin, 2013/07/09 18:19:59]

The above method would post QueryInfo() before PrepareQueryOnUIThread() is
guaranteed to have finished. Instead, I meant:

  ...
  StartQueryInfo() {
    ...
    InitializeProvider(Bind(&StartQueryInfoPostInitialization));
  }

 private:
  StartQueryInfoPostInitialization() {
    PrepareQueryOnUIThread();
    PostQueryTaskToBlockPool(&QueryInfo);  // Probably inline this call.
  }
}



Greg's likely right that this whole structure is premature optimization. The
reason to have a singleton around for system info querying is as a cache to
avoid redundant computations if multiple extensions request the information
between when it changes. However, I don't know that we've seen evidence that
this is a problem, and there's probably a cleaner and more efficient way to
cache things than this class.

[Haojian Wu, 2013/07/10 01:33:27]

In this way, we still need to virtual InitializeProvider, so in
StorageInfoProvider, we have

class StorageInfoProvider {
 ...
 virtual PrepareQueryOnUIThread() { GetAllStoragesIntoInfoList(); }
 virtual InitializeProvider(callback) {
   StorageMonitor::Get()->EnsureInitialized(callback);
 }
}

right?

We can avoid posting QueryInfo task to the blocking pool in SystemInfoProvider
subclass through this way, but I still can not see too many improvements on this
change except rename StartQueryInfoImpl() to InitializeProvider(callback) and
add two virtual functions PrepareQueryOnUIThread and InitializeProvider. Is it
worth doing this?

[Jeffrey Yasskin, 2013/07/10 01:57:42]

Yes, exactly.
The main improvements from this change are that the constraints on the subclass
implementations of the virtual functions get much easier to describe, and you
don't need to duplicate any code in the subclass implementations. As a side
improvement, you can then make PostQueryTaskToBlockingPool private, which lets
you refactor it in you want. The downside would be that the total number of
virtual functions goes up by 1, but I think that's worth it in order to simplify
their specification.

[Haojian Wu, 2013/07/10 03:11:37]

Done.

+  // StorageMonitor::GetAllAvailableStorages to get the correct results.
+  QueryInfo();
+
+  PostQueryTaskToBlockingPool(
       base::Bind(&StorageInfoProvider::QueryAvailableCapacityOnBlockingPool,
-          this));
+                 this));
 }
 
-void StorageInfoProvider::QueryAvailableCapacityOnBlockingPool() {
+bool StorageInfoProvider::QueryAvailableCapacityOnBlockingPool() {
   DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());
   for (StorageInfo::iterator it = info_.begin(); it != info_.end(); ++it) {
     int64 amount = GetStorageFreeSpaceFromTransientId((*it)->id);
@@ -78,16 +83,14 @@ void StorageInfoProvider::QueryAvailableCapacityOnBlockingPool() {
       (*it)->available_capacity = static_cast<double>(amount);
   }
 
-  // Notify UI thread that the querying operation has completed.
-  BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
-      base::Bind(&StorageInfoProvider::OnQueryCompleted, this, true));
+  return true;
 }
 
 std::vector<chrome::StorageInfo> StorageInfoProvider::GetAllStorages() const {
   return StorageMonitor::GetInstance()->GetAllAvailableStorages();
 }
 
-bool StorageInfoProvider::QueryInfo(StorageInfo* info) {
+bool StorageInfoProvider::QueryInfo() {
   std::vector<chrome::StorageInfo> storage_list = GetAllStorages();

[Greg Billock, 2013/07/08 18:05:06]

These two type names are confusing. Could we call our own StorageInfo something
like StorageUnitInfoList?

[Haojian Wu, 2013/07/09 13:45:04]

Done.

   StorageInfo results;
   std::vector<chrome::StorageInfo>::const_iterator it = storage_list.begin();
@@ -96,8 +99,7 @@ bool StorageInfoProvider::QueryInfo(StorageInfo* info) {
     systeminfo::BuildStorageUnitInfo(*it, unit.get());
     results.push_back(unit);
   }
-  info->swap(results);
-
+  info_.swap(results);

[Greg Billock, 2013/07/08 18:05:06]

Why not add these directly into info_ ? Is there an atomicity thing going on
here? We're on the UI thread already.

[Haojian Wu, 2013/07/09 13:45:04]

Done. My original thought is that we should keep minimal changes to the code.

   return true;
 }