Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2529083002:
Make extensions developer mode adhere to policy (Closed)
|
Created:
4 years ago by pmarko Modified:
4 years ago Reviewers:
bartfab (slow), Bernhard Bauer, Devlin, Gaurav, Andrew T Wilson (Slow), Thiemo Nagel, mkearney1 CC:
chromium-reviews, extensions-reviews_chromium.org, arv+watch_chromium.org, jlklein+watch-closure_chromium.org, vitalyp+closure_chromium.org, chromium-apps-reviews_chromium.org, dbeam+watch-closure_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake extensions developer mode adhere to policy
Make the developer mode checkbox on chrome://extensions adhere to the DeveloperToolsDisabled policy. If it is disabled, display an indicator.
Add corresponding indicator test to policy_prefs_browsertest.
Add an end-to-end test to policy_browsertest.
BUG=633684
TEST=browser_tests *PolicyPrefIndicatorTest*, PolicyTest.DeveloperToolsDisabledExtensionsDevMode
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/6e36b463913efcea94e14a2de86caff388351290
Cr-Commit-Position: refs/heads/master@{#438215}
Patch Set 1 #Patch Set 2 : Fixed typo in comment #Patch Set 3 : Get rid of busy loop in test #
Total comments: 12
Patch Set 4 : Use ConfigurationPolicyPrefStore for policy->pref mapping #Patch Set 5 : Guard against Preference not being found #
Total comments: 26
Patch Set 6 : Improve browsertest, names and format #
Total comments: 1
Patch Set 7 : Asterisk goes next to the type #
Total comments: 6
Patch Set 8 : Add api unit tests, code style #Patch Set 9 : Fix and improve api unittest #
Total comments: 6
Patch Set 10 : Reviewers' comments #
Total comments: 14
Patch Set 11 : ASSERT_ instead of EXPECT_ in tests, comments #
Total comments: 4
Patch Set 12 : Improve comments, git cl format #Patch Set 13 : Remove unnecessary .get() #Patch Set 14 : Rebase #Messages
Total messages: 56 (35 generated)
Description was changed from ========== Make extensions developer mode adhere to policy Make the developer mode checkbox on chrome://extensions adhere to the DeveloperToolsDisabled policy. If it is disabled, display an indicator. Add corresponding indicator test to policy_prefs_browsertest. Add an end-to-end test to policy_browsertest. BUG=633684 TEST=browser_tests *PolicyPrefIndicatorTest*, PolicyTest.DeveloperToolsDisabledExtensionsDevMode ========== to ========== Make extensions developer mode adhere to policy Make the developer mode checkbox on chrome://extensions adhere to the DeveloperToolsDisabled policy. If it is disabled, display an indicator. Add corresponding indicator test to policy_prefs_browsertest. Add an end-to-end test to policy_browsertest. BUG=633684 TEST=browser_tests *PolicyPrefIndicatorTest*, PolicyTest.DeveloperToolsDisabledExtensionsDevMode CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by pmarko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by pmarko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
mkearney@chromium.org: Please review changes in developer_private.idl tnagel@chromium.org: Please review changes in policy_browsertest.cc / policy_prefs_browsertest.cc / policy_test_cases.json atwilson@chromium.org: Please review changes in policy_browsertest.cc / policy_prefs_browsertest.cc / policy_test_cases.json bartfab@chromium.org: Please review changes in policy_browsertest.cc / policy_prefs_browsertest.cc / policy_test_cases.json rdevlin.cronin@chromium.org: Please review changes in extensions.html / extensions.js / extension_settings_handler.cc / developer_private.js grv@chromium.org: Please review changes in developer_private_api.h bauerb@chromium.org: Please review changes in extensions.html / extensions.js / extension_settings_handler.cc / developer_private.js
https://codereview.chromium.org/2529083002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/2529083002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/developer_private_api.cc:389: prefs->SetBoolean(prefs::kExtensionsUIDeveloperMode, false); Could you set this value via the ConfigurationPolicyPrefStore? Usually we try to avoid modifying user preferences for things that aren't actually a user choice. https://codereview.chromium.org/2529083002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/developer_private_api.cc:625: return RespondNow(Error( This is a multi-line statement, so the block needs braces. https://codereview.chromium.org/2529083002/diff/40001/chrome/browser/policy/p... File chrome/browser/policy/policy_prefs_browsertest.cc (right): https://codereview.chromium.org/2529083002/diff/40001/chrome/browser/policy/p... chrome/browser/policy/policy_prefs_browsertest.cc:783: if (!(*pref_mapping)->indicator_test_url().empty()) { For consistency with the surrounding code, leave out the braces for single-line bodies. https://codereview.chromium.org/2529083002/diff/40001/chrome/browser/resource... File chrome/browser/resources/extensions/extensions.html (right): https://codereview.chromium.org/2529083002/diff/40001/chrome/browser/resource... chrome/browser/resources/extensions/extensions.html:80: <span id="dev-toggle-disabled-by-policy-indicator" class="controlled-setting-indicator"></span> Please break this to fit into 80 columns. https://codereview.chromium.org/2529083002/diff/40001/chrome/browser/resource... File chrome/browser/resources/extensions/extensions.js (right): https://codereview.chromium.org/2529083002/diff/40001/chrome/browser/resource... chrome/browser/resources/extensions/extensions.js:199: var developerModeDisabledByPolicy = profileInfo. Ugh. Can you break after the equals sign? https://codereview.chromium.org/2529083002/diff/40001/chrome/browser/resource... chrome/browser/resources/extensions/extensions.js:250: else If the if-clause uses braces, so should the else.
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by pmarko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/25 15:43:35, Bernhard Bauer wrote: > https://codereview.chromium.org/2529083002/diff/40001/chrome/browser/extensio... > File chrome/browser/extensions/api/developer_private/developer_private_api.cc > (right): > > https://codereview.chromium.org/2529083002/diff/40001/chrome/browser/extensio... > chrome/browser/extensions/api/developer_private/developer_private_api.cc:389: > prefs->SetBoolean(prefs::kExtensionsUIDeveloperMode, false); > Could you set this value via the ConfigurationPolicyPrefStore? Usually we try to > avoid modifying user preferences for things that aren't actually a user choice. > > https://codereview.chromium.org/2529083002/diff/40001/chrome/browser/extensio... > chrome/browser/extensions/api/developer_private/developer_private_api.cc:625: > return RespondNow(Error( > This is a multi-line statement, so the block needs braces. > > https://codereview.chromium.org/2529083002/diff/40001/chrome/browser/policy/p... > File chrome/browser/policy/policy_prefs_browsertest.cc (right): > > https://codereview.chromium.org/2529083002/diff/40001/chrome/browser/policy/p... > chrome/browser/policy/policy_prefs_browsertest.cc:783: if > (!(*pref_mapping)->indicator_test_url().empty()) { > For consistency with the surrounding code, leave out the braces for single-line > bodies. > > https://codereview.chromium.org/2529083002/diff/40001/chrome/browser/resource... > File chrome/browser/resources/extensions/extensions.html (right): > > https://codereview.chromium.org/2529083002/diff/40001/chrome/browser/resource... > chrome/browser/resources/extensions/extensions.html:80: <span > id="dev-toggle-disabled-by-policy-indicator" > class="controlled-setting-indicator"></span> > Please break this to fit into 80 columns. > > https://codereview.chromium.org/2529083002/diff/40001/chrome/browser/resource... > File chrome/browser/resources/extensions/extensions.js (right): > > https://codereview.chromium.org/2529083002/diff/40001/chrome/browser/resource... > chrome/browser/resources/extensions/extensions.js:199: var > developerModeDisabledByPolicy = profileInfo. > Ugh. Can you break after the equals sign? > > https://codereview.chromium.org/2529083002/diff/40001/chrome/browser/resource... > chrome/browser/resources/extensions/extensions.js:250: else > If the if-clause uses braces, so should the else. Thanks for your review! I have corrected the points you mentioned and have also fixed the browsertest flakiness introduced in patchset 3. Please take another look.
PTAL https://codereview.chromium.org/2529083002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/2529083002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/developer_private_api.cc:389: prefs->SetBoolean(prefs::kExtensionsUIDeveloperMode, false); On 2016/11/25 15:43:35, Bernhard Bauer wrote: > Could you set this value via the ConfigurationPolicyPrefStore? Usually we try to > avoid modifying user preferences for things that aren't actually a user choice. Thank you for pointing this out -- done. Your suggestion is much more elegant and actually has the correct behavior when the policy is removed again. https://codereview.chromium.org/2529083002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/developer_private_api.cc:625: return RespondNow(Error( On 2016/11/25 15:43:35, Bernhard Bauer wrote: > This is a multi-line statement, so the block needs braces. Done. https://codereview.chromium.org/2529083002/diff/40001/chrome/browser/policy/p... File chrome/browser/policy/policy_prefs_browsertest.cc (right): https://codereview.chromium.org/2529083002/diff/40001/chrome/browser/policy/p... chrome/browser/policy/policy_prefs_browsertest.cc:783: if (!(*pref_mapping)->indicator_test_url().empty()) { On 2016/11/25 15:43:35, Bernhard Bauer wrote: > For consistency with the surrounding code, leave out the braces for single-line > bodies. Done. https://codereview.chromium.org/2529083002/diff/40001/chrome/browser/resource... File chrome/browser/resources/extensions/extensions.html (right): https://codereview.chromium.org/2529083002/diff/40001/chrome/browser/resource... chrome/browser/resources/extensions/extensions.html:80: <span id="dev-toggle-disabled-by-policy-indicator" class="controlled-setting-indicator"></span> On 2016/11/25 15:43:35, Bernhard Bauer wrote: > Please break this to fit into 80 columns. Done. https://codereview.chromium.org/2529083002/diff/40001/chrome/browser/resource... File chrome/browser/resources/extensions/extensions.js (right): https://codereview.chromium.org/2529083002/diff/40001/chrome/browser/resource... chrome/browser/resources/extensions/extensions.js:199: var developerModeDisabledByPolicy = profileInfo. On 2016/11/25 15:43:35, Bernhard Bauer wrote: > Ugh. Can you break after the equals sign? Done. https://codereview.chromium.org/2529083002/diff/40001/chrome/browser/resource... chrome/browser/resources/extensions/extensions.js:250: else On 2016/11/25 15:43:35, Bernhard Bauer wrote: > If the if-clause uses braces, so should the else. Done.
https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/developer_private_api.cc:172: info->is_developer_mode_disabled_by_policy = pref && pref->IsManaged(); When would the preference not be found? Usually we treat the set of registered preferences as fixed at compile time, so a missing preference shouldn't (need to) be handled. If there is a test that crashes here, you might have to register preferences on the profile first? https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/policy/c... File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right): https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/policy/c... chrome/browser/policy/configuration_policy_handler_list_factory.cc:159: { key::kDeveloperToolsDisabled, Could you just duplicate this entry with prefs::kExtensionsUIDeveloperMode?
https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/developer_private_api.cc:604: if (GetProfile()->IsSupervised()) We should add a check here. Otherwise, the only guards we have against this are in JS, and that's very weak if anyone opens the developer console (e.g. the user could just type chrome.developerPrivate.updateProfileConfiguration({in_developer_mode: true}) and never have to hit the checkbox). I realize that, right now, these prefs are actually piggy backing, so theoretically the user might not be able to open the console, but that's a pretty weak guarantee, and very subject to change. Enforcing this at the C++ level is much less likely to be circumvented. :) https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/policy/p... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/policy/p... chrome/browser/policy/policy_browsertest.cc:1349: IN_PROC_BROWSER_TEST_F(PolicyTest, DeveloperToolsDisabledExtensionsDevMode) { This is a pretty fragile browser test for a number of reasons - mainly that a) it's very breakable with any changes in the extensions page and b) it only tests that clicking with a mouse doesn't work (see also comment in developer_private_api.cc). I'd prefer to remove this and instead use a new api unittest test for the developerPrivate.updateProfileConfiguration method. https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/resource... File chrome/browser/resources/extensions/extensions.js (right): https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/resource... chrome/browser/resources/extensions/extensions.js:212: // but extension developer tools were visible nitty-nit: comments should end with punctuation (i.e., add a '.'). https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/resource... chrome/browser/resources/extensions/extensions.js:213: this.updateDevControlsVisibility_(true); nit: We probably don't need to animate here - usually that's only done in response to some sort of user action. https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/resource... chrome/browser/resources/extensions/extensions.js:245: devToggleControlledIndicator.setAttribute('controlled-by', nit: prefer wrapping so that either all arguments are on the same line: devToggleControlledIndicator.setAttribute( 'controlled-by', controlledBy); or so that they align: devToggleControlledIndicator.setAttribute('controlled-by', controlledBy); (Goes for all places) https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/resource... chrome/browser/resources/extensions/extensions.js:250: else { else on same line as closing } https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/resource... chrome/browser/resources/extensions/extensions.js:251: devToggleControlledIndicator.removeAttribute('controlled-by'); does removing the attribute fully hide the element? It seems like it would be easier to either use .hidden or to just create/remove the element fully (as we do in extension_list.js). https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right): https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_settings_handler.cc:281: source->AddString("controlledSettingPolicy", prefix this with 'extensions' like the others. These strings are used between all options pages, so we don't want to accidentally pollute one of the other pages.
Thanks for your comments! I have corrected the places where I was sure, and added two questions. In particular, I'd be interested in your opinion - about the browsertest DeveloperToolsDisabledExtensionsDevMode in policy_browsertest.cc after my recent changes and comments - if the idea that we don't need an explicit check in developer_private_api.cc:604 because ConfigurationPolicyPrefStore 'overrides' user prefs is fine. Thank you! https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/developer_private_api.cc:172: info->is_developer_mode_disabled_by_policy = pref && pref->IsManaged(); On 2016/11/28 17:11:03, Bernhard Bauer wrote: > When would the preference not be found? Usually we treat the set of registered > preferences as fixed at compile time, so a missing preference shouldn't (need > to) be handled. If there is a test that crashes here, you might have to register > preferences on the profile first? Good point, tests were working fine, this was just me being overly cautious because I'm new :) Done. https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/developer_private_api.cc:604: if (GetProfile()->IsSupervised()) On 2016/11/28 17:34:01, Devlin wrote: > We should add a check here. Otherwise, the only guards we have against this are > in JS, and that's very weak if anyone opens the developer console (e.g. the user > could just type > chrome.developerPrivate.updateProfileConfiguration({in_developer_mode: true}) > and never have to hit the checkbox). > > I realize that, right now, these prefs are actually piggy backing, so > theoretically the user might not be able to open the console, but that's a > pretty weak guarantee, and very subject to change. Enforcing this at the C++ > level is much less likely to be circumvented. :) I'm not sure it's necessary -- I had a check here back in patchset 3 when the plan was to set the pref to false explicitly. Now that it is set through ConfigurationPolicyPrefStore (see configuration_policy_handler_list_factory.cc), the policy-controlled pref value overrides any value set here, so I removed the check. If the user passes that JS, he will only affect the value the pref has after the policy will be removed. So I think it should not be possible to trigger it that way when the policy is in place. Or am I missing something? https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/policy/c... File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right): https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/policy/c... chrome/browser/policy/configuration_policy_handler_list_factory.cc:159: { key::kDeveloperToolsDisabled, On 2016/11/28 17:11:03, Bernhard Bauer wrote: > Could you just duplicate this entry with prefs::kExtensionsUIDeveloperMode? Actually, I don't think I could - the issue is that this has to be negated: If kDeveloperToolDisabled is true, kExtensionsUIDeveloperMode shall be forced to false. That's why we need a special handler. https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/policy/p... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/policy/p... chrome/browser/policy/policy_browsertest.cc:1349: IN_PROC_BROWSER_TEST_F(PolicyTest, DeveloperToolsDisabledExtensionsDevMode) { On 2016/11/28 17:34:01, Devlin wrote: > This is a pretty fragile browser test for a number of reasons - mainly that a) > it's very breakable with any changes in the extensions page and b) it only tests > that clicking with a mouse doesn't work (see also comment in > developer_private_api.cc). I'd prefer to remove this and instead use a new api > unittest test for the developerPrivate.updateProfileConfiguration method. unittest: good idea, I'll check if I can add one. browsertest: I should have described that better - the main purpose of the test was the sequence: - user enables devmode -> the dev controls appear - policy comes in -> (1) dev controls automatically disappear and (2) user can not re-enable it. You are right in that it is only testing clicking on the toggle control -- but it clicks when the checkbox is still enabled. After the policy comes in, I've changed it to only check that the toggle control is disabled. As for the fragility: The test depends on the following assumptions about the extensions UI: - that the control for toggling dev mode has the ID 'toggle-dev-on' - that the dev-mode-buttons are in a DOM element with the ID 'dev-controls' - that the implementor chose to display/hide the 'dev-controls' using the 'height' attribute. On the other hand, it seemed to me like a good trade-off for an end-to-end test of the feature. This way we can have a regression test seeing if the controls are being actively hidden when the policy comes in. I'd suggest the following: - I will extract the IDs/assumptions into constants and comment what they are - I will clearly document the why it's checking for height Would you think that this makes the test better or would you still favor not having such a test at all? https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/resource... File chrome/browser/resources/extensions/extensions.js (right): https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/resource... chrome/browser/resources/extensions/extensions.js:212: // but extension developer tools were visible On 2016/11/28 17:34:01, Devlin wrote: > nitty-nit: comments should end with punctuation (i.e., add a '.'). Done. https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/resource... chrome/browser/resources/extensions/extensions.js:213: this.updateDevControlsVisibility_(true); On 2016/11/28 17:34:01, Devlin wrote: > nit: We probably don't need to animate here - usually that's only done in > response to some sort of user action. Good point - passing false. Done. https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/resource... chrome/browser/resources/extensions/extensions.js:245: devToggleControlledIndicator.setAttribute('controlled-by', On 2016/11/28 17:34:01, Devlin wrote: > nit: prefer wrapping so that either all arguments are on the same line: > devToggleControlledIndicator.setAttribute( > 'controlled-by', controlledBy); > or so that they align: > devToggleControlledIndicator.setAttribute('controlled-by', > controlledBy); > > (Goes for all places) Done. https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/resource... chrome/browser/resources/extensions/extensions.js:250: else { On 2016/11/28 17:34:01, Devlin wrote: > else on same line as closing } Done. https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/resource... chrome/browser/resources/extensions/extensions.js:251: devToggleControlledIndicator.removeAttribute('controlled-by'); On 2016/11/28 17:34:01, Devlin wrote: > does removing the attribute fully hide the element? It seems like it would be > easier to either use .hidden or to just create/remove the element fully (as we > do in extension_list.js). Removing it actually fully hides it due to controlled-indicator.css which states: .controlled-setting-indicator:not([controlled-by]) { display: none; } As far as I understand it, this is also the mechanism controlled_setting.js uses for showing the indicator (including deciding which icon to display)/hiding it (in detail, a listener on the Preference's controlledBy attrbiute transfers its value to the controlledBy of the indicator object). That mechanism is used on the main (non-extensions) settings page, that's why I did it that way. Do you think it would be better - if we constantly set controlled-by=policy and did visibility control using .hidden - if we removed/recreated the element on the fily - if we did it using the controlled-by attribute and added a comment that removing the attribute fully hides the element? https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right): https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/extensions/extension_settings_handler.cc:281: source->AddString("controlledSettingPolicy", On 2016/11/28 17:34:01, Devlin wrote: > prefix this with 'extensions' like the others. These strings are used between > all options pages, so we don't want to accidentally pollute one of the other > pages. Done.
The CQ bit was checked by pmarko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM when Devlin is happy. https://codereview.chromium.org/2529083002/diff/100001/chrome/browser/policy/... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/2529083002/diff/100001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:1326: const char *dev_controls_accessor_js, Asterisk goes next to the type.
Looks pretty good, but I'd feel better if we could add a unittest for the developerPrivate API. :) https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/developer_private_api.cc:604: if (GetProfile()->IsSupervised()) On 2016/11/29 12:40:21, pmarko wrote: > On 2016/11/28 17:34:01, Devlin wrote: > > We should add a check here. Otherwise, the only guards we have against this > are > > in JS, and that's very weak if anyone opens the developer console (e.g. the > user > > could just type > > chrome.developerPrivate.updateProfileConfiguration({in_developer_mode: true}) > > and never have to hit the checkbox). > > > > I realize that, right now, these prefs are actually piggy backing, so > > theoretically the user might not be able to open the console, but that's a > > pretty weak guarantee, and very subject to change. Enforcing this at the C++ > > level is much less likely to be circumvented. :) > > I'm not sure it's necessary -- I had a check here back in patchset 3 when the > plan was to set the pref to false explicitly. > > Now that it is set through ConfigurationPolicyPrefStore (see > configuration_policy_handler_list_factory.cc), the policy-controlled pref value > overrides any value set here, so I removed the check. > If the user passes that JS, he will only affect the value the pref has after the > policy will be removed. > So I think it should not be possible to trigger it that way when the policy is > in place. Or am I missing something? I'll trust you and bauerb - my pref knowledge is limited. :) In either case, seems like a developerPrivate unittest would still be useful? Making sure we behave as expected when passed unexpected values? https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/policy/p... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/policy/p... chrome/browser/policy/policy_browsertest.cc:1349: IN_PROC_BROWSER_TEST_F(PolicyTest, DeveloperToolsDisabledExtensionsDevMode) { On 2016/11/29 12:40:22, pmarko wrote: > On 2016/11/28 17:34:01, Devlin wrote: > > This is a pretty fragile browser test for a number of reasons - mainly that a) > > it's very breakable with any changes in the extensions page and b) it only > tests > > that clicking with a mouse doesn't work (see also comment in > > developer_private_api.cc). I'd prefer to remove this and instead use a new > api > > unittest test for the developerPrivate.updateProfileConfiguration method. > unittest: good idea, I'll check if I can add one. > > browsertest: > I should have described that better - the main purpose of the test was the > sequence: > - user enables devmode -> the dev controls appear > - policy comes in -> (1) dev controls automatically disappear and > (2) user can not re-enable it. > You are right in that it is only testing clicking on the toggle control -- but > it clicks when the checkbox is still enabled. After the policy comes in, I've > changed it to only check that the toggle control is disabled. > > As for the fragility: The test depends on the following assumptions about the > extensions UI: > - that the control for toggling dev mode has the ID 'toggle-dev-on' > - that the dev-mode-buttons are in a DOM element with the ID 'dev-controls' > - that the implementor chose to display/hide the 'dev-controls' using the > 'height' attribute. > On the other hand, it seemed to me like a good trade-off for an end-to-end test > of the feature. This way we can have a regression test seeing if the controls > are being actively hidden when the policy comes in. > > I'd suggest the following: > - I will extract the IDs/assumptions into constants and comment what they are > - I will clearly document the why it's checking for height > > Would you think that this makes the test better or would you still favor not > having such a test at all? Ah, okay, that makes more sense. If this is deliberately just checking the UI, I think it's fine. https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/resource... File chrome/browser/resources/extensions/extensions.js (right): https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/resource... chrome/browser/resources/extensions/extensions.js:251: devToggleControlledIndicator.removeAttribute('controlled-by'); On 2016/11/29 12:40:22, pmarko wrote: > On 2016/11/28 17:34:01, Devlin wrote: > > does removing the attribute fully hide the element? It seems like it would be > > easier to either use .hidden or to just create/remove the element fully (as we > > do in extension_list.js). > > Removing it actually fully hides it due to controlled-indicator.css which > states: > .controlled-setting-indicator:not([controlled-by]) { > display: none; > } > As far as I understand it, this is also the mechanism controlled_setting.js uses > for showing the indicator (including deciding which icon to display)/hiding it > (in detail, a listener on the Preference's controlledBy attrbiute transfers its > value to the controlledBy of the indicator object). That mechanism is used on > the main (non-extensions) settings page, that's why I did it that way. > > Do you think it would be better > - if we constantly set controlled-by=policy and did visibility control using > .hidden > - if we removed/recreated the element on the fily > - if we did it using the controlled-by attribute and added a comment that > removing the attribute fully hides the element? I like option 3 - adding a comment that this hides the element from the DOM. That should be sufficient. https://codereview.chromium.org/2529083002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/extensions/extensions.js (right): https://codereview.chromium.org/2529083002/diff/120001/chrome/browser/resourc... chrome/browser/resources/extensions/extensions.js:236: setDevToggleControlledIndicator_: function(devModeControlledByPolicy) { nitty nit: maybe updateDevModeControlledIndicator? https://codereview.chromium.org/2529083002/diff/120001/chrome/browser/resourc... chrome/browser/resources/extensions/extensions.js:237: var devToggleControlledIndicator = document.querySelector( nitty nit: can we just name this something like 'controlIndicator' for brevity? https://codereview.chromium.org/2529083002/diff/120001/chrome/browser/resourc... chrome/browser/resources/extensions/extensions.js:243: if (devModeControlledByPolicy) { nit: can we also check the current status here? e.g. var isActive = devToggleControlledIndicator.getAttribute('controlled-by'); if (devModeControlledByPolicy && !isActive) { ... } else if (!devModeControlledByPolicy && isActive) { ... }
The CQ bit was checked by pmarko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL I have added two api unit tests. @Devlin: to also have policy in the api unit test I had to change extension_service_test_base, could you please take a look if that's OK https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/developer_private_api.cc:604: if (GetProfile()->IsSupervised()) On 2016/11/30 19:19:50, Devlin wrote: > On 2016/11/29 12:40:21, pmarko wrote: > > On 2016/11/28 17:34:01, Devlin wrote: > > > We should add a check here. Otherwise, the only guards we have against this > > are > > > in JS, and that's very weak if anyone opens the developer console (e.g. the > > user > > > could just type > > > chrome.developerPrivate.updateProfileConfiguration({in_developer_mode: > true}) > > > and never have to hit the checkbox). > > > > > > I realize that, right now, these prefs are actually piggy backing, so > > > theoretically the user might not be able to open the console, but that's a > > > pretty weak guarantee, and very subject to change. Enforcing this at the > C++ > > > level is much less likely to be circumvented. :) > > > > I'm not sure it's necessary -- I had a check here back in patchset 3 when the > > plan was to set the pref to false explicitly. > > > > Now that it is set through ConfigurationPolicyPrefStore (see > > configuration_policy_handler_list_factory.cc), the policy-controlled pref > value > > overrides any value set here, so I removed the check. > > If the user passes that JS, he will only affect the value the pref has after > the > > policy will be removed. > > So I think it should not be possible to trigger it that way when the policy is > > in place. Or am I missing something? > > I'll trust you and bauerb - my pref knowledge is limited. :) In either case, > seems like a developerPrivate unittest would still be useful? Making sure we > behave as expected when passed unexpected values? Sorry, I almost forgot -- added. https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/policy/p... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/policy/p... chrome/browser/policy/policy_browsertest.cc:1349: IN_PROC_BROWSER_TEST_F(PolicyTest, DeveloperToolsDisabledExtensionsDevMode) { On 2016/11/30 19:19:50, Devlin wrote: > On 2016/11/29 12:40:22, pmarko wrote: > > On 2016/11/28 17:34:01, Devlin wrote: > > > This is a pretty fragile browser test for a number of reasons - mainly that > a) > > > it's very breakable with any changes in the extensions page and b) it only > > tests > > > that clicking with a mouse doesn't work (see also comment in > > > developer_private_api.cc). I'd prefer to remove this and instead use a new > > api > > > unittest test for the developerPrivate.updateProfileConfiguration method. > > unittest: good idea, I'll check if I can add one. > > > > browsertest: > > I should have described that better - the main purpose of the test was the > > sequence: > > - user enables devmode -> the dev controls appear > > - policy comes in -> (1) dev controls automatically disappear and > > (2) user can not re-enable it. > > You are right in that it is only testing clicking on the toggle control -- but > > it clicks when the checkbox is still enabled. After the policy comes in, I've > > changed it to only check that the toggle control is disabled. > > > > As for the fragility: The test depends on the following assumptions about the > > extensions UI: > > - that the control for toggling dev mode has the ID 'toggle-dev-on' > > - that the dev-mode-buttons are in a DOM element with the ID 'dev-controls' > > - that the implementor chose to display/hide the 'dev-controls' using the > > 'height' attribute. > > On the other hand, it seemed to me like a good trade-off for an end-to-end > test > > of the feature. This way we can have a regression test seeing if the controls > > are being actively hidden when the policy comes in. > > > > I'd suggest the following: > > - I will extract the IDs/assumptions into constants and comment what they are > > - I will clearly document the why it's checking for height > > > > Would you think that this makes the test better or would you still favor not > > having such a test at all? > > Ah, okay, that makes more sense. If this is deliberately just checking the UI, > I think it's fine. Acknowledged. https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/resource... File chrome/browser/resources/extensions/extensions.js (right): https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/resource... chrome/browser/resources/extensions/extensions.js:251: devToggleControlledIndicator.removeAttribute('controlled-by'); On 2016/11/30 19:19:50, Devlin wrote: > On 2016/11/29 12:40:22, pmarko wrote: > > On 2016/11/28 17:34:01, Devlin wrote: > > > does removing the attribute fully hide the element? It seems like it would > be > > > easier to either use .hidden or to just create/remove the element fully (as > we > > > do in extension_list.js). > > > > Removing it actually fully hides it due to controlled-indicator.css which > > states: > > .controlled-setting-indicator:not([controlled-by]) { > > display: none; > > } > > As far as I understand it, this is also the mechanism controlled_setting.js > uses > > for showing the indicator (including deciding which icon to display)/hiding it > > (in detail, a listener on the Preference's controlledBy attrbiute transfers > its > > value to the controlledBy of the indicator object). That mechanism is used on > > the main (non-extensions) settings page, that's why I did it that way. > > > > Do you think it would be better > > - if we constantly set controlled-by=policy and did visibility control using > > .hidden > > - if we removed/recreated the element on the fily > > - if we did it using the controlled-by attribute and added a comment that > > removing the attribute fully hides the element? > > I like option 3 - adding a comment that this hides the element from the DOM. > That should be sufficient. Done. https://codereview.chromium.org/2529083002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/extensions/extensions.js (right): https://codereview.chromium.org/2529083002/diff/120001/chrome/browser/resourc... chrome/browser/resources/extensions/extensions.js:236: setDevToggleControlledIndicator_: function(devModeControlledByPolicy) { On 2016/11/30 19:19:51, Devlin wrote: > nitty nit: maybe updateDevModeControlledIndicator? Done. https://codereview.chromium.org/2529083002/diff/120001/chrome/browser/resourc... chrome/browser/resources/extensions/extensions.js:237: var devToggleControlledIndicator = document.querySelector( On 2016/11/30 19:19:51, Devlin wrote: > nitty nit: can we just name this something like 'controlIndicator' for brevity? Done. https://codereview.chromium.org/2529083002/diff/120001/chrome/browser/resourc... chrome/browser/resources/extensions/extensions.js:243: if (devModeControlledByPolicy) { On 2016/11/30 19:19:51, Devlin wrote: > nit: can we also check the current status here? e.g. > var isActive = devToggleControlledIndicator.getAttribute('controlled-by'); > if (devModeControlledByPolicy && !isActive) { > ... > } else if (!devModeControlledByPolicy && isActive) { > ... > } Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by pmarko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/02 00:20:07, pmarko wrote: > PTAL > I have added two api unit tests. > @Devlin: to also have policy in the api unit test I had to change > extension_service_test_base, could you please take a look if that's OK > > https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/extensio... > File chrome/browser/extensions/api/developer_private/developer_private_api.cc > (right): > > https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/extensio... > chrome/browser/extensions/api/developer_private/developer_private_api.cc:604: if > (GetProfile()->IsSupervised()) > On 2016/11/30 19:19:50, Devlin wrote: > > On 2016/11/29 12:40:21, pmarko wrote: > > > On 2016/11/28 17:34:01, Devlin wrote: > > > > We should add a check here. Otherwise, the only guards we have against > this > > > are > > > > in JS, and that's very weak if anyone opens the developer console (e.g. > the > > > user > > > > could just type > > > > chrome.developerPrivate.updateProfileConfiguration({in_developer_mode: > > true}) > > > > and never have to hit the checkbox). > > > > > > > > I realize that, right now, these prefs are actually piggy backing, so > > > > theoretically the user might not be able to open the console, but that's a > > > > pretty weak guarantee, and very subject to change. Enforcing this at the > > C++ > > > > level is much less likely to be circumvented. :) > > > > > > I'm not sure it's necessary -- I had a check here back in patchset 3 when > the > > > plan was to set the pref to false explicitly. > > > > > > Now that it is set through ConfigurationPolicyPrefStore (see > > > configuration_policy_handler_list_factory.cc), the policy-controlled pref > > value > > > overrides any value set here, so I removed the check. > > > If the user passes that JS, he will only affect the value the pref has after > > the > > > policy will be removed. > > > So I think it should not be possible to trigger it that way when the policy > is > > > in place. Or am I missing something? > > > > I'll trust you and bauerb - my pref knowledge is limited. :) In either case, > > seems like a developerPrivate unittest would still be useful? Making sure we > > behave as expected when passed unexpected values? > > Sorry, I almost forgot -- added. > > https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/policy/p... > File chrome/browser/policy/policy_browsertest.cc (right): > > https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/policy/p... > chrome/browser/policy/policy_browsertest.cc:1349: > IN_PROC_BROWSER_TEST_F(PolicyTest, DeveloperToolsDisabledExtensionsDevMode) { > On 2016/11/30 19:19:50, Devlin wrote: > > On 2016/11/29 12:40:22, pmarko wrote: > > > On 2016/11/28 17:34:01, Devlin wrote: > > > > This is a pretty fragile browser test for a number of reasons - mainly > that > > a) > > > > it's very breakable with any changes in the extensions page and b) it only > > > tests > > > > that clicking with a mouse doesn't work (see also comment in > > > > developer_private_api.cc). I'd prefer to remove this and instead use a > new > > > api > > > > unittest test for the developerPrivate.updateProfileConfiguration method. > > > unittest: good idea, I'll check if I can add one. > > > > > > browsertest: > > > I should have described that better - the main purpose of the test was the > > > sequence: > > > - user enables devmode -> the dev controls appear > > > - policy comes in -> (1) dev controls automatically disappear and > > > (2) user can not re-enable it. > > > You are right in that it is only testing clicking on the toggle control -- > but > > > it clicks when the checkbox is still enabled. After the policy comes in, > I've > > > changed it to only check that the toggle control is disabled. > > > > > > As for the fragility: The test depends on the following assumptions about > the > > > extensions UI: > > > - that the control for toggling dev mode has the ID 'toggle-dev-on' > > > - that the dev-mode-buttons are in a DOM element with the ID 'dev-controls' > > > - that the implementor chose to display/hide the 'dev-controls' using the > > > 'height' attribute. > > > On the other hand, it seemed to me like a good trade-off for an end-to-end > > test > > > of the feature. This way we can have a regression test seeing if the > controls > > > are being actively hidden when the policy comes in. > > > > > > I'd suggest the following: > > > - I will extract the IDs/assumptions into constants and comment what they > are > > > - I will clearly document the why it's checking for height > > > > > > Would you think that this makes the test better or would you still favor not > > > having such a test at all? > > > > Ah, okay, that makes more sense. If this is deliberately just checking the > UI, > > I think it's fine. > > Acknowledged. > > https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/resource... > File chrome/browser/resources/extensions/extensions.js (right): > > https://codereview.chromium.org/2529083002/diff/80001/chrome/browser/resource... > chrome/browser/resources/extensions/extensions.js:251: > devToggleControlledIndicator.removeAttribute('controlled-by'); > On 2016/11/30 19:19:50, Devlin wrote: > > On 2016/11/29 12:40:22, pmarko wrote: > > > On 2016/11/28 17:34:01, Devlin wrote: > > > > does removing the attribute fully hide the element? It seems like it > would > > be > > > > easier to either use .hidden or to just create/remove the element fully > (as > > we > > > > do in extension_list.js). > > > > > > Removing it actually fully hides it due to controlled-indicator.css which > > > states: > > > .controlled-setting-indicator:not([controlled-by]) { > > > display: none; > > > } > > > As far as I understand it, this is also the mechanism controlled_setting.js > > uses > > > for showing the indicator (including deciding which icon to display)/hiding > it > > > (in detail, a listener on the Preference's controlledBy attrbiute transfers > > its > > > value to the controlledBy of the indicator object). That mechanism is used > on > > > the main (non-extensions) settings page, that's why I did it that way. > > > > > > Do you think it would be better > > > - if we constantly set controlled-by=policy and did visibility control using > > > .hidden > > > - if we removed/recreated the element on the fily > > > - if we did it using the controlled-by attribute and added a comment that > > > removing the attribute fully hides the element? > > > > I like option 3 - adding a comment that this hides the element from the DOM. > > That should be sufficient. > > Done. > > https://codereview.chromium.org/2529083002/diff/120001/chrome/browser/resourc... > File chrome/browser/resources/extensions/extensions.js (right): > > https://codereview.chromium.org/2529083002/diff/120001/chrome/browser/resourc... > chrome/browser/resources/extensions/extensions.js:236: > setDevToggleControlledIndicator_: function(devModeControlledByPolicy) { > On 2016/11/30 19:19:51, Devlin wrote: > > nitty nit: maybe updateDevModeControlledIndicator? > > Done. > > https://codereview.chromium.org/2529083002/diff/120001/chrome/browser/resourc... > chrome/browser/resources/extensions/extensions.js:237: var > devToggleControlledIndicator = document.querySelector( > On 2016/11/30 19:19:51, Devlin wrote: > > nitty nit: can we just name this something like 'controlIndicator' for > brevity? > > Done. > > https://codereview.chromium.org/2529083002/diff/120001/chrome/browser/resourc... > chrome/browser/resources/extensions/extensions.js:243: if > (devModeControlledByPolicy) { > On 2016/11/30 19:19:51, Devlin wrote: > > nit: can we also check the current status here? e.g. > > var isActive = devToggleControlledIndicator.getAttribute('controlled-by'); > > if (devModeControlledByPolicy && !isActive) { > > ... > > } else if (!devModeControlledByPolicy && isActive) { > > ... > > } > > Done. Fixed api unittests & modified them to test the updateProfileConfiguration .. getProfileConfiguration API call sequence.
lgtm, thanks! https://codereview.chromium.org/2529083002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc (right): https://codereview.chromium.org/2529083002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:108: void UpdateProfileConfigurationDevMode(bool devMode); nit: add descriptive function comment nit: dev_mode https://codereview.chromium.org/2529083002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:110: std::unique_ptr<api::developer_private::ProfileInfo> nit: add comment https://codereview.chromium.org/2529083002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:262: bool devMode) { (here too, dev_mode)
LGTM with a few suggestions https://codereview.chromium.org/2529083002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/api/developer_private/developer_private_api.h (right): https://codereview.chromium.org/2529083002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/developer_private/developer_private_api.h:125: // Handles a profile preference change. This is fine, but in general I don't like to pull in files to a CL just to fix typos. If you're already in the file, then OK, but to only add the file to fix a typo is something I usually avoid (drive a separate CL for that if you really want to fix typos). https://codereview.chromium.org/2529083002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc (right): https://codereview.chromium.org/2529083002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:286: EXPECT_TRUE(function->GetResultList()); ASSERT_TRUE() because your test will crash on the next line if this ever returns nullptr, so you might as well terminate the test now. https://codereview.chromium.org/2529083002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:287: EXPECT_EQ(1u, function->GetResultList()->GetSize()); I'd probably also ASSERT_EQ or ASSERT_GE here because the code below will crash if the result list is zero-sized. https://codereview.chromium.org/2529083002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:287: EXPECT_EQ(1u, function->GetResultList()->GetSize()); I'd probably also ASSERT_EQ or ASSERT_GE here because the code below will crash if the result list is zero-sized. https://codereview.chromium.org/2529083002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/extension_service_test_base.h (right): https://codereview.chromium.org/2529083002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/extension_service_test_base.h:61: bool profile_is_supervised; // defaults to false. I think intent is for comments to line up, so can you make them continue to line up (remove extra space, or add space to previous ones) https://codereview.chromium.org/2529083002/diff/180001/chrome/browser/policy/... File chrome/browser/policy/policy_prefs_browsertest.cc (right): https://codereview.chromium.org/2529083002/diff/180001/chrome/browser/policy/... chrome/browser/policy/policy_prefs_browsertest.cc:710: // If there should ever be many of these, we could consider grouping For whatever reason, I don't actually understand what you're saying in this comment. I can't tell if it's just because I'm being stupid or not. https://codereview.chromium.org/2529083002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/extensions/extensions.js (right): https://codereview.chromium.org/2529083002/diff/180001/chrome/browser/resourc... chrome/browser/resources/extensions/extensions.js:255: controlledIndicator.removeAttribute('controlled-by'); BTW, are the checks for isVisible/!isVisible needed here? Can't we just add/remove the attributes every time (isn't that action idempotent)?
https://codereview.chromium.org/2529083002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/extensions/extensions.js (right): https://codereview.chromium.org/2529083002/diff/180001/chrome/browser/resourc... chrome/browser/resources/extensions/extensions.js:255: controlledIndicator.removeAttribute('controlled-by'); On 2016/12/04 14:54:54, Andrew T Wilson (Slow) wrote: > BTW, are the checks for isVisible/!isVisible needed here? Can't we just > add/remove the attributes every time (isn't that action idempotent)? I requested it change from that. I slightly prefer this for the clarity and because it saves a small amount of work. If you feel strongly, I won't object to returning it to the original (unconditional setting).
On 2016/12/04 16:07:11, Devlin wrote: > https://codereview.chromium.org/2529083002/diff/180001/chrome/browser/resourc... > File chrome/browser/resources/extensions/extensions.js (right): > > https://codereview.chromium.org/2529083002/diff/180001/chrome/browser/resourc... > chrome/browser/resources/extensions/extensions.js:255: > controlledIndicator.removeAttribute('controlled-by'); > On 2016/12/04 14:54:54, Andrew T Wilson (Slow) wrote: > > BTW, are the checks for isVisible/!isVisible needed here? Can't we just > > add/remove the attributes every time (isn't that action idempotent)? > > I requested it change from that. I slightly prefer this for the clarity and > because it saves a small amount of work. If you feel strongly, I won't object > to returning it to the original (unconditional setting). I defer to Devlin on this - since he feels the optimization is worth the (small) extra complexity I'm good with it.
Drew: PTAL Devlin: PTAL Devlin: The coloring tool shows that developer_private_api has no LGTM and extension_service_test_base.* has no OWNER in the reviewer list -- but you are an OWNER of all of these files, or am I mistaken? Just cheking :) https://codereview.chromium.org/2529083002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc (right): https://codereview.chromium.org/2529083002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:108: void UpdateProfileConfigurationDevMode(bool devMode); On 2016/12/02 20:47:39, Devlin wrote: > nit: add descriptive function comment > nit: dev_mode Done. https://codereview.chromium.org/2529083002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:110: std::unique_ptr<api::developer_private::ProfileInfo> On 2016/12/02 20:47:39, Devlin wrote: > nit: add comment Done. https://codereview.chromium.org/2529083002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:262: bool devMode) { On 2016/12/02 20:47:39, Devlin wrote: > (here too, dev_mode) Done. https://codereview.chromium.org/2529083002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/api/developer_private/developer_private_api.h (right): https://codereview.chromium.org/2529083002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/developer_private/developer_private_api.h:125: // Handles a profile preference change. On 2016/12/04 14:54:54, Andrew T Wilson (Slow) wrote: > This is fine, but in general I don't like to pull in files to a CL just to fix > typos. If you're already in the file, then OK, but to only add the file to fix a > typo is something I usually avoid (drive a separate CL for that if you really > want to fix typos). This was more of an accident - I was adding something in here which I later reverted due to reviewers' feedback but forgot to revert the comment change. I'll take this out as it pollutes the file list :) https://codereview.chromium.org/2529083002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc (right): https://codereview.chromium.org/2529083002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:286: EXPECT_TRUE(function->GetResultList()); On 2016/12/04 14:54:54, Andrew T Wilson (Slow) wrote: > ASSERT_TRUE() because your test will crash on the next line if this ever returns > nullptr, so you might as well terminate the test now. Good point - had have to change the method type to void for that but I think that's a good trade-off. https://codereview.chromium.org/2529083002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:287: EXPECT_EQ(1u, function->GetResultList()->GetSize()); On 2016/12/04 14:54:54, Andrew T Wilson (Slow) wrote: > I'd probably also ASSERT_EQ or ASSERT_GE here because the code below will crash > if the result list is zero-sized. Done - taking ASSERT_EQ because the API is supposed to return exactly one response. https://codereview.chromium.org/2529083002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/extension_service_test_base.h (right): https://codereview.chromium.org/2529083002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/extension_service_test_base.h:61: bool profile_is_supervised; // defaults to false. On 2016/12/04 14:54:54, Andrew T Wilson (Slow) wrote: > I think intent is for comments to line up, so can you make them continue to line > up (remove extra space, or add space to previous ones) Done. https://codereview.chromium.org/2529083002/diff/180001/chrome/browser/policy/... File chrome/browser/policy/policy_prefs_browsertest.cc (right): https://codereview.chromium.org/2529083002/diff/180001/chrome/browser/policy/... chrome/browser/policy/policy_prefs_browsertest.cc:710: // If there should ever be many of these, we could consider grouping On 2016/12/04 14:54:54, Andrew T Wilson (Slow) wrote: > For whatever reason, I don't actually understand what you're saying in this > comment. I can't tell if it's just because I'm being stupid or not. I've tried to make it understandable, PTAL. https://codereview.chromium.org/2529083002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/extensions/extensions.js (right): https://codereview.chromium.org/2529083002/diff/180001/chrome/browser/resourc... chrome/browser/resources/extensions/extensions.js:255: controlledIndicator.removeAttribute('controlled-by'); On 2016/12/04 16:07:11, Devlin (in MON - PST plus 3) wrote: > On 2016/12/04 14:54:54, Andrew T Wilson (Slow) wrote: > > BTW, are the checks for isVisible/!isVisible needed here? Can't we just > > add/remove the attributes every time (isn't that action idempotent)? > > I requested it change from that. I slightly prefer this for the clarity and > because it saves a small amount of work. If you feel strongly, I won't object > to returning it to the original (unconditional setting). Acknowledged.
> Devlin: The coloring tool shows that developer_private_api has no LGTM and > extension_service_test_base.* has no OWNER in the reviewer list -- but you are > an OWNER of all of these files, or am I mistaken? > Just cheking :) I'm not sure which coloring tool you mean, but yeah, I'm an owner of all of those (and approve the changes there). s lgtm. https://codereview.chromium.org/2529083002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc (right): https://codereview.chromium.org/2529083002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:636: // Test developerPrivate.updateProfileConfiguration nitty nit: comment wrapping here and on line 654 looks odd? Also, comments need proper punctuation, so should end in a '.'. https://codereview.chromium.org/2529083002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/extension_service_test_base.cc (right): https://codereview.chromium.org/2529083002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/extension_service_test_base.cc:308: return profile_.get()->GetTestingPrefService(); nit: .get() is unnecessary; just use profile_->GetTestingPrefService().
The CQ bit was checked by pmarko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2529083002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc (right): https://codereview.chromium.org/2529083002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc:636: // Test developerPrivate.updateProfileConfiguration On 2016/12/07 16:21:46, Devlin (in MON - PST plus 3) wrote: > nitty nit: comment wrapping here and on line 654 looks odd? Also, comments need > proper punctuation, so should end in a '.'. Done. https://codereview.chromium.org/2529083002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/extension_service_test_base.cc (right): https://codereview.chromium.org/2529083002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/extension_service_test_base.cc:308: return profile_.get()->GetTestingPrefService(); On 2016/12/07 16:21:47, Devlin wrote: > nit: .get() is unnecessary; just use profile_->GetTestingPrefService(). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by pmarko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by pmarko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, atwilson@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2529083002/#ps260001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1481650651031170,
"parent_rev": "651771e0a46019b4ce3fc046b708e92b3a76cb65", "commit_rev":
"eb50621a24f992a49d019733b6e95d0646a01c1b"}
Message was sent while issue was closed.
Description was changed from ========== Make extensions developer mode adhere to policy Make the developer mode checkbox on chrome://extensions adhere to the DeveloperToolsDisabled policy. If it is disabled, display an indicator. Add corresponding indicator test to policy_prefs_browsertest. Add an end-to-end test to policy_browsertest. BUG=633684 TEST=browser_tests *PolicyPrefIndicatorTest*, PolicyTest.DeveloperToolsDisabledExtensionsDevMode CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Make extensions developer mode adhere to policy Make the developer mode checkbox on chrome://extensions adhere to the DeveloperToolsDisabled policy. If it is disabled, display an indicator. Add corresponding indicator test to policy_prefs_browsertest. Add an end-to-end test to policy_browsertest. BUG=633684 TEST=browser_tests *PolicyPrefIndicatorTest*, PolicyTest.DeveloperToolsDisabledExtensionsDevMode CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2529083002 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Make extensions developer mode adhere to policy Make the developer mode checkbox on chrome://extensions adhere to the DeveloperToolsDisabled policy. If it is disabled, display an indicator. Add corresponding indicator test to policy_prefs_browsertest. Add an end-to-end test to policy_browsertest. BUG=633684 TEST=browser_tests *PolicyPrefIndicatorTest*, PolicyTest.DeveloperToolsDisabledExtensionsDevMode CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2529083002 ========== to ========== Make extensions developer mode adhere to policy Make the developer mode checkbox on chrome://extensions adhere to the DeveloperToolsDisabled policy. If it is disabled, display an indicator. Add corresponding indicator test to policy_prefs_browsertest. Add an end-to-end test to policy_browsertest. BUG=633684 TEST=browser_tests *PolicyPrefIndicatorTest*, PolicyTest.DeveloperToolsDisabledExtensionsDevMode CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/6e36b463913efcea94e14a2de86caff388351290 Cr-Commit-Position: refs/heads/master@{#438215} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/6e36b463913efcea94e14a2de86caff388351290 Cr-Commit-Position: refs/heads/master@{#438215} |
