Update AutoImport to import nothing by default (in absence of policy and master_prefs).

Update AutoImport to import nothing by default (in absence of policy and master_prefs).

As decided in conclusion of experiment https://crbug.com/555550#c114

This simplifies the code a lot, dropping all the weird defaults accrued
over the years. AutoImport is now strictly driven by policies + master_prefs.

Also deprecated the distribution entries for the import prefs which were
duplicates of the prefs:: entries... added legacy mapping in one place in code
and it then simplifies the rest of the logic (the kDistroDict is never
registered so this was somewhat of a pain).

The new code also makes it trivial to fix issue 601697 :) -- done!
(i.e. added support for auto import of autofill form data and saved passwords
 although actual support will depend on the support of the importer
 for the imported browser -- which is the same as the support from a
 manual import).

BUG=555550, 601697
Review-Url: https://codereview.chromium.org/2705113005
Cr-Commit-Position: refs/heads/master@{#458853}
Committed: https://chromium.googlesource.com/chromium/src/+/dac09c03b9f3cc44adf71e8836f6bebabec7a61d

[gab, 2017-02-22 21:59:02]

gab@chromium.org changed reviewers:
+ grt@chromium.org

[gab, 2017-02-22 21:59:03]

Sir Greg PTaL =D!

[gab, 2017-02-22 21:59:23]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-22 22:00:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 22:14:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 22:14:16]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[grt (UTC plus 2), 2017-02-23 14:05:49]

nice cleanup!

https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:8: #include <memory>
nit: still needed for std::unique_ptr

https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:434: void
MapLegacyDistroPrefs(base::DictionaryValue* master_prefs_dict) {
MasterPreferences::EnforceLegacyPreferences does something kinda like this for
other prefs. can these two similar operations happen in the same place? i'd
almost lean toward removing the installer::master_preferences constants and
using inline literals in this mapping to make it impossible for someone to
accidentally use them in the future.

https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:441: struct LegacyDistroPrefMapping {
  static constexpr struct {... (see comment below)

https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:723: } static constexpr kImportItems[] = {
nit: "static constexpr" should come first, and this type doesn't really need a
name, so this could become:
  static constexpr struct {
    ....
  } kImportItems[] = {

https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:724: {prefs::kImportHistory,
importer::HISTORY},
the pedant in me would like to see this sorted either by pref name or by the
position of the ImportItem bit. :-)

https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:728: };
prefs::kImportAutofillFormData and prefs::kImportSavedPasswords are notably
absent. why? is fixing https://crbug.com/601697 now as simple as adding
{prefs::kImportSavedPasswords, importer::PASSWORDS} here? if i'm not mistaken,
adding both will suddenly make the GP settings work as documented, right?

https://www.chromium.org/administrators/policy-list-3#ImportAutofillFormData
https://www.chromium.org/administrators/policy-list-3#ImportSavedPasswords

https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:739: std::unique_ptr<ImporterList>
importer_list(new ImporterList());
nit:
    auto importer_list = base::MakeUnique<ImporterList>();

https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
File chrome/browser/first_run/first_run_browsertest.cc (right):

https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run_browsertest.cc:186: extern const char
kImportAll[] =
why do these all have external linkage? why not just "constexpr char
kImportAll[] = "?

https://codereview.chromium.org/2705113005/diff/1/chrome/browser/ui/browser_u...
File chrome/browser/ui/browser_ui_prefs.cc (right):

https://codereview.chromium.org/2705113005/diff/1/chrome/browser/ui/browser_u...
chrome/browser/ui/browser_ui_prefs.cc:74:
registry->RegisterBooleanPref(prefs::kImportAutofillFormData, false);
this will cause all checkboxes in chrome://settings/importData to be unchecked
by default, no? i don't think this is the right thing.

[gab, 2017-02-23 15:04:22]

Description was changed from

==========
Disable AutoImport by default.

As decided in conclusion of experiment https://crbug.com/555550#c114

This simplifies the code a lot, dropping all the weird defaults accrued
over the years. AutoImport is now strictly driven by policies + master_prefs.

Also deprecated the distribution entries for the import prefs which were
duplicates of the prefs:: entries... added legacy mapping in one place in code
and it then simplifies the rest of the logic (the kDistroDict is never
registered so this was somewhat of a pain).

BUG=555550
==========

to

==========
Disable AutoImport by default.

As decided in conclusion of experiment https://crbug.com/555550#c114

This simplifies the code a lot, dropping all the weird defaults accrued
over the years. AutoImport is now strictly driven by policies + master_prefs.

Also deprecated the distribution entries for the import prefs which were
duplicates of the prefs:: entries... added legacy mapping in one place in code
and it then simplifies the rest of the logic (the kDistroDict is never
registered so this was somewhat of a pain).

BUG=555550, 601697
==========

[gab, 2017-02-23 15:07:59]

Description was changed from

==========
Disable AutoImport by default.

As decided in conclusion of experiment https://crbug.com/555550#c114

This simplifies the code a lot, dropping all the weird defaults accrued
over the years. AutoImport is now strictly driven by policies + master_prefs.

Also deprecated the distribution entries for the import prefs which were
duplicates of the prefs:: entries... added legacy mapping in one place in code
and it then simplifies the rest of the logic (the kDistroDict is never
registered so this was somewhat of a pain).

BUG=555550, 601697
==========

to

==========
Disable AutoImport by default.

As decided in conclusion of experiment https://crbug.com/555550#c114

This simplifies the code a lot, dropping all the weird defaults accrued
over the years. AutoImport is now strictly driven by policies + master_prefs.

Also deprecated the distribution entries for the import prefs which were
duplicates of the prefs:: entries... added legacy mapping in one place in code
and it then simplifies the rest of the logic (the kDistroDict is never
registered so this was somewhat of a pain).

The new code also makes it trivial to fix issue 601697 :) -- done!
(i.e. added support for auto import of autofill form data and saved passwords
 although actual support will depend on the support of the importer
 for the imported browser -- which is the same as the support from a
 manual import).


This CL also adds stripping of the "distribution" dictionary before mapping
master_preferences to Preferences (it is never used nor even registered after 
first run) :).


BUG=555550, 601697
==========

[gab, 2017-02-23 15:08:12]

Description was changed from

==========
Disable AutoImport by default.

As decided in conclusion of experiment https://crbug.com/555550#c114

This simplifies the code a lot, dropping all the weird defaults accrued
over the years. AutoImport is now strictly driven by policies + master_prefs.

Also deprecated the distribution entries for the import prefs which were
duplicates of the prefs:: entries... added legacy mapping in one place in code
and it then simplifies the rest of the logic (the kDistroDict is never
registered so this was somewhat of a pain).

