Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
309553011:
Enable policy support for registering protocol handler. (Closed)
|
Created:
6 years, 6 months ago by kaliamoorthi Modified:
6 years, 6 months ago CC:
chromium-reviews, joaodasilva+watch_chromium.org, asvitkine+watch_chromium.org, jar+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@Issue_116119_pre Visibility:
Public. |
DescriptionEnable policy support for registering protocol handler.
The CL adds a new policy for registering protocol handlers.
BUG=116119
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277475
Patch Set 1 #
Total comments: 34
Patch Set 2 : Fixes comments from Bartosz #Patch Set 3 : After rebase #
Total comments: 22
Patch Set 4 : Fixes nits #
Total comments: 34
Patch Set 5 : Fixes nits #Patch Set 6 : Adds IDS_POLICY_LEVEL_ERROR grit_whitelist.txt to fix build error for ios #
Total comments: 1
Patch Set 7 : Rebase and fixes nits #
Total comments: 5
Patch Set 8 : Updates policy_templates as per the comments from Daniel #Patch Set 9 : Fix for the windows compile error #Messages
Total messages: 17 (0 generated)
PTAL
https://codereview.chromium.org/309553011/diff/1/chrome/test/data/policy/poli... File chrome/test/data/policy/policy_test_cases.json (right): https://codereview.chromium.org/309553011/diff/1/chrome/test/data/policy/poli... chrome/test/data/policy/policy_test_cases.json:1576: "check_for_mandatory" : false 1: Can this policy only be set as recommended, not as mandatory? This is not documented anyhwere. 2: Nit: Remove space before ":". https://codereview.chromium.org/309553011/diff/1/chrome/test/data/policy/poli... chrome/test/data/policy/policy_test_cases.json:1587: "check_for_mandatory" : false 1: Can this policy only be set as recommended, not as mandatory? This is not documented anyhwere. 2: Nit: Remove space before ":". https://codereview.chromium.org/309553011/diff/1/components/policy/core/brows... File components/policy/core/browser/configuration_policy_handler.cc (right): https://codereview.chromium.org/309553011/diff/1/components/policy/core/brows... components/policy/core/browser/configuration_policy_handler.cc:388: SimpleSchemaValidatingPolicyHandler::SimpleSchemaValidatingPolicyHandler( Nit: Reorder these constructors so that the implementation order matches the declaration order. https://codereview.chromium.org/309553011/diff/1/components/policy/core/brows... components/policy/core/browser/configuration_policy_handler.cc:408: policies.Get(SchemaValidatingPolicyHandler::policy_name()); Nit: No need for the SchemaValidatingPolicyHandler:: here. https://codereview.chromium.org/309553011/diff/1/components/policy/core/brows... components/policy/core/browser/configuration_policy_handler.cc:410: return false; SchemaValidatingPolicyHandler returns |true| if there is no setting for the current policy. Are you sure that returning |false| instead is the right thing to do? https://codereview.chromium.org/309553011/diff/1/components/policy/core/brows... components/policy/core/browser/configuration_policy_handler.cc:414: if (errors != NULL) Nit: "if (errors)" is sufficient https://codereview.chromium.org/309553011/diff/1/components/policy/core/brows... File components/policy/core/browser/configuration_policy_handler.h (right): https://codereview.chromium.org/309553011/diff/1/components/policy/core/brows... components/policy/core/browser/configuration_policy_handler.h:276: class POLICY_EXPORT SimpleSchemaValidatingPolicyHandler 1: Document the properties of this policy handler type. 2: Please add tests for this class. https://codereview.chromium.org/309553011/diff/1/components/policy/core/brows... components/policy/core/browser/configuration_policy_handler.h:288: bool mandatory); Nit: How about renaming these to something like |allow_recommended| and |allow_mandatory|, making it clearer what they mean? https://codereview.chromium.org/309553011/diff/1/components/policy/core/brows... components/policy/core/browser/configuration_policy_handler.h:294: Nit: Remove blank line. https://codereview.chromium.org/309553011/diff/1/components/policy/core/brows... components/policy/core/browser/configuration_policy_handler.h:300: bool recommended_; Nit: const. https://codereview.chromium.org/309553011/diff/1/components/policy/core/brows... components/policy/core/browser/configuration_policy_handler.h:301: bool mandatory_; Nit: const. https://codereview.chromium.org/309553011/diff/1/components/policy/resources/... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/309553011/diff/1/components/policy/resources/... components/policy/resources/policy_templates.json:2694: 'supported_on': [ Nit: Would the supported_on entries not have all fit on one line? https://codereview.chromium.org/309553011/diff/1/components/policy/resources/... components/policy/resources/policy_templates.json:2701: 'dynamic_refresh': False, Why can this not be dynamically changed? https://codereview.chromium.org/309553011/diff/1/components/policy/resources/... components/policy/resources/policy_templates.json:2707: 'desc': '''Allows you to register a list of protocol handlers.''', You should briefly document the format of the list entries, what %s does, and how merging with user-set handlers works. https://codereview.chromium.org/309553011/diff/1/components/policy/resources/... components/policy/resources/policy_templates.json:2736: 'dynamic_refresh': False, Why can this not be dynamically changed? https://codereview.chromium.org/309553011/diff/1/components/policy/resources/... components/policy/resources/policy_templates.json:2742: 'desc': '''Allows you to ignore a list of protocol handlers.''', What does this mean? The documentation is not clear at all. https://codereview.chromium.org/309553011/diff/1/components/policy_strings.grdp File components/policy_strings.grdp (right): https://codereview.chromium.org/309553011/diff/1/components/policy_strings.gr... components/policy_strings.grdp:204: <message name="IDS_POLICY_LEVEL_ERROR" desc="Text displayed in the status column when a policy is set in an unsupported level."> Nit: s/in an/at an/
PTAL. https://codereview.chromium.org/309553011/diff/1/chrome/test/data/policy/poli... File chrome/test/data/policy/policy_test_cases.json (right): https://codereview.chromium.org/309553011/diff/1/chrome/test/data/policy/poli... chrome/test/data/policy/policy_test_cases.json:1576: "check_for_mandatory" : false On 2014/06/03 11:58:21, bartfab wrote: > 1: Can this policy only be set as recommended, not as mandatory? This is not > documented anyhwere. > 2: Nit: Remove space before ":". This is documented in policy_templates.json. Space removed. https://codereview.chromium.org/309553011/diff/1/chrome/test/data/policy/poli... chrome/test/data/policy/policy_test_cases.json:1587: "check_for_mandatory" : false On 2014/06/03 11:58:21, bartfab wrote: > 1: Can this policy only be set as recommended, not as mandatory? This is not > documented anyhwere. > 2: Nit: Remove space before ":". This is documented in policy_templates.json. Space removed. https://codereview.chromium.org/309553011/diff/1/components/policy/core/brows... File components/policy/core/browser/configuration_policy_handler.cc (right): https://codereview.chromium.org/309553011/diff/1/components/policy/core/brows... components/policy/core/browser/configuration_policy_handler.cc:388: SimpleSchemaValidatingPolicyHandler::SimpleSchemaValidatingPolicyHandler( On 2014/06/03 11:58:21, bartfab wrote: > Nit: Reorder these constructors so that the implementation order matches the > declaration order. Done. https://codereview.chromium.org/309553011/diff/1/components/policy/core/brows... components/policy/core/browser/configuration_policy_handler.cc:408: policies.Get(SchemaValidatingPolicyHandler::policy_name()); On 2014/06/03 11:58:21, bartfab wrote: > Nit: No need for the SchemaValidatingPolicyHandler:: here. Done. https://codereview.chromium.org/309553011/diff/1/components/policy/core/brows... components/policy/core/browser/configuration_policy_handler.cc:410: return false; On 2014/06/03 11:58:21, bartfab wrote: > SchemaValidatingPolicyHandler returns |true| if there is no setting for the > current policy. Are you sure that returning |false| instead is the right thing > to do? Done. https://codereview.chromium.org/309553011/diff/1/components/policy/core/brows... components/policy/core/browser/configuration_policy_handler.cc:414: if (errors != NULL) On 2014/06/03 11:58:21, bartfab wrote: > Nit: "if (errors)" is sufficient Done. https://codereview.chromium.org/309553011/diff/1/components/policy/core/brows... File components/policy/core/browser/configuration_policy_handler.h (right): https://codereview.chromium.org/309553011/diff/1/components/policy/core/brows... components/policy/core/browser/configuration_policy_handler.h:276: class POLICY_EXPORT SimpleSchemaValidatingPolicyHandler On 2014/06/03 11:58:21, bartfab wrote: > 1: Document the properties of this policy handler type. > 2: Please add tests for this class. Done. https://codereview.chromium.org/309553011/diff/1/components/policy/core/brows... components/policy/core/browser/configuration_policy_handler.h:288: bool mandatory); On 2014/06/03 11:58:21, bartfab wrote: > Nit: How about renaming these to something like |allow_recommended| and > |allow_mandatory|, making it clearer what they mean? Done. https://codereview.chromium.org/309553011/diff/1/components/policy/core/brows... components/policy/core/browser/configuration_policy_handler.h:294: On 2014/06/03 11:58:21, bartfab wrote: > Nit: Remove blank line. Done. https://codereview.chromium.org/309553011/diff/1/components/policy/core/brows... components/policy/core/browser/configuration_policy_handler.h:300: bool recommended_; On 2014/06/03 11:58:21, bartfab wrote: > Nit: const. Done. https://codereview.chromium.org/309553011/diff/1/components/policy/core/brows... components/policy/core/browser/configuration_policy_handler.h:301: bool mandatory_; On 2014/06/03 11:58:21, bartfab wrote: > Nit: const. Done. https://codereview.chromium.org/309553011/diff/1/components/policy/resources/... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/309553011/diff/1/components/policy/resources/... components/policy/resources/policy_templates.json:2694: 'supported_on': [ On 2014/06/03 11:58:21, bartfab wrote: > Nit: Would the supported_on entries not have all fit on one line? Done. https://codereview.chromium.org/309553011/diff/1/components/policy/resources/... components/policy/resources/policy_templates.json:2701: 'dynamic_refresh': False, On 2014/06/03 11:58:21, bartfab wrote: > Why can this not be dynamically changed? We discussed this, I'll file a separate bug. https://codereview.chromium.org/309553011/diff/1/components/policy/resources/... components/policy/resources/policy_templates.json:2707: 'desc': '''Allows you to register a list of protocol handlers.''', On 2014/06/03 11:58:21, bartfab wrote: > You should briefly document the format of the list entries, what %s does, and > how merging with user-set handlers works. Done. https://codereview.chromium.org/309553011/diff/1/components/policy/resources/... components/policy/resources/policy_templates.json:2736: 'dynamic_refresh': False, On 2014/06/03 11:58:21, bartfab wrote: > Why can this not be dynamically changed? We discussed this, I'll file a separate bug. https://codereview.chromium.org/309553011/diff/1/components/policy/resources/... components/policy/resources/policy_templates.json:2742: 'desc': '''Allows you to ignore a list of protocol handlers.''', On 2014/06/03 11:58:21, bartfab wrote: > What does this mean? The documentation is not clear at all. I have expanded this. https://codereview.chromium.org/309553011/diff/1/components/policy_strings.grdp File components/policy_strings.grdp (right): https://codereview.chromium.org/309553011/diff/1/components/policy_strings.gr... components/policy_strings.grdp:204: <message name="IDS_POLICY_LEVEL_ERROR" desc="Text displayed in the status column when a policy is set in an unsupported level."> On 2014/06/03 11:58:21, bartfab wrote: > Nit: s/in an/at an/ Done.
https://codereview.chromium.org/309553011/diff/50001/chrome/test/data/policy/... File chrome/test/data/policy/policy_test_cases.json (right): https://codereview.chromium.org/309553011/diff/50001/chrome/test/data/policy/... chrome/test/data/policy/policy_test_cases.json:1581: "test_policy": { "RegisteredProtocolHandlers": {"protocol": "test", "url": "http://test.com/%s", "default": "true"} }, Nit: s/test.com/example.com/ (example.com is specifically reserved for such purposes) https://codereview.chromium.org/309553011/diff/50001/chrome/test/data/policy/... chrome/test/data/policy/policy_test_cases.json:1592: "test_policy": { "IgnoredProtocolHandlers": {"protocol": "test1", "url": "http://test1.com/%s"} }, Nit 1: s/test1.com/example.com/ (example.com is specifically reserved for such purposes) Nit 2: Why not use the same protocol "test" here? The two tests will be run independently and will not get in each other's ways. https://codereview.chromium.org/309553011/diff/50001/components/policy/core/b... File components/policy/core/browser/configuration_policy_handler.h (right): https://codereview.chromium.org/309553011/diff/50001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler.h:279: // |allow_recommended| and |allow_mandatory| enables the handler for recommended Nit 1: s/enables/enable/ Nit 2: It would be good to document what happens when the allow_* flags are not set. How about something like: The |allow_recommended| and |allow_mandatory| flags indicate the levels at which the policy can be set. A value set at an unsupported level will be ignored. https://codereview.chromium.org/309553011/diff/50001/components/policy/core/b... File components/policy/core/browser/configuration_policy_handler_unittest.cc (right): https://codereview.chromium.org/309553011/diff/50001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler_unittest.cc:560: TEST(SimpleSchemaValidatingPolicyHandlerTest, CompleteTest) { I think the test name |CheckAndGetValue| that you based this on is more appropriate. The things you test here are checking the value and getting it. https://codereview.chromium.org/309553011/diff/50001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler_unittest.cc:562: static const char kSchemaJson[] = Nit: It looks like this constant is shared with SchemaValidatingPolicyHandlerTest.CheckAndGetValue. It should be defined at the top of the file instead of copy & pasting. https://codereview.chromium.org/309553011/diff/50001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler_unittest.cc:586: static const char kPolicyMapJson[] = Nit: It looks like this constant is shared with SchemaValidatingPolicyHandlerTest.CheckAndGetValue. It should be defined at the top of the file instead of copy & pasting. https://codereview.chromium.org/309553011/diff/50001/components/policy/resour... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/309553011/diff/50001/components/policy/resour... components/policy/resources/policy_templates.json:2694: 'supported_on': ['chrome.*:37-', 'chrome_os:37-', 'ios:37-', 'android:37-'], 1: As pointed out by Joao, please verify whether this works on the mobile platforms before listing them here. 2: This does not actually fit on one line. I had asked whether it would fit - the answer is no. It should be split into separate lines. https://codereview.chromium.org/309553011/diff/50001/components/policy/resour... components/policy/resources/policy_templates.json:2702: 'desc': '''Allows you to register a list of protocol handlers. This can only be a recommended policy. The field protocol should be set to the scheme such as 'mailto' and the field url should be set to the URL pattern of the application that handles the scheme. The pattern should include a '%s' as a placeholder for data. Nit: Put the field names in pipes to make them easier to distinguish from the surrounding prose, e.g. |protocol|. https://codereview.chromium.org/309553011/diff/50001/components/policy/resour... components/policy/resources/policy_templates.json:2704: The protocol handlers registered by policy are merged with the ones registered by user via settings and both available for use. The user can override the protocol handlers installed by policy by installing a new default handler, but is not allowed to remove a protocol handler registered by policy. ''', Nit 1: s/by user/by the user/ Nit 2: s/ '''/'''/ https://codereview.chromium.org/309553011/diff/50001/components/policy/resour... components/policy/resources/policy_templates.json:2734: 'desc': '''Allows you to ignore a list of protocol handlers, i.e., when an app attempts to install a protocol handler in the list, the attempt is silently ignored without requiring user input. This can only be a recommended policy. The field protocol should be set to the scheme such as 'mailto' and the field url should be set to the URL pattern of the application that handles the scheme. 1: The combination of preventing the user from doing something and recommended policy makes no sense to me. If this policy overrides the user's choices, it is a mandatory policy. 2: Nit: Put the field names in pipes to make them easier to distinguish from the surrounding prose, e.g. |protocol|. https://codereview.chromium.org/309553011/diff/50001/components/policy/resour... components/policy/resources/policy_templates.json:2736: The protocol handlers ignored by policy are merged with the ones ignored by user via settings. The user is not allowed to remove a protocol handler ignored by policy. ''', Nit 1: s/by user/by the user/ Nit 2: s/ '''/'''/
PTAL https://codereview.chromium.org/309553011/diff/50001/chrome/test/data/policy/... File chrome/test/data/policy/policy_test_cases.json (right): https://codereview.chromium.org/309553011/diff/50001/chrome/test/data/policy/... chrome/test/data/policy/policy_test_cases.json:1581: "test_policy": { "RegisteredProtocolHandlers": {"protocol": "test", "url": "http://test.com/%s", "default": "true"} }, On 2014/06/06 12:01:57, bartfab wrote: > Nit: s/test.com/example.com/ (http://example.com is specifically reserved for such > purposes) Done. https://codereview.chromium.org/309553011/diff/50001/chrome/test/data/policy/... chrome/test/data/policy/policy_test_cases.json:1592: "test_policy": { "IgnoredProtocolHandlers": {"protocol": "test1", "url": "http://test1.com/%s"} }, On 2014/06/06 12:01:57, bartfab wrote: > Nit 1: s/test1.com/example.com/ (http://example.com is specifically reserved for such > purposes) > Nit 2: Why not use the same protocol "test" here? The two tests will be run > independently and will not get in each other's ways. Not relevant anymore https://codereview.chromium.org/309553011/diff/50001/components/policy/core/b... File components/policy/core/browser/configuration_policy_handler.h (right): https://codereview.chromium.org/309553011/diff/50001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler.h:279: // |allow_recommended| and |allow_mandatory| enables the handler for recommended On 2014/06/06 12:01:57, bartfab wrote: > Nit 1: s/enables/enable/ > Nit 2: It would be good to document what happens when the allow_* flags are not > set. How about something like: > > The |allow_recommended| and |allow_mandatory| flags indicate the levels at which > the policy can be set. A value set at an unsupported level will be ignored. Done. https://codereview.chromium.org/309553011/diff/50001/components/policy/core/b... File components/policy/core/browser/configuration_policy_handler_unittest.cc (right): https://codereview.chromium.org/309553011/diff/50001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler_unittest.cc:560: TEST(SimpleSchemaValidatingPolicyHandlerTest, CompleteTest) { On 2014/06/06 12:01:57, bartfab wrote: > I think the test name |CheckAndGetValue| that you based this on is more > appropriate. The things you test here are checking the value and getting it. Done. https://codereview.chromium.org/309553011/diff/50001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler_unittest.cc:562: static const char kSchemaJson[] = On 2014/06/06 12:01:57, bartfab wrote: > Nit: It looks like this constant is shared with > SchemaValidatingPolicyHandlerTest.CheckAndGetValue. It should be defined at the > top of the file instead of copy & pasting. We discussed https://codereview.chromium.org/309553011/diff/50001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler_unittest.cc:586: static const char kPolicyMapJson[] = On 2014/06/06 12:01:57, bartfab wrote: > Nit: It looks like this constant is shared with > SchemaValidatingPolicyHandlerTest.CheckAndGetValue. It should be defined at the > top of the file instead of copy & pasting. We discussed https://codereview.chromium.org/309553011/diff/50001/components/policy/resour... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/309553011/diff/50001/components/policy/resour... components/policy/resources/policy_templates.json:2694: 'supported_on': ['chrome.*:37-', 'chrome_os:37-', 'ios:37-', 'android:37-'], On 2014/06/06 12:01:58, bartfab wrote: > 1: As pointed out by Joao, please verify whether this works on the mobile > platforms before listing them here. > > 2: This does not actually fit on one line. I had asked whether it would fit - > the answer is no. It should be split into separate lines. We discusses this. https://codereview.chromium.org/309553011/diff/50001/components/policy/resour... components/policy/resources/policy_templates.json:2702: 'desc': '''Allows you to register a list of protocol handlers. This can only be a recommended policy. The field protocol should be set to the scheme such as 'mailto' and the field url should be set to the URL pattern of the application that handles the scheme. The pattern should include a '%s' as a placeholder for data. On 2014/06/06 12:01:58, bartfab wrote: > Nit: Put the field names in pipes to make them easier to distinguish from the > surrounding prose, e.g. |protocol|. Done. https://codereview.chromium.org/309553011/diff/50001/components/policy/resour... components/policy/resources/policy_templates.json:2704: The protocol handlers registered by policy are merged with the ones registered by user via settings and both available for use. The user can override the protocol handlers installed by policy by installing a new default handler, but is not allowed to remove a protocol handler registered by policy. ''', On 2014/06/06 12:01:58, bartfab wrote: > Nit 1: s/by user/by the user/ > Nit 2: s/ '''/'''/ Done. https://codereview.chromium.org/309553011/diff/50001/components/policy/resour... components/policy/resources/policy_templates.json:2734: 'desc': '''Allows you to ignore a list of protocol handlers, i.e., when an app attempts to install a protocol handler in the list, the attempt is silently ignored without requiring user input. This can only be a recommended policy. The field protocol should be set to the scheme such as 'mailto' and the field url should be set to the URL pattern of the application that handles the scheme. On 2014/06/06 12:01:58, bartfab wrote: > 1: The combination of preventing the user from doing something and recommended > policy makes no sense to me. If this policy overrides the user's choices, it is > a mandatory policy. > > 2: Nit: Put the field names in pipes to make them easier to distinguish from the > surrounding prose, e.g. |protocol|. This is not valid anymore. https://codereview.chromium.org/309553011/diff/50001/components/policy/resour... components/policy/resources/policy_templates.json:2736: The protocol handlers ignored by policy are merged with the ones ignored by user via settings. The user is not allowed to remove a protocol handler ignored by policy. ''', On 2014/06/06 12:01:58, bartfab wrote: > Nit 1: s/by user/by the user/ > Nit 2: s/ '''/'''/ This is not valid anymore.
As discussed offline, please file bugs to keep track of the following features that will need to be added in follow-up CLs: * Enable dynamic refresh * Ensure the UI does not offer to remove a policy-set handler * Add controlled setting indicators to the UI * Add a browser test https://codereview.chromium.org/309553011/diff/70001/chrome/browser/policy/co... File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right): https://codereview.chromium.org/309553011/diff/70001/chrome/browser/policy/co... chrome/browser/policy/configuration_policy_handler_list_factory.cc:533: #if !defined(OS_IOS) && !defined(OS_ANDROID) Nit: There is a section guarded by "#if !defined(OS_ANDROID) && !defined(OS_IOS)" already, starting in line 584. You can add your handler there instead of adding another conditional section. https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... File components/policy/core/browser/configuration_policy_handler.h (right): https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler.h:284: SimpleSchemaValidatingPolicyHandler(const char* policy_name, This constructor is not used anywhere. https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... File components/policy/core/browser/configuration_policy_handler_unittest.cc (right): https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler_unittest.cc:627: EXPECT_TRUE(handler_all.CheckPolicySettings(policy_map_mandatory, &errors)); Nit: Verify that |errors| is empty. https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler_unittest.cc:630: EXPECT_TRUE(prefs.GetValue(kTestPref, &value_set_in_pref)); Nit: Verify that the pref value was set as mandatory. https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler_unittest.cc:634: handler_recommended.CheckPolicySettings(policy_map_mandatory, &errors)); Nit: Verify that |errors| is not empty. https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler_unittest.cc:636: EXPECT_TRUE( Nit: Verify that |errors| is empty. https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler_unittest.cc:640: EXPECT_TRUE(prefs.GetValue(kTestPref, &value_set_in_pref)); Nit: Verify that the pref value was set as mandatory. https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler_unittest.cc:643: EXPECT_FALSE(handler_none.CheckPolicySettings(policy_map_mandatory, &errors)); Nit: Verify that |errors| is not empty. https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler_unittest.cc:645: EXPECT_TRUE(handler_all.CheckPolicySettings(policy_map_recommended, &errors)); Nit: Verify that |errors| is empty. https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler_unittest.cc:648: EXPECT_TRUE(prefs.GetValue(kTestPref, &value_set_in_pref)); Nit: Verify that the pref value was set as recommended. https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler_unittest.cc:652: handler_mandatory.CheckPolicySettings(policy_map_recommended, &errors)); Nit: Verify that |errors| is not empty. https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler_unittest.cc:655: handler_recommended.CheckPolicySettings(policy_map_recommended, &errors)); Nit: Verify that |errors| is empty. https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler_unittest.cc:658: EXPECT_TRUE(prefs.GetValue(kTestPref, &value_set_in_pref)); Nit: Verify that the pref value was set as recommended. https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler_unittest.cc:662: handler_none.CheckPolicySettings(policy_map_recommended, &errors)); Nit: Verify that |errors| is not empty. https://codereview.chromium.org/309553011/diff/70001/components/policy/resour... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/309553011/diff/70001/components/policy/resour... components/policy/resources/policy_templates.json:2704: The protocol handlers registered by policy are merged with the ones registered by the user via settings and both available for use. The user can override the protocol handlers installed by policy by installing a new default handler, but is not allowed to remove a protocol handler registered by policy.''', s/both/both are/
deferring to Bartosz for the rest of the review, but LGTM from my part. https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... File components/policy/core/browser/configuration_policy_handler.h (right): https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler.h:284: SimpleSchemaValidatingPolicyHandler(const char* policy_name, On 2014/06/11 10:08:44, bartfab wrote: > This constructor is not used anywhere. If you are going to leave this constructor in place, document what the defaults for allow_recommended/mandatory will be. https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler.h:292: bool allow_recommended, nit: I don't like passing raw bools around to constructors and other APIs, because you end up with a constructor that looks like Object(x, y, z, true, false) with no understanding of what true/false means in that context. Consider using something like: enum MandatoryAllowed { MANDATORY_ALLOWED, MANDATORY_PROHIBITED } Then your constructor looks like: Object(a, b, c, REQUIRED_ALLOWED, MANDATORY_PROHIBITED) which is much more readable.
PTAL https://codereview.chromium.org/309553011/diff/70001/chrome/browser/policy/co... File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right): https://codereview.chromium.org/309553011/diff/70001/chrome/browser/policy/co... chrome/browser/policy/configuration_policy_handler_list_factory.cc:533: #if !defined(OS_IOS) && !defined(OS_ANDROID) On 2014/06/11 10:08:44, bartfab wrote: > Nit: There is a section guarded by "#if !defined(OS_ANDROID) && > !defined(OS_IOS)" already, starting in line 584. You can add your handler there > instead of adding another conditional section. Done. https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... File components/policy/core/browser/configuration_policy_handler.h (right): https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler.h:284: SimpleSchemaValidatingPolicyHandler(const char* policy_name, On 2014/06/11 12:32:55, Andrew T Wilson wrote: > On 2014/06/11 10:08:44, bartfab wrote: > > This constructor is not used anywhere. > > If you are going to leave this constructor in place, document what the defaults > for allow_recommended/mandatory will be. I have removed it. https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler.h:284: SimpleSchemaValidatingPolicyHandler(const char* policy_name, On 2014/06/11 10:08:44, bartfab wrote: > This constructor is not used anywhere. I have removed it. https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler.h:292: bool allow_recommended, On 2014/06/11 12:32:55, Andrew T Wilson wrote: > nit: I don't like passing raw bools around to constructors and other APIs, > because you end up with a constructor that looks like Object(x, y, z, true, > false) with no understanding of what true/false means in that context. > > Consider using something like: > > enum MandatoryAllowed { > MANDATORY_ALLOWED, > MANDATORY_PROHIBITED > } > > Then your constructor looks like: > > Object(a, b, c, REQUIRED_ALLOWED, MANDATORY_PROHIBITED) > > which is much more readable. Done. https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... File components/policy/core/browser/configuration_policy_handler_unittest.cc (right): https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler_unittest.cc:627: EXPECT_TRUE(handler_all.CheckPolicySettings(policy_map_mandatory, &errors)); On 2014/06/11 10:08:44, bartfab wrote: > Nit: Verify that |errors| is empty. Done. https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler_unittest.cc:630: EXPECT_TRUE(prefs.GetValue(kTestPref, &value_set_in_pref)); On 2014/06/11 10:08:44, bartfab wrote: > Nit: Verify that the pref value was set as mandatory. We discussed this. https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler_unittest.cc:634: handler_recommended.CheckPolicySettings(policy_map_mandatory, &errors)); On 2014/06/11 10:08:44, bartfab wrote: > Nit: Verify that |errors| is not empty. Done. https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler_unittest.cc:636: EXPECT_TRUE( On 2014/06/11 10:08:44, bartfab wrote: > Nit: Verify that |errors| is empty. Done. https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler_unittest.cc:640: EXPECT_TRUE(prefs.GetValue(kTestPref, &value_set_in_pref)); On 2014/06/11 10:08:44, bartfab wrote: > Nit: Verify that the pref value was set as mandatory. We discussed this https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler_unittest.cc:643: EXPECT_FALSE(handler_none.CheckPolicySettings(policy_map_mandatory, &errors)); On 2014/06/11 10:08:44, bartfab wrote: > Nit: Verify that |errors| is not empty. Done. https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler_unittest.cc:645: EXPECT_TRUE(handler_all.CheckPolicySettings(policy_map_recommended, &errors)); On 2014/06/11 10:08:44, bartfab wrote: > Nit: Verify that |errors| is empty. Done. https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler_unittest.cc:648: EXPECT_TRUE(prefs.GetValue(kTestPref, &value_set_in_pref)); On 2014/06/11 10:08:44, bartfab wrote: > Nit: Verify that the pref value was set as recommended. We discussed this https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler_unittest.cc:652: handler_mandatory.CheckPolicySettings(policy_map_recommended, &errors)); On 2014/06/11 10:08:44, bartfab wrote: > Nit: Verify that |errors| is not empty. Done. https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler_unittest.cc:655: handler_recommended.CheckPolicySettings(policy_map_recommended, &errors)); On 2014/06/11 10:08:44, bartfab wrote: > Nit: Verify that |errors| is empty. Done. https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler_unittest.cc:658: EXPECT_TRUE(prefs.GetValue(kTestPref, &value_set_in_pref)); On 2014/06/11 10:08:44, bartfab wrote: > Nit: Verify that the pref value was set as recommended. We discussed this https://codereview.chromium.org/309553011/diff/70001/components/policy/core/b... components/policy/core/browser/configuration_policy_handler_unittest.cc:662: handler_none.CheckPolicySettings(policy_map_recommended, &errors)); On 2014/06/11 10:08:44, bartfab wrote: > Nit: Verify that |errors| is not empty. Done. https://codereview.chromium.org/309553011/diff/70001/components/policy/resour... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/309553011/diff/70001/components/policy/resour... components/policy/resources/policy_templates.json:2704: The protocol handlers registered by policy are merged with the ones registered by the user via settings and both available for use. The user can override the protocol handlers installed by policy by installing a new default handler, but is not allowed to remove a protocol handler registered by policy.''', On 2014/06/11 10:08:44, bartfab wrote: > s/both/both are/ Done.
lgtm https://codereview.chromium.org/309553011/diff/120001/components/policy/core/... File components/policy/core/browser/configuration_policy_handler.h (right): https://codereview.chromium.org/309553011/diff/120001/components/policy/core/... components/policy/core/browser/configuration_policy_handler.h:279: // |allow_recommended| and |allow_mandatory| flags indicate the levels at which Nit 1: s/allow_recommended/recommended_permission/ Nit 2: s/allow_mandatory/mandatory_permission/
Hi Daniel, Can you give me a owner LGTM?
policy_templates.json LGTM after making corrections. https://codereview.chromium.org/309553011/diff/140001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/309553011/diff/140001/components/policy/resou... components/policy/resources/policy_templates.json:2699: 'example_value': [{'protocol': 'mailto', 'url': 'https://mail.google.com/mail/?extsrc=mailto&url=%s'}], Add another object to the array with the 'default' property set. https://codereview.chromium.org/309553011/diff/140001/components/policy/resou... components/policy/resources/policy_templates.json:2702: 'desc': '''Allows you to register a list of protocol handlers. This can only be a recommended policy. The field |protocol| should be set to the scheme such as 'mailto' and the field |url| should be set to the URL pattern of the application that handles the scheme. The pattern should include a '%s' as a placeholder for data. s/field/property/g https://codereview.chromium.org/309553011/diff/140001/components/policy/resou... components/policy/resources/policy_templates.json:2702: 'desc': '''Allows you to register a list of protocol handlers. This can only be a recommended policy. The field |protocol| should be set to the scheme such as 'mailto' and the field |url| should be set to the URL pattern of the application that handles the scheme. The pattern should include a '%s' as a placeholder for data. Looks like the "%s" isn't actually required by the implementation, so replace "should" with "can." Make this something like the following: "The pattern can include a '%s', which if present will be replaced by the handled URL." https://codereview.chromium.org/309553011/diff/140001/components/policy/resou... components/policy/resources/policy_templates.json:2704: The protocol handlers registered by policy are merged with the ones registered by the user via settings and both are available for use. The user can override the protocol handlers installed by policy by installing a new default handler, but is not allowed to remove a protocol handler registered by policy.''', Protocol handlers can come from elsewhere, not just users via settings. Web pages in the wild can also register them. https://codereview.chromium.org/309553011/diff/140001/components/policy/resou... components/policy/resources/policy_templates.json:2704: The protocol handlers registered by policy are merged with the ones registered by the user via settings and both are available for use. The user can override the protocol handlers installed by policy by installing a new default handler, but is not allowed to remove a protocol handler registered by policy.''', Remove allowed: "... a new default handler, but cannot remove a..."
The CQ bit was checked by kaliamoorthi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kaliamoorthi@chromium.org/309553011/16...
histograms lgtm
The CQ bit was checked by kaliamoorthi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kaliamoorthi@chromium.org/309553011/18...
Message was sent while issue was closed.
Change committed as 277475 |
