Added group policies to enable/disable importing of data from other browsers.

Added group policies to enable/disable importing of data from other browsers.
These are now proper user policies and disable/enable import settings even after the first run. There are separate policies for bookmarks, history, saved passwords, search engines and homepage.
BUG=71080
TEST=


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

[Mattias Nissler (ping if slow), 2011-07-18 15:02:30]

+James for webui settings stuff

Pretty good for your first patch. Only major thing I'd like to have resolved is
that we write master_preferences settings to user prefs.

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

http://codereview.chromium.org/7390027/diff/1/chrome/app/policy/policy_templa...
chrome/app/policy/policy_templates.json:1604: 'desc': '''This policy forces
bookmarks to be imported from the current default browser if enabled. If
disabled, no bookmarks are imported. If it is not configured, the default
behaviour is used.''',
It might be worth mentioning that this also affects the import dialog.

http://codereview.chromium.org/7390027/diff/1/chrome/browser/first_run/first_...
File chrome/browser/first_run/first_run.cc (left):

http://codereview.chromium.org/7390027/diff/1/chrome/browser/first_run/first_...
chrome/browser/first_run/first_run.cc:102: 
Please don't remove white space before comments.

http://codereview.chromium.org/7390027/diff/1/chrome/browser/first_run/first_...
chrome/browser/first_run/first_run.cc:498: 
why did you remove this?

http://codereview.chromium.org/7390027/diff/1/chrome/browser/first_run/first_...
chrome/browser/first_run/first_run.cc:500: 
and this?

http://codereview.chromium.org/7390027/diff/1/chrome/browser/first_run/first_...
File chrome/browser/first_run/first_run.cc (right):

http://codereview.chromium.org/7390027/diff/1/chrome/browser/first_run/first_...
chrome/browser/first_run/first_run.cc:548: 
Only one blank line before comments.

http://codereview.chromium.org/7390027/diff/1/chrome/browser/first_run/first_...
chrome/browser/first_run/first_run.cc:553: // policy
Missing period.

http://codereview.chromium.org/7390027/diff/1/chrome/browser/first_run/first_...
chrome/browser/first_run/first_run.cc:553: // policy
This also means that the default selection in the Import dialog corresponds to
master_preferences, correct? I thought we wanted to keep current behavior of
just selecting all of the checkboxes?

http://codereview.chromium.org/7390027/diff/1/chrome/browser/policy/managed_p...
File chrome/browser/policy/managed_prefs_banner_base.cc (right):

http://codereview.chromium.org/7390027/diff/1/chrome/browser/policy/managed_p...
chrome/browser/policy/managed_prefs_banner_base.cc:85:
AddUserPref(prefs::kImportSavedPasswords);
You may want to sync your working tree, since this file is gone by now :)

http://codereview.chromium.org/7390027/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/import_data_overlay.js (right):

http://codereview.chromium.org/7390027/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/import_data_overlay.js:92:
self.import_history_pref.name,
Indentation should be +4 spaces after a line break

http://codereview.chromium.org/7390027/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/import_data_overlay.js:95:
self.import_history_pref,
same here, but it'll probably fit on the previous line if you reindent the block

http://codereview.chromium.org/7390027/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/import_data_overlay.js:150: 
only one blank line

http://codereview.chromium.org/7390027/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/import_data_overlay.js:189: 
don't need this blank line.

