Add the implementation skeleton for systeminfo.cpu API

Add the code skeleton for systemInfo.cpu.get API implementation.

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


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

[Hongbo Min, 2012-08-09 14:05:00]

Mihai,

This is the first patch for systeminfo.cpu. According to your suggestion, it
just focuses on providing a basic API implementaion skeleton for
systeminfo.cpu.get API. Please review it. Thanks.

[Mihai Parparita -not on Chrome, 2012-08-10 20:24:56]

Thanks for the smaller patch, this is much easier to review.

I think it should be good to land after one more review cycle.

http://codereview.chromium.org/10830240/diff/14/chrome/browser/extensions/api...
File chrome/browser/extensions/api/systeminfo_cpu/systeminfo_cpu_api.cc (right):

http://codereview.chromium.org/10830240/diff/14/chrome/browser/extensions/api...
chrome/browser/extensions/api/systeminfo_cpu/systeminfo_cpu_api.cc:30: void
SysteminfoCpuGetFunction::GetCpuInfoOnFileThread() {
Can you move this to be below WorkOnFileThread, so that that the functions
appear in the file in the same order that they call each-other in?

http://codereview.chromium.org/10830240/diff/14/chrome/browser/extensions/api...
chrome/browser/extensions/api/systeminfo_cpu/systeminfo_cpu_api.cc:49: provider_
= CpuInfoProvider::Create();
It seems expensive to create a provider per function call, especially if you
have to wait for at least one sample to be collected each time. Is the eventual
plan to have a single shared instance?

http://codereview.chromium.org/10830240/diff/14/chrome/browser/extensions/api...
chrome/browser/extensions/api/systeminfo_cpu/systeminfo_cpu_api.cc:50: // Delay
500ms to give a chance for CPU information sampling.
The 500ms delay seems arbitrary and prone to flakiness. Can the provider support
a OnNextSample(callback) method instead?

http://codereview.chromium.org/10830240/diff/14/chrome/browser/extensions/api...
File chrome/browser/extensions/api/systeminfo_cpu/systeminfo_cpu_api.h (right):

http://codereview.chromium.org/10830240/diff/14/chrome/browser/extensions/api...
chrome/browser/extensions/api/systeminfo_cpu/systeminfo_cpu_api.h:15:
DECLARE_EXTENSION_FUNCTION_NAME("experimental.systeminfo.cpu.get");
Can you capitalize the "info" in "systeminfo" (and in the classname too)? Other
two-word APIs (webRequest, webNavigation, contextMenus) also use camel-case.

http://codereview.chromium.org/10830240/diff/14/chrome/browser/extensions/api...
chrome/browser/extensions/api/systeminfo_cpu/systeminfo_cpu_api.h:29: class
SysteminfoCpuSetUpdateIntervalFunction : public AsyncExtensionFunction {
If this is a no-op, can you remove this from the CL?

http://codereview.chromium.org/10830240/diff/14/chrome/common/extensions/api/...
File chrome/common/extensions/api/experimental_systeminfo_cpu.idl (right):

http://codereview.chromium.org/10830240/diff/14/chrome/common/extensions/api/...
chrome/common/extensions/api/experimental_systeminfo_cpu.idl:9: // Indicates the
load of CPU core, as a number between 0 and 100
Nit: comments end with a period (applies to the whole file).

http://codereview.chromium.org/10830240/diff/14/chrome/test/data/extensions/a...
File chrome/test/data/extensions/api_test/systeminfo/cpu/manifest.json (right):

http://codereview.chromium.org/10830240/diff/14/chrome/test/data/extensions/a...
chrome/test/data/extensions/api_test/systeminfo/cpu/manifest.json:2: "name":
"cpuinfo",
This file appears to use tabs. We use two-space indentation in manifest files,
just like the rest of the codebase.

http://codereview.chromium.org/10830240/diff/14/chrome/test/data/extensions/a...
File chrome/test/data/extensions/api_test/systeminfo/cpu/test_cpuinfo_api.html
(right):

http://codereview.chromium.org/10830240/diff/14/chrome/test/data/extensions/a...
chrome/test/data/extensions/api_test/systeminfo/cpu/test_cpuinfo_api.html:1:
<script src="test_cpuinfo_api.js"></script>
If all your background page does is include a .js file, you can avoid having a
page entirely by using "background": {"scripts": [test_cpuinfo_api.js"]} in your
manifest.

[Hongbo Min, 2012-08-12 14:22:43]

Mihaip, I have updated the patch according to your comments. Pls have a review.
Thanks.

http://codereview.chromium.org/10830240/diff/14/chrome/browser/extensions/api...
File chrome/browser/extensions/api/systeminfo_cpu/systeminfo_cpu_api.cc (right):

http://codereview.chromium.org/10830240/diff/14/chrome/browser/extensions/api...
chrome/browser/extensions/api/systeminfo_cpu/systeminfo_cpu_api.cc:30: void
SysteminfoCpuGetFunction::GetCpuInfoOnFileThread() {
On 2012/08/10 20:24:56, Mihai Parparita wrote:
> Can you move this to be below WorkOnFileThread, so that that the functions
> appear in the file in the same order that they call each-other in?

Done.

http://codereview.chromium.org/10830240/diff/14/chrome/browser/extensions/api...
chrome/browser/extensions/api/systeminfo_cpu/systeminfo_cpu_api.cc:49: provider_
= CpuInfoProvider::Create();
On 2012/08/10 20:24:56, Mihai Parparita wrote:
> It seems expensive to create a provider per function call, especially if you
> have to wait for at least one sample to be collected each time. Is the
eventual
> plan to have a single shared instance?

Yes, it makes lots of senses to make it as a single shared instance via
Singleton or  base::LazyInstance support.

Here the code for provider creation is actually premature and just for the API
skeleton. Next step I will focus on providing the its refinement and the
provider implementation.

http://codereview.chromium.org/10830240/diff/14/chrome/browser/extensions/api...
chrome/browser/extensions/api/systeminfo_cpu/systeminfo_cpu_api.cc:50: // Delay
500ms to give a chance for CPU information sampling.
On 2012/08/10 20:24:56, Mihai Parparita wrote:
> The 500ms delay seems arbitrary and prone to flakiness. Can the provider
support
> a OnNextSample(callback) method instead?

The 500ms is a tricky for getting cpu usage per core on linux. On linux, the cpu
load is calculated from /proc/stat file. It requires to read /proc/stat file at
a interval, for example, 0.5 seconds. The cpu usage is actually a statistics for
that interval.

On Windows, no such a tricky is needed. 

Here I just remove it and let it going in next reviewing cycle. It is relevant
to provider implementation.

http://codereview.chromium.org/10830240/diff/14/chrome/browser/extensions/api...
File chrome/browser/extensions/api/systeminfo_cpu/systeminfo_cpu_api.h (right):

http://codereview.chromium.org/10830240/diff/14/chrome/browser/extensions/api...
chrome/browser/extensions/api/systeminfo_cpu/systeminfo_cpu_api.h:15:
DECLARE_EXTENSION_FUNCTION_NAME("experimental.systeminfo.cpu.get");
On 2012/08/10 20:24:56, Mihai Parparita wrote:
> Can you capitalize the "info" in "systeminfo" (and in the classname too)?
Other
> two-word APIs (webRequest, webNavigation, contextMenus) also use camel-case.

Done.

http://codereview.chromium.org/10830240/diff/14/chrome/browser/extensions/api...
chrome/browser/extensions/api/systeminfo_cpu/systeminfo_cpu_api.h:29: class
SysteminfoCpuSetUpdateIntervalFunction : public AsyncExtensionFunction {
On 2012/08/10 20:24:56, Mihai Parparita wrote:
> If this is a no-op, can you remove this from the CL?
Remove setUpdateInterval from cc and idl file.

Done.

http://codereview.chromium.org/10830240/diff/14/chrome/common/extensions/api/...
File chrome/common/extensions/api/experimental_systeminfo_cpu.idl (right):

http://codereview.chromium.org/10830240/diff/14/chrome/common/extensions/api/...
chrome/common/extensions/api/experimental_systeminfo_cpu.idl:9: // Indicates the
load of CPU core, as a number between 0 and 100
On 2012/08/10 20:24:56, Mihai Parparita wrote:
> Nit: comments end with a period (applies to the whole file).

Done.

http://codereview.chromium.org/10830240/diff/14/chrome/test/data/extensions/a...
File chrome/test/data/extensions/api_test/systeminfo/cpu/manifest.json (right):

http://codereview.chromium.org/10830240/diff/14/chrome/test/data/extensions/a...
chrome/test/data/extensions/api_test/systeminfo/cpu/manifest.json:2: "name":
"cpuinfo",
On 2012/08/10 20:24:56, Mihai Parparita wrote:
> This file appears to use tabs. We use two-space indentation in manifest files,
> just like the rest of the codebase.

Done.

http://codereview.chromium.org/10830240/diff/14/chrome/test/data/extensions/a...
File chrome/test/data/extensions/api_test/systeminfo/cpu/test_cpuinfo_api.html
(right):

http://codereview.chromium.org/10830240/diff/14/chrome/test/data/extensions/a...
chrome/test/data/extensions/api_test/systeminfo/cpu/test_cpuinfo_api.html:1:
<script src="test_cpuinfo_api.js"></script>
On 2012/08/10 20:24:56, Mihai Parparita wrote:
> If all your background page does is include a .js file, you can avoid having a
> page entirely by using "background": {"scripts": [test_cpuinfo_api.js"]} in
your
> manifest.

Done.

[Mihai Parparita -not on Chrome, 2012-08-13 23:55:07]

LGTM

+Nico for chrome/OWNERS

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

http://codereview.chromium.org/10830240/diff/1015/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_info_cpu/system_info_cpu_apitest.cc:10:
public:
Nit: should be indented by one space.

http://codereview.chromium.org/10830240/diff/1015/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental_system_info_cpu.idl (right):

http://codereview.chromium.org/10830240/diff/1015/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_system_info_cpu.idl:33: interface
Events {
Remove events from the IDL, since they're not implemented yet.

[Nico, 2012-08-13 23:57:35]

lgtm for chrome/*.gypi

[commit-bot: I haz the power, 2012-08-14 03:10:40]

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

[commit-bot: I haz the power, 2012-08-14 03:32:38]

Try job failure for 10830240-7025 (retry) on mac for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[commit-bot: I haz the power, 2012-08-14 04:33:05]

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

[commit-bot: I haz the power, 2012-08-14 08:05:21]

Try job failure for 10830240-12023 (retry) on linux_chromeos for steps
"browser_tests, content_browsertests".
It's a second try, previously, steps "browser_tests, content_browsertests"
failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-08-14 08:23:41]

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

[commit-bot: I haz the power, 2012-08-14 09:47:11]

Change committed as 151446