The new code also makes it trivial to fix issue 601697 :) -- done!
(i.e. added support for auto import of autofill form data and saved passwords
 although actual support will depend on the support of the importer
 for the imported browser -- which is the same as the support from a
 manual import).


This CL also adds stripping of the "distribution" dictionary before mapping
master_preferences to Preferences (it is never used nor even registered after 
first run) :).


BUG=555550, 601697
==========

to

==========
Disable AutoImport by default.

As decided in conclusion of experiment https://crbug.com/555550#c114

This simplifies the code a lot, dropping all the weird defaults accrued
over the years. AutoImport is now strictly driven by policies + master_prefs.

Also deprecated the distribution entries for the import prefs which were
duplicates of the prefs:: entries... added legacy mapping in one place in code
and it then simplifies the rest of the logic (the kDistroDict is never
registered so this was somewhat of a pain).

The new code also makes it trivial to fix issue 601697 :) -- done!
(i.e. added support for auto import of autofill form data and saved passwords
 although actual support will depend on the support of the importer
 for the imported browser -- which is the same as the support from a
 manual import).


This CL also adds stripping of the "distribution" dictionary before mapping
master_preferences to Preferences (it is never used nor even registered after 
first run) :).

BUG=555550, 601697
==========

[gab, 2017-02-23 20:35:03]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 20:35:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2017-02-23 20:37:39]

gab@chromium.org changed reviewers:
+ ewald@chromium.org

[gab, 2017-02-23 20:37:41]

+ewald : this CL brings a slight change in behavior.

Previously there were two import prefs:

kDistroImport* and kImport*

both state they drive the first run import...

In practice kDistroImport* only drove first run auto import while kImport* drove
that + stuck in the chrome://settings/importData UI later (as initial values for
the checkboxes -- potentially sticky values if enforced by policy).

A negligible change in this CL:
 1) Maps kDistroImport* to kImport* (results in kDistroImport* to be applied to
chrome://settings/importData post first run -- seems fine)

The question (and this is made important by changing the default for kImport* to
false which results in chrome://settings/importData's checkboxes being unticked
by default :(...):
 2) Should kImport* prefs even drive the UI? It doesn't make much sense to me
for auto import policies to later control the user's ability to do a manual
import (if it's a recommended policy that merely tweaks the checkboxes' default
state that's not bad but if it's an enforced policy the user can't interact with
the UI... actually I'm not sure what the UI does but whichever boxes are checked
in the end won't be honored...).

IMO that UI shouldn't even be hooked up to prefs (all checkboxes should just be
checked when it's brought up without UI-memory of last time it came up -- who
imports more than once and cares to have the checkboxes set up their way
anyways...). Making it sticky has little upside and making it honor policies is
just weird...

Can I let you resolve what to do here? And ideally find someone to remove the
UI's dependency on the kImport* prefs in settings_import_data_handler.cc so I
can move along with this CL (I don't have cycles to tweak the UI code right
now).

Thanks!
Gab

https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:8: #include <memory>
On 2017/02/23 14:05:48, grt (UTC plus 1) wrote:
> nit: still needed for std::unique_ptr

Hmm, right, and this is adding it..?!

https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:434: void
MapLegacyDistroPrefs(base::DictionaryValue* master_prefs_dict) {
On 2017/02/23 14:05:48, grt (UTC plus 1) wrote:
> MasterPreferences::EnforceLegacyPreferences does something kinda like this for
> other prefs. can these two similar operations happen in the same place? i'd
> almost lean toward removing the installer::master_preferences constants and
> using inline literals in this mapping to make it impossible for someone to
> accidentally use them in the future.

Done.

https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:441: struct LegacyDistroPrefMapping {
On 2017/02/23 14:05:48, grt (UTC plus 1) wrote:
>   static constexpr struct {... (see comment below)

Done.

https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:723: } static constexpr kImportItems[] = {
On 2017/02/23 14:05:48, grt (UTC plus 1) wrote:
> nit: "static constexpr" should come first, and this type doesn't really need a
> name, so this could become:
>   static constexpr struct {
>     ....
>   } kImportItems[] = {

Done.

https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:724: {prefs::kImportHistory,
importer::HISTORY},
On 2017/02/23 14:05:48, grt (UTC plus 1) wrote:
> the pedant in me would like to see this sorted either by pref name or by the
> position of the ImportItem bit. :-)

Done.

https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:728: };
On 2017/02/23 14:05:48, grt (UTC plus 1) wrote:
> prefs::kImportAutofillFormData and prefs::kImportSavedPasswords are notably
> absent. why? is fixing https://crbug.com/601697 now as simple as adding
> {prefs::kImportSavedPasswords, importer::PASSWORDS} here? if i'm not mistaken,
> adding both will suddenly make the GP settings work as documented, right?
> 
> https://www.chromium.org/administrators/policy-list-3#ImportAutofillFormData
> https://www.chromium.org/administrators/policy-list-3#ImportSavedPasswords

Interesting, I guess it probably is =D (though in practice it will depend on
that importer's support for these ImportItems).

Done.

One thing to note: I've long thought we should split first run only
master_preferences from prefs. On the other hand, policies only work with real
prefs (so it's kind of weird to have master_preferences only things -- i.e.
kDistro*). And the prefs::kImport* values happen to be used at runtime after
first_run in a few corner UIs so I figured they might as well all use the same
thing (those used to not be directly seeded by master_preferences but I don't
think it's wrong that they now be).

Also, while writing this CL I realized we never even register any distro prefs
(so I just went ahead and added logic to strip the kDistroDict from
master_preferences before mapping them to Preferences -- as well as cleaning it
from existing Preferences :) ).

https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:739: std::unique_ptr<ImporterList>
importer_list(new ImporterList());
On 2017/02/23 14:05:48, grt (UTC plus 1) wrote:
> nit:
>     auto importer_list = base::MakeUnique<ImporterList>();

this was cut-paste from above but why not :)

https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
File chrome/browser/first_run/first_run_browsertest.cc (right):

https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run_browsertest.cc:186: extern const char
kImportAll[] =
On 2017/02/23 14:05:48, grt (UTC plus 1) wrote:
> why do these all have external linkage? why not just "constexpr char
> kImportAll[] = "?

I forget, IIRC it was required for the template magic below to link when I
originally wrote it but this no longer appears to be the case (maybe an
improvement in VC++ :)).

Done.

https://codereview.chromium.org/2705113005/diff/1/chrome/browser/ui/browser_u...
File chrome/browser/ui/browser_ui_prefs.cc (right):

https://codereview.chromium.org/2705113005/diff/1/chrome/browser/ui/browser_u...
chrome/browser/ui/browser_ui_prefs.cc:74:
registry->RegisterBooleanPref(prefs::kImportAutofillFormData, false);
On 2017/02/23 14:05:48, grt (UTC plus 1) wrote:
> this will cause all checkboxes in chrome://settings/importData to be unchecked
> by default, no? i don't think this is the right thing.

Right, I'd seen those and initially thought it made sense for them to reflect
what was imported on first run but you now make me realize that having these all
unchecked by default is not great.

Also, I thought it was merely reading the value without setting it back but now
I realize I just happen to know nothing about webUI: it's actually hooked up to
the pref (and toggles the actual pref live when using the UI) :
https://cs.chromium.org/search/?q=(%22%7C%27)import_history(%22%7C%27)&sq=pac...
(I think the import_data_overlay.html bit is the critical hook).

Thinking about this some more, I don't think it makes sense for this UI to be
driven by policies for the same pref as auto import... I don't even know that it
makes sense for it to be driven by policies at all? That would mean that a
policy could prevent import even if the user explicitly wants to tick that box?

+ewald, can you look into this?

[commit-bot: I haz the power, 2017-02-23 20:38:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-23 20:38:35]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[gab, 2017-02-23 20:40:54]

On 2017/02/23 20:37:41, gab wrote:
> +ewald : this CL brings a slight change in behavior.
> 
> Previously there were two import prefs:
> 
> kDistroImport* and kImport*
> 
> both state they drive the first run import...
> 
> In practice kDistroImport* only drove first run auto import while kImport*
drove
> that + stuck in the chrome://settings/importData UI later (as initial values
for
> the checkboxes -- potentially sticky values if enforced by policy).
> 
> A negligible change in this CL:
>  1) Maps kDistroImport* to kImport* (results in kDistroImport* to be applied
to
> chrome://settings/importData post first run -- seems fine)
> 
> The question (and this is made important by changing the default for kImport*
to
> false which results in chrome://settings/importData's checkboxes being
unticked
> by default :(...):
>  2) Should kImport* prefs even drive the UI? It doesn't make much sense to me
> for auto import policies to later control the user's ability to do a manual
> import (if it's a recommended policy that merely tweaks the checkboxes'
default
> state that's not bad but if it's an enforced policy the user can't interact
with
> the UI... actually I'm not sure what the UI does but whichever boxes are
checked
> in the end won't be honored...).
> 
> IMO that UI shouldn't even be hooked up to prefs (all checkboxes should just
be
> checked when it's brought up without UI-memory of last time it came up -- who
> imports more than once and cares to have the checkboxes set up their way
> anyways...). Making it sticky has little upside and making it honor policies
is
> just weird...

Actually looks like the hook to the kImport* prefs is new in M56
(http://crrev.com/432753) as part of MD settings.

Looks like they reused those prefs but shouldn't have. I'm flagging it on their
CL.

> 
> Can I let you resolve what to do here? And ideally find someone to remove the
> UI's dependency on the kImport* prefs in settings_import_data_handler.cc so I
> can move along with this CL (I don't have cycles to tweak the UI code right
> now).
> 
> Thanks!
> Gab
> 
>
https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
> File chrome/browser/first_run/first_run.cc (right):
> 
>
https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
> chrome/browser/first_run/first_run.cc:8: #include <memory>
> On 2017/02/23 14:05:48, grt (UTC plus 1) wrote:
> > nit: still needed for std::unique_ptr
> 
> Hmm, right, and this is adding it..?!
> 
>
https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
> chrome/browser/first_run/first_run.cc:434: void
> MapLegacyDistroPrefs(base::DictionaryValue* master_prefs_dict) {
> On 2017/02/23 14:05:48, grt (UTC plus 1) wrote:
> > MasterPreferences::EnforceLegacyPreferences does something kinda like this
for
> > other prefs. can these two similar operations happen in the same place? i'd
> > almost lean toward removing the installer::master_preferences constants and
> > using inline literals in this mapping to make it impossible for someone to
> > accidentally use them in the future.
> 
> Done.
> 
>
https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
> chrome/browser/first_run/first_run.cc:441: struct LegacyDistroPrefMapping {
> On 2017/02/23 14:05:48, grt (UTC plus 1) wrote:
> >   static constexpr struct {... (see comment below)
> 
> Done.
> 
>
https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
> chrome/browser/first_run/first_run.cc:723: } static constexpr kImportItems[] =
{
> On 2017/02/23 14:05:48, grt (UTC plus 1) wrote:
> > nit: "static constexpr" should come first, and this type doesn't really need
a
> > name, so this could become:
> >   static constexpr struct {
> >     ....
> >   } kImportItems[] = {
> 
> Done.
> 
>
https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
> chrome/browser/first_run/first_run.cc:724: {prefs::kImportHistory,
> importer::HISTORY},
> On 2017/02/23 14:05:48, grt (UTC plus 1) wrote:
> > the pedant in me would like to see this sorted either by pref name or by the
> > position of the ImportItem bit. :-)
> 
> Done.
> 
>
https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
> chrome/browser/first_run/first_run.cc:728: };
> On 2017/02/23 14:05:48, grt (UTC plus 1) wrote:
> > prefs::kImportAutofillFormData and prefs::kImportSavedPasswords are notably
> > absent. why? is fixing https://crbug.com/601697 now as simple as adding
> > {prefs::kImportSavedPasswords, importer::PASSWORDS} here? if i'm not
mistaken,
> > adding both will suddenly make the GP settings work as documented, right?
> > 
> > https://www.chromium.org/administrators/policy-list-3#ImportAutofillFormData
> > https://www.chromium.org/administrators/policy-list-3#ImportSavedPasswords
> 
> Interesting, I guess it probably is =D (though in practice it will depend on
> that importer's support for these ImportItems).
> 
> Done.
> 
> One thing to note: I've long thought we should split first run only
> master_preferences from prefs. On the other hand, policies only work with real
> prefs (so it's kind of weird to have master_preferences only things -- i.e.
> kDistro*). And the prefs::kImport* values happen to be used at runtime after
> first_run in a few corner UIs so I figured they might as well all use the same
> thing (those used to not be directly seeded by master_preferences but I don't
> think it's wrong that they now be).
> 
> Also, while writing this CL I realized we never even register any distro prefs
> (so I just went ahead and added logic to strip the kDistroDict from
> master_preferences before mapping them to Preferences -- as well as cleaning
it
> from existing Preferences :) ).
> 
>
https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
> chrome/browser/first_run/first_run.cc:739: std::unique_ptr<ImporterList>
> importer_list(new ImporterList());
> On 2017/02/23 14:05:48, grt (UTC plus 1) wrote:
> > nit:
> >     auto importer_list = base::MakeUnique<ImporterList>();
> 
> this was cut-paste from above but why not :)
> 
>
https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
> File chrome/browser/first_run/first_run_browsertest.cc (right):
> 
>
https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
> chrome/browser/first_run/first_run_browsertest.cc:186: extern const char
> kImportAll[] =
> On 2017/02/23 14:05:48, grt (UTC plus 1) wrote:
> > why do these all have external linkage? why not just "constexpr char
> > kImportAll[] = "?
> 
> I forget, IIRC it was required for the template magic below to link when I
> originally wrote it but this no longer appears to be the case (maybe an
> improvement in VC++ :)).
> 
> Done.
> 
>
https://codereview.chromium.org/2705113005/diff/1/chrome/browser/ui/browser_u...
> File chrome/browser/ui/browser_ui_prefs.cc (right):
> 
>
https://codereview.chromium.org/2705113005/diff/1/chrome/browser/ui/browser_u...
> chrome/browser/ui/browser_ui_prefs.cc:74:
> registry->RegisterBooleanPref(prefs::kImportAutofillFormData, false);
> On 2017/02/23 14:05:48, grt (UTC plus 1) wrote:
> > this will cause all checkboxes in chrome://settings/importData to be
unchecked
> > by default, no? i don't think this is the right thing.
> 
> Right, I'd seen those and initially thought it made sense for them to reflect
> what was imported on first run but you now make me realize that having these
all
> unchecked by default is not great.
> 
> Also, I thought it was merely reading the value without setting it back but
now
> I realize I just happen to know nothing about webUI: it's actually hooked up
to
> the pref (and toggles the actual pref live when using the UI) :
>
https://cs.chromium.org/search/?q=(%22%7C%27)import_history(%22%7C%27)&sq=pac...
> (I think the import_data_overlay.html bit is the critical hook).
> 
> Thinking about this some more, I don't think it makes sense for this UI to be
> driven by policies for the same pref as auto import... I don't even know that
it
> makes sense for it to be driven by policies at all? That would mean that a
> policy could prevent import even if the user explicitly wants to tick that
box?
> 
> +ewald, can you look into this?

[gab, 2017-02-23 20:58:23]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[gab, 2017-02-23 20:58:31]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 20:58:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[grt (UTC plus 2), 2017-02-23 21:19:01]

mostly just nits at this point. i agree that the use of prefs to drive the
checkboxes in the UI is odd. that needs to be fixed before this can land, no?

https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
chrome/browser/first_run/first_run.cc:8: #include <memory>
On 2017/02/23 20:37:40, gab wrote:
> On 2017/02/23 14:05:48, grt (UTC plus 1) wrote:
> > nit: still needed for std::unique_ptr
> 
> Hmm, right, and this is adding it..?!

Wow. My brain swapped adding and removing. Guess I didn't get enough sleep last
night...

https://codereview.chromium.org/2705113005/diff/20001/chrome/browser/first_ru...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/2705113005/diff/20001/chrome/browser/first_ru...
chrome/browser/first_run/first_run.cc:658: // The distribution dictionary (and
any prefs below it) are never registered
would you believe that *one* preference in the distro dict is registered (line
573)?

https://codereview.chromium.org/2705113005/diff/20001/chrome/browser/first_ru...
chrome/browser/first_run/first_run.cc:713: std::unique_ptr<ImporterList>
importer_list =
i think "auto" is appropriate here since the type is obvious

https://codereview.chromium.org/2705113005/diff/20001/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/2705113005/diff/20001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:745:
profile_prefs->ClearPref(kDistroDict);
can't do this because of PingDelay :-(

https://codereview.chromium.org/2705113005/diff/20001/chrome/browser/prefs/pr...
File chrome/browser/prefs/profile_pref_store_manager_unittest.cc (right):

https://codereview.chromium.org/2705113005/diff/20001/chrome/browser/prefs/pr...
chrome/browser/prefs/profile_pref_store_manager_unittest.cc:323:
std::unique_ptr<base::DictionaryValue> master_prefs =
auto here, too

https://codereview.chromium.org/2705113005/diff/20001/chrome/installer/util/m...
File chrome/installer/util/master_preferences.cc (right):

https://codereview.chromium.org/2705113005/diff/20001/chrome/installer/util/m...
chrome/installer/util/master_preferences.cc:228: // Deprecated boolean import
master preferences now mapped to their duplicate
duplicate -> duplicates

https://codereview.chromium.org/2705113005/diff/20001/chrome/installer/util/m...
chrome/installer/util/master_preferences.cc:247: if
(GetBool(mapping.old_distro_pref_path, &value)) {
nit: omit braces

[commit-bot: I haz the power, 2017-02-23 21:35:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-23 21:35:56]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[gab, 2017-02-23 23:17:15]

Description was changed from

==========
Disable AutoImport by default.

As decided in conclusion of experiment https://crbug.com/555550#c114

This simplifies the code a lot, dropping all the weird defaults accrued
over the years. AutoImport is now strictly driven by policies + master_prefs.

Also deprecated the distribution entries for the import prefs which were
duplicates of the prefs:: entries... added legacy mapping in one place in code
and it then simplifies the rest of the logic (the kDistroDict is never
registered so this was somewhat of a pain).

The new code also makes it trivial to fix issue 601697 :) -- done!
(i.e. added support for auto import of autofill form data and saved passwords
 although actual support will depend on the support of the importer
 for the imported browser -- which is the same as the support from a
 manual import).


This CL also adds stripping of the "distribution" dictionary before mapping
master_preferences to Preferences (it is never used nor even registered after 
first run) :).

BUG=555550, 601697
==========

to

==========
Disable AutoImport by default.

As decided in conclusion of experiment https://crbug.com/555550#c114

This simplifies the code a lot, dropping all the weird defaults accrued
over the years. AutoImport is now strictly driven by policies + master_prefs.

Also deprecated the distribution entries for the import prefs which were
duplicates of the prefs:: entries... added legacy mapping in one place in code
and it then simplifies the rest of the logic (the kDistroDict is never
registered so this was somewhat of a pain).

The new code also makes it trivial to fix issue 601697 :) -- done!
(i.e. added support for auto import of autofill form data and saved passwords
 although actual support will depend on the support of the importer
 for the imported browser -- which is the same as the support from a
 manual import).

BUG=555550, 601697
==========

[gab, 2017-02-23 23:26:05]

Patchset #5 (id:70001) has been deleted

[gab, 2017-02-23 23:27:53]

On 2017/02/23 21:19:01, grt (UTC plus 1) wrote:
> mostly just nits at this point. i agree that the use of prefs to drive the
> checkboxes in the UI is odd. that needs to be fixed before this can land, no?

Yeah, I'll work on resolving that in parallel but you can still review and stamp
this in the meantime, thanks!

https://codereview.chromium.org/2705113005/diff/20001/chrome/browser/first_ru...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/2705113005/diff/20001/chrome/browser/first_ru...
chrome/browser/first_run/first_run.cc:658: // The distribution dictionary (and
any prefs below it) are never registered
On 2017/02/23 21:19:01, grt (UTC plus 1) wrote:
> would you believe that *one* preference in the distro dict is registered (line
> 573)?

Oh my god....! This code is killling me..! Good catch!

Removal of kDistroDict is getting bigger and more unrelated, moved to precursor
CL @ https://codereview.chromium.org/2714853002/

https://codereview.chromium.org/2705113005/diff/20001/chrome/browser/first_ru...
chrome/browser/first_run/first_run.cc:713: std::unique_ptr<ImporterList>
importer_list =
On 2017/02/23 21:19:01, grt (UTC plus 1) wrote:
> i think "auto" is appropriate here since the type is obvious

