Move systemInfo.cpu API out out experimental namespace.

Move systemInfo.cpu API out out experimental namespace.

As a consequence, we move systemInfo.cpu from using the experimental API permissions to its own permission based on an extension whitelist.

BUG=237294
TEST=updated unit tests

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

[Tim Song, 2013-05-29 17:33:13]

Please take a look at this attempt to move this API out of experimental. After
this move, I will remove the onUpdated functionality, and add authorized
extensions to the whitelist.

[Tim Song, 2013-05-29 18:19:24]

https://codereview.chromium.org/15817008/diff/2001/chrome/common/extensions/p...
File chrome/common/extensions/permissions/api_permission.h (right):

https://codereview.chromium.org/15817008/diff/2001/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission.h:115: kSystemInfoDisplay,
There is a kSystemInfoDisplay permission here. Would it be a good idea to merge
them into a single systemInfo permission?

[Hongbo Min, 2013-05-30 11:51:12]

Thanks!

I found the systmeInfo.cpu.get is limited to only be accessed by whiltelist
extension, may I know the background? e.g. which app should use it, and what is
the main consideration about its usage since you mention cpu.onUpdated event
will be removed in next CL.

https://codereview.chromium.org/15817008/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/api/system_info_cpu/system_info_cpu_api.h
(right):

https://codereview.chromium.org/15817008/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_cpu/system_info_cpu_api.h:15:
SYSTEMINFO_CPU_GET)
You might want to merge 14-15 lines.

https://codereview.chromium.org/15817008/diff/2001/chrome/common/extensions/a...
File chrome/common/extensions/api/api.gyp (right):

https://codereview.chromium.org/15817008/diff/2001/chrome/common/extensions/a...
chrome/common/extensions/api/api.gyp:55: 'system_info_cpu.idl',
Don't we need to keep alhpa-order in this file?

https://codereview.chromium.org/15817008/diff/2001/chrome/common/extensions/p...
File chrome/common/extensions/permissions/api_permission.h (right):

https://codereview.chromium.org/15817008/diff/2001/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission.h:115: kSystemInfoDisplay,
On 2013/05/29 18:19:24, tengs wrote:
> There is a kSystemInfoDisplay permission here. Would it be a good idea to
merge
> them into a single systemInfo permission?

Agree.

[asargent_no_longer_on_chrome, 2013-05-30 19:14:00]

https://codereview.chromium.org/15817008/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/activity_log/api_name_constants.h (right):

https://codereview.chromium.org/15817008/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/api_name_constants.h:85:
"systemInfo.cpu.onUpdated",
Please see comments at the top of this enum.

https://codereview.chromium.org/15817008/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/api/system_info_cpu/system_info_cpu_api.cc
(right):

https://codereview.chromium.org/15817008/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_cpu/system_info_cpu_api.cc:29: return
false;
Do you even need to do this? Doesn't ExtensionFunctionDispatcher take care of it
for you?

https://codereview.chromium.org/15817008/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_function_histogram_value.h (right):

https://codereview.chromium.org/15817008/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/extension_function_histogram_value.h:417:
SYSTEMINFO_CPU_GET,
See comments at the top of this enum.

https://codereview.chromium.org/15817008/diff/2001/chrome/common/extensions/a...
File chrome/common/extensions/api/system_info_cpu.idl (right):

https://codereview.chromium.org/15817008/diff/2001/chrome/common/extensions/a...
chrome/common/extensions/api/system_info_cpu.idl:28: // Get CPU information.
nit: can we include something here about whether this queries the state or
returns a cached value?

https://codereview.chromium.org/15817008/diff/2001/chrome/common/extensions/a...
chrome/common/extensions/api/system_info_cpu.idl:34: // period interval is 1
seconds.
I don't understand this comment about the "default" period interval, since it
doesn't seem that developers can change it. Can we be more precise here?

I can definitely see how it would be interesting to be able to specify an
interval, but the API style might need to change to be more like chrome.alarms.

https://codereview.chromium.org/15817008/diff/2001/chrome/common/extensions/p...
File chrome/common/extensions/permissions/permission_message.h (right):

https://codereview.chromium.org/15817008/diff/2001/chrome/common/extensions/p...
chrome/common/extensions/permissions/permission_message.h:65: kSystemInfoCpu,
Since there is no message displayed in the confirmation UI for this permission,
I think we don't need this here right?

