Closed Bug 1180008 Opened 9 years ago Closed 9 years ago

don't measure size of decorations for override-redirect windows

Categories

(Core :: Widget: Gtk, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: karlt, Assigned: karlt)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

override-redirect windows (GTK_WINDOW_POPUP) are positioned by the client, not the window manager.  They do not typically have window manager decorations, and even if they did, the offsets would not be relevant to Gecko's positioning.
_NET_FRAME_EXTENTS is only relevant for managed windows, and so a waste of time for override-redirect windows.

I initially wrote these when trying to understand the proposed solution in bug 1146297 as they may help reduce the number of situations of possible unintended consequences observed in https://bugzilla.mozilla.org/show_bug.cgi?id=1146297#c23 with the first proposed solution (though I'm not clear why the problem happens only with popup windows).  However, I think the changes are worthwhile on their own.
Since https://hg.mozilla.org/mozilla-central/rev/9541dbf6e020#l2.184
there is only one GdkWindow per nsWindow.

The mGdkWindow pointer is cleared in OnContainerUnrealize() before the shell
widget destruction completes:
https://hg.mozilla.org/mozilla-central/annotate/50b95032152c/widget/gtk/nsWindow.cpp#l2480
Attachment #8629120 - Flags: review?(acomminos)
Comment on attachment 8629120 [details] [diff] [review]
use mGdkWindow instead of finding it from gtk_widget_get_window(mShell)

Review of attachment 8629120 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.
Attachment #8629120 - Flags: review?(acomminos) → review+
Comment on attachment 8629122 [details] [diff] [review]
don't measure size of decorations for override-redirect windows

Review of attachment 8629122 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gtk/nsWindow.cpp
@@ +1520,5 @@
>  {
>      PROFILER_LABEL("nsWindow", "GetClientOffset", js::ProfileEntry::Category::GRAPHICS);
>  
> +    if (!mIsTopLevel || !mShell || !mGdkWindow ||
> +        gtk_window_get_window_type(GTK_WINDOW(mShell)) == GTK_WINDOW_POPUP) {

Perhaps it might be better to be more general and use gtk_window_get_decorated (or gdk_window_get_decorations) to determine if a window has no border?
Attachment #8629122 - Flags: review?(acomminos) → review+
Attachment #8629119 - Flags: review?(mh+mozilla) → review+
(In reply to Andrew Comminos [:acomminos] from comment #5)
> Perhaps it might be better to be more general and use
> gtk_window_get_decorated (or gdk_window_get_decorations) to determine if a
> window has no border?

gtk_window_get_decorated() sounds like the function we want, but, AFAICS from GTK source code, it returns true even for GTK_WINDOW_POPUP windows if they have not had gtk_window_set_decorated() explicitly called.

gdk_window_get_decorations would correctly return FALSE for override-redirect windows, but requires a round trip for managed windows.

We could probably use both gtk_window_get_window_type and gtk_window_get_decorated, but the number of instances where the addition of gtk_window_get_decorated would avoid an unnecessary property get is small, and there may be a risk of some window managers decorating when we have asked them not to.
This busts linking on GTK3 builds due to gtk_window_get_window_type not being defined as a stub in mozgtk.c. Uploading a patch for it.
Comment on attachment 8629986 [details] [diff] [review]
Define gtk_window_get_window_type in mozgtk.

Thank you.

I'm not certain whether this is better in COMMON_SYMBOLS or GTK3_SYMBOLS, but I think COMMON_SYMBOLS as you have here should be fine because I expect at least version 2.20 of GTK2 on systems with GTK3.

widget/gtk/compat/gtk/gtkwindow.h should prevent Gecko from using this symbol with GTK2, but there could be a risk of problems if this was in GTK3_SYMBOLS and some plugin or library loaded from the plugin-container was looking for the symbol.
Attachment #8629986 - Flags: review?(karlt) → review+
Improvement: Mozilla-Inbound-Non-PGO - Customization Animation Tests - Ubuntu HW 12.04 x64 - 8.51% decrease
-----------------------------------------------------------------------------------------------------------
    Previous: avg 32.456 stddev 0.496 of 12 runs up to revision c51f9fc02d33
    New     : avg 29.694 stddev 0.343 of 12 runs since revision 797634c4fd75
    Change  : -2.762 (8.51% / z=5.570)
    Graph   : http://mzl.la/1M7AWO9

Improvement: Mozilla-Inbound-Non-PGO - Customization Animation Tests - Ubuntu HW 12.04 - 7.26% decrease
-------------------------------------------------------------------------------------------------------
    Previous: avg 33.946 stddev 0.390 of 12 runs up to revision c51f9fc02d33
    New     : avg 31.481 stddev 0.181 of 12 runs since revision 797634c4fd75
    Change  : -2.464 (7.26% / z=6.317)
    Graph   : http://mzl.la/1M7B00m

I didn't expect these tests to use popup windows.
Improvement: Mozilla-Inbound - Devtools At Maximum Performance - Ubuntu HW 12.04 x64 - 3.28% decrease
-----------------------------------------------------------------------------------------------------
    Previous: avg 253.703 stddev 2.338 of 12 runs up to revision d9074143dc58
    New     : avg 245.384 stddev 1.682 of 12 runs since revision 797634c4fd75
    Change  : -8.319 (3.28% / z=3.559)
    Graph   : http://mzl.la/1eBRpyP
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: