[SystemInfo API] Finish TODOs in SystemInfoProvider

[SystemInfo API] Finish TODOs in SystemInfoProvider

* Remove parameter T-type in SystemInfoProvider::QueryInfo method.
* Use PostBlockingPoolTaskAndReply to avoid unnecessary trampolines trip
  on UI thread and blocking pool.
* Remove parameter T-type in QueryInfoCompletionCallback function.

BUG=None
TEST=unit_tests --gtest_filter=*InfoProviderTest.*
TEST=browser_tests --gtest_filter=SystemInfo*.*

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=211087

[Haojian Wu, 2013-06-30 09:03:00]

This CL should get landed after "Rewrite StorageInfoProvider" CL:
https://codereview.chromium.org/16707002/.

I have decoupled the template T type and SystemInfoProvider<T> class. After this
CL gets landed, it can easy to abstract a non-templated SystemInfoProviderBase
class and avoid using template.

hongbo@ please review this patch. Thanks.

[Hongbo Min, 2013-07-01 10:23:40]

Haojian, Good work. It is a good practice to do such work and I can review your
major idea as early as possible, which could absolutely accelerate the progress!

https://codereview.chromium.org/18290002/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/system_info/system_info_provider.h (right):

https://codereview.chromium.org/18290002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/system_info/system_info_provider.h:100: success_ =
false;
why to keep a member for success flag instead of the return value of QueryInfo? 

QueryInfo returning a boolean type seems to be a bit reasonable.

https://codereview.chromium.org/18290002/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(right):

