Closed Bug 1364355 Opened 8 years ago Closed 8 years ago

Implement client side decorations - Support ARGB visuals

Categories

(Core :: Widget: Gtk, enhancement, P3)

Product:
Core
Component:
Widget: Gtk
Version:
50 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: stransky, Assigned: stransky)

References

(Blocks 1 open bug)

Details

(Whiteboard: tpi:+)

Attachments

(3 files)

Bug 1364355 - Enable argb visual for GTK window behind hidden preference mozilla.widget.use-argb-visuals,
59 bytes, text/x-review-board-request
karlt
: review+
Details
Bug 1364355 - Add support for using ARGB windows with OMTC when an X11 compositor is active.
59 bytes, text/x-review-board-request
karlt
: review+
Details
Bug 1364355 - Implement UpdateOpaqueRegion() by gdk_window_set_opaque_region(),
59 bytes, text/x-review-board-request
karlt
: review+
Details
Follow up from Bug 1283299 patches 3,4.
Whiteboard: tpi:+
Comment on attachment 8867135 [details] Bug 1364355 - Enable argb visual for GTK window behind hidden preference mozilla.widget.use-argb-visuals, https://reviewboard.mozilla.org/r/138728/#review146326 ::: widget/gtk/nsWindow.cpp:3619 (Diff revision 1) > + > + // We need to select an ARGB visual here instead of in > + // SetTransparencyMode() because it has to be done before the > + // widget is realized. An ARGB visual is only useful if we > + // are on a compositing window manager. > + if (useAlphaVisual) { Keep the gdk_screen_is_composited(screen) test. (That should ensure that gdk_screen_get_rgba_colormap(screen) returns non-null.)
Attachment #8867135 - Flags: review?(karlt)
Comment on attachment 8867136 [details] Bug 1364355 - Add support for using ARGB windows with OMTC when an X11 compositor is active. https://reviewboard.mozilla.org/r/138730/#review146316 ::: widget/gtk/nsWindow.cpp:6792 (Diff revision 1) > + return gdk_screen_is_composited(gdk_screen_get_default()) && > + (gdk_window_get_visual(mGdkWindow) > + == gdk_screen_get_rgba_visual(gdk_screen_get_default())); Please call gdk_screen_get_default() only once and use the result from a local variable.
Attachment #8867136 - Flags: review?(karlt) → review+
Comment on attachment 8867137 [details] Bug 1364355 - Implement UpdateOpaqueRegion() by gdk_window_set_opaque_region(), https://reviewboard.mozilla.org/r/138732/#review146294 Looks like GtkWindow will call gdk_window_set_opaque_region() on resize from _gtk_window_set_allocation(), but I expect that should be fine because this code will end up overriding that soon enough. ::: widget/gtk/nsWindow.cpp:4375 (Diff revision 1) > > +#if (MOZ_WIDGET_GTK >= 3) > +void nsWindow::UpdateOpaqueRegion(const LayoutDeviceIntRegion& aOpaqueRegion) > +{ > + // Available as of GTK 3.10+ > + static auto sGdkWindowSetOpaqueRegion = (void (*)(GdkWindow*, cairo_region_t *)) Remove space between cairo_region_t and *. Break line after = to keep within 80 columns. ::: widget/gtk/nsWindow.cpp:4378 (Diff revision 1) > +{ > + // Available as of GTK 3.10+ > + static auto sGdkWindowSetOpaqueRegion = (void (*)(GdkWindow*, cairo_region_t *)) > + dlsym(RTLD_DEFAULT, "gdk_window_set_opaque_region"); > + > + if (sGdkWindowSetOpaqueRegion && mGdkWindow) { The documentation for gdk_window_set_opaque_region() says "This function only works for toplevel windows", but it looks like the implementation will apply the region to the toplevel window when called on a child window. The bug is that the offset from parent to child is not applied. This is probably relevant only when GTK is providing CSD, and so adding && gdk_window_get_window_type(mGdkWindow) == GDK_WINDOW_TOPLEVEL to the check here would be sufficient to avoid problems in that case. The other option is to add the offset from mShell to mContainer (assuming that doesn't change too often), and call gdk_window_get_window_type() on the window of mShell, but the GDK_WINDOW_TOPLEVEL check is sufficient for now, at least. ::: widget/gtk/nsWindow.cpp:4383 (Diff revision 1) > + if (sGdkWindowSetOpaqueRegion && mGdkWindow) { > + if (aOpaqueRegion.IsEmpty()) { > + (*sGdkWindowSetOpaqueRegion)(mGdkWindow, nullptr); > + } else { > + cairo_region_t *region = cairo_region_create(); > + for (auto iter = aOpaqueRegion.RectIter(); !iter.Done(); iter.Next()) { Break line after ; to keep within 80 columns.
Attachment #8867137 - Flags: review?(karlt) → review+
Comment on attachment 8867137 [details] Bug 1364355 - Implement UpdateOpaqueRegion() by gdk_window_set_opaque_region(), https://reviewboard.mozilla.org/r/138732/#review146294 > The documentation for gdk_window_set_opaque_region() says "This function only > works for toplevel windows", but it looks like the implementation will apply > the region to the toplevel window when called on a child window. The bug is > that the offset from parent to child is not applied. This is probably > relevant only when GTK is providing CSD, and so adding > && gdk_window_get_window_type(mGdkWindow) == GDK_WINDOW_TOPLEVEL > to the check here would be sufficient to avoid problems in that case. > > The other option is to add the offset from mShell to mContainer (assuming that > doesn't change too often), and call gdk_window_get_window_type() on the window > of mShell, but the GDK_WINDOW_TOPLEVEL check is sufficient for now, at least. Are you sure about the translation from child to parents? I'm looking at the gdk_window_set_opaque_region() implementation and it calls directly impl_class->set_opaque_region() which (for X11) sets directly _NET_WM_OPAQUE_REGION on underlying X11 window. I don't see any transitions here.
Comment on attachment 8867137 [details] Bug 1364355 - Implement UpdateOpaqueRegion() by gdk_window_set_opaque_region(), https://reviewboard.mozilla.org/r/138732/#review146294 > Are you sure about the translation from child to parents? I'm looking at the gdk_window_set_opaque_region() implementation and it calls directly impl_class->set_opaque_region() which (for X11) sets directly _NET_WM_OPAQUE_REGION on underlying X11 window. I don't see any transitions here. Uploaded new patches without this change. Please let me know if you want to address that.
Comment on attachment 8867137 [details] Bug 1364355 - Implement UpdateOpaqueRegion() by gdk_window_set_opaque_region(), https://reviewboard.mozilla.org/r/138732/#review146294 > Uploaded new patches without this change. Please let me know if you want to address that. The bug to which I was referring was that gdk_x11_window_set_opaque_region() applies this to the X11 implementation of the window [1]. The potential transition is that a client-side (and child) window will point through |impl| to the x11 window of an ancestor [2]. However, I had forgotten that Gecko depends on the child window having its own native window which is ensured at [3]. That means the child window will not be a client side window and so will have its own impl window and so will not be affected by this bug. Consistent with "This function only works for toplevel windows" in the documentation [4], I expect the window manager will only look at properties on toplevel windows [5]. So, setting the property on the child window will have no benefit, but no harm either, for Gecko here. Feel free to land with either "&& gdk_window_get_window_type(mGdkWindow) == GDK_WINDOW_TOPLEVEL" to make it clear there is no effect, or as is. The GTK CSD situation can be addressed if/when that is important. [1] https://git.gnome.org/browse/gtk+/tree/gdk/x11/gdkwindow-x11.c?h=3.22.15#n5626 [2] https://git.gnome.org/browse/gtk+/tree/gdk/gdkwindow.c?h=3.22.15#n114 [3] http://searchfox.org/mozilla-central/rev/a14524a72de6f4ff738a5e784970f0730cea03d8/widget/gtk/nsWindow.cpp#3965 [4] https://developer.gnome.org/gdk3/stable/gdk3-Windows.html#gdk-window-set-opaque-region [5] https://tronche.com/gui/x/icccm/sec-4#s-4.1.2
Comment on attachment 8867135 [details] Bug 1364355 - Enable argb visual for GTK window behind hidden preference mozilla.widget.use-argb-visuals, https://reviewboard.mozilla.org/r/138728/#review146760 Thanks :)
Attachment #8867135 - Flags: review?(karlt) → review+
Comment on attachment 8867137 [details] Bug 1364355 - Implement UpdateOpaqueRegion() by gdk_window_set_opaque_region(), https://reviewboard.mozilla.org/r/138732/#review146294 > The bug to which I was referring was that gdk_x11_window_set_opaque_region() > applies this to the X11 implementation of the window [1]. > > The potential transition is that a client-side (and child) window will point > through |impl| to the x11 window of an ancestor [2]. > > However, I had forgotten that Gecko depends on the child window having its own > native window which is ensured at [3]. That means the child window will not > be a client side window and so will have its own impl window and so will not > be affected by this bug. > > Consistent with "This function only works for toplevel windows" in the > documentation [4], I expect the window manager will only look at properties on > toplevel windows [5]. > > So, setting the property on the child window will have no > benefit, but no harm either, for Gecko here. > > Feel free to land with either "&& gdk_window_get_window_type(mGdkWindow) == > GDK_WINDOW_TOPLEVEL" to make it clear there is no effect, or as is. > The GTK CSD situation can be addressed if/when that is important. > > [1] https://git.gnome.org/browse/gtk+/tree/gdk/x11/gdkwindow-x11.c?h=3.22.15#n5626 > [2] https://git.gnome.org/browse/gtk+/tree/gdk/gdkwindow.c?h=3.22.15#n114 > [3] http://searchfox.org/mozilla-central/rev/a14524a72de6f4ff738a5e784970f0730cea03d8/widget/gtk/nsWindow.cpp#3965 > [4] https://developer.gnome.org/gdk3/stable/gdk3-Windows.html#gdk-window-set-opaque-region > [5] https://tronche.com/gui/x/icccm/sec-4#s-4.1.2 You got me, I suspected it wasn't so simple :) Uploaded with the GDK_WINDOW_TOPLEVEL check. Thanks!
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/c4c2645ca8b7 Enable argb visual for GTK window behind hidden preference mozilla.widget.use-argb-visuals, r=karlt https://hg.mozilla.org/integration/autoland/rev/2ee6598089a3 Add support for using ARGB windows with OMTC when an X11 compositor is active. r=karlt https://hg.mozilla.org/integration/autoland/rev/7cc2790574dc Implement UpdateOpaqueRegion() by gdk_window_set_opaque_region(), r=karlt
Keywords: checkin-needed
(In reply to Phil Ringnalda (:philor) from comment #17) > Backed out in https://hg.mozilla.org/integration/autoland/rev/fa6f4aec2a5c > for timeouts in test_panel_anchoradjust.xul, > https://treeherder.mozilla.org/logviewer.html#?job_id=102516717&repo=autoland Seems to be caused by the second patch, investigating.
Looks like related to Bug 630346 - the test passes when CreateBasicLayerManager() is always created for transparent rendering and !IsComposited() check is removed from nsWindow::GetLayerManager(). There's the try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=570caa34cbb10d2a5009db8219061fc5bce6a191 Karl, what do you think?
Flags: needinfo?(karlt)
If it looks similar to symptoms in Bug 630346, then https://bugzilla.mozilla.org/show_bug.cgi?id=1365660#c35 may be related. For the symptoms like bug 630346, I would first check that the surface backing the window is appropriately initialized as transparent. However, I have no idea why a change in compositor would cause timeouts. Bas was looking at similar changes for the WINNT port in https://bugzilla.mozilla.org/show_bug.cgi?id=1356317#c18 and so it may be worth watching that bug report. The !IsComposited() check was what made https://bugzilla.mozilla.org/attachment.cgi?id=8867136 use OMTC on ARGB windows, so there's not a lot of value in that patch if that is removed. I suggest landing the other two patches, and investigating the OMTC ARGB issue in a new bug.
Flags: needinfo?(karlt)
Comment on attachment 8867135 [details] Bug 1364355 - Enable argb visual for GTK window behind hidden preference mozilla.widget.use-argb-visuals, https://reviewboard.mozilla.org/r/138726/#review148710 Okay, let's commit the recent patches without failing part and investigate that in a new bug.
(In reply to Karl Tomlinson (:karlt) from comment #20) > If it looks similar to symptoms in Bug 630346, then > https://bugzilla.mozilla.org/show_bug.cgi?id=1365660#c35 > may be related. > > For the symptoms like bug 630346, I would first check that the surface > backing the window is appropriately initialized as transparent. Yes, when running locally the surface backing the window has black borders instead of rounding shadows. > However, I have no idea why a change in compositor would cause timeouts. > > Bas was looking at similar changes for the WINNT port in > https://bugzilla.mozilla.org/show_bug.cgi?id=1356317#c18 and so it may be > worth watching that bug report. > > The !IsComposited() check was what made > https://bugzilla.mozilla.org/attachment.cgi?id=8867136 > use OMTC on ARGB windows, so there's not a lot of value in that patch if > that is removed. > > I suggest landing the other two patches, and investigating the OMTC ARGB > issue in a new bug. Okay, I submitted the patches to land without the failing part (the !IsComposited() check) to minimize changes in subsequent patches.
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/411f357ea485 Enable argb visual for GTK window behind hidden preference mozilla.widget.use-argb-visuals, r=karlt https://hg.mozilla.org/integration/autoland/rev/d04f932669e7 Add support for using ARGB windows with OMTC when an X11 compositor is active. r=karlt https://hg.mozilla.org/integration/autoland/rev/457e6aea4650 Implement UpdateOpaqueRegion() by gdk_window_set_opaque_region(), r=karlt
Keywords: checkin-needed
(In reply to Karl Tomlinson (:karlt) from comment #20) > I suggest landing the other two patches, and investigating the OMTC ARGB > issue in a new bug. Bug 1369488
Comment on attachment 8867136 [details] Bug 1364355 - Add support for using ARGB windows with OMTC when an X11 compositor is active. https://reviewboard.mozilla.org/r/138730/#review161080
See Also: → 1377950
No longer blocks: 1385586
Blocks: 1392435
No longer blocks: 1392435
See Also: → 1396418
Depends on: 1405267
Assignee: nobody → stransky
Depends on: 1489463
No longer depends on: 1489463
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: