Add systemInfo.cpu.onUpdated event implementation.

Add systemInfo.cpu.onUpdated event implementation (re-committed it again since the static initializer regression issue was resolved. See http://code.google.com/p/chromium/issues/detail?id=155472)

BUG=136519
TEST=browser_tests --gtest_filter=SystemInfoCpuApiTest.*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=161499

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=161742

[Hongbo Min, 2012-09-11 11:29:43]

Pls have a reivew.

[Hongbo Min, 2012-09-13 00:00:59]

mihai, could you please review it?

[Mihai Parparita -not on Chrome, 2012-09-16 06:23:05]

http://codereview.chromium.org/10905171/diff/4001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.cc (right):

http://codereview.chromium.org/10905171/diff/4001/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.cc:26:
DCHECK(!sampling_timer_) << "Fogot to call StopSampling?";
"StartSampling call without a corresponding StopSampling call" seems like a more
helpful message.

http://codereview.chromium.org/10905171/diff/4001/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.cc:26:
DCHECK(!sampling_timer_) << "Fogot to call StopSampling?";
Also, this will still leak the sampling timer in non-debug builds.

You may instead want to make sampling_timer_ into a scoped_ptr, so that it'll
still get cleaned up in this case.

http://codereview.chromium.org/10905171/diff/4001/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.cc:65: this,
&CpuInfoProvider::DoStartSampling);
DoStartSampling seems like a misnomer, since it isn't called just when starting
to sample, but every sampling interval. Just "DoSample" perhaps?

http://codereview.chromium.org/10905171/diff/4001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.h (right):

http://codereview.chromium.org/10905171/diff/4001/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.h:32: //
returned. Can be called from any thread.
Allowing this to be called from any thread may mask bugs. Generally, all
extension function entry points are on the UI thread. It's better to check that
you're on the UI thread, and hand off execution to a StartSamplingOnFileThread
function (see
http://code.google.com/p/chromium/source/search?q=onfilethread+file%3Aextensions
for the pattern).

http://codereview.chromium.org/10905171/diff/4001/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.h:63: // Called
when the sampling timer is expired.
"expired" doesn't seem right. "fired" perhaps?

http://codereview.chromium.org/10905171/diff/4001/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.h:69: // The
sampling value returned from the most recent QueryCpuTimePerProcessor
This doesn't seem to be true. You set the variable when StartSampling is called,
but subsequent QueryCpuTimePerProcessor calls update a local variable only, and
compare that against the saved sampling_value_.  Call this start_cpu_time_ or
baseline_cpu_time_ instead?

http://codereview.chromium.org/10905171/diff/4001/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.h:76: // The
timer used for polling CPU time periodically. Live on File thread.
"file thread" or "FILE thread" are the more common ways of capitalizing.

http://codereview.chromium.org/10905171/diff/4001/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.h:79: // The
time interval for sampling.
Mention the time units.

http://codereview.chromium.org/10905171/diff/4001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/system_info_cpu/system_info_cpu_apitest.cc
(right):

http://codereview.chromium.org/10905171/diff/4001/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_cpu/system_info_cpu_apitest.cc:33:
private:
Add a newline before this.

http://codereview.chromium.org/10905171/diff/4001/chrome/browser/extensions/s...
File chrome/browser/extensions/system_info_event_router.cc (right):

