Options/MD Settings: use new prefs to drive import data dialog

Options/MD Settings: use new prefs to drive import data dialog

Previously, first run and the import data dialog used the same prefs.
This led to confusion and make it hard to change. This CL splits the
import data dialog (chrome://settings/importData) to use a different
set of prefs but continue to harness the same policies.

R=gab@chromium.org,dpapad@chromium.org
BUG=695611
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2727813002
Cr-Commit-Position: refs/heads/master@{#455149}
Committed: https://chromium.googlesource.com/chromium/src/+/0b1f71dfa6f7dc388e7b7bec334c030b9d203e6e

[Dan Beam, 2017-03-02 01:32:09]

Description was changed from

==========
Options/MD Settings: use new prefs to drive import data dialog

Previously, first run and the import data dialog used the same prefs.
This led to confusion and make it hard to change. This CL splits the
import data dialog (chrome://settings/importData) to use a different
set of prefs but continue to harness the same policies.

R=gab@chromium.org
BUG=695611
==========

to

==========
Options/MD Settings: use new prefs to drive import data dialog

Previously, first run and the import data dialog used the same prefs.
This led to confusion and make it hard to change. This CL splits the
import data dialog (chrome://settings/importData) to use a different
set of prefs but continue to harness the same policies.

R=gab@chromium.org
BUG=695611
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2017-03-02 01:33:33]

Description was changed from

==========
Options/MD Settings: use new prefs to drive import data dialog

Previously, first run and the import data dialog used the same prefs.
This led to confusion and make it hard to change. This CL splits the
import data dialog (chrome://settings/importData) to use a different
set of prefs but continue to harness the same policies.

R=gab@chromium.org
BUG=695611
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Options/MD Settings: use new prefs to drive import data dialog

Previously, first run and the import data dialog used the same prefs.
This led to confusion and make it hard to change. This CL splits the
import data dialog (chrome://settings/importData) to use a different
set of prefs but continue to harness the same policies.

R=gab@chromium.org,dpapad@chromium.org
BUG=695611
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2017-03-02 01:33:33]

dbeam@chromium.org changed reviewers:
+ dpapad@chromium.org

[gab, 2017-03-02 01:47:33]

Nice! lgtm if it works :) -- unless UI folks elect that unticked is better than
I guess we don't even need to do this

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

https://codereview.chromium.org/2727813002/diff/1/chrome/browser/ui/browser_u...
chrome/browser/ui/browser_ui_prefs.cc:74: // TODO(gab): flip the kImport*
defaults to false here.
Let's not bother, will merely create a conflict with my pending CL :)

https://codereview.chromium.org/2727813002/diff/1/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/2727813002/diff/1/chrome/common/pref_names.h#...
chrome/common/pref_names.h:399: // Used in the "Import Data" dialog in settings
(chrome://settings/importData).
No comments in the .h like other prefs.

[Dan Beam, 2017-03-02 01:50:06]

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

[commit-bot: I haz the power, 2017-03-02 01:51:09]

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

[Dan Beam, 2017-03-02 01:55:56]

Patchset #2 (id:20001) has been deleted

[Dan Beam, 2017-03-02 01:57:10]

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

[Dan Beam, 2017-03-02 02:00:58]

+pastarmovj@ for chrome/browser/policy/OWNERS (and general thoughts)

note: if we want to add new master_prefs for just the *import* dialog, we can
potentially do that.  right now policy will continue to be honored.

this simply splits the import dialog off to a different set of prefs so that
gab@ can change the defaults of the *first run* import prefs (which he his
changing here: https://codereview.chromium.org/2705113005/)

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

https://codereview.chromium.org/2727813002/diff/1/chrome/browser/ui/browser_u...
chrome/browser/ui/browser_ui_prefs.cc:74: // TODO(gab): flip the kImport*
defaults to false here.
On 2017/03/02 01:47:32, gab (slow - travel and perf) wrote:
> Let's not bother, will merely create a conflict with my pending CL :)

Done.

https://codereview.chromium.org/2727813002/diff/1/chrome/common/pref_names.h
File chrome/common/pref_names.h (right):

https://codereview.chromium.org/2727813002/diff/1/chrome/common/pref_names.h#...
chrome/common/pref_names.h:399: // Used in the "Import Data" dialog in settings
(chrome://settings/importData).
On 2017/03/02 01:47:32, gab (slow - travel and perf) wrote:
> No comments in the .h like other prefs.

finnnne

[Dan Beam, 2017-03-02 02:01:06]

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

[commit-bot: I haz the power, 2017-03-02 02:02: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-03-02 03:20:55]

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

[commit-bot: I haz the power, 2017-03-02 03:20:56]

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_...)

[gab, 2017-03-02 07:25:35]

On 2017/03/02 02:00:58, Dan Beam wrote:
> +pastarmovj@ for chrome/browser/policy/OWNERS (and general thoughts)
> 
> note: if we want to add new master_prefs for just the *import* dialog, we can
> potentially do that.  right now policy will continue to be honored.

Note: master_preferences are actually all prefs -- master_preferences is copied
to Preferences as-is on first run. So we get master_preferences for free here :)

> 
> this simply splits the import dialog off to a different set of prefs so that
> gab@ can change the defaults of the *first run* import prefs (which he his
> changing here: https://codereview.chromium.org/2705113005/)

[Dan Beam, 2017-03-03 03:15:13]

ping pastarmovj@

I haven't had time enough to fully investigate the tests that are failing.  It
would be nice to simplify them (like maybe mock out the C++ side and just do JS
/ C++ testing separately).

I understand generally how JS<->C++ works in the test, but using
domAutomationController is kinda fragile, IMO, because of the way that we
navigate to pages (we ensure that the 'load' event has fired but anything done
after that can race/flake, so post-load chrome.send() calls may not work
consistently).

Any tips in why this test would break (even though I believe locally this still
showed indicators and uses the same mechanism to set the pref by policy) or how
to fix would be appreciated.

[pastarmovj, 2017-03-03 06:48:25]

I don't think this is the most elegant solution but nothing better comes to mind
either. 

LGTM with a nit.

https://codereview.chromium.org/2727813002/diff/40001/chrome/browser/policy/c...
File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right):

https://codereview.chromium.org/2727813002/diff/40001/chrome/browser/policy/c...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:324:
base::Value::Type::BOOLEAN },
nit: Since this is a rather special case to have one policy map in two prefs can
you move all of those down below all the rest and write a comment above them why
is this needed.

[dpapad, 2017-03-07 01:39:41]

LGTM for WebUI related code.

[Dan Beam, 2017-03-07 02:23:59]

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

[commit-bot: I haz the power, 2017-03-07 02:24:50]

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

[Dan Beam, 2017-03-07 02:44:56]

https://codereview.chromium.org/2727813002/diff/40001/chrome/browser/policy/c...
File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right):

https://codereview.chromium.org/2727813002/diff/40001/chrome/browser/policy/c...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:324:
base::Value::Type::BOOLEAN },
On 2017/03/03 06:48:25, pastarmovj wrote:
> nit: Since this is a rather special case to have one policy map in two prefs
can
> you move all of those down below all the rest and write a comment above them
why
> is this needed.

Done.

[Dan Beam, 2017-03-07 04:04:36]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2017-03-07 04:04:37]

