Properly propagate the current Chrome channel into the Feature system on the

Properly propagate the current Chrome channel into the Feature system on the
renderer.


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

[not at google - send to devlin, 2012-08-08 08:19:53]

This fixes the bug. I think it's generally the "right way" to do it, though I
think perhaps rather than skipping the permissions check when there's no channel
given, it gets checked as though it were stable (or just be set to stable by
default). This should make those test errors be caught properly, and also remove
those CHECKs that I find a bit worrying.

I'll do that tomorrow.

The problem with this change with respect to the bug is that I'm not sure how
easily cherry-pickable it is. A more cherry-pickable patch would be to just call
those SetForTesting methods then in a separate change clean it up properly.

[not at google - send to devlin, 2012-08-08 08:21:07]

Changing reviewer to mpcomplete, you wrote that renderer code initially?

[Matt Perry, 2012-08-08 19:17:26]

https://chromiumcodereview.appspot.com/10826199/diff/2001/chrome/common/exten...
File chrome/common/extensions/features/feature.h (right):

https://chromiumcodereview.appspot.com/10826199/diff/2001/chrome/common/exten...
chrome/common/extensions/features/feature.h:101:
SetCurrentChannel(original_channel_);
else: ClearCurrentChannel?

https://chromiumcodereview.appspot.com/10826199/diff/2001/chrome/renderer/ext...
File chrome/renderer/extensions/dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10826199/diff/2001/chrome/renderer/ext...
chrome/renderer/extensions/dispatcher.cc:305: }
So I take it from your review message that your plan to add a call to
SetCurrentChannel(STABLE) here? LGTM with that change.

[not at google - send to devlin, 2012-08-09 00:14:37]

Does it still lgty if I made that STABLE change at the Feature level?

https://chromiumcodereview.appspot.com/10826199/diff/2001/chrome/common/exten...
File chrome/common/extensions/features/feature.h (right):

https://chromiumcodereview.appspot.com/10826199/diff/2001/chrome/common/exten...
chrome/common/extensions/features/feature.h:101:
SetCurrentChannel(original_channel_);
On 2012/08/08 19:17:26, Matt Perry wrote:
> else: ClearCurrentChannel?

Oops. Thanks.

https://chromiumcodereview.appspot.com/10826199/diff/2001/chrome/renderer/ext...
File chrome/renderer/extensions/dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10826199/diff/2001/chrome/renderer/ext...
chrome/renderer/extensions/dispatcher.cc:305: }
On 2012/08/08 19:17:26, Matt Perry wrote:
> So I take it from your review message that your plan to add a call to
> SetCurrentChannel(STABLE) here? LGTM with that change.

Ah, that would work too.

I actually meant, changing it in Feature itself to default to STABLE rather than
defaulting to nothing. It would simply some of the other code (e.g. I wouldn't
need ClearCurrentChannel nor HasCurrentChannel), and force the tests which fail
on stable builds to pass. Well, at least, hopefully help.

[Matt Perry, 2012-08-09 00:24:42]

https://chromiumcodereview.appspot.com/10826199/diff/2001/chrome/renderer/ext...
File chrome/renderer/extensions/dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10826199/diff/2001/chrome/renderer/ext...
chrome/renderer/extensions/dispatcher.cc:305: }
On 2012/08/09 00:14:37, kalman wrote:
> On 2012/08/08 19:17:26, Matt Perry wrote:
> > So I take it from your review message that your plan to add a call to
> > SetCurrentChannel(STABLE) here? LGTM with that change.
> 
> Ah, that would work too.
> 
> I actually meant, changing it in Feature itself to default to STABLE rather
than
> defaulting to nothing. It would simply some of the other code (e.g. I wouldn't
> need ClearCurrentChannel nor HasCurrentChannel), and force the tests which
fail
> on stable builds to pass. Well, at least, hopefully help.

I don't understand how it helps you get rid of ClearCurrentChannel and
HasCurrentChannel.

[commit-bot: I haz the power, 2012-08-09 02:01:26]

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

[commit-bot: I haz the power, 2012-08-09 04:17:28]

Change committed as 150731