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)
Core
Widget: Gtk
Tracking
()
RESOLVED
FIXED
mozilla42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: karlt, Assigned: karlt)
References
(Blocks 1 open bug)
Details
Attachments
(4 files)
2.60 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
7.56 KB,
patch
|
acomminos
:
review+
|
Details | Diff | Splinter Review |
1.01 KB,
patch
|
acomminos
:
review+
|
Details | Diff | Splinter Review |
849 bytes,
patch
|
karlt
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8629119 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 2•9 years ago
|
||
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)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8629122 -
Flags: review?(acomminos)
Comment 4•9 years ago
|
||
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 5•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8629119 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 6•9 years ago
|
||
(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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c545ed125da https://hg.mozilla.org/integration/mozilla-inbound/rev/14c98ce60b8a https://hg.mozilla.org/integration/mozilla-inbound/rev/7afaa79f611e
Comment 8•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1c545ed125da https://hg.mozilla.org/mozilla-central/rev/14c98ce60b8a https://hg.mozilla.org/mozilla-central/rev/7afaa79f611e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Comment 9•9 years ago
|
||
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 10•9 years ago
|
||
Attachment #8629986 -
Flags: review?(karlt)
Assignee | ||
Comment 11•9 years ago
|
||
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+
Assignee | ||
Comment 12•9 years ago
|
||
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.
Updated•9 years ago
|
Keywords: checkin-needed
Comment 13•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fa40c837b53
Keywords: checkin-needed
Assignee | ||
Comment 14•9 years ago
|
||
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.
Description
•