http://codereview.chromium.org/7390027/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/import_data_overlay.js:192: for (var i = 0; i <
checkboxes.length; i++) {
In this block, it might be simpler to just grab the right pref object (maybe
using this.importPrefs[i]) and call a function that will evaluate managed and
value and change the checkbox state.

http://codereview.chromium.org/7390027/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/import_data_overlay.js:193: var enabled =
this.shouldEnableCheckbox_(checkboxes[i]) && sensitive;
+2 space indentation.

http://codereview.chromium.org/7390027/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/import_data_overlay.js:228: var enabled =
!importPrefs[i].managed
&& goes on this line

http://codereview.chromium.org/7390027/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/import_data_overlay.js:236: * Handles change
event of the different import preferences
missing period, also 2 lines below.

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

http://codereview.chromium.org/7390027/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:2207: PrefService::UNSYNCABLE_PREF);
blank line before comment.

http://codereview.chromium.org/7390027/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/7390027/diff/1/chrome/common/pref_names.cc#new...
chrome/common/pref_names.cc:1302: const char kImportBookmarks[] =
"import_bookmarks";
These are in the wrong section (this is LOCAL_STATE) move up to PROFILE

[simo, 2011-07-19 12:24:47]

http://codereview.chromium.org/7390027/diff/1/chrome/browser/first_run/first_...
File chrome/browser/first_run/first_run.cc (right):

http://codereview.chromium.org/7390027/diff/1/chrome/browser/first_run/first_...
chrome/browser/first_run/first_run.cc:553: // policy
On 2011/07/18 15:02:30, Mattias Nissler wrote:
> This also means that the default selection in the Import dialog corresponds to
> master_preferences, correct? I thought we wanted to keep current behavior of
> just selecting all of the checkboxes?

It does select all the checkboxes if the corresponding policies are disabled. If
any of them are enabled, the corresponding checkbox will be checked/unchecked
according to the value of the policy and greyed out.

http://codereview.chromium.org/7390027/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/import_data_overlay.js (left):

http://codereview.chromium.org/7390027/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/import_data_overlay.js:45: };
just noticed that I removed this by mistake so I'll put this back in with the
other changes.

[Mattias Nissler (ping if slow), 2011-07-20 09:28:31]

getting closer...

http://codereview.chromium.org/7390027/diff/1/chrome/browser/first_run/first_...
File chrome/browser/first_run/first_run.cc (right):

http://codereview.chromium.org/7390027/diff/1/chrome/browser/first_run/first_...
chrome/browser/first_run/first_run.cc:553: // policy
On 2011/07/19 12:24:47, simo wrote:
> On 2011/07/18 15:02:30, Mattias Nissler wrote:
> > This also means that the default selection in the Import dialog corresponds
to
> > master_preferences, correct? I thought we wanted to keep current behavior of
> > just selecting all of the checkboxes?
> 
> It does select all the checkboxes if the corresponding policies are disabled.
If
> any of them are enabled, the corresponding checkbox will be checked/unchecked
> according to the value of the policy and greyed out.

Yes, but what happens if no policy is present? Won't it initialize the
checkboxes to what master_preferences says?

http://codereview.chromium.org/7390027/diff/6001/chrome/browser/first_run/fir...
chrome/browser/first_run/first_run.cc:561: items = items | importer::FAVORITES;
align with previous line, also below

http://codereview.chromium.org/7390027/diff/6001/chrome/browser/resources/opt...
File chrome/browser/resources/options/import_data_overlay.js (right):

http://codereview.chromium.org/7390027/diff/6001/chrome/browser/resources/opt...
chrome/browser/resources/options/import_data_overlay.js:124:
browserProfile[importOptions[i]] : false);
maybe break the line after the initial = and then the second line will maybe fit
without breaking after the ?

If you don't like this, then you need to fix alignment: one space less in 123
and 4 spaces indentation from 123 to 124

http://codereview.chromium.org/7390027/diff/6001/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

http://codereview.chromium.org/7390027/diff/6001/chrome/common/pref_names.h#n...
chrome/common/pref_names.h:495: extern const char kImportBookmarks[];
Please keep declaration order here in sync with definition order in
pref_names.cc

[Mattias Nissler (ping if slow), 2011-07-25 11:12:36]

mostly nits, I'm basically happy with this provided you're reasonably confident
we have solved the logic puzzle in the right way :)

http://codereview.chromium.org/7390027/diff/9001/chrome/browser/first_run/fir...
File chrome/browser/first_run/first_run.cc (right):

http://codereview.chromium.org/7390027/diff/9001/chrome/browser/first_run/fir...
chrome/browser/first_run/first_run.cc:610: int import_type,
should be enum ImportItem

http://codereview.chromium.org/7390027/diff/9001/chrome/browser/first_run/fir...
chrome/browser/first_run/first_run.cc:632: }
newline before comment

http://codereview.chromium.org/7390027/diff/9001/chrome/browser/importer/impo...
File chrome/browser/importer/importer_host.cc (right):

http://codereview.chromium.org/7390027/diff/9001/chrome/browser/importer/impo...
chrome/browser/importer/importer_host.cc:116: // Make sure only items that were
not disabled by policy are imported.
Does this work in case of recommended policy? Policy could say no, but the user
should still be able to override.

http://codereview.chromium.org/7390027/diff/9001/chrome/browser/resources/opt...
File chrome/browser/resources/options/import_data_overlay.js (right):

