Make GTK file dialog box modal for parent window, instead of for the entire...

Make GTK file dialog box modal for parent window, instead of for the entire
application.

This works by adding the GtkWindow in BrowserWindowGtk to its own
GtkWindowGroup and adding the SelectFile dialog to the same group. The
following call to gtk_grab_add(...) makes the SelectFile dialog modal, but
only to the windows within the same group.

Similarly, the bookmark manager window is also added to its own unique
GtkWindowGroup, so the import/export dialogs behave correctly.

If I'm understanding things correctly, the GtkWindowGroup objects are
reference counted once the have windows attached to them, and will delete
themselves after all references to them are destroyed. I'm not sure how to
verify this.

Test:

- Open two new chrome window: A and B
- Open "Save file as..." dialog in window A
- Verify that window A does not respond to keyboard or mouse events.
- Verify that window B does responde to keyboard and mouse events.
- Open "Save file as..." dialog in window B
- Verify that window B does not respond to keyboard or mouse events.
- Cancel dialog on window A.
- Verify that window A starts responding to keyboard and mouse events.
- Cancel dialog on window B.
- Verify that window B starts responding to keyboard and mouse events.

Test 2:
- Verify that <select> tag allows for correct selection of items.

Test 3:
- Click bookmark star and verify that info bubble works correctly.

BUG=8727
TEST=none

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

[tony, 2009-07-13 18:00:34]

Adding some reviewers so someone will look at this patch.  In the future, try
svn blame to get a list of potential reviewers.

[Evan Martin, 2009-07-13 18:00:55]

I don't believe this actually works.  Setting transient_for just is a hint to
the window manager.  Have you verified you can't interact with windows in the
background with this change?

[mohit_muthanna.com, 2009-07-13 18:05:21]

