With enable-action-box add extensions to BrowserActionContainer only if they have preference kBrows…

Added browser_action_pinned extension preference and modified BrowserActionContainer to show extensions based on it.

BUG=125307
TEST=make sure you have extensions that show icon in browser action area; add --enable_action_box to command line; make sure browser doesn't show any extensions in browser actions area; go to extensions setting, try show/hide extensions

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

[Aaron Boodman, 2012-05-14 18:36:17]

It seems like it would be cleaner/easier to re-use the existing
Get/SetBrowserActionVisibility methods, and the existing notification type.

The only difference would be inside their implementations. If
--enable-action-box is present, then use the kBrowserActionPinned preference and
default to false (not visible). If --enable-action-box is not present, then use
kBrowserActionVisibility preference and default to true (is visible).

That way we can even use the existing 'show browser action' button in
chrome://extensions/ to manually run the new code.

http://codereview.chromium.org/10378117/diff/8002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/10378117/diff/8002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:118: // Whether the browser action
is visible in the toolbar.
Change this to:

Whether the browser action is pinned in the toolbar. This will eventually
replace kBrowserActionVisible.

http://codereview.chromium.org/10378117/diff/8002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:120: const char
kBrowserActionPined[] = "browser_action_pined";
Spelling here and elsewhere: pinned (two 'n's).

http://codereview.chromium.org/10378117/diff/8002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:1280: return true;
return false. Default to not-pinned.

http://codereview.chromium.org/10378117/diff/8002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:1283: if
(command_line.HasSwitch(switches::kEnableActionBox)) {
Do we expect this method to be called when the switch is not present? It seems
like no. If so, you can just skip checking this explicitly and default to false.

[yefimt, 2012-05-14 18:50:45]

http://codereview.chromium.org/10378117/diff/8002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/10378117/diff/8002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:118: // Whether the browser action
is visible in the toolbar.
On 2012/05/14 18:36:17, Aaron Boodman wrote:
> Change this to:
> 
> Whether the browser action is pinned in the toolbar. This will eventually
> replace kBrowserActionVisible.

Done.

http://codereview.chromium.org/10378117/diff/8002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:120: const char
kBrowserActionPined[] = "browser_action_pined";
On 2012/05/14 18:36:17, Aaron Boodman wrote:
> Spelling here and elsewhere: pinned (two 'n's).

Done.

http://codereview.chromium.org/10378117/diff/8002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:1280: return true;
On 2012/05/14 18:36:17, Aaron Boodman wrote:
> return false. Default to not-pinned.

Done.

http://codereview.chromium.org/10378117/diff/8002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:1283: if
(command_line.HasSwitch(switches::kEnableActionBox)) {
We do, could be call from ExtensionToolbarModel when switch is set or not
On 2012/05/14 18:36:17, Aaron Boodman wrote:
> Do we expect this method to be called when the switch is not present? It seems
> like no. If so, you can just skip checking this explicitly and default to
false.

[yefimt, 2012-05-14 19:34:39]

all done

[Aaron Boodman, 2012-05-14 20:18:13]

http://codereview.chromium.org/10378117/diff/3003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/10378117/diff/3003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:1280: bool enable_action_box =
command_line.HasSwitch(switches::kEnableActionBox);
Nit: variable should not have typically have verb names. So action_box_enabled
would be better.

http://codereview.chromium.org/10378117/diff/3003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:1281: if (!extension_prefs) {
Nit: If an if statement is less than two lines total, it does not require
braces.

http://codereview.chromium.org/10378117/diff/3003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:1281: if (!extension_prefs) {
You can simplify this logic somewhat:

bool default_value = !action_box_enabled;

ExtensionPrefs* prefs = ...
if (!prefs)
  return default_value;

bool value;
if (!prefs->GetBoolean(name, &value))
  return default_value;

return value;

http://codereview.chromium.org/10378117/diff/3003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:1307: bool enable_action_box =
command_line.HasSwitch(switches::kEnableActionBox);
Since you are checking this in multiple places, can you please add a function to
extension_switch_utils.*, and call that function instead?

http://codereview.chromium.org/10378117/diff/3003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.h (left):

http://codereview.chromium.org/10378117/diff/3003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.h:259: bool
GetBrowserActionVisibility(const Extension* extension);
Thanks for fixing this!

[yefimt, 2012-05-14 21:07:36]

http://codereview.chromium.org/10378117/diff/3003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/10378117/diff/3003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:1280: bool enable_action_box =
command_line.HasSwitch(switches::kEnableActionBox);
On 2012/05/14 20:18:13, Aaron Boodman wrote:
> Nit: variable should not have typically have verb names. So action_box_enabled
> would be better.

Done.

http://codereview.chromium.org/10378117/diff/3003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:1281: if (!extension_prefs) {
On 2012/05/14 20:18:13, Aaron Boodman wrote:
> Nit: If an if statement is less than two lines total, it does not require
> braces.

Done.

http://codereview.chromium.org/10378117/diff/3003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:1281: if (!extension_prefs) {
On 2012/05/14 20:18:13, Aaron Boodman wrote:
> Nit: If an if statement is less than two lines total, it does not require
> braces.

Done.

http://codereview.chromium.org/10378117/diff/3003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_prefs.cc:1281: if (!extension_prefs) {
On 2012/05/14 20:18:13, Aaron Boodman wrote:
> Nit: If an if statement is less than two lines total, it does not require
> braces.

Done.

http://codereview.chromium.org/10378117/diff/3003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.h (left):

http://codereview.chromium.org/10378117/diff/3003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.h:259: bool
GetBrowserActionVisibility(const Extension* extension);
On 2012/05/14 20:18:13, Aaron Boodman wrote:
> Thanks for fixing this!

np

[Aaron Boodman, 2012-05-14 21:14:52]

LGTM

[Aaron Boodman, 2012-05-14 21:15:41]

+jhawkins for webui

[James Hawkins, 2012-05-14 21:44:35]

webui LGTM

[yefimt, 2012-05-15 00:09:19]

+asvitkine for cocoa

[yefimt, 2012-05-15 00:12:54]

+ sky@ for chrome/browser/ui/views/browser_actions_container_browsertest.cc

[sky, 2012-05-15 00:20:58]

LGTM

[Alexei Svitkine (slow), 2012-05-15 00:24:32]

cocoa/ LGTM

Please add BUG= and TEST= lines to your CL description and fix the spelling of
"pinned" in the description.

[commit-bot: I haz the power, 2012-05-15 03:09:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yefim@chromium.org/10378117/6018

[Aaron Boodman, 2012-05-15 03:30:16]

cc:kalman

Yefim, kalman@ is also working on (a different aspect of) the UI. Can you two
please cc each other on your reviews so that you can be aware of where each
other are?

[commit-bot: I haz the power, 2012-05-15 05:21:34]

Change committed as 137081