Add policies to control the disk cache size.

Add policies to control the disk cache size.

Two new policies added:
  DiskCacheSize (general cache size)
  MediaCacheSize (cache size for media files)

BUG=100549
TEST=unit_tests *Policy*


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

[pastarmovj, 2011-11-15 12:57:29]

Joao, can you please review this one. 
Especially whether I registered the policy correctly with your new workflow for
policy registration. Local tests seem to pass. Trybots are running now too.

[Joao da Silva, 2011-11-15 14:12:53]

The policy registration looks cool, but I think these policies should be user
policies instead of local_state. See inline.

http://codereview.chromium.org/8572006/diff/1/chrome/app/policy/policy_templa...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/8572006/diff/1/chrome/app/policy/policy_templa...
chrome/app/policy/policy_templates.json:598: 'supported_on': ['chrome.*:17-'],
These policies can be supported on chrome_os too with the changes proposed in
the other comments.

http://codereview.chromium.org/8572006/diff/1/chrome/app/policy/policy_templa...
chrome/app/policy/policy_templates.json:622: If you set this policy, <ph
name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> will use the provided
directory regardless whether the user has specified the '--media-cache-dir' flag
or not.
"will use the provided _directory_"? "--media-cache-dir"? :-)

http://codereview.chromium.org/8572006/diff/1/chrome/app/policy/policy_templa...
chrome/app/policy/policy_templates.json:627: 'label': '''Set media disk cache
size''',
Nit: the label defaults to the caption value, so if they're the same the label
can be left out.

http://codereview.chromium.org/8572006/diff/1/chrome/browser/browser_process_...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/8572006/diff/1/chrome/browser/browser_process_...
chrome/browser/browser_process_impl.cc:943:
local_state_->RegisterIntegerPref(prefs::kMediaCacheSize, 0);
See other comments; I think these policies shouldn't be local_state.

http://codereview.chromium.org/8572006/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/8572006/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_impl.cc:210: }
I think this policy should be user policy. The caches are stored in the profile
dir, and these policies could be used on chromeos too. It's also easier to
change in the future (e.g. to allow the user to configure the cache sizes
through some UI).

How about these changes:

1. don't read the command line here (remove the block above), and do the
equivalent logic in CommandLinePrefStore instead (it might require introducing
an IntSwitchToPreferenceMapEntry type)

2. register the prefs in the user prefs, with default value 0; this way, the
command line flags will override the default through the CommandLinePrefStore

3. the policies will override those prefs, if set;

4. in this function, just read the pref value from the profile PrefService (it
has to be passed as an argument to this function). It will either be the default
(0), the value from CommandLinePrefStore, or the policy value; in any case, it's
the right value. Also, don't wrap this reading with !OS_CHROMEOS.

[pastarmovj, 2011-11-16 15:43:22]

Thanks for reviewing this one! Made the suggested changes. PTAL.

[Joao da Silva, 2011-11-16 16:10:42]

Looks good! Only a bunch of nits. Please fix the text in the policy template.

http://codereview.chromium.org/8572006/diff/4001/chrome/app/policy/policy_tem...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/8572006/diff/4001/chrome/app/policy/policy_tem...
chrome/app/policy/policy_templates.json:622: If you set this policy, <ph
name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> will use the provided
directory regardless whether the user has specified the '--media-cache-dir' flag
or not.
I think this text is meant to say "will use the provided cache size". Also, the
related command line flag is --media-cache-size.

http://codereview.chromium.org/8572006/diff/4001/chrome/browser/prefs/command...
File chrome/browser/prefs/command_line_pref_store.cc (right):

http://codereview.chromium.org/8572006/diff/4001/chrome/browser/prefs/command...
chrome/browser/prefs/command_line_pref_store.cc:24: { switches::kDiskCacheDir,
prefs::kDiskCacheDir }
Nitty nit: comma at the end

http://codereview.chromium.org/8572006/diff/4001/chrome/browser/prefs/command...
chrome/browser/prefs/command_line_pref_store.cc:54: { switches::kMediaCacheSize,
prefs::kMediaCacheSize }
Same

http://codereview.chromium.org/8572006/diff/4001/chrome/browser/prefs/command...
File chrome/browser/prefs/command_line_pref_store.h (right):

http://codereview.chromium.org/8572006/diff/4001/chrome/browser/prefs/command...
chrome/browser/prefs/command_line_pref_store.h:45: static const
BooleanSwitchToPreferenceMapEntry boolean_switch_map_[];
Nit: please move this next to the other arrays

http://codereview.chromium.org/8572006/diff/4001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/8572006/diff/4001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:1586: 
Nit: remove newline at end of file

