gtk: Disconnect the accelerator group keys when closing the browser window....

gtk: Disconnect the accelerator group keys when closing the browser window.

BUG=10737
TEST=Watch the valgrind bot stay green.

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

[tony, 2009-08-11 00:33:57]

I think dean was working on this.  You should ask him to review.

[tony, 2009-08-11 00:34:16]

(actually add dean this time)

[Dean McNamee, 2009-08-11 00:57:38]

This looks like:

http://codereview.chromium.org/155921

I just never checked it in.

On Mon, Aug 10, 2009 at 5:34 PM, <tony@chromium.org> wrote:
> (actually add dean this time)
>
> http://codereview.chromium.org/164294
>

[James Hawkins, 2009-08-11 01:17:27]

On 2009/08/11 00:57:38, Dean McNamee wrote:
> This looks like:
> 
> http://codereview.chromium.org/155921
> 
> I just never checked it in.
> 
> On Mon, Aug 10, 2009 at 5:34 PM, <tony@chromium.org> wrote:
> > (actually add dean this time)
> >
> > http://codereview.chromium.org/164294
> >

Can you commit your CL so I can update?  This CL fixes the leakage of the
closures.

[Dean McNamee, 2009-08-11 01:53:01]

If you want, you could just commit yours instead.  It seems like the
closures shouldn't be leaked (when I looked at it) ?

On Mon, Aug 10, 2009 at 6:17 PM, <jhawkins@chromium.org> wrote:
> On 2009/08/11 00:57:38, Dean McNamee wrote:
>>
>> This looks like:
>
>> http://codereview.chromium.org/155921
>
>> I just never checked it in.
>
>> On Mon, Aug 10, 2009 at 5:34 PM, <tony@chromium.org> wrote:
>> > (actually add dean this time)
>> >
>> > http://codereview.chromium.org/164294
>> >
>
> Can you commit your CL so I can update? =A0This CL fixes the leakage of
> the closures.
>
> http://codereview.chromium.org/164294
>

[James Hawkins, 2009-08-11 18:17:54]

On 2009/08/11 01:53:01, Dean McNamee wrote:
> If you want, you could just commit yours instead.  It seems like the
> closures shouldn't be leaked (when I looked at it) ?
> 

Unfortunately, the accel group code in gtk doesn't do any cleanup for us when we
remove the accel group from the window.  We have to disconnect the keys manually
in order for the closures to be freed.

I've modified my CL to match yours in that the browser window now owns the accel
group; it's much cleaner.  Can I get another review from someone on this?

[tony, 2009-08-11 18:43:54]

LGTM.  It's unfortunate that we have to disconnect manually.  It might be
slightly cleaner to g_closure_unref right after we add the closure and then we
wouldn't have to disconnect manually, but I don't have a strong opinion here.

[tony, 2009-08-11 18:44:19]

Oh, also, what's up with the string include.

[James Hawkins, 2009-08-11 19:36:00]

On 2009/08/11 18:44:19, tony wrote:
> Oh, also, what's up with the string include.

That's from the include-what-you-use lint error.  Still lgtm?

[tony, 2009-08-11 19:55:45]

Yeah, LGTM++.