https://codereview.chromium.org/18290002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:53:
const StorageInfo* StorageInfoProvider::storage_info() const {
Is it possible to return a const reference to StorageInfo?

[Haojian Wu, 2013-07-01 15:16:36]

https://codereview.chromium.org/18290002/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/system_info/system_info_provider.h (right):

https://codereview.chromium.org/18290002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/system_info/system_info_provider.h:100: success_ =
false;
On 2013/07/01 10:23:41, Hongbo Min wrote:
> why to keep a member for success flag instead of the return value of
QueryInfo? 
> 
> QueryInfo returning a boolean type seems to be a bit reasonable.

Since we are using PostBlockingPoolTaskAndReply function (see
system_info_provider.h line 80). The function second and third parameters' type
are base::Closure which actually is Callback<void(void)>. If QueryInfo returns a
boolean type, then will cause a failed compile.

https://codereview.chromium.org/18290002/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(right):

https://codereview.chromium.org/18290002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:53:
const StorageInfo* StorageInfoProvider::storage_info() const {
On 2013/07/01 10:23:41, Hongbo Min wrote:
> Is it possible to return a const reference to StorageInfo?

I think so. So you prefer to reference rather than pointer?

[Hongbo Min, 2013-07-02 00:57:58]

https://codereview.chromium.org/18290002/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/system_info/system_info_provider.h (right):

https://codereview.chromium.org/18290002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/system_info/system_info_provider.h:100: success_ =
false;
On 2013/07/01 15:16:42, Haojian Wu wrote:
> On 2013/07/01 10:23:41, Hongbo Min wrote:
> > why to keep a member for success flag instead of the return value of
> QueryInfo? 
> > 
> > QueryInfo returning a boolean type seems to be a bit reasonable.
> 
> Since we are using PostBlockingPoolTaskAndReply function (see
> system_info_provider.h line 80). The function second and third parameters'
type
> are base::Closure which actually is Callback<void(void)>. If QueryInfo returns
a
> boolean type, then will cause a failed compile.

If that is the only reason to let QueryInfo return void, I suggest that you
could try to base::PostTaskAndReplyWithResult instead. The pattern to use it is:

scoped_refptr<base::SequencedWorkerPool> pool =
    content::BrowserThread::GetBlockingPool();
blocking_task_runner_ = pool->GetSequencedTaskRunner(pool->GetSequenceToken());
base::PostTaskAndReplyWithResult(
    blocking_task_runner_,
    FROM_HERE,
    base::Bind(&SystemInfoProvider<T>::QueryInfo, this),
    base::Bind(&SystemInfoProvider<T>::OnQueryCompleted, this));

Note the input parameter of OnQueryCompleted should be the return value of
QueryInfo.

https://codereview.chromium.org/18290002/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(right):

https://codereview.chromium.org/18290002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:53:
const StorageInfo* StorageInfoProvider::storage_info() const {
On 2013/07/01 15:16:42, Haojian Wu wrote:
> On 2013/07/01 10:23:41, Hongbo Min wrote:
> > Is it possible to return a const reference to StorageInfo?
> 
> I think so. So you prefer to reference rather than pointer?

Yes.

[Haojian Wu, 2013-07-05 03:01:33]

This patch has been updated according to the comments of hongbo@ and Jeffrey@.
Please review it again.

https://codereview.chromium.org/18290002/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/system_info/system_info_provider.h (right):

https://codereview.chromium.org/18290002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/system_info/system_info_provider.h:100: success_ =
false;
On 2013/07/02 00:57:58, Hongbo Min wrote:
> On 2013/07/01 15:16:42, Haojian Wu wrote:
> > On 2013/07/01 10:23:41, Hongbo Min wrote:
> > > why to keep a member for success flag instead of the return value of
> > QueryInfo? 
> > > 
> > > QueryInfo returning a boolean type seems to be a bit reasonable.
> > 
> > Since we are using PostBlockingPoolTaskAndReply function (see
> > system_info_provider.h line 80). The function second and third parameters'
> type
> > are base::Closure which actually is Callback<void(void)>. If QueryInfo
returns
> a
> > boolean type, then will cause a failed compile.
> 
> If that is the only reason to let QueryInfo return void, I suggest that you
> could try to base::PostTaskAndReplyWithResult instead. The pattern to use it
is:
> 
> scoped_refptr<base::SequencedWorkerPool> pool =
>     content::BrowserThread::GetBlockingPool();
> blocking_task_runner_ =
pool->GetSequencedTaskRunner(pool->GetSequenceToken());
> base::PostTaskAndReplyWithResult(
>     blocking_task_runner_,
>     FROM_HERE,
>     base::Bind(&SystemInfoProvider<T>::QueryInfo, this),
>     base::Bind(&SystemInfoProvider<T>::OnQueryCompleted, this));
> 
> Note the input parameter of OnQueryCompleted should be the return value of
> QueryInfo.
>     
> 

Done.

https://codereview.chromium.org/18290002/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(right):

https://codereview.chromium.org/18290002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:53:
const StorageInfo* StorageInfoProvider::storage_info() const {
On 2013/07/02 00:57:58, Hongbo Min wrote:
> On 2013/07/01 15:16:42, Haojian Wu wrote:
> > On 2013/07/01 10:23:41, Hongbo Min wrote:
> > > Is it possible to return a const reference to StorageInfo?
> > 
> > I think so. So you prefer to reference rather than pointer?
> 
> Yes. 

Done.

[Hongbo Min, 2013-07-05 05:01:34]

https://codereview.chromium.org/18290002/diff/7001/chrome/browser/extensions/...
File chrome/browser/extensions/api/system_info/system_info_provider.h (right):

https://codereview.chromium.org/18290002/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info/system_info_provider.h:75:
StartQueryInfoPostInitialization();
What is the major change except for just renaming StartQueryInfoImpl to 
StartQueryInfoPostInitialization, and PostQueryTaskToBlockingPool ->
StartQueryInfoImpl?

Actually, I didn't see the improvement we got.

https://codereview.chromium.org/18290002/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info/system_info_provider.h:103: // Query
the system information synchronously and the result is info_.
"Query the system information and put the result into |info_|. Return true if no
error occurs."

https://codereview.chromium.org/18290002/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info/system_info_provider.h:135: 
nits: no need to extra blank line.

https://codereview.chromium.org/18290002/diff/7001/chrome/browser/extensions/...
File
chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc
(right):

https://codereview.chromium.org/18290002/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc:418:
base::Bind(callback, info_, success_));
nits: sucess_ should be already removed, use the return value of QueryInfo
instead. Otherwise, it would fail to build for ChromeOS

https://codereview.chromium.org/18290002/diff/7001/chrome/browser/extensions/...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(right):

https://codereview.chromium.org/18290002/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:70:
// StorageMonitor::GetAllAvailableStorages to get the correct results.
nit: GetAllAvailableStorages->GetAllStorages

https://codereview.chromium.org/18290002/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:102:
StorageInfo* info = &info_;
Directly use info_.swap(results) instead.

[Haojian Wu, 2013-07-06 05:31:37]

https://codereview.chromium.org/18290002/diff/7001/chrome/browser/extensions/...
File chrome/browser/extensions/api/system_info/system_info_provider.h (right):

https://codereview.chromium.org/18290002/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info/system_info_provider.h:75:
StartQueryInfoPostInitialization();
On 2013/07/05 05:01:34, Hongbo Min wrote:
> What is the major change except for just renaming StartQueryInfoImpl to 
> StartQueryInfoPostInitialization, and PostQueryTaskToBlockingPool ->
> StartQueryInfoImpl?
> 
> Actually, I didn't see the improvement we got.

In this way we can make OnQueryCompleted method private so that the subclass
StorageInfoProvider has no dependency on it.

Also, StorageInfoProvider can reuse the related "PostTaskAndReplyWithResult"
code (line 89~100 on this file) in SystemInfoProvider. In StorageInfoProvider,
we just call StartQueryInfoImpl method to post our custom callback task to the
blocking pool rather than rewrite the "PostTaskAndReplyWithResult" code again.

https://codereview.chromium.org/18290002/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info/system_info_provider.h:103: // Query
the system information synchronously and the result is info_.
On 2013/07/05 05:01:34, Hongbo Min wrote:
> "Query the system information and put the result into |info_|. Return true if
no
> error occurs."

Done.

https://codereview.chromium.org/18290002/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info/system_info_provider.h:135: 
On 2013/07/05 05:01:34, Hongbo Min wrote:
> nits: no need to extra blank line.

Done.

https://codereview.chromium.org/18290002/diff/7001/chrome/browser/extensions/...
File
chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc
(right):

https://codereview.chromium.org/18290002/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_display/display_info_provider_chromeos.cc:418:
base::Bind(callback, info_, success_));
On 2013/07/05 05:01:34, Hongbo Min wrote:
> nits: sucess_ should be already removed, use the return value of QueryInfo
> instead. Otherwise, it would fail to build for ChromeOS

Done.

https://codereview.chromium.org/18290002/diff/7001/chrome/browser/extensions/...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(right):

https://codereview.chromium.org/18290002/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:70:
// StorageMonitor::GetAllAvailableStorages to get the correct results.
On 2013/07/05 05:01:34, Hongbo Min wrote:
> nit: GetAllAvailableStorages->GetAllStorages

Here GetAllStorage is a StorageInfoProvider method(see line 90 on this file)
which will invoke StorageMonitor::GetAllAvailableStorages. So I think we should
keep this.

https://codereview.chromium.org/18290002/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:102:
StorageInfo* info = &info_;
On 2013/07/05 05:01:34, Hongbo Min wrote:
> Directly use info_.swap(results) instead.

Done.

[Hongbo Min, 2013-07-06 14:26:35]

Thanks. LGTM with one nit to be addressed.

https://codereview.chromium.org/18290002/diff/7001/chrome/browser/extensions/...
File chrome/browser/extensions/api/system_info/system_info_provider.h (right):

https://codereview.chromium.org/18290002/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info/system_info_provider.h:75:
StartQueryInfoPostInitialization();
On 2013/07/06 05:31:37, Haojian Wu wrote:
> On 2013/07/05 05:01:34, Hongbo Min wrote:
> > What is the major change except for just renaming StartQueryInfoImpl to 
> > StartQueryInfoPostInitialization, and PostQueryTaskToBlockingPool ->
> > StartQueryInfoImpl?
> > 
> > Actually, I didn't see the improvement we got.
> 
> In this way we can make OnQueryCompleted method private so that the subclass
> StorageInfoProvider has no dependency on it.
> 
> Also, StorageInfoProvider can reuse the related "PostTaskAndReplyWithResult"
> code (line 89~100 on this file) in SystemInfoProvider. In StorageInfoProvider,
> we just call StartQueryInfoImpl method to post our custom callback task to the
> blocking pool rather than rewrite the "PostTaskAndReplyWithResult" code again.

OK. That's an improvement. But I still feel not comfortable about the names. How
about changingStartQueryInfoPostInitialization->StartQueryInfoImp, and
StartQueryInfoImpl->PostQueryTaskToBlockingPool since they are more close to
their real functionalities? e.g. in the current impl, StartQueryInfoImpl is
doing a posting work, right?

https://codereview.chromium.org/18290002/diff/7001/chrome/browser/extensions/...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(right):

https://codereview.chromium.org/18290002/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:70:
// StorageMonitor::GetAllAvailableStorages to get the correct results.
On 2013/07/06 05:31:37, Haojian Wu wrote:
> On 2013/07/05 05:01:34, Hongbo Min wrote:
> > nit: GetAllAvailableStorages->GetAllStorages
> 
> Here GetAllStorage is a StorageInfoProvider method(see line 90 on this file)
> which will invoke StorageMonitor::GetAllAvailableStorages. So I think we
should
> keep this.

I see now. Thanks.

[Haojian Wu, 2013-07-08 09:26:35]

ping Greg@.

https://codereview.chromium.org/18290002/diff/7001/chrome/browser/extensions/...
File chrome/browser/extensions/api/system_info/system_info_provider.h (right):

https://codereview.chromium.org/18290002/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info/system_info_provider.h:75:
StartQueryInfoPostInitialization();
On 2013/07/06 14:26:35, Hongbo Min wrote:
> On 2013/07/06 05:31:37, Haojian Wu wrote:
> > On 2013/07/05 05:01:34, Hongbo Min wrote:
> > > What is the major change except for just renaming StartQueryInfoImpl to 
> > > StartQueryInfoPostInitialization, and PostQueryTaskToBlockingPool ->
> > > StartQueryInfoImpl?
> > > 
> > > Actually, I didn't see the improvement we got.
> > 
> > In this way we can make OnQueryCompleted method private so that the subclass
> > StorageInfoProvider has no dependency on it.
> > 
> > Also, StorageInfoProvider can reuse the related "PostTaskAndReplyWithResult"
> > code (line 89~100 on this file) in SystemInfoProvider. In
StorageInfoProvider,
> > we just call StartQueryInfoImpl method to post our custom callback task to
the
> > blocking pool rather than rewrite the "PostTaskAndReplyWithResult" code
again.
> 
> OK. That's an improvement. But I still feel not comfortable about the names.
How
> about changingStartQueryInfoPostInitialization->StartQueryInfoImp, and
> StartQueryInfoImpl->PostQueryTaskToBlockingPool since they are more close to
> their real functionalities? e.g. in the current impl, StartQueryInfoImpl is
> doing a posting work, right?
> 

Done.

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

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info/system_info_api.cc (right):

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_api.cc:130: void
SystemInfoEventRouter::StartWatchingStorages(bool success) {
It looks like this should just forward the call directly to the
StorageInfoProvider and let it iterate its current list of storages.

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_api.cc:141: void
SystemInfoEventRouter::StopWatchingStorages(bool success) {
Same here.

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info/system_info_provider.h (right):

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_provider.h:43: // Callback
type for completing to get information. The callback accepts
Comment is now stale.

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_provider.h:79: // We can
override this method with our own custom query info Callback
I think you can just get rid of this comment.

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_provider.h:82:
base::Callback<bool()> query_info_callback =
You can put this in the PostQueryTaskToBlockingPool call directly and avoid the
temporary.

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_provider.h:87: // Post the
query info task to blocking pool.
Your method name says this. Delete comment or replace with one indicating that
OnQueryCompleted will be the next thing we see called.

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_provider.h:120: T info_;
This change is most of the way to de-templating the design. |info_| is basically
no longer used here, and there are direct accessors on subclasses. Lets get this
in and hold off on separating out the templating stuff. I think we'll be able to
nicely use subclasses after this.

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_provider.h:154:
SystemInfoProvider<T>::single_shared_provider_ = LAZY_INSTANCE_INITIALIZER;
This name makes it sound like there's only one provider, but really there's one
for each type. And its a LazyInstance already, and they have public GetInstance
methods, so how about just calling it "provider_".

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(right):

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:61:
// StorageMonitor::Initialize about when the callback gets run.
EnsureInitialized. And I think it would be good to remove details here about
what the StorageMonitor does internally. You're already pointing readers to that
file, which I think is the right thing to do.

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:94:
std::vector<chrome::StorageInfo> storage_list = GetAllStorages();
These two type names are confusing. Could we call our own StorageInfo something
like StorageUnitInfoList?

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:102:
info_.swap(results);
Why not add these directly into info_ ? Is there an atomicity thing going on
here? We're on the UI thread already.

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

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info/system_info_provider.h (right):

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_provider.h:79: // We can
override this method with our own custom query info Callback
On 2013/07/08 18:05:06, Greg Billock wrote:
> I think you can just get rid of this comment.

If this function stays virtual, it does need a comment saying what guarantees an
overriding implementation has to provide. I agree that this comment isn't enough
to know how to override the function.

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_provider.h:93:
pool->GetSequenceToken(),
Although this is one reading of what I suggested, it's not the reading that
preserves the sequencing behavior in your original code. Can you read the
documentation
(https://code.google.com/p/chromium/codesearch/#chromium/src/base/threading/se...
and
https://code.google.com/p/chromium/codesearch/#chromium/src/base/threading/se...)
and modify this so it does guarantee that all tasks are executed in sequence?

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_provider.h:134:
is_waiting_for_completion_ = false;
This is a bit subtle. Could you add a comment to StartQueryInfo(callback) saying
that if the callback itself calls StartQueryInfo(callback2), callback2 will be
called immediately rather than triggering another call to the system?

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_provider.h:138: static
typename base::LazyInstance<
You can't use static members of templates because Windows creates a separate
instance for each DLL the header appears in. You'll need each subclass to define
this in its .cc file.

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
File
chrome/browser/extensions/api/system_info_display/display_info_provider_unittest.cc
(right):

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_display/display_info_provider_unittest.cc:89:
const DisplayInfo& display_info = display_info_provider_->display_info();
FWIW, this doesn't test anything that runs in the production system except the
type definitions of DisplayInfo and api::system_info_display::DisplayUnitInfo.
You've overridden QueryInfo in the test fixture, and that's all you're calling
here, so no production code runs.

You didn't make this worse in this CL, but I wanted to point it out so that Greg
doesn't think these APIs are ready to escape experimental before it's fixed.

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(right):

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:69:
// QueryInfo can be called on UI thread since in the implementation we use
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]

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info/system_info_api.cc (right):

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_api.cc:130: void
SystemInfoEventRouter::StartWatchingStorages(bool success) {
On 2013/07/08 18:05:06, Greg Billock wrote:
> It looks like this should just forward the call directly to the
> StorageInfoProvider and let it iterate its current list of storages.

Done.

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_api.cc:141: void
SystemInfoEventRouter::StopWatchingStorages(bool success) {
On 2013/07/08 18:05:06, Greg Billock wrote:
> Same here.

Done.

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info/system_info_provider.h (right):

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_provider.h:43: // Callback
type for completing to get information. The callback accepts
On 2013/07/08 18:05:06, Greg Billock wrote:
> Comment is now stale.

Done.

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_provider.h:79: // We can
override this method with our own custom query info Callback
On 2013/07/08 19:09:49, Jeffrey Yasskin wrote:
> On 2013/07/08 18:05:06, Greg Billock wrote:
> > I think you can just get rid of this comment.
> 
> If this function stays virtual, it does need a comment saying what guarantees
an
> overriding implementation has to provide. I agree that this comment isn't
enough
> to know how to override the function.

Done. Update the comments.

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_provider.h:82:
base::Callback<bool()> query_info_callback =
On 2013/07/08 18:05:06, Greg Billock wrote:
> You can put this in the PostQueryTaskToBlockingPool call directly and avoid
the
> temporary.

Done.

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_provider.h:87: // Post the
query info task to blocking pool.
On 2013/07/08 18:05:06, Greg Billock wrote:
> Your method name says this. Delete comment or replace with one indicating that
> OnQueryCompleted will be the next thing we see called.

Done.

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_provider.h:93:
pool->GetSequenceToken(),
On 2013/07/08 19:09:49, Jeffrey Yasskin wrote:
> Although this is one reading of what I suggested, it's not the reading that
> preserves the sequencing behavior in your original code. Can you read the
> documentation
>
(https://code.google.com/p/chromium/codesearch/#chromium/src/base/threading/se...
> and
>
https://code.google.com/p/chromium/codesearch/#chromium/src/base/threading/se...)
> and modify this so it does guarantee that all tasks are executed in sequence?

Done.

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_provider.h:120: T info_;
On 2013/07/08 18:05:06, Greg Billock wrote:
> This change is most of the way to de-templating the design. |info_| is
basically
> no longer used here, and there are direct accessors on subclasses. Lets get
this
> in and hold off on separating out the templating stuff. I think we'll be able
to
> nicely use subclasses after this.

Yes, I think so. I will separate a new CL to do it after this patch gets landed
since it also requires a big change of SystemInfo API.

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_provider.h:134:
is_waiting_for_completion_ = false;
On 2013/07/08 19:09:49, Jeffrey Yasskin wrote:
> This is a bit subtle. Could you add a comment to StartQueryInfo(callback)
saying
> that if the callback itself calls StartQueryInfo(callback2), callback2 will be
> called immediately rather than triggering another call to the system?

Done.

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_provider.h:138: static
typename base::LazyInstance<
On 2013/07/08 19:09:49, Jeffrey Yasskin wrote:
> You can't use static members of templates because Windows creates a separate
> instance for each DLL the header appears in. You'll need each subclass to
define
> this in its .cc file.

Done.

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_provider.h:154:
SystemInfoProvider<T>::single_shared_provider_ = LAZY_INSTANCE_INITIALIZER;
On 2013/07/08 18:05:06, Greg Billock wrote:
> This name makes it sound like there's only one provider, but really there's
one
> for each type. And its a LazyInstance already, and they have public
GetInstance
> methods, so how about just calling it "provider_".

Done.

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
File
chrome/browser/extensions/api/system_info_display/display_info_provider_unittest.cc
(right):

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_display/display_info_provider_unittest.cc:89:
const DisplayInfo& display_info = display_info_provider_->display_info();
On 2013/07/08 19:09:49, Jeffrey Yasskin wrote:
> FWIW, this doesn't test anything that runs in the production system except the
> type definitions of DisplayInfo and api::system_info_display::DisplayUnitInfo.
> You've overridden QueryInfo in the test fixture, and that's all you're calling
> here, so no production code runs.
> 
> You didn't make this worse in this CL, but I wanted to point it out so that
Greg
> doesn't think these APIs are ready to escape experimental before it's fixed.

Your suggestion reminds me that the unit_tests of CpuInfoProvider,
MemoryInfoProvider, DisplayInfoProvider
are the same, just overriding the QueryInfo. We need to improve the way of
testing SystemInfo API, especially StorageInfo api. 

About the unit_tests here, as you said, Are they unnecessary? Should we remove
them?

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(right):

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:61:
// StorageMonitor::Initialize about when the callback gets run.
On 2013/07/08 18:05:06, Greg Billock wrote:
> EnsureInitialized. And I think it would be good to remove details here about
> what the StorageMonitor does internally. You're already pointing readers to
that
> file, which I think is the right thing to do.

Done.

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:69:
// QueryInfo can be called on UI thread since in the implementation we use
On 2013/07/08 19:09:49, Jeffrey Yasskin wrote:
> 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.

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.

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:94:
std::vector<chrome::StorageInfo> storage_list = GetAllStorages();
On 2013/07/08 18:05:06, Greg Billock wrote:
> These two type names are confusing. Could we call our own StorageInfo
something
> like StorageUnitInfoList?

Done.

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:102:
info_.swap(results);
On 2013/07/08 18:05:06, Greg Billock wrote:
> Why not add these directly into info_ ? Is there an atomicity thing going on
> here? We're on the UI thread already.

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

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

Perhaps I should have put my code direction comment here since it's a fairly
large change.

Hongbo, if there's a reason we should avoid a more ExtensionFunction-oriented
model for this code, and keep singletons around, let us know.

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(right):

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:69:
// QueryInfo can be called on UI thread since in the implementation we use
On 2013/07/09 13:45:04, Haojian Wu wrote:
> On 2013/07/08 19:09:49, Jeffrey Yasskin wrote:
> > 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.
> 
> 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.

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]

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
File
chrome/browser/extensions/api/system_info_display/display_info_provider_unittest.cc
(right):

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_display/display_info_provider_unittest.cc:89:
const DisplayInfo& display_info = display_info_provider_->display_info();
On 2013/07/09 13:45:04, Haojian Wu wrote:
> On 2013/07/08 19:09:49, Jeffrey Yasskin wrote:
> > FWIW, this doesn't test anything that runs in the production system except
the
> > type definitions of DisplayInfo and
api::system_info_display::DisplayUnitInfo.
> > You've overridden QueryInfo in the test fixture, and that's all you're
calling
> > here, so no production code runs.
> > 
> > You didn't make this worse in this CL, but I wanted to point it out so that
> Greg
> > doesn't think these APIs are ready to escape experimental before it's fixed.
> 
> Your suggestion reminds me that the unit_tests of CpuInfoProvider,
> MemoryInfoProvider, DisplayInfoProvider
> are the same, just overriding the QueryInfo. We need to improve the way of
> testing SystemInfo API, especially StorageInfo api. 
> 
> About the unit_tests here, as you said, Are they unnecessary? Should we remove
> them?

Yeah, we should remove them and replace them (likely in a future CL) with
something that actually tests the production code. It's possible that these
classes are too thin to be worth testing in a unit test, or that it's extremely
hard to mock the underlying system results, in which case you should just use
the apitest to make sure that the functions exist all the way down and return
results that match what more direct calls return.

By the way, this is why I don't like putting virtual methods in the class I'm
trying to test
(https://codereview.chromium.org/16707002/diff/123001/chrome/browser/extension...),
and especially not overriding them inside the test. It's way too easy to
accidentally wind up testing nothing.

I prefer to "design for testing" by designing pure virtual interfaces (or
classes with some pure virtual functions and other non-virtual helper
functions), and then passing mock or fake instances of those classes to the
constructor of the class I'm trying to test. It's still possible to wind up
testing a very thin function with a mock instance under it, but then it's much
more obvious when looking at the production code that this class or function
isn't doing anything.

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(right):

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:69:
// QueryInfo can be called on UI thread since in the implementation we use
On 2013/07/09 13:45:04, Haojian Wu wrote:
> On 2013/07/08 19:09:49, Jeffrey Yasskin wrote:
> > 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.
> 
> 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().
>   }

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.

https://codereview.chromium.org/18290002/diff/53001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info/system_info_provider.h (right):

https://codereview.chromium.org/18290002/diff/53001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_provider.h:52:
content::BrowserThread::GetBlockingPool()->GetSequenceToken();
Change worker_pool_token_ to be declared as
"scoped_refptr<base::SequencedTaskRunner> worker_pool_;" (still a member
variable), and initialize it as 

pool->GetSequencedTaskRunnerWithShutdownBehavior(pool->GetSequenceToken(),
base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN);

Then remove the extra SequencedTaskRunner creation in
PostQueryTaskToBlockingPool, and just use the member variable directly.

[Haojian Wu, 2013-07-10 01:33:26]

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(right):

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:69:
// QueryInfo can be called on UI thread since in the implementation we use
On 2013/07/09 18:19:59, Jeffrey Yasskin wrote:
> On 2013/07/09 13:45:04, Haojian Wu wrote:
> > On 2013/07/08 19:09:49, Jeffrey Yasskin wrote:
> > > 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.
> > 
> > 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().
> >   }
> 
> 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.
>   }
> }
> 

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:41]

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(right):

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:69:
// QueryInfo can be called on UI thread since in the implementation we use
On 2013/07/10 01:33:27, Haojian Wu wrote:
> 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?

Yes, exactly.

> 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?

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]

Next step is to improve the test of SystemInfoProvider(In another CL).

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc
(right):

https://codereview.chromium.org/18290002/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info_storage/storage_info_provider.cc:69:
// QueryInfo can be called on UI thread since in the implementation we use
On 2013/07/10 01:57:42, Jeffrey Yasskin wrote:
> On 2013/07/10 01:33:27, Haojian Wu wrote:
> > 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?
> 
> Yes, exactly.
> 
> > 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?
> 
> 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.

Done.

https://codereview.chromium.org/18290002/diff/53001/chrome/browser/extensions...
File chrome/browser/extensions/api/system_info/system_info_provider.h (right):

https://codereview.chromium.org/18290002/diff/53001/chrome/browser/extensions...
chrome/browser/extensions/api/system_info/system_info_provider.h:52:
content::BrowserThread::GetBlockingPool()->GetSequenceToken();
On 2013/07/09 18:19:59, Jeffrey Yasskin wrote:
> Change worker_pool_token_ to be declared as
> "scoped_refptr<base::SequencedTaskRunner> worker_pool_;" (still a member
> variable), and initialize it as 
> 
> pool->GetSequencedTaskRunnerWithShutdownBehavior(pool->GetSequenceToken(),
> base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN);
> 
> Then remove the extra SequencedTaskRunner creation in
> PostQueryTaskToBlockingPool, and just use the member variable directly.

Done.

[Jeffrey Yasskin, 2013-07-10 17:05:25]

lgtm; thanks for bearing with me.

[Greg Billock, 2013-07-10 18:20:43]

lgtm

[commit-bot: I haz the power, 2013-07-10 23:05:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/Hokein.Wu@gmail.com/18290002/63001

[commit-bot: I haz the power, 2013-07-10 23:59:46]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
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.

[commit-bot: I haz the power, 2013-07-11 00:20:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/Hokein.Wu@gmail.com/18290002/96001

[commit-bot: I haz the power, 2013-07-11 00:58:15]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
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.

[commit-bot: I haz the power, 2013-07-11 01:48:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/Hokein.Wu@gmail.com/18290002/114001

[commit-bot: I haz the power, 2013-07-11 02:24:28]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
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.

[commit-bot: I haz the power, 2013-07-11 05:55:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/Hokein.Wu@gmail.com/18290002/117003

[commit-bot: I haz the power, 2013-07-11 07:03:07]

Retried try job too often on linux_chromeos for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-07-11 07:58:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/Hokein.Wu@gmail.com/18290002/142001

[commit-bot: I haz the power, 2013-07-11 11:57:52]

Change committed as 211087