Fix layer leak in SystemTrayBubble

Fix layer leak in SystemTrayBubble
This is a bit ugly, but less ugly than 1st patch.

BUG=164676, 166862
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=174142

[sadrul, 2012-12-19 23:50:33]

https://codereview.chromium.org/11647022/diff/4001/ash/system/tray/system_tra...
File ash/system/tray/system_tray_bubble.cc (right):

https://codereview.chromium.org/11647022/diff/4001/ash/system/tray/system_tra...
ash/system/tray/system_tray_bubble.cc:158: ui::Layer* layer =
bubble_view_->RecreateLayer();
Does it make sense to make |layer| a scoped_ptr, and then do a release() when
creating AnimationObserverDeleteLayer?

[oshima, 2012-12-20 00:05:19]

https://codereview.chromium.org/11647022/diff/4001/ash/system/tray/system_tra...
File ash/system/tray/system_tray_bubble.cc (right):

https://codereview.chromium.org/11647022/diff/4001/ash/system/tray/system_tra...
ash/system/tray/system_tray_bubble.cc:158: ui::Layer* layer =
bubble_view_->RecreateLayer();
On 2012/12/19 23:50:33, sadrul wrote:
> Does it make sense to make |layer| a scoped_ptr, and then do a release() when
> creating AnimationObserverDeleteLayer?

As I mentioned in the bug description, layer is being used after it's passed to
AnimationObserverDeleteLayer, so
I can't simply do new AnimationObserverDeleteLayer(layer.release());

I tried Pass(), but it didnt' work either.

[sadrul, 2012-12-20 00:08:01]

https://codereview.chromium.org/11647022/diff/4001/ash/system/tray/system_tra...
File ash/system/tray/system_tray_bubble.cc (right):

https://codereview.chromium.org/11647022/diff/4001/ash/system/tray/system_tra...
ash/system/tray/system_tray_bubble.cc:158: ui::Layer* layer =
bubble_view_->RecreateLayer();
On 2012/12/20 00:05:19, oshima wrote:
> On 2012/12/19 23:50:33, sadrul wrote:
> > Does it make sense to make |layer| a scoped_ptr, and then do a release()
when
> > creating AnimationObserverDeleteLayer?
> 
> As I mentioned in the bug description, layer is being used after it's passed
to
> AnimationObserverDeleteLayer, so
> I can't simply do new AnimationObserverDeleteLayer(layer.release());
> 
> I tried Pass(), but it didnt' work either.
> 

release() wouldn't destroy the layer, right? So it should be OK for it to be
used in AnimationObserverDeleteLayer?

[oshima, 2012-12-20 00:12:24]

On 2012/12/20 00:08:01, sadrul wrote:
>
https://codereview.chromium.org/11647022/diff/4001/ash/system/tray/system_tra...
> File ash/system/tray/system_tray_bubble.cc (right):
> 
>
https://codereview.chromium.org/11647022/diff/4001/ash/system/tray/system_tra...
> ash/system/tray/system_tray_bubble.cc:158: ui::Layer* layer =
> bubble_view_->RecreateLayer();
> On 2012/12/20 00:05:19, oshima wrote:
> > On 2012/12/19 23:50:33, sadrul wrote:
> > > Does it make sense to make |layer| a scoped_ptr, and then do a release()
> when
> > > creating AnimationObserverDeleteLayer?
> > 
> > As I mentioned in the bug description, layer is being used after it's passed
> to
> > AnimationObserverDeleteLayer, so
> > I can't simply do new AnimationObserverDeleteLayer(layer.release());
> > 
> > I tried Pass(), but it didnt' work either.
> > 
> 
> release() wouldn't destroy the layer, right? So it should be OK for it to be
> used in AnimationObserverDeleteLayer?

It makes it NULL.

[sadrul, 2012-12-20 00:14:37]

On 2012/12/20 00:12:24, oshima wrote:
> On 2012/12/20 00:08:01, sadrul wrote:
> >
>
https://codereview.chromium.org/11647022/diff/4001/ash/system/tray/system_tra...
> > File ash/system/tray/system_tray_bubble.cc (right):
> > 
> >
>
https://codereview.chromium.org/11647022/diff/4001/ash/system/tray/system_tra...
> > ash/system/tray/system_tray_bubble.cc:158: ui::Layer* layer =
> > bubble_view_->RecreateLayer();
> > On 2012/12/20 00:05:19, oshima wrote:
> > > On 2012/12/19 23:50:33, sadrul wrote:
> > > > Does it make sense to make |layer| a scoped_ptr, and then do a release()
> > when
> > > > creating AnimationObserverDeleteLayer?
> > > 
> > > As I mentioned in the bug description, layer is being used after it's
passed
> > to
> > > AnimationObserverDeleteLayer, so
> > > I can't simply do new AnimationObserverDeleteLayer(layer.release());
> > > 
> > > I tried Pass(), but it didnt' work either.
> > > 
> > 
> > release() wouldn't destroy the layer, right? So it should be OK for it to be
> > used in AnimationObserverDeleteLayer?
> 
> It makes it NULL.

