Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1684063002:
Add ArcEnabled policy implementation (Closed)
|
Created:
4 years, 10 months ago by Polina Bondarenko Modified:
4 years, 8 months ago Reviewers:
bartfab (slow), jochen (gone - plz use gerrit), Dan Beam, Thiemo Nagel, hidehiko, hashimoto CC:
chromium-reviews, dbeam+watch-options_chromium.org, michaelpg+watch-options_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@26869593 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd ArcEnabled policy implementation
Hide "ARC OptIn" control from Chrome:Settings for enterprise users,
map ArcEnabled policy to ArcEnabled pref.
BUG=582440
Committed: https://crrev.com/33cc17d626fd54020cacc43e1cf2c08298ae8d52
Cr-Commit-Position: refs/heads/master@{#382266}
Patch Set 1 : #
Total comments: 3
Patch Set 2 : Disable android app section checkbox for managed profiles #
Total comments: 8
Patch Set 3 : andorid->android #
Total comments: 12
Patch Set 4 : Added browser test. #
Total comments: 55
Patch Set 5 : Fixed comments. #
Total comments: 11
Patch Set 6 : Fixed comments. #
Total comments: 6
Patch Set 7 : Inject ArcBridgeService for testing. #
Total comments: 2
Patch Set 8 : Rebase. #Patch Set 9 : Fixed indent. #Patch Set 10 : Fixed build issue. #Patch Set 11 : Fixed non-chromeos and gn build errors. #Patch Set 12 : Fixed GuestModeOptionsUIBrowserTest.testSections #Messages
Total messages: 70 (28 generated)
Patchset #1 (id:1) has been deleted
pbond@chromium.org changed reviewers: + tnagel@chromium.org
Hi Thiemo, PTAL
Description was changed from ========== Add ArcEnabled policy implementation Hide "ARC OptIn" control from Chrome:Settings for enterprise users, map ArcEnabled policy to ArcEnabled pref. BUG= ========== to ========== Add ArcEnabled policy implementation Hide "ARC OptIn" control from Chrome:Settings for enterprise users, map ArcEnabled policy to ArcEnabled pref. BUG=582440 ==========
https://codereview.chromium.org/1684063002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/1684063002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:1051: if (policy::ProfilePolicyConnectorFactory::GetForBrowserContext(profile) Wouldn't that hide the ARC checkbox for all managed users? Also, in case ARC is disabled by policy, we want to grey out the checkbox (not hide it).
https://codereview.chromium.org/1684063002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/1684063002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:1051: if (policy::ProfilePolicyConnectorFactory::GetForBrowserContext(profile) On 2016/02/12 17:32:08, Thiemo Nagel wrote: > Wouldn't that hide the ARC checkbox for all managed users? Also, in case ARC is > disabled by policy, we want to grey out the checkbox (not hide it). Do you mean that if ARC is enabled by policy, then user can use this checkbox? I thought that if ARC is enabled, it should be enabled without asking user.
https://codereview.chromium.org/1684063002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/1684063002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:1051: if (policy::ProfilePolicyConnectorFactory::GetForBrowserContext(profile) > Do you mean that if ARC is enabled by policy, then user can use this checkbox? I > thought that if ARC is enabled, it should be enabled without asking user. Yes, sure. But in that case the checkbox shouldn't be hidden, it should just be greyed out (and a management icon added to it). That's what we do whenever a policy overrides a setting.
Patchset #2 (id:40001) has been deleted
Disabled Android apps section for managed profiles.
https://codereview.chromium.org/1684063002/diff/60001/chrome/browser/resource... File chrome/browser/resources/options/browser_options.html (right): https://codereview.chromium.org/1684063002/diff/60001/chrome/browser/resource... chrome/browser/resources/options/browser_options.html:225: <section id="andorid-apps-section" guest-visibility="hidden"> "andorid" doesn't seem right. https://codereview.chromium.org/1684063002/diff/60001/chrome/browser/resource... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/1684063002/diff/60001/chrome/browser/resource... chrome/browser/resources/options/browser_options.js:2323: * disables it for managed profiles (ChromeOS only) Nit: Please keep the period at the end of the sentence. https://codereview.chromium.org/1684063002/diff/60001/chrome/browser/resource... chrome/browser/resources/options/browser_options.js:2326: var section = $('andorid-apps-section'); "andorid" doesn't look right. https://codereview.chromium.org/1684063002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/1684063002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:1056: base::FundamentalValue( I don't really understand how the settings page works, but is it really necessary to explicitly specify the state of management? It seems that for other settings this is done implicitly. How do you handle the case in which the policy is changed while the settings page is displayed? Also, does ARC shut down if it is disabled by policy while the session is running, and does it start it if it is enabled by policy while the session is running? Please add browser tests for these cases!
Patchset #3 (id:80001) has been deleted
https://codereview.chromium.org/1684063002/diff/60001/chrome/browser/resource... File chrome/browser/resources/options/browser_options.html (right): https://codereview.chromium.org/1684063002/diff/60001/chrome/browser/resource... chrome/browser/resources/options/browser_options.html:225: <section id="andorid-apps-section" guest-visibility="hidden"> On 2016/02/16 10:37:16, Thiemo Nagel wrote: > "andorid" doesn't seem right. Done. https://codereview.chromium.org/1684063002/diff/60001/chrome/browser/resource... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/1684063002/diff/60001/chrome/browser/resource... chrome/browser/resources/options/browser_options.js:2323: * disables it for managed profiles (ChromeOS only) On 2016/02/16 10:37:16, Thiemo Nagel wrote: > Nit: Please keep the period at the end of the sentence. Fixed. https://codereview.chromium.org/1684063002/diff/60001/chrome/browser/resource... chrome/browser/resources/options/browser_options.js:2326: var section = $('andorid-apps-section'); On 2016/02/16 10:37:16, Thiemo Nagel wrote: > "andorid" doesn't look right. Done. https://codereview.chromium.org/1684063002/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/1684063002/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/browser_options_handler.cc:1056: base::FundamentalValue( On 2016/02/16 10:37:16, Thiemo Nagel wrote: > I don't really understand how the settings page works, but is it really > necessary to explicitly specify the state of management? It seems that for > other settings this is done implicitly. > > How do you handle the case in which the policy is changed while the settings > page is displayed? > > Also, does ARC shut down if it is disabled by policy while the session is > running, and does it start it if it is enabled by policy while the session is > running? Please add browser tests for these cases! The implicit mechanism of applying policy: For enterprise users: ArcEnabled policy -> ArcEnabled pref -> checkbox The checkbox is disabled for managed profiles. (it's done for some other settings, see in browser_options.js file). For non-enterprise users: checkbox > ArcEnabled pref. If ARC is disabled by policy: ArcEnabled pref = false, checkbox is unchecked and disabled (no way for user to check it). The changes apply automatically on the fly to checkbox and ArcEnabled pref. The connection between ArcEnabled pref and ArcAuthService (and then -> Bridge -> ChromeOS SessionManager) is tested here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
pbond@chromium.org changed reviewers: + bartfab@chromium.org
bartfab@chromium.org: Please review changes.
https://codereview.chromium.org/1684063002/diff/100001/chrome/browser/policy/... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/1684063002/diff/100001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3947: IN_PROC_BROWSER_TEST_F(PolicyTest, ArcEnabled) { This test duplicates what policy_test_cases.json does already. Browser tests should not just check that a policy maps to a pref correctly. They should verify that the intended subsystems honor the policy/pref. https://codereview.chromium.org/1684063002/diff/100001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3948: PrefService* pref = browser()->profile()->GetPrefs(); Nit: const pointer. https://codereview.chromium.org/1684063002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/1684063002/diff/100001/chrome/browser/resourc... chrome/browser/resources/options/browser_options.js:2323: * Disables Android Apps checkbox for managed profiles. (ChromeOS only) Nit 1: s/profiles/users/ Nit 2: s/ChromeOS/Chrome OS/ while you are here. Nit 3: Move the full stop back after the brackets, not before. https://codereview.chromium.org/1684063002/diff/100001/chrome/browser/resourc... chrome/browser/resources/options/browser_options.js:2329: section.disabled = managed; The check box should disable itself when the pref becomes managed by policy. Why do you need to do it manually here? If the automatic mechanism is not working, please fix it rather than hacking around it. https://codereview.chromium.org/1684063002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/browser_options_handler.cc (right): https://codereview.chromium.org/1684063002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:1055: || arc::ArcAuthService::IsOptInVerificationDisabled()), Nit: Indentation. https://codereview.chromium.org/1684063002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/browser_options_handler.cc:1058: ->IsManaged())); Nit: Indentation.
https://codereview.chromium.org/1684063002/diff/100001/chrome/browser/policy/... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/1684063002/diff/100001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3947: IN_PROC_BROWSER_TEST_F(PolicyTest, ArcEnabled) { On 2016/02/16 15:23:00, bartfab (slow) wrote: > This test duplicates what policy_test_cases.json does already. Browser tests > should not just check that a policy maps to a pref correctly. They should verify > that the intended subsystems honor the policy/pref. There are some tests that verify ArcEnabled pref impact on ArcAuthService. And if we already have ArcEnabled policy->pref tests in policy_test_cases.json, should I add new policy_browsertest that verifies ArcEnabled policy -> ArcAuthService ?
https://codereview.chromium.org/1684063002/diff/100001/chrome/browser/policy/... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/1684063002/diff/100001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3947: IN_PROC_BROWSER_TEST_F(PolicyTest, ArcEnabled) { On 2016/02/16 15:41:13, Polina Bondarenko wrote: > On 2016/02/16 15:23:00, bartfab (slow) wrote: > > This test duplicates what policy_test_cases.json does already. Browser tests > > should not just check that a policy maps to a pref correctly. They should > verify > > that the intended subsystems honor the policy/pref. > > There are some tests that verify ArcEnabled pref impact on ArcAuthService. And > if we already have ArcEnabled policy->pref tests in policy_test_cases.json, > should I add new policy_browsertest that verifies ArcEnabled policy -> > ArcAuthService ? Yes. ArcEnabled policy->pref is one unit test, ArcEnabled pref->ArcAuthService is another. The missing piece is an integration test that verifies everything end-to-end in one go.
Patchset #4 (id:120001) has been deleted
Fixed comments, added browser test. https://codereview.chromium.org/1684063002/diff/100001/chrome/browser/policy/... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/1684063002/diff/100001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3948: PrefService* pref = browser()->profile()->GetPrefs(); On 2016/02/16 15:23:00, bartfab (slow) wrote: > Nit: const pointer. Done. https://codereview.chromium.org/1684063002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/1684063002/diff/100001/chrome/browser/resourc... chrome/browser/resources/options/browser_options.js:2323: * Disables Android Apps checkbox for managed profiles. (ChromeOS only) On 2016/02/16 15:23:00, bartfab (slow) wrote: > Nit 1: s/profiles/users/ > Nit 2: s/ChromeOS/Chrome OS/ while you are here. > Nit 3: Move the full stop back after the brackets, not before. Removed my changes, it works without explicit disable this section after reboot. https://codereview.chromium.org/1684063002/diff/100001/chrome/browser/resourc... chrome/browser/resources/options/browser_options.js:2329: section.disabled = managed; On 2016/02/16 15:23:00, bartfab (slow) wrote: > The check box should disable itself when the pref becomes managed by policy. Why > do you need to do it manually here? If the automatic mechanism is not working, > please fix it rather than hacking around it. Done.
https://codereview.chromium.org/1684063002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/1684063002/diff/100001/chrome/browser/resourc... chrome/browser/resources/options/browser_options.js:2323: * Disables Android Apps checkbox for managed profiles. (ChromeOS only) On 2016/02/25 15:28:30, Polina Bondarenko wrote: > On 2016/02/16 15:23:00, bartfab (slow) wrote: > > Nit 1: s/profiles/users/ > > Nit 2: s/ChromeOS/Chrome OS/ while you are here. > > Nit 3: Move the full stop back after the brackets, not before. > > Removed my changes, it works without explicit disable this section after reboot. What do you mean by "after reboot" - do you need to reboot your device for the policy to be reflected by the check box? That should not be. https://codereview.chromium.org/1684063002/diff/140001/chrome/browser/DEPS File chrome/browser/DEPS (right): https://codereview.chromium.org/1684063002/diff/140001/chrome/browser/DEPS#ne... chrome/browser/DEPS:14: "+components/arc/test", Does the above dependency on "components" not pull in all components already? https://codereview.chromium.org/1684063002/diff/140001/chrome/browser/policy/... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/1684063002/diff/140001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3975: scoped_ptr<chromeos::SessionManagerClient>( Nit: #include "chromeos/dbus/session_manager_client.h" https://codereview.chromium.org/1684063002/diff/140001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3976: fake_session_manager_client_.get())); You now have two scoped_ptrs who both think they own the same object (fake_session_manager_client_ and the new scoped_ptr<chromeos::SessionManagerClient> you just created). This will lead to a double-free. https://codereview.chromium.org/1684063002/diff/140001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3988: }; Nit: DISALLOW_COPY_AND_ASSIGN(ArcPolicyTest) https://codereview.chromium.org/1684063002/diff/140001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3994: const PrefService* pref = browser()->profile()->GetPrefs(); Nit: const pointer. https://codereview.chromium.org/1684063002/diff/140001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3995: const arc::ArcBridgeService* arc_bridge_service Nit: const pointer. https://codereview.chromium.org/1684063002/diff/140001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:4009: NULL); Nit: s/NULL/nullptr/ https://codereview.chromium.org/1684063002/diff/140001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:4020: NULL); Nit: s/NULL/nullptr/ https://codereview.chromium.org/1684063002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/1684063002/diff/140001/chrome/browser/resourc... chrome/browser/resources/options/browser_options.js:2327: * Hides Android Apps settings when they are not available. (ChromeOS only). Nit, while you are here: s/ChromeOS/Chrome OS/ https://codereview.chromium.org/1684063002/diff/140001/chromeos/dbus/fake_ses... File chromeos/dbus/fake_session_manager_client.cc (right): https://codereview.chromium.org/1684063002/diff/140001/chromeos/dbus/fake_ses... chromeos/dbus/fake_session_manager_client.cc:20: arc_available_(false) {} Nit: Put the closing curly brace on its own line. https://codereview.chromium.org/1684063002/diff/140001/components/arc.gypi File components/arc.gypi (right): https://codereview.chromium.org/1684063002/diff/140001/components/arc.gypi#ne... components/arc.gypi:76: 'arc/test/fake_arc_bridge_bootstrap.cc', Nit: Sort alphabetically. https://codereview.chromium.org/1684063002/diff/140001/components/arc/arc_bri... File components/arc/arc_bridge_bootstrap.cc (right): https://codereview.chromium.org/1684063002/diff/140001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:37: scoped_ptr<ArcBridgeBootstrap> fake_bootstrap_; 1) Nit: It may not be a fake - it could be a mock. A name like "bootstrap_for_testing" would be more general. 2) Nit: Globals are prefixed with "g_" 3) Nit: Remove the "_" suffix. This is not a member. 4) This is probably introducing a static initializer, which is not allowed in Chrome code. https://codereview.chromium.org/1684063002/diff/140001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:346: ArcBridgeBootstrap::~ArcBridgeBootstrap() { Nit: Add a blank line above. https://codereview.chromium.org/1684063002/diff/140001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:347: fake_bootstrap_.reset(); This should not be necessary. The global fake_bootstrap_ will eventually be destroyed, reaping any ArcBridgeBootstrap it may be holding. https://codereview.chromium.org/1684063002/diff/140001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:352: if (fake_bootstrap_ != NULL) { Nit: s/NULL/nullptr/ or, even easier, "if (fake_bootstrap_) {" https://codereview.chromium.org/1684063002/diff/140001/components/arc/arc_bri... File components/arc/arc_bridge_bootstrap.h (right): https://codereview.chromium.org/1684063002/diff/140001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.h:44: // until Create() is called, passes ownership to a caller. Nit 1: s/passes/then passes/ Nit 2: s/ a / the / https://codereview.chromium.org/1684063002/diff/140001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.h:45: static void SetBootstrapForTesting(ArcBridgeBootstrap* bootstrap); Use a scoped_ptr to indicate ownership transfer. https://codereview.chromium.org/1684063002/diff/140001/components/arc/arc_bri... File components/arc/arc_bridge_service_unittest.cc (right): https://codereview.chromium.org/1684063002/diff/140001/components/arc/arc_bri... components/arc/arc_bridge_service_unittest.cc:5: #include <utility> Nit: Is this still used? https://codereview.chromium.org/1684063002/diff/140001/components/arc/test/fa... File components/arc/test/fake_arc_bridge_bootstrap.cc (right): https://codereview.chromium.org/1684063002/diff/140001/components/arc/test/fa... components/arc/test/fake_arc_bridge_bootstrap.cc:9: #include "base/bind.h" Nit: Not used. https://codereview.chromium.org/1684063002/diff/140001/components/arc/test/fa... components/arc/test/fake_arc_bridge_bootstrap.cc:10: #include "base/bind_helpers.h" Nit: Not used. https://codereview.chromium.org/1684063002/diff/140001/components/arc/test/fa... components/arc/test/fake_arc_bridge_bootstrap.cc:16: : instance_(instance) {} Nit: If a method declaration is not inline, the closing brace always goes on its own line. https://codereview.chromium.org/1684063002/diff/140001/components/arc/test/fa... components/arc/test/fake_arc_bridge_bootstrap.cc:19: DCHECK(delegate_); Nit: #include "base/logging.h" https://codereview.chromium.org/1684063002/diff/140001/components/arc/test/fa... components/arc/test/fake_arc_bridge_bootstrap.cc:20: ArcBridgeInstancePtr instance; Nit: #include "components/arc/common/arc_bridge.mojom.h" https://codereview.chromium.org/1684063002/diff/140001/components/arc/test/fa... components/arc/test/fake_arc_bridge_bootstrap.cc:21: instance_->Bind(mojo::GetProxy(&instance)); Nit: #include "mojo/public/cpp/bindings/interface_request.h" https://codereview.chromium.org/1684063002/diff/140001/components/arc/test/fa... File components/arc/test/fake_arc_bridge_bootstrap.h (right): https://codereview.chromium.org/1684063002/diff/140001/components/arc/test/fa... components/arc/test/fake_arc_bridge_bootstrap.h:18: ~FakeArcBridgeBootstrap() override {} Nit: Do we really need to override this? https://codereview.chromium.org/1684063002/diff/140001/components/arc/test/fa... components/arc/test/fake_arc_bridge_bootstrap.h:20: void Start() override; Nit: // ArcBridgeBootstrap: https://codereview.chromium.org/1684063002/diff/140001/components/arc/test/fa... components/arc/test/fake_arc_bridge_bootstrap.h:26: DISALLOW_COPY_AND_ASSIGN(FakeArcBridgeBootstrap); Nit 1: Add blank line above. Nit 2: #include "base/macros.h"
Patchset #5 (id:160001) has been deleted
https://codereview.chromium.org/1684063002/diff/140001/chrome/browser/DEPS File chrome/browser/DEPS (right): https://codereview.chromium.org/1684063002/diff/140001/chrome/browser/DEPS#ne... chrome/browser/DEPS:14: "+components/arc/test", On 2016/03/02 14:45:40, bartfab (slow) wrote: > Does the above dependency on "components" not pull in all components already? Done. https://codereview.chromium.org/1684063002/diff/140001/chrome/browser/policy/... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/1684063002/diff/140001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3975: scoped_ptr<chromeos::SessionManagerClient>( On 2016/03/02 14:45:40, bartfab (slow) wrote: > Nit: #include "chromeos/dbus/session_manager_client.h" Done. https://codereview.chromium.org/1684063002/diff/140001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3976: fake_session_manager_client_.get())); On 2016/03/02 14:45:40, bartfab (slow) wrote: > You now have two scoped_ptrs who both think they own the same object > (fake_session_manager_client_ and the new > scoped_ptr<chromeos::SessionManagerClient> you just created). This will lead to > a double-free. Fixed. https://codereview.chromium.org/1684063002/diff/140001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3988: }; On 2016/03/02 14:45:40, bartfab (slow) wrote: > Nit: DISALLOW_COPY_AND_ASSIGN(ArcPolicyTest) Done. https://codereview.chromium.org/1684063002/diff/140001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3994: const PrefService* pref = browser()->profile()->GetPrefs(); On 2016/03/02 14:45:40, bartfab (slow) wrote: > Nit: const pointer. Done. https://codereview.chromium.org/1684063002/diff/140001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3995: const arc::ArcBridgeService* arc_bridge_service On 2016/03/02 14:45:40, bartfab (slow) wrote: > Nit: const pointer. Done. https://codereview.chromium.org/1684063002/diff/140001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:4009: NULL); On 2016/03/02 14:45:40, bartfab (slow) wrote: > Nit: s/NULL/nullptr/ Done. https://codereview.chromium.org/1684063002/diff/140001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:4020: NULL); On 2016/03/02 14:45:40, bartfab (slow) wrote: > Nit: s/NULL/nullptr/ Done. https://codereview.chromium.org/1684063002/diff/140001/chrome/browser/resourc... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/1684063002/diff/140001/chrome/browser/resourc... chrome/browser/resources/options/browser_options.js:2327: * Hides Android Apps settings when they are not available. (ChromeOS only). On 2016/03/02 14:45:40, bartfab (slow) wrote: > Nit, while you are here: s/ChromeOS/Chrome OS/ Done. https://codereview.chromium.org/1684063002/diff/140001/chromeos/dbus/fake_ses... File chromeos/dbus/fake_session_manager_client.cc (right): https://codereview.chromium.org/1684063002/diff/140001/chromeos/dbus/fake_ses... chromeos/dbus/fake_session_manager_client.cc:20: arc_available_(false) {} On 2016/03/02 14:45:40, bartfab (slow) wrote: > Nit: Put the closing curly brace on its own line. Formatting tool (git cl format) fixes it back:( https://codereview.chromium.org/1684063002/diff/140001/components/arc.gypi File components/arc.gypi (right): https://codereview.chromium.org/1684063002/diff/140001/components/arc.gypi#ne... components/arc.gypi:76: 'arc/test/fake_arc_bridge_bootstrap.cc', On 2016/03/02 14:45:40, bartfab (slow) wrote: > Nit: Sort alphabetically. Done. https://codereview.chromium.org/1684063002/diff/140001/components/arc/arc_bri... File components/arc/arc_bridge_bootstrap.cc (right): https://codereview.chromium.org/1684063002/diff/140001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:37: scoped_ptr<ArcBridgeBootstrap> fake_bootstrap_; On 2016/03/02 14:45:40, bartfab (slow) wrote: > 1) Nit: It may not be a fake - it could be a mock. A name like > "bootstrap_for_testing" would be more general. > 2) Nit: Globals are prefixed with "g_" > 3) Nit: Remove the "_" suffix. This is not a member. > 4) This is probably introducing a static initializer, which is not allowed in > Chrome code. Done, fixed to pointer. https://codereview.chromium.org/1684063002/diff/140001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:346: ArcBridgeBootstrap::~ArcBridgeBootstrap() { On 2016/03/02 14:45:40, bartfab (slow) wrote: > Nit: Add a blank line above. Done. https://codereview.chromium.org/1684063002/diff/140001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:352: if (fake_bootstrap_ != NULL) { On 2016/03/02 14:45:40, bartfab (slow) wrote: > Nit: s/NULL/nullptr/ or, even easier, "if (fake_bootstrap_) {" Done. https://codereview.chromium.org/1684063002/diff/140001/components/arc/arc_bri... File components/arc/arc_bridge_bootstrap.h (right): https://codereview.chromium.org/1684063002/diff/140001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.h:44: // until Create() is called, passes ownership to a caller. On 2016/03/02 14:45:40, bartfab (slow) wrote: > Nit 1: s/passes/then passes/ > Nit 2: s/ a / the / Done. https://codereview.chromium.org/1684063002/diff/140001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.h:45: static void SetBootstrapForTesting(ArcBridgeBootstrap* bootstrap); On 2016/03/02 14:45:40, bartfab (slow) wrote: > Use a scoped_ptr to indicate ownership transfer. Done. https://codereview.chromium.org/1684063002/diff/140001/components/arc/arc_bri... File components/arc/arc_bridge_service_unittest.cc (right): https://codereview.chromium.org/1684063002/diff/140001/components/arc/arc_bri... components/arc/arc_bridge_service_unittest.cc:5: #include <utility> On 2016/03/02 14:45:40, bartfab (slow) wrote: > Nit: Is this still used? Done. https://codereview.chromium.org/1684063002/diff/140001/components/arc/test/fa... File components/arc/test/fake_arc_bridge_bootstrap.cc (right): https://codereview.chromium.org/1684063002/diff/140001/components/arc/test/fa... components/arc/test/fake_arc_bridge_bootstrap.cc:9: #include "base/bind.h" On 2016/03/02 14:45:41, bartfab (slow) wrote: > Nit: Not used. Done. https://codereview.chromium.org/1684063002/diff/140001/components/arc/test/fa... components/arc/test/fake_arc_bridge_bootstrap.cc:10: #include "base/bind_helpers.h" On 2016/03/02 14:45:41, bartfab (slow) wrote: > Nit: Not used. Done. https://codereview.chromium.org/1684063002/diff/140001/components/arc/test/fa... components/arc/test/fake_arc_bridge_bootstrap.cc:16: : instance_(instance) {} On 2016/03/02 14:45:41, bartfab (slow) wrote: > Nit: If a method declaration is not inline, the closing brace always goes on its > own line. Done. https://codereview.chromium.org/1684063002/diff/140001/components/arc/test/fa... components/arc/test/fake_arc_bridge_bootstrap.cc:19: DCHECK(delegate_); On 2016/03/02 14:45:41, bartfab (slow) wrote: > Nit: #include "base/logging.h" Done. https://codereview.chromium.org/1684063002/diff/140001/components/arc/test/fa... components/arc/test/fake_arc_bridge_bootstrap.cc:20: ArcBridgeInstancePtr instance; On 2016/03/02 14:45:41, bartfab (slow) wrote: > Nit: #include "components/arc/common/arc_bridge.mojom.h" Done. https://codereview.chromium.org/1684063002/diff/140001/components/arc/test/fa... components/arc/test/fake_arc_bridge_bootstrap.cc:21: instance_->Bind(mojo::GetProxy(&instance)); On 2016/03/02 14:45:41, bartfab (slow) wrote: > Nit: #include "mojo/public/cpp/bindings/interface_request.h" Done. https://codereview.chromium.org/1684063002/diff/140001/components/arc/test/fa... File components/arc/test/fake_arc_bridge_bootstrap.h (right): https://codereview.chromium.org/1684063002/diff/140001/components/arc/test/fa... components/arc/test/fake_arc_bridge_bootstrap.h:18: ~FakeArcBridgeBootstrap() override {} On 2016/03/02 14:45:41, bartfab (slow) wrote: > Nit: Do we really need to override this? Yes, g_bootstrap_for_testing is deleted in destructor of ArcBridgeBootstrap. https://codereview.chromium.org/1684063002/diff/140001/components/arc/test/fa... components/arc/test/fake_arc_bridge_bootstrap.h:20: void Start() override; On 2016/03/02 14:45:41, bartfab (slow) wrote: > Nit: // ArcBridgeBootstrap: Done. https://codereview.chromium.org/1684063002/diff/140001/components/arc/test/fa... components/arc/test/fake_arc_bridge_bootstrap.h:26: DISALLOW_COPY_AND_ASSIGN(FakeArcBridgeBootstrap); On 2016/03/02 14:45:41, bartfab (slow) wrote: > Nit 1: Add blank line above. > Nit 2: #include "base/macros.h" Done.
https://codereview.chromium.org/1684063002/diff/140001/chromeos/dbus/fake_ses... File chromeos/dbus/fake_session_manager_client.cc (right): https://codereview.chromium.org/1684063002/diff/140001/chromeos/dbus/fake_ses... chromeos/dbus/fake_session_manager_client.cc:20: arc_available_(false) {} On 2016/03/06 20:22:33, Polina Bondarenko wrote: > On 2016/03/02 14:45:40, bartfab (slow) wrote: > > Nit: Put the closing curly brace on its own line. > > Formatting tool (git cl format) fixes it back:( Then the tool disagrees with the style guide, but it is easier to just live with it than to fight back :). https://codereview.chromium.org/1684063002/diff/140001/components/arc/test/fa... File components/arc/test/fake_arc_bridge_bootstrap.h (right): https://codereview.chromium.org/1684063002/diff/140001/components/arc/test/fa... components/arc/test/fake_arc_bridge_bootstrap.h:18: ~FakeArcBridgeBootstrap() override {} On 2016/03/06 20:22:34, Polina Bondarenko wrote: > On 2016/03/02 14:45:41, bartfab (slow) wrote: > > Nit: Do we really need to override this? > > Yes, g_bootstrap_for_testing is deleted in destructor of ArcBridgeBootstrap. I do not understand. Yes, ArcBridgeBootstrap::~ArcBridgeBootstrap() will clean up g_bootstrap_for_testing. What does that have to do with overriding the method? The inherited destructor will run anyway - you cannot prevent this via inheritance. https://codereview.chromium.org/1684063002/diff/180001/chrome/browser/policy/... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/1684063002/diff/180001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3984: arc::ArcBridgeBootstrap::SetBootstrapForTesting(make_scoped_ptr( This is another place where one object has two owners: |fake_arc_bridge_instance_| and |g_bootstrap_for_testing| both think they own the instance. https://codereview.chromium.org/1684063002/diff/180001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:4001: PrefService const *pref = browser()->profile()->GetPrefs(); Nit 1: You had a pointer to const here before. If you are only calling const methods, you should be using a const pointer to const (two consts). Nit 2: Put the * on the type. https://codereview.chromium.org/1684063002/diff/180001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:4002: arc::ArcBridgeService const *arc_bridge_service Nit 1: As above, if you are calling const methods only, this should be a const pointer to const. Nit 2: Put the * on the type. https://codereview.chromium.org/1684063002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/1684063002/diff/180001/chrome/browser/resourc... chrome/browser/resources/options/browser_options.js:2327: * Hides Android Apps settings when they are not available. Nit: Does this get unhidden when the policy changes on the fly? https://codereview.chromium.org/1684063002/diff/180001/components/arc/arc_bri... File components/arc/arc_bridge_bootstrap.cc (right): https://codereview.chromium.org/1684063002/diff/180001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:356: ArcBridgeBootstrap *result = g_bootstrap_for_testing; Nit 1: const pointer. Nit 2: Move the * to the type.
https://codereview.chromium.org/1684063002/diff/180001/chrome/browser/policy/... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/1684063002/diff/180001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3984: arc::ArcBridgeBootstrap::SetBootstrapForTesting(make_scoped_ptr( On 2016/03/07 13:44:04, bartfab (slow) wrote: > This is another place where one object has two owners: > |fake_arc_bridge_instance_| and |g_bootstrap_for_testing| both think they own > the instance. No, fake_arc_bridge_bootstrap_ doesn't own this instance. There is a comment in the code, that explicitly says that. https://codereview.chromium.org/1684063002/diff/180001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:4001: PrefService const *pref = browser()->profile()->GetPrefs(); On 2016/03/07 13:44:04, bartfab (slow) wrote: > Nit 1: You had a pointer to const here before. If you are only calling const > methods, you should be using a const pointer to const (two consts). > Nit 2: Put the * on the type. Done. https://codereview.chromium.org/1684063002/diff/180001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:4002: arc::ArcBridgeService const *arc_bridge_service On 2016/03/07 13:44:04, bartfab (slow) wrote: > Nit 1: As above, if you are calling const methods only, this should be a const > pointer to const. > Nit 2: Put the * on the type. Done. https://codereview.chromium.org/1684063002/diff/180001/chrome/browser/resourc... File chrome/browser/resources/options/browser_options.js (right): https://codereview.chromium.org/1684063002/diff/180001/chrome/browser/resourc... chrome/browser/resources/options/browser_options.js:2327: * Hides Android Apps settings when they are not available. On 2016/03/07 13:44:04, bartfab (slow) wrote: > Nit: Does this get unhidden when the policy changes on the fly? This is hidden only if ARC is not available on current device. E.g. for minnies: - enterprise user sees grayed-out checkbox and current state of ARC: checked or not depending on policy. (If policy changes on the fly, then a state of checkbox changes.) - Non-enterprise user can manage this checkbox himself. https://codereview.chromium.org/1684063002/diff/180001/components/arc/arc_bri... File components/arc/arc_bridge_bootstrap.cc (right): https://codereview.chromium.org/1684063002/diff/180001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:356: ArcBridgeBootstrap *result = g_bootstrap_for_testing; On 2016/03/07 13:44:04, bartfab (slow) wrote: > Nit 1: const pointer. > Nit 2: Move the * to the type. Done.
https://codereview.chromium.org/1684063002/diff/180001/chrome/browser/policy/... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/1684063002/diff/180001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3984: arc::ArcBridgeBootstrap::SetBootstrapForTesting(make_scoped_ptr( On 2016/03/09 17:59:15, Polina Bondarenko wrote: > On 2016/03/07 13:44:04, bartfab (slow) wrote: > > This is another place where one object has two owners: > > |fake_arc_bridge_instance_| and |g_bootstrap_for_testing| both think they own > > the instance. > > No, fake_arc_bridge_bootstrap_ doesn't own this instance. There is a comment in > the code, that explicitly says that. If SetBootstrapForTesting() does not take ownership, why is it taking a scoped_ptr?
lgtm
pbond@chromium.org changed reviewers: + dbeam@chromium.org, hashimoto@chromium.org, hidehiko@chromium.org
Hi, I've added implementation of ArcEnabled policy, which enables or disables ARC for enterprise users. Enterprise user can't manage ARC checkbox, but they can see ARC policy state in that. hashimoto@chromium.org: Please review changes in chromeos/dbus/* files. dbeam@chromium.org: Please review changes in chrome/browser/resources/options/browser_options* files. hidehiko@chromium.org: Please review changes in components/arc* files. Thanks!
lgtm
https://codereview.chromium.org/1684063002/diff/200001/chrome/browser/policy/... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/1684063002/diff/200001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3984: arc::ArcBridgeBootstrap::SetBootstrapForTesting(make_scoped_ptr( Can we inject ArcBridgeService instead of Bootstrap? https://codereview.chromium.org/1684063002/diff/200001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3987: PolicyTest::SetUpInProcessBrowserTestFixture(); nit: In most cases, parent method for SetUp is called at first. If there is a special reason to put this last, comment about it would be useful. https://codereview.chromium.org/1684063002/diff/200001/components/arc/arc_bri... File components/arc/arc_bridge_bootstrap.h (right): https://codereview.chromium.org/1684063002/diff/200001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.h:43: // Set instance for testing. ArcBridgeBootstrap owns |bootstrap| ArcBridgeBootstrap is not a singleton, so Set*ForTesting is something we should not have here, IMHO. Could you move the injection into upper layer, instead?
Patchset #7 (id:220001) has been deleted
Fixed comments. Added injection of ArcBridgeService for testing. https://codereview.chromium.org/1684063002/diff/200001/chrome/browser/policy/... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/1684063002/diff/200001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3984: arc::ArcBridgeBootstrap::SetBootstrapForTesting(make_scoped_ptr( On 2016/03/15 07:01:53, hidehiko wrote: > Can we inject ArcBridgeService instead of Bootstrap? Done. https://codereview.chromium.org/1684063002/diff/200001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:3987: PolicyTest::SetUpInProcessBrowserTestFixture(); On 2016/03/15 07:01:53, hidehiko wrote: > nit: In most cases, parent method for SetUp is called at first. If there is a > special reason to put this last, comment about it would be useful. Fixed. https://codereview.chromium.org/1684063002/diff/200001/components/arc/arc_bri... File components/arc/arc_bridge_bootstrap.h (right): https://codereview.chromium.org/1684063002/diff/200001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.h:43: // Set instance for testing. ArcBridgeBootstrap owns |bootstrap| On 2016/03/15 07:01:53, hidehiko wrote: > ArcBridgeBootstrap is not a singleton, so Set*ForTesting is something we should > not have here, IMHO. > Could you move the injection into upper layer, instead? Done.
hidehiko@ and hashimoto@ ping:)
hidehiko@ and hashimoto@ ping:)
Sorry for being late. https://codereview.chromium.org/1684063002/diff/240001/components/arc/arc_ser... File components/arc/arc_service_manager.cc (right): https://codereview.chromium.org/1684063002/diff/240001/components/arc/arc_ser... components/arc/arc_service_manager.cc:26: ArcBridgeService* g_arc_bridge_service_for_testing = nullptr; Could you avoid the global var here? Would it be possible to inject the ArcBridgeService instance via ctor's argument? ArcServiceManager::ArcServiceManager() : ArcServiceManager(nullptr) { } ArcServiceManager::ArcServiceManager(scoped_ptr<ArcBridgeService> arc_bridge_service) { if (!arc_bridge_service) { arc_bridge_service.reset(new ArcBridgeServiceImpl(...)); } arc_bridge_service_ = arc_bridge_service.Pass(); : }
Patchset #8 (id:260001) has been deleted
chromeos/dbus lgtm Sorry for being late.
Fixed hidehiko@'s comment: ArcBridgeService injection. https://codereview.chromium.org/1684063002/diff/240001/components/arc/arc_ser... File components/arc/arc_service_manager.cc (right): https://codereview.chromium.org/1684063002/diff/240001/components/arc/arc_ser... components/arc/arc_service_manager.cc:26: ArcBridgeService* g_arc_bridge_service_for_testing = nullptr; On 2016/03/16 16:45:35, hidehiko wrote: > Could you avoid the global var here? > Would it be possible to inject the ArcBridgeService instance via ctor's > argument? > > ArcServiceManager::ArcServiceManager() : ArcServiceManager(nullptr) { > } > > ArcServiceManager::ArcServiceManager(scoped_ptr<ArcBridgeService> > arc_bridge_service) { > if (!arc_bridge_service) { > arc_bridge_service.reset(new ArcBridgeServiceImpl(...)); > } > arc_bridge_service_ = arc_bridge_service.Pass(); > > : > } Fixed: 1) Create ArcServiceManager(fake arc_bridge_service) in browser_test to pass arc_bridge_service to real ArcServiceManager (owned by ArcPolicyTest). 2) Create ArcServiceManager in ArcServiceLauncher::Initialize(), which takes passed from browser test arc_bridge_service and sets itself as g_arc_service_manager. 3) There is correct ArcServiceManager and stuff with faked ArcBridgeService. 4) Run browser test ArcPolicyTest. 5) Destroy ArcServiceManager which is owned by ArcPolicyTest. 6) Destroy real ArcServiceManager (= g_arc_service_manager). The most weird thing is that there are 2 ArcServiceManagers but it has to be one. One of them is used just to pass arc_bridge_service. That's why DCHECKs for checking uniqueness of ArcServiceManager are removed. But all instances of ArcServiceManager are properly owned and destroyed. I'm not sure which approach is better. New one may be seems to be clearer.
pbond@chromium.org changed reviewers: + jochen@chromium.org
jochen@chromium.org: Please review changes in components/arc.gypi
Ah you're right we accidentally create two ArcServiceManager in this way. Hmm, but it worries me, to be honest. I think we can avoid it, but it requires more refactoring which is unrelated to yours. I do not think it's reasonable to block you. So, would you mind to move back the injection code to PS8 (i.e. using SetArcBridgeServiceForTesting)? Sorry for inconvenient dup-work. Assuming you agree with me about the above comment, LGTM PS8 for components/arc. https://codereview.chromium.org/1684063002/diff/300001/components/arc/test/fa... File components/arc/test/fake_arc_bridge_bootstrap.cc (right): https://codereview.chromium.org/1684063002/diff/300001/components/arc/test/fa... components/arc/test/fake_arc_bridge_bootstrap.cc:28: DCHECK(delegate_); nit: indent.
Patchset #9 (id:300001) has been deleted
lgtm
Thanks everybody for reviewing!
The CQ bit was checked by pbond@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bartfab@chromium.org, dbeam@chromium.org, hashimoto@chromium.org, hidehiko@chromium.org Link to the patchset: https://codereview.chromium.org/1684063002/#ps320001 (title: "Fixed indent.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1684063002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1684063002/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by pbond@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bartfab@chromium.org, jochen@chromium.org, dbeam@chromium.org, hidehiko@chromium.org, hashimoto@chromium.org Link to the patchset: https://codereview.chromium.org/1684063002/#ps340001 (title: "Fixed build issue.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1684063002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1684063002/340001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by pbond@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bartfab@chromium.org, jochen@chromium.org, dbeam@chromium.org, hidehiko@chromium.org, hashimoto@chromium.org Link to the patchset: https://codereview.chromium.org/1684063002/#ps350001 (title: "Fixed non-chromeos and gn build errors.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1684063002/350001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1684063002/350001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by pbond@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1684063002/370001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1684063002/370001
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 pbond@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bartfab@chromium.org, jochen@chromium.org, dbeam@chromium.org, hidehiko@chromium.org, hashimoto@chromium.org Link to the patchset: https://codereview.chromium.org/1684063002/#ps370001 (title: "Fixed GuestModeOptionsUIBrowserTest.testSections")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1684063002/370001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1684063002/370001
Message was sent while issue was closed.
Description was changed from ========== Add ArcEnabled policy implementation Hide "ARC OptIn" control from Chrome:Settings for enterprise users, map ArcEnabled policy to ArcEnabled pref. BUG=582440 ========== to ========== Add ArcEnabled policy implementation Hide "ARC OptIn" control from Chrome:Settings for enterprise users, map ArcEnabled policy to ArcEnabled pref. BUG=582440 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:370001)
Message was sent while issue was closed.
Description was changed from ========== Add ArcEnabled policy implementation Hide "ARC OptIn" control from Chrome:Settings for enterprise users, map ArcEnabled policy to ArcEnabled pref. BUG=582440 ========== to ========== Add ArcEnabled policy implementation Hide "ARC OptIn" control from Chrome:Settings for enterprise users, map ArcEnabled policy to ArcEnabled pref. BUG=582440 Committed: https://crrev.com/33cc17d626fd54020cacc43e1cf2c08298ae8d52 Cr-Commit-Position: refs/heads/master@{#382266} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/33cc17d626fd54020cacc43e1cf2c08298ae8d52 Cr-Commit-Position: refs/heads/master@{#382266} |
