Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(2286)

Issue 820133002: Reusing names of policy keys from policy_constants.h (Closed)

Created:
6 years ago by Łukasz Anforowicz
Modified:
5 years, 11 months ago
CC:
chromium-reviews, asvitkine+watch_chromium.org, chromoting-reviews_chromium.org, kelvinp, jar (doing other things), brettw
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Reusing names of policy keys from policy_constants.h Before this change, Chromoting-specific code under src/remoting/host/policy_hack was introducing its own constants for names of Chromoting policies, rather than reusing the constants already generated into policy_constants.h from src/components/policy/resources/policy_templates.json. This change fixes this (as asked for by crbug.com/368321). Removing names of policy keys from policy_watcher.cc is good because: - It enourages keeping policy_templates.json in-sync with remoting code (i.e. RemoteAccessHostTokenUrl was introduced to policy_watcher.cc but not to policy_templates.json; after this changes this should be less likely). - Code removal is good. Updating and keeping in-sync policy_templates.json is good because: - This is where rest of Chromium infrastructure looks for policy descriptions, examples, supported-since, etc. - Up-to-date policy_templates.json is a pre-requisite for removing src/remoting/host/policy_hack and instead reusing PolicyService and/or ConfigurationPolicyProvider(s) for Win/Linux/Mac. While working on the change, I've noticed that 5 out of 15 Chromoting policies are not covered in policy_templates.json (i.e. src/components/policy and src/remoting/host/policy_hack got out of sync). This change fixes this as well. Notes: - Making src/components/policy and src/remoting/host/policy_hack in-sync again is good for the short-term. - In the long-term we might want to somehow separate Chromoting-specific policies out of policy_templates.json (separate chromoting_policy_templates.json? separate supported_on?). This should be done in a separate changelist because 1) the separation concerns should not block current changelist and more importantly crrev.com/830193002 which gets rid of most code under policy_hack and 2) the separation is quite involved in itself (i.e. need to account for it2me/chromeos differences and make sure that tools for enterprise admins continue to support chromoting policies). BUG=368321 R=asvitkine@chromium.org, bartfab@chromium.org, mnissler@chromium.org, sergeyu@chromium.org Committed: https://chromium.googlesource.com/chromium/src/+/2b91a480c5ea6fc6e20703058b1d1748dd0de332

Patch Set 1 #

Total comments: 6

Patch Set 2 : Rebasing... #

Total comments: 8

Patch Set 3 : Removed all Chromoting policies from Chrome prefs. #

Total comments: 1

Patch Set 4 : Completed changes to policy_templates.json and related files. #

Total comments: 5

Patch Set 5 : Tweaked description of RemoteAccessHostMatchUsername policy. #

Total comments: 14

Patch Set 6 : Tweaks to policy_templates.json as suggested by Sergey and Bartosz. #

Total comments: 4

Patch Set 7 : Addressed code review feedback from Bartosz. #

Total comments: 10

Patch Set 8 : Addressed code review feedback from Bartosz. #

Patch Set 9 : Rebasing... #

Patch Set 10 : Marked the 5 "new" policies as chrome_os:42- #

Unified diffs Side-by-side diffs Delta from patch set Stats (+261 lines, -321 lines) Patch
M chrome/browser/policy/configuration_policy_handler_list_factory.cc View 1 2 1 chunk +0 lines, -27 lines 0 comments Download
M chrome/common/pref_names.h View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -10 lines 0 comments Download
M chrome/common/pref_names.cc View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -42 lines 0 comments Download
M chrome/test/data/policy/policy_test_cases.json View 1 2 3 4 5 6 7 8 1 chunk +15 lines, -47 lines 0 comments Download
M components/policy/resources/policy_templates.json View 1 2 3 4 5 6 7 8 9 11 chunks +100 lines, -12 lines 0 comments Download
M remoting/DEPS View 1 chunk +2 lines, -0 lines 0 comments Download
M remoting/host/BUILD.gn View 1 1 chunk +1 line, -0 lines 0 comments Download
M remoting/host/it2me/it2me_host.cc View 1 2 3 4 5 6 7 8 2 chunks +3 lines, -3 lines 0 comments Download
M remoting/host/policy_hack/policy_watcher.h View 1 chunk +0 lines, -40 lines 0 comments Download
M remoting/host/policy_hack/policy_watcher.cc View 1 2 3 4 5 6 7 8 4 chunks +33 lines, -60 lines 0 comments Download
M remoting/host/policy_hack/policy_watcher_unittest.cc View 3 chunks +63 lines, -49 lines 0 comments Download
M remoting/host/remoting_me2me_host.cc View 1 2 3 4 5 6 7 8 11 chunks +24 lines, -29 lines 0 comments Download
M remoting/remoting_host.gypi View 1 2 chunks +2 lines, -0 lines 0 comments Download
M remoting/remoting_test.gypi View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 3 chunks +17 lines, -2 lines 0 comments Download

