Enable ImprovedGeoLanguageData by default.

Also adds doc and test around s2langquadtree's unique face constraint.

Bug: 850947
Change-Id: Ie8a48040bddeb011b59a8e3be230f1fdfa0d3406
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1553586
Commit-Queue: Alexandre Frechette <frechette@chromium.org>
Reviewed-by: anthonyvd <anthonyvd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#650443}

components/language/content/browser/geo_language_model_unittest.cc

@@ -10,6 +10,7 @@
 #include "base/timer/timer.h"
 #include "components/language/content/browser/geo_language_provider.h"
 #include "components/language/content/browser/test_utils.h"
+#include "components/language/content/browser/ulp_language_code_locator/ulp_language_code_locator.h"
 #include "components/prefs/testing_pref_service.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -42,6 +43,8 @@ class GeoLanguageModelTest : public testing::Test {
                             base::Unretained(&mock_ip_geo_location_provider_)));
     language::GeoLanguageProvider::RegisterLocalStatePrefs(
         local_state_.registry());
+    language::UlpLanguageCodeLocator::RegisterLocalStatePrefs(
+        local_state_.registry());
   }
 
  protected:
@@ -77,16 +80,14 @@ TEST_F(GeoLanguageModelTest, InsideIndia) {
   EXPECT_THAT(language_model()->GetLanguages(),
               testing::ElementsAre(
                   EqualsLd(LanguageModel::LanguageDetails("hi", 0.f)),
-                  EqualsLd(LanguageModel::LanguageDetails("mr", 0.f)),
-                  EqualsLd(LanguageModel::LanguageDetails("ur", 0.f))));
+                  EqualsLd(LanguageModel::LanguageDetails("en", 0.f))));
 }
 
 TEST_F(GeoLanguageModelTest, OutsideIndia) {
   // Setup a random place outside of India.
-  MoveToLocation(0.0, 0.0);
+  MoveToLocation(45.5, 73.5);
   StartGeoLanguageProvider();
   scoped_task_environment_.RunUntilIdle();
-
   EXPECT_EQ(0UL, language_model()->GetLanguages().size());
 }
 

components/language/content/browser/geo_language_provider_unittest.cc

@@ -93,7 +93,8 @@ class GeoLanguageProviderTest : public testing::Test {
   TestingPrefServiceSimple local_state_;
 };
 