Done.

https://codereview.chromium.org/2705113005/diff/20001/chrome/browser/prefs/br...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/2705113005/diff/20001/chrome/browser/prefs/br...
chrome/browser/prefs/browser_prefs.cc:745:
profile_prefs->ClearPref(kDistroDict);
On 2017/02/23 21:19:01, grt (UTC plus 1) wrote:
> can't do this because of PingDelay :-(

Fixed in precursor CL.

https://codereview.chromium.org/2705113005/diff/20001/chrome/browser/prefs/pr...
File chrome/browser/prefs/profile_pref_store_manager_unittest.cc (right):

https://codereview.chromium.org/2705113005/diff/20001/chrome/browser/prefs/pr...
chrome/browser/prefs/profile_pref_store_manager_unittest.cc:323:
std::unique_ptr<base::DictionaryValue> master_prefs =
On 2017/02/23 21:19:01, grt (UTC plus 1) wrote:
> auto here, too

Done.

https://codereview.chromium.org/2705113005/diff/20001/chrome/installer/util/m...
File chrome/installer/util/master_preferences.cc (right):

https://codereview.chromium.org/2705113005/diff/20001/chrome/installer/util/m...
chrome/installer/util/master_preferences.cc:228: // Deprecated boolean import
master preferences now mapped to their duplicate
On 2017/02/23 21:19:01, grt (UTC plus 1) wrote:
> duplicate -> duplicates

Done.

https://codereview.chromium.org/2705113005/diff/20001/chrome/installer/util/m...
chrome/installer/util/master_preferences.cc:247: if
(GetBool(mapping.old_distro_pref_path, &value)) {
On 2017/02/23 21:19:01, grt (UTC plus 1) wrote:
> nit: omit braces

Done.

[gab, 2017-02-23 23:28:23]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 23:28:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 23:40:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-23 23:40:06]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[gab, 2017-02-24 00:38:39]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 00:39:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 00:44:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 00:44:50]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[gab, 2017-02-24 00:49:08]

Patchset #6 (id:110001) has been deleted

[gab, 2017-02-24 00:50:03]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 00:50:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 01:10:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 01:10:02]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[gab, 2017-02-24 01:18:00]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 01:18:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eli Wald, 2017-02-24 01:21:36]

