arc: kArcCompatibleFilesystemChosen pref to local state and integer.

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 <linux/magic.h>
 #include <sys/statfs.h>
 
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/sys_info.h"
 #include "base/threading/thread_restrictions.h"
 #include "chrome/browser/chromeos/arc/arc_session_manager.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/known_user.h"
 #include "components/user_manager/user.h"
 #include "components/user_manager/user_manager.h"
 
 namespace arc {
 
 namespace {
 
 constexpr char kLsbReleaseArcVersionKey[] = "CHROMEOS_ARC_ANDROID_SDK_VERSION";
 constexpr char kAndroidMSdkVersion[] = "23";
 
@@ skipping 82 matching lines @@
   if (user_manager::UserManager::Get()
           ->IsCurrentUserCryptohomeDataEphemeral()) {
     VLOG(1) << "Users with ephemeral data are not supported in ARC.";
     return false;
   }
 
   return true;
 }
 
 bool IsArcCompatibleFileSystemUsedForProfile(const Profile* profile) {
-  // chromeos::UserSessionManager::PrepareProfile does the actual file system
-  // check and stores the result to prefs, so that it survives crash-restart.
+  const user_manager::User* user =
+      chromeos::ProfileHelper::Get()->GetUserByProfile(profile);
+  if (!user)
+    return false;

[hidehiko, 2017/04/13 12:38:27]

IIUC, this case is not expected even for testing?
Then, could you add NOTREACHED()? If it is expected, then could you add VLOG()
with comments?

[xiyuan, 2017/04/13 16:35:59]

Think this should only happen in tests. GetUserByProfile is based on profile
path and should work after a user is logged in and we know the user hash.

[kinaba, 2017/04/14 04:33:55]

For instance, the sign-in profile does not have an user associated.

Currently this function is not called for such profiles, but its just because
IsArcAllowedForProfile() checks chromeos::ProfileHelper::IsPrimaryProfile
earlier than checking with this function--and the order is kind of arbitrary.
I'd like to keep the check with a comment added. WDYT?

+
+  // chromeos::UserSessionManager does the actual file system check and stores
+  // the result to prefs, so that it survives crash-restart.
+  int filesystem_compatibility = kFileSystemIncompatible;
+  user_manager::known_user::GetIntegerPref(
+      user->GetAccountId(), prefs::kArcCompatibleFilesystemChosen,
+      &filesystem_compatibility);
+
   const bool is_filesystem_compatible =
-      profile->GetPrefs()->GetBoolean(prefs::kArcCompatibleFilesystemChosen);
+      filesystem_compatibility >= kFileSystemCompatible;

[hidehiko, 2017/04/13 12:38:27]

Could you use "==" instead, just in case that a new value is added?
Or, could you comment a caution in the enum definition?

[kinaba, 2017/04/14 04:33:55]

changed to "!= Incompatible" (to align with the usage below,
and added a comment to the enum definition.

Only a safe choice here is to use switch-case and rely on the static
check on exhaustiveness, but to me it sounds like overkill.

   std::string arc_sdk_version;
   const bool is_M = base::SysInfo::GetLsbReleaseValue(kLsbReleaseArcVersionKey,
                                                       &arc_sdk_version) &&
                     arc_sdk_version == kAndroidMSdkVersion;
 
   // To run ARC we want to make sure either
   // - Underlying file system is compatible with ARC, or
   // - SDK version is M.
   if (!is_filesystem_compatible && !is_M) {
     VLOG(1)
@@ skipping 55 matching lines @@
   base::ThreadRestrictions::AssertIOAllowed();
 
   // If it can be verified it is not on ecryptfs, then it is ok.
   struct statfs statfs_buf;
   if (statfs(path.value().c_str(), &statfs_buf) < 0)
     return false;
   return statfs_buf.f_type != ECRYPTFS_SUPER_MAGIC;
 }
 
 }  // namespace arc