Initial change that allows to disable bookmarks in Chrome for Chrome OS (BWSI...

Initial change that allows to disable bookmarks in Chrome for Chrome OS (BWSI mode). Added disable-bookmarks flag and disabled some UI elements.

BUG=chromium-os:4302
TEST=Run Chrome build for Chrome OS with --disable-bookmarks flag and verify that the corresponding UI is disabled. Verify that BWSI mode runs with this flag.

[whywhat, 2010-08-13 14:25:21]

PTAL!

[oshima, 2010-08-16 21:51:30]

lgtm

[sky, 2010-08-17 17:58:39]

http://codereview.chromium.org/3122014/diff/1/2
File chrome/app/chrome_dll_main.cc (right):

http://codereview.chromium.org/3122014/diff/1/2#newcode575
chrome/app/chrome_dll_main.cc:575:
singleton_command_line->AppendSwitch(switches::kDisableBookmarks);
How come we're modifying the command line switches for these? Why not just set a
boolean some where? I would be inclined to modify defaults for these sort of
things.

http://codereview.chromium.org/3122014/diff/1/3
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/3122014/diff/1/3#newcode1094
chrome/browser/browser.cc:1094: command_updater_.UpdateCommandEnabled(
Slews of ifdefs like this make the code really hard to follow. It would be
better to centralize this code in a common place. If you modify defaults to
contain the bookmarks_enabled boolean, then all this could would just be:
 command_updater_.UpdateCommandEnabled(..., defaults::bookmarks_enabled).

[Dmitry Polukhin, 2010-08-18 08:37:51]

Anton is on vacation and asked me to land this CL for him.

Please see my cloned CL: http://codereview.chromium.org/3191007

http://codereview.chromium.org/3122014/diff/1/2
File chrome/app/chrome_dll_main.cc (right):

http://codereview.chromium.org/3122014/diff/1/2#newcode575
chrome/app/chrome_dll_main.cc:575:
singleton_command_line->AppendSwitch(switches::kDisableBookmarks);
On 2010/08/17 17:58:39, sky wrote:
> How come we're modifying the command line switches for these? Why not just set
a
> boolean some where? I would be inclined to modify defaults for these sort of
> things.

As far as I understand we need to modify switches to easy propagate them to all
child Chrome processes. Having this as a command line option has advantage that
it can be activated very easy and not only together with BWSI.

http://codereview.chromium.org/3122014/diff/1/3
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/3122014/diff/1/3#newcode1094
chrome/browser/browser.cc:1094: command_updater_.UpdateCommandEnabled(
On 2010/08/17 17:58:39, sky wrote:
> Slews of ifdefs like this make the code really hard to follow. It would be
> better to centralize this code in a common place. If you modify defaults to
> contain the bookmarks_enabled boolean, then all this could would just be:
>  command_updater_.UpdateCommandEnabled(..., defaults::bookmarks_enabled).
> 

I don't see "defaults::", I see only "browser_defaults::" that are all real
consts but we need runtime variable.

[sky, 2010-08-18 16:09:35]

On Wed, Aug 18, 2010 at 1:37 AM,  <dpolukhin@chromium.org> wrote:
> Anton is on vacation and asked me to land this CL for him.
>
> Please see my cloned CL: http://codereview.chromium.org/3191007
>
>
> http://codereview.chromium.org/3122014/diff/1/2
> File chrome/app/chrome_dll_main.cc (right):
>
> http://codereview.chromium.org/3122014/diff/1/2#newcode575
> chrome/app/chrome_dll_main.cc:575:
> singleton_command_line->AppendSwitch(switches::kDisableBookmarks);
> On 2010/08/17 17:58:39, sky wrote:
>>
>> How come we're modifying the command line switches for these? Why not
>
> just set a
>>
>> boolean some where? I would be inclined to modify defaults for these
>
> sort of
>>
>> things.
>
> As far as I understand we need to modify switches to easy propagate them
> to all child Chrome processes.

These command line switches don't effect the renderers though, right?

> Having this as a command line option has
> advantage that it can be activated very easy and not only together with
> BWSI.

Certainly, but there are other costs associated with it. Ben wrote an
email on this a while back. Here it is:

> I think we should clarify our command line switches policy. I've noticed over
time command line
> switches being added to control, disable or enable features to suit user
preference. We should not
> be doing this. It adds to the configuration matrix we end up having to support
in testing and in code.
>
> In my mind these are the three reasons to have command line switches:
>
> - As a way to develop an experimental feature without impacting everyone else.
The idea is the feature
> is ultimately turned on by default and the switch removed.
> - As an internal API when starting child processes.
> - As an aid to developing/debugging - e.g. things like --single-process,
--user-data-dir, --lang, etc.
>
> In short, command line switches should not become our version of about:config.
In addition, please consider
> cleaning up code that uses a switch that doesn't fall into one of these
categories.


> http://codereview.chromium.org/3122014/diff/1/3
> File chrome/browser/browser.cc (right):
>
> http://codereview.chromium.org/3122014/diff/1/3#newcode1094
> chrome/browser/browser.cc:1094: command_updater_.UpdateCommandEnabled(
> On 2010/08/17 17:58:39, sky wrote:
>>
>> Slews of ifdefs like this make the code really hard to follow. It
>
> would be
>>
>> better to centralize this code in a common place. If you modify
>
> defaults to
>>
>> contain the bookmarks_enabled boolean, then all this could would just
>
> be:
>>
>>  command_updater_.UpdateCommandEnabled(...,
>
> defaults::bookmarks_enabled).
>
>
> I don't see "defaults::", I see only "browser_defaults::" that are all
> real consts but we need runtime variable.

Yes, I meant browser_defaults. You could make some values non-const,
or use friends to only allow browser_init/main to set values.

  -Scott