Yep, I can open the dialog with one window, and interact with other windows.
(This wasn't the case before.)

On Jul 13, 2009 2:00 PM, <evan@chromium.org> wrote:

I don't believe this actually works.  Setting transient_for just is a
hint to the window manager.  Have you verified you can't interact with
windows in the background with this change?

http://codereview.chromium.org/149548

[Evan Martin, 2009-07-13 18:05:53]

No, my question is whether it properly blocks interaction with its parent
window.  (That is, whether it's actually modal...)

[mohit_muthanna.com, 2009-07-13 18:14:37]

Ah, I see. I'll test that and get back to you shortly.

On Jul 13, 2009 2:05 PM, <evan@chromium.org> wrote:

No, my question is whether it properly blocks interaction with its
parent window.  (That is, whether it's actually modal...)

http://codereview.chromium.org/149548

[Mohit Muthanna Cheppudira, 2009-07-13 18:38:00]

Okay, I just tested this and it does block interaction with it's parent
window. From what I can tell, it's actually modal.

Mohit.

On Mon, Jul 13, 2009 at 2:05 PM, <evan@chromium.org> wrote:

> No, my question is whether it properly blocks interaction with its
> parent window.  (That is, whether it's actually modal...)
>
>
> http://codereview.chromium.org/149548
>



-- 
Mohit Muthanna [mohit (at) muthanna (uhuh) com]

[Evan Martin, 2009-07-13 19:30:10]

Sorry to be so skeptical, but I could swear that gtk_window_set_modal exists for
a reason.  :)

Could you try:
- make two windows
- bring up a dialog in one
- click the "x" at the top to close the one with the dialog

[Mohit Muthanna Cheppudira, 2009-07-13 19:39:39]

Turns out your skepticism is not unfounded. This case does fail. :-(

I was only testing with keyboard events, which get captured correctly by the
dialog. The mouse events on the other hand are happily processed by the
parent window.

So... "doh!" and thanks for your time... I'll continue to pursue this bug.

Mohit.

On Mon, Jul 13, 2009 at 3:30 PM, <evan@chromium.org> wrote:

> Sorry to be so skeptical, but I could swear that gtk_window_set_modal
> exists for a reason.  :)
>
> Could you try:
> - make two windows
> - bring up a dialog in one
> - click the "x" at the top to close the one with the dialog
>
>
>
> http://codereview.chromium.org/149548
>



-- 
Mohit Muthanna [mohit (at) muthanna (uhuh) com]

[Mohit Muthanna Cheppudira, 2009-07-13 20:40:46]

I researched this a bit more and found that the right way to do this is with
GtkWindowGroup and gtk_grab_add(...).

I tested it out, and it works well (for real this time :-)

I'll clean the patch up and send it in shortly.

Mohit.

On Mon, Jul 13, 2009 at 3:39 PM, Mohit Muthanna <mohit.muthanna@gmail.com>wrote:

> Turns out your skepticism is not unfounded. This case does fail. :-(
>
> I was only testing with keyboard events, which get captured correctly by
> the dialog. The mouse events on the other hand are happily processed by the
> parent window.
>
> So... "doh!" and thanks for your time... I'll continue to pursue this bug.
>
> Mohit.
>
>
> On Mon, Jul 13, 2009 at 3:30 PM, <evan@chromium.org> wrote:
>
>> Sorry to be so skeptical, but I could swear that gtk_window_set_modal
>> exists for a reason.  :)
>>
>> Could you try:
>> - make two windows
>> - bring up a dialog in one
>> - click the "x" at the top to close the one with the dialog
>>
>>
>>
>> http://codereview.chromium.org/149548
>>
>
>
>
> --
> Mohit Muthanna [mohit (at) muthanna (uhuh) com]
>



-- 
Mohit Muthanna [mohit (at) muthanna (uhuh) com]

[Evan Stade, 2009-07-13 20:42:38]

On 2009/07/13 20:40:46, Mohit Muthanna Cheppudira wrote:
> I researched this a bit more and found that the right way to do this is with
> GtkWindowGroup and gtk_grab_add(...).
> 
> I tested it out, and it works well (for real this time :-)
> 
> I'll clean the patch up and send it in shortly.
> 
> Mohit.
> 
> On Mon, Jul 13, 2009 at 3:39 PM, Mohit Muthanna
<mohit.muthanna@gmail.com>wrote:
> 
> > Turns out your skepticism is not unfounded. This case does fail. :-(
> >
> > I was only testing with keyboard events, which get captured correctly by
> > the dialog. The mouse events on the other hand are happily processed by the
> > parent window.
> >
> > So... "doh!" and thanks for your time... I'll continue to pursue this bug.
> >
> > Mohit.
> >
> >
> > On Mon, Jul 13, 2009 at 3:30 PM, <evan@chromium.org> wrote:
> >
> >> Sorry to be so skeptical, but I could swear that gtk_window_set_modal
> >> exists for a reason.  :)
> >>
> >> Could you try:
> >> - make two windows
> >> - bring up a dialog in one
> >> - click the "x" at the top to close the one with the dialog
> >>
> >>
> >>
> >> http://codereview.chromium.org/149548
> >>
> >
> >
> >
> > --
> > Mohit Muthanna [mohit (at) muthanna (uhuh) com]
> >
> 
> 
> 
> -- 
> Mohit Muthanna [mohit (at) muthanna (uhuh) com]
> 

the problem is that GtkWindowGroup is only Since 2.14. We are targeting 2.12.

[Evan Martin, 2009-07-13 20:43:02]

Note, from the original bug, that GtkWindowGroup isn't available on the system
we're targeting.  We discussed manually reimplementing its functionality (if
possible), but if you want you can just wrap it in #ifdefs based on the GTK
version or something.

[Mohit Muthanna Cheppudira, 2009-07-13 21:07:23]

It seems that GtkWindowGroup has been available since 2.4.

I have it working on my machine which looks like it has GTK 2.10. Any idea
how I can verify this?

$ ldd chrome
        linux-gate.so.1 =>  (0xffffe000)
        libgtk-x11-2.0.so.0 => /usr/lib32/libgtk-x11-2.0.so.0 (0xf7b89000)
        libgdk-x11-2.0.so.0 => /usr/lib32/libgdk-x11-2.0.so.0 (0xf7b05000)

$ ls -l /usr/lib32/gtk-2.0/
total 8
drwxr-xr-x 8 root root 4096 Jan 21 11:08 2.10.0
drwxr-xr-x 2 root root 4096 Jun 23 04:51 modules

On Mon, Jul 13, 2009 at 4:43 PM, <evan@chromium.org> wrote:

> Note, from the original bug, that GtkWindowGroup isn't available on the
> system we're targeting.  We discussed manually reimplementing its
> functionality (if possible), but if you want you can just wrap it in
> #ifdefs based on the GTK version or something.
>
>
> http://codereview.chromium.org/149548
>



-- 
Mohit Muthanna [mohit (at) muthanna (uhuh) com]

[Mohit Muthanna Cheppudira, 2009-07-13 21:24:16]

Okay, pkg-config tells me that I have version 2.12.9:

 $ pkg-config --modversion gtk+-x11-2.0
2.12.9

Since it works correctly on my  machine, I've wrapped the code in #if
GTK_CHECK_VERSION(2, 12, 19).

PTAL. (I've updated the description with testing details).
Thanks,
Mohit.

On Mon, Jul 13, 2009 at 5:06 PM, Mohit Muthanna <mohit.muthanna@gmail.com>wrote:

> It seems that GtkWindowGroup has been available since 2.4.
>
> I have it working on my machine which looks like it has GTK 2.10. Any idea
> how I can verify this?
>
> $ ldd chrome
>         linux-gate.so.1 =>  (0xffffe000)
>         libgtk-x11-2.0.so.0 => /usr/lib32/libgtk-x11-2.0.so.0 (0xf7b89000)
>         libgdk-x11-2.0.so.0 => /usr/lib32/libgdk-x11-2.0.so.0 (0xf7b05000)
>
> $ ls -l /usr/lib32/gtk-2.0/
> total 8
> drwxr-xr-x 8 root root 4096 Jan 21 11:08 2.10.0
> drwxr-xr-x 2 root root 4096 Jun 23 04:51 modules
>
>
> On Mon, Jul 13, 2009 at 4:43 PM, <evan@chromium.org> wrote:
>
>> Note, from the original bug, that GtkWindowGroup isn't available on the
>> system we're targeting.  We discussed manually reimplementing its
>> functionality (if possible), but if you want you can just wrap it in
>> #ifdefs based on the GTK version or something.
>>
>>
>> http://codereview.chromium.org/149548
>>
>
>
>
> --
> Mohit Muthanna [mohit (at) muthanna (uhuh) com]
>



-- 
Mohit Muthanna [mohit (at) muthanna (uhuh) com]

[Evan Martin, 2009-07-13 21:25:20]

Huh, maybe the GTK docs were wrong!

% objdump -T /usr/lib32/libgtk-x11-2.0.so.0.1200.9 | grep gtk_window_group
00279a70 g    DF .text      0000005e  Base        gtk_window_group_get_type
0027ba30 g    DF .text  000000ed  Base        gtk_window_group_add_window
00279ad0 g    DF .text  0000002d  Base        gtk_window_group_new
0027b950 g    DF .text  000000d8  Base        gtk_window_group_remove_window

[Evan Martin, 2009-07-13 21:26:29]

I think estade should review.  I think we maybe don't want to create a new group
every time the dialog comes up, does that leak?

(Thank you for being patient with us)

[Mohit Muthanna Cheppudira, 2009-07-13 21:32:09]

On Mon, Jul 13, 2009 at 5:26 PM, <evan@chromium.org> wrote:

> I think estade should review.  I think we maybe don't want to create a
> new group every time the dialog comes up, does that leak?


If I understand the GTK object model correctly, it shouldn't leak once it is
attached to a window, since all derivatives of GObject are reference
counted. (maybe estade can confirm?).

Also, to clarify, a new group is not created every time the dialog comes up,
it is only created when a new browser or bookmarks window is opened.


>
> (Thank you for being patient with us)


Likewise!

Mohit.


>
>
> http://codereview.chromium.org/149548
>



-- 
Mohit Muthanna [mohit (at) muthanna (uhuh) com]

[Evan Stade, 2009-07-13 21:42:54]

whaddayaknow

I suspect Evan is right and that window group is leaking. GtkWindowGroups are
not initially unowned so even if the window takes a reference on its group
(which is likely), that will bump the ref count up to 2.

http://codereview.chromium.org/149548/diff/16/17
File chrome/browser/gtk/dialogs_gtk.cc (right):

http://codereview.chromium.org/149548/diff/16/17#newcode208
Line 208: #if GTK_CHECK_VERSION(2, 12, 9)
I don't think this check is necessary since we don't target earlier versions

(same elsewhere)

[Mohit Muthanna Cheppudira, 2009-07-13 21:52:44]

On Mon, Jul 13, 2009 at 5:42 PM, <estade@chromium.org> wrote:

> whaddayaknow
>
> I suspect Evan is right and that window group is leaking.
> GtkWindowGroups are not initially unowned so even if the window takes a
> reference on its group (which is likely), that will bump the ref count
> up to 2.


Makes sense, I'll dig into this some more.


>
>
> http://codereview.chromium.org/149548/diff/16/17
> File chrome/browser/gtk/dialogs_gtk.cc (right):
>
> http://codereview.chromium.org/149548/diff/16/17#newcode208
> Line 208: #if GTK_CHECK_VERSION(2, 12, 9)
> I don't think this check is necessary since we don't target earlier
> versions


Okay, I'll remove these references.

Mohit.

>


>
> (same elsewhere)
>
>
> http://codereview.chromium.org/149548
>



-- 
Mohit Muthanna [mohit (at) muthanna (uhuh) com]

[Mohit Muthanna Cheppudira, 2009-07-14 01:10:02]

On Mon, Jul 13, 2009 at 5:52 PM, Mohit Muthanna <mohit.muthanna@gmail.com>wrote:

>
>
> On Mon, Jul 13, 2009 at 5:42 PM, <estade@chromium.org> wrote:
>
>> whaddayaknow
>>
>> I suspect Evan is right and that window group is leaking.
>> GtkWindowGroups are not initially unowned so even if the window takes a
>> reference on its group (which is likely), that will bump the ref count
>> up to 2.
>
>
> Makes sense, I'll dig into this some more.
>

I went through the code in gtkwindow.c, and it looks like the GtkWindowGroup
references are dropped when windows in the group are destroyed. Also, it
doesn't seem to do anything special in gtk_window_group_new() to have a
valid ref count upon creation.

If I'm reading the code correctly:

gtk_window_group_new(): simply calls g_object_new(), which creates an object
with a floating (uninitialized) reference.
gtk_window_group_add_window(...): causes the refcount of the window group to
init to 1 if its the first time being called, else increments it.
gtk_window_group_remove_window(...): removes all grabs, and drops refcount
of group.
gtk_window_destroy(): calls gtk_group_remove_window(...) on the window group
(which drops the refcount).

So, it seems that when all windows that are part of the group are destroyed,
the GtkWindowGroup's reference count will drop to 0, and the group will be
deleted. (This is assuming that the dialog box is destroyed after it is
done.)

Here is the code for gtkwindow.c:
http://git.testbit.eu/Gtk/tree/gtk/gtkwindow.c

Let me know if I'm misreading something.

Mohit.





>
>
>>
>>
>> http://codereview.chromium.org/149548/diff/16/17
>> File chrome/browser/gtk/dialogs_gtk.cc (right):
>>
>> http://codereview.chromium.org/149548/diff/16/17#newcode208
>> Line 208: #if GTK_CHECK_VERSION(2, 12, 9)
>> I don't think this check is necessary since we don't target earlier
>> versions
>
>
> Okay, I'll remove these references.
>
> Mohit.
>
>>
>
>
>>
>> (same elsewhere)
>>
>>
>> http://codereview.chromium.org/149548
>>
>
>
>
> --
> Mohit Muthanna [mohit (at) muthanna (uhuh) com]
>



-- 
Mohit Muthanna [mohit (at) muthanna (uhuh) com]

[Evan Martin, 2009-07-14 01:13:25]

On 2009/07/14 01:10:02, Mohit Muthanna Cheppudira wrote:
> I went through the code in gtkwindow.c, and it looks like the GtkWindowGroup
> references are dropped when windows in the group are destroyed. Also, it
> doesn't seem to do anything special in gtk_window_group_new() to have a
> valid ref count upon creation.
> 
> If I'm reading the code correctly:
> 
> gtk_window_group_new(): simply calls g_object_new(), which creates an object
> with a floating (uninitialized) reference.

I believe floating references are specific to GtkObjects, not GObjects, while
GtkWindowGroup is the latter.

> gtk_window_group_add_window(...): causes the refcount of the window group to
> init to 1 if its the first time being called, else increments it.
> gtk_window_group_remove_window(...): removes all grabs, and drops refcount
> of group.
> gtk_window_destroy(): calls gtk_group_remove_window(...) on the window group
> (which drops the refcount).
> 
> So, it seems that when all windows that are part of the group are destroyed,
> the GtkWindowGroup's reference count will drop to 0, and the group will be
> deleted. (This is assuming that the dialog box is destroyed after it is
> done.)
> 
> Here is the code for gtkwindow.c:
> http://git.testbit.eu/Gtk/tree/gtk/gtkwindow.c
> 
> Let me know if I'm misreading something.
> 
> Mohit.
> 
> 
> 
> 
> 
> >
> >
> >>
> >>
> >> http://codereview.chromium.org/149548/diff/16/17
> >> File chrome/browser/gtk/dialogs_gtk.cc (right):
> >>
> >> http://codereview.chromium.org/149548/diff/16/17#newcode208
> >> Line 208: #if GTK_CHECK_VERSION(2, 12, 9)
> >> I don't think this check is necessary since we don't target earlier
> >> versions
> >
> >
> > Okay, I'll remove these references.
> >
> > Mohit.
> >
> >>
> >
> >
> >>
> >> (same elsewhere)
> >>
> >>
> >> http://codereview.chromium.org/149548
> >>
> >
> >
> >
> > --
> > Mohit Muthanna [mohit (at) muthanna (uhuh) com]
> >
> 
> 
> 
> -- 
> Mohit Muthanna [mohit (at) muthanna (uhuh) com]
>

[Mohit Muthanna Cheppudira, 2009-07-14 01:24:51]

On Mon, Jul 13, 2009 at 9:13 PM, <evan@chromium.org> wrote:

> On 2009/07/14 01:10:02, Mohit Muthanna Cheppudira wrote:
>
>> I went through the code in gtkwindow.c, and it looks like the
>>
> GtkWindowGroup
>
>> references are dropped when windows in the group are destroyed. Also,
>>
> it
>
>> doesn't seem to do anything special in gtk_window_group_new() to have
>>
> a
>
>> valid ref count upon creation.
>>
>
>  If I'm reading the code correctly:
>>
>
>  gtk_window_group_new(): simply calls g_object_new(), which creates an
>>
> object
>
>> with a floating (uninitialized) reference.
>>
>
> I believe floating references are specific to GtkObjects, not GObjects,
> while GtkWindowGroup is the latter.
>

Okay, there lies my confusion. I didn't notice the distinction.

So, there are two ways I can solve this.

1) Call g_object_unref(...) on the window group upon destruction of the
parent window (BrowserWindow and BookmarkManager).

2) Call g_object_force_floating(...) on the window group upon its creation.

I'm leaning towards (2) because it's the simpler solution, but (1) may be
clearer from a readability perspective.

Any preference?

Mohit.



>
>
>  gtk_window_group_add_window(...): causes the refcount of the window
>>
> group to
>
>> init to 1 if its the first time being called, else increments it.
>> gtk_window_group_remove_window(...): removes all grabs, and drops
>>
> refcount
>
>> of group.
>> gtk_window_destroy(): calls gtk_group_remove_window(...) on the window
>>
> group
>
>> (which drops the refcount).
>>
>
>  So, it seems that when all windows that are part of the group are
>>
> destroyed,
>
>> the GtkWindowGroup's reference count will drop to 0, and the group
>>
> will be
>
>> deleted. (This is assuming that the dialog box is destroyed after it
>>
> is
>
>> done.)
>>
>
>  Here is the code for gtkwindow.c:
>> http://git.testbit.eu/Gtk/tree/gtk/gtkwindow.c
>>
>
>  Let me know if I'm misreading something.
>>
>
>  Mohit.
>>
>
>
>
>
>
>  >
>> >
>> >>
>> >>
>> >> http://codereview.chromium.org/149548/diff/16/17
>> >> File chrome/browser/gtk/dialogs_gtk.cc (right):
>> >>
>> >> http://codereview.chromium.org/149548/diff/16/17#newcode208
>> >> Line 208: #if GTK_CHECK_VERSION(2, 12, 9)
>> >> I don't think this check is necessary since we don't target earlier
>> >> versions
>> >
>> >
>> > Okay, I'll remove these references.
>> >
>> > Mohit.
>> >
>> >>
>> >
>> >
>> >>
>> >> (same elsewhere)
>> >>
>> >>
>> >> http://codereview.chromium.org/149548
>> >>
>> >
>> >
>> >
>> > --
>> > Mohit Muthanna [mohit (at) muthanna (uhuh) com]
>> >
>>
>
>
>
>  --
>> Mohit Muthanna [mohit (at) muthanna (uhuh) com]
>>
>
>
>
>
> http://codereview.chromium.org/149548
>



-- 
Mohit Muthanna [mohit (at) muthanna (uhuh) com]

[Evan Stade, 2009-07-14 01:30:07]

simply unref the group right after you set it

gtk_window_group_add_window(gtk_window_group_new(), window_);
g_object_unref(gtk_window_get_group(window_));

[Mohit Muthanna Cheppudira, 2009-07-14 01:31:10]

On Mon, Jul 13, 2009 at 9:24 PM, Mohit Muthanna <mohit.muthanna@gmail.com>wrote:

> On Mon, Jul 13, 2009 at 9:13 PM, <evan@chromium.org> wrote:
>
>> On 2009/07/14 01:10:02, Mohit Muthanna Cheppudira wrote:
>>
>>> I went through the code in gtkwindow.c, and it looks like the
>>>
>> GtkWindowGroup
>>
>>> references are dropped when windows in the group are destroyed. Also,
>>>
>> it
>>
>>> doesn't seem to do anything special in gtk_window_group_new() to have
>>>
>> a
>>
>>> valid ref count upon creation.
>>>
>>
>>  If I'm reading the code correctly:
>>>
>>
>>  gtk_window_group_new(): simply calls g_object_new(), which creates an
>>>
>> object
>>
>>> with a floating (uninitialized) reference.
>>>
>>
>> I believe floating references are specific to GtkObjects, not GObjects,
>> while GtkWindowGroup is the latter.
>>
>
> Okay, there lies my confusion. I didn't notice the distinction.
>
> So, there are two ways I can solve this.
>
> 1) Call g_object_unref(...) on the window group upon destruction of the
> parent window (BrowserWindow and BookmarkManager).
>
> 2) Call g_object_force_floating(...) on the window group upon its creation.
>
> I'm leaning towards (2) because it's the simpler solution, but (1) may be
> clearer from a readability perspective.
>