On 2017/02/23 20:40:54, gab wrote:
> On 2017/02/23 20:37:41, gab wrote:
> > +ewald : this CL brings a slight change in behavior.
> > 
> > Previously there were two import prefs:
> > 
> > kDistroImport* and kImport*
> > 
> > both state they drive the first run import...
> > 
> > In practice kDistroImport* only drove first run auto import while kImport*
> drove
> > that + stuck in the chrome://settings/importData UI later (as initial values
> for
> > the checkboxes -- potentially sticky values if enforced by policy).
> > 
> > A negligible change in this CL:
> >  1) Maps kDistroImport* to kImport* (results in kDistroImport* to be applied
> to
> > chrome://settings/importData post first run -- seems fine)
> > 
> > The question (and this is made important by changing the default for
kImport*
> to
> > false which results in chrome://settings/importData's checkboxes being
> unticked
> > by default :(...):
> >  2) Should kImport* prefs even drive the UI? It doesn't make much sense to
me
> > for auto import policies to later control the user's ability to do a manual
> > import (if it's a recommended policy that merely tweaks the checkboxes'
> default
> > state that's not bad but if it's an enforced policy the user can't interact
> with
> > the UI... actually I'm not sure what the UI does but whichever boxes are
> checked
> > in the end won't be honored...).
> > 
> > IMO that UI shouldn't even be hooked up to prefs (all checkboxes should just
> be
> > checked when it's brought up without UI-memory of last time it came up --
who
> > imports more than once and cares to have the checkboxes set up their way
> > anyways...). Making it sticky has little upside and making it honor policies
> is
> > just weird...
> 
> Actually looks like the hook to the kImport* prefs is new in M56
> (http://crrev.com/432753) as part of MD settings.
> 
> Looks like they reused those prefs but shouldn't have. I'm flagging it on
their
> CL.
> 
> > 
> > Can I let you resolve what to do here? And ideally find someone to remove
the
> > UI's dependency on the kImport* prefs in settings_import_data_handler.cc so
I
> > can move along with this CL (I don't have cycles to tweak the UI code right
> > now).
> > 
> > Thanks!
> > Gab
> > 
> >
>
https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
> > File chrome/browser/first_run/first_run.cc (right):
> > 
> >
>
https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
> > chrome/browser/first_run/first_run.cc:8: #include <memory>
> > On 2017/02/23 14:05:48, grt (UTC plus 1) wrote:
> > > nit: still needed for std::unique_ptr
> > 
> > Hmm, right, and this is adding it..?!
> > 
> >
>
https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
> > chrome/browser/first_run/first_run.cc:434: void
> > MapLegacyDistroPrefs(base::DictionaryValue* master_prefs_dict) {
> > On 2017/02/23 14:05:48, grt (UTC plus 1) wrote:
> > > MasterPreferences::EnforceLegacyPreferences does something kinda like this
> for
> > > other prefs. can these two similar operations happen in the same place?
i'd
> > > almost lean toward removing the installer::master_preferences constants
and
> > > using inline literals in this mapping to make it impossible for someone to
> > > accidentally use them in the future.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
> > chrome/browser/first_run/first_run.cc:441: struct LegacyDistroPrefMapping {
> > On 2017/02/23 14:05:48, grt (UTC plus 1) wrote:
> > >   static constexpr struct {... (see comment below)
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
> > chrome/browser/first_run/first_run.cc:723: } static constexpr kImportItems[]
=
> {
> > On 2017/02/23 14:05:48, grt (UTC plus 1) wrote:
> > > nit: "static constexpr" should come first, and this type doesn't really
need
> a
> > > name, so this could become:
> > >   static constexpr struct {
> > >     ....
> > >   } kImportItems[] = {
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
> > chrome/browser/first_run/first_run.cc:724: {prefs::kImportHistory,
> > importer::HISTORY},
> > On 2017/02/23 14:05:48, grt (UTC plus 1) wrote:
> > > the pedant in me would like to see this sorted either by pref name or by
the
> > > position of the ImportItem bit. :-)
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
> > chrome/browser/first_run/first_run.cc:728: };
> > On 2017/02/23 14:05:48, grt (UTC plus 1) wrote:
> > > prefs::kImportAutofillFormData and prefs::kImportSavedPasswords are
notably
> > > absent. why? is fixing https://crbug.com/601697 now as simple as adding
> > > {prefs::kImportSavedPasswords, importer::PASSWORDS} here? if i'm not
> mistaken,
> > > adding both will suddenly make the GP settings work as documented, right?
> > > 
> > >
https://www.chromium.org/administrators/policy-list-3#ImportAutofillFormData
> > > https://www.chromium.org/administrators/policy-list-3#ImportSavedPasswords
> > 
> > Interesting, I guess it probably is =D (though in practice it will depend on
> > that importer's support for these ImportItems).
> > 
> > Done.
> > 
> > One thing to note: I've long thought we should split first run only
> > master_preferences from prefs. On the other hand, policies only work with
real
> > prefs (so it's kind of weird to have master_preferences only things -- i.e.
> > kDistro*). And the prefs::kImport* values happen to be used at runtime after
> > first_run in a few corner UIs so I figured they might as well all use the
same
> > thing (those used to not be directly seeded by master_preferences but I
don't
> > think it's wrong that they now be).
> > 
> > Also, while writing this CL I realized we never even register any distro
prefs
> > (so I just went ahead and added logic to strip the kDistroDict from
> > master_preferences before mapping them to Preferences -- as well as cleaning
> it
> > from existing Preferences :) ).
> > 
> >
>
https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
> > chrome/browser/first_run/first_run.cc:739: std::unique_ptr<ImporterList>
> > importer_list(new ImporterList());
> > On 2017/02/23 14:05:48, grt (UTC plus 1) wrote:
> > > nit:
> > >     auto importer_list = base::MakeUnique<ImporterList>();
> > 
> > this was cut-paste from above but why not :)
> > 
> >
>
https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
> > File chrome/browser/first_run/first_run_browsertest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2705113005/diff/1/chrome/browser/first_run/fi...
> > chrome/browser/first_run/first_run_browsertest.cc:186: extern const char
> > kImportAll[] =
> > On 2017/02/23 14:05:48, grt (UTC plus 1) wrote:
> > > why do these all have external linkage? why not just "constexpr char
> > > kImportAll[] = "?
> > 
> > I forget, IIRC it was required for the template magic below to link when I
> > originally wrote it but this no longer appears to be the case (maybe an
> > improvement in VC++ :)).
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2705113005/diff/1/chrome/browser/ui/browser_u...
> > File chrome/browser/ui/browser_ui_prefs.cc (right):
> > 
> >
>
https://codereview.chromium.org/2705113005/diff/1/chrome/browser/ui/browser_u...
> > chrome/browser/ui/browser_ui_prefs.cc:74:
> > registry->RegisterBooleanPref(prefs::kImportAutofillFormData, false);
> > On 2017/02/23 14:05:48, grt (UTC plus 1) wrote:
> > > this will cause all checkboxes in chrome://settings/importData to be
> unchecked
> > > by default, no? i don't think this is the right thing.
> > 
> > Right, I'd seen those and initially thought it made sense for them to
reflect
> > what was imported on first run but you now make me realize that having these
> all
> > unchecked by default is not great.
> > 
> > Also, I thought it was merely reading the value without setting it back but
> now
> > I realize I just happen to know nothing about webUI: it's actually hooked up
> to
> > the pref (and toggles the actual pref live when using the UI) :
> >
>
https://cs.chromium.org/search/?q=(%22%7C%27)import_history(%22%7C%27)&sq=pac...
> > (I think the import_data_overlay.html bit is the critical hook).
> > 
> > Thinking about this some more, I don't think it makes sense for this UI to
be
> > driven by policies for the same pref as auto import... I don't even know
that
> it
> > makes sense for it to be driven by policies at all? That would mean that a
> > policy could prevent import even if the user explicitly wants to tick that
> box?
> > 
> > +ewald, can you look into this?

Agreed that the default for the chrome://settings/importData checkboxes should
just always be "checked." It shouldn't matter what the prefs at first run are.

Is crbug.com/695611 about finding an owner for tweaking the UI to not depend on
the prefs anymore? I think the MD settings team is the right team to own making
those UI changes. Though it sounds like that's the behavior that we've had for a
while based on their comments in there.

[commit-bot: I haz the power, 2017-02-24 02:14:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 02:14:55]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[grt (UTC plus 2), 2017-02-24 20:47:48]

this is awesomesauce. lgtm after the UI is fixed so that it doesn't depend on
the values of the prefs.

https://codereview.chromium.org/2705113005/diff/150001/chrome/browser/first_r...
File chrome/browser/first_run/first_run_browsertest.cc (right):

https://codereview.chromium.org/2705113005/diff/150001/chrome/browser/first_r...
chrome/browser/first_run/first_run_browsertest.cc:171: typedef
FirstRunMasterPrefsBrowserTestT<kImportDefault>
huh. there are compile errors here after all -- maybe the constants do need to
be extern?

[pastarmovj, 2017-03-02 06:35:57]

pastarmovj@chromium.org changed reviewers:
+ pastarmovj@chromium.org

[pastarmovj, 2017-03-02 06:35:58]

lgtm. Thanks for doing this!

[gab, 2017-03-21 22:10:44]

gab@chromium.org changed reviewers:
+ rkaplow@chromium.org
- ewald@chromium.org

[gab, 2017-03-21 22:10:45]

@rkaplow for actions.xml 1-liner

[gab, 2017-03-21 22:11:51]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 22:12:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2017-03-21 22:24:03]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[gab, 2017-03-21 22:24:47]

On 2017/02/24 20:47:48, grt (UTC plus 1) wrote:
> this is awesomesauce. lgtm after the UI is fixed so that it doesn't depend on
> the values of the prefs.

Dependency fixed @ http://crbug.com/695611. Also moved the registration as part
of this CL now that the prefs are no longer UI related.

[commit-bot: I haz the power, 2017-03-21 22:24:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2017-03-21 22:25:07]

https://codereview.chromium.org/2705113005/diff/150001/chrome/browser/first_r...
File chrome/browser/first_run/first_run_browsertest.cc (right):

https://codereview.chromium.org/2705113005/diff/150001/chrome/browser/first_r...
chrome/browser/first_run/first_run_browsertest.cc:171: typedef
FirstRunMasterPrefsBrowserTestT<kImportDefault>
On 2017/02/24 20:47:48, grt (UTC plus 1) wrote:
> huh. there are compile errors here after all -- maybe the constants do need to
> be extern?

Done.

[gab, 2017-03-21 22:26:38]

Patchset #9 (id:190001) has been deleted

[gab, 2017-03-21 22:26:58]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 22:27:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2017-03-21 22:28:12]

gab@chromium.org changed reviewers:
+ msw@chromium.org

[gab, 2017-03-21 22:28:12]

+msw for prefs registration move away from chrome\browser\ui\browser_ui_prefs.cc
(to first_run.cc)

[Alexei Svitkine (slow), 2017-03-21 22:30:42]

asvitkine@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2017-03-21 22:30:43]

