Extract kArcEnabled preference from ArcSessionManager part 1.

chrome/browser/chromeos/arc/arc_util.cc

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/arc/arc_util.h"
 
 #include "base/logging.h"
 #include "chrome/browser/chromeos/login/user_flow.h"
 #include "chrome/browser/chromeos/login/users/chrome_user_manager.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/common/pref_names.h"
 #include "components/arc/arc_util.h"
+#include "components/prefs/pref_service.h"
 #include "components/user_manager/user.h"
 #include "components/user_manager/user_manager.h"
 
 namespace arc {
 
 namespace {
 
 // Let IsAllowedForProfile() return "false" for any profile.
 bool g_disallow_for_testing = false;
 
@@ skipping 66 matching lines @@
     return false;
   }
 
   return true;
 }
 
 void DisallowArcForTesting() {
   g_disallow_for_testing = true;
 }
 
+bool IsArcPlayStoreEnabledForProfile(const Profile* profile) {
+  return IsArcAllowedForProfile(profile) &&
+         profile->GetPrefs()->GetBoolean(prefs::kArcEnabled);
+}
+
+bool IsArcPlayStoreEnabledPreferenceManagedForProfile(const Profile* profile) {
+  DCHECK(IsArcAllowedForProfile(profile));

[Luis Héctor Chávez, 2017/02/21 16:50:01]

Similar to the comments I left below: this is a probably aggressive behavioral
change because not all places that call this actually ensure one way or the
other that this is true since they rely on the old behavior of being a simple
if-not-allowed-return-false.

[hidehiko, 2017/02/21 17:25:14]

This is not behavior change actually.
In the old code ArcSessionManager::IsArcManaged() has DCHECK(profile_), which is
corresponding to this DCHECK, because if IsArcAllowedForProfile(profile) is
false, ASM::profile_ won't be set.

Technicaly, we can change the behavior of the case where
IsArcAllowedForProfile() returns false, but at the moment, I think it's ok to
keep the DCHECK because it is more strict than returning false.

WDYT?

[Luis Héctor Chávez, 2017/02/21 18:18:08]

Makes sense, but it's still something that can be easy to miss for someone
that's not super careful.

How about we do

if (!IsArcAllowedForProfile(profile)) {
  DCHECK(false) << "ARC is not allowed for profile";
  return false;
}

that way we have the best of both worlds: we don't accidentally regress in
production builds and we get a nicer log.

[hidehiko, 2017/02/23 04:17:36]

Done. BTW, LOG(DFATAL) is the way in this case :-).

+  return profile->GetPrefs()->IsManagedPreference(prefs::kArcEnabled);
+}
+
+void SetArcPlayStoreEnabledForProfile(Profile* profile, bool enabled) {
+  DCHECK(IsArcAllowedForProfile(profile));
+  if (IsArcPlayStoreEnabledPreferenceManagedForProfile(profile)) {
+    VLOG(1) << "Do nothing, if the Google-Play-Store-enabled pref is managed.";

[Luis Héctor Chávez, 2017/02/21 16:50:02]

nit: "Doing nothing since the Google-Play-Store-enabled pref is managed."

[hidehiko, 2017/02/21 17:25:14]

Done.

+    return;
+  }
+  profile->GetPrefs()->SetBoolean(prefs::kArcEnabled, enabled);
+}
+
 }  // namespace arc