There's also:

3) Call g_object_unref(...) on the group immediately after adding the parent
window to the group, which might be the best.

Mohit.


>
> Any preference?
>
> Mohit.
>
>
>
>>
>>
>>  gtk_window_group_add_window(...): causes the refcount of the window
>>>
>> group to
>>
>>> init to 1 if its the first time being called, else increments it.
>>> gtk_window_group_remove_window(...): removes all grabs, and drops
>>>
>> refcount
>>
>>> of group.
>>> gtk_window_destroy(): calls gtk_group_remove_window(...) on the window
>>>
>> group
>>
>>> (which drops the refcount).
>>>
>>
>>  So, it seems that when all windows that are part of the group are
>>>
>> destroyed,
>>
>>> the GtkWindowGroup's reference count will drop to 0, and the group
>>>
>> will be
>>
>>> deleted. (This is assuming that the dialog box is destroyed after it
>>>
>> is
>>
>>> done.)
>>>
>>
>>  Here is the code for gtkwindow.c:
>>> http://git.testbit.eu/Gtk/tree/gtk/gtkwindow.c
>>>
>>
>>  Let me know if I'm misreading something.
>>>
>>
>>  Mohit.
>>>
>>
>>
>>
>>
>>
>>  >
>>> >
>>> >>
>>> >>
>>> >> http://codereview.chromium.org/149548/diff/16/17
>>> >> File chrome/browser/gtk/dialogs_gtk.cc (right):
>>> >>
>>> >> http://codereview.chromium.org/149548/diff/16/17#newcode208
>>> >> Line 208: #if GTK_CHECK_VERSION(2, 12, 9)
>>> >> I don't think this check is necessary since we don't target earlier
>>> >> versions
>>> >
>>> >
>>> > Okay, I'll remove these references.
>>> >
>>> > Mohit.
>>> >
>>> >>
>>> >
>>> >
>>> >>
>>> >> (same elsewhere)
>>> >>
>>> >>
>>> >> http://codereview.chromium.org/149548
>>> >>
>>> >
>>> >
>>> >
>>> > --
>>> > Mohit Muthanna [mohit (at) muthanna (uhuh) com]
>>> >
>>>
>>
>>
>>
>>  --
>>> Mohit Muthanna [mohit (at) muthanna (uhuh) com]
>>>
>>
>>
>>
>>
>> http://codereview.chromium.org/149548
>>
>
>
>
> --
> Mohit Muthanna [mohit (at) muthanna (uhuh) com]
>



