Add account_type into AccountId

components/signin/core/account_id/account_id.cc

Index: components/signin/core/account_id/account_id.cc
diff --git a/components/signin/core/account_id/account_id.cc b/components/signin/core/account_id/account_id.cc
index 00ceb2e2f52bf9f5225ca76c4af50f99e0f58105..973112ed605ef12600148352cd6112b80ae457cf 100644
--- a/components/signin/core/account_id/account_id.cc
+++ b/components/signin/core/account_id/account_id.cc
@@ -16,25 +16,18 @@
 
 namespace {
 
-// Known account types.
-const char kGoogle[] = "google";
-
 // Serialization keys
 const char kGaiaIdKey[] = "gaia_id";
 const char kEmailKey[] = "email";
+const char kAccountTypeKey[] = "account_type";
+
+// Serialization values for account type.
+const std::string kGoogle = "google";
+const std::string kAd = "ad";
 
 // Prefix for GetAccountIdKey().
 const char kKeyGaiaIdPrefix[] = "g-";
-
-struct GoogleStringSingleton {
-  GoogleStringSingleton() : google(kGoogle) {}
-
-  static GoogleStringSingleton* GetInstance() {
-    return base::Singleton<GoogleStringSingleton>::get();
-  }
-
-  const std::string google;
-};
+const char kKeyAdIdPrefix[] = "a-";
 
 }  // anonymous namespace
 
@@ -61,8 +54,17 @@ AccountId::AccountId(const std::string& gaia_id, const std::string& user_email)
   // TODO(alemate): check gaia_id is not empty once it is required.
 }
 
+AccountId::AccountId(const std::string& gaia_id,
+                     const std::string& user_email,
+                     const AccountType& account_type)
+    : gaia_id_(gaia_id), user_email_(user_email), account_type_(account_type) {
+  DCHECK(account_type != AccountType::UNKNOWN);
+}
+
 AccountId::AccountId(const AccountId& other)
