Switch SupportsUserData uses to use unique_ptr.

Switch SupportsUserData uses to use unique_ptr.

The interface taking a raw pointer is deprecated and being removed.

BUG=690937
Review-Url: https://codereview.chromium.org/2846333004
Cr-Commit-Position: refs/heads/master@{#468862}
Committed: https://chromium.googlesource.com/chromium/src/+/284ec61c76d48490010b2f1c80628306d4d75a7f

[Avi (use Gerrit), 2017-04-29 02:19:19]

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

[commit-bot: I haz the power, 2017-04-29 02:19:42]

Dry run: CQ is trying da patch.

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

[Avi (use Gerrit), 2017-04-29 03:08:29]

avi@chromium.org changed reviewers:
+ thakis@chromium.org

[commit-bot: I haz the power, 2017-04-29 04:49:42]

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

[commit-bot: I haz the power, 2017-04-29 04:49:43]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Avi (use Gerrit), 2017-04-29 04:50:36]

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

[commit-bot: I haz the power, 2017-04-29 04:50:42]

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-04-29 05:15:19]

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

[commit-bot: I haz the power, 2017-04-29 05:15:19]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2017-05-02 01:31:52]

ping

[Nico, 2017-05-02 20:54:10]

lgtm, very sorry about the delay.

https://codereview.chromium.org/2846333004/diff/1/chrome/browser/data_use_mea...
File chrome/browser/data_use_measurement/data_use_web_contents_observer.cc
(right):

https://codereview.chromium.org/2846333004/diff/1/chrome/browser/data_use_mea...
chrome/browser/data_use_measurement/data_use_web_contents_observer.cc:39:
base::WrapUnique(new DataUseWebContentsObserver(web_contents, service)));
does MakeUnique<> not work here?

https://codereview.chromium.org/2846333004/diff/1/chrome/browser/ntp_snippets...
File chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc (right):

https://codereview.chromium.org/2846333004/diff/1/chrome/browser/ntp_snippets...
chrome/browser/ntp_snippets/bookmark_last_visit_updater.cc:43:
base::WrapUnique(new BookmarkLastVisitUpdater(
Same question

https://codereview.chromium.org/2846333004/diff/1/chrome/browser/page_load_me...
File
chrome/browser/page_load_metrics/observers/delay_navigation_page_load_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/2846333004/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/delay_navigation_page_load_metrics_observer_unittest.cc:78:
web_contents->SetUserData(UserDataKey(), base::WrapUnique(metrics));
same question

https://codereview.chromium.org/2846333004/diff/1/chrome/browser/password_man...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/2846333004/diff/1/chrome/browser/password_man...
chrome/browser/password_manager/chrome_password_manager_client.cc:155: contents,
autofill_client)));
same question

[Avi (use Gerrit), 2017-05-03 00:04:45]

To be clear, every time I used WrapUnique, it was deliberate. MakeUnique doesn't
work when the constructor is private, and I'm not going to start changing the
member access of constructors of code I don't own.

This is a huge issue with MakeUnique and std::make_unique, but it's a
fundamental design issue that we can't address here.

[Avi (use Gerrit), 2017-05-03 00:06:01]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2017-05-03 00:06:29]

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-03 01:52:28]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1493769961000710, "parent_rev":
"7ff8b217c5545b52012f2ce7ab84c1214e6a3a96", "commit_rev":
"284ec61c76d48490010b2f1c80628306d4d75a7f"}

[commit-bot: I haz the power, 2017-05-03 01:52:39]

Description was changed from

==========
Switch SupportsUserData uses to use unique_ptr.

The interface taking a raw pointer is deprecated and being removed.

BUG=690937
==========

to

==========
Switch SupportsUserData uses to use unique_ptr.

The interface taking a raw pointer is deprecated and being removed.

BUG=690937

Review-Url: https://codereview.chromium.org/2846333004
Cr-Commit-Position: refs/heads/master@{#468862}
Committed:
https://chromium.googlesource.com/chromium/src/+/284ec61c76d48490010b2f1c8062...
==========

[commit-bot: I haz the power, 2017-05-03 01:52:41]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/284ec61c76d48490010b2f1c8062...

[Nico, 2017-05-03 15:28:20]

On Tue, May 2, 2017 at 8:04 PM, <avi@chromium.org> wrote:

> To be clear, every time I used WrapUnique, it was deliberate. MakeUnique
> doesn't
> work when the constructor is private, and I'm not going to start changing
> the
> member access of constructors of code I don't own.
>

You changed the access from some constructors. I feel we should have some
global guideline for this, "which files are owned by Avi" maybe isn't the
best guideline :-)


>
> This is a huge issue with MakeUnique and std::make_unique, but it's a
> fundamental design issue that we can't address here.
>
> https://codereview.chromium.org/2846333004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Avi (use Gerrit), 2017-05-03 15:31:32]

No, I changed the access of destructors.

I believe I was making them conform to the style guide. The classes
publicly inherited from a base class that had a public destructor, so I
made the overriding destructor public too. I don't see a problem with that.

On Wed, May 3, 2017 at 11:28 AM, Nico Weber <thakis@chromium.org> wrote:

> On Tue, May 2, 2017 at 8:04 PM, <avi@chromium.org> wrote:
>
>> To be clear, every time I used WrapUnique, it was deliberate. MakeUnique
>> doesn't
>> work when the constructor is private, and I'm not going to start changing
>> the
>> member access of constructors of code I don't own.
>>
>
> You changed the access from some constructors. I feel we should have some
> global guideline for this, "which files are owned by Avi" maybe isn't the
> best guideline :-)
>
>
>>
>> This is a huge issue with MakeUnique and std::make_unique, but it's a
>> fundamental design issue that we can't address here.
>>
>> https://codereview.chromium.org/2846333004/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2017-05-03 15:37:50]

On Wed, May 3, 2017 at 11:31 AM, Avi Drissman <avi@chromium.org> wrote:

> No, I changed the access of destructors.
>
> I believe I was making them conform to the style guide. The classes
> publicly inherited from a base class that had a public destructor, so I
> made the overriding destructor public too. I don't see a problem with that.
>

Wait, you changed them needlessly? If so,

a) please do changes like this in separate changes next time
b) restricting method visibility in subclasses isn't uncommon, see this
thread:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/XdsXTHy9lis


>
> On Wed, May 3, 2017 at 11:28 AM, Nico Weber <thakis@chromium.org> wrote:
>
>> On Tue, May 2, 2017 at 8:04 PM, <avi@chromium.org> wrote:
>>
>>> To be clear, every time I used WrapUnique, it was deliberate. MakeUnique
>>> doesn't
>>> work when the constructor is private, and I'm not going to start
>>> changing the
>>> member access of constructors of code I don't own.
>>>
>>
>> You changed the access from some constructors. I feel we should have some
>> global guideline for this, "which files are owned by Avi" maybe isn't the
>> best guideline :-)
>>
>>
>>>
>>> This is a huge issue with MakeUnique and std::make_unique, but it's a
>>> fundamental design issue that we can't address here.
>>>
>>> https://codereview.chromium.org/2846333004/
>>>
>>
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mmenke, 2017-05-03 15:40:50]

Description was changed from

==========
Switch SupportsUserData uses to use unique_ptr.

The interface taking a raw pointer is deprecated and being removed.

BUG=690937