-- 
Mohit Muthanna [mohit (at) muthanna (uhuh) com]

[Mohit Muthanna Cheppudira, 2009-07-14 01:32:13]

On Mon, Jul 13, 2009 at 9:30 PM, <estade@chromium.org> wrote:

> simply unref the group right after you set it
>

You beat me to it... :-)

I'll send in an updated patch set tomorrow.

Mohit.


>
> gtk_window_group_add_window(gtk_window_group_new(), window_);
> g_object_unref(gtk_window_get_group(window_));
>
>
> http://codereview.chromium.org/149548
>



-- 
Mohit Muthanna [mohit (at) muthanna (uhuh) com]

[Mohit Muthanna Cheppudira, 2009-07-14 15:44:43]

Done and tested. PTAL.

On Mon, Jul 13, 2009 at 9:31 PM, Mohit Muthanna <mohit.muthanna@gmail.com>wrote:

> On Mon, Jul 13, 2009 at 9:30 PM, <estade@chromium.org> wrote:
>
>> simply unref the group right after you set it
>>
>
> You beat me to it... :-)
>
> I'll send in an updated patch set tomorrow.
>
> Mohit.
>
>
>>
>> gtk_window_group_add_window(gtk_window_group_new(), window_);
>> g_object_unref(gtk_window_get_group(window_));
>>
>>
>> http://codereview.chromium.org/149548
>>
>
>
>
> --
> Mohit Muthanna [mohit (at) muthanna (uhuh) com]
>



