Add chrome.power extension API.

Add chrome.power extension API.

This moves the chrome.experimental.power API to
chrome.power, makes minor changes to its public interface
(adding a "level" parameter to requestKeepAwake() and
removing the callbacks from both methods), adds unit tests
for it, and adds a sample "Keep Awake" extension meant to
replace the existing Chrome OS-specific extension at
http://goo.gl/CrUzi.

BUG=178944
TBR=sky@chromium.org

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

[not at google - send to devlin, 2013-03-19 21:16:39]

sorry, forgot about this until now. Main comment is making the level an enum.

https://codereview.chromium.org/12576018/diff/28001/chrome/browser/extensions...
File chrome/browser/extensions/api/power/power_api.cc (right):

https://codereview.chromium.org/12576018/diff/28001/chrome/browser/extensions...
chrome/browser/extensions/api/power/power_api.cc:21: level =
PowerApiManager::LEVEL_SYSTEM;
enums get compiled down to real C++ enums, so you could switch(params->level){}
then.

https://codereview.chromium.org/12576018/diff/28001/chrome/browser/extensions...
File chrome/browser/extensions/api/power/power_api_manager.cc (right):

https://codereview.chromium.org/12576018/diff/28001/chrome/browser/extensions...
chrome/browser/extensions/api/power/power_api_manager.cc:49: if (type ==
chrome::NOTIFICATION_EXTENSION_UNLOADED) {
prefer switch

https://codereview.chromium.org/12576018/diff/28001/chrome/browser/extensions...
chrome/browser/extensions/api/power/power_api_manager.cc:71:
power_save_blocker_.reset();
nit: early return

https://codereview.chromium.org/12576018/diff/28001/chrome/browser/extensions...
chrome/browser/extensions/api/power/power_api_manager.cc:83: if
(!power_save_blocker_.get() || new_level != current_level_) {
.get() unnecessary

https://codereview.chromium.org/12576018/diff/28001/chrome/browser/extensions...
chrome/browser/extensions/api/power/power_api_manager.cc:87:
create_blocker_function_.get() ?
.get() unnecessary

https://codereview.chromium.org/12576018/diff/28001/chrome/browser/extensions...
chrome/browser/extensions/api/power/power_api_manager.cc:89:
content::PowerSaveBlocker::Create(type, kPowerSaveBlockerReason));
could you initialize create_blocker_function_ to
content::PowerSaveBlocker::Create? then you wouldn't need this extra logic at
all.

https://codereview.chromium.org/12576018/diff/28001/chrome/browser/extensions...
File chrome/browser/extensions/api/power/power_api_manager.h (right):

https://codereview.chromium.org/12576018/diff/28001/chrome/browser/extensions...
chrome/browser/extensions/api/power/power_api_manager.h:31: LEVEL_DISPLAY = 1,
if it were an enum, you wouldn't need this definition in the first place.

https://codereview.chromium.org/12576018/diff/28001/chrome/common/extensions/...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/12576018/diff/28001/chrome/common/extensions/...
chrome/common/extensions/api/_permission_features.json:318: "extension_types":
["extension", "hosted_app", "packaged_app"]
why hosted_app?

https://codereview.chromium.org/12576018/diff/28001/chrome/common/extensions/...
File chrome/common/extensions/api/api.gyp (left):

https://codereview.chromium.org/12576018/diff/28001/chrome/common/extensions/...
chrome/common/extensions/api/api.gyp:50: 'experimental_power.json',
delete this file?

https://codereview.chromium.org/12576018/diff/28001/chrome/common/extensions/...
File chrome/common/extensions/api/power.idl (right):

https://codereview.chromium.org/12576018/diff/28001/chrome/common/extensions/...
chrome/common/extensions/api/power.idl:10: // Requests that power management be
temporarily disabled. |level|
Should level be an enum?

https://codereview.chromium.org/12576018/diff/28001/chrome/common/extensions/...
File chrome/common/extensions/permissions/api_permission.cc (right):

https://codereview.chromium.org/12576018/diff/28001/chrome/common/extensions/...
chrome/common/extensions/permissions/api_permission.cc:219: {
APIPermission::kPower, "power", },
should there be a warning for this permission? it seems like there should be.

[Daniel Erat, 2013-03-19 23:39:38]

https://codereview.chromium.org/12576018/diff/28001/chrome/browser/extensions...
File chrome/browser/extensions/api/power/power_api.cc (right):

https://codereview.chromium.org/12576018/diff/28001/chrome/browser/extensions...
chrome/browser/extensions/api/power/power_api.cc:21: level =
PowerApiManager::LEVEL_SYSTEM;
On 2013/03/19 21:16:39, kalman wrote:
> enums get compiled down to real C++ enums, so you could
switch(params->level){}
> then.

Done (well, not using a switch, but got rid of the constants here).

https://codereview.chromium.org/12576018/diff/28001/chrome/browser/extensions...
File chrome/browser/extensions/api/power/power_api_manager.cc (right):

https://codereview.chromium.org/12576018/diff/28001/chrome/browser/extensions...
chrome/browser/extensions/api/power/power_api_manager.cc:49: if (type ==
chrome::NOTIFICATION_EXTENSION_UNLOADED) {
On 2013/03/19 21:16:39, kalman wrote:
> prefer switch

Done.

https://codereview.chromium.org/12576018/diff/28001/chrome/browser/extensions...
chrome/browser/extensions/api/power/power_api_manager.cc:71:
power_save_blocker_.reset();
On 2013/03/19 21:16:39, kalman wrote:
> nit: early return

Done.

https://codereview.chromium.org/12576018/diff/28001/chrome/browser/extensions...
chrome/browser/extensions/api/power/power_api_manager.cc:83: if
(!power_save_blocker_.get() || new_level != current_level_) {
On 2013/03/19 21:16:39, kalman wrote:
> .get() unnecessary

Done.

https://codereview.chromium.org/12576018/diff/28001/chrome/browser/extensions...
chrome/browser/extensions/api/power/power_api_manager.cc:87:
create_blocker_function_.get() ?
On 2013/03/19 21:16:39, kalman wrote:
> .get() unnecessary

Done.

https://codereview.chromium.org/12576018/diff/28001/chrome/browser/extensions...
chrome/browser/extensions/api/power/power_api_manager.cc:89:
content::PowerSaveBlocker::Create(type, kPowerSaveBlockerReason));
On 2013/03/19 21:16:39, kalman wrote:
> could you initialize create_blocker_function_ to
> content::PowerSaveBlocker::Create? then you wouldn't need this extra logic at
> all.

Done.

https://codereview.chromium.org/12576018/diff/28001/chrome/browser/extensions...
File chrome/browser/extensions/api/power/power_api_manager.h (right):

https://codereview.chromium.org/12576018/diff/28001/chrome/browser/extensions...
chrome/browser/extensions/api/power/power_api_manager.h:31: LEVEL_DISPLAY = 1,
On 2013/03/19 21:16:39, kalman wrote:
> if it were an enum, you wouldn't need this definition in the first place.

Done.

https://codereview.chromium.org/12576018/diff/28001/chrome/common/extensions/...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/12576018/diff/28001/chrome/common/extensions/...
chrome/common/extensions/api/_permission_features.json:318: "extension_types":
["extension", "hosted_app", "packaged_app"]
On 2013/03/19 21:16:39, kalman wrote:
> why hosted_app?

The intent is to permit e.g. a slide show app to prevent the screen from dimming
while displaying a presentation.

https://codereview.chromium.org/12576018/diff/28001/chrome/common/extensions/...
File chrome/common/extensions/api/api.gyp (left):

https://codereview.chromium.org/12576018/diff/28001/chrome/common/extensions/...
chrome/common/extensions/api/api.gyp:50: 'experimental_power.json',
On 2013/03/19 21:16:39, kalman wrote:
> delete this file?

Whoops.  Thanks, done.

https://codereview.chromium.org/12576018/diff/28001/chrome/common/extensions/...
File chrome/common/extensions/api/power.idl (right):

https://codereview.chromium.org/12576018/diff/28001/chrome/common/extensions/...
chrome/common/extensions/api/power.idl:10: // Requests that power management be
temporarily disabled. |level|
On 2013/03/19 21:16:39, kalman wrote:
> Should level be an enum?

Done.

https://codereview.chromium.org/12576018/diff/28001/chrome/common/extensions/...
File chrome/common/extensions/permissions/api_permission.cc (right):

https://codereview.chromium.org/12576018/diff/28001/chrome/common/extensions/...
chrome/common/extensions/permissions/api_permission.cc:219: {
APIPermission::kPower, "power", },
On 2013/03/19 21:16:39, kalman wrote:
> should there be a warning for this permission? it seems like there should be.

The consensus at the API review meeting last week seemed to be that a warning
was unneeded.

[not at google - send to devlin, 2013-03-20 00:25:50]

lgtm

https://codereview.chromium.org/12576018/diff/28001/chrome/common/extensions/...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/12576018/diff/28001/chrome/common/extensions/...
chrome/common/extensions/api/_permission_features.json:318: "extension_types":
["extension", "hosted_app", "packaged_app"]
On 2013/03/19 23:39:38, Daniel Erat wrote:
> On 2013/03/19 21:16:39, kalman wrote:
> > why hosted_app?
> 
> The intent is to permit e.g. a slide show app to prevent the screen from
dimming
> while displaying a presentation.

Ah, cool. Let's include platform_app too, then.

https://codereview.chromium.org/12576018/diff/28001/chrome/common/extensions/...
File chrome/common/extensions/permissions/api_permission.cc (right):

https://codereview.chromium.org/12576018/diff/28001/chrome/common/extensions/...
chrome/common/extensions/permissions/api_permission.cc:219: {
APIPermission::kPower, "power", },
On 2013/03/19 23:39:38, Daniel Erat wrote:
> On 2013/03/19 21:16:39, kalman wrote:
> > should there be a warning for this permission? it seems like there should
be.
> 
> The consensus at the API review meeting last week seemed to be that a warning
> was unneeded.

cool, I missed the one last week.

https://codereview.chromium.org/12576018/diff/42001/chrome/browser/extensions...
File chrome/browser/extensions/api/power/power_api.cc (right):

https://codereview.chromium.org/12576018/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/power/power_api.cc:17: params->level ==
api::power::LEVEL_NONE ? api::power::LEVEL_DISPLAY :
seems like PowerApiManager needs to deal with the NONE value already, so might
as well pass through params->level and normalize it there.

P.S. it can't possibly be NONE - schema compiler guarantees it (so in fact you
could EXTENSION_FUNCITON_VALIDATE(params->level != NONE) if you wanted). I tried
to make the schema compiler not generate it but it ended up being too much of a
yak.

https://codereview.chromium.org/12576018/diff/42001/chrome/browser/extensions...
File chrome/browser/extensions/api/power/power_api_manager.cc (right):

https://codereview.chromium.org/12576018/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/power/power_api_manager.cc:53:
create_blocker_function_ = base::Bind(&content::PowerSaveBlocker::Create);
if you like ternary

https://codereview.chromium.org/12576018/diff/42001/chrome/common/extensions/...
File chrome/common/extensions/api/power.idl (right):

https://codereview.chromium.org/12576018/diff/42001/chrome/common/extensions/...
chrome/common/extensions/api/power.idl:21: static void requestKeepAwake(optional
Level level);
Document what it means for level to be left out? Or just make it required?

[Daniel Erat, 2013-03-20 00:47:46]

https://codereview.chromium.org/12576018/diff/42001/chrome/browser/extensions...
File chrome/browser/extensions/api/power/power_api.cc (right):

https://codereview.chromium.org/12576018/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/power/power_api.cc:17: params->level ==
api::power::LEVEL_NONE ? api::power::LEVEL_DISPLAY :
On 2013/03/20 00:25:50, kalman wrote:
> seems like PowerApiManager needs to deal with the NONE value already, so might
> as well pass through params->level and normalize it there.
> 
> P.S. it can't possibly be NONE - schema compiler guarantees it (so in fact you
> could EXTENSION_FUNCITON_VALIDATE(params->level != NONE) if you wanted). I
tried
> to make the schema compiler not generate it but it ended up being too much of
a
> yak.