https://codereview.chromium.org/15817008/diff/2001/tools/metrics/histograms/h...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/15817008/diff/2001/tools/metrics/histograms/h...
tools/metrics/histograms/histograms.xml:11050: <int value="355"
label="SYSTEMINFO_CPU_GET"/>
I don't think you should change this here - instead you should be adding a new
entry.

[Tim Song, 2013-05-31 01:08:34]

https://codereview.chromium.org/15817008/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/activity_log/api_name_constants.h (right):

https://codereview.chromium.org/15817008/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/api_name_constants.h:85:
"systemInfo.cpu.onUpdated",
On 2013/05/30 19:14:00, Antony Sargent wrote:
> Please see comments at the top of this enum. 

I think just renaming existing functions should be fine because the order
doesn't change, right?

https://codereview.chromium.org/15817008/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/api/system_info_cpu/system_info_cpu_api.cc
(right):

https://codereview.chromium.org/15817008/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_cpu/system_info_cpu_api.cc:29: return
false;
On 2013/05/30 19:14:00, Antony Sargent wrote:
> Do you even need to do this? Doesn't ExtensionFunctionDispatcher take care of
it
> for you?

Done. Apparently, I needed an non-empty list for the "whitelist" value in
_permission_features.json for the system to enforce the permissions.

https://codereview.chromium.org/15817008/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/api/system_info_cpu/system_info_cpu_api.h
(right):

https://codereview.chromium.org/15817008/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/api/system_info_cpu/system_info_cpu_api.h:15:
SYSTEMINFO_CPU_GET)
On 2013/05/30 11:51:12, Hongbo Min wrote:
> You might want to merge 14-15 lines.

Done.

https://codereview.chromium.org/15817008/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_function_histogram_value.h (right):

https://codereview.chromium.org/15817008/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/extension_function_histogram_value.h:417:
SYSTEMINFO_CPU_GET,
On 2013/05/30 19:14:00, Antony Sargent wrote:
> See comments at the top of this enum.

The comments does say that renaming an existing function should be fine, unless
if we still want to track the experimental calls separately.

https://codereview.chromium.org/15817008/diff/2001/chrome/common/extensions/a...
File chrome/common/extensions/api/api.gyp (right):

https://codereview.chromium.org/15817008/diff/2001/chrome/common/extensions/a...
chrome/common/extensions/api/api.gyp:55: 'system_info_cpu.idl',
On 2013/05/30 11:51:12, Hongbo Min wrote:
> Don't we need to keep alhpa-order in this file?

Done.

https://codereview.chromium.org/15817008/diff/2001/chrome/common/extensions/a...
File chrome/common/extensions/api/system_info_cpu.idl (right):

https://codereview.chromium.org/15817008/diff/2001/chrome/common/extensions/a...
chrome/common/extensions/api/system_info_cpu.idl:28: // Get CPU information.
On 2013/05/30 19:14:00, Antony Sargent wrote:
> nit: can we include something here about whether this queries the state or
> returns a cached value?

This function gets basic CPU information using base::SysInfo, and doesn't cache
anything.

https://codereview.chromium.org/15817008/diff/2001/chrome/common/extensions/a...
chrome/common/extensions/api/system_info_cpu.idl:34: // period interval is 1
seconds.
On 2013/05/30 19:14:00, Antony Sargent wrote:
> I don't understand this comment about the "default" period interval, since it
> doesn't seem that developers can change it. Can we be more precise here?
> 
> I can definitely see how it would be interesting to be able to specify an
> interval, but the API style might need to change to be more like
chrome.alarms. 

From what I understand, we're planning on removing this function as it's not
being used anywhere.

https://codereview.chromium.org/15817008/diff/2001/chrome/common/extensions/p...
File chrome/common/extensions/permissions/api_permission.h (right):

https://codereview.chromium.org/15817008/diff/2001/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission.h:115: kSystemInfoDisplay,
On 2013/05/30 11:51:12, Hongbo Min wrote:
> On 2013/05/29 18:19:24, tengs wrote:
> > There is a kSystemInfoDisplay permission here. Would it be a good idea to
> merge
> > them into a single systemInfo permission?
> 
> Agree.

With further thought, we should keep these permissions separate as
systemInfo.display is permitted for the stable channel right now, and we should
just leave systemInfo.cpu in dev.