actions.xml lgtm

[commit-bot: I haz the power, 2017-03-21 22:39:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 22:39:32]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msw, 2017-03-21 22:41:29]

https://codereview.chromium.org/2705113005/diff/210001/chrome/browser/first_r...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/2705113005/diff/210001/chrome/browser/first_r...
chrome/browser/first_run/first_run.cc:532:
registry->RegisterBooleanPref(prefs::kImportAutofillFormData, false);
q: Was changing the default true->false for all these intentional?
Comment #42's suggests that the checkboxes should be checked regardless of the
pref value, but if the pref value doesn't matter, why change the defaults to
false? Also then, why does AutoImport check the pref value on line 695?

[msw, 2017-03-21 22:43:57]

c/b/ui and the new first_run::RegisterProfilePrefs lgtm

https://codereview.chromium.org/2705113005/diff/210001/chrome/browser/first_r...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/2705113005/diff/210001/chrome/browser/first_r...
chrome/browser/first_run/first_run.cc:532:
registry->RegisterBooleanPref(prefs::kImportAutofillFormData, false);
On 2017/03/21 22:41:29, msw wrote:
> q: Was changing the default true->false for all these intentional?
> Comment #42's suggests that the checkboxes should be checked regardless of the
> pref value, but if the pref value doesn't matter, why change the defaults to
> false? Also then, why does AutoImport check the pref value on line 695?

Ha! I answered my own question by reading the CL title. I guess the checkboxes
mentioned live elsewhere and their behavior isn't contingent on these values.

[gab, 2017-03-21 22:53:41]

@msw: thanks, see below for details

https://codereview.chromium.org/2705113005/diff/210001/chrome/browser/first_r...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/2705113005/diff/210001/chrome/browser/first_r...
chrome/browser/first_run/first_run.cc:532:
registry->RegisterBooleanPref(prefs::kImportAutofillFormData, false);
On 2017/03/21 22:43:57, msw wrote:
> On 2017/03/21 22:41:29, msw wrote:
> > q: Was changing the default true->false for all these intentional?
> > Comment #42's suggests that the checkboxes should be checked regardless of
the
> > pref value, but if the pref value doesn't matter, why change the defaults to
> > false? Also then, why does AutoImport check the pref value on line 695?
> 
> Ha! I answered my own question by reading the CL title. I guess the checkboxes
> mentioned live elsewhere and their behavior isn't contingent on these values.

Right, used to be tangled up but was just separated specifically so we could
make these false : http://crbug.com/695611

[gab, 2017-03-21 23:00:37]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 23:01:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 23:13:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 23:13:46]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[gab, 2017-03-22 16:32:53]

The CQ bit was checked by gab@chromium.org

[gab, 2017-03-22 16:32:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
pastarmovj@chromium.org, grt@chromium.org, asvitkine@chromium.org,
msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2705113005/#ps230001
(title: "extern constexpr -> extern const?")

[commit-bot: I haz the power, 2017-03-22 16:33:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2017-03-22 16:33:23]

The CQ bit was unchecked by gab@chromium.org

[gab, 2017-03-22 16:39:09]

The CQ bit was checked by gab@chromium.org

[gab, 2017-03-22 16:39:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, grt@chromium.org, pastarmovj@chromium.org,
msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2705113005/#ps250001
(title: "Move registration to !OS_ANDROID per first_run.cc's GN rules")

[commit-bot: I haz the power, 2017-03-22 16:39:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 16:49:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 16:49:11]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[gab, 2017-03-22 17:50:12]

gab@chromium.org changed reviewers:
+ thakis@chromium.org

[gab, 2017-03-22 17:50:13]

+Nico for chrome_browser_main.cc (cleans up after an experiment)

[gab, 2017-03-22 17:55:20]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 17:55:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nico, 2017-03-22 18:09:37]

https://codereview.chromium.org/2705113005/diff/250001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/2705113005/diff/250001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:1684: first_run::AutoImport(profile_,
master_prefs_->import_bookmarks_path);
CL description says "Disable AutoImport by default", code as I read it does
"always run autoimport on first run" which sounds like the opposite. Which one
is it?

...aha, looks like the AutoImport() used to import history in addition to
bookmarks, but it still imports bookmarks. The finch thing disabled everything
including bookmarks, and base::win::IsEnterpriseManaged() also controlled
bookmarks.

So doesn't this do more than "import nothing" now, while it didn't before?

[commit-bot: I haz the power, 2017-03-22 18:26:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 18:26:22]

Dry run: This issue passed the CQ dry run.

[gab, 2017-03-22 18:54:47]

Description was changed from

==========
Disable AutoImport by default.

As decided in conclusion of experiment https://crbug.com/555550#c114

This simplifies the code a lot, dropping all the weird defaults accrued
over the years. AutoImport is now strictly driven by policies + master_prefs.

Also deprecated the distribution entries for the import prefs which were
duplicates of the prefs:: entries... added legacy mapping in one place in code
and it then simplifies the rest of the logic (the kDistroDict is never
registered so this was somewhat of a pain).

The new code also makes it trivial to fix issue 601697 :) -- done!
(i.e. added support for auto import of autofill form data and saved passwords
 although actual support will depend on the support of the importer
 for the imported browser -- which is the same as the support from a
 manual import).

BUG=555550, 601697
==========

to

==========
Update AutoImport to import nothing by default (in absence of policy and
master_prefs).

As decided in conclusion of experiment https://crbug.com/555550#c114

This simplifies the code a lot, dropping all the weird defaults accrued
over the years. AutoImport is now strictly driven by policies + master_prefs.

Also deprecated the distribution entries for the import prefs which were
duplicates of the prefs:: entries... added legacy mapping in one place in code
and it then simplifies the rest of the logic (the kDistroDict is never
registered so this was somewhat of a pain).

The new code also makes it trivial to fix issue 601697 :) -- done!
(i.e. added support for auto import of autofill form data and saved passwords
 although actual support will depend on the support of the importer
 for the imported browser -- which is the same as the support from a
 manual import).

BUG=555550, 601697
==========

[gab, 2017-03-22 18:55:49]