http://codereview.chromium.org/10905171/diff/4001/chrome/browser/extensions/s...
chrome/browser/extensions/system_info_event_router.cc:45: if
(event_name.compare(
To make this code easier to read (and less repetitive with RemoveEventListener),
I would suggest adding IsStorageEvent and IsCpuEvent helper functions.

http://codereview.chromium.org/10905171/diff/4001/chrome/browser/extensions/s...
chrome/browser/extensions/system_info_event_router.cc:78:
CpuInfoProvider::Get()->StopSampling();
When quitting Chrome (or unloading an extension), event listeners are not
explicitly removed. I believe your DCHECK about StopSampling not being called
will fire. I think it's better to silently stop sampling in the destructor.

[Hongbo Min, 2012-09-16 14:07:49]

mihai, thanks for your review. The path is updated per your comments.

http://codereview.chromium.org/10905171/diff/4001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.h (right):

http://codereview.chromium.org/10905171/diff/4001/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.h:32: //
returned. Can be called from any thread.
On 2012/09/16 06:23:05, Mihai Parparita wrote:
> Allowing this to be called from any thread may mask bugs. Generally, all
> extension function entry points are on the UI thread. It's better to check
that
> you're on the UI thread, and hand off execution to a StartSamplingOnFileThread
> function (see
>
http://code.google.com/p/chromium/source/search?q=onfilethread+file%253Aexten...
> for the pattern).

Done.

http://codereview.chromium.org/10905171/diff/4001/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.h:63: // Called
when the sampling timer is expired.
On 2012/09/16 06:23:05, Mihai Parparita wrote:
> "expired" doesn't seem right. "fired" perhaps?

Done.

http://codereview.chromium.org/10905171/diff/4001/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.h:69: // The
sampling value returned from the most recent QueryCpuTimePerProcessor
On 2012/09/16 06:23:05, Mihai Parparita wrote:
> This doesn't seem to be true. You set the variable when StartSampling is
called,
> but subsequent QueryCpuTimePerProcessor calls update a local variable only,
and
> compare that against the saved sampling_value_.  Call this start_cpu_time_ or
> baseline_cpu_time_ instead?

Use baseline_cpu_time_. Done

http://codereview.chromium.org/10905171/diff/4001/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.h:76: // The
timer used for polling CPU time periodically. Live on File thread.
On 2012/09/16 06:23:05, Mihai Parparita wrote:
> "file thread" or "FILE thread" are the more common ways of capitalizing.

Done.

http://codereview.chromium.org/10905171/diff/4001/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.h:79: // The
time interval for sampling.
On 2012/09/16 06:23:05, Mihai Parparita wrote:
> Mention the time units.

Done.

http://codereview.chromium.org/10905171/diff/4001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/system_info_cpu/system_info_cpu_apitest.cc
(right):

http://codereview.chromium.org/10905171/diff/4001/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_cpu/system_info_cpu_apitest.cc:33:
private:
On 2012/09/16 06:23:05, Mihai Parparita wrote:
> Add a newline before this.

Done.

http://codereview.chromium.org/10905171/diff/4001/chrome/browser/extensions/s...
File chrome/browser/extensions/system_info_event_router.cc (right):

http://codereview.chromium.org/10905171/diff/4001/chrome/browser/extensions/s...
chrome/browser/extensions/system_info_event_router.cc:45: if
(event_name.compare(
On 2012/09/16 06:23:05, Mihai Parparita wrote:
> To make this code easier to read (and less repetitive with
RemoveEventListener),
> I would suggest adding IsStorageEvent and IsCpuEvent helper functions.

Done.

http://codereview.chromium.org/10905171/diff/4001/chrome/browser/extensions/s...
chrome/browser/extensions/system_info_event_router.cc:78:
CpuInfoProvider::Get()->StopSampling();
On 2012/09/16 06:23:05, Mihai Parparita wrote:
> When quitting Chrome (or unloading an extension), event listeners are not
> explicitly removed. I believe your DCHECK about StopSampling not being called
> will fire. I think it's better to silently stop sampling in the destructor.

Done.

[Hongbo Min, 2012-09-20 10:12:54]

benwells@

One more patch needs your review. Thanks.

[benwells, 2012-09-21 08:19:50]

http://codereview.chromium.org/10905171/diff/2002/chrome/browser/extensions/a...
File chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.cc (right):

http://codereview.chromium.org/10905171/diff/2002/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.cc:27:
BrowserThread::DeleteSoon(BrowserThread::FILE, FROM_HERE, sampling_timer_);
This feels racy. Can the timer fire before it is delted?

http://codereview.chromium.org/10905171/diff/2002/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.cc:55:
base::Unretained(this)));
Why use unretained here? If you descend from
RefCountedThreadSafe<CpuInfoProvider> you don't need to. As it is, it might be
racy if the CpuInfoProvider can be freed before the StopSamplingOnFileThread is
delivered.

http://codereview.chromium.org/10905171/diff/2002/chrome/browser/extensions/a...
File chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.h (right):

http://codereview.chromium.org/10905171/diff/2002/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.h:21:
SamplingCallback;
Sorry, but what is the void here for?

http://codereview.chromium.org/10905171/diff/2002/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.h:78: // The
timer used for polling CPU time periodically. Live on FILE thread.
Nit: s/Live/Lives/

http://codereview.chromium.org/10905171/diff/2002/chrome/browser/extensions/a...
File chrome/browser/extensions/api/system_info_cpu/system_info_cpu_apitest.cc
(right):

http://codereview.chromium.org/10905171/diff/2002/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_cpu/system_info_cpu_apitest.cc:36: if
(!times) return false;
This should be  DCHECK so your test can catch (and report nicely on) misuse of
this function.

http://codereview.chromium.org/10905171/diff/2002/chrome/browser/extensions/s...
File chrome/browser/extensions/system_info_event_router.cc (right):

http://codereview.chromium.org/10905171/diff/2002/chrome/browser/extensions/s...
chrome/browser/extensions/system_info_event_router.cc:68:
base::Unretained(this)));
Will sampling be stopped by the RemoveEventListener at shutdown?

