Make geolocation API and X-Geo header access consistent

chrome/browser/content_settings/host_content_settings_map_unittest.cc

Index: chrome/browser/content_settings/host_content_settings_map_unittest.cc
diff --git a/chrome/browser/content_settings/host_content_settings_map_unittest.cc b/chrome/browser/content_settings/host_content_settings_map_unittest.cc
index e8fd7c404340c4691b49ef90e9d2c0c4868c8255..da1ae3840cf622833c1e0dd0cb708b72378ffaa2 100644
--- a/chrome/browser/content_settings/host_content_settings_map_unittest.cc
+++ b/chrome/browser/content_settings/host_content_settings_map_unittest.cc
@@ -23,6 +23,7 @@
 #include "components/content_settings/core/browser/website_settings_info.h"
 #include "components/content_settings/core/browser/website_settings_registry.h"
 #include "components/content_settings/core/common/pref_names.h"
+#include "components/policy/core/common/policy_service.h"
 #include "components/prefs/pref_service.h"
 #include "components/prefs/scoped_user_pref_update.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
@@ -32,6 +33,10 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+#include "chrome/browser/extensions/extension_special_storage_policy.h"
+#endif

[raymes, 2017/01/11 08:30:12]

Are these 2 includes above needed?

[benwells, 2017/01/11 09:26:11]

Yes, they are needed because two of the nullptrs passed into TestingProfile
constructor are smart pointers. These can't be constructed unless they know the
size of the type etc.

+
 using content::BrowserThread;
 
 using ::testing::_;
