Replace raw ptr from ContentSettingsPattern Builder with unique_ptr

Replace raw ptr from ContentSettingsPattern Builder with unique_ptr

Replace the raw ptr returned by ContentSettingsPattern::CreateBuilder
with a unique_ptr.
Remove legacy_validate parameter and function because it isn't used
anywhere.

BUG=
TBR=msarda@chromium.org,finnur@chromium.org,stevenjb@chromium.org

Review-Url: https://codereview.chromium.org/2909273003
Cr-Commit-Position: refs/heads/master@{#476236}
Committed: https://chromium.googlesource.com/chromium/src/+/3de4c449d558eae83a47e1e209e97f73b3470b99

[dullweber, 2017-05-30 14:44:00]

The CQ bit was checked by dullweber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-30 14:44:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-30 15:48:49]

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

[commit-bot: I haz the power, 2017-05-30 15:48:50]

Dry run: This issue passed the CQ dry run.

[dullweber, 2017-05-31 08:12:22]

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

[dullweber, 2017-05-31 08:12:23]

Hi Martin,
when working on counting ZoomLevels, I noticed that the
ContentSettingsPattern::CreateBuilder returns a raw ptr instead of a unique_ptr
and the legacy_validate parameter isn't used anymore, so I fixed this and
removed the legacy validation code.

[msramek, 2017-05-31 10:42:32]

https://codereview.chromium.org/2909273003/diff/1/components/content_settings...
File components/content_settings/core/common/content_settings_pattern.cc
(right):

https://codereview.chromium.org/2909273003/diff/1/components/content_settings...
components/content_settings/core/common/content_settings_pattern.cc:341: auto
builder = ContentSettingsPattern::CreateBuilder();
The style guide suggests that auto should not be used if the type cannot be
inferred by looking at the surrounding lines. I think that holds here and in the
other replacements in this file.

However, auto would be permitted if the type was visible from the template. That
brings me to a question - do we even need CreateBuilder()? Nowadays, one could
just do

auto builder = base::MakeUnique<ContentSettingsPattern>();

I think the existence of CreateBuilder() was only supposed to hide the raw
"new", and nowadays we have standard ways to do that.

[dullweber, 2017-05-31 11:42:37]

https://codereview.chromium.org/2909273003/diff/1/components/content_settings...
File components/content_settings/core/common/content_settings_pattern.cc
(right):

https://codereview.chromium.org/2909273003/diff/1/components/content_settings...
components/content_settings/core/common/content_settings_pattern.cc:341: auto
builder = ContentSettingsPattern::CreateBuilder();
On 2017/05/31 10:42:32, msramek wrote:
> The style guide suggests that auto should not be used if the type cannot be
> inferred by looking at the surrounding lines. I think that holds here and in
the
> other replacements in this file.
> 
> However, auto would be permitted if the type was visible from the template.
That
> brings me to a question - do we even need CreateBuilder()? Nowadays, one could
> just do
> 
> auto builder = base::MakeUnique<ContentSettingsPattern>();
> 
> I think the existence of CreateBuilder() was only supposed to hide the raw
> "new", and nowadays we have standard ways to do that.

CreateBuilder returns a ContentSettingsBuilder::BuilderInterface.
The only reason to have BuilderInterface is that
ContentSettingsPatternParserTest mocks the builder to check for the right calls.
I could either move the ::Builder class to the header and make it public, so it
could be created directly (even on the stack by ContentSettingsPattern::Builder
builder;) or keep it this way and use an explicit type instead of auto. I will
do the second for now because it requires less changes. I can still use stack
allocation inside ContentSettingsPattern.

[msramek, 2017-05-31 16:08:47]

LGTM

https://codereview.chromium.org/2909273003/diff/1/components/content_settings...
File components/content_settings/core/common/content_settings_pattern.cc
(right):

https://codereview.chromium.org/2909273003/diff/1/components/content_settings...
components/content_settings/core/common/content_settings_pattern.cc:341: auto
builder = ContentSettingsPattern::CreateBuilder();
On 2017/05/31 11:42:37, dullweber wrote:
> On 2017/05/31 10:42:32, msramek wrote:
> > The style guide suggests that auto should not be used if the type cannot be
> > inferred by looking at the surrounding lines. I think that holds here and in
> the
> > other replacements in this file.
> > 
> > However, auto would be permitted if the type was visible from the template.
> That
> > brings me to a question - do we even need CreateBuilder()? Nowadays, one
could
> > just do
> > 
> > auto builder = base::MakeUnique<ContentSettingsPattern>();
> > 
> > I think the existence of CreateBuilder() was only supposed to hide the raw
> > "new", and nowadays we have standard ways to do that.
> 
> CreateBuilder returns a ContentSettingsBuilder::BuilderInterface.
> The only reason to have BuilderInterface is that
> ContentSettingsPatternParserTest mocks the builder to check for the right
calls.
> I could either move the ::Builder class to the header and make it public, so
it
> could be created directly (even on the stack by
ContentSettingsPattern::Builder
> builder;) or keep it this way and use an explicit type instead of auto. I will
> do the second for now because it requires less changes. I can still use stack
> allocation inside ContentSettingsPattern.