http://codereview.chromium.org/10905171/diff/2002/chrome/browser/extensions/s...
File chrome/browser/extensions/system_info_event_router.h (right):

http://codereview.chromium.org/10905171/diff/2002/chrome/browser/extensions/s...
chrome/browser/extensions/system_info_event_router.h:17: namespace
experimental_system_info_cpu {
Nit: extra blank lines after each namespace statement and before closing
namespace brace.

http://codereview.chromium.org/10905171/diff/2002/chrome/browser/extensions/s...
chrome/browser/extensions/system_info_event_router.h:60: // The callback for CPU
sampling cycle. Can be called from any thread.
Is this just called from the FILE thread now?

http://codereview.chromium.org/10905171/diff/2002/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/systeminfo/cpu/test_cpu_api.js
(right):

http://codereview.chromium.org/10905171/diff/2002/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/systeminfo/cpu/test_cpu_api.js:27: function
testUpdatedEvent() {
Blank line above here.

http://codereview.chromium.org/10905171/diff/2002/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/systeminfo/cpu/test_cpu_api.js:31: function
listener(updateInfo) {
Indent this two block two more spaces.

http://codereview.chromium.org/10905171/diff/2002/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/systeminfo/cpu/test_cpu_api.js:35: for (var
i = 0; i < updateInfo.usagePerProcessor.length; ++i) {
Can drop these curly braces.

[Hongbo Min, 2012-09-21 09:00:23]

Minor changes. Thanks for your review.

A question is, for all SystemInfoProvider subclasses, is it necessary to make
them become thread-safe?

http://codereview.chromium.org/10905171/diff/2002/chrome/browser/extensions/a...
File chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.cc (right):

http://codereview.chromium.org/10905171/diff/2002/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.cc:27:
BrowserThread::DeleteSoon(BrowserThread::FILE, FROM_HERE, sampling_timer_);
On 2012/09/21 08:19:50, benwells wrote:
> This feels racy. Can the timer fire before it is delted?
Your concern is valid, but if the CpuInfoProvider is a global lazy instance and
will be destroyed at process exiting time, is it OK?

http://codereview.chromium.org/10905171/diff/2002/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.cc:55:
base::Unretained(this)));
On 2012/09/21 08:19:50, benwells wrote:
> Why use unretained here? If you descend from
> RefCountedThreadSafe<CpuInfoProvider> you don't need to. As it is, it might be
> racy if the CpuInfoProvider can be freed before the StopSamplingOnFileThread
is
> delivered.

The CpuInfoProvider is a global lazy instance and will be destroyed at process
exiting time.

http://codereview.chromium.org/10905171/diff/2002/chrome/browser/extensions/a...
File chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.h (right):

http://codereview.chromium.org/10905171/diff/2002/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.h:21:
SamplingCallback;
On 2012/09/21 08:19:50, benwells wrote:
> Sorry, but what is the void here for?

It is the callback signature, right? All SamplingCallback should have
void(scoped_ptr<CpuUpdateInfo> info) prototype.

http://codereview.chromium.org/10905171/diff/2002/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.h:78: // The
timer used for polling CPU time periodically. Live on FILE thread.
On 2012/09/21 08:19:50, benwells wrote:
> Nit: s/Live/Lives/

Done.

http://codereview.chromium.org/10905171/diff/2002/chrome/browser/extensions/a...
File chrome/browser/extensions/api/system_info_cpu/system_info_cpu_apitest.cc
(right):

http://codereview.chromium.org/10905171/diff/2002/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_cpu/system_info_cpu_apitest.cc:36: if
(!times) return false;
On 2012/09/21 08:19:50, benwells wrote:
> This should be  DCHECK so your test can catch (and report nicely on) misuse of
> this function.

Done.

http://codereview.chromium.org/10905171/diff/2002/chrome/browser/extensions/s...
File chrome/browser/extensions/system_info_event_router.cc (right):

http://codereview.chromium.org/10905171/diff/2002/chrome/browser/extensions/s...
chrome/browser/extensions/system_info_event_router.cc:68:
base::Unretained(this)));
On 2012/09/21 08:19:50, benwells wrote:
> Will sampling be stopped by the RemoveEventListener at shutdown?