https://codereview.chromium.org/2705113005/diff/250001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/2705113005/diff/250001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:1684: first_run::AutoImport(profile_,
master_prefs_->import_bookmarks_path);
On 2017/03/22 18:09:37, Nico wrote:
> CL description says "Disable AutoImport by default", code as I read it does
> "always run autoimport on first run" which sounds like the opposite. Which one
> is it?
> 
> ...aha, looks like the AutoImport() used to import history in addition to
> bookmarks, but it still imports bookmarks. The finch thing disabled everything
> including bookmarks, and base::win::IsEnterpriseManaged() also controlled
> bookmarks.
> 
> So doesn't this do more than "import nothing" now, while it didn't before?

Right, updated title to "Update AutoImport to import nothing by default (in
absence of policy and master_prefs).", we now always make the
first_run::AutoImport() call but it no-ops in absence of policies/master_prefs
telling it to do something.

[Nico, 2017-03-22 18:59:45]

https://codereview.chromium.org/2705113005/diff/250001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/2705113005/diff/250001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:1684: first_run::AutoImport(profile_,
master_prefs_->import_bookmarks_path);
On 2017/03/22 18:55:49, gab wrote:
> On 2017/03/22 18:09:37, Nico wrote:
> > CL description says "Disable AutoImport by default", code as I read it does
> > "always run autoimport on first run" which sounds like the opposite. Which
one
> > is it?
> > 
> > ...aha, looks like the AutoImport() used to import history in addition to
> > bookmarks, but it still imports bookmarks. The finch thing disabled
everything
> > including bookmarks, and base::win::IsEnterpriseManaged() also controlled
> > bookmarks.
> > 
> > So doesn't this do more than "import nothing" now, while it didn't before?
> 
> Right, updated title to "Update AutoImport to import nothing by default (in
> absence of policy and master_prefs).", we now always make the
> first_run::AutoImport() call but it no-ops in absence of policies/master_prefs
> telling it to do something.

Maybe update the comment right above the if then?

[Nico, 2017-03-22 19:02:16]

Other than that, lgtm. You're losing the call to IsEnterpriseManaged(); I'm
assuming enterprise folks are ok with this going away.

[gab, 2017-03-22 19:06:26]

On 2017/03/22 19:02:16, Nico wrote:
> Other than that, lgtm. You're losing the call to IsEnterpriseManaged(); I'm
> assuming enterprise folks are ok with this going away.

Yes, it was only there as an explicit opt-out of the experiment for enterprise
so it's no longer relevant now that the experiment is removed (and yes
enterprise team is aware of new functionality).

https://codereview.chromium.org/2705113005/diff/250001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/2705113005/diff/250001/chrome/browser/chrome_...
chrome/browser/chrome_browser_main.cc:1684: first_run::AutoImport(profile_,
master_prefs_->import_bookmarks_path);
On 2017/03/22 18:59:45, Nico wrote:
> On 2017/03/22 18:55:49, gab wrote:
> > On 2017/03/22 18:09:37, Nico wrote:
> > > CL description says "Disable AutoImport by default", code as I read it
does
> > > "always run autoimport on first run" which sounds like the opposite. Which
> one
> > > is it?
> > > 
> > > ...aha, looks like the AutoImport() used to import history in addition to
> > > bookmarks, but it still imports bookmarks. The finch thing disabled
> everything
> > > including bookmarks, and base::win::IsEnterpriseManaged() also controlled
> > > bookmarks.
> > > 
> > > So doesn't this do more than "import nothing" now, while it didn't before?
> > 
> > Right, updated title to "Update AutoImport to import nothing by default (in
> > absence of policy and master_prefs).", we now always make the
> > first_run::AutoImport() call but it no-ops in absence of
policies/master_prefs
> > telling it to do something.
> 
> Maybe update the comment right above the if then?

Oh wow, had missed that comment, it's soooo old (we haven't shown a First Run UI
in forever). Updated: only relevant part is that this uses prefs and therefore
must run after they're initialized.

[gab, 2017-03-22 19:06:45]

The CQ bit was checked by gab@chromium.org

[gab, 2017-03-22 19:06:46]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
thakis@chromium.org, asvitkine@chromium.org, pastarmovj@chromium.org,
grt@chromium.org
Link to the patchset: https://codereview.chromium.org/2705113005/#ps270001
(title: "update comment in chrome_browser_main.cc")

[commit-bot: I haz the power, 2017-03-22 19:07:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 20:00:16]

CQ is committing da patch.

Bot data: {"patchset_id": 270001, "attempt_start_ts": 1490209605835810,
"parent_rev": "0fb24c9c9560467127a52f8a77df9cbb66f55b69", "commit_rev":
"dac09c03b9f3cc44adf71e8836f6bebabec7a61d"}

[commit-bot: I haz the power, 2017-03-22 20:01:07]

Description was changed from

==========
Update AutoImport to import nothing by default (in absence of policy and
master_prefs).

As decided in conclusion of experiment https://crbug.com/555550#c114

This simplifies the code a lot, dropping all the weird defaults accrued
over the years. AutoImport is now strictly driven by policies + master_prefs.

Also deprecated the distribution entries for the import prefs which were
duplicates of the prefs:: entries... added legacy mapping in one place in code
and it then simplifies the rest of the logic (the kDistroDict is never
registered so this was somewhat of a pain).

The new code also makes it trivial to fix issue 601697 :) -- done!
(i.e. added support for auto import of autofill form data and saved passwords
 although actual support will depend on the support of the importer
 for the imported browser -- which is the same as the support from a
 manual import).

BUG=555550, 601697
==========

to

==========
Update AutoImport to import nothing by default (in absence of policy and
master_prefs).

As decided in conclusion of experiment https://crbug.com/555550#c114

This simplifies the code a lot, dropping all the weird defaults accrued
over the years. AutoImport is now strictly driven by policies + master_prefs.

Also deprecated the distribution entries for the import prefs which were
duplicates of the prefs:: entries... added legacy mapping in one place in code
and it then simplifies the rest of the logic (the kDistroDict is never
registered so this was somewhat of a pain).

The new code also makes it trivial to fix issue 601697 :) -- done!
(i.e. added support for auto import of autofill form data and saved passwords
 although actual support will depend on the support of the importer
 for the imported browser -- which is the same as the support from a
 manual import).

BUG=555550, 601697

Review-Url: https://codereview.chromium.org/2705113005
Cr-Commit-Position: refs/heads/master@{#458853}
Committed:
https://chromium.googlesource.com/chromium/src/+/dac09c03b9f3cc44adf71e8836f6...
==========

[commit-bot: I haz the power, 2017-03-22 20:01:09]

Committed patchset #12 (id:270001) as
https://chromium.googlesource.com/chromium/src/+/dac09c03b9f3cc44adf71e8836f6...