https://codereview.chromium.org/15817008/diff/2001/chrome/common/extensions/p...
File chrome/common/extensions/permissions/permission_message.h (right):

https://codereview.chromium.org/15817008/diff/2001/chrome/common/extensions/p...
chrome/common/extensions/permissions/permission_message.h:65: kSystemInfoCpu,
On 2013/05/30 19:14:00, Antony Sargent wrote:
> Since there is no message displayed in the confirmation UI for this
permission,
> I think we don't need this here right?

I think you're right. Done.

https://codereview.chromium.org/15817008/diff/2001/tools/metrics/histograms/h...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/15817008/diff/2001/tools/metrics/histograms/h...
tools/metrics/histograms/histograms.xml:11050: <int value="355"
label="SYSTEMINFO_CPU_GET"/>
On 2013/05/30 19:14:00, Antony Sargent wrote:
> I don't think you should change this here - instead you should be adding a new
> entry. 

I think renaming should be fine. The comment above says these fields were
auto-generated from
chrome/browser/extensions/extension_function_histogram_value.h, and the order of
the enum fields are the same. The comment in
extension_function_histogram_value.h states that renaming is fine because the
index of the histogram value remains the same.

[asargent_no_longer_on_chrome, 2013-05-31 18:31:31]

https://codereview.chromium.org/15817008/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/activity_log/api_name_constants.h (right):

https://codereview.chromium.org/15817008/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/api_name_constants.h:85:
"systemInfo.cpu.onUpdated",
On 2013/05/31 01:08:35, tengs wrote:
> On 2013/05/30 19:14:00, Antony Sargent wrote:
> > Please see comments at the top of this enum. 
> 
> I think just renaming existing functions should be fine because the order
> doesn't change, right?

We should create new entries, not rename the old ones.

https://codereview.chromium.org/15817008/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_function_histogram_value.h (right):

https://codereview.chromium.org/15817008/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/extension_function_histogram_value.h:417:
SYSTEMINFO_CPU_GET,
On 2013/05/31 01:08:35, tengs wrote:
> On 2013/05/30 19:14:00, Antony Sargent wrote:
> > See comments at the top of this enum.
> 
> The comments does say that renaming an existing function should be fine,
unless
> if we still want to track the experimental calls separately. 

I think it's actually useful for us to be able to distinguish between the
experimental and non-experimental versions, so please follow the "new function"
case.

https://codereview.chromium.org/15817008/diff/2001/chrome/common/extensions/a...
File chrome/common/extensions/api/system_info_cpu.idl (right):

https://codereview.chromium.org/15817008/diff/2001/chrome/common/extensions/a...
chrome/common/extensions/api/system_info_cpu.idl:34: // period interval is 1
seconds.
On 2013/05/31 01:08:35, tengs wrote:
> On 2013/05/30 19:14:00, Antony Sargent wrote:
> > I don't understand this comment about the "default" period interval, since
it
> > doesn't seem that developers can change it. Can we be more precise here?
> > 
> > I can definitely see how it would be interesting to be able to specify an
> > interval, but the API style might need to change to be more like
> chrome.alarms. 
> 
> From what I understand, we're planning on removing this function as it's not
> being used anywhere.

Any reason not to just remove it now? In any event, since we are controlling
access to the entire API via the whitelist it's ok to leave in if we need to, we
just need to make sure not to make it generally available on stable channel if
we know we plan to remove it in the future.

https://codereview.chromium.org/15817008/diff/2001/tools/metrics/histograms/h...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/15817008/diff/2001/tools/metrics/histograms/h...
tools/metrics/histograms/histograms.xml:11050: <int value="355"
label="SYSTEMINFO_CPU_GET"/>
On 2013/05/31 01:08:35, tengs wrote:
> On 2013/05/30 19:14:00, Antony Sargent wrote:
> > I don't think you should change this here - instead you should be adding a
new
> > entry. 
> 
> I think renaming should be fine. The comment above says these fields were
> auto-generated from
> chrome/browser/extensions/extension_function_histogram_value.h, and the order
of
> the enum fields are the same. The comment in
> extension_function_histogram_value.h states that renaming is fine because the
> index of the histogram value remains the same.

As I mentioned elsewhere, I think it's actually useful data to have separate
histograms for both the old and new names.