-- 
Mohit Muthanna [mohit (at) muthanna (uhuh) com]

[Evan Stade, 2009-07-14 18:41:45]

lgtm, barring objections from Evan M. I will commit this.

[Evan Martin, 2009-07-14 18:46:20]

I am a little scared of gtk_grab_add -- I hope the grab is released on all
possible code paths, but if you tested it, this LGTM as well.  Thanks for
persevering on the change.  :)

[Mohit Muthanna Cheppudira, 2009-07-14 18:54:41]

On Tue, Jul 14, 2009 at 2:46 PM, <evan@chromium.org> wrote:

> I am a little scared of gtk_grab_add -- I hope the grab is released on
> all possible code paths,


Grabs are released upon dialog destruction (by way of
gkt_window_group_remove_window(...)).


> but if you tested it, this LGTM as well.


Yep, I tested various scenarios (as detailed in the issue description).


>
> Thanks for persevering on the change.  :)


Thanks for the review.

Mohit.


>
>
> http://codereview.chromium.org/149548
>



-- 
Mohit Muthanna [mohit (at) muthanna (uhuh) com]

[Mohit Muthanna Cheppudira, 2009-07-14 19:14:08]

Thanks for the review.
I'm happy to look at other modality issues in GTK. I noticed that
src/views/window/window_gtk.cc has the following:
   // TODO(erg): Fix once modality works.
    // BecomeModal();