@@ -1251,8 +1256,13 @@ TEST_F(HostContentSettingsMapTest, AddContentSettingsObserver) {
 }
 
 TEST_F(HostContentSettingsMapTest, GuestProfile) {

[msramek, 2017/01/11 09:51:10]

The guest profile is Desktop only (unless I'm really missing something).

I haven't looked deeper into Profile and TestingProfile, but my hunch is that
the previous code SetGuestSession(true) was somehow interpreted as incognito on
Android and so nothing was written to the disk, while your new code is
interpreted as regular mode and thus we do write to the disk.

In that sense, disabling this test on Android would actually be the right thing
to do.

-  TestingProfile profile;
-  profile.SetGuestSession(true);
+  TestingProfile profile(base::FilePath(),
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+                         nullptr,
+#endif
+                         nullptr, nullptr, nullptr, true /* guest_profile */,
+                         "", nullptr, TestingProfile::TestingFactories(),

[raymes, 2017/01/11 08:30:12]

nit: std::string() for empty strings

+                         "testing_profile");
   HostContentSettingsMap* host_content_settings_map =
       HostContentSettingsMapFactory::GetForProfile(&profile);
 
@@ -1282,8 +1292,13 @@ TEST_F(HostContentSettingsMapTest, GuestProfile) {
 // Default settings should not be modifiable for the guest profile (there is no
 // UI to do this).
 TEST_F(HostContentSettingsMapTest, GuestProfileDefaultSetting) {
-  TestingProfile profile;
-  profile.SetGuestSession(true);
+  TestingProfile profile(base::FilePath(),
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+                         nullptr,
+#endif
+                         nullptr, nullptr, nullptr, true /* guest_profile */,
+                         "", nullptr, TestingProfile::TestingFactories(),
+                         "testing_profile");
   HostContentSettingsMap* host_content_settings_map =
       HostContentSettingsMapFactory::GetForProfile(&profile);
 
@@ -1305,8 +1320,13 @@ TEST_F(HostContentSettingsMapTest, GuestProfileDefaultSetting) {
 // We used to incorrectly store content settings in prefs for the guest profile.
 // We need to ensure these get deleted appropriately.
 TEST_F(HostContentSettingsMapTest, GuestProfileMigration) {
-  TestingProfile profile;
-  profile.SetGuestSession(true);
+  TestingProfile profile(base::FilePath(),
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+                         nullptr,
+#endif
+                         nullptr, nullptr, nullptr, true /* guest_profile */,
+                         "", nullptr, TestingProfile::TestingFactories(),
+                         "testing_profile");
 
   // Set a pref manually in the guest profile.
   std::unique_ptr<base::Value> value =
@@ -1497,6 +1517,13 @@ TEST_F(HostContentSettingsMapTest, MigrateDomainScopedSettings) {
 // once after syncing (even when these events occur multiple times).
 TEST_F(HostContentSettingsMapTest, DomainToOriginMigrationStatus) {
   TestingProfile profile;
+
+  // Construct the map now to make the various platforms equivalent. On Android
+  // the map is created with the profile (due to dependencies), while on other
+  // platform it is created lazily. Note, migration should be run here.

[raymes, 2017/01/11 08:30:12]

nit: platforms

[benwells, 2017/01/12 03:10:43]

Done.

+  HostContentSettingsMap* host_content_settings_map =
+      HostContentSettingsMapFactory::GetForProfile(&profile);
+
   PrefService* prefs = profile.GetPrefs();
 
   GURL http_host("http://example.com/");
@@ -1522,13 +1549,38 @@ TEST_F(HostContentSettingsMapTest, DomainToOriginMigrationStatus) {
   EXPECT_TRUE(all_settings_dictionary->GetDictionaryWithoutPathExpansion(
       "[*.]example.com,*", &result));
 
-  // Migration is done on construction of HostContentSettingsMap.
-  HostContentSettingsMap* host_content_settings_map =
-      HostContentSettingsMapFactory::GetForProfile(&profile);
+  // Migration is done on construction of HostContentSettingsMap. True to run it

[raymes, 2017/01/11 08:30:12]

nit: True->Try

[benwells, 2017/01/12 03:10:43]

Done.

+  // again. This should not do anything as it has already ran.
+  host_content_settings_map->MigrateDomainScopedSettings(
+      false /* after_sync */);
 
   // Change default setting to BLOCK.
   host_content_settings_map->SetDefaultContentSetting(
       CONTENT_SETTINGS_TYPE_POPUPS, CONTENT_SETTING_BLOCK);
+
+  EXPECT_EQ(
+      CONTENT_SETTING_ALLOW,
+      host_content_settings_map->GetContentSetting(
+          http_host, http_host, CONTENT_SETTINGS_TYPE_POPUPS, std::string()));
+
+  // The setting should still be allow as it hasn't been migrated.
+  EXPECT_EQ(CONTENT_SETTING_ALLOW,
+            host_content_settings_map->GetContentSetting(
+                http_host_narrower, http_host_narrower,
+                CONTENT_SETTINGS_TYPE_POPUPS, std::string()));
+
+  // Set the pref to its initial state so that migration can be triggered again,
+  // just for the sake of testing.
+  int default_value;
+  prefs->GetDefaultPrefValue(prefs::kDomainToOriginMigrationStatus)
+      ->GetAsInteger(&default_value);
+  prefs->SetInteger(prefs::kDomainToOriginMigrationStatus, default_value);
+
+  // Now, do the migration. This should work as we've cleared the pref back to
+  // its default value.
+  host_content_settings_map->MigrateDomainScopedSettings(false);
+
+  // Now the settings should be migrated.
   EXPECT_EQ(
       CONTENT_SETTING_ALLOW,
       host_content_settings_map->GetContentSetting(
@@ -1546,6 +1598,7 @@ TEST_F(HostContentSettingsMapTest, DomainToOriginMigrationStatus) {
       ContentSettingsPattern::FromURL(https_host),
       ContentSettingsPattern::Wildcard(), CONTENT_SETTINGS_TYPE_POPUPS,
       std::string(), CONTENT_SETTING_ALLOW);
+
   EXPECT_EQ(
       CONTENT_SETTING_ALLOW,
       host_content_settings_map->GetContentSetting(