From scoped_ptr doc:

//   {
//     scoped_ptr<Foo> foo;          // No pointer managed.
//     foo.reset(new Foo("wee"));    // Now a pointer is managed.
//     foo.reset(new Foo("wee2"));   // Foo("wee") was destroyed.
//     foo.reset(new Foo("wee3"));   // Foo("wee2") was destroyed.
//     foo->Method();                // Foo::Method() called.
//     foo.get()->Method();          // Foo::Method() called.
//     SomeFunc(foo.release());      // SomeFunc takes ownership, foo no longer
//                                   // manages a pointer.
//     foo.reset(new Foo("wee4"));   // foo manages a pointer again.
//     foo.reset();                  // Foo("wee4") destroyed, foo no longer
//                                   // manages a pointer.

Note how foo.release() is used to send the pointer to SomeFunc

[oshima, 2012-12-20 00:21:01]

You probably misunderstood my comment. I uploaded alternative approach that
you probably like better. PTAL.


On Wed, Dec 19, 2012 at 4:14 PM, <sadrul@chromium.org> wrote:

> On 2012/12/20 00:12:24, oshima wrote:
>
>  On 2012/12/20 00:08:01, sadrul wrote:
>> >
>>
>
> https://codereview.chromium.**org/11647022/diff/4001/ash/**
>
system/tray/system_tray_**bubble.cc<https://codereview.chromium.org/11647022/diff/4001/ash/system/tray/system_tray_bubble.cc>
>
>> > File ash/system/tray/system_tray_**bubble.cc (right):
>> >
>> >
>>
>
> https://codereview.chromium.**org/11647022/diff/4001/ash/**
>
system/tray/system_tray_**bubble.cc#newcode158<https://codereview.chromium.org/11647022/diff/4001/ash/system/tray/system_tray_bubble.cc#newcode158>
>
>> > ash/system/tray/system_tray_**bubble.cc:158: ui::Layer* layer =
>> > bubble_view_->RecreateLayer();
>> > On 2012/12/20 00:05:19, oshima wrote:
>> > > On 2012/12/19 23:50:33, sadrul wrote:
>> > > > Does it make sense to make |layer| a scoped_ptr, and then do a
>> release()
>> > when
>> > > > creating AnimationObserverDeleteLayer?
>> > >
>> > > As I mentioned in the bug description, layer is being used after it's
>>
> passed
>
>> > to
>> > > AnimationObserverDeleteLayer, so
>> > > I can't simply do new AnimationObserverDeleteLayer(**
>> layer.release());
>> > >
>> > > I tried Pass(), but it didnt' work either.
>> > >
>> >
>> > release() wouldn't destroy the layer, right? So it should be OK for it
>> to be
>> > used in AnimationObserverDeleteLayer?
>>
>
>  It makes it NULL.
>>
>
> From scoped_ptr doc:
>
> //   {
> //     scoped_ptr<Foo> foo;          // No pointer managed.
> //     foo.reset(new Foo("wee"));    // Now a pointer is managed.
> //     foo.reset(new Foo("wee2"));   // Foo("wee") was destroyed.
> //     foo.reset(new Foo("wee3"));   // Foo("wee2") was destroyed.
> //     foo->Method();                // Foo::Method() called.
> //     foo.get()->Method();          // Foo::Method() called.
> //     SomeFunc(foo.release());      // SomeFunc takes ownership, foo no
> longer
> //                                   // manages a pointer.
> //     foo.reset(new Foo("wee4"));   // foo manages a pointer again.
> //     foo.reset();                  // Foo("wee4") destroyed, foo no
> longer
> //                                   // manages a pointer.
>
> Note how foo.release() is used to send the pointer to SomeFunc
>
>
https://chromiumcodereview.**appspot.com/11647022/<https://chromiumcodereview...
>

[sadrul, 2012-12-20 00:26:09]

On 2012/12/20 00:21:01, oshima wrote:
> You probably misunderstood my comment. I uploaded alternative approach that
> you probably like better. PTAL.

I did misunderstand your comment, sorry.

Yes, I like this one better. Thanks! LGTM

[commit-bot: I haz the power, 2012-12-20 00:30:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/11647022/10001

[commit-bot: I haz the power, 2012-12-20 02:57:12]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2012-12-20 11:33:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/11647022/10001

[commit-bot: I haz the power, 2012-12-20 12:13:37]

Change committed as 174142