Yes, but mihai mentioned that RemoveEventListener may not be called at shutdown.

http://codereview.chromium.org/10905171/diff/2002/chrome/browser/extensions/s...
File chrome/browser/extensions/system_info_event_router.h (right):

http://codereview.chromium.org/10905171/diff/2002/chrome/browser/extensions/s...
chrome/browser/extensions/system_info_event_router.h:17: namespace
experimental_system_info_cpu {
On 2012/09/21 08:19:50, benwells wrote:
> Nit: extra blank lines after each namespace statement and before closing
> namespace brace.

Done.

http://codereview.chromium.org/10905171/diff/2002/chrome/browser/extensions/s...
chrome/browser/extensions/system_info_event_router.h:60: // The callback for CPU
sampling cycle. Can be called from any thread.
On 2012/09/21 08:19:50, benwells wrote:
> Is this just called from the FILE thread now?

Yes.

http://codereview.chromium.org/10905171/diff/2002/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/systeminfo/cpu/test_cpu_api.js
(right):

http://codereview.chromium.org/10905171/diff/2002/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/systeminfo/cpu/test_cpu_api.js:27: function
testUpdatedEvent() {
On 2012/09/21 08:19:50, benwells wrote:
> Blank line above here.

Done.

http://codereview.chromium.org/10905171/diff/2002/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/systeminfo/cpu/test_cpu_api.js:31: function
listener(updateInfo) {
On 2012/09/21 08:19:50, benwells wrote:
> Indent this two block two more spaces.

Done.

http://codereview.chromium.org/10905171/diff/2002/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/systeminfo/cpu/test_cpu_api.js:35: for (var
i = 0; i < updateInfo.usagePerProcessor.length; ++i) {
On 2012/09/21 08:19:50, benwells wrote:
> Can drop these curly braces.

Done.

[benwells, 2012-09-25 17:32:13]

http://codereview.chromium.org/10905171/diff/2002/chrome/browser/extensions/a...
File chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.cc (right):

http://codereview.chromium.org/10905171/diff/2002/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.cc:27:
BrowserThread::DeleteSoon(BrowserThread::FILE, FROM_HERE, sampling_timer_);
On 2012/09/21 09:00:23, Hongbo Min wrote:
> On 2012/09/21 08:19:50, benwells wrote:
> > This feels racy. Can the timer fire before it is delted?
> Your concern is valid, but if the CpuInfoProvider is a global lazy instance
and
> will be destroyed at process exiting time, is it OK?

Lazy instances are destroyed at program exit. I don't know if the threads are
still handling tasks at this point or not. If they are, you have a problem as
the event could fire. If they are not, you have a problem as this won't be
freed.

Perhaps a better approach to this is to ensure, before program exit, that the
timer is stopped, and then log an error at shutdown if the timer is running. I'd
be more comfortable with that anyway, having timer events firing through
shutdown would be good to avoid.

http://codereview.chromium.org/10905171/diff/2002/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.cc:55:
base::Unretained(this)));
On 2012/09/21 09:00:23, Hongbo Min wrote:
> On 2012/09/21 08:19:50, benwells wrote:
> > Why use unretained here? If you descend from
> > RefCountedThreadSafe<CpuInfoProvider> you don't need to. As it is, it might
be
> > racy if the CpuInfoProvider can be freed before the StopSamplingOnFileThread
> is
> > delivered.
> 
> The CpuInfoProvider is a global lazy instance and will be destroyed at process
> exiting time.

OK, again if you ensure the timer is stopped when shutdown starts this should
avoid any problems here.

http://codereview.chromium.org/10905171/diff/2002/chrome/browser/extensions/a...
File chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.h (right):

http://codereview.chromium.org/10905171/diff/2002/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.h:21:
SamplingCallback;
On 2012/09/21 09:00:23, Hongbo Min wrote:
> On 2012/09/21 08:19:50, benwells wrote:
> > Sorry, but what is the void here for?
> 
> It is the callback signature, right? All SamplingCallback should have
> void(scoped_ptr<CpuUpdateInfo> info) prototype.

Yes, you're right.

http://codereview.chromium.org/10905171/diff/2002/chrome/browser/extensions/s...
File chrome/browser/extensions/system_info_event_router.h (right):

http://codereview.chromium.org/10905171/diff/2002/chrome/browser/extensions/s...
chrome/browser/extensions/system_info_event_router.h:60: // The callback for CPU
sampling cycle. Can be called from any thread.
On 2012/09/21 09:00:23, Hongbo Min wrote:
> On 2012/09/21 08:19:50, benwells wrote:
> > Is this just called from the FILE thread now?
> 
> Yes.

Can you updated the comment then?

[Hongbo Min, 2012-09-26 15:06:59]

> chrome/browser/extensions/system_info_event_router.h:60: // The callback for
CPU
> sampling cycle. Can be called from any thread.
> On 2012/09/21 09:00:23, Hongbo Min wrote:
> > On 2012/09/21 08:19:50, benwells wrote:
> > > Is this just called from the FILE thread now?
> > 
> > Yes.
> 
> Can you updated the comment then?

Done!

[Hongbo Min, 2012-09-26 15:07:32]

The patch is updated according to your comments, see the Patch Set 4. Thanks.

[benwells, 2012-09-27 00:30:52]

On 2012/09/26 15:07:32, Hongbo Min wrote:
> The patch is updated according to your comments, see the Patch Set 4. Thanks.

The shutdown behavior feels ill-defined. Have a look at
http://dev.chromium.org/developers/shutdown.

My understanding of what will happen in this case is that the timer will not be
stopped at shutdown, the message loops will stop, the provider will be freed, a
task will be posted to the file thread to free the timer, which won't ever
happen as the threads are stopped.

It would be better if during shutdown (perhaps in a APP_TERMINATING handler) the
timer is stopped, then while freeing if we still have a timer it is an error
condition and should be DCHECKED.

[Hongbo Min, 2012-10-03 13:47:52]

On 2012/09/27 00:30:52, benwells wrote:
> On 2012/09/26 15:07:32, Hongbo Min wrote:
> > The patch is updated according to your comments, see the Patch Set 4.
Thanks.
> 
> The shutdown behavior feels ill-defined. Have a look at
> http://dev.chromium.org/developers/shutdown.
> 
> My understanding of what will happen in this case is that the timer will not
be
> stopped at shutdown, the message loops will stop, the provider will be freed,
a
> task will be posted to the file thread to free the timer, which won't ever
> happen as the threads are stopped.
> 
> It would be better if during shutdown (perhaps in a APP_TERMINATING handler)
the
> timer is stopped, then while freeing if we still have a timer it is an error
> condition and should be DCHECKED.

Good suggestion. I have updated the patch. The change is to observe
NOTIFICATION_APP_TERMINATING to stop the sampling timer. Pls have a review.

[benwells, 2012-10-04 06:58:59]

Getting close...

http://codereview.chromium.org/10905171/diff/36001/chrome/browser/extensions/...
File chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.cc (right):

http://codereview.chromium.org/10905171/diff/36001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.cc:71: const
content::NotificationSource& source,
Nit: fix indenting here. If you have room line up the types inside the bracket:

foo bar(int type,
        const content::.....

otherwise move down a line and indent 4 spaces:

foo bar(
    int type,
    const content::.....

http://codereview.chromium.org/10905171/diff/36001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.cc:94:
DCHECK(!sampling_timer_) << "The sampling timer is leaked. Did you forgot "
Nit: The sampling timer has leaked. Did you forget to call...

http://codereview.chromium.org/10905171/diff/36001/chrome/browser/extensions/...
File chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.h (right):

http://codereview.chromium.org/10905171/diff/36001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.h:50: class
CpuTime {
Nit: leave this as a struct.

[Hongbo Min, 2012-10-05 02:40:56]

http://codereview.chromium.org/10905171/diff/36001/chrome/browser/extensions/...
File chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.cc (right):

http://codereview.chromium.org/10905171/diff/36001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.cc:71: const
content::NotificationSource& source,
On 2012/10/04 06:58:59, benwells wrote:
> Nit: fix indenting here. If you have room line up the types inside the
bracket:
> 
> foo bar(int type,
>         const content::.....
> 
> otherwise move down a line and indent 4 spaces:
> 
> foo bar(
>     int type,
>     const content::.....

Done.

http://codereview.chromium.org/10905171/diff/36001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.cc:94:
DCHECK(!sampling_timer_) << "The sampling timer is leaked. Did you forgot "
On 2012/10/04 06:58:59, benwells wrote:
> Nit: The sampling timer has leaked. Did you forget to call...

Done.

http://codereview.chromium.org/10905171/diff/36001/chrome/browser/extensions/...
File chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.h (right):

http://codereview.chromium.org/10905171/diff/36001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_cpu/cpu_info_provider.h:50: class
CpuTime {
On 2012/10/04 06:58:59, benwells wrote:
> Nit: leave this as a struct.

Done.

[benwells, 2012-10-08 05:38:33]

lgtm

[commit-bot: I haz the power, 2012-10-09 00:18:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/10905171/46001

[commit-bot: I haz the power, 2012-10-09 00:39:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/10905171/52004

[commit-bot: I haz the power, 2012-10-09 01:07:51]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
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, 2012-10-09 01:42:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/10905171/64001

[commit-bot: I haz the power, 2012-10-09 01:42:27]

Presubmit check for 10905171-64001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 4.3s to calculate.

[Hongbo Min, 2012-10-09 01:50:43]

thakis@, need your lgtm for gypi changes.

The change is for android build since cpu_info_provider.cc is needed for
linking.

[Hongbo Min, 2012-10-10 11:58:01]

On 2012/10/09 01:50:43, Hongbo Min wrote:
> thakis@, need your lgtm for gypi changes.
> 
> The change is for android build since cpu_info_provider.cc is needed for
> linking.
ping thakis@

[Hongbo Min, 2012-10-10 23:22:45]

brettw@, could you pls help review it? Thanks

[brettw, 2012-10-11 21:22:43]

chrome/chrome_browser_extensions.gypi LGTM (this is all you needed me to look
at, right?).

[Hongbo Min, 2012-10-11 23:46:34]

On 2012/10/11 21:22:43, brettw wrote:
> chrome/chrome_browser_extensions.gypi LGTM (this is all you needed me to look
> at, right?).

exactly. Thanks.

[commit-bot: I haz the power, 2012-10-11 23:47:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/10905171/64001

[commit-bot: I haz the power, 2012-10-12 00:18:59]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
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, 2012-10-12 01:16:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/10905171/66003

[commit-bot: I haz the power, 2012-10-12 04:15:44]

Change committed as 161499

[falken, 2012-10-12 05:24:11]

On 2012/10/12 04:15:44, I haz the power (commit-bot) wrote:
> Change committed as 161499

I'm sorry to revert this change. It seemed to regress pref sizes on Linux:
http://build.chromium.org/p/chromium/builders/Linux/builds/32047

I suspect the real cause is the include of <iostream> in
cpu_info_provider_linux.cc, which apparently includes a static initializer.
Would you consider rewriting the code to not use streams? They are also
discouraged by the style guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Stream...

[Hongbo Min, 2012-10-12 05:50:30]

On 2012/10/12 05:24:11, falken wrote:
> On 2012/10/12 04:15:44, I haz the power (commit-bot) wrote:
> > Change committed as 161499
> 
> I'm sorry to revert this change. It seemed to regress pref sizes on Linux:
> http://build.chromium.org/p/chromium/builders/Linux/builds/32047
> 
> I suspect the real cause is the include of <iostream> in
> cpu_info_provider_linux.cc, which apparently includes a static initializer.
> Would you consider rewriting the code to not use streams? They are also
> discouraged by the style guide:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Stream...

Actually the cpu_info_provider_linux.cc is changed in this CL. It is in
http://codereview.chromium.org/10907139/.

About the pref size, how is it measured?

[Hongbo Min, 2012-10-12 05:55:49]

On 2012/10/12 05:50:30, Hongbo Min wrote:
> On 2012/10/12 05:24:11, falken wrote:
> > On 2012/10/12 04:15:44, I haz the power (commit-bot) wrote:
> > > Change committed as 161499
> > 
> > I'm sorry to revert this change. It seemed to regress pref sizes on Linux:
> > http://build.chromium.org/p/chromium/builders/Linux/builds/32047
> > 
> > I suspect the real cause is the include of <iostream> in
> > cpu_info_provider_linux.cc, which apparently includes a static initializer.
> > Would you consider rewriting the code to not use streams? They are also
> > discouraged by the style guide:
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Stream...

Correct: Actually the cpu_info_provider_linux.cc is NOT changed in this CL. It
is in http://codereview.chromium.org/10907139/.

About the pref size, how is it measured?

[Hongbo Min, 2012-10-13 00:11:54]

On 2012/10/12 05:55:49, Hongbo Min wrote:
> On 2012/10/12 05:50:30, Hongbo Min wrote:
> > On 2012/10/12 05:24:11, falken wrote:
> > > On 2012/10/12 04:15:44, I haz the power (commit-bot) wrote:
> > > > Change committed as 161499
> > > 
> > > I'm sorry to revert this change. It seemed to regress pref sizes on Linux:
> > > http://build.chromium.org/p/chromium/builders/Linux/builds/32047
> > > 
> > > I suspect the real cause is the include of <iostream> in
> > > cpu_info_provider_linux.cc, which apparently includes a static
initializer.
> > > Would you consider rewriting the code to not use streams? They are also
> > > discouraged by the style guide:
> > >
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Stream...
> 
> Correct: Actually the cpu_info_provider_linux.cc is NOT changed in this CL. It
> is in http://codereview.chromium.org/10907139/.
> 
> About the pref size, how is it measured?

Reopen it and will commit once the http://codereview.chromium.org/11114010/ is
landed.

[commit-bot: I haz the power, 2012-10-13 06:43:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hongbo.min@intel.com/10905171/66003

[commit-bot: I haz the power, 2012-10-13 06:44:24]

Change committed as 161742

[Nico, 2012-10-15 15:20:41]

On 2012/10/10 11:58:01, Hongbo Min wrote:
> On 2012/10/09 01:50:43, Hongbo Min wrote:
> > thakis@, need your lgtm for gypi changes.
> > 
> > The change is for android build since cpu_info_provider.cc is needed for
> > linking.
> ping thakis@

lgtm. Sorry for the delay, I was on vacation.