Rewrite GetCookieSetting to return a value, instead of using an out

param.

Returning a value is preferred over using an out parameter, and is more
straightforward as to what the semantics of the method are. E.g. in
general, the out parameter may not be written to at all, so a reader
cannot assume that the method always "returns" something. In fact, in
general a caller could pass nullptr as an out parameter, so the reader
should not assume the method is capable of returning something in all
cases.

Neither of these considerations need apply in this situation, since the
methods always have a value to return, and a location to put that value.
So we should just do this the straightforward way with a normal `return`
instead.

Change-Id: I5f09507ff45743cd27e042b21ce9e6b727f7bc6e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2786733
Reviewed-by: Lily Chen <chlily@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#868167}

chrome/browser/extensions/api/content_settings/content_settings_api.cc

@@ -157,13 +157,11 @@ ContentSettingsContentSettingGetFunction::Run() {
     cookie_settings = CookieSettingsFactory::GetForProfile(profile).get();
   }
 
-  ContentSetting setting;
-  if (content_type == ContentSettingsType::COOKIES) {
-    cookie_settings->GetCookieSetting(primary_url, secondary_url, nullptr,
-                                      &setting);
-  } else {
-    setting = map->GetContentSetting(primary_url, secondary_url, content_type);
-  }
+  ContentSetting setting =
+      content_type == ContentSettingsType::COOKIES
+          ? cookie_settings->GetCookieSetting(primary_url, secondary_url,
+                                              nullptr)
+          : map->GetContentSetting(primary_url, secondary_url, content_type);
 
   std::unique_ptr<base::DictionaryValue> result(new base::DictionaryValue());
   std::string setting_string =

chrome/browser/federated_learning/floc_id_provider_unittest.cc

