Default Provider style fixes.

Default Provider style fixes.

Addressing comments from CL 1004733003.

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

[msramek, 2015-03-27 16:22:12]

msramek@chromium.org changed reviewers:
+ bauerb@chromium.org

[msramek, 2015-03-27 16:22:13]

Hi Bernhard,

PTAL. I'm doing a style cleanup according to your comments in
https://codereview.chromium.org/1004733003/ .

Thanks,
Martin

[Bernhard Bauer, 2015-04-07 11:00:30]

LGTM with a suggestion:

https://codereview.chromium.org/1038203003/diff/40001/components/content_sett...
File components/content_settings/core/browser/content_settings_utils.h (right):

https://codereview.chromium.org/1038203003/diff/40001/components/content_sett...
components/content_settings/core/browser/content_settings_utils.h:64:
base::Value* ContentSettingToValue(ContentSetting setting);
You could return a scoped_ptr here.

[msramek, 2015-04-08 08:50:58]

https://codereview.chromium.org/1038203003/diff/40001/components/content_sett...
File components/content_settings/core/browser/content_settings_utils.h (right):

https://codereview.chromium.org/1038203003/diff/40001/components/content_sett...
components/content_settings/core/browser/content_settings_utils.h:64:
base::Value* ContentSettingToValue(ContentSetting setting);
On 2015/04/07 11:00:30, Bernhard Bauer wrote:
> You could return a scoped_ptr here.

Done. I agree that it's cleaner (although 3 out of 4 of my callsites actually
require a naked one :) )

[Bernhard Bauer, 2015-04-08 09:17:42]

https://codereview.chromium.org/1038203003/diff/60001/components/content_sett...
File
components/content_settings/core/browser/content_settings_default_provider.cc
(right):

https://codereview.chromium.org/1038203003/diff/60001/components/content_sett...
components/content_settings/core/browser/content_settings_default_provider.cc:325:
kDefaultSettings[content_type].default_value).release());
You can assign the scoped_ptr directly instead of using release() and reset() :)

[msramek, 2015-04-08 09:42:02]

Thanks for the sanity check :)

https://codereview.chromium.org/1038203003/diff/60001/components/content_sett...
File
components/content_settings/core/browser/content_settings_default_provider.cc
(right):

https://codereview.chromium.org/1038203003/diff/60001/components/content_sett...
components/content_settings/core/browser/content_settings_default_provider.cc:325:
kDefaultSettings[content_type].default_value).release());
On 2015/04/08 09:17:42, Bernhard Bauer wrote:
> You can assign the scoped_ptr directly instead of using release() and reset()
:)

Eh... I just wasn't sure if I wouldn't leak the old referenced object. 

But of course scoped_ptr::operator= does reset(release()) itself. Which is of
course the correct semantic of operator=. Never mind me.

[msramek, 2015-04-08 12:17:38]

The CQ bit was checked by msramek@chromium.org

[msramek, 2015-04-08 12:17:39]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1038203003/#ps80001
(title: "Direct assignment.")

[commit-bot: I haz the power, 2015-04-08 12:18:00]

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

[commit-bot: I haz the power, 2015-04-08 12:22:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-08 12:22:40]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[msramek, 2015-04-08 13:48:36]

https://codereview.chromium.org/1038203003/diff/60001/components/content_sett...
File
components/content_settings/core/browser/content_settings_default_provider.cc
(right):

https://codereview.chromium.org/1038203003/diff/60001/components/content_sett...
components/content_settings/core/browser/content_settings_default_provider.cc:325:
kDefaultSettings[content_type].default_value).release());
On 2015/04/08 09:42:02, msramek wrote:
> On 2015/04/08 09:17:42, Bernhard Bauer wrote:
> > You can assign the scoped_ptr directly instead of using release() and
reset()
> :)
> 
> Eh... I just wasn't sure if I wouldn't leak the old referenced object. 
> 
> But of course scoped_ptr::operator= does reset(release()) itself. Which is of
> course the correct semantic of operator=. Never mind me.

Hm, and we actually cannot do that, because ValueMap uses linked_ptr and not
scoped_ptr. I guess the reason is that ValueMap is an std::map working with
value semantics, and scoped_ptr does not have a copy constructor.

I reverted this to reset(release()) for now. We should probably get rid of
linked_ptr, as their usage is discouraged, and I don't think we really need ref
counting here, but I'd do that in a separate CL.

[msramek, 2015-04-09 08:11:26]

The CQ bit was checked by msramek@chromium.org

[msramek, 2015-04-09 08:11:26]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1038203003/#ps120001
(title: "Back to reset(release()).")

[commit-bot: I haz the power, 2015-04-09 08:11:40]

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

[Bernhard Bauer, 2015-04-09 08:13:37]

https://codereview.chromium.org/1038203003/diff/60001/components/content_sett...
File
components/content_settings/core/browser/content_settings_default_provider.cc
(right):

https://codereview.chromium.org/1038203003/diff/60001/components/content_sett...
components/content_settings/core/browser/content_settings_default_provider.cc:325:
kDefaultSettings[content_type].default_value).release());
On 2015/04/08 13:48:35, msramek wrote:
> On 2015/04/08 09:42:02, msramek wrote:
> > On 2015/04/08 09:17:42, Bernhard Bauer wrote:
> > > You can assign the scoped_ptr directly instead of using release() and
> reset()
> > :)
> > 
> > Eh... I just wasn't sure if I wouldn't leak the old referenced object. 
> > 
> > But of course scoped_ptr::operator= does reset(release()) itself. Which is
of
> > course the correct semantic of operator=. Never mind me.
> 
> Hm, and we actually cannot do that, because ValueMap uses linked_ptr and not
> scoped_ptr. I guess the reason is that ValueMap is an std::map working with
> value semantics, and scoped_ptr does not have a copy constructor.
> 
> I reverted this to reset(release()) for now. We should probably get rid of
> linked_ptr, as their usage is discouraged, and I don't think we really need
ref
> counting here, but I'd do that in a separate CL.

Too bad. Getting rid of linked_ptr in a followup CL sounds good.

[commit-bot: I haz the power, 2015-04-09 09:31:08]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-04-09 09:32:04]

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