Bug 126840 – [GTK] Make MiniBrowser windows non-transient by default

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Adrian Perez]

Instead of making new browser windows transient as soon as they are
created, do ir later when handling the "run-as-modal" signal. This
makes it easier to use the MiniBrowser to do tests involving multiple
windows, and will be useful as well when multi-web-process mode is
enabled for the GTK port.

[Adrian Perez]

Created attachment 220965 [details]
Patch

[WebKit Commit Bot]

Attachment 220965 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/MiniBrowser/gtk/BrowserWindow.c', '--commit-queue']" exit_code: 1
ERROR: Tools/MiniBrowser/gtk/BrowserWindow.c:273:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/MiniBrowser/gtk/BrowserWindow.c:274:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/MiniBrowser/gtk/BrowserWindow.c:706:  Declaration has space between * and variable name in GtkWidget* window  [whitespace/declaration] [3]
ERROR: Tools/MiniBrowser/gtk/BrowserWindow.c:707:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 4 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adrian Perez]

Created attachment 220969 [details]
Patch

[WebKit Commit Bot]

Attachment 220969 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/MiniBrowser/gtk/BrowserWindow.c', '--commit-queue']" exit_code: 1
ERROR: Tools/MiniBrowser/gtk/BrowserWindow.c:706:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adrian Perez]

Created attachment 220970 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 220970 [details]
Patch

I think we can simply make the browser windows independent to each other. If you open two windows, then close the parent, and run-as-modal is emitted on the child window, it will crash because the parent window has already been destroyed.

[Adrian Perez]

(In reply to comment #6)
> (From update of attachment 220970 [details])
> I think we can simply make the browser windows independent to each other. If you open two windows, then close the parent, and run-as-modal is emitted on the child window, it will crash because the parent window has already been destroyed.

Good point, though I think it is more correct to connect to the "destroy"
signal of the parent window, and clear the pointer to it in the child
window. This way closing parent windows before run-as-modal is emitted
will not cause a crash, and the transient-for property is still set when
parent windows do exist.

[Adrian Perez]

Created attachment 220973 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 220973 [details]
Patch

I still think we could just make windows independent to each other, but if we really want to keep the parent window, I think it would be simpler to keep the pointer to the parent window in the BroserWindow struct and use g_object_add_weak_pointer to make sure the parent pointer is NULL when the parent is destroyed.

[Adrian Perez]

(In reply to comment #9)
> (From update of attachment 220973 [details])
> I still think we could just make windows independent to each other, but if we really want to keep the parent window, I think it would be simpler to keep the pointer to the parent window in the BroserWindow struct and use g_object_add_weak_pointer to make sure the parent pointer is NULL when the parent is destroyed.

IMHO it is more correct to try to make the windows set the transient-for
when they are meant to be modal. Not that it will matter much for the
MiniBrowser, but provided that the fix is easy, I am making the change
you suggested to use  g_object_add_weak_pointer() and re-upload.

[Adrian Perez]

Created attachment 220979 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 220979 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=220979&action=review

> Tools/MiniBrowser/gtk/BrowserWindow.c:704
> +static void parentWindowDestroy(GtkWidget *parent_window, gpointer window)
> +{
> +    if (window)
> +        g_object_set_data(G_OBJECT(window), "transient-for", NULL);
> +}

You forgot to remove this.

[Adrian Perez]

Created attachment 220980 [details]
Patch

[Adrian Perez]

(In reply to comment #12)
> (From update of attachment 220979 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220979&action=review
> 
> > Tools/MiniBrowser/gtk/BrowserWindow.c:704
> > +static void parentWindowDestroy(GtkWidget *parent_window, gpointer window)
> > +{
> > +    if (window)
> > +        g_object_set_data(G_OBJECT(window), "transient-for", NULL);
> > +}
> 
> You forgot to remove this.

Certainly, good catch. I have already re-uploaded with this cleaned.

[WebKit Commit Bot]

Comment on attachment 220980 [details]
Patch

Clearing flags on attachment: 220980

Committed r161864: <http://trac.webkit.org/changeset/161864>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.