@@ -71,13 +71,13 @@ class FakeCookieSettings : public content_settings::CookieSettings {
  public:
   using content_settings::CookieSettings::CookieSettings;
 
-  void GetCookieSettingInternal(const GURL& url,
-                                const GURL& first_party_url,
-                                bool is_third_party_request,
-                                content_settings::SettingSource* source,
-                                ContentSetting* cookie_setting) const override {
-    *cookie_setting =
-        allow_cookies_internal_ ? CONTENT_SETTING_ALLOW : CONTENT_SETTING_BLOCK;
+  ContentSetting GetCookieSettingInternal(
+      const GURL& url,
+      const GURL& first_party_url,
+      bool is_third_party_request,
+      content_settings::SettingSource* source) const override {
+    return allow_cookies_internal_ ? CONTENT_SETTING_ALLOW
+                                   : CONTENT_SETTING_BLOCK;
   }
 
   bool ShouldBlockThirdPartyCookies() const override {

chrome/browser/ui/content_settings/content_setting_bubble_model.cc

@@ -126,8 +126,8 @@ bool GetSettingManagedByUser(const GURL& url,
   SettingSource source;
   ContentSetting setting;
   if (type == ContentSettingsType::COOKIES) {
-    CookieSettingsFactory::GetForProfile(profile)->GetCookieSetting(
-        url, url, &source, &setting);
+    setting = CookieSettingsFactory::GetForProfile(profile)->GetCookieSetting(
+        url, url, &source);
   } else {
     SettingInfo info;
     std::unique_ptr<base::Value> value =

components/content_settings/core/browser/cookie_settings.cc

@@ -90,9 +90,7 @@ bool CookieSettings::IsThirdPartyAccessAllowed(
     content_settings::SettingSource* source) {
   // Use GURL() as an opaque primary url to check if any site
   // could access cookies in a 3p context on |first_party_url|.
-  ContentSetting setting;
-  GetCookieSetting(GURL(), first_party_url, source, &setting);
-  return IsAllowed(setting);
+  return IsAllowed(GetCookieSetting(GURL(), first_party_url, source));
 }
 
 void CookieSettings::SetThirdPartyCookieSetting(const GURL& first_party_url,
@@ -164,17 +162,14 @@ bool CookieSettings::ShouldAlwaysAllowCookies(
   return false;
 }
 
-void CookieSettings::GetCookieSettingInternal(
+ContentSetting CookieSettings::GetCookieSettingInternal(
     const GURL& url,
     const GURL& first_party_url,
     bool is_third_party_request,
-    content_settings::SettingSource* source,
-    ContentSetting* cookie_setting) const {
-  DCHECK(cookie_setting);
+    content_settings::SettingSource* source) const {
   // Auto-allow in extensions or for WebUI embedding a secure origin.
   if (ShouldAlwaysAllowCookies(url, first_party_url)) {
-    *cookie_setting = CONTENT_SETTING_ALLOW;
-    return;
+    return CONTENT_SETTING_ALLOW;
   }
 
   // First get any host-specific settings.
@@ -227,7 +222,7 @@ void CookieSettings::GetCookieSettingInternal(
         net::cookie_util::StorageAccessResult::ACCESS_BLOCKED);
   }
 
-  *cookie_setting = block ? CONTENT_SETTING_BLOCK : setting;
+  return block ? CONTENT_SETTING_BLOCK : setting;
 }
 
 CookieSettings::~CookieSettings() = default;

components/content_settings/core/browser/cookie_settings.h

@@ -156,11 +156,11 @@ class CookieSettings : public CookieSettingsBase,
                                 const GURL& first_party_url) const;
 
   // content_settings::CookieSettingsBase:
-  void GetCookieSettingInternal(const GURL& url,
-                                const GURL& first_party_url,
-                                bool is_third_party_request,
-                                content_settings::SettingSource* source,
-                                ContentSetting* cookie_setting) const override;
+  ContentSetting GetCookieSettingInternal(
+      const GURL& url,
+      const GURL& first_party_url,
+      bool is_third_party_request,
+      content_settings::SettingSource* source) const override;
 
   // content_settings::Observer:
   void OnContentSettingChanged(const ContentSettingsPattern& primary_pattern,

components/content_settings/core/browser/cookie_settings_unittest.cc

@@ -455,9 +455,8 @@ TEST_F(CookieSettingsTest, GetCookieSettingAllowedTelemetry) {
   base::HistogramTester histogram_tester;
   histogram_tester.ExpectTotalCount(kAllowedRequestsHistogram, 0);
 
-  ContentSetting setting;
-  cookie_settings_->GetCookieSetting(url, top_level_url, nullptr, &setting);
-  EXPECT_EQ(setting, CONTENT_SETTING_ALLOW);
+  EXPECT_EQ(cookie_settings_->GetCookieSetting(url, top_level_url, nullptr),
+            CONTENT_SETTING_ALLOW);
   histogram_tester.ExpectTotalCount(kAllowedRequestsHistogram, 1);
   histogram_tester.ExpectBucketCount(
       kAllowedRequestsHistogram,
@@ -482,9 +481,8 @@ TEST_F(CookieSettingsTest, GetCookieSettingDisabledSAA) {
       ContentSettingsPattern::FromURLNoWildcard(top_level_url),
       ContentSettingsType::STORAGE_ACCESS, CONTENT_SETTING_ALLOW);
 
-  ContentSetting setting;
-  cookie_settings_->GetCookieSetting(url, top_level_url, nullptr, &setting);
-  EXPECT_EQ(setting, CONTENT_SETTING_BLOCK);
+  EXPECT_EQ(cookie_settings_->GetCookieSetting(url, top_level_url, nullptr),
+            CONTENT_SETTING_BLOCK);
 }
 
 // The current default behaviour of the Storage Access API should be to not
@@ -504,9 +502,8 @@ TEST_F(CookieSettingsTest, GetCookieSettingDefaultSAA) {
       ContentSettingsPattern::FromURLNoWildcard(top_level_url),
       ContentSettingsType::STORAGE_ACCESS, CONTENT_SETTING_ALLOW);
 
-  ContentSetting setting;
-  cookie_settings_->GetCookieSetting(url, top_level_url, nullptr, &setting);
-  EXPECT_EQ(setting, CONTENT_SETTING_BLOCK);
+  EXPECT_EQ(cookie_settings_->GetCookieSetting(url, top_level_url, nullptr),
+            CONTENT_SETTING_BLOCK);
   histogram_tester.ExpectTotalCount(kAllowedRequestsHistogram, 1);
   histogram_tester.ExpectBucketCount(
       kAllowedRequestsHistogram,
@@ -538,9 +535,8 @@ TEST_F(CookieSettingsTest, GetCookieSettingEnabledSAA) {
   // When requesting our setting for the url/top-level combination our
   // grant is for access should be allowed. For any other domain pairs access
   // should still be blocked.
-  ContentSetting setting;
-  cookie_settings_->GetCookieSetting(url, top_level_url, nullptr, &setting);
-  EXPECT_EQ(setting, CONTENT_SETTING_ALLOW);
+  EXPECT_EQ(cookie_settings_->GetCookieSetting(url, top_level_url, nullptr),
+            CONTENT_SETTING_ALLOW);
   histogram_tester.ExpectTotalCount(kAllowedRequestsHistogram, 1);
   histogram_tester.ExpectBucketCount(
       kAllowedRequestsHistogram,
@@ -550,15 +546,15 @@ TEST_F(CookieSettingsTest, GetCookieSettingEnabledSAA) {
 
   // Invalid pair the |top_level_url| granting access to |url| is now
   // being loaded under |url| as the top level url.
-  cookie_settings_->GetCookieSetting(top_level_url, url, nullptr, &setting);
-  EXPECT_EQ(setting, CONTENT_SETTING_BLOCK);
+  EXPECT_EQ(cookie_settings_->GetCookieSetting(top_level_url, url, nullptr),
+            CONTENT_SETTING_BLOCK);
 
   // Invalid pairs where a |third_url| is used.
-  cookie_settings_->GetCookieSetting(url, third_url, nullptr, &setting);
-  EXPECT_EQ(setting, CONTENT_SETTING_BLOCK);
-  cookie_settings_->GetCookieSetting(third_url, top_level_url, nullptr,
-                                     &setting);
-  EXPECT_EQ(setting, CONTENT_SETTING_BLOCK);
+  EXPECT_EQ(cookie_settings_->GetCookieSetting(url, third_url, nullptr),
+            CONTENT_SETTING_BLOCK);
+  EXPECT_EQ(
+      cookie_settings_->GetCookieSetting(third_url, top_level_url, nullptr),
+      CONTENT_SETTING_BLOCK);
 }
 
 // Subdomains of the granted resource url should not gain access if a valid
@@ -578,13 +574,11 @@ TEST_F(CookieSettingsTest, GetCookieSettingSAAResourceWildcards) {
       ContentSettingsPattern::FromURLNoWildcard(top_level_url),
       ContentSettingsType::STORAGE_ACCESS, CONTENT_SETTING_ALLOW);
 
-  ContentSetting setting;
-  cookie_settings_->GetCookieSetting(url, top_level_url, nullptr, &setting);
-  EXPECT_EQ(setting, CONTENT_SETTING_ALLOW);
-
-  cookie_settings_->GetCookieSetting(GURL(kHttpsSubdomainSite), top_level_url,
-                                     nullptr, &setting);
-  EXPECT_EQ(setting, CONTENT_SETTING_BLOCK);
+  EXPECT_EQ(cookie_settings_->GetCookieSetting(url, top_level_url, nullptr),
+            CONTENT_SETTING_ALLOW);
+  EXPECT_EQ(cookie_settings_->GetCookieSetting(GURL(kHttpsSubdomainSite),
+                                               top_level_url, nullptr),
+            CONTENT_SETTING_BLOCK);
 }
 
 // Subdomains of the granted top level url should not grant access if a valid
@@ -604,13 +598,11 @@ TEST_F(CookieSettingsTest, GetCookieSettingSAATopLevelWildcards) {
       ContentSettingsPattern::FromURLNoWildcard(top_level_url),
       ContentSettingsType::STORAGE_ACCESS, CONTENT_SETTING_ALLOW);
 
-  ContentSetting setting;
-  cookie_settings_->GetCookieSetting(url, top_level_url, nullptr, &setting);
-  EXPECT_EQ(setting, CONTENT_SETTING_ALLOW);
-
-  cookie_settings_->GetCookieSetting(url, GURL(kHttpsSubdomainSite), nullptr,
-                                     &setting);
-  EXPECT_EQ(setting, CONTENT_SETTING_BLOCK);
+  EXPECT_EQ(cookie_settings_->GetCookieSetting(url, top_level_url, nullptr),
+            CONTENT_SETTING_ALLOW);
+  EXPECT_EQ(cookie_settings_->GetCookieSetting(url, GURL(kHttpsSubdomainSite),
+                                               nullptr),
+            CONTENT_SETTING_BLOCK);
 }
 
 // Any Storage Access API grant should not override an explicit setting to block
@@ -629,9 +621,8 @@ TEST_F(CookieSettingsTest, GetCookieSettingSAARespectsSettings) {
       ContentSettingsPattern::FromURLNoWildcard(top_level_url),
       ContentSettingsType::STORAGE_ACCESS, CONTENT_SETTING_ALLOW);
 
-  ContentSetting setting;
-  cookie_settings_->GetCookieSetting(url, top_level_url, nullptr, &setting);
-  EXPECT_EQ(setting, CONTENT_SETTING_BLOCK);
+  EXPECT_EQ(cookie_settings_->GetCookieSetting(url, top_level_url, nullptr),
+            CONTENT_SETTING_BLOCK);
 }
 
 // Once a grant expires access should no longer be given.
@@ -656,15 +647,14 @@ TEST_F(CookieSettingsTest, GetCookieSettingSAAExpiredGrant) {
   // When requesting our setting for the url/top-level combination our
   // grant is for access should be allowed. For any other domain pairs access
   // should still be blocked.
-  ContentSetting setting;
-  cookie_settings_->GetCookieSetting(url, top_level_url, nullptr, &setting);
-  EXPECT_EQ(setting, CONTENT_SETTING_ALLOW);
+  EXPECT_EQ(cookie_settings_->GetCookieSetting(url, top_level_url, nullptr),
+            CONTENT_SETTING_ALLOW);
 
   // If we fastforward past the expiration of our grant the result should be
   // CONTENT_SETTING_BLOCK now.
   FastForwardTime(base::TimeDelta::FromSeconds(101));
-  cookie_settings_->GetCookieSetting(url, top_level_url, nullptr, &setting);
-  EXPECT_EQ(setting, CONTENT_SETTING_BLOCK);
+  EXPECT_EQ(cookie_settings_->GetCookieSetting(url, top_level_url, nullptr),
+            CONTENT_SETTING_BLOCK);
 }
 #endif
 

components/content_settings/core/common/cookie_settings_base.cc

@@ -31,8 +31,7 @@ bool CookieSettingsBase::ShouldDeleteCookieOnExit(
     const std::string& domain,
     bool is_https) const {
   GURL origin = net::cookie_util::CookieOriginToURL(domain, is_https);
-  ContentSetting setting;
-  GetCookieSetting(origin, origin, nullptr, &setting);
+  ContentSetting setting = GetCookieSetting(origin, origin, nullptr);
   DCHECK(IsValidSetting(setting));
   if (setting == CONTENT_SETTING_ALLOW)
     return false;
@@ -61,14 +60,12 @@ bool CookieSettingsBase::ShouldDeleteCookieOnExit(
   return setting == CONTENT_SETTING_SESSION_ONLY || matches_session_only_rule;
 }
 
-void CookieSettingsBase::GetCookieSetting(
+ContentSetting CookieSettingsBase::GetCookieSetting(
     const GURL& url,
     const GURL& first_party_url,
-    content_settings::SettingSource* source,
-    ContentSetting* cookie_setting) const {
-  GetCookieSettingInternal(url, first_party_url,
-                           IsThirdPartyRequest(url, first_party_url), source,
-                           cookie_setting);
+    content_settings::SettingSource* source) const {
+  return GetCookieSettingInternal(
+      url, first_party_url, IsThirdPartyRequest(url, first_party_url), source);
 }
 
 bool CookieSettingsBase::IsCookieAccessAllowed(
@@ -79,25 +76,21 @@ bool CookieSettingsBase::IsCookieAccessAllowed(
   // content settings on IOS, so it does not matter.
   DCHECK(!first_party_url.is_empty() || url.is_empty()) << url;
 #endif
-  ContentSetting setting;
-  GetCookieSetting(url, first_party_url, nullptr, &setting);
-  return IsAllowed(setting);
+  return IsAllowed(GetCookieSetting(url, first_party_url, nullptr));
 }
 
 bool CookieSettingsBase::IsCookieAccessAllowed(
     const GURL& url,
     const GURL& site_for_cookies,
     const base::Optional<url::Origin>& top_frame_origin) const {
-  ContentSetting setting;
-  GetCookieSettingInternal(
+  ContentSetting setting = GetCookieSettingInternal(
       url, top_frame_origin ? top_frame_origin->GetURL() : site_for_cookies,
-      IsThirdPartyRequest(url, site_for_cookies), nullptr, &setting);
+      IsThirdPartyRequest(url, site_for_cookies), nullptr);
   return IsAllowed(setting);
 }
 
 bool CookieSettingsBase::IsCookieSessionOnly(const GURL& origin) const {
-  ContentSetting setting;
-  GetCookieSetting(origin, origin, nullptr, &setting);
+  ContentSetting setting = GetCookieSetting(origin, origin, nullptr);
   DCHECK(IsValidSetting(setting));
   return setting == CONTENT_SETTING_SESSION_ONLY;
 }

components/content_settings/core/common/cookie_settings_base.h

@@ -107,10 +107,10 @@ class CookieSettingsBase {
   bool IsCookieSessionOnly(const GURL& url) const;
 
   // A helper for applying third party cookie blocking rules.
-  void GetCookieSetting(const GURL& url,
-                        const GURL& first_party_url,
-                        content_settings::SettingSource* source,
-                        ContentSetting* cookie_setting) const;
+  ContentSetting GetCookieSetting(
+      const GURL& url,
+      const GURL& first_party_url,
+      content_settings::SettingSource* source) const;
 
   // Returns the cookie access semantics (legacy or nonlegacy) to be applied for
   // cookies on the given domain. The |cookie_domain| can be provided as the
@@ -171,12 +171,11 @@ class CookieSettingsBase {
   static bool IsValidSettingForLegacyAccess(ContentSetting setting);
 
  private:
-  virtual void GetCookieSettingInternal(
+  virtual ContentSetting GetCookieSettingInternal(
       const GURL& url,
       const GURL& first_party_url,
       bool is_third_party_request,
-      content_settings::SettingSource* source,
-      ContentSetting* cookie_setting) const = 0;
+      content_settings::SettingSource* source) const = 0;
 
   DISALLOW_COPY_AND_ASSIGN(CookieSettingsBase);
 };

components/content_settings/core/common/cookie_settings_base_unittest.cc

@@ -38,12 +38,12 @@ class CallbackCookieSettings : public CookieSettingsBase {
       : callback_(std::move(callback)) {}
 
   // CookieSettingsBase:
-  void GetCookieSettingInternal(const GURL& url,
-                                const GURL& first_party_url,
-                                bool is_third_party_request,
-                                content_settings::SettingSource* source,
-                                ContentSetting* cookie_setting) const override {
-    *cookie_setting = callback_.Run(url);
+  ContentSetting GetCookieSettingInternal(
+      const GURL& url,
+      const GURL& first_party_url,
+      bool is_third_party_request,
+      content_settings::SettingSource* source) const override {
+    return callback_.Run(url);
   }
   void GetSettingForLegacyCookieAccess(const std::string& cookie_domain,
                                        ContentSetting* setting) const override {