http://codereview.chromium.org/8572006/diff/4001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/8572006/diff/4001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.h:188: int* max_size);
Nit: one arg per line

http://codereview.chromium.org/8572006/diff/4001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/8572006/diff/4001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:2459: PrefService::UNSYNCABLE_PREF);
Nit: indent of 2nd and 3rd args on these 3 prefs.

[pastarmovj, 2011-11-17 13:10:10]

Addressed comments :)

http://codereview.chromium.org/8572006/diff/4001/chrome/app/policy/policy_tem...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/8572006/diff/4001/chrome/app/policy/policy_tem...
chrome/app/policy/policy_templates.json:622: If you set this policy, <ph
name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> will use the provided
directory regardless whether the user has specified the '--media-cache-dir' flag
or not.
On 2011/11/16 16:10:42, Joao da Silva wrote:
> I think this text is meant to say "will use the provided cache size". Also,
the
> related command line flag is --media-cache-size.

Done.

http://codereview.chromium.org/8572006/diff/4001/chrome/browser/prefs/command...
File chrome/browser/prefs/command_line_pref_store.cc (right):

http://codereview.chromium.org/8572006/diff/4001/chrome/browser/prefs/command...
chrome/browser/prefs/command_line_pref_store.cc:24: { switches::kDiskCacheDir,
prefs::kDiskCacheDir }
On 2011/11/16 16:10:42, Joao da Silva wrote:
> Nitty nit: comma at the end

I never got the idea why should we end those lists with comma, but apparently
there is some deep reason beyond my understanding :)

http://codereview.chromium.org/8572006/diff/4001/chrome/browser/prefs/command...
chrome/browser/prefs/command_line_pref_store.cc:54: { switches::kMediaCacheSize,
prefs::kMediaCacheSize }
On 2011/11/16 16:10:42, Joao da Silva wrote:
> Same

Done.

http://codereview.chromium.org/8572006/diff/4001/chrome/browser/prefs/command...
File chrome/browser/prefs/command_line_pref_store.h (right):

http://codereview.chromium.org/8572006/diff/4001/chrome/browser/prefs/command...
chrome/browser/prefs/command_line_pref_store.h:45: static const
BooleanSwitchToPreferenceMapEntry boolean_switch_map_[];
On 2011/11/16 16:10:42, Joao da Silva wrote:
> Nit: please move this next to the other arrays

Done. Extra treat commented the section too :)

http://codereview.chromium.org/8572006/diff/4001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/8572006/diff/4001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:1586: 
On 2011/11/16 16:10:42, Joao da Silva wrote:
> Nit: remove newline at end of file

Done.

http://codereview.chromium.org/8572006/diff/4001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/8572006/diff/4001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.h:188: int* max_size);
On 2011/11/16 16:10:42, Joao da Silva wrote:
> Nit: one arg per line

True I did ausschneiden+einfu"gen here without taking care of the definition any
further :)

http://codereview.chromium.org/8572006/diff/4001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/8572006/diff/4001/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:2459: PrefService::UNSYNCABLE_PREF);
On 2011/11/16 16:10:42, Joao da Silva wrote:
> Nit: indent of 2nd and 3rd args on these 3 prefs.

Hehe true i removed an _ there and those slipped :)

[Joao da Silva, 2011-11-17 13:17:43]

lgtm

[commit-bot: I haz the power, 2011-11-17 15:00:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pastarmovj@chromium.org/8572006/11002

[commit-bot: I haz the power, 2011-11-17 15:00:29]

Presubmit check for 8572006-11002 failed and returned exit status 1.

Running presubmit commit checks ...
Finished checking
/mnt/data/b/commit-queue/workdir/chromium/chrome/app/policy/policy_templates.json.
0 errors, 0 warnings.

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/profiles/profile_impl.cc,chrome/browser/profiles/profile_impl.h

Presubmit checks took 2.2s to calculate.

[pastarmovj, 2011-11-17 15:11:44]

Hi Miranda, 
can you please give me owner approval for this small CL for the stuff in
chrome/browser/profiles.
Thanks!

[Miranda Callahan, 2011-11-17 15:25:58]

On 2011/11/17 15:11:44, pastarmovj wrote:
> Hi Miranda, 
> can you please give me owner approval for this small CL for the stuff in
> chrome/browser/profiles.
> Thanks!

Sure -- LGTM for profiles!

[commit-bot: I haz the power, 2011-11-18 09:19:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pastarmovj@chromium.org/8572006/11002

[commit-bot: I haz the power, 2011-11-18 10:21:52]

Change committed as 110681