Ah, okay; I figured that NONE was there for optional params.  I'll just make it
non-optional and _VALIDATE here.

https://codereview.chromium.org/12576018/diff/42001/chrome/browser/extensions...
File chrome/browser/extensions/api/power/power_api_manager.cc (right):

https://codereview.chromium.org/12576018/diff/42001/chrome/browser/extensions...
chrome/browser/extensions/api/power/power_api_manager.cc:53:
create_blocker_function_ = base::Bind(&content::PowerSaveBlocker::Create);
On 2013/03/20 00:25:50, kalman wrote:
> if you like ternary

Done.

https://codereview.chromium.org/12576018/diff/42001/chrome/common/extensions/...
File chrome/common/extensions/api/power.idl (right):

https://codereview.chromium.org/12576018/diff/42001/chrome/common/extensions/...
chrome/common/extensions/api/power.idl:21: static void requestKeepAwake(optional
Level level);
On 2013/03/20 00:25:50, kalman wrote:
> Document what it means for level to be left out? Or just make it required?

Done (required).

[Daniel Erat, 2013-03-20 01:04:03]

TBR sky for chrome/*.gypi

[commit-bot: I haz the power, 2013-03-20 01:15:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/derat@chromium.org/12576018/54004

[commit-bot: I haz the power, 2013-03-20 01:15:57]

Presubmit check for 12576018-54004 failed and returned exit status 1.

INFO:root:Found 28 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/common/extensions/PRESUBMIT.py

** Presubmit Messages **
--tbr was specified, skipping OWNERS check

** Presubmit Warnings **
License must match:
.*? Copyright (\(c\) )?(2013|2012|2011|2010|2009|2008|2007|2006|2006-2008) The
Chromium Authors\. All rights reserved\.\n.*? Use of this source code is
governed by a BSD-style license that can be\n.*? found in the LICENSE file\.(?:
\*/)?\n
Found a bad license header in these files:
  chrome/common/extensions/docs/examples/api/power/background.js

