Add a New Tab page location group policy

Add a New Tab page location group policy

BUG=601044
Review-Url: https://codereview.chromium.org/2643833002
Cr-Commit-Position: refs/heads/master@{#444964}
Committed: https://chromium.googlesource.com/chromium/src/+/fbd10854de98cb409beb7fe940f458539555b2e0

[MAD, 2017-01-18 15:32:47]

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

[commit-bot: I haz the power, 2017-01-18 15:33:04]

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

[MAD, 2017-01-18 15:35:37]

Description was changed from

==========
Add a New Tab page location group policy

BUG=601044
==========

to

==========
Add a New Tab page location group policy

BUG=601044
==========

[MAD, 2017-01-18 15:35:37]

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

[commit-bot: I haz the power, 2017-01-18 16:34:36]

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

[commit-bot: I haz the power, 2017-01-18 16:34:37]

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

[MAD, 2017-01-18 16:36:46]

Just noticed a browser test failure for PolicyPref stuff, working on it... ;-)

[MAD, 2017-01-18 16:51:11]

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

[commit-bot: I haz the power, 2017-01-18 16:51:39]

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

[MAD, 2017-01-18 16:52:29]

Done, should all be good now...

Le mer. 18 janv. 2017 à 11:36, <mad@chromium.org> a écrit :

> Just noticed a browser test failure for PolicyPref stuff, working on it...
> ;-)
>
> https://codereview.chromium.org/2643833002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2017-01-18 17:35:27]

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

[commit-bot: I haz the power, 2017-01-18 17:35:28]

Dry run: This issue passed the CQ dry run.

[pastarmovj, 2017-01-19 14:56:21]

Mostly fine. I only have a few comments and questions.

https://codereview.chromium.org/2643833002/diff/20001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client_browsertest.cc (right):

https://codereview.chromium.org/2643833002/diff/20001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client_browsertest.cc:85: GURL
ntp_url(chrome::kChromeUINewTabURL);
You can make this const.

https://codereview.chromium.org/2643833002/diff/20001/chrome/browser/policy/p...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/2643833002/diff/20001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2095:
IN_PROC_BROWSER_TEST_F(PolicyTest, NewTabPageLocation) {
IIRC this is tested by the policy_test_cases.json test below. It verifies that
the mentioned pref is changed when the policy is set.

https://codereview.chromium.org/2643833002/diff/20001/chrome/test/data/policy...
File chrome/test/data/policy/policy_test_cases.json (right):

https://codereview.chromium.org/2643833002/diff/20001/chrome/test/data/policy...
chrome/test/data/policy/policy_test_cases.json:70: { "pref": "homepage",
Shouldn't this be "newtab_page_location_override"?

https://codereview.chromium.org/2643833002/diff/20001/components/policy/core/...
File components/policy/core/common/policy_loader_win.cc (right):

https://codereview.chromium.org/2643833002/diff/20001/components/policy/core/...
components/policy/core/common/policy_loader_win.cc:67:
key::kMetricsReportingEnabled, key::kDefaultSearchProviderEnabled,
nit: I have a slight preference for the one item per line formatting as it makes
it easier to scan the list for a particular item. I guess this is what the
clang-format tool made of it though so I will leave it to you to decide if you
want to revert it or keep it as is.

https://codereview.chromium.org/2643833002/diff/20001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2643833002/diff/20001/components/policy/resou...
components/policy/resources/policy_templates.json:143: #   And don't forget to
also update the EnterprisePolicies enum of
Good idea! :)

https://codereview.chromium.org/2643833002/diff/20001/components/policy/resou...
components/policy/resources/policy_templates.json:311: The New Tab page is the
page opened when new tabs are created (including the one opened in new
windowsa). The pages that open on startup are controlled by the RestoreOnStartup
policies, which could be to open the New Tab page.
s/windowsa/windows/.

Also I would extend a little bit the second sentence into a paragraph on its own
by stating with something like. "This policy does not decide which pages to be
opened on start up. They are controlled by ...."

https://codereview.chromium.org/2643833002/diff/20001/components/policy/resou...
components/policy/resources/policy_templates.json:315: This policy is not
available on Windows instances that are not joined to an Active Directory
domain.''',
Add one more sentence below or above this one. e.g. "If the policy is not set or
left empty the default new tab page is used." It is a MS recommendation for the
policy documentation to always state explicitly the default behavior when the
policy is not set.

https://codereview.chromium.org/2643833002/diff/40001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client_browsertest.cc (right):

https://codereview.chromium.org/2643833002/diff/40001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client_browsertest.cc:85: GURL
ntp_url(chrome::kChromeUINewTabURL);
this can be const right?

https://codereview.chromium.org/2643833002/diff/40001/components/policy/core/...
File components/policy/core/common/policy_loader_win.cc (right):

https://codereview.chromium.org/2643833002/diff/40001/components/policy/core/...
components/policy/core/common/policy_loader_win.cc:67:
key::kMetricsReportingEnabled, key::kDefaultSearchProviderEnabled,
nit: I have a weak preference to the one policy per line formatting. I guess
this is what clang-format does but it makes it harder to see if a particular
entry is in the list. WDYT?

https://codereview.chromium.org/2643833002/diff/40001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2643833002/diff/40001/components/policy/resou...
components/policy/resources/policy_templates.json:143: #   And don't forget to
also update the EnterprisePolicies enum of
Nice addition :)

https://codereview.chromium.org/2643833002/diff/40001/components/policy/resou...
components/policy/resources/policy_templates.json:311: The New Tab page is the
page opened when new tabs are created (including the one opened in new
windowsa). The pages that open on startup are controlled by the RestoreOnStartup
policies, which could be to open the New Tab page.
s/windowsa/windows/

[MAD, 2017-01-19 15:45:51]

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

[commit-bot: I haz the power, 2017-01-19 15:46:04]

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

[MAD, 2017-01-19 16:13:30]

Thanks for the comments, all addressed/answered.
Anything else?

BYE
MAD

https://codereview.chromium.org/2643833002/diff/20001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client_browsertest.cc (right):

https://codereview.chromium.org/2643833002/diff/20001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client_browsertest.cc:85: GURL
ntp_url(chrome::kChromeUINewTabURL);
On 2017/01/19 14:56:21, pastarmovj wrote:
> You can make this const.

Done.

https://codereview.chromium.org/2643833002/diff/20001/chrome/browser/policy/p...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/2643833002/diff/20001/chrome/browser/policy/p...
chrome/browser/policy/policy_browsertest.cc:2095:
IN_PROC_BROWSER_TEST_F(PolicyTest, NewTabPageLocation) {
On 2017/01/19 14:56:21, pastarmovj wrote:
> IIRC this is tested by the policy_test_cases.json test below. It verifies that
> the mentioned pref is changed when the policy is set.

Ho! Good point... Thanks!

https://codereview.chromium.org/2643833002/diff/20001/chrome/test/data/policy...
File chrome/test/data/policy/policy_test_cases.json (right):

https://codereview.chromium.org/2643833002/diff/20001/chrome/test/data/policy...
chrome/test/data/policy/policy_test_cases.json:70: { "pref": "homepage",
On 2017/01/19 14:56:21, pastarmovj wrote:
> Shouldn't this be "newtab_page_location_override"?

Done.

https://codereview.chromium.org/2643833002/diff/20001/components/policy/core/...
File components/policy/core/common/policy_loader_win.cc (right):

https://codereview.chromium.org/2643833002/diff/20001/components/policy/core/...
components/policy/core/common/policy_loader_win.cc:67:
key::kMetricsReportingEnabled, key::kDefaultSearchProviderEnabled,
On 2017/01/19 14:56:21, pastarmovj wrote:
> nit: I have a slight preference for the one item per line formatting as it
makes
> it easier to scan the list for a particular item. I guess this is what the
> clang-format tool made of it though so I will leave it to you to decide if you
> want to revert it or keep it as is.

I prefer to always run git cl format (and git cl lint), so if I change it back
to how it was, git cl format will keep changing it back. I also don't  always
agree with the formating of git cl format, but I prefer consistency. :-)

https://codereview.chromium.org/2643833002/diff/20001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2643833002/diff/20001/components/policy/resou...
components/policy/resources/policy_templates.json:143: #   And don't forget to
also update the EnterprisePolicies enum of
On 2017/01/19 14:56:21, pastarmovj wrote:
> Good idea! :)

Acknowledged.

https://codereview.chromium.org/2643833002/diff/20001/components/policy/resou...
components/policy/resources/policy_templates.json:311: The New Tab page is the
page opened when new tabs are created (including the one opened in new
windowsa). The pages that open on startup are controlled by the RestoreOnStartup
policies, which could be to open the New Tab page.
On 2017/01/19 14:56:21, pastarmovj wrote:
> s/windowsa/windows/.
> 
> Also I would extend a little bit the second sentence into a paragraph on its
own
> by stating with something like. "This policy does not decide which pages to be
> opened on start up. They are controlled by ...."

Done.

https://codereview.chromium.org/2643833002/diff/20001/components/policy/resou...
components/policy/resources/policy_templates.json:315: This policy is not
available on Windows instances that are not joined to an Active Directory
domain.''',
On 2017/01/19 14:56:21, pastarmovj wrote:
> Add one more sentence below or above this one. e.g. "If the policy is not set
or
> left empty the default new tab page is used." It is a MS recommendation for
the
> policy documentation to always state explicitly the default behavior when the
> policy is not set.

Done.

https://codereview.chromium.org/2643833002/diff/40001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client_browsertest.cc (right):

https://codereview.chromium.org/2643833002/diff/40001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client_browsertest.cc:85: GURL
ntp_url(chrome::kChromeUINewTabURL);
On 2017/01/19 14:56:21, pastarmovj wrote:
> this can be const right?

Done.

https://codereview.chromium.org/2643833002/diff/40001/components/policy/core/...
File components/policy/core/common/policy_loader_win.cc (right):

https://codereview.chromium.org/2643833002/diff/40001/components/policy/core/...
components/policy/core/common/policy_loader_win.cc:67:
key::kMetricsReportingEnabled, key::kDefaultSearchProviderEnabled,
On 2017/01/19 14:56:21, pastarmovj wrote:
> nit: I have a weak preference to the one policy per line formatting. I guess
> this is what clang-format does but it makes it harder to see if a particular
> entry is in the list. WDYT?

Done.

https://codereview.chromium.org/2643833002/diff/40001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/2643833002/diff/40001/components/policy/resou...
components/policy/resources/policy_templates.json:143: #   And don't forget to
also update the EnterprisePolicies enum of
On 2017/01/19 14:56:21, pastarmovj wrote:
> Nice addition :)

Acknowledged.

https://codereview.chromium.org/2643833002/diff/40001/components/policy/resou...
components/policy/resources/policy_templates.json:311: The New Tab page is the
page opened when new tabs are created (including the one opened in new
windowsa). The pages that open on startup are controlled by the RestoreOnStartup
policies, which could be to open the New Tab page.
On 2017/01/19 14:56:21, pastarmovj wrote:
> s/windowsa/windows/

Done.

[commit-bot: I haz the power, 2017-01-19 16:49:32]

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

[commit-bot: I haz the power, 2017-01-19 16:49:33]

Dry run: This issue passed the CQ dry run.

[pastarmovj, 2017-01-19 17:16:38]

lgtm

[MAD, 2017-01-19 19:07:06]

mad@chromium.org changed reviewers:
+ anthonyvd@chromium.org

[MAD, 2017-01-19 19:07:07]

Thanks!

Adding anthonyvd@ for chrome\browser\profiles\profile_impl.cc OWNER approval.

BYE
MAD

[anthonyvd, 2017-01-19 20:30:11]

c/b/profiles/profile_impl.cc lgtm

[MAD, 2017-01-20 03:09:55]

The CQ bit was checked by mad@chromium.org

[commit-bot: I haz the power, 2017-01-20 03:10:12]

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-01-20 03:15:41]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484881795371350,
"parent_rev": "2ca43b1c99640f0578ba96397f820e1a0cfe7485", "commit_rev":
"fbd10854de98cb409beb7fe940f458539555b2e0"}

[commit-bot: I haz the power, 2017-01-20 03:16:15]

Description was changed from

==========
Add a New Tab page location group policy

BUG=601044
==========

to

==========
Add a New Tab page location group policy

BUG=601044

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

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

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/fbd10854de98cb409beb7fe940f4...

[Wez, 2017-02-08 01:01:53]

Description was changed from

==========
Add a New Tab page location group policy

BUG=601044

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

to

==========
Add a New Tab page location group policy

BUG=601044

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

[Wez, 2017-02-08 01:01:53]

wez@chromium.org changed reviewers:
+ holte@chromium.org

[Wez, 2017-02-08 02:08:21]

Hallo holte@chromium.org!
Due to a depot_tools patch which mistakenly removed the OWNERS check for
non-source files (see crbug.com/684270), the following files landed in this CL
and need a retrospective review from you:
	tools/metrics/histograms/histograms.xml
Thanks,
Wez

[Alexei Svitkine (slow), 2017-02-08 18:19:48]

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

[Alexei Svitkine (slow), 2017-02-08 18:19:49]

lgtm for histograms.xml