Extract kArcEnabled preference from ArcSessionManager part 1.

chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc

Index: chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc
diff --git a/chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc b/chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc
index 017ecaa23967ecdede358b9d5744db36615be9a9..ea1386565f296c1e33bc5b70af00eec239c1673b 100644
--- a/chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc
+++ b/chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc
@@ -13,6 +13,7 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/task_scheduler/post_task.h"
+#include "chrome/browser/chromeos/arc/arc_session_manager.h"
 #include "chrome/browser/chromeos/arc/arc_util.h"
 #include "chrome/browser/chromeos/arc/policy/arc_policy_util.h"
 #include "chrome/browser/profiles/profile.h"
@@ -141,9 +142,13 @@ void DeleteAppFolderFromFileThread(const base::FilePath& path) {
   DCHECK(deleted);
 }
 
-bool IsArcEnabled() {
+// TODO(crbug.com/672829): Due to shutdown procedure dependency,
+// ArcAppListPrefs may try to touch ArcSessionManager related stuff.
+// Specifically, this returns false on shutdown phase.
+// Remove this check after the shutdown behavior is fixed.
+bool IsArcAlive() {
   const auto* arc_session_manager = arc::ArcSessionManager::Get();
-  return arc_session_manager && arc_session_manager->IsArcPlayStoreEnabled();
+  return arc_session_manager && arc_session_manager->IsAllowed();
 }
 
 bool GetInt64FromPref(const base::DictionaryValue* dict,
@@ -263,8 +268,15 @@ void ArcAppListPrefs::StartPrefs() {
   arc::ArcSessionManager* arc_session_manager = arc::ArcSessionManager::Get();
   CHECK(arc_session_manager);
 
-  if (arc_session_manager->profile())
-    OnArcPlayStoreEnabledChanged(arc_session_manager->IsArcPlayStoreEnabled());
+  if (arc_session_manager->profile()) {
+    // Note: If ArcSessionManager has profile, it should be as same as the one
+    // this instance has, because ArcAppListPrefsFactory creates an instance
+    // only if the given Profile meets ARC's requirement.
+    // Anyway, just in case, check it here.
+    DCHECK_EQ(profile_, arc_session_manager->profile());
+    OnArcPlayStoreEnabledChanged(
+        arc::IsArcPlayStoreEnabledForProfile(profile_));
+  }
   arc_session_manager->AddObserver(this);
 
   app_instance_holder_->AddObserver(this);
@@ -385,7 +397,7 @@ bool ArcAppListPrefs::HasObserver(Observer* observer) {
 
 std::unique_ptr<ArcAppListPrefs::PackageInfo> ArcAppListPrefs::GetPackage(
     const std::string& package_name) const {
-  if (!IsArcEnabled())
+  if (!(IsArcAlive() && arc::IsArcPlayStoreEnabledForProfile(profile_)))

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

I prefer to use de Morgan's law to simplify these kinds of checks.

Then again, you are using it in several places (and in L465 where de Morgan's
does not help :/), so maybe extract it into a function?

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

Done for expansion for de Morgan's law.

As for wrapping function; indeed I actually thought about in the prev PS.
However, I couldn't come up with a good abstraction to wrap. Maybe very
straightforward name would be

IsArcAliveAndArcPlayStoreEnabledForProfile(profile).

Though, IMHO, the meaning looks complicated and actually the name looks too
long.
On the other hand, I do not want to re-define IsArcPlayStoreEnabledForProfile()
for different meaning (ASM check + Google Play Store pref check), because it
sounds just confusing.

Considering those reasons, I just kept them two as is.
If you have better abstraction and naming, please let me know. I'm happy to use
it :-).

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

It looks better in PS4, so I'm fine with the way you left it.

     return nullptr;
 
   const base::DictionaryValue* package = nullptr;
@@ -417,7 +429,7 @@ std::unique_ptr<ArcAppListPrefs::PackageInfo> ArcAppListPrefs::GetPackage(
 }
 
 std::vector<std::string> ArcAppListPrefs::GetAppIds() const {
-  if (!IsArcEnabled()) {
+  if (!(IsArcAlive() && arc::IsArcPlayStoreEnabledForProfile(profile_))) {
     // Default ARC apps available before OptIn.
     std::vector<std::string> ids;
     for (const auto& default_app : default_apps_.app_map()) {
@@ -450,7 +462,8 @@ std::vector<std::string> ArcAppListPrefs::GetAppIdsNoArcEnabledCheck() const {
 std::unique_ptr<ArcAppListPrefs::AppInfo> ArcAppListPrefs::GetApp(
     const std::string& app_id) const {
   // Information for default app is available before ARC enabled.
-  if (!IsArcEnabled() && !default_apps_.HasApp(app_id))
+  if (!(IsArcAlive() && arc::IsArcPlayStoreEnabledForProfile(profile_)) &&
+      !default_apps_.HasApp(app_id))
     return std::unique_ptr<AppInfo>();
 
   const base::DictionaryValue* app = nullptr;
@@ -505,7 +518,8 @@ std::unique_ptr<ArcAppListPrefs::AppInfo> ArcAppListPrefs::GetApp(
 }
 
 bool ArcAppListPrefs::IsRegistered(const std::string& app_id) const {
-  if (!IsArcEnabled() && !default_apps_.HasApp(app_id))
+  if (!(IsArcAlive() && arc::IsArcPlayStoreEnabledForProfile(profile_)) &&
+      !default_apps_.HasApp(app_id))
     return false;
 
   const base::DictionaryValue* app = nullptr;
@@ -704,7 +718,7 @@ void ArcAppListPrefs::MaybeAddNonLaunchableApp(
     const base::Optional<std::string>& name,
     const std::string& package_name,
     const std::string& activity) {
-  DCHECK(IsArcEnabled());
+  DCHECK(arc::IsArcPlayStoreEnabledForProfile(profile_));
   if (IsRegistered(GetAppId(package_name, activity)))
     return;
 
@@ -841,7 +855,7 @@ void ArcAppListPrefs::RemoveApp(const std::string& app_id) {
 
 void ArcAppListPrefs::AddOrUpdatePackagePrefs(
     PrefService* prefs, const arc::mojom::ArcPackageInfo& package) {
-  DCHECK(IsArcEnabled());
+  DCHECK(arc::IsArcPlayStoreEnabledForProfile(profile_));
   const std::string& package_name = package.package_name;
   default_apps_.MaybeMarkPackageUninstalled(package_name, false);
   if (package_name.empty()) {
@@ -864,7 +878,7 @@ void ArcAppListPrefs::AddOrUpdatePackagePrefs(
 
 void ArcAppListPrefs::RemovePackageFromPrefs(PrefService* prefs,
                                              const std::string& package_name) {
-  DCHECK(IsArcEnabled());
+  DCHECK(arc::IsArcPlayStoreEnabledForProfile(profile_));
   default_apps_.MaybeMarkPackageUninstalled(package_name, true);
   if (!default_apps_.HasPackage(package_name)) {
     DictionaryPrefUpdate update(prefs, prefs::kArcPackages);
@@ -881,7 +895,7 @@ void ArcAppListPrefs::RemovePackageFromPrefs(PrefService* prefs,
 
 void ArcAppListPrefs::OnAppListRefreshed(
     std::vector<arc::mojom::AppInfoPtr> apps) {
-  DCHECK(IsArcEnabled());
+  DCHECK(arc::IsArcPlayStoreEnabledForProfile(profile_));
   std::vector<std::string> old_apps = GetAppIds();
 
   ready_apps_.clear();
@@ -1170,7 +1184,7 @@ bool ArcAppListPrefs::IsUnknownPackage(const std::string& package_name) const {
 
 void ArcAppListPrefs::OnPackageAdded(
     arc::mojom::ArcPackageInfoPtr package_info) {
-  DCHECK(IsArcEnabled());
+  DCHECK(arc::IsArcPlayStoreEnabledForProfile(profile_));
 
   // Ignore packages installed by internal sync.
   DCHECK(sync_service_);
@@ -1188,7 +1202,7 @@ void ArcAppListPrefs::OnPackageAdded(
 
 void ArcAppListPrefs::OnPackageModified(
     arc::mojom::ArcPackageInfoPtr package_info) {
-  DCHECK(IsArcEnabled());
+  DCHECK(arc::IsArcPlayStoreEnabledForProfile(profile_));
   AddOrUpdatePackagePrefs(prefs_, *package_info);
   for (auto& observer : observer_list_)
     observer.OnPackageModified(*package_info);
@@ -1196,7 +1210,7 @@ void ArcAppListPrefs::OnPackageModified(
 
 void ArcAppListPrefs::OnPackageListRefreshed(
     std::vector<arc::mojom::ArcPackageInfoPtr> packages) {
-  DCHECK(IsArcEnabled());
+  DCHECK(arc::IsArcPlayStoreEnabledForProfile(profile_));
 
   const std::vector<std::string> old_packages(GetPackagesFromPrefs());
   std::unordered_set<std::string> current_packages;
@@ -1228,7 +1242,8 @@ std::vector<std::string> ArcAppListPrefs::GetPackagesFromPrefs() const {
 std::vector<std::string> ArcAppListPrefs::GetPackagesFromPrefs(
     bool installed) const {
   std::vector<std::string> packages;
-  if (!IsArcEnabled() && installed)
+  if (!(IsArcAlive() && arc::IsArcPlayStoreEnabledForProfile(profile_)) &&
+      installed)
     return packages;
 
   const base::DictionaryValue* package_prefs =