Presubmit checks took 2.5s to calculate.

[commit-bot: I haz the power, 2013-03-20 01:32:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/derat@chromium.org/12576018/63002

[commit-bot: I haz the power, 2013-03-20 02:34:03]

Retried try job too often on win_rel for step(s) content_unittests, ipc_tests,
remoting_unittests, sql_unittests, sync_unit_tests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-03-20 02:37:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/derat@chromium.org/12576018/63002

[Daniel Erat, 2013-03-20 03:45:25]

https://codereview.chromium.org/12576018/diff/28001/chrome/common/extensions/...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/12576018/diff/28001/chrome/common/extensions/...
chrome/common/extensions/api/_permission_features.json:318: "extension_types":
["extension", "hosted_app", "packaged_app"]
On 2013/03/20 00:25:50, kalman wrote:
> On 2013/03/19 23:39:38, Daniel Erat wrote:
> > On 2013/03/19 21:16:39, kalman wrote:
> > > why hosted_app?
> > 
> > The intent is to permit e.g. a slide show app to prevent the screen from
> dimming
> > while displaying a presentation.
> 
> Ah, cool. Let's include platform_app too, then.

Whoops, I missed this.  I'll add it in a second change, if that's okay.

Is there documentation somewhere about what exactly platform_app refers to?  I
think I understand hosted vs. packaged, but I'm left guessing about "packaged"
-- does it refer to v1 apps?

[commit-bot: I haz the power, 2013-03-20 12:18:28]

Change committed as 189253

[not at google - send to devlin, 2013-03-20 14:58:08]

https://codereview.chromium.org/12576018/diff/28001/chrome/common/extensions/...
File chrome/common/extensions/api/_permission_features.json (right):

https://codereview.chromium.org/12576018/diff/28001/chrome/common/extensions/...
chrome/common/extensions/api/_permission_features.json:318: "extension_types":
["extension", "hosted_app", "packaged_app"]
On 2013/03/20 03:45:25, Daniel Erat wrote:
> On 2013/03/20 00:25:50, kalman wrote:
> > On 2013/03/19 23:39:38, Daniel Erat wrote:
> > > On 2013/03/19 21:16:39, kalman wrote:
> > > > why hosted_app?
> > > 
> > > The intent is to permit e.g. a slide show app to prevent the screen from
> > dimming
> > > while displaying a presentation.
> > 
> > Ah, cool. Let's include platform_app too, then.
> 
> Whoops, I missed this.  I'll add it in a second change, if that's okay.
> 
> Is there documentation somewhere about what exactly platform_app refers to?  I
> think I understand hosted vs. packaged, but I'm left guessing about "packaged"
> -- does it refer to v1 apps?

Agreed, we need to document this. Packaged is v1, platform is v2.