Review-Url: https://codereview.chromium.org/2846333004
Cr-Commit-Position: refs/heads/master@{#468862}
Committed:
https://chromium.googlesource.com/chromium/src/+/284ec61c76d48490010b2f1c8062...
==========

to

==========
Switch SupportsUserData uses to use unique_ptr.

The interface taking a raw pointer is deprecated and being removed.

BUG=690937

Review-Url: https://codereview.chromium.org/2846333004
Cr-Commit-Position: refs/heads/master@{#468862}
Committed:
https://chromium.googlesource.com/chromium/src/+/284ec61c76d48490010b2f1c8062...
==========

[Avi (use Gerrit), 2017-05-03 15:43:08]

I didn't do it needlessly. The API for SupportsUserData is being changed to
use unique_ptr for ownership semantics, and unique_ptr needs access to the
destructor..

On Wed, May 3, 2017 at 11:37 AM, Nico Weber <thakis@chromium.org> wrote:

> On Wed, May 3, 2017 at 11:31 AM, Avi Drissman <avi@chromium.org> wrote:
>
>> No, I changed the access of destructors.
>>
>> I believe I was making them conform to the style guide. The classes
>> publicly inherited from a base class that had a public destructor, so I
>> made the overriding destructor public too. I don't see a problem with that.
>>
>
> Wait, you changed them needlessly? If so,
>
> a) please do changes like this in separate changes next time
> b) restricting method visibility in subclasses isn't uncommon, see this
> thread: https://groups.google.com/a/chromium.org/forum/#!
> topic/chromium-dev/XdsXTHy9lis
>
>
>>
>> On Wed, May 3, 2017 at 11:28 AM, Nico Weber <thakis@chromium.org> wrote:
>>
>>> On Tue, May 2, 2017 at 8:04 PM, <avi@chromium.org> wrote:
>>>
>>>> To be clear, every time I used WrapUnique, it was deliberate.
>>>> MakeUnique doesn't
>>>> work when the constructor is private, and I'm not going to start
>>>> changing the
>>>> member access of constructors of code I don't own.
>>>>
>>>
>>> You changed the access from some constructors. I feel we should have
>>> some global guideline for this, "which files are owned by Avi" maybe isn't
>>> the best guideline :-)
>>>
>>>
>>>>
>>>> This is a huge issue with MakeUnique and std::make_unique, but it's a
>>>> fundamental design issue that we can't address here.
>>>>
>>>> https://codereview.chromium.org/2846333004/
>>>>
>>>
>>>
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2017-05-03 15:49:23]

Ok, so you're saying you're fine with changing dtor visibility if the base
class already has a private dtor, but you're not fine with changing ctor
visibility? (That'd make sense to me.)

Sorry about being slow.

On Wed, May 3, 2017 at 11:42 AM, Avi Drissman <avi@chromium.org> wrote:

> I didn't do it needlessly. The API for SupportsUserData is being changed
> to use unique_ptr for ownership semantics, and unique_ptr needs access to
> the destructor..
>
> On Wed, May 3, 2017 at 11:37 AM, Nico Weber <thakis@chromium.org> wrote:
>
>> On Wed, May 3, 2017 at 11:31 AM, Avi Drissman <avi@chromium.org> wrote:
>>
>>> No, I changed the access of destructors.
>>>
>>> I believe I was making them conform to the style guide. The classes
>>> publicly inherited from a base class that had a public destructor, so I
>>> made the overriding destructor public too. I don't see a problem with that.
>>>
>>
>> Wait, you changed them needlessly? If so,
>>
>> a) please do changes like this in separate changes next time
>> b) restricting method visibility in subclasses isn't uncommon, see this
>> thread: https://groups.google.com/a/chromium.org/forum/#!top
>> ic/chromium-dev/XdsXTHy9lis
>>
>>
>>>
>>> On Wed, May 3, 2017 at 11:28 AM, Nico Weber <thakis@chromium.org> wrote:
>>>
>>>> On Tue, May 2, 2017 at 8:04 PM, <avi@chromium.org> wrote:
>>>>
>>>>> To be clear, every time I used WrapUnique, it was deliberate.
>>>>> MakeUnique doesn't
>>>>> work when the constructor is private, and I'm not going to start
>>>>> changing the
>>>>> member access of constructors of code I don't own.
>>>>>
>>>>
>>>> You changed the access from some constructors. I feel we should have
>>>> some global guideline for this, "which files are owned by Avi" maybe isn't
>>>> the best guideline :-)
>>>>
>>>>
>>>>>
>>>>> This is a huge issue with MakeUnique and std::make_unique, but it's a
>>>>> fundamental design issue that we can't address here.
>>>>>
>>>>> https://codereview.chromium.org/2846333004/
>>>>>
>>>>
>>>>
>>>
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Avi (use Gerrit), 2017-05-03 16:01:10]