http://codereview.chromium.org/7390027/diff/9001/chrome/browser/resources/opt...
chrome/browser/resources/options/import_data_overlay.js:168:
Preferences.clearPref(importPrefs[i], undefined);
indentation

[James Hawkins, 2011-08-01 19:41:51]

http://codereview.chromium.org/7390027/diff/13001/chrome/browser/first_run/fi...
File chrome/browser/first_run/first_run.cc (right):

http://codereview.chromium.org/7390027/diff/13001/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:228: if (value) {
Optional, but recommended: Don't use braces for one line logic blocks.

http://codereview.chromium.org/7390027/diff/13001/chrome/browser/first_run/fi...
File chrome/browser/first_run/first_run.h (right):

http://codereview.chromium.org/7390027/diff/13001/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.h:156: static void SetImportItem(PrefService*
user_prefs,
Move to an unnamed namespace in the implementation and remove the static
keyword.

http://codereview.chromium.org/7390027/diff/13001/chrome/browser/resources/op...
File chrome/browser/resources/options/import_data_overlay.js (right):

http://codereview.chromium.org/7390027/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/import_data_overlay.js:91: var importOptions =
['history', 'favorites', 'passwords', 'search'];
Why did you change this?  This is fragile and doesn't scale well.

[simo, 2011-08-02 09:08:14]

http://codereview.chromium.org/7390027/diff/13001/chrome/browser/first_run/fi...
File chrome/browser/first_run/first_run.cc (right):

http://codereview.chromium.org/7390027/diff/13001/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:228: if (value) {
On 2011/08/01 19:41:51, James Hawkins wrote:
> Optional, but recommended: Don't use braces for one line logic blocks.

Done.

http://codereview.chromium.org/7390027/diff/13001/chrome/browser/first_run/fi...
File chrome/browser/first_run/first_run.h (right):

http://codereview.chromium.org/7390027/diff/13001/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.h:156: static void SetImportItem(PrefService*
user_prefs,
On 2011/08/01 19:41:51, James Hawkins wrote:
> Move to an unnamed namespace in the implementation and remove the static
> keyword.

Done.

http://codereview.chromium.org/7390027/diff/13001/chrome/browser/resources/op...
File chrome/browser/resources/options/import_data_overlay.js (right):

http://codereview.chromium.org/7390027/diff/13001/chrome/browser/resources/op...
chrome/browser/resources/options/import_data_overlay.js:91: var importOptions =
['history', 'favorites', 'passwords', 'search'];
On 2011/08/01 19:41:51, James Hawkins wrote:
> Why did you change this?  This is fragile and doesn't scale well.
Yes, you're right, I had changed this function around a lot and for various
reasons that are now irrelevant I found it easier to change it to using
importOptions and didn't think to revert it. Done.

[James Hawkins, 2011-08-02 17:51:40]

http://codereview.chromium.org/7390027/diff/18001/chrome/browser/first_run/fi...
File chrome/browser/first_run/first_run.cc (right):

http://codereview.chromium.org/7390027/diff/18001/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:60: void SetImportItem(PrefService*
user_prefs,
Document this method.

http://codereview.chromium.org/7390027/diff/18001/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:66: 
Remove extra blank line.

http://codereview.chromium.org/7390027/diff/18001/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:74: || ((import_type !=
importer::FAVORITES)
Style guide mandates operators spanning multiple lines be on the first line at
the end.

http://codereview.chromium.org/7390027/diff/18001/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:78: // in master_preferences.
Please don't add a newline for every sentence in a comment. Very rarely are
these sentences unrelated and thus require a new 'paragraph'.  Here and
elsewhere.

http://codereview.chromium.org/7390027/diff/18001/chrome/browser/resources/op...
File chrome/browser/resources/options/import_data_overlay.js (right):