Acknowledged. I'm fine with the other proposal too, but let's not overcomplicate
this CL.

[dullweber, 2017-06-01 07:58:41]

The CQ bit was checked by dullweber@chromium.org

[commit-bot: I haz the power, 2017-06-01 07:58:56]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-01 08:07:13]

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

[commit-bot: I haz the power, 2017-06-01 08:07:14]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dullweber, 2017-06-01 08:20:00]

The CQ bit was checked by dullweber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-01 08:20:07]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dullweber, 2017-06-01 08:29:25]

Description was changed from

==========
Replace raw ptr from ContentSettingsPattern Builder with unique_ptr

Replace the raw ptr returned by ContentSettingsPattern::CreateBuilder
with a unique_ptr.
Remove legacy_validate parameter and function because it isn't used
anywhere.

BUG=
==========

to

==========
Replace raw ptr from ContentSettingsPattern Builder with unique_ptr

Replace the raw ptr returned by ContentSettingsPattern::CreateBuilder
with a unique_ptr.
Remove legacy_validate parameter and function because it isn't used
anywhere.

BUG=
TBR=msarda@chromium.org,finnur@chromium.org,stevenjb@chromium.org
==========

[dullweber, 2017-06-01 08:30:09]

dullweber@chromium.org changed reviewers:
+ finnur@chromium.org, msarda@chromium.org, stevenjb@chromium.org

[dullweber, 2017-06-01 08:30:10]

TBR for minor changes
msarda@chromium.org: chrome/browser/signin/account_reconcilor_unittest.cc
finnur@chromium.org:
chrome/browser/extensions/api/content_settings/content_settings_helpers.cc
stevenjb@chromium.org:
chrome/browser/ui/webui/options/pepper_flash_content_settings_utils.cc

[dullweber, 2017-06-01 08:30:21]

The CQ bit was unchecked by dullweber@chromium.org

[dullweber, 2017-06-01 08:30:25]

The CQ bit was checked by dullweber@chromium.org

[dullweber, 2017-06-01 08:30:25]

The patchset sent to the CQ was uploaded after l-g-t-m from msramek@chromium.org
Link to the patchset: https://codereview.chromium.org/2909273003/#ps40001
(title: "remove more auto")

[commit-bot: I haz the power, 2017-06-01 08:30:37]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msarda, 2017-06-01 08:33:17]

LGTM for chrome/browser/signin/account_reconcilor_unittest.cc

[commit-bot: I haz the power, 2017-06-01 09:28:38]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496305825113670,
"parent_rev": "9374f06a7c08b17751ea465e6abcb68721db85b8", "commit_rev":
"3de4c449d558eae83a47e1e209e97f73b3470b99"}

[commit-bot: I haz the power, 2017-06-01 09:28:50]

Description was changed from

==========
Replace raw ptr from ContentSettingsPattern Builder with unique_ptr

Replace the raw ptr returned by ContentSettingsPattern::CreateBuilder
with a unique_ptr.
Remove legacy_validate parameter and function because it isn't used
anywhere.

BUG=
TBR=msarda@chromium.org,finnur@chromium.org,stevenjb@chromium.org
==========

to

==========
Replace raw ptr from ContentSettingsPattern Builder with unique_ptr

Replace the raw ptr returned by ContentSettingsPattern::CreateBuilder
with a unique_ptr.
Remove legacy_validate parameter and function because it isn't used
anywhere.

BUG=
TBR=msarda@chromium.org,finnur@chromium.org,stevenjb@chromium.org

Review-Url: https://codereview.chromium.org/2909273003
Cr-Commit-Position: refs/heads/master@{#476236}
Committed:
https://chromium.googlesource.com/chromium/src/+/3de4c449d558eae83a47e1e209e9...
==========

[commit-bot: I haz the power, 2017-06-01 09:28:54]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/3de4c449d558eae83a47e1e209e9...