If there's a related bug, feel free to assign it to me.

Mohit.

On Tue, Jul 14, 2009 at 2:41 PM, <estade@chromium.org> wrote:

> lgtm, barring objections from Evan M. I will commit this.
>
>
> http://codereview.chromium.org/149548
>



-- 
Mohit Muthanna [mohit (at) muthanna (uhuh) com]

[Evan Stade, 2009-07-15 04:25:00]

I reverted this change as it broke dropdown menus. I suspect it will break us
whenever we use gtk_grab_add. A quick grep of the code shows just 2 places:

gtk/info_bubble_gtk.cc
205:  gtk_grab_add(window_);

renderer_host/render_widget_host_view_gtk.cc
412:    gtk_grab_add(view_.get());

thanks for the patch, if you want you can fix these instances and re-submit it.

[Mohit Muthanna Cheppudira, 2009-07-15 12:14:38]

On Wed, Jul 15, 2009 at 12:25 AM, <estade@chromium.org> wrote:

> I reverted this change as it broke dropdown menus. I suspect it will
>

darn it... :-(


> break us whenever we use gtk_grab_add. A quick grep of the code shows
> just 2 places:
>
> gtk/info_bubble_gtk.cc
> 205:  gtk_grab_add(window_);
>
> renderer_host/render_widget_host_view_gtk.cc
> 412:    gtk_grab_add(view_.get());
>
> thanks for the patch, if you want you can fix these instances and
> re-submit it.
>

I'm away from my desktop today, but I'll dig into it tomorrow.

Mohit.


>
>
>
> http://codereview.chromium.org/149548
>



-- 
Mohit Muthanna [mohit (at) muthanna (uhuh) com]

[Mohit Muthanna Cheppudira, 2009-07-15 15:18:41]

On Wed, Jul 15, 2009 at 8:13 AM, Mohit Muthanna <mohit.muthanna@gmail.com>wrote:

> On Wed, Jul 15, 2009 at 12:25 AM, <estade@chromium.org> wrote:
>
>> I reverted this change as it broke dropdown menus. I suspect it will
>>
>
Any idea as to how I can reproduce this breakage?

Mohit.


>
> darn it... :-(
>
>
>> break us whenever we use gtk_grab_add. A quick grep of the code shows
>> just 2 places:
>>
>> gtk/info_bubble_gtk.cc
>> 205:  gtk_grab_add(window_);
>>
>> renderer_host/render_widget_host_view_gtk.cc
>> 412:    gtk_grab_add(view_.get());
>>
>> thanks for the patch, if you want you can fix these instances and
>> re-submit it.
>>
>
> I'm away from my desktop today, but I'll dig into it tomorrow.
>
> Mohit.
>
>
>>
>>
>>
>> http://codereview.chromium.org/149548
>>
>
>
>
> --
> Mohit Muthanna [mohit (at) muthanna (uhuh) com]
>



-- 
Mohit Muthanna [mohit (at) muthanna (uhuh) com]

[Evan Stade, 2009-07-15 18:36:26]

On Wed, Jul 15, 2009 at 3:18 PM, Mohit Muthanna<mohit.muthanna@gmail.com> w=
rote:
> On Wed, Jul 15, 2009 at 8:13 AM, Mohit Muthanna <mohit.muthanna@gmail.com=
>
> wrote:
>>
>> On Wed, Jul 15, 2009 at 12:25 AM, <estade@chromium.org> wrote:
>>>
>>> I reverted this change as it broke dropdown menus. I suspect it will
>
> Any idea as to how I can reproduce this breakage?

use a <select>

>
> Mohit.
>
>>
>> darn it... :-(
>>
>>>
>>> break us whenever we use gtk_grab_add. A quick grep of the code shows
>>> just 2 places:
>>>
>>> gtk/info_bubble_gtk.cc
>>> 205: =A0gtk_grab_add(window_);
>>>
>>> renderer_host/render_widget_host_view_gtk.cc
>>> 412: =A0 =A0gtk_grab_add(view_.get());
>>>
>>> thanks for the patch, if you want you can fix these instances and
>>> re-submit it.
>>
>> I'm away from my desktop today, but I'll dig into it tomorrow.
>>
>> Mohit.
>>
>>>
>>>
>>> http://codereview.chromium.org/149548
>>
>>
>>
>> --
>> Mohit Muthanna [mohit (at) muthanna (uhuh) com]
>
>
>
> --
> Mohit Muthanna [mohit (at) muthanna (uhuh) com]
>

[mohit_muthanna.com, 2009-07-18 15:56:25]

Okay, this is going to be tricky. Using gtk_window_set_modal() on the dialog
does not fix the broken drop down boxes. The effect seems to be the same as
using gtk_grab_add().

It appears that the grab() calls on the drop down widgets affect the default
window group, which is not the same group as the browser window. Since these
widgets are drawn on a GdkWindow and not a GtkWindow, I can't seem to figure
out how to add them to the browser window group.

I'll continue to dig in on Monday.

Mohit.


On Wed, Jul 15, 2009 at 2:35 PM, Evan Stade <estade@chromium.org> wrote:

> On Wed, Jul 15, 2009 at 3:18 PM, Mohit Muthanna<mohit.muthanna@gmail.com>
> wrote:
> > On Wed, Jul 15, 2009 at 8:13 AM, Mohit Muthanna <
> mohit.muthanna@gmail.com>
> > wrote:
> >>
> >> On Wed, Jul 15, 2009 at 12:25 AM, <estade@chromium.org> wrote:
> >>>
> >>> I reverted this change as it broke dropdown menus. I suspect it will
> >
> > Any idea as to how I can reproduce this breakage?
>
> use a <select>
>
> >
> > Mohit.
> >
> >>
> >> darn it... :-(
> >>
> >>>
> >>> break us whenever we use gtk_grab_add. A quick grep of the code shows
> >>> just 2 places:
> >>>
> >>> gtk/info_bubble_gtk.cc
> >>> 205:  gtk_grab_add(window_);
> >>>
> >>> renderer_host/render_widget_host_view_gtk.cc
> >>> 412:    gtk_grab_add(view_.get());
> >>>
> >>> thanks for the patch, if you want you can fix these instances and
> >>> re-submit it.
> >>
> >> I'm away from my desktop today, but I'll dig into it tomorrow.
> >>
> >> Mohit.
> >>
> >>>
> >>>
> >>> http://codereview.chromium.org/149548
> >>
> >>
> >>
> >> --
> >> Mohit Muthanna [mohit (at) muthanna (uhuh) com]
> >
> >
> >
> > --
> > Mohit Muthanna [mohit (at) muthanna (uhuh) com]
> >
>



-- 
Mohit Muthanna [mohit (at) muthanna (uhuh) com]

[Evan Stade, 2009-07-19 07:01:54]

You can add the dropdown window (which is a popup, a particular kind of
GtkWindow---see InitAsPopup) to the browser's window group.

You can get the browser window by asking the parent host view for its toplevel
(use gtk_widget_get_toplevel).

Same approach should be used for the info bubble.

Side note: if you want to understand the distinction between a GdkWindow and a
GtkWindow you can read the GdkWindow docs:
<http://library.gnome.org/devel/gdk/stable/gdk-Windows.html>. It seems to have a
fairly clear description, or you can ping me in chat on Monday if you want.

[Mohit Muthanna Cheppudira, 2009-07-19 13:30:10]

Thanks for the pointer. I'll go through that doc and ping you if I have any
questions.

Mohit.

On Sun, Jul 19, 2009 at 3:01 AM, <estade@chromium.org> wrote:

> You can add the dropdown window (which is a popup, a particular kind of
> GtkWindow---see InitAsPopup) to the browser's window group.
>
> You can get the browser window by asking the parent host view for its
> toplevel (use gtk_widget_get_toplevel).
>
> Same approach should be used for the info bubble.
>
> Side note: if you want to understand the distinction between a GdkWindow
> and a GtkWindow you can read the GdkWindow docs:
> <http://library.gnome.org/devel/gdk/stable/gdk-Windows.html>. It seems
> to have a fairly clear description, or you can ping me in chat on Monday
> if you want.
>
>
> http://codereview.chromium.org/149548
>



-- 
Mohit Muthanna [mohit (at) muthanna (uhuh) com]

[Mohit Muthanna Cheppudira, 2009-07-20 22:46:20]

Okay, all done.

- Added the popup from RenderWidgetHostViewGtk::InitAsPopup() to the
browser's window group.
- Added the info bubble to the browser's window group.
- Changed gtk_grab_add(...) to gtk_window_set_modal(...) in the Save dialog.
This wasn't necessary since the latter function is implemented with the
former, and the behaviour is the same. I used gtk_window_set_modal(...) to
guard against any changes in the modality (or grab) implementation.

Everything seems to work correctly now.

PTAL.

Mohit.

On Sun, Jul 19, 2009 at 9:29 AM, Mohit Muthanna <mohit.muthanna@gmail.com>wrote:

> Thanks for the pointer. I'll go through that doc and ping you if I have any
> questions.
>
> Mohit.
>
>
> On Sun, Jul 19, 2009 at 3:01 AM, <estade@chromium.org> wrote:
>
>> You can add the dropdown window (which is a popup, a particular kind of
>> GtkWindow---see InitAsPopup) to the browser's window group.
>>
>> You can get the browser window by asking the parent host view for its
>> toplevel (use gtk_widget_get_toplevel).
>>
>> Same approach should be used for the info bubble.
>>
>> Side note: if you want to understand the distinction between a GdkWindow
>> and a GtkWindow you can read the GdkWindow docs:
>> <http://library.gnome.org/devel/gdk/stable/gdk-Windows.html>. It seems
>> to have a fairly clear description, or you can ping me in chat on Monday
>> if you want.
>>
>>
>> http://codereview.chromium.org/149548
>>
>
>
>
> --
> Mohit Muthanna [mohit (at) muthanna (uhuh) com]
>



-- 
Mohit Muthanna [mohit (at) muthanna (uhuh) com]

[Evan Martin, 2009-07-20 22:49:17]

http://codereview.chromium.org/149548/diff/1054/2021
File chrome/browser/gtk/dialogs_gtk.cc (right):

http://codereview.chromium.org/149548/diff/1054/2021#newcode209
Line 209: // group as the parent, and calling gtk_grab_add(...)
This comment is no longer correct.

[Evan Stade, 2009-07-20 22:51:07]

can you open a new issue with this patch please?