Reset geolocation permission embargo for an origin when it becomes DSE

chrome/browser/android/search_geolocation/search_geolocation_service_unittest.cc

Index: chrome/browser/android/search_geolocation/search_geolocation_service_unittest.cc
diff --git a/chrome/browser/android/search_geolocation/search_geolocation_service_unittest.cc b/chrome/browser/android/search_geolocation/search_geolocation_service_unittest.cc
index 1458315d9a2d189055568942a5fcdceece3d0a7f..7450e0b6dc32fa7bc57206e93e2740ad937695fd 100644
--- a/chrome/browser/android/search_geolocation/search_geolocation_service_unittest.cc
+++ b/chrome/browser/android/search_geolocation/search_geolocation_service_unittest.cc
@@ -13,6 +13,8 @@
 #include "base/test/scoped_feature_list.h"
 #include "chrome/browser/android/search_geolocation/search_geolocation_disclosure_tab_helper.h"
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
+#include "chrome/browser/permissions/permission_decision_auto_blocker.h"
+#include "chrome/browser/permissions/permission_result.h"
 #include "chrome/common/chrome_features.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/testing_profile.h"
@@ -374,3 +376,31 @@ TEST_F(SearchGeolocationServiceTest, DSEChangesAndDisclosure) {
   EXPECT_TRUE(SearchGeolocationDisclosureTabHelper::IsDisclosureResetForTests(
       profile()));
 }
+
+TEST_F(SearchGeolocationServiceTest, Embargo) {
+  test_delegate()->SetDSEOrigin(kGoogleURL);
+
+  // Place another origin under embargo.
+  GURL google_aus_url(kGoogleAusURL);
+  PermissionDecisionAutoBlocker* auto_blocker =
+      PermissionDecisionAutoBlocker::GetForProfile(profile());
+  auto_blocker->RecordDismissAndEmbargo(google_aus_url,
+                                        CONTENT_SETTINGS_TYPE_GEOLOCATION);
+  auto_blocker->RecordDismissAndEmbargo(google_aus_url,
+                                        CONTENT_SETTINGS_TYPE_GEOLOCATION);
+  auto_blocker->RecordDismissAndEmbargo(google_aus_url,
+                                        CONTENT_SETTINGS_TYPE_GEOLOCATION);
+  EXPECT_EQ(auto_blocker

[dominickn, 2017/06/15 00:34:53]

It might be more robust to check the embargo state via
PermissionManager::GetPermissionStatus since that's the primary external
interface.

[benwells, 2017/06/15 04:00:40]

As discussed it makes the tests not work, as the search geo stuff hooks in too
soon for embargo to take effect.

+                ->GetEmbargoResult(GURL(kGoogleAusURL),
+                                   CONTENT_SETTINGS_TYPE_GEOLOCATION)
+                .source,
+            PermissionStatusSource::MULTIPLE_DISMISSALS);

[dominickn, 2017/06/15 00:34:53]

You should explicitly check the PermissionResult::content_setting is
CONTENT_SETTING_BLOCK rather than the source (which is really just for UI
purposes).

[benwells, 2017/06/15 04:00:40]

Checked as well.

+
+  // Now change the DSE to this origin and make sure the embargo is cleared.
+  test_delegate()->SetDSEOrigin(kGoogleAusURL);
+  EXPECT_EQ(auto_blocker
+                ->GetEmbargoResult(GURL(kGoogleAusURL),
+                                   CONTENT_SETTINGS_TYPE_GEOLOCATION)
+                .source,

[dominickn, 2017/06/15 00:34:53]

You should explicitly check the PermissionResult::content_setting is
CONTENT_SETTING_ASK.

[benwells, 2017/06/15 04:00:40]

Checked as well.

+            PermissionStatusSource::UNSPECIFIED);
+}