Record the application locale in local consents.

This should not change during Chrome runtime, and thus is passed
directly from the factory, similarly as we do for product version.

Since we use the term "app locale", for consistency we also rename
"product version" into "app version".

Finally, as this addition caused even more repetition in tests, some
common functionality has been extracted, and on the way, the ordering
of expectations fixed.

BASE: 731423
Bug: 781765
Change-Id: Id8093692a0cadd0d3ac127266dc9a814e780f602
Reviewed-on: https://chromium-review.googlesource.com/763510
Commit-Queue: Martin Šrámek <msramek@chromium.org>
Reviewed-by: Christian Dullweber <dullweber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515940}

chrome/browser/consent_auditor/consent_auditor_factory.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/consent_auditor/consent_auditor_factory.h"
 
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sync/user_event_service_factory.h"
 #include "components/consent_auditor/consent_auditor.h"
@@ -45,7 +46,10 @@ KeyedService* ConsentAuditorFactory::BuildServiceInstanceFor(
   return new consent_auditor::ConsentAuditor(
       profile->GetPrefs(),
       browser_sync::UserEventServiceFactory::GetForProfile(profile),
-      version_info::GetVersionNumber());
+      // The browser version and locale do not change runtime, so we can pass
+      // them directly.
+      version_info::GetVersionNumber(),
+      g_browser_process->GetApplicationLocale());
 }
 
 // static

components/consent_auditor/consent_auditor.cc

@@ -20,16 +20,19 @@ namespace {
 
 const char kLocalConsentDescriptionKey[] = "description";
 const char kLocalConsentConfirmationKey[] = "confirmation";
-const char kLocalConsentVersionInfoKey[] = "version";
+const char kLocalConsentVersionKey[] = "version";
+const char kLocalConsentLocaleKey[] = "locale";
 
 }  // namespace
 
 ConsentAuditor::ConsentAuditor(PrefService* pref_service,
                                syncer::UserEventService* user_event_service,
-                               const std::string& product_version)
+                               const std::string& app_version,
+                               const std::string& app_locale)
     : pref_service_(pref_service),
       user_event_service_(user_event_service),
