Refactor HostContentSettingsMap::SetWebsiteSettingDefaultScope to take a std::unique_ptr.

Refactor HostContentSettingsMap::SetWebsiteSettingDefaultScope to take a std::unique_ptr.

Currently, HostContentSettingsMap::SetWebsiteSettingDefaultScope
takes a raw base::Value*, and its clients typically release() a
std::unique_ptr<base::Value> when calling it. However, this method
returns without doing anything if provided with an invalid primary or
secondary URL pattern (e.g. the view source page). When this happens,
the base::Value is leaked, as it has been released() by its previous
owner without being taken into a unique_ptr in
SetWebsiteSettingsDefaultScope.

This CL eliminates the leak by making the method take a std::unique_ptr,
and adjusting its call sites to use either std::move or base::WrapUnique.

BUG=618529
TBR=felt@chromium.org,mkwst@chromium.org,benwells@chromium.org

Committed: https://crrev.com/d7e7fc45f0d5f84430707280a18dfe7d6217aed8
Cr-Commit-Position: refs/heads/master@{#398806}

[dominickn, 2016-06-09 01:52:57]

Description was changed from

==========
Refactor HostContentSettingsMap::SetWebsiteSettingDefaultScope to take a
std::unique_ptr.

Currently, HostContentSettingsMap::SetWebsiteSettingDefaultScope takes
a raw base::Value*, and its clients typically release() a
std::unique_ptr when calling it. However, this method returns without
doing anything if provided with an invalid primary or secondary URL
pattern (e.g. the view source page). When this happens, the base::Value
is leaked.

This CL eliminates the leak by making the method take a std::unique_ptr,
and adjusting its call sites to std::move their std::unique_ptr when
calling it.

BUG=618529
==========

to

==========
Refactor HostContentSettingsMap::SetWebsiteSettingDefaultScope to take a
std::unique_ptr.

Currently, HostContentSettingsMap::SetWebsiteSettingDefaultScope takes
a raw base::Value*, and its clients typically release() a
std::unique_ptr when calling it. However, this method returns without
doing anything if provided with an invalid primary or secondary URL
pattern (e.g. the view source page). When this happens, the base::Value
is leaked.

This CL eliminates the leak by making the method take a std::unique_ptr,
and adjusting its call sites to std::move their std::unique_ptr when
calling it.

BUG=618529
==========

[dominickn, 2016-06-09 01:52:57]

dominickn@chromium.org changed reviewers:
+ raymes@chromium.org

[dominickn, 2016-06-09 01:53:45]

Description was changed from

==========
Refactor HostContentSettingsMap::SetWebsiteSettingDefaultScope to take a
std::unique_ptr.

Currently, HostContentSettingsMap::SetWebsiteSettingDefaultScope takes
a raw base::Value*, and its clients typically release() a
std::unique_ptr when calling it. However, this method returns without
doing anything if provided with an invalid primary or secondary URL
pattern (e.g. the view source page). When this happens, the base::Value
is leaked.

This CL eliminates the leak by making the method take a std::unique_ptr,
and adjusting its call sites to std::move their std::unique_ptr when
calling it.

BUG=618529
==========

to

==========
Refactor HostContentSettingsMap::SetWebsiteSettingDefaultScope to take a
std::unique_ptr.

Currently, HostContentSettingsMap::SetWebsiteSettingDefaultScope takes
a raw base::Value*, and its clients typically release() a
std::unique_ptr when calling it. However, this method returns without
doing anything if provided with an invalid primary or secondary URL
pattern (e.g. the view source page). When this happens, the base::Value
is leaked.

This CL eliminates the leak by making the method take a std::unique_ptr,
and adjusting its call sites to std::move their std::unique_ptr when
calling it.

BUG=618529
TBR=felt@chromium.org,mkwst@chromium.org,benwells@chromium.org
==========

[dominickn, 2016-06-09 03:05:49]

raymes: PTAL, thanks!

[dominickn, 2016-06-09 03:07:31]

Description was changed from

==========
Refactor HostContentSettingsMap::SetWebsiteSettingDefaultScope to take a
std::unique_ptr.

Currently, HostContentSettingsMap::SetWebsiteSettingDefaultScope takes
a raw base::Value*, and its clients typically release() a
std::unique_ptr when calling it. However, this method returns without
doing anything if provided with an invalid primary or secondary URL
pattern (e.g. the view source page). When this happens, the base::Value
is leaked.

This CL eliminates the leak by making the method take a std::unique_ptr,
and adjusting its call sites to std::move their std::unique_ptr when
calling it.

BUG=618529
TBR=felt@chromium.org,mkwst@chromium.org,benwells@chromium.org
==========

to

==========
Refactor HostContentSettingsMap::SetWebsiteSettingDefaultScope to take a
std::unique_ptr.

Currently, HostContentSettingsMap::SetWebsiteSettingDefaultScope takes
a raw base::Value*, and its clients typically release() a
std::unique_ptr when calling it. However, this method returns without
doing anything if provided with an invalid primary or secondary URL
pattern (e.g. the view source page). When this happens, the base::Value
is leaked, as it has been released() by its previous owner without being
taken into a unique_ptr in SetWebsiteSettingsDefaultScope.

This CL eliminates the leak by making the method take a std::unique_ptr,
and adjusting its call sites to std::move their std::unique_ptr when
calling it.

BUG=618529
TBR=felt@chromium.org,mkwst@chromium.org,benwells@chromium.org
==========

[dominickn, 2016-06-09 03:31:20]

Description was changed from

==========
Refactor HostContentSettingsMap::SetWebsiteSettingDefaultScope to take a
std::unique_ptr.

Currently, HostContentSettingsMap::SetWebsiteSettingDefaultScope takes
a raw base::Value*, and its clients typically release() a
std::unique_ptr when calling it. However, this method returns without
doing anything if provided with an invalid primary or secondary URL
pattern (e.g. the view source page). When this happens, the base::Value
is leaked, as it has been released() by its previous owner without being
taken into a unique_ptr in SetWebsiteSettingsDefaultScope.

This CL eliminates the leak by making the method take a std::unique_ptr,
and adjusting its call sites to std::move their std::unique_ptr when
calling it.

BUG=618529
TBR=felt@chromium.org,mkwst@chromium.org,benwells@chromium.org
==========

to

==========
Refactor HostContentSettingsMap::SetWebsiteSettingDefaultScope to take a
std::unique_ptr.

Currently, HostContentSettingsMap::SetWebsiteSettingDefaultScope
takes a raw base::Value*, and its clients typically release() a
std::unique_ptr<base::Value> when calling it. However, this method
returns without doing anything if provided with an invalid primary or
secondary URL pattern (e.g. the view source page). When this happens,
the base::Value is leaked, as it has been released() by its previous
owner without being taken into a unique_ptr in
SetWebsiteSettingsDefaultScope.

This CL eliminates the leak by making the method take a std::unique_ptr,
and adjusting its call sites to std::move their std::unique_ptr when
calling it.

BUG=618529
TBR=felt@chromium.org,mkwst@chromium.org,benwells@chromium.org
==========

[dominickn, 2016-06-09 05:53:14]

Description was changed from

==========
Refactor HostContentSettingsMap::SetWebsiteSettingDefaultScope to take a
std::unique_ptr.

Currently, HostContentSettingsMap::SetWebsiteSettingDefaultScope
takes a raw base::Value*, and its clients typically release() a
std::unique_ptr<base::Value> when calling it. However, this method
returns without doing anything if provided with an invalid primary or
secondary URL pattern (e.g. the view source page). When this happens,
the base::Value is leaked, as it has been released() by its previous
owner without being taken into a unique_ptr in
SetWebsiteSettingsDefaultScope.

This CL eliminates the leak by making the method take a std::unique_ptr,
and adjusting its call sites to std::move their std::unique_ptr when
calling it.

BUG=618529
TBR=felt@chromium.org,mkwst@chromium.org,benwells@chromium.org
==========

to

==========
Refactor HostContentSettingsMap::SetWebsiteSettingDefaultScope to take a
std::unique_ptr.

Currently, HostContentSettingsMap::SetWebsiteSettingDefaultScope
takes a raw base::Value*, and its clients typically release() a
std::unique_ptr<base::Value> when calling it. However, this method
returns without doing anything if provided with an invalid primary or
secondary URL pattern (e.g. the view source page). When this happens,
the base::Value is leaked, as it has been released() by its previous
owner without being taken into a unique_ptr in
SetWebsiteSettingsDefaultScope.

This CL eliminates the leak by making the method take a std::unique_ptr,
and adjusting its call sites to use either std::move or base::WrapUnique.

BUG=618529
TBR=felt@chromium.org,mkwst@chromium.org,benwells@chromium.org
==========

[raymes, 2016-06-09 05:59:21]

lgtm thanks Dom!

https://codereview.chromium.org/2054623002/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right):

https://codereview.chromium.org/2054623002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:29: #include
"build/build_config.h"
Should probably include ptr_util.h here too

https://codereview.chromium.org/2054623002/diff/1/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

https://codereview.chromium.org/2054623002/diff/1/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1320: }
Could we add a simple test to cover the case of an invalid pattern? This would
probably have caught the leak if we had it. 

(I haven't tested this but I tried to write it out for you :)

TEST_F(HostContentSettingsMapTest, InvalidPattern) {
  TestingProfile profile;
  HostContentSettingsMap* host_content_settings_map =
HostContentSettingsMapFactory::GetForProfile(&profile);
  GURL unsupported_url = GURL("abcd://not/valid");
  base::DictionaryValue test_value;
  test_value.SetString("test", "value");	 
  host_content_settings_map->SetWebsiteSettingDefaultScope(unsupported_url,
unsupported_url, CONTENT_SETTINGS_TYPE_APP_BANNER,
base::WrapUnique(test_value.DeepCopy()), std::string());
  EXPECT_EQ(nullptr,
host_content_settings_map->GetWebsiteSetting(unsupported_url, unsupported_url,
CONTENT_SETTINGS_TYPE_APP_BANNER, std::string(), nullptr));
}

[dominickn, 2016-06-09 06:09:44]

Thanks!

https://codereview.chromium.org/2054623002/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right):

https://codereview.chromium.org/2054623002/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:29: #include
"build/build_config.h"
On 2016/06/09 05:59:21, raymes wrote:
> Should probably include ptr_util.h here too

It's already there (line 22 :))

https://codereview.chromium.org/2054623002/diff/1/chrome/browser/content_sett...
File chrome/browser/content_settings/host_content_settings_map_unittest.cc
(right):

https://codereview.chromium.org/2054623002/diff/1/chrome/browser/content_sett...
chrome/browser/content_settings/host_content_settings_map_unittest.cc:1320: }
On 2016/06/09 05:59:21, raymes wrote:
> Could we add a simple test to cover the case of an invalid pattern? This would
> probably have caught the leak if we had it. 
> 
> (I haven't tested this but I tried to write it out for you :)
> 
> TEST_F(HostContentSettingsMapTest, InvalidPattern) {
>   TestingProfile profile;
>   HostContentSettingsMap* host_content_settings_map =
> HostContentSettingsMapFactory::GetForProfile(&profile);
>   GURL unsupported_url = GURL("abcd://not/valid");
>   base::DictionaryValue test_value;
>   test_value.SetString("test", "value");	 
>   host_content_settings_map->SetWebsiteSettingDefaultScope(unsupported_url,
> unsupported_url, CONTENT_SETTINGS_TYPE_APP_BANNER,
> base::WrapUnique(test_value.DeepCopy()), std::string());
>   EXPECT_EQ(nullptr,
> host_content_settings_map->GetWebsiteSetting(unsupported_url, unsupported_url,
> CONTENT_SETTINGS_TYPE_APP_BANNER, std::string(), nullptr));
> }

Thanks for the test! Done!

[dominickn, 2016-06-09 07:17:18]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2016-06-09 07:17:19]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/2054623002/#ps20001
(title: "Addressing reviewer comments")

[commit-bot: I haz the power, 2016-06-09 07:17:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2054623002/20001

[commit-bot: I haz the power, 2016-06-09 07:30:45]

Description was changed from

==========
Refactor HostContentSettingsMap::SetWebsiteSettingDefaultScope to take a
std::unique_ptr.

Currently, HostContentSettingsMap::SetWebsiteSettingDefaultScope
takes a raw base::Value*, and its clients typically release() a
std::unique_ptr<base::Value> when calling it. However, this method
returns without doing anything if provided with an invalid primary or
secondary URL pattern (e.g. the view source page). When this happens,
the base::Value is leaked, as it has been released() by its previous
owner without being taken into a unique_ptr in
SetWebsiteSettingsDefaultScope.

This CL eliminates the leak by making the method take a std::unique_ptr,
and adjusting its call sites to use either std::move or base::WrapUnique.

BUG=618529
TBR=felt@chromium.org,mkwst@chromium.org,benwells@chromium.org
==========

to

==========
Refactor HostContentSettingsMap::SetWebsiteSettingDefaultScope to take a
std::unique_ptr.

Currently, HostContentSettingsMap::SetWebsiteSettingDefaultScope
takes a raw base::Value*, and its clients typically release() a
std::unique_ptr<base::Value> when calling it. However, this method
returns without doing anything if provided with an invalid primary or
secondary URL pattern (e.g. the view source page). When this happens,
the base::Value is leaked, as it has been released() by its previous
owner without being taken into a unique_ptr in
SetWebsiteSettingsDefaultScope.

This CL eliminates the leak by making the method take a std::unique_ptr,
and adjusting its call sites to use either std::move or base::WrapUnique.

BUG=618529
TBR=felt@chromium.org,mkwst@chromium.org,benwells@chromium.org
==========

[commit-bot: I haz the power, 2016-06-09 07:30:46]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-09 07:32:22]

Description was changed from

==========
Refactor HostContentSettingsMap::SetWebsiteSettingDefaultScope to take a
std::unique_ptr.

Currently, HostContentSettingsMap::SetWebsiteSettingDefaultScope
takes a raw base::Value*, and its clients typically release() a
std::unique_ptr<base::Value> when calling it. However, this method
returns without doing anything if provided with an invalid primary or
secondary URL pattern (e.g. the view source page). When this happens,
the base::Value is leaked, as it has been released() by its previous
owner without being taken into a unique_ptr in
SetWebsiteSettingsDefaultScope.

This CL eliminates the leak by making the method take a std::unique_ptr,
and adjusting its call sites to use either std::move or base::WrapUnique.

BUG=618529
TBR=felt@chromium.org,mkwst@chromium.org,benwells@chromium.org
==========

to

==========
Refactor HostContentSettingsMap::SetWebsiteSettingDefaultScope to take a
std::unique_ptr.

Currently, HostContentSettingsMap::SetWebsiteSettingDefaultScope
takes a raw base::Value*, and its clients typically release() a
std::unique_ptr<base::Value> when calling it. However, this method
returns without doing anything if provided with an invalid primary or
secondary URL pattern (e.g. the view source page). When this happens,
the base::Value is leaked, as it has been released() by its previous
owner without being taken into a unique_ptr in
SetWebsiteSettingsDefaultScope.

This CL eliminates the leak by making the method take a std::unique_ptr,
and adjusting its call sites to use either std::move or base::WrapUnique.

BUG=618529
TBR=felt@chromium.org,mkwst@chromium.org,benwells@chromium.org

Committed: https://crrev.com/d7e7fc45f0d5f84430707280a18dfe7d6217aed8
Cr-Commit-Position: refs/heads/master@{#398806}
==========

[commit-bot: I haz the power, 2016-06-09 07:32:23]

Patchset 2 (id:??) landed as
https://crrev.com/d7e7fc45f0d5f84430707280a18dfe7d6217aed8
Cr-Commit-Position: refs/heads/master@{#398806}