Add a feature-controlled persistence checkbox to geolocation prompts on desktop Views platforms.

chrome/browser/permissions/permission_context_base_unittest.cc

Index: chrome/browser/permissions/permission_context_base_unittest.cc
diff --git a/chrome/browser/permissions/permission_context_base_unittest.cc b/chrome/browser/permissions/permission_context_base_unittest.cc
index d446c10f307023c86741e84be3ae0859c4883012..56032d00d57caa6e8337705d33ddc03ea5f913de 100644
--- a/chrome/browser/permissions/permission_context_base_unittest.cc
+++ b/chrome/browser/permissions/permission_context_base_unittest.cc
@@ -128,23 +128,23 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {
   void RespondToPermission(TestPermissionContext* context,
                            const PermissionRequestID& id,
                            const GURL& url,
+                           bool persist,
                            ContentSetting response) {
     DCHECK(response == CONTENT_SETTING_ALLOW ||
            response == CONTENT_SETTING_BLOCK ||
            response == CONTENT_SETTING_ASK);
 #if defined(OS_ANDROID)
-    bool update_content_setting = response != CONTENT_SETTING_ASK;
     PermissionAction decision = DISMISSED;
     if (response == CONTENT_SETTING_ALLOW)
       decision = GRANTED;
     else if (response == CONTENT_SETTING_BLOCK)
       decision = DENIED;
     context->GetInfoBarController()->OnPermissionSet(
-        id, url, url, false /* user_gesture */, update_content_setting,
-        decision);
+        id, url, url, false /* user_gesture */, persist, decision);
 #else
     PermissionRequestManager* manager =
         PermissionRequestManager::FromWebContents(web_contents());
+    manager->TogglePersist(persist);
     switch (response) {
       case CONTENT_SETTING_ALLOW:
         manager->Accept();
@@ -161,11 +161,13 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {
 #endif
   }
 
-  void TestAskAndGrant_TestContent() {
-    TestPermissionContext permission_context(
-        profile(), content::PermissionType::NOTIFICATIONS,
-        CONTENT_SETTINGS_TYPE_NOTIFICATIONS);
-    GURL url("http://www.google.com");
+  void TestAskAndDecide_TestContent(content::PermissionType permission,

[raymes, 2016/08/24 04:00:17]

I'm a bit concerned that these test functions will become a mess with new checks
which conflate different things that are being tested, e.g. this already tests
-what the result of the request is
-what gets stored
-what histogram values get set (I think this is going to be more complicated
with https://codereview.chromium.org/2250993002 applied, right?)

Personally I think the histogram tests fit better as separate tests, for
example. A good general pattern is to manipulate the test state and then check
the test expectations, which for histograms would look like:
-Request permission and Accept/Deny/Ignore/etc
-Check histogram
-Repeat as needed

At the very least let's keep this in mind as we do more work here.

[dominickn, 2016/08/24 06:30:57]

Acknowledged. I have a plan for improving these tests, splitting out the
histogram testing (and adding new histogram tests for all of the permission
metrics, which have never been tested before). If it's okay with you, I'd like
to do that in separate CLs. :)

+                                    ContentSettingsType content_settings_type,
+                                    ContentSetting decision,
+                                    bool persist) {
+    TestPermissionContext permission_context(profile(), permission,
+                                             content_settings_type);
+    GURL url("https://www.google.com");
     NavigateAndCommit(url);
 
     const PermissionRequestID id(
@@ -178,12 +180,17 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {
         base::Bind(&TestPermissionContext::TrackPermissionDecision,
                    base::Unretained(&permission_context)));
 
-    RespondToPermission(&permission_context, id, url, CONTENT_SETTING_ALLOW);
+    RespondToPermission(&permission_context, id, url, persist, decision);
     EXPECT_EQ(1u, permission_context.decisions().size());
-    EXPECT_EQ(CONTENT_SETTING_ALLOW, permission_context.decisions()[0]);
+    EXPECT_EQ(decision, permission_context.decisions()[0]);
     EXPECT_TRUE(permission_context.tab_context_updated());
-    EXPECT_EQ(CONTENT_SETTING_ALLOW,
-              permission_context.GetContentSettingFromMap(url, url));
+    if (persist) {
+      EXPECT_EQ(decision,
+                permission_context.GetContentSettingFromMap(url, url));
+    } else {
+      EXPECT_EQ(CONTENT_SETTING_ASK,
+                permission_context.GetContentSettingFromMap(url, url));
+    }
   }
 
   void TestAskAndDismiss_TestContent() {

[raymes, 2016/08/24 04:00:17]

Can this be removed in favour of TestAskAndDecide_TestContent?

[dominickn, 2016/08/24 06:30:57]

Done.

@@ -203,7 +210,8 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {
         base::Bind(&TestPermissionContext::TrackPermissionDecision,
                    base::Unretained(&permission_context)));
 
-    RespondToPermission(&permission_context, id, url, CONTENT_SETTING_ASK);
+    RespondToPermission(&permission_context, id, url, false,

[raymes, 2016/08/24 04:00:17]

Please document the boolean params, e.g.:
false /* persist */,

[dominickn, 2016/08/24 06:30:57]

Done.

+                        CONTENT_SETTING_ASK);
     EXPECT_EQ(1u, permission_context.decisions().size());
     EXPECT_EQ(CONTENT_SETTING_ASK, permission_context.decisions()[0]);
     EXPECT_TRUE(permission_context.tab_context_updated());
@@ -234,7 +242,8 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {
           base::Bind(&TestPermissionContext::TrackPermissionDecision,
                     base::Unretained(&permission_context)));
 
-      RespondToPermission(&permission_context, id, url, CONTENT_SETTING_ASK);
+      RespondToPermission(&permission_context, id, url, false,
+                          CONTENT_SETTING_ASK);
       histograms.ExpectTotalCount(
           "Permissions.Prompt.DismissCount." +
               PermissionUtil::GetPermissionString(permission_type),
@@ -273,7 +282,8 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {
           base::Bind(&TestPermissionContext::TrackPermissionDecision,
                     base::Unretained(&permission_context)));
 
-      RespondToPermission(&permission_context, id, url, CONTENT_SETTING_ASK);
+      RespondToPermission(&permission_context, id, url, false,
+                          CONTENT_SETTING_ASK);
       histograms.ExpectTotalCount("Permissions.Prompt.DismissCount.Geolocation",
                                   i + 1);
       EXPECT_EQ(1u, permission_context.decisions().size());
@@ -359,7 +369,8 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {
           base::Bind(&TestPermissionContext::TrackPermissionDecision,
                     base::Unretained(&permission_context)));
 
-      RespondToPermission(&permission_context, id, url, CONTENT_SETTING_ASK);
+      RespondToPermission(&permission_context, id, url, false,
+                          CONTENT_SETTING_ASK);
       EXPECT_EQ(1u, permission_context.decisions().size());
       EXPECT_EQ(expected, permission_context.decisions()[0]);
       EXPECT_TRUE(permission_context.tab_context_updated());
@@ -421,7 +432,8 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {
         base::Bind(&TestPermissionContext::TrackPermissionDecision,
                    base::Unretained(&permission_context)));
 
-    RespondToPermission(&permission_context, id, url, CONTENT_SETTING_ALLOW);
+    RespondToPermission(&permission_context, id, url, true,
+                        CONTENT_SETTING_ALLOW);
     EXPECT_EQ(1u, permission_context.decisions().size());
     EXPECT_EQ(CONTENT_SETTING_ALLOW, permission_context.decisions()[0]);
     EXPECT_TRUE(permission_context.tab_context_updated());
@@ -484,7 +496,9 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {
 
     EXPECT_EQ(0u, permission_context.decisions().size());
 
-    RespondToPermission(&permission_context, id0, url, response);
+    bool persist = (response == CONTENT_SETTING_ALLOW ||
+                    response == CONTENT_SETTING_BLOCK);
+    RespondToPermission(&permission_context, id0, url, persist, response);
 
     EXPECT_EQ(2u, permission_context.decisions().size());
     EXPECT_EQ(response, permission_context.decisions()[0]);
@@ -510,8 +524,33 @@ class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {
 
 // Simulates clicking Accept. The permission should be granted and
 // saved for future use.
-TEST_F(PermissionContextBaseTests, TestAskAndGrant) {
-  TestAskAndGrant_TestContent();
+TEST_F(PermissionContextBaseTests, TestAskAndGrantPersist) {
+  TestAskAndDecide_TestContent(content::PermissionType::NOTIFICATIONS,
+                               CONTENT_SETTINGS_TYPE_NOTIFICATIONS,
+                               CONTENT_SETTING_ALLOW, true);
+}
+
+// Simulates clicking Accept. The permission should be granted, but not
+// persisted.
+TEST_F(PermissionContextBaseTests, TestAskAndGrantNoPersist) {
+  TestAskAndDecide_TestContent(content::PermissionType::NOTIFICATIONS,
+                               CONTENT_SETTINGS_TYPE_NOTIFICATIONS,
+                               CONTENT_SETTING_ALLOW, false);
+}
+
+// Simulates clicking Block. The permission should be denied and
+// saved for future use.
+TEST_F(PermissionContextBaseTests, TestAskAndBlockPersist) {
+  TestAskAndDecide_TestContent(content::PermissionType::GEOLOCATION,
+                               CONTENT_SETTINGS_TYPE_GEOLOCATION,
+                               CONTENT_SETTING_BLOCK, true);
+}
+
+// Simulates clicking Block. The permission should be denied, but not persisted.
+TEST_F(PermissionContextBaseTests, TestAskAndBlockNoPersist) {
+  TestAskAndDecide_TestContent(content::PermissionType::GEOLOCATION,
+                               CONTENT_SETTINGS_TYPE_GEOLOCATION,
+                               CONTENT_SETTING_BLOCK, false);
 }
 
 // Simulates clicking Dismiss (X) in the infobar/bubble.