[Tim Song, 2013-05-31 19:29:02]

https://codereview.chromium.org/15817008/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/activity_log/api_name_constants.h (right):

https://codereview.chromium.org/15817008/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/api_name_constants.h:85:
"systemInfo.cpu.onUpdated",
On 2013/05/31 18:31:32, Antony Sargent wrote:
> On 2013/05/31 01:08:35, tengs wrote:
> > On 2013/05/30 19:14:00, Antony Sargent wrote:
> > > Please see comments at the top of this enum. 
> > 
> > I think just renaming existing functions should be fine because the order
> > doesn't change, right?
> 
> We should create new entries, not rename the old ones.

Done.

https://codereview.chromium.org/15817008/diff/2001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_function_histogram_value.h (right):

https://codereview.chromium.org/15817008/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/extension_function_histogram_value.h:417:
SYSTEMINFO_CPU_GET,
On 2013/05/31 18:31:32, Antony Sargent wrote:
> On 2013/05/31 01:08:35, tengs wrote:
> > On 2013/05/30 19:14:00, Antony Sargent wrote:
> > > See comments at the top of this enum.
> > 
> > The comments does say that renaming an existing function should be fine,
> unless
> > if we still want to track the experimental calls separately. 
> 
> I think it's actually useful for us to be able to distinguish between the
> experimental and non-experimental versions, so please follow the "new
function"
> case.

Done.

https://codereview.chromium.org/15817008/diff/2001/chrome/common/extensions/a...
File chrome/common/extensions/api/system_info_cpu.idl (right):

https://codereview.chromium.org/15817008/diff/2001/chrome/common/extensions/a...
chrome/common/extensions/api/system_info_cpu.idl:34: // period interval is 1
seconds.
On 2013/05/31 18:31:32, Antony Sargent wrote:
> On 2013/05/31 01:08:35, tengs wrote:
> > On 2013/05/30 19:14:00, Antony Sargent wrote:
> > > I don't understand this comment about the "default" period interval, since
> it
> > > doesn't seem that developers can change it. Can we be more precise here?
> > > 
> > > I can definitely see how it would be interesting to be able to specify an
> > > interval, but the API style might need to change to be more like
> > chrome.alarms. 
> > 
> > From what I understand, we're planning on removing this function as it's not
> > being used anywhere.
> 
> Any reason not to just remove it now? In any event, since we are controlling
> access to the entire API via the whitelist it's ok to leave in if we need to,
we
> just need to make sure not to make it generally available on stable channel if
> we know we plan to remove it in the future. 

I was planning on doing this after this changelist as there is quite a bit of
code to be removed.

https://codereview.chromium.org/15817008/diff/2001/tools/metrics/histograms/h...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/15817008/diff/2001/tools/metrics/histograms/h...
tools/metrics/histograms/histograms.xml:11050: <int value="355"
label="SYSTEMINFO_CPU_GET"/>
On 2013/05/31 18:31:32, Antony Sargent wrote:
> On 2013/05/31 01:08:35, tengs wrote:
> > On 2013/05/30 19:14:00, Antony Sargent wrote:
> > > I don't think you should change this here - instead you should be adding a
> new
> > > entry. 
> > 
> > I think renaming should be fine. The comment above says these fields were
> > auto-generated from
> > chrome/browser/extensions/extension_function_histogram_value.h, and the
order
> of
> > the enum fields are the same. The comment in
> > extension_function_histogram_value.h states that renaming is fine because
the
> > index of the histogram value remains the same.
> 
> As I mentioned elsewhere, I think it's actually useful data to have separate
> histograms for both the old and new names. 

Done.

[asargent_no_longer_on_chrome, 2013-05-31 19:36:34]

lgtm

[commit-bot: I haz the power, 2013-05-31 19:47:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tengs@chromium.org/15817008/27001

[commit-bot: I haz the power, 2013-05-31 19:59:49]

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

[Tim Song, 2013-05-31 20:11:13]

+isherman

Can you take a look at histograms.xml for OWNERS approval?

[Ilya Sherman, 2013-05-31 20:18:54]

lgtm

[commit-bot: I haz the power, 2013-05-31 20:19:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tengs@chromium.org/15817008/27001

[commit-bot: I haz the power, 2013-06-01 00:41:19]

Change committed as 203537