-TEST_F(GeoLanguageProviderTest, GetCurrentGeoLanguages) {
+TEST_F(GeoLanguageProviderTest, GetCurrentGeoLanguages_OldGeo) {
+  scoped_feature_list_.InitAndDisableFeature(kImprovedGeoLanguageData);
   // Setup a random place in Madhya Pradesh, India.
   MoveToLocation(23.0, 80.0);
   StartGeoLanguageProvider();
@@ -106,8 +107,7 @@ TEST_F(GeoLanguageProviderTest, GetCurrentGeoLanguages) {
   EXPECT_EQ(expected_langs, GetCachedLanguages());
 }
 
-TEST_F(GeoLanguageProviderTest, GetCurrentGeoLanguagesImproved_India) {
-  scoped_feature_list_.InitAndEnableFeature(kImprovedGeoLanguageData);
+TEST_F(GeoLanguageProviderTest, GetCurrentGeoLanguages_India) {
   // Setup a random place in West Bengal, India.
   MoveToLocation(23.0, 85.7);
   StartGeoLanguageProvider();
@@ -117,8 +117,7 @@ TEST_F(GeoLanguageProviderTest, GetCurrentGeoLanguagesImproved_India) {
   EXPECT_EQ(expected_langs, GetCurrentGeoLanguages());
 }
 
-TEST_F(GeoLanguageProviderTest, GetCurrentGeoLanguagesImproved_OutsideIndia) {
-  scoped_feature_list_.InitAndEnableFeature(kImprovedGeoLanguageData);
+TEST_F(GeoLanguageProviderTest, GetCurrentGeoLanguages_OutsideIndia) {
   // Setup a random place in Montreal, Canada.
   MoveToLocation(45.5, 73.5);
   StartGeoLanguageProvider();
@@ -135,7 +134,7 @@ TEST_F(GeoLanguageProviderTest, NoFrequentCalls) {
   scoped_task_environment_.RunUntilIdle();
 
   const std::vector<std::string>& result = GetCurrentGeoLanguages();
-  std::vector<std::string> expected_langs = {"hi", "mr", "ur"};
+  std::vector<std::string> expected_langs = {"hi", "en"};
   EXPECT_EQ(expected_langs, result);
 
   scoped_task_environment_.FastForwardBy(base::TimeDelta::FromHours(12));
@@ -150,17 +149,17 @@ TEST_F(GeoLanguageProviderTest, ButDoCallInTheNextDay) {
   scoped_task_environment_.RunUntilIdle();
 
   std::vector<std::string> result = GetCurrentGeoLanguages();
-  std::vector<std::string> expected_langs = {"hi", "mr", "ur"};
+  std::vector<std::string> expected_langs = {"hi", "en"};
   EXPECT_EQ(expected_langs, result);
   EXPECT_EQ(expected_langs, GetCachedLanguages());
 
   // Move to another random place in Karnataka, India.
-  MoveToLocation(15.0, 75.0);
+  MoveToLocation(23.0, 85.7);
   scoped_task_environment_.FastForwardBy(base::TimeDelta::FromHours(25));
   EXPECT_EQ(2, GetQueryNextPositionCalledTimes());
 
   result = GetCurrentGeoLanguages();
-  std::vector<std::string> expected_langs_2 = {"kn", "ur", "te", "mr", "ta"};
+  std::vector<std::string> expected_langs_2 = {"hi", "bn", "en"};
   EXPECT_EQ(expected_langs_2, result);
   EXPECT_EQ(expected_langs_2, GetCachedLanguages());
 }
@@ -175,7 +174,7 @@ TEST_F(GeoLanguageProviderTest, CachedLanguagesPresent) {
 
   scoped_task_environment_.RunUntilIdle();
 
-  expected_langs = {"hi", "mr", "ur"};
+  expected_langs = {"hi", "en"};
   EXPECT_EQ(expected_langs, GetCurrentGeoLanguages());
   EXPECT_EQ(expected_langs, GetCachedLanguages());
 }

components/language/content/browser/ulp_language_code_locator/s2langquadtree.h

@@ -69,6 +69,8 @@ class S2LangQuadTreeNode {
   // Empty string if a null-leaf contains given |cell|.
   // |level_ptr| is set to the level (see S2CellId::level) of the leaf. (-1 if
   // |cell| matches an internal node).
+  // |cell|'s face needs to match the face of data used to create the tree;
+  // there is no check in place to verify that.
   std::string Get(const S2CellId& cell, int* level_ptr) const;
   std::string Get(const S2CellId& cell) const {
     int level;

components/language/content/browser/ulp_language_code_locator/s2langquadtree_datatest.cc

@@ -51,8 +51,15 @@ const std::map<S2LatLng, std::string> GetData(int rank) {
 
 void ExpectTreeContainsData(const S2LangQuadTreeNode& root,
                             const std::map<S2LatLng, std::string>& data) {
+  int face = -1;
   for (const auto& latlng_lang : data) {
     S2CellId cell(latlng_lang.first);
+
+    // All data is not on the same face, tree will fail.
+    if (face != -1)
+      EXPECT_EQ(face, cell.face());
+    face = cell.face();
+
     EXPECT_EQ(latlng_lang.second, root.Get(cell));
   }
 }

components/language/core/common/language_experiments.cc

@@ -20,7 +20,7 @@ const base::Feature kOverrideTranslateTriggerInIndia{
 const base::Feature kExplicitLanguageAsk{"ExplicitLanguageAsk",
                                          base::FEATURE_DISABLED_BY_DEFAULT};
 const base::Feature kImprovedGeoLanguageData{"ImprovedGeoLanguageData",
-                                             base::FEATURE_DISABLED_BY_DEFAULT};
+                                             base::FEATURE_ENABLED_BY_DEFAULT};
 const base::Feature kUseFluentLanguageModel{"UseFluentLanguageModel",
                                             base::FEATURE_DISABLED_BY_DEFAULT};