-      product_version_(product_version) {
+      app_version_(app_version),
+      app_locale_(app_locale) {
   DCHECK(pref_service_);
   DCHECK(user_event_service_);
 }
@@ -56,9 +59,8 @@ void ConsentAuditor::RecordLocalConsent(const std::string& feature,
   base::DictionaryValue record;
   record.SetKey(kLocalConsentDescriptionKey, base::Value(description_text));
   record.SetKey(kLocalConsentConfirmationKey, base::Value(confirmation_text));
-  record.SetKey(kLocalConsentVersionInfoKey, base::Value(product_version_));
-
-  // TODO(msramek): Record the user language. crbug.com/781765
+  record.SetKey(kLocalConsentVersionKey, base::Value(app_version_));
+  record.SetKey(kLocalConsentLocaleKey, base::Value(app_locale_));
 
   consents->SetKey(feature, std::move(record));
 }

components/consent_auditor/consent_auditor.h

@@ -23,7 +23,8 @@ class ConsentAuditor : public KeyedService {
  public:
   ConsentAuditor(PrefService* pref_service,
                  syncer::UserEventService* user_event_service,
-                 const std::string& product_version);
+                 const std::string& app_version,
+                 const std::string& app_locale);
   ~ConsentAuditor() override;
 
   // KeyedService:
@@ -43,7 +44,8 @@ class ConsentAuditor : public KeyedService {
  private:
   PrefService* pref_service_;
   syncer::UserEventService* user_event_service_;
-  std::string product_version_;
+  std::string app_version_;
+  std::string app_locale_;
 
   DISALLOW_COPY_AND_ASSIGN(ConsentAuditor);
 };

components/consent_auditor/consent_auditor_unittest.cc

@@ -17,10 +17,46 @@ namespace {
 
 const char kLocalConsentDescriptionKey[] = "description";
 const char kLocalConsentConfirmationKey[] = "confirmation";
-const char kLocalConsentVersionInfoKey[] = "version";
+const char kLocalConsentVersionKey[] = "version";
+const char kLocalConsentLocaleKey[] = "locale";
 
 // Fake product version for testing.
-const char kCurrentProductVersion[] = "1.2.3.4";
+const char kCurrentAppVersion[] = "1.2.3.4";
+const char kCurrentAppLocale[] = "en-US";
+
+// A helper function to load the |description|, |confirmation|, |version|,
+// and |locale|, in that order, from a record for the |feature| in
+// the |consents| dictionary.
+void LoadEntriesFromLocalConsentRecord(const base::Value* consents,
+                                       const std::string& feature,
+                                       std::string* description,
+                                       std::string* confirmation,
+                                       std::string* version,
+                                       std::string* locale) {
+  SCOPED_TRACE(::testing::Message() << "|feature| = " << feature);
+
+  const base::Value* record =
+      consents->FindKeyOfType(feature, base::Value::Type::DICTIONARY);
+  ASSERT_TRUE(record);
+  SCOPED_TRACE(::testing::Message() << "|record| = " << record);
+
+  const base::Value* description_entry =
+      record->FindKey(kLocalConsentDescriptionKey);
+  const base::Value* confirmation_entry =
+      record->FindKey(kLocalConsentConfirmationKey);
+  const base::Value* version_entry = record->FindKey(kLocalConsentVersionKey);
+  const base::Value* locale_entry = record->FindKey(kLocalConsentLocaleKey);
+
+  ASSERT_TRUE(description_entry);
+  ASSERT_TRUE(confirmation_entry);
+  ASSERT_TRUE(version_entry);
+  ASSERT_TRUE(locale_entry);
+
+  *description = description_entry->GetString();
+  *confirmation = confirmation_entry->GetString();
+  *version = version_entry->GetString();
+  *locale = locale_entry->GetString();
+}
 
 }  // namespace
 
@@ -31,15 +67,18 @@ class ConsentAuditorTest : public testing::Test {
     user_event_service_ = base::MakeUnique<syncer::FakeUserEventService>();
     ConsentAuditor::RegisterProfilePrefs(pref_service_->registry());
     consent_auditor_ = base::MakeUnique<ConsentAuditor>(
-        pref_service_.get(), user_event_service_.get(), kCurrentProductVersion);
+        pref_service_.get(), user_event_service_.get(), kCurrentAppVersion,
+        kCurrentAppLocale);
   }
 
-  void UpdateProductVersion(const std::string& new_product_version) {
-    // We'll have to recreate |consent_auditor| in order to update
-    // a new version. This is not a problem, as in reality we'd have to restart
-    // Chrome to update the version, let alone just recreate this class.
+  void UpdateAppVersionAndLocale(const std::string& new_product_version,
+                                 const std::string& new_app_locale) {
+    // We'll have to recreate |consent_auditor| in order to update the version
+    // and locale. This is not a problem, as in reality we'd have to restart
+    // Chrome to update both, let alone just recreate this class.
     consent_auditor_ = base::MakeUnique<ConsentAuditor>(
-        pref_service_.get(), user_event_service_.get(), new_product_version);
+        pref_service_.get(), user_event_service_.get(), new_product_version,
+        new_app_locale);
   }
 
   ConsentAuditor* consent_auditor() { return consent_auditor_.get(); }
