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

chrome/browser/permissions/permission_context_base_unittest.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/permissions/permission_context_base.h"
 
 #include <vector>
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 110 matching lines @@
 };
 
 class PermissionContextBaseTests : public ChromeRenderViewHostTestHarness {
  protected:
   PermissionContextBaseTests() {}
 
   // Accept or dismiss the permission bubble or infobar.
   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();
         break;
       case CONTENT_SETTING_BLOCK:
         manager->Deny();
         break;
       case CONTENT_SETTING_ASK:
         manager->Closing();
         break;
       default:
         NOTREACHED();
     }
 #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(
         web_contents()->GetRenderProcessHost()->GetID(),
         web_contents()->GetMainFrame()->GetRoutingID(),
         -1);
     permission_context.RequestPermission(
         web_contents(),
         id, url, true /* user_gesture */,
         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.

     TestPermissionContext permission_context(
         profile(), content::PermissionType::MIDI_SYSEX,
         CONTENT_SETTINGS_TYPE_MIDI_SYSEX);
     GURL url("http://www.google.es");
     NavigateAndCommit(url);
 
     const PermissionRequestID id(
         web_contents()->GetRenderProcessHost()->GetID(),
         web_contents()->GetMainFrame()->GetRoutingID(),
         -1);
     permission_context.RequestPermission(
         web_contents(),
         id, url, true /* user_gesture */,
         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());
     EXPECT_EQ(CONTENT_SETTING_ASK,
               permission_context.GetContentSettingFromMap(url, url));
   }
 
   void DismissMultipleTimesAndExpectBlock(
       const GURL& url,
       content::PermissionType permission_type,
@@ skipping 10 matching lines @@
           (i < (iterations - 1)) ? CONTENT_SETTING_ASK : CONTENT_SETTING_BLOCK;
 
       const PermissionRequestID id(
           web_contents()->GetRenderProcessHost()->GetID(),
           web_contents()->GetMainFrame()->GetRoutingID(), i);
       permission_context.RequestPermission(
           web_contents(), id, url, true /* user_gesture */,
           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),
           i + 1);
 
       EXPECT_EQ(1u, permission_context.decisions().size());
       EXPECT_EQ(expected, permission_context.decisions()[0]);
       EXPECT_TRUE(permission_context.tab_context_updated());
       EXPECT_EQ(expected,
                 permission_context.GetContentSettingFromMap(url, url));
@@ skipping 18 matching lines @@
           CONTENT_SETTINGS_TYPE_GEOLOCATION);
 
       const PermissionRequestID id(
           web_contents()->GetRenderProcessHost()->GetID(),
           web_contents()->GetMainFrame()->GetRoutingID(), i);
       permission_context.RequestPermission(
           web_contents(), id, url, true /* user_gesture */,
           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());
       EXPECT_EQ(CONTENT_SETTING_ASK, permission_context.decisions()[0]);
       EXPECT_TRUE(permission_context.tab_context_updated());
       EXPECT_EQ(CONTENT_SETTING_ASK,
                 permission_context.GetContentSettingFromMap(url, url));
     }
 
     // Flush the dismissal counts. Enable the block on too many dismissals
@@ skipping 65 matching lines @@
       ContentSetting expected =
           (i < 4) ? CONTENT_SETTING_ASK : CONTENT_SETTING_BLOCK;
       const PermissionRequestID id(
           web_contents()->GetRenderProcessHost()->GetID(),
           web_contents()->GetMainFrame()->GetRoutingID(), i);
       permission_context.RequestPermission(
           web_contents(), id, url, true /* user_gesture */,
           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());
       EXPECT_EQ(expected,
                 permission_context.GetContentSettingFromMap(url, url));
     }
 
     // Ensure that we finish in the block state.
     TestPermissionContext permission_context(
         profile(), content::PermissionType::MIDI_SYSEX,
@@ skipping 41 matching lines @@
     const PermissionRequestID id(
         web_contents()->GetRenderProcessHost()->GetID(),
         web_contents()->GetMainFrame()->GetRoutingID(),
         -1);
     permission_context.RequestPermission(
         web_contents(),
         id, url, true /* user_gesture */,
         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());
     EXPECT_EQ(CONTENT_SETTING_ALLOW,
               permission_context.GetContentSettingFromMap(url, url));
 
     // Try to reset permission.
     permission_context.ResetPermission(url.GetOrigin(), url.GetOrigin());
     ContentSetting setting_after_reset =
         permission_context.GetContentSettingFromMap(url, url);
@@ skipping 42 matching lines @@
         web_contents(), id0, url, true /* user_gesture */,
         base::Bind(&TestPermissionContext::TrackPermissionDecision,
                    base::Unretained(&permission_context)));
     permission_context.RequestPermission(
         web_contents(), id1, url, true /* user_gesture */,
         base::Bind(&TestPermissionContext::TrackPermissionDecision,
                    base::Unretained(&permission_context)));
 
     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]);
     EXPECT_EQ(response, permission_context.decisions()[1]);
     EXPECT_TRUE(permission_context.tab_context_updated());
 
     EXPECT_EQ(response, permission_context.GetContentSettingFromMap(url, url));
   }
 
  private:
   // ChromeRenderViewHostTestHarness:
   void SetUp() override {
     ChromeRenderViewHostTestHarness::SetUp();
 #if defined(OS_ANDROID)
     InfoBarService::CreateForWebContents(web_contents());
 #else
     PermissionRequestManager::CreateForWebContents(web_contents());
 #endif
   }
 
   DISALLOW_COPY_AND_ASSIGN(PermissionContextBaseTests);
 };
 
 // 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.
 // The permission should be denied but not saved for future use.
 TEST_F(PermissionContextBaseTests, TestAskAndDismiss) {
   TestAskAndDismiss_TestContent();
 }
 
 // Simulates clicking Dismiss (X) in the infobar/bubble with the block on too
 // many dismissals feature active. The permission should be blocked after
@@ skipping 91 matching lines @@
   TestParallelRequests(CONTENT_SETTING_ALLOW);
 }
 
 TEST_F(PermissionContextBaseTests, TestParallelRequestsBlocked) {
   TestParallelRequests(CONTENT_SETTING_BLOCK);
 }
 
 TEST_F(PermissionContextBaseTests, TestParallelRequestsDismissed) {
   TestParallelRequests(CONTENT_SETTING_ASK);
 }