If the base class already has a _public_ dtor, I'm fine with changing a
derived class to have a public dtor as well, as it gains us better
ownership correctness with scoped_ptr. Existing code already had the
ability to destroy the object by casting to the base class first, so
nothing is lost.

I would _never_ change the visibility of the ctor in maintenance work. I'm
not going to second-guess the author of the code.

I hope this is not controversial. If it is then I ask that we have a
discussion about it on a larger forum and come to a conclusion there rather
than on a CL.

--

The real issue here is that we prefer unique_ptr. It has _huge_ gains in
ownership semantic clarity, and the switch to it has fixed real bugs. The
problems are that 1) it requires public destructors at the level of the
class to which the pointer is made, and 2) make_unique (or our MakeUnique)
doesn't work when the constructor is private, even if the code calling
make_unique has the privilege of calling the constructor. IMO, the
inability of a caller to delegate the privilege of construction of the
object to the helper function make_unique is a flaw that makes make_unique
much less useful than it ought to be.

Avi

On Wed, May 3, 2017 at 11:49 AM, Nico Weber <thakis@chromium.org> wrote:

> Ok, so you're saying you're fine with changing dtor visibility if the base
> class already has a private dtor, but you're not fine with changing ctor
> visibility? (That'd make sense to me.)
>
> Sorry about being slow.
>
> On Wed, May 3, 2017 at 11:42 AM, Avi Drissman <avi@chromium.org> wrote:
>
>> I didn't do it needlessly. The API for SupportsUserData is being changed
>> to use unique_ptr for ownership semantics, and unique_ptr needs access to
>> the destructor..
>>
>> On Wed, May 3, 2017 at 11:37 AM, Nico Weber <thakis@chromium.org> wrote:
>>
>>> On Wed, May 3, 2017 at 11:31 AM, Avi Drissman <avi@chromium.org> wrote:
>>>
>>>> No, I changed the access of destructors.
>>>>
>>>> I believe I was making them conform to the style guide. The classes
>>>> publicly inherited from a base class that had a public destructor, so I
>>>> made the overriding destructor public too. I don't see a problem with
that.
>>>>
>>>
>>> Wait, you changed them needlessly? If so,
>>>
>>> a) please do changes like this in separate changes next time
>>> b) restricting method visibility in subclasses isn't uncommon, see this
>>> thread: https://groups.google.com/a/chromium.org/forum/#!top
>>> ic/chromium-dev/XdsXTHy9lis
>>>
>>>
>>>>
>>>> On Wed, May 3, 2017 at 11:28 AM, Nico Weber <thakis@chromium.org>
>>>> wrote:
>>>>
>>>>> On Tue, May 2, 2017 at 8:04 PM, <avi@chromium.org> wrote:
>>>>>
>>>>>> To be clear, every time I used WrapUnique, it was deliberate.
>>>>>> MakeUnique doesn't
>>>>>> work when the constructor is private, and I'm not going to start
>>>>>> changing the
>>>>>> member access of constructors of code I don't own.
>>>>>>
>>>>>
>>>>> You changed the access from some constructors. I feel we should have
>>>>> some global guideline for this, "which files are owned by Avi" maybe
isn't
>>>>> the best guideline :-)
>>>>>
>>>>>
>>>>>>
>>>>>> This is a huge issue with MakeUnique and std::make_unique, but it's a
>>>>>> fundamental design issue that we can't address here.
>>>>>>
>>>>>> https://codereview.chromium.org/2846333004/
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.