Added flag --extensions-in-action-box to split Action Box and Extensions in it.

Added flag --extensions-in-action-box to split Action Box and Extensions in it.

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

[not at google - send to devlin, 2012-08-14 04:09:53]

lgtm

http://codereview.chromium.org/10827268/diff/1/chrome/app/generated_resources...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10827268/diff/1/chrome/app/generated_resources...
chrome/app/generated_resources.grd:5486: +        Extensions in an Action box
s/an/the

might read better (there is only 1 action box).

http://codereview.chromium.org/10827268/diff/1/chrome/app/generated_resources...
chrome/app/generated_resources.grd:5488: +      <message
name="IDS_FLAGS_EXTENSIONS_IN_ACTION_BOX_DESCRIPTION" desc="Description of the
'Extensions in an Action Box' lab.">
ditto

http://codereview.chromium.org/10827268/diff/1/chrome/app/generated_resources...
chrome/app/generated_resources.grd:5489: +        Enables to show "Extensions an
Action Box".
Also reference that it has no effect unless the action box itself is enabled?

http://codereview.chromium.org/10827268/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/10827268/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_prefs.cc:1322:
extensions::switch_utils::IsExtensionsInActionBoxEnabled();
add a method like GetToolbarKey or something, which does this ternary logic,
call from here and in SetToolbarOrder?

http://codereview.chromium.org/10827268/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_prefs.cc:1430: bool default_value =
!(extensions_in_action_box_enabled);
parens unnecessary?

http://codereview.chromium.org/10827268/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_prefs.cc:1439: kBrowserActionPinned :
kBrowserActionVisible;
likewise, add GetToolbarVisibilityKey() perhaps, with "default_visibility =
!switch:utils::IsExtensionInActionBoxEnabled()"?

http://codereview.chromium.org/10827268/diff/1/chrome/common/extensions/exten...
File chrome/common/extensions/extension_switch_utils.h (right):

http://codereview.chromium.org/10827268/diff/1/chrome/common/extensions/exten...
chrome/common/extensions/extension_switch_utils.h:14: bool IsActionBoxEnabled();
do extensions care about this flag any more? Perhaps we should remove it so that
developers don't call it rather than IsExtensionsInActionBoxEnabled by accident.

[yefimt, 2012-08-14 20:59:07]

http://codereview.chromium.org/10827268/diff/1/chrome/app/generated_resources...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10827268/diff/1/chrome/app/generated_resources...
chrome/app/generated_resources.grd:5486: +        Extensions in an Action box
On 2012/08/14 04:09:53, kalman wrote:
> s/an/the
> 
> might read better (there is only 1 action box).

Done.

http://codereview.chromium.org/10827268/diff/1/chrome/app/generated_resources...
chrome/app/generated_resources.grd:5488: +      <message
name="IDS_FLAGS_EXTENSIONS_IN_ACTION_BOX_DESCRIPTION" desc="Description of the
'Extensions in an Action Box' lab.">
On 2012/08/14 04:09:53, kalman wrote:
> ditto

Done.

http://codereview.chromium.org/10827268/diff/1/chrome/app/generated_resources...
chrome/app/generated_resources.grd:5489: +        Enables to show "Extensions an
Action Box".
On 2012/08/14 04:09:53, kalman wrote:
> Also reference that it has no effect unless the action box itself is enabled?

Done.

http://codereview.chromium.org/10827268/diff/1/chrome/browser/extensions/exte...
File chrome/browser/extensions/extension_prefs.cc (right):

http://codereview.chromium.org/10827268/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_prefs.cc:1322:
extensions::switch_utils::IsExtensionsInActionBoxEnabled();
On 2012/08/14 04:09:53, kalman wrote:
> add a method like GetToolbarKey or something, which does this ternary logic,
> call from here and in SetToolbarOrder?

Done.

http://codereview.chromium.org/10827268/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_prefs.cc:1430: bool default_value =
!(extensions_in_action_box_enabled);
On 2012/08/14 04:09:53, kalman wrote:
> parens unnecessary?

