Aura build fix: Make TooltipManagerViews an observer of parent Widget and

Aura build fix: Make TooltipManagerViews an observer of parent Widget and
stop updating tooltips after the widget is destroyed.

BUG=none
TEST=views_unittests pass

TBR=ben@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107695

[commit-bot: I haz the power, 2011-10-28 06:58:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/varunjain@chromium.org/8414012/1

[commit-bot: I haz the power, 2011-10-28 07:59:48]

Change committed as 107695

[sky, 2011-10-28 16:21:04]

I don't understand this change. Widget owns the TooltipManager. So, how come
TooltipManagerViews needs to listen for when the widget is closing?

[varunjain, 2011-10-28 16:46:32]

On 2011/10/28 16:21:04, sky wrote:
> I don't understand this change. Widget owns the TooltipManager. So, how come
> TooltipManagerViews needs to listen for when the widget is closing?

In the case when WIDGET_OWNS_NATIVE_WIDGET, NativeWidgetAura lives even when the
associated aura::Window is destroyed. OnWidgetClosing is a misleading name as it
is called when the native widget is closing, not the widget itself

[oshima, 2011-10-28 16:55:41]

On Fri, Oct 28, 2011 at 9:46 AM, <varunjain@chromium.org> wrote:

> On 2011/10/28 16:21:04, sky wrote:
>
>> I don't understand this change. Widget owns the TooltipManager. So, how
>> come
>> TooltipManagerViews needs to listen for when the widget is closing?
>>
>
> In the case when WIDGET_OWNS_NATIVE_WIDGET, NativeWidgetAura lives even
> when the
> associated aura::Window is destroyed. OnWidgetClosing is a misleading name
> as it
> is called when the native widget is closing, not the widget itself
>
>
Since tooltip manager needs a window, can you delete tooltip manager in
OnWindowDestroyed instead?

- oshima


>
http://codereview.chromium.**org/8414012/<http://codereview.chromium.org/8414...
>

[varunjain, 2011-10-28 16:57:38]

On 2011/10/28 16:55:41, oshima wrote:
> On Fri, Oct 28, 2011 at 9:46 AM, <mailto:varunjain@chromium.org> wrote:
> 
> > On 2011/10/28 16:21:04, sky wrote:
> >
> >> I don't understand this change. Widget owns the TooltipManager. So, how
> >> come
> >> TooltipManagerViews needs to listen for when the widget is closing?
> >>
> >
> > In the case when WIDGET_OWNS_NATIVE_WIDGET, NativeWidgetAura lives even
> > when the
> > associated aura::Window is destroyed. OnWidgetClosing is a misleading name
> > as it
> > is called when the native widget is closing, not the widget itself
> >
> >
> Since tooltip manager needs a window, can you delete tooltip manager in
> OnWindowDestroyed instead?

This change effectively stops the tooltip manager. I think deleting the actual
object should happen when the native_widget_aura is deleted as it owns the
tooltip manager object.

> 
> - oshima
> 
> 
> >
>
http://codereview.chromium.**org/8414012/%3Chttp://codereview.chromium.org/84...>
> >

[sky, 2011-10-28 17:50:10]

I agree with Oshima. It's cleaner to destroy the TooltipManager when
we know it shouldn't be used anymore.

  -Scott

On Fri, Oct 28, 2011 at 9:57 AM,  <varunjain@chromium.org> wrote:
> On 2011/10/28 16:55:41, oshima wrote:
>>
>> On Fri, Oct 28, 2011 at 9:46 AM, <mailto:varunjain@chromium.org> wrote:
>
>> > On 2011/10/28 16:21:04, sky wrote:
>> >
>> >> I don't understand this change. Widget owns the TooltipManager. So, how
>> >> come
>> >> TooltipManagerViews needs to listen for when the widget is closing?
>> >>
>> >
>> > In the case when WIDGET_OWNS_NATIVE_WIDGET, NativeWidgetAura lives even
>> > when the
>> > associated aura::Window is destroyed. OnWidgetClosing is a misleading
>> > name
>> > as it
>> > is called when the native widget is closing, not the widget itself
>> >
>> >
>> Since tooltip manager needs a window, can you delete tooltip manager in
>> OnWindowDestroyed instead?
>
> This change effectively stops the tooltip manager. I think deleting the
> actual
> object should happen when the native_widget_aura is deleted as it owns the
> tooltip manager object.
>
>
>> - oshima
>
>
>> >
>
>
http://codereview.chromium.**org/8414012/%3Chttp://codereview.chromium.org/84...>
>>
>> >
>
>
>
> http://codereview.chromium.org/8414012/
>

[oshima, 2011-10-28 17:51:46]

On Fri, Oct 28, 2011 at 9:57 AM, <varunjain@chromium.org> wrote:

> On 2011/10/28 16:55:41, oshima wrote:
>
>  On Fri, Oct 28, 2011 at 9:46 AM, <mailto:varunjain@chromium.org**> wrote:
>>
>
>  > On 2011/10/28 16:21:04, sky wrote:
>> >
>> >> I don't understand this change. Widget owns the TooltipManager. So, how
>> >> come
>> >> TooltipManagerViews needs to listen for when the widget is closing?
>> >>
>> >
>> > In the case when WIDGET_OWNS_NATIVE_WIDGET, NativeWidgetAura lives even
>> > when the
>> > associated aura::Window is destroyed. OnWidgetClosing is a misleading
>> name
>> > as it
>> > is called when the native widget is closing, not the widget itself
>> >
>> >
>> Since tooltip manager needs a window, can you delete tooltip manager in
>> OnWindowDestroyed instead?
>>
>
> This change effectively stops the tooltip manager. I think deleting the
> actual
> object should happen when the native_widget_aura is deleted as it owns the
> tooltip manager object.
>

I think sky's point (which I agree) is that nativew widget owns tooltip, and
native widget knows when it's
being destroyed, there should be no need for tooltip to listen to its ower's
destroy signal.

- oshima


>
>
>  - oshima
>>
>
>
>  >
>>
>
> http://codereview.chromium.****org/8414012/%3Chttp://coderevi**
> ew.chromium.org/8414012/ <http://codereview.chromium.org/8414012/>>
>
>> >
>>
>
>
>
>
http://codereview.chromium.**org/8414012/<http://codereview.chromium.org/8414...
>

[varunjain, 2011-10-28 18:01:51]

On 2011/10/28 17:50:10, sky wrote:
> I agree with Oshima. It's cleaner to destroy the TooltipManager when
> we know it shouldn't be used anymore.

ok.. I have created another CL (also includes revert of this one) in
http://codereview.chromium.org/8418028/

> 
>   -Scott
> 
> On Fri, Oct 28, 2011 at 9:57 AM,  <mailto:varunjain@chromium.org> wrote:
> > On 2011/10/28 16:55:41, oshima wrote:
> >>
> >> On Fri, Oct 28, 2011 at 9:46 AM, <mailto:varunjain@chromium.org> wrote:
> >
> >> > On 2011/10/28 16:21:04, sky wrote:
> >> >
> >> >> I don't understand this change. Widget owns the TooltipManager. So, how
> >> >> come
> >> >> TooltipManagerViews needs to listen for when the widget is closing?
> >> >>
> >> >
> >> > In the case when WIDGET_OWNS_NATIVE_WIDGET, NativeWidgetAura lives even
> >> > when the
> >> > associated aura::Window is destroyed. OnWidgetClosing is a misleading
> >> > name
> >> > as it
> >> > is called when the native widget is closing, not the widget itself
> >> >
> >> >
> >> Since tooltip manager needs a window, can you delete tooltip manager in
> >> OnWindowDestroyed instead?
> >
> > This change effectively stops the tooltip manager. I think deleting the
> > actual
> > object should happen when the native_widget_aura is deleted as it owns the
> > tooltip manager object.
> >
> >
> >> - oshima
> >
> >
> >> >
> >
> >
>
http://codereview.chromium.**org/8414012/%253Chttp://codereview.chromium.org/...>
> >>
> >> >
> >
> >
> >
> > http://codereview.chromium.org/8414012/
> >