The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org,
pastarmovj@chromium.org, dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2727813002/#ps80001
(title: "pastarmovj@ review")

[commit-bot: I haz the power, 2017-03-07 04:05:37]

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-07 06:59:05]

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

[commit-bot: I haz the power, 2017-03-07 06:59:07]

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_...)

[Dan Beam, 2017-03-07 15:56:13]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2017-03-07 15:57:11]

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-07 18:36:40]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488902173300090,
"parent_rev": "d23ef9b37d71d31b82613f8ddb8da614cd54c665", "commit_rev":
"0b1f71dfa6f7dc388e7b7bec334c030b9d203e6e"}

[commit-bot: I haz the power, 2017-03-07 18:37:25]

Description was changed from

==========
Options/MD Settings: use new prefs to drive import data dialog

Previously, first run and the import data dialog used the same prefs.
This led to confusion and make it hard to change. This CL splits the
import data dialog (chrome://settings/importData) to use a different
set of prefs but continue to harness the same policies.

R=gab@chromium.org,dpapad@chromium.org
BUG=695611
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Options/MD Settings: use new prefs to drive import data dialog

Previously, first run and the import data dialog used the same prefs.
This led to confusion and make it hard to change. This CL splits the
import data dialog (chrome://settings/importData) to use a different
set of prefs but continue to harness the same policies.

R=gab@chromium.org,dpapad@chromium.org
BUG=695611
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2727813002
Cr-Commit-Position: refs/heads/master@{#455149}
Committed:
https://chromium.googlesource.com/chromium/src/+/0b1f71dfa6f7dc388e7b7bec334c...
==========

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

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/0b1f71dfa6f7dc388e7b7bec334c...