@@ -65,60 +104,49 @@ TEST_F(ConsentAuditorTest, LocalConsentPrefRepresentation) {
   const base::DictionaryValue* consents =
       pref_service()->GetDictionary(prefs::kLocalConsentsDictionary);
   ASSERT_TRUE(consents);
-  const base::Value* record =
-      consents->FindKeyOfType("feature1", base::Value::Type::DICTIONARY);
-  ASSERT_TRUE(record);
-  const base::Value* description = record->FindKey(kLocalConsentDescriptionKey);
-  const base::Value* confirmation =
-      record->FindKey(kLocalConsentConfirmationKey);
-  const base::Value* version = record->FindKey(kLocalConsentVersionInfoKey);
-  ASSERT_TRUE(description);
-  ASSERT_TRUE(confirmation);
-  ASSERT_TRUE(version);
-  EXPECT_EQ(description->GetString(), kFeature1Description);
-  EXPECT_EQ(confirmation->GetString(), kFeature1Confirmation);
-  EXPECT_EQ(version->GetString(), kCurrentProductVersion);
+  std::string description;
+  std::string confirmation;
+  std::string version;
+  std::string locale;
+  LoadEntriesFromLocalConsentRecord(consents, "feature1", &description,
+                                    &confirmation, &version, &locale);
+  EXPECT_EQ(kFeature1Description, description);
+  EXPECT_EQ(kFeature1Confirmation, confirmation);
+  EXPECT_EQ(kCurrentAppVersion, version);
+  EXPECT_EQ(kCurrentAppLocale, locale);
 
   // Do the same for another feature.
   const std::string kFeature2Description = "Enable feature 2?";
   const std::string kFeature2Confirmation = "Yes.";
   consent_auditor()->RecordLocalConsent("feature2", kFeature2Description,
                                         kFeature2Confirmation);
-  record = consents->FindKeyOfType("feature2", base::Value::Type::DICTIONARY);
-  ASSERT_TRUE(record);
-  description = record->FindKey(kLocalConsentDescriptionKey);
-  confirmation = record->FindKey(kLocalConsentConfirmationKey);
-  version = record->FindKey(kLocalConsentVersionInfoKey);
-  ASSERT_TRUE(description);
-  ASSERT_TRUE(confirmation);
-  ASSERT_TRUE(version);
-  EXPECT_EQ(description->GetString(), kFeature2Description);
-  EXPECT_EQ(confirmation->GetString(), kFeature2Confirmation);
-  EXPECT_EQ(version->GetString(), kCurrentProductVersion);
+  LoadEntriesFromLocalConsentRecord(consents, "feature2", &description,
+                                    &confirmation, &version, &locale);
+  EXPECT_EQ(kFeature2Description, description);
+  EXPECT_EQ(kFeature2Confirmation, confirmation);
+  EXPECT_EQ(kCurrentAppVersion, version);
+  EXPECT_EQ(kCurrentAppLocale, locale);
 
   // They are two separate records; the latter did not overwrite the former.
   EXPECT_EQ(2u, consents->size());
   EXPECT_TRUE(
       consents->FindKeyOfType("feature1", base::Value::Type::DICTIONARY));
 
-  // Overwrite an existing consent and use a different product version.
+  // Overwrite an existing consent, this time use a different product version
+  // and a different locale.
   const std::string kFeature2NewDescription = "Re-enable feature 2?";
   const std::string kFeature2NewConfirmation = "Yes again.";
-  const std::string kFeature2NewProductVersion = "5.6.7.8";
-  UpdateProductVersion(kFeature2NewProductVersion);
+  const std::string kFeature2NewAppVersion = "5.6.7.8";
+  const std::string kFeature2NewAppLocale = "de";
+  UpdateAppVersionAndLocale(kFeature2NewAppVersion, kFeature2NewAppLocale);
   consent_auditor()->RecordLocalConsent("feature2", kFeature2NewDescription,
                                         kFeature2NewConfirmation);
-  record = consents->FindKeyOfType("feature2", base::Value::Type::DICTIONARY);
-  ASSERT_TRUE(record);
-  description = record->FindKey(kLocalConsentDescriptionKey);
-  confirmation = record->FindKey(kLocalConsentConfirmationKey);
-  version = record->FindKey(kLocalConsentVersionInfoKey);
-  ASSERT_TRUE(description);
-  ASSERT_TRUE(confirmation);
-  ASSERT_TRUE(version);
-  EXPECT_EQ(description->GetString(), kFeature2NewDescription);
-  EXPECT_EQ(confirmation->GetString(), kFeature2NewConfirmation);
-  EXPECT_EQ(version->GetString(), kFeature2NewProductVersion);
+  LoadEntriesFromLocalConsentRecord(consents, "feature2", &description,
+                                    &confirmation, &version, &locale);
+  EXPECT_EQ(kFeature2NewDescription, description);
+  EXPECT_EQ(kFeature2NewConfirmation, confirmation);
+  EXPECT_EQ(kFeature2NewAppVersion, version);
+  EXPECT_EQ(kFeature2NewAppLocale, locale);
 
   // We still have two records.
   EXPECT_EQ(2u, consents->size());