http://codereview.chromium.org/7390027/diff/18001/chrome/browser/resources/op...
chrome/browser/resources/options/import_data_overlay.js:93: for (var i = 0; i <
checkboxes.length; i++) {
Remove braces (no braces for single line blocks).

http://codereview.chromium.org/7390027/diff/18001/chrome/browser/resources/op...
chrome/browser/resources/options/import_data_overlay.js:191: if (state) {
Get rid of the if/else and transform this to:

for (var i = 0; i < checkboxes.length; i++)
  checkboxes[i].setDisabled("Importing", state);

Don't forget to remove the braces.

[simo, 2011-08-03 08:31:44]

http://codereview.chromium.org/7390027/diff/18001/chrome/browser/first_run/fi...
File chrome/browser/first_run/first_run.cc (right):

http://codereview.chromium.org/7390027/diff/18001/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:60: void SetImportItem(PrefService*
user_prefs,
On 2011/08/02 17:51:43, James Hawkins wrote:
> Document this method.

Done.

http://codereview.chromium.org/7390027/diff/18001/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:66: 
On 2011/08/02 17:51:43, James Hawkins wrote:
> Remove extra blank line.

Done.

http://codereview.chromium.org/7390027/diff/18001/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:74: || ((import_type !=
importer::FAVORITES)
On 2011/08/02 17:51:43, James Hawkins wrote:
> Style guide mandates operators spanning multiple lines be on the first line at
> the end.

Done.

http://codereview.chromium.org/7390027/diff/18001/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:78: // in master_preferences.
On 2011/08/02 17:51:43, James Hawkins wrote:
> Please don't add a newline for every sentence in a comment. Very rarely are
> these sentences unrelated and thus require a new 'paragraph'.  Here and
> elsewhere.

Done.

http://codereview.chromium.org/7390027/diff/18001/chrome/browser/resources/op...
File chrome/browser/resources/options/import_data_overlay.js (right):

http://codereview.chromium.org/7390027/diff/18001/chrome/browser/resources/op...
chrome/browser/resources/options/import_data_overlay.js:93: for (var i = 0; i <
checkboxes.length; i++) {
On 2011/08/02 17:51:43, James Hawkins wrote:
> Remove braces (no braces for single line blocks).

Done.

http://codereview.chromium.org/7390027/diff/18001/chrome/browser/resources/op...
chrome/browser/resources/options/import_data_overlay.js:191: if (state) {
On 2011/08/02 17:51:43, James Hawkins wrote:
> Get rid of the if/else and transform this to:
> 
> for (var i = 0; i < checkboxes.length; i++)
>   checkboxes[i].setDisabled("Importing", state);
> 
> Don't forget to remove the braces.

Done.

[James Hawkins, 2011-08-03 17:45:01]

LGTM

[Mattias Nissler (ping if slow), 2011-08-04 10:08:55]

LGTM too. Simo, can you rebase? Patch doesn't apply cleanly on current trunk.
Ping me after you did, so I can run try jobs.

http://codereview.chromium.org/7390027/diff/23001/chrome/browser/first_run/fi...
File chrome/browser/first_run/first_run.cc (right):

http://codereview.chromium.org/7390027/diff/23001/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:108: 
remove new line

[simo, 2011-08-04 14:26:22]

Done! I hope this works...
On 2011/08/04 10:08:55, Mattias Nissler wrote:
> LGTM too. Simo, can you rebase? Patch doesn't apply cleanly on current trunk.
> Ping me after you did, so I can run try jobs.
> 
>
http://codereview.chromium.org/7390027/diff/23001/chrome/browser/first_run/fi...
> File chrome/browser/first_run/first_run.cc (right):
> 
>
http://codereview.chromium.org/7390027/diff/23001/chrome/browser/first_run/fi...
> chrome/browser/first_run/first_run.cc:108: 
> remove new line

[simo, 2011-08-05 08:34:37]

Updated patch.
On 2011/08/04 14:26:22, simo wrote:
> Done! I hope this works...
> On 2011/08/04 10:08:55, Mattias Nissler wrote:
> > LGTM too. Simo, can you rebase? Patch doesn't apply cleanly on current
trunk.
> > Ping me after you did, so I can run try jobs.
> > 
> >
>
http://codereview.chromium.org/7390027/diff/23001/chrome/browser/first_run/fi...
> > File chrome/browser/first_run/first_run.cc (right):
> > 
> >
>
http://codereview.chromium.org/7390027/diff/23001/chrome/browser/first_run/fi...
> > chrome/browser/first_run/first_run.cc:108: 
> > remove new line

[commit-bot: I haz the power, 2011-08-16 07:25:50]

Can't apply patch for file chrome/app/policy/policy_templates.json.
While running patch -p1 --forward --force;
patching file chrome/app/policy/policy_templates.json
Hunk #1 FAILED at 94.
Hunk #3 succeeded at 1747 with fuzz 2 (offset 10 lines).
1 out of 3 hunks FAILED -- saving rejects to file
chrome/app/policy/policy_templates.json.rej

[commit-bot: I haz the power, 2011-08-16 11:27:15]

Change committed as 96921