Use GURLs instead of patterns in SetContentSetting() in geolocation and push_messaging

chrome/browser/push_messaging/push_messaging_permission_context_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/push_messaging/push_messaging_permission_context.h"
 
 #include "chrome/browser/content_settings/host_content_settings_map_factory.h"
 #include "chrome/browser/permissions/permission_request_id.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "chrome/test/base/testing_profile.h"
@@ skipping 43 matching lines @@
 
 class PushMessagingPermissionContextTest
     : public ChromeRenderViewHostTestHarness {
  public:
   PushMessagingPermissionContextTest() {}
 
  protected:
   void SetContentSetting(Profile* profile,
                          ContentSettingsType setting,
                          ContentSetting value) {
     // These patterns must match those in

[Michael van Ouwerkerk, 2016/02/17 11:19:26]

Please update these comments, they still refer to patterns.

[lshang, 2016/02/18 00:23:53]

Done.

     // PermissionContextBase::UpdateContentSetting, since the tests below use
     // this method to overwrite patterns set as a result of
     // PushMessagingPermissionContext::NotifyPermissionSet.
-    ContentSettingsPattern pattern_a =
-        ContentSettingsPattern::FromURLNoWildcard(GURL(kOriginA));
-    ContentSettingsPattern insecure_pattern =
-        ContentSettingsPattern::FromURLNoWildcard(GURL(kInsecureOrigin));
+    GURL url_a = GURL(kOriginA);
+    GURL insecure_url = GURL(kInsecureOrigin);
     HostContentSettingsMap* host_content_settings_map =
         HostContentSettingsMapFactory::GetForProfile(profile);
-    host_content_settings_map->SetContentSetting(pattern_a, pattern_a, setting,
-                                                 std::string(), value);
-    host_content_settings_map->SetContentSetting(
-        insecure_pattern, insecure_pattern, setting, std::string(), value);
+
+    if (setting == CONTENT_SETTINGS_TYPE_NOTIFICATIONS) {

[Michael van Ouwerkerk, 2016/02/17 11:19:26]

Why is this branching needed, just to pass a blank GURL() for the notifications
case? What would fail if |url_a| or |insecure_url| was passed?

[lshang, 2016/02/18 00:23:53]

It will fail in DCHECK in:
https://codereview.chromium.org/1686343002/diff/180001/components/content_set...
line134
since the scoping type of notification is REQUESTING_ORIGIN_ONLY_SCOPE  so its
top_level_url needs to be empty().
Or we can use SetContentSettingCustomScope() here which can pass in specific
patterns. It doesn't check scoping types so that we can get rid of the branch.
But unless adding a custom-scoped setting(e.g. in tests and special cases), we
recommend developers to use SetContentSettingDefaultScope() instead.

[Michael van Ouwerkerk, 2016/02/18 09:42:44]

I see, thank you!

+      host_content_settings_map->SetContentSettingDefaultScope(
+          url_a, GURL(), setting, std::string(), value);
+      host_content_settings_map->SetContentSettingDefaultScope(
+          insecure_url, GURL(), setting, std::string(), value);
+    } else if (setting == CONTENT_SETTINGS_TYPE_PUSH_MESSAGING) {
+      host_content_settings_map->SetContentSettingDefaultScope(
+          url_a, url_a, setting, std::string(), value);
+      host_content_settings_map->SetContentSettingDefaultScope(
+          insecure_url, insecure_url, setting, std::string(), value);
+    }
   }
 };
 
 }  // namespace
 
 TEST_F(PushMessagingPermissionContextTest, HasPermissionPrompt) {
   TestingProfile profile;
   PushMessagingPermissionContext context(&profile);
   EXPECT_EQ(CONTENT_SETTING_ASK,
             context.GetPermissionStatus(GURL(kOriginA), GURL(kOriginA)));
@@ skipping 204 matching lines @@
   EXPECT_EQ(CONTENT_SETTING_BLOCK,
             context.GetPermissionStatus(GURL(kInsecureOrigin),
                                         GURL(kInsecureOrigin)));
 
   SetContentSetting(&profile, CONTENT_SETTINGS_TYPE_PUSH_MESSAGING,
                     CONTENT_SETTING_ASK);
   EXPECT_EQ(CONTENT_SETTING_BLOCK,
             context.GetPermissionStatus(GURL(kInsecureOrigin),
                                         GURL(kInsecureOrigin)));
 }