declaring ownership

[Thiago Farina]

Hi chromiumers,

I was reading TranslateBubbleView [1] and noticed that its constructor takes scoped_ptr<TranslateBubbleModel> and that TBV owns the model as it has a |model_| member variable also declared as scoped_ptr.

Is this a good pratice (i.e., the ctor taking scoped_ptr)?

I'm also asking because when reading ConfirmBubbleViews [2], it also owns the model, but it takes a raw pointer in the ctor.

[1] - https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/ui/views/translate/translate_bubble_view.h

[2] - https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/ui/views/confirm_bubble_views.h

-- 

Thiago Farina

[Peter Kasting]

On Thu, Dec 11, 2014 at 6:01 PM, Thiago Farina <tfa...@chromium.org> wrote:

Hi chromiumers,

I was reading TranslateBubbleView [1] and noticed that its constructor takes scoped_ptr<TranslateBubbleModel> and that TBV owns the model as it has a |model_| member variable also declared as scoped_ptr.

Is this a good pratice (i.e., the ctor taking scoped_ptr)?

Passing a scoped_ptr is generally a good practice if you're trying to transfer ownership from caller to callee.  Using a raw pointer doesn't make it clear what the ownership effect is, so you need comments, and it's possbile to get it wrong.  Passing scoped_ptr is harder to misuse.

The majority of our code doesn't follow this practice for historical reasons (or no good reason).  In general, we could change most code to doing this.

PK

[Avi Drissman]

You are correct that we're not consistent. What TranslateBubbleView does is preferred, as it enforces ownership with the compiler, and makes ownership clear, as opposed to ConfirmBubbleViews where you have to dig in the code to understand what it's trying to do, and you don't have the compiler to enforce it.

Avi

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Thiago Farina]

On Fri, Dec 12, 2014 at 12:18 AM, Avi Drissman <a...@chromium.org> wrote:

You are correct that we're not consistent. What TranslateBubbleView does is preferred, as it enforces ownership with the compiler, and makes ownership clear, as opposed to ConfirmBubbleViews where you have to dig in the code to understand what it's trying to do, and you don't have the compiler to enforce it.

Thanks both for the comments.

I have uploaded https://codereview.chromium.org/801043003 for review.

Regards,

--

Thiago Farina