Done.

http://codereview.chromium.org/10827268/diff/1/chrome/browser/extensions/exte...
chrome/browser/extensions/extension_prefs.cc:1439: kBrowserActionPinned :
kBrowserActionVisible;
On 2012/08/14 04:09:53, kalman wrote:
> likewise, add GetToolbarVisibilityKey() perhaps, with "default_visibility =
> !switch:utils::IsExtensionInActionBoxEnabled()"?

Done.

http://codereview.chromium.org/10827268/diff/1/chrome/common/extensions/exten...
File chrome/common/extensions/extension_switch_utils.h (right):

http://codereview.chromium.org/10827268/diff/1/chrome/common/extensions/exten...
chrome/common/extensions/extension_switch_utils.h:14: bool IsActionBoxEnabled();
On 2012/08/14 04:09:53, kalman wrote:
> do extensions care about this flag any more? Perhaps we should remove it so
that
> developers don't call it rather than IsExtensionsInActionBoxEnabled by
accident.

It is still used in location_bar_view

[commit-bot: I haz the power, 2012-08-14 20:59:50]

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

[commit-bot: I haz the power, 2012-08-14 20:59:57]

Presubmit check for 10827268-4001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 1.1s to calculate.

[yefimt, 2012-08-14 21:17:08]

adding
 cpu@ for chrome/app/generated_resources.grd
 ben@ for chrome/browser and chrome/common

[Ben Goodger (Google), 2012-08-14 21:37:27]

lgtm

[not at google - send to devlin, 2012-08-14 22:03:53]

http://codereview.chromium.org/10827268/diff/1/chrome/common/extensions/exten...
File chrome/common/extensions/extension_switch_utils.h (right):

http://codereview.chromium.org/10827268/diff/1/chrome/common/extensions/exten...
chrome/common/extensions/extension_switch_utils.h:14: bool IsActionBoxEnabled();
On 2012/08/14 20:59:07, yefimt wrote:
> On 2012/08/14 04:09:53, kalman wrote:
> > do extensions care about this flag any more? Perhaps we should remove it so
> that
> > developers don't call it rather than IsExtensionsInActionBoxEnabled by
> accident.
> 
> It is still used in location_bar_view

Yes but it's not really a concern of extensions in location_bar_view, but a
general concern of the UI. So I'm not sure if it should be in
extension_switch_utils.h

[yefimt, 2012-08-14 22:24:45]

On 2012/08/14 22:03:53, kalman wrote:
>
http://codereview.chromium.org/10827268/diff/1/chrome/common/extensions/exten...
> File chrome/common/extensions/extension_switch_utils.h (right):
> 
>
http://codereview.chromium.org/10827268/diff/1/chrome/common/extensions/exten...
> chrome/common/extensions/extension_switch_utils.h:14: bool
IsActionBoxEnabled();
> On 2012/08/14 20:59:07, yefimt wrote:
> > On 2012/08/14 04:09:53, kalman wrote:
> > > do extensions care about this flag any more? Perhaps we should remove it
so
> > that
> > > developers don't call it rather than IsExtensionsInActionBoxEnabled by
> > accident.
> > 
> > It is still used in location_bar_view
> 
> Yes but it's not really a concern of extensions in location_bar_view, but a
> general concern of the UI. So I'm not sure if it should be in
> extension_switch_utils.h

It is used in IsExtensionsInActionBoxEnabled() :)
Moving it will introduce extension_switch_utils dependence on some other file or
code duplication.

[not at google - send to devlin, 2012-08-14 22:30:07]

