Fix build when ENABLE_MANAGED_USERS is not defined

chrome/browser/ui/webui/options/create_profile_handler.cc

Index: chrome/browser/ui/webui/options/create_profile_handler.cc
diff --git a/chrome/browser/ui/webui/options/create_profile_handler.cc b/chrome/browser/ui/webui/options/create_profile_handler.cc
index facc7f0054b23dbd8c8708035b146d20d17f8294..ac7f4522054401d9c2852de3418d1fb21ca28bf4 100644
--- a/chrome/browser/ui/webui/options/create_profile_handler.cc
+++ b/chrome/browser/ui/webui/options/create_profile_handler.cc
@@ -14,18 +14,22 @@
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/profiles/profile_metrics.h"
 #include "chrome/browser/profiles/profiles_state.h"
-#include "chrome/browser/supervised_user/supervised_user_registration_utility.h"
-#include "chrome/browser/supervised_user/supervised_user_service.h"
-#include "chrome/browser/supervised_user/supervised_user_service_factory.h"
-#include "chrome/browser/supervised_user/supervised_user_sync_service.h"
-#include "chrome/browser/supervised_user/supervised_user_sync_service_factory.h"
 #include "chrome/browser/sync/profile_sync_service.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/browser/ui/webui/options/options_handlers_helper.h"
 #include "chrome/common/pref_names.h"
+#include "content/public/browser/web_ui.h"
 #include "grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 
+#if defined(ENABLE_MANAGED_USERS)

[Bernhard Bauer, 2014/07/14 16:39:05]

This file is a bit pushing it with regards to the number of #ifdefs I think are
palatable. Another approach would be not to #ifdef out any and all code that
refers to supervised users, but e.g. only code that refers to classes from
c/b/supervised_user/, replace it with NOTREACHED(), and just disable entry
points for the feature (i.e. callbacks from WebUI in this case).

[mckev, 2014/07/14 23:50:03]

I tend to agree - this is a huge number of #ifdefs that I'd like to avoid as
well.  The only issue I foresee with this approach would be that this class
wouldn't be compilable, since it would permanently include these files and all
of their dependencies.

For this class at least, would it be feasible to put the supervised user logic
in a separate .h/.cc files, a la create_profile_handler_supervised?  There might
be some #ifdefs in the main files, but at least we'd be one step closer to
factoring it out entirely.

[Bernhard Bauer, 2014/07/16 09:54:00]

Maybe -- it's a bit tricky of course, because our goal here was reusing existing
profile creation code, not keeping supervised user code separate from other
code. That being said, if you want to try your hand at it, I'd be willing to
review patches :) 

