Allow extension APIs to be restricted to a subset of Chrome channels using the

Allow extension APIs to be restricted to a subset of Chrome channels using the
Feature system. For example, restricting an API to "dev" means having it
available on trunk/canary/dev but not beta/stable.


BUG=114231
TEST=unit_tests --gtest_filter=*Feature*


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

[not at google - send to devlin, 2012-04-11 23:05:54]

In the meantime: I'm going to play around with compiling a beta branch or
something and making sure everything still works.

https://chromiumcodereview.appspot.com/10054035/diff/1/chrome/common/extensio...
File chrome/common/extensions/feature.cc (left):

https://chromiumcodereview.appspot.com/10054035/diff/1/chrome/common/extensio...
chrome/common/extensions/feature.cc:203: return "";
Unrelated cleanup, but clang won't compile if there are cases missing in a
switch, so specifying a default case is actually preventing the compile-time
check. FYI.

[Aaron Boodman, 2012-04-11 23:35:52]

https://chromiumcodereview.appspot.com/10054035/diff/1/chrome/common/extensio...
File chrome/common/extensions/feature.cc (left):

https://chromiumcodereview.appspot.com/10054035/diff/1/chrome/common/extensio...
chrome/common/extensions/feature.cc:203: return "";
On 2012/04/11 23:05:54, kalman wrote:
> Unrelated cleanup, but clang won't compile if there are cases missing in a
> switch, so specifying a default case is actually preventing the compile-time
> check. FYI.

Neat. Thanks for the info.

https://chromiumcodereview.appspot.com/10054035/diff/1/chrome/common/extensio...
File chrome/common/extensions/feature.cc (right):

https://chromiumcodereview.appspot.com/10054035/diff/1/chrome/common/extensio...
chrome/common/extensions/feature.cc:36: channels["trunk"] =
VersionInfo::CHANNEL_UNKNOWN;
.insert("\n")

https://chromiumcodereview.appspot.com/10054035/diff/1/chrome/common/extensio...
File chrome/common/extensions/feature.h (right):

https://chromiumcodereview.appspot.com/10054035/diff/1/chrome/common/extensio...
chrome/common/extensions/feature.h:161: chrome::VersionInfo::Channel channel_;
Sucks to have a separate value here for every single instance just to support
the testing case.

Can we make this test value be static instead?

https://chromiumcodereview.appspot.com/10054035/diff/1/chrome/common/extensio...
File chrome/common/extensions/feature_unittest.cc (right):

https://chromiumcodereview.appspot.com/10054035/diff/1/chrome/common/extensio...
chrome/common/extensions/feature_unittest.cc:437: // Default supported channel
(stable).
I thought the default was going to be trunk.

[not at google - send to devlin, 2012-04-12 00:05:21]

https://chromiumcodereview.appspot.com/10054035/diff/1/chrome/common/extensio...
File chrome/common/extensions/feature.cc (right):

https://chromiumcodereview.appspot.com/10054035/diff/1/chrome/common/extensio...
chrome/common/extensions/feature.cc:36: channels["trunk"] =
VersionInfo::CHANNEL_UNKNOWN;
On 2012/04/11 23:35:53, Aaron Boodman wrote:
> .insert("\n")

Done.

https://chromiumcodereview.appspot.com/10054035/diff/1/chrome/common/extensio...
File chrome/common/extensions/feature.h (right):

https://chromiumcodereview.appspot.com/10054035/diff/1/chrome/common/extensio...
chrome/common/extensions/feature.h:161: chrome::VersionInfo::Channel channel_;
On 2012/04/11 23:35:53, Aaron Boodman wrote:
> Sucks to have a separate value here for every single instance just to support
> the testing case.
> 
> Can we make this test value be static instead?

Yeah, nicer.

https://chromiumcodereview.appspot.com/10054035/diff/1/chrome/common/extensio...
File chrome/common/extensions/feature_unittest.cc (right):

https://chromiumcodereview.appspot.com/10054035/diff/1/chrome/common/extensio...
chrome/common/extensions/feature_unittest.cc:437: // Default supported channel
(stable).
On 2012/04/11 23:35:53, Aaron Boodman wrote:
> I thought the default was going to be trunk.

Mm, we went back and forth, I can't remember. Yeah you're right, we did.

but then I got lazy and made it stable, because that'd require updating a whole
bunch of json files.

This is me justifying my laziness, but this the way it works now and it seems ok
to leave it that way.

Further justification of laziness: in a way it's also consistent with
experimental APIs, you need to explicitly mark something as experimental to make
it experimental.  It's stable by default.

[Aaron Boodman, 2012-04-12 00:45:48]

On Wed, Apr 11, 2012 at 5:05 PM,  <kalman@chromium.org> wrote:
>
>
https://chromiumcodereview.appspot.com/10054035/diff/1/chrome/common/extensio...
> File chrome/common/extensions/feature.cc (right):
>
>
https://chromiumcodereview.appspot.com/10054035/diff/1/chrome/common/extensio...
> chrome/common/extensions/feature.cc:36: channels["trunk"] =
> VersionInfo::CHANNEL_UNKNOWN;
> On 2012/04/11 23:35:53, Aaron Boodman wrote:
>>
>> .insert("\n")
>
>
> Done.
>
>
>
https://chromiumcodereview.appspot.com/10054035/diff/1/chrome/common/extensio...
> File chrome/common/extensions/feature.h (right):
>
>
https://chromiumcodereview.appspot.com/10054035/diff/1/chrome/common/extensio...
> chrome/common/extensions/feature.h:161: chrome::VersionInfo::Channel
> channel_;
> On 2012/04/11 23:35:53, Aaron Boodman wrote:
>>
>> Sucks to have a separate value here for every single instance just to
>
> support
>>
>> the testing case.
>
>
>> Can we make this test value be static instead?
>
>
> Yeah, nicer.
>
>
>
https://chromiumcodereview.appspot.com/10054035/diff/1/chrome/common/extensio...
> File chrome/common/extensions/feature_unittest.cc (right):
>
>
https://chromiumcodereview.appspot.com/10054035/diff/1/chrome/common/extensio...
> chrome/common/extensions/feature_unittest.cc:437: // Default supported
> channel (stable).
> On 2012/04/11 23:35:53, Aaron Boodman wrote:
>>
>> I thought the default was going to be trunk.
>
>
> Mm, we went back and forth, I can't remember. Yeah you're right, we did.
>
> but then I got lazy and made it stable, because that'd require updating
> a whole bunch of json files.
>
> This is me justifying my laziness, but this the way it works now and it
> seems ok to leave it that way.
>
> Further justification of laziness: in a way it's also consistent with
> experimental APIs, you need to explicitly mark something as experimental
> to make it experimental.  It's stable by default.

Sorry, I'd just rather it be consistent with the rest of the system,
which defaults to "nothing allowed". It's just two JSON files you'll
have to update (plus probably many unit tests).

- a

[not at google - send to devlin, 2012-04-12 01:08:42]

Ok, default supported channel changed to trunk.

There weren't any unit tests to update because the channel reported in them is
trunk.  We'll need to add it to every API json that is updated to use the
feature system, though.

[Aaron Boodman, 2012-04-12 02:17:37]

tanks!
lgtm

[commit-bot: I haz the power, 2012-04-12 02:21:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/10054035/3004

[commit-bot: I haz the power, 2012-04-12 04:45:50]

Change committed as 131929