On 2012/08/14 22:24:45, yefimt wrote:
> On 2012/08/14 22:03:53, kalman wrote:
> >
>
http://codereview.chromium.org/10827268/diff/1/chrome/common/extensions/exten...
> > File chrome/common/extensions/extension_switch_utils.h (right):
> > 
> >
>
http://codereview.chromium.org/10827268/diff/1/chrome/common/extensions/exten...
> > chrome/common/extensions/extension_switch_utils.h:14: bool
> IsActionBoxEnabled();
> > On 2012/08/14 20:59:07, yefimt wrote:
> > > On 2012/08/14 04:09:53, kalman wrote:
> > > > do extensions care about this flag any more? Perhaps we should remove it
> so
> > > that
> > > > developers don't call it rather than IsExtensionsInActionBoxEnabled by
> > > accident.
> > > 
> > > It is still used in location_bar_view
> > 
> > Yes but it's not really a concern of extensions in location_bar_view, but a
> > general concern of the UI. So I'm not sure if it should be in
> > extension_switch_utils.h
> 
> It is used in IsExtensionsInActionBoxEnabled() :)
> Moving it will introduce extension_switch_utils dependence on some other file
or
> code duplication.

Check CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableActionBox) in
IsExtensionsInActionBoxEnabled? It's very common practice to write out flag
checks like that. extension_switch_utils is an anomaly there. I think here the
very small amount of code duplication (a few characters) is better than a flag
being in extension_switch_utils that shouldn't be.

[cpu_(ooo_6.6-7.5), 2012-08-14 22:48:23]

lgtm on the grd

[yefimt, 2012-08-14 23:17:55]

On 2012/08/14 22:30:07, kalman wrote:
> On 2012/08/14 22:24:45, yefimt wrote:
> > On 2012/08/14 22:03:53, kalman wrote:
> > >
> >
>
http://codereview.chromium.org/10827268/diff/1/chrome/common/extensions/exten...
> > > File chrome/common/extensions/extension_switch_utils.h (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/10827268/diff/1/chrome/common/extensions/exten...
> > > chrome/common/extensions/extension_switch_utils.h:14: bool
> > IsActionBoxEnabled();
> > > On 2012/08/14 20:59:07, yefimt wrote:
> > > > On 2012/08/14 04:09:53, kalman wrote:
> > > > > do extensions care about this flag any more? Perhaps we should remove
it
> > so
> > > > that
> > > > > developers don't call it rather than IsExtensionsInActionBoxEnabled by
> > > > accident.
> > > > 
> > > > It is still used in location_bar_view
> > > 
> > > Yes but it's not really a concern of extensions in location_bar_view, but
a
> > > general concern of the UI. So I'm not sure if it should be in
> > > extension_switch_utils.h
> > 
> > It is used in IsExtensionsInActionBoxEnabled() :)
> > Moving it will introduce extension_switch_utils dependence on some other
file
> or
> > code duplication.
> 
> Check CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableActionBox)
in
> IsExtensionsInActionBoxEnabled? It's very common practice to write out flag
> checks like that. extension_switch_utils is an anomaly there. I think here the
> very small amount of code duplication (a few characters) is better than a flag
> being in extension_switch_utils that shouldn't be.

Done

[yefimt, 2012-08-14 23:20:30]

Added
asvitkine@ for chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm
erg@ for chrome/browser/ui/gtk/location_bar_view_gtk.cc

[Elliot Glaysher, 2012-08-14 23:23:22]

one line gtk change lgtm

[Alexei Svitkine (slow), 2012-08-15 01:22:53]

chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm LGTM

[commit-bot: I haz the power, 2012-08-15 04:23:55]

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

[commit-bot: I haz the power, 2012-08-15 06:47:55]

Change committed as 151670

[Aaron Boodman, 2012-08-21 18:33:20]

Yefim, can you please remove the change to about_flags when you have a chance? I
don't want to enable-extensions-in-action-box right now.

[yefim, 2012-08-21 18:35:27]

Sure

On Tue, Aug 21, 2012 at 11:33 AM, <aa@chromium.org> wrote:

> Yefim, can you please remove the change to about_flags when you have a
> chance? I
> don't want to enable-extensions-in-action-**box right now.
>
>
http://codereview.chromium.**org/10827268/<http://codereview.chromium.org/108...
>