+#include "chrome/browser/supervised_user/supervised_user_registration_utility.h"
+#include "chrome/browser/supervised_user/supervised_user_service.h"
+#include "chrome/browser/supervised_user/supervised_user_service_factory.h"
+#include "chrome/browser/supervised_user/supervised_user_sync_service.h"
+#include "chrome/browser/supervised_user/supervised_user_sync_service_factory.h"
+#endif
+
 namespace options {
 
 CreateProfileHandler::CreateProfileHandler()
@@ -53,12 +57,12 @@ void CreateProfileHandler::RegisterMessages() {
 }
 
 void CreateProfileHandler::CreateProfile(const base::ListValue* args) {
+#if defined(ENABLE_MANAGED_USERS)
   // This handler could have been called for a supervised user, for example
-  // because the user fiddled with the web inspector. Silently return in this
-  // case.
-  Profile* current_profile = Profile::FromWebUI(web_ui());
-  if (current_profile->IsSupervised())
+  // because the user fiddled with the web inspector. Silently return.
+  if (Profile::FromWebUI(web_ui())->IsSupervised())
     return;
+#endif
 
   if (!profiles::IsMultipleProfilesEnabled())
     return;
@@ -76,16 +80,23 @@ void CreateProfileHandler::CreateProfile(const base::ListValue* args) {
 
   base::string16 name;
   base::string16 icon;
-  std::string supervised_user_id;
   bool create_shortcut = false;
-  bool supervised_user = false;
+  // If supervised users aren't enabled, an empty user ID is fine.
+  std::string supervised_user_id;
+
   if (args->GetString(0, &name) && args->GetString(1, &icon)) {
-    if (args->GetBoolean(2, &create_shortcut)) {
+    args->GetBoolean(2, &create_shortcut);
+  }
+
+#if defined(ENABLE_MANAGED_USERS)
+  bool supervised_user = false;
+
+  if (args->GetSize() == 4) {
       bool success = args->GetBoolean(3, &supervised_user);
       DCHECK(success);
+
       success = args->GetString(4, &supervised_user_id);
       DCHECK(success);
-    }
   }
 
   if (supervised_user) {
@@ -99,11 +110,11 @@ void CreateProfileHandler::CreateProfile(const base::ListValue* args) {
           SupervisedUserRegistrationUtility::GenerateNewSupervisedUserId();
 
       // If sync is not yet fully initialized, the creation may take extra time,
-      // so show a message. Import doesn't wait for an acknowledgement, so it
+      // so show a message. Import doesn't wait for an acknowledgment, so it
       // won't have the same potential delay.
       ProfileSyncService* sync_service =
           ProfileSyncServiceFactory::GetInstance()->GetForProfile(
-              current_profile);
+              Profile::FromWebUI(web_ui()));
       ProfileSyncService::SyncStatusSummary status =
           sync_service->QuerySyncStatusSummary();
       if (status == ProfileSyncService::DATATYPES_NOT_INITIALIZED) {
@@ -112,6 +123,7 @@ void CreateProfileHandler::CreateProfile(const base::ListValue* args) {
       }
     }
   }
+#endif
 
   ProfileMetrics::LogProfileAddNewUser(ProfileMetrics::ADD_NEW_USER_DIALOG);
 
@@ -173,66 +185,20 @@ void CreateProfileHandler::HandleProfileCreationSuccess(
       CreateShortcutAndShowSuccess(create_shortcut, desktop_type, profile);
       break;
     }
+#if defined(ENABLE_MANAGED_USERS)
     case SUPERVISED_PROFILE_CREATION:
     case SUPERVISED_PROFILE_IMPORT:
       RegisterSupervisedUser(create_shortcut, desktop_type,
                              supervised_user_id, profile);
       break;
+#endif
     case NO_CREATION_IN_PROGRESS:
+    default:

[Bernhard Bauer, 2014/07/14 16:39:05]

Is this actually necessary? If ENABLE_MANAGED_USERS is not defined, there are no
SUPERVISED_PROFILE_* values in the enum.

[mckev, 2014/07/14 23:50:03]

Yep, you're right.  I added that opportunistically to guard against unused
future values.  I just re-read the style guide and I see this isn't correct for
enumerations (the compiler will already complain about unused values).

       NOTREACHED();
       break;
   }
 }
 
-void CreateProfileHandler::RegisterSupervisedUser(
-    bool create_shortcut,
-    chrome::HostDesktopType desktop_type,
-    const std::string& supervised_user_id,
-    Profile* new_profile) {
-  DCHECK_EQ(profile_path_being_created_.value(),
-            new_profile->GetPath().value());
-
-  SupervisedUserService* supervised_user_service =
-      SupervisedUserServiceFactory::GetForProfile(new_profile);
-
-  // Register the supervised user using the profile of the custodian.
-  supervised_user_registration_utility_ =
-      SupervisedUserRegistrationUtility::Create(Profile::FromWebUI(web_ui()));
-  supervised_user_service->RegisterAndInitSync(
-      supervised_user_registration_utility_.get(),
-      Profile::FromWebUI(web_ui()),
-      supervised_user_id,
-      base::Bind(&CreateProfileHandler::OnSupervisedUserRegistered,
-                 weak_ptr_factory_.GetWeakPtr(),
-                 create_shortcut,
-                 desktop_type,
-                 new_profile));
-}
-
-void CreateProfileHandler::OnSupervisedUserRegistered(
-    bool create_shortcut,
-    chrome::HostDesktopType desktop_type,
-    Profile* profile,
-    const GoogleServiceAuthError& error) {
-  GoogleServiceAuthError::State state = error.state();
-  RecordSupervisedProfileCreationMetrics(state);
-  if (state == GoogleServiceAuthError::NONE) {
-    CreateShortcutAndShowSuccess(create_shortcut, desktop_type, profile);
-    return;
-  }
-
-  base::string16 error_msg;
-  if (state == GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS ||
-      state == GoogleServiceAuthError::USER_NOT_SIGNED_UP ||
-      state == GoogleServiceAuthError::ACCOUNT_DELETED ||
-      state == GoogleServiceAuthError::ACCOUNT_DISABLED) {
-    error_msg = GetProfileCreationErrorMessage(SIGNIN_ERROR);
-  } else {
-    error_msg = GetProfileCreationErrorMessage(REMOTE_ERROR);
-  }
-  ShowProfileCreationError(profile, error_msg);
-}
-
 void CreateProfileHandler::CreateShortcutAndShowSuccess(
     bool create_shortcut,
     chrome::HostDesktopType desktop_type,
@@ -252,10 +218,12 @@ void CreateProfileHandler::CreateShortcutAndShowSuccess(
   dict.SetString("name",
                  profile->GetPrefs()->GetString(prefs::kProfileName));
   dict.Set("filePath", base::CreateFilePathValue(profile->GetPath()));
+#if defined(ENABLE_MANAGED_USERS)
   bool is_supervised =
       profile_creation_type_ == SUPERVISED_PROFILE_CREATION ||
       profile_creation_type_ == SUPERVISED_PROFILE_IMPORT;
   dict.SetBoolean("isManaged", is_supervised);
+#endif
   web_ui()->CallJavascriptFunction(
       GetJavascriptMethodName(PROFILE_CREATION_SUCCESS), dict);
 
@@ -264,7 +232,7 @@ void CreateProfileHandler::CreateShortcutAndShowSuccess(
   // dialog. If we are importing an existing supervised profile or creating a
   // new non-supervised user profile we don't show any confirmation, so open
   // the new window now.
-  if (profile_creation_type_ != SUPERVISED_PROFILE_CREATION) {
+  if (profile_creation_type_ == NON_SUPERVISED_PROFILE_CREATION) {

[Bernhard Bauer, 2014/07/14 16:39:06]

This doesn't quite match the old behavior for importing an existing supervised
user (see the comment).

[mckev, 2014/07/14 23:50:03]

Indeed.  My apologies for missing that.

     // Opening the new window must be the last action, after all callbacks
     // have been run, to give them a chance to initialize the profile.
     helper::OpenNewWindowForProfile(desktop_type,
@@ -286,13 +254,6 @@ void CreateProfileHandler::ShowProfileCreationError(
   helper::DeleteProfileAtPath(profile->GetPath(), web_ui());
 }
 
-void CreateProfileHandler::ShowProfileCreationWarning(
-    const base::string16& warning) {
-  DCHECK_EQ(SUPERVISED_PROFILE_CREATION, profile_creation_type_);
-  web_ui()->CallJavascriptFunction("BrowserOptions.showCreateProfileWarning",
-                                   base::StringValue(warning));
-}
-
 void CreateProfileHandler::HandleCancelProfileCreation(
     const base::ListValue* args) {
   CancelProfileRegistration(true);
@@ -307,12 +268,17 @@ void CreateProfileHandler::CancelProfileRegistration(bool user_initiated) {
   if (!new_profile)
     return;
 
+#if defined(ENABLE_MANAGED_USERS)
   // Non-supervised user creation cannot be canceled. (Creating a non-supervised
   // profile shouldn't take significant time, and it can easily be deleted
   // afterward.)
   if (!new_profile->IsSupervised())

[Bernhard Bauer, 2014/07/14 16:39:06]

This is also a slight change from the old behavior. If ENABLE_MANAGED_USERS is
not defined, no profile is supervised, so we should always return.

[mckev, 2014/07/14 23:50:03]

Agreed.  I added an early return at the beginning of the function in that case.

[Bernhard Bauer, 2014/07/16 09:54:00]

Would it make sense to put the whole method body into one big #ifdef? Then we
don't need the second one here.

     return;
 
+  DCHECK(supervised_user_registration_utility_.get());
+  supervised_user_registration_utility_.reset();
+#endif
+
   if (user_initiated) {
     UMA_HISTOGRAM_MEDIUM_TIMES(
         "Profile.CreateTimeCanceledNoTimeout",
@@ -320,9 +286,6 @@ void CreateProfileHandler::CancelProfileRegistration(bool user_initiated) {
     RecordProfileCreationMetrics(Profile::CREATE_STATUS_CANCELED);
   }
 
-  DCHECK(supervised_user_registration_utility_.get());
-  supervised_user_registration_utility_.reset();
-
   DCHECK_NE(NO_CREATION_IN_PROGRESS, profile_creation_type_);
   profile_creation_type_ = NO_CREATION_IN_PROGRESS;
 
@@ -342,6 +305,130 @@ void CreateProfileHandler::RecordProfileCreationMetrics(
       base::TimeTicks::Now() - profile_creation_start_time_);
 }
 
+base::string16 CreateProfileHandler::GetProfileCreationErrorMessage(

[Bernhard Bauer, 2014/07/14 16:39:06]

This whole method is only called for supervised user creation. I would restore
it to the old version and put it into the #ifdef as a whole.

[mckev, 2014/07/14 23:50:03]

It looks there is one exception - it's also called from OnProfileCreated -
GetProfileCreationErrorMessage(LOCAL_ERROR).  I think nesting switch statements
is less than ideal, but I can't think of a better way to consolidate this logic
completely or without duplicating a few more lines of code.  If you can suggest
something, I'll be happy to comply.

[Bernhard Bauer, 2014/07/16 09:54:00]

Hmm, looking at it in more detail, each of the error types is used exactly once,
as a constant argument to this method. We could split this up into three
methods, two of which only make sense for supervised users (so they could be
completely wrapped in #ifdef's), so only the third one would have an #ifdef in
the method body. WDYT?

+    ProfileCreationErrorType error) const {
+  int message_id = -1;
+  switch (profile_creation_type_) {
+#if defined(ENABLE_MANAGED_USERS)
+    case SUPERVISED_PROFILE_IMPORT:
+      switch (error) {
+        case SIGNIN_ERROR:
+          message_id = IDS_SUPERVISED_USER_IMPORT_SIGN_IN_ERROR;
+          break;
+        case REMOTE_ERROR:
+          message_id = IDS_SUPERVISED_USER_IMPORT_REMOTE_ERROR;
+          break;
+        case LOCAL_ERROR:
+          message_id = IDS_SUPERVISED_USER_IMPORT_LOCAL_ERROR;
+          break;
+        default:
+          NOTREACHED();
+      }
+    break;
+#endif
+    default:
+      switch (error) {
+        case SIGNIN_ERROR:
+          message_id = IDS_PROFILES_CREATE_SIGN_IN_ERROR;
+          break;
+        case REMOTE_ERROR:
+          message_id = IDS_PROFILES_CREATE_REMOTE_ERROR;
+          break;
+        case LOCAL_ERROR:
+          message_id = IDS_PROFILES_CREATE_LOCAL_ERROR;
+          break;
+        default:
+          NOTREACHED();
+      }
+  }
+
+  return l10n_util::GetStringUTF16(message_id);
+}
+
+std::string CreateProfileHandler::GetJavascriptMethodName(
+    ProfileCreationStatus status) const {
+  switch (profile_creation_type_) {
+#if defined(ENABLE_MANAGED_USERS)
+    case SUPERVISED_PROFILE_IMPORT:
+      switch (status) {
+        case PROFILE_CREATION_SUCCESS:
+          return "BrowserOptions.showManagedUserImportSuccess";
+        case PROFILE_CREATION_ERROR:
+          return "BrowserOptions.showManagedUserImportError";
+      }
+      break;
+#endif
+    default:
+      switch (status) {
+        case PROFILE_CREATION_SUCCESS:
+          return "BrowserOptions.showCreateProfileSuccess";
+        case PROFILE_CREATION_ERROR:
+          return "BrowserOptions.showCreateProfileError";
+      }
+      break;
+  }
+
+  NOTREACHED();
+  return std::string();
+}
+
+#if defined(ENABLE_MANAGED_USERS)
+void CreateProfileHandler::RegisterSupervisedUser(
+    bool create_shortcut,
+    chrome::HostDesktopType desktop_type,
+    const std::string& supervised_user_id,
+    Profile* new_profile) {
+  DCHECK_EQ(profile_path_being_created_.value(),
+            new_profile->GetPath().value());
+
+  SupervisedUserService* supervised_user_service =
+      SupervisedUserServiceFactory::GetForProfile(new_profile);
+
+  // Register the supervised user using the profile of the custodian.
+  supervised_user_registration_utility_ =
+      SupervisedUserRegistrationUtility::Create(Profile::FromWebUI(web_ui()));
+  supervised_user_service->RegisterAndInitSync(
+      supervised_user_registration_utility_.get(),
+      Profile::FromWebUI(web_ui()),
+      supervised_user_id,
+      base::Bind(&CreateProfileHandler::OnSupervisedUserRegistered,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 create_shortcut,
+                 desktop_type,
+                 new_profile));
+}
+
+void CreateProfileHandler::OnSupervisedUserRegistered(
+    bool create_shortcut,
+    chrome::HostDesktopType desktop_type,
+    Profile* profile,
+    const GoogleServiceAuthError& error) {
+  GoogleServiceAuthError::State state = error.state();
+  RecordSupervisedProfileCreationMetrics(state);
+  if (state == GoogleServiceAuthError::NONE) {
+    CreateShortcutAndShowSuccess(create_shortcut, desktop_type, profile);
+    return;
+  }
+
+  base::string16 error_msg;
+  if (state == GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS ||
+      state == GoogleServiceAuthError::USER_NOT_SIGNED_UP ||
+      state == GoogleServiceAuthError::ACCOUNT_DELETED ||
+      state == GoogleServiceAuthError::ACCOUNT_DISABLED) {
+    error_msg = GetProfileCreationErrorMessage(SIGNIN_ERROR);
+  } else {
+    error_msg = GetProfileCreationErrorMessage(REMOTE_ERROR);
+  }
+  ShowProfileCreationError(profile, error_msg);
+}
+
+void CreateProfileHandler::ShowProfileCreationWarning(
+    const base::string16& warning) {
+  DCHECK_EQ(SUPERVISED_PROFILE_CREATION, profile_creation_type_);
+  web_ui()->CallJavascriptFunction("BrowserOptions.showCreateProfileWarning",
+                                   base::StringValue(warning));
+}
+
 void CreateProfileHandler::RecordSupervisedProfileCreationMetrics(
     GoogleServiceAuthError::State error_state) {
   if (profile_creation_type_ == SUPERVISED_PROFILE_CREATION) {
@@ -362,50 +449,6 @@ void CreateProfileHandler::RecordSupervisedProfileCreationMetrics(
   }
 }
 
-base::string16 CreateProfileHandler::GetProfileCreationErrorMessage(
-    ProfileCreationErrorType error) const {
-  int message_id = -1;
-  switch (error) {
-    case SIGNIN_ERROR:
-      message_id =
-          profile_creation_type_ == SUPERVISED_PROFILE_IMPORT ?
-              IDS_SUPERVISED_USER_IMPORT_SIGN_IN_ERROR :
-              IDS_PROFILES_CREATE_SIGN_IN_ERROR;
-      break;
-    case REMOTE_ERROR:
-      message_id =
-          profile_creation_type_ == SUPERVISED_PROFILE_IMPORT ?
-              IDS_SUPERVISED_USER_IMPORT_REMOTE_ERROR :
-              IDS_PROFILES_CREATE_REMOTE_ERROR;
-      break;
-    case LOCAL_ERROR:
-      message_id =
-          profile_creation_type_ == SUPERVISED_PROFILE_IMPORT ?
-              IDS_SUPERVISED_USER_IMPORT_LOCAL_ERROR :
-              IDS_PROFILES_CREATE_LOCAL_ERROR;
-      break;
-  }
-
-  return l10n_util::GetStringUTF16(message_id);
-}
-
-std::string CreateProfileHandler::GetJavascriptMethodName(
-    ProfileCreationStatus status) const {
-  switch (status) {
-    case PROFILE_CREATION_SUCCESS:
-      return profile_creation_type_ == SUPERVISED_PROFILE_IMPORT ?
-          "BrowserOptions.showManagedUserImportSuccess" :
-          "BrowserOptions.showCreateProfileSuccess";
-    case PROFILE_CREATION_ERROR:
-      return profile_creation_type_ == SUPERVISED_PROFILE_IMPORT ?
-          "BrowserOptions.showManagedUserImportError" :
-          "BrowserOptions.showCreateProfileError";
-  }
-
-  NOTREACHED();
-  return std::string();
-}
-
 bool CreateProfileHandler::IsValidExistingSupervisedUserId(
     const std::string& existing_supervised_user_id) const {
   if (existing_supervised_user_id.empty())
@@ -428,5 +471,6 @@ bool CreateProfileHandler::IsValidExistingSupervisedUserId(
   }
   return true;
 }
+#endif
 
 }  // namespace options