-    : gaia_id_(other.gaia_id_), user_email_(other.user_email_) {}
+    : gaia_id_(other.gaia_id_),
+      user_email_(other.user_email_),
+      account_type_(other.account_type_) {}
 
 bool AccountId::operator==(const AccountId& other) const {
   return (this == &other) ||
@@ -93,8 +95,8 @@ void AccountId::clear() {
   user_email_.clear();
 }
 
-const std::string& AccountId::GetAccountType() const {
-  return GoogleStringSingleton::GetInstance()->google;
+const AccountType& AccountId::GetAccountType() const {
+  return account_type_;
 }
 
 const std::string& AccountId::GetGaiaId() const {
@@ -114,7 +116,12 @@ const std::string AccountId::GetAccountIdKey() const {
   CHECK(!gaia_id_.empty());
 #endif
 
-  return std::string(kKeyGaiaIdPrefix) + gaia_id_;
+  if (GetAccountType() == AccountType::GOOGLE)
+    return std::string(kKeyGaiaIdPrefix) + gaia_id_;
+  if (GetAccountType() == AccountType::ACTIVE_DIRECTORY)
+    return std::string(kKeyAdIdPrefix) + gaia_id_;

[Roger Tawa OOO till Jul 10th, 2016/12/05 16:34:31]

Will there be a later pass to change |gaia_id_| to something more generic?

[Alexander Alekseev, 2016/12/06 02:21:14]

Uuuups, I missed this. Thank you.

I thought of having other account types extend AccountId and make particular
ones have only special "account-specific" fiedls.

For now I think we should add another member like "string uuid_" (or what it
actually is), and check that AD accounts have empty gaia_id_ .

WDYT?

[Roger Tawa OOO till Jul 10th, 2016/12/07 22:09:37]

Since |gaia_id_| is a private member, seems easier to just rename it to like
|id_| and then store the appropriate id there.  Not sure adding a new member is
easier.

+  NOTREACHED() << "Unknown account type";
+  return std::string();

[Roger Tawa OOO till Jul 10th, 2016/12/05 16:34:31]

Nit: use a switch() statement.  Also line 173.

[Roman Sorokin (ftl), 2016/12/13 13:45:59]

Done.

 }
 
 void AccountId::SetGaiaId(const std::string& gaia_id) {
@@ -127,6 +134,11 @@ void AccountId::SetUserEmail(const std::string& email) {
   user_email_ = email;
 }
 
+void AccountId::SetAccountType(const AccountType& account_type) {
+  DCHECK(account_type != AccountType::UNKNOWN);
+  account_type_ = account_type;
+}
+
 // static
 AccountId AccountId::FromUserEmail(const std::string& email) {
   // TODO(alemate): DCHECK(!email.empty());
@@ -145,10 +157,46 @@ AccountId AccountId::FromUserEmailGaiaId(const std::string& email,
   return AccountId(gaia_id, email);
 }
 
+// static
+AccountType AccountId::StringToAccountType(
+    const std::string& account_type_string) {
+  if (account_type_string == kGoogle)
+    return AccountType::GOOGLE;
+  if (account_type_string == kAd)
+    return AccountType::ACTIVE_DIRECTORY;
+  NOTREACHED() << "Unknown account type " << account_type_string;
+  return AccountType::UNKNOWN;
+}
+
+// static
+std::string AccountId::AccountTypeToString(const AccountType& account_type) {
+  if (account_type == AccountType::GOOGLE)
+    return kGoogle;
+  if (account_type == AccountType::ACTIVE_DIRECTORY)
+    return kAd;
+  NOTREACHED() << "Unknown account type";
+  return std::string();
+}
+
+AccountId AccountId::FromUserEmailGaiaIdAccountType(
+    const std::string& email,
+    const std::string& gaia_id,
+    const AccountType& account_type) {
+  DCHECK(!(email.empty() && gaia_id.empty()));
+  return AccountId(gaia_id, email, account_type);
+}
+
+AccountId AccountId::FromGaiaIdAccountType(const std::string& gaia_id,
+                                           const AccountType& account_type) {
+  return FromUserEmailGaiaIdAccountType(std::string() /* email */, gaia_id,
+                                        account_type);
+}

[Roger Tawa OOO till Jul 10th, 2016/12/05 16:34:31]

Since these are new methods, maybe not use "Gaia" in the name?  So something
like:

FromUserEmailIdAccountType()
FromIdAccountType()

[Alexander Alekseev, 2016/12/06 02:21:14]

Or, probably, we should create AccountId::ADFromUserEmailUUID(), and
AccountId::GoogleFromUserEmailGaiaId(). WDYT?

To somplify this Cl we can add AccountId::ADFromUserEmailUUID() and TODO at
AccountId::FromUserEmailGaiaId() to rename it to
AccountId::GoogleFromUserEmailGaiaId() .

[Roger Tawa OOO till Jul 10th, 2016/12/07 22:09:37]

Acknowledged.

+

[Roger Tawa OOO till Jul 10th, 2016/12/05 16:34:31]

These two new methods should have "// static" comment.

[Roman Sorokin (ftl), 2016/12/13 13:45:59]

Done.

 std::string AccountId::Serialize() const {
   base::DictionaryValue value;
   value.SetString(kGaiaIdKey, gaia_id_);
   value.SetString(kEmailKey, user_email_);
+  value.SetString(kAccountTypeKey, AccountTypeToString(account_type_));
 
   std::string serialized;
   base::JSONWriter::Write(value, &serialized);
@@ -167,6 +215,7 @@ bool AccountId::Deserialize(const std::string& serialized,
 
   std::string gaia_id;
   std::string user_email;
+  AccountType account_type = AccountType::GOOGLE;
 
   const bool found_gaia_id = dictionary_value->GetString(kGaiaIdKey, &gaia_id);
   const bool found_user_email =
@@ -181,7 +230,12 @@ bool AccountId::Deserialize(const std::string& serialized,
   if (!found_gaia_id && !found_user_email)
     return false;
 
-  *account_id = FromUserEmailGaiaId(user_email, gaia_id);
+  std::string account_type_string;
+  if (dictionary_value->GetString(kAccountTypeKey, &account_type_string))
+    account_type = StringToAccountType(account_type_string);

[Roger Tawa OOO till Jul 10th, 2016/12/05 16:34:31]

I think you should do some error checking here since the serialized source is
external.  Probably OK not to do error checking at line 199 above.

[Alexander Alekseev, 2016/12/06 02:21:14]

Totally agree.

[Roman Sorokin (ftl), 2016/12/13 13:45:58]

done

+
+  *account_id =
+      FromUserEmailGaiaIdAccountType(user_email, gaia_id, account_type);
 
   return true;
 }