Messages

Total messages: 45 (14 generated)
Łukasz Anforowicz
Hi Kelvin, Could you please review a *draft* of changes to reuse policy names from ...
6 years ago (2014-12-24 00:58:09 UTC) #2
kelvinp
I am not sure if I am the best person to review this CL, as ...
5 years, 11 months ago (2014-12-29 22:30:37 UTC) #3
Łukasz Anforowicz
Hi Daniel, Let me start by trying to give a little bit of context. I ...
5 years, 11 months ago (2014-12-30 21:12:01 UTC) #5
Mattias Nissler (ping if slow)
https://codereview.chromium.org/820133002/diff/20001/chrome/browser/policy/configuration_policy_handler_list_factory.cc File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right): https://codereview.chromium.org/820133002/diff/20001/chrome/browser/policy/configuration_policy_handler_list_factory.cc#newcode292 chrome/browser/policy/configuration_policy_handler_list_factory.cc:292: base::Value::TYPE_STRING }, If I'm not mistaken, you're not using ...
5 years, 11 months ago (2015-01-05 09:45:33 UTC) #7
Sergey Ulanov
src/remoting looks good, but as Mattias I'm not sure why you need any changes in ...
5 years, 11 months ago (2015-01-05 17:51:57 UTC) #9
Łukasz Anforowicz
I've removed Chromoting-related preferences altogether. I'll start working on removing the remaining "DO NOT SUBMIT"-s. ...
5 years, 11 months ago (2015-01-05 19:40:23 UTC) #11
Mattias Nissler (ping if slow)
policy and prefs bits LGTM, except for policy_templates.json (which you need to get separate OWNERS ...
5 years, 11 months ago (2015-01-06 08:30:21 UTC) #12
rmsousa
https://codereview.chromium.org/820133002/diff/40001/components/policy/resources/policy_templates.json File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/820133002/diff/40001/components/policy/resources/policy_templates.json#newcode774 components/policy/resources/policy_templates.json:774: 'supported_on DO NOT SUBMIT': 'rmsousa@chromium.org 2013-04-06 04:50:43 4386f0a', RemoteAccessHostTokenUrl ...
5 years, 11 months ago (2015-01-06 23:32:41 UTC) #13
Łukasz Anforowicz
Thanks for the feedback Mattias. With Renato's help, I've completed the changes to policy_templates.json and ...
5 years, 11 months ago (2015-01-07 17:47:15 UTC) #15
jar (doing other things)
+asvitkine for histograms.xml https://codereview.chromium.org/820133002/diff/60001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/820133002/diff/60001/tools/metrics/histograms/histograms.xml#newcode45095 tools/metrics/histograms/histograms.xml:45095: + <int value="289" label="RemoteAccessHostDebugOverridePolicies"/> Question (which ...
5 years, 11 months ago (2015-01-07 18:13:56 UTC) #17
Łukasz Anforowicz
https://codereview.chromium.org/820133002/diff/60001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/820133002/diff/60001/tools/metrics/histograms/histograms.xml#newcode45095 tools/metrics/histograms/histograms.xml:45095: + <int value="289" label="RemoteAccessHostDebugOverridePolicies"/> On 2015/01/07 18:13:56, jar wrote: ...
5 years, 11 months ago (2015-01-07 18:25:45 UTC) #18
Alexei Svitkine (slow)
histograms lgtm https://codereview.chromium.org/820133002/diff/60001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/820133002/diff/60001/tools/metrics/histograms/histograms.xml#newcode45095 tools/metrics/histograms/histograms.xml:45095: + <int value="289" label="RemoteAccessHostDebugOverridePolicies"/> On 2015/01/07 18:25:45, ...
5 years, 11 months ago (2015-01-07 18:39:39 UTC) #19
Sergey Ulanov
https://codereview.chromium.org/820133002/diff/60001/components/policy/resources/policy_templates.json File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/820133002/diff/60001/components/policy/resources/policy_templates.json#newcode762 components/policy/resources/policy_templates.json:762: If this setting is enabled, then the remote access ...
5 years, 11 months ago (2015-01-07 20:41:14 UTC) #20
Łukasz Anforowicz
https://codereview.chromium.org/820133002/diff/60001/components/policy/resources/policy_templates.json File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/820133002/diff/60001/components/policy/resources/policy_templates.json#newcode762 components/policy/resources/policy_templates.json:762: If this setting is enabled, then the remote access ...
5 years, 11 months ago (2015-01-07 21:59:16 UTC) #21
Sergey Ulanov
lgtm https://codereview.chromium.org/820133002/diff/80001/components/policy/resources/policy_templates.json File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/820133002/diff/80001/components/policy/resources/policy_templates.json#newcode762 components/policy/resources/policy_templates.json:762: If this setting is enabled, then the remote ...
5 years, 11 months ago (2015-01-09 00:46:56 UTC) #22
bartfab (slow)
https://codereview.chromium.org/820133002/diff/80001/components/policy/resources/policy_templates.json File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/820133002/diff/80001/components/policy/resources/policy_templates.json#newcode752 components/policy/resources/policy_templates.json:752: 'supported_on': ['chrome.*:25-'], So these policies were actually working as ...
5 years, 11 months ago (2015-01-09 19:20:15 UTC) #24
Łukasz Anforowicz
I've addressed pending comment from Sergey and most of comments from Bartosz. Bartosz - could ...
5 years, 11 months ago (2015-01-09 19:47:28 UTC) #25
bartfab (slow)
https://codereview.chromium.org/820133002/diff/80001/components/policy/resources/policy_templates.json File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/820133002/diff/80001/components/policy/resources/policy_templates.json#newcode752 components/policy/resources/policy_templates.json:752: 'supported_on': ['chrome.*:25-'], On 2015/01/09 19:47:27, lukasza wrote: > On ...
5 years, 11 months ago (2015-01-12 13:16:33 UTC) #27
Łukasz Anforowicz
Bartosz - I've tweaked the CL description + tried to address your concerns in the ...
5 years, 11 months ago (2015-01-12 18:32:59 UTC) #28
bartfab (slow)
components/policy/resources/policy_templates.json lgtm https://codereview.chromium.org/820133002/diff/80001/components/policy/resources/policy_templates.json File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/820133002/diff/80001/components/policy/resources/policy_templates.json#newcode836 components/policy/resources/policy_templates.json:836: The value is parsed as a JSON ...
5 years, 11 months ago (2015-01-13 18:49:46 UTC) #29
Łukasz Anforowicz
Bartosz - I tried addressing your code review feedback in patchset #7. Could you please ...
5 years, 11 months ago (2015-01-13 20:10:25 UTC) #30
Łukasz Anforowicz
Brett, Could you please review the DEPS changes here? The changes have been already LGTM-ed ...
5 years, 11 months ago (2015-01-13 23:05:52 UTC) #32
bartfab (slow)
Still LGTM, with a few more nits. https://codereview.chromium.org/820133002/diff/120001/chrome/test/data/policy/policy_test_cases.json File chrome/test/data/policy/policy_test_cases.json (left): https://codereview.chromium.org/820133002/diff/120001/chrome/test/data/policy/policy_test_cases.json#oldcode204 chrome/test/data/policy/policy_test_cases.json:204: "RemoteAccessHostFirewallTraversal": { ...
5 years, 11 months ago (2015-01-14 11:09:02 UTC) #33
Łukasz Anforowicz
I've addressed the nits pointed out by Bartosz. I talked with Sergey and he has ...
5 years, 11 months ago (2015-01-14 19:35:12 UTC) #35
bartfab (slow)
https://codereview.chromium.org/820133002/diff/120001/components/policy/resources/policy_templates.json File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/820133002/diff/120001/components/policy/resources/policy_templates.json#newcode588 components/policy/resources/policy_templates.json:588: 'supported_on': ['chrome.*:14-', 'chrome_os:41-'], On 2015/01/14 19:35:12, lukasza wrote: > ...
5 years, 11 months ago (2015-01-15 09:41:44 UTC) #36
Łukasz Anforowicz
Thanks for the feedback Bartosz! I've applied the suggested fix to patchset #10. https://codereview.chromium.org/820133002/diff/120001/components/policy/resources/policy_templates.json File ...
5 years, 11 months ago (2015-01-15 17:35:22 UTC) #38
bartfab (slow)
Still LGTM. https://codereview.chromium.org/820133002/diff/120001/components/policy/resources/policy_templates.json File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/820133002/diff/120001/components/policy/resources/policy_templates.json#newcode588 components/policy/resources/policy_templates.json:588: 'supported_on': ['chrome.*:14-', 'chrome_os:41-'], On 2015/01/15 17:35:22, lukasza ...
5 years, 11 months ago (2015-01-16 09:02:23 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/820133002/200001
5 years, 11 months ago (2015-01-16 21:29:41 UTC) #41
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/36634)
5 years, 11 months ago (2015-01-16 21:37:18 UTC) #43
commit-bot: I haz the power
Patchset 10 (id:??) landed as https://crrev.com/2b91a480c5ea6fc6e20703058b1d1748dd0de332 Cr-Commit-Position: refs/heads/master@{#311968}
5 years, 11 months ago (2015-01-16 22:41:43 UTC) #44
Sergey Ulanov
5 years, 11 months ago (2015-01-16 22:41:47 UTC) #45
Message was sent while issue was closed.
Committed patchset #10 (id:200001) manually as
2b91a480c5ea6fc6e20703058b1d1748dd0de332.

Powered by Google App Engine
This is Rietveld 408576698