vm_tools: sommelier: Use quirks to enable client window positioning.

To make the behaviour as visible as possible, log a message the first
time a quirk is enabled for a particular window, and set a window
property listing all the quirks that have ever been enabled for that
window.

This CL has no effect without accompanying configuration changes.

BUG=b:296327414
TEST=run unit tests: meson setup builddir -Dquirks=true; pushd builddir; ninja && ./sommelier_test; popd

Change-Id: I581be33775331e3345969ff3d5a0bf31b9a42ce4
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/4903568
Reviewed-by: Nic Hollingum <[email protected]>
Commit-Queue: Nic Hollingum <[email protected]>
Auto-Submit: Chloe Pelling <[email protected]>
Tested-by: Chloe Pelling <[email protected]>

quirks/sommelier-quirks-test.cc

@@ -121,4 +121,24 @@ TEST_F(QuirksTest, AllConditionsMustMatch) {
   EXPECT_FALSE(ctx.quirks.IsEnabled(game123, quirks::FEATURE_X11_MOVE_WINDOWS));
 }
 
+TEST_F(QuirksTest, SetsQuirkAppliedProperty) {
+  xcb.DelegateToFake();
+  sl_window* game123 = CreateToplevelWindow();
+  xcb.create_window(nullptr, 32, game123->id, XCB_WINDOW_NONE, 0, 0, 800, 600,
+                    0, XCB_WINDOW_CLASS_INPUT_OUTPUT, XCB_COPY_FROM_PARENT, 0,
+                    nullptr);
+  game123->steam_game_id = 123;
+
+  ctx.quirks.Load(
+      "sommelier { \n"
+      "  condition { steam_game_id: 123 }\n"
+      "  enable: FEATURE_X11_MOVE_WINDOWS\n"
+      "}");
+  EXPECT_TRUE(ctx.quirks.IsEnabled(game123, quirks::FEATURE_X11_MOVE_WINDOWS));
+
+  EXPECT_EQ(StringPropertyForTesting(
+                game123->id, ctx.atoms[ATOM_SOMMELIER_QUIRK_APPLIED].value),
+            "FEATURE_X11_MOVE_WINDOWS");
+}
+
 }  // namespace vm_tools::sommelier

quirks/sommelier-quirks.cc

@@ -2,16 +2,17 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "builddir/libsommelier.a.p/quirks/quirks.pb.h"
 #include <fcntl.h>
 #include <google/protobuf/io/zero_copy_stream_impl.h>
 #include <google/protobuf/text_format.h>
+#include <unistd.h>
+#include <xcb/xproto.h>
+
 #include "quirks/quirks.pb.h"
 #include "quirks/sommelier-quirks.h"
-#include <set>
+#include "sommelier-ctx.h"     // NOLINT(build/include_directory)
 #include "sommelier-window.h"  // NOLINT(build/include_directory)
-#include <string.h>
-#include <unistd.h>
+#include "xcb/xcb-shim.h"
 
 void Quirks::Load(std::string textproto) {
   google::protobuf::TextFormat::MergeFromString(textproto, &active_config_);
@@ -48,8 +49,34 @@ void Quirks::LoadFromFile(std::string path) {
 }
 
 bool Quirks::IsEnabled(struct sl_window* window, int feature) {
-  return enabled_features_.find(std::make_pair(
-             window->steam_game_id, feature)) != enabled_features_.end();
+  bool is_enabled =
+      enabled_features_.find(std::make_pair(window->steam_game_id, feature)) !=
+      enabled_features_.end();
+
+  // Log enabled quirks once per quirk, per window.
+  if (is_enabled && sl_window_should_log_quirk(window, feature)) {
+    fprintf(stderr,
+            "Quirk %s applied to window 0x%x due to rule `steam_game_id: %d`\n",
+            quirks::Feature_Name(feature).c_str(), window->id,
+            window->steam_game_id);
+
+    // Also set a window property for additional debugging.
+    // Create comma-separated list of quirks
+    std::string all_quirks;
+    for (int f : sl_window_logged_quirks(window)) {
+      all_quirks.append(quirks::Feature_Name(f));
+      all_quirks.append(",");
+    }
+    // Remove trailing comma
+    if (!all_quirks.empty()) {
+      all_quirks.pop_back();
+    }
+    xcb()->change_property(
+        window->ctx->connection, XCB_PROP_MODE_REPLACE, window->id,
+        window->ctx->atoms[ATOM_SOMMELIER_QUIRK_APPLIED].value, XCB_ATOM_STRING,
+        8, all_quirks.size(), all_quirks.data());
+  }
+  return is_enabled;
 }
 
 void Quirks::Update() {

sommelier-ctx.cc

@@ -92,6 +92,8 @@ const char* sl_context_atom_name(int atom_enum) {
       return "STEAM_GAME";
     case ATOM_XWAYLAND_RANDR_EMU_MONITOR_RECTS:
       return "_XWAYLAND_RANDR_EMU_MONITOR_RECTS";
+    case ATOM_SOMMELIER_QUIRK_APPLIED:
+      return "SOMMELIER_QUIRK_APPLIED";
   }
   return nullptr;
 }

sommelier-ctx.h

@@ -63,7 +63,8 @@ enum {
   ATOM_GTK_THEME_VARIANT,
   ATOM_STEAM_GAME,
   ATOM_XWAYLAND_RANDR_EMU_MONITOR_RECTS,
-  ATOM_LAST = ATOM_XWAYLAND_RANDR_EMU_MONITOR_RECTS,
+  ATOM_SOMMELIER_QUIRK_APPLIED,
+  ATOM_LAST = ATOM_SOMMELIER_QUIRK_APPLIED,
 };
 
 struct sl_context {

sommelier-window.cc

@@ -16,6 +16,10 @@
 #include "aura-shell-client-protocol.h"  // NOLINT(build/include_directory)
 #include "xdg-shell-client-protocol.h"   // NOLINT(build/include_directory)
 
+#ifdef QUIRKS_SUPPORT
+#include "quirks/sommelier-quirks.h"
+#endif
+
 #define XID_APPLICATION_ID_FORMAT APPLICATION_ID_FORMAT_PREFIX ".xid.%d"
 #define WM_CLIENT_LEADER_APPLICATION_ID_FORMAT \
   APPLICATION_ID_FORMAT_PREFIX ".wmclientleader.%d"
@@ -458,7 +462,7 @@ static const struct wl_callback_listener configure_event_barrier_listener = {
 void sl_toplevel_send_window_bounds_to_host(struct sl_window* window) {
   // Don't send window bounds if fullscreen/maximized/resizing,
   // or if the feature is unsupported by the host or disabled by flag.
-  if (!window->allow_resize || !window->ctx->enable_x11_move_windows ||
+  if (!window->allow_resize || !sl_window_is_client_positioned(window) ||
       !window->ctx->aura_shell ||
       window->ctx->aura_shell->version <
           ZAURA_TOPLEVEL_SET_WINDOW_BOUNDS_SINCE_VERSION ||
@@ -694,8 +698,9 @@ void sl_window_update(struct sl_window* window) {
                                 &sl_internal_xdg_toplevel_listener, window);
     }
 
-    // aura_toplevel is only needed for the --enable-x11-move-windows case
-    // right now. Setting it up means we get x and y coordinates in configure
+    // Right now, aura_toplevel is only needed for windows positioned by the
+    // client (which is all windows if --enable-x11-move-windows is passed).
+    // Setting it up means we get x and y coordinates in configure
     // events (aura_toplevel.configure replaces xdg_toplevel.configure), which
     // changes how windows are positioned in the X server's coordinate space. If
     // windows end up partially offscreen in that space, we get bugs like
@@ -709,7 +714,7 @@ void sl_window_update(struct sl_window* window) {
     // coordinates, but for now the most conservative approach is to avoid using
     // aura_toplevel entirely. This can be revisited later if we need
     // aura_toplevel for anything else.
-    if (ctx->enable_x11_move_windows && ctx->aura_shell &&
+    if (sl_window_is_client_positioned(window) && ctx->aura_shell &&
         window->xdg_toplevel && !window->aura_toplevel) {
       window->aura_toplevel = zaura_shell_get_aura_toplevel_for_xdg_toplevel(
           ctx->aura_shell->internal, window->xdg_toplevel);
@@ -786,3 +791,22 @@ void sl_window_update(struct sl_window* window) {
   if (host_surface->contents_width && host_surface->contents_height)
     window->realized = 1;
 }
+
+#ifdef QUIRKS_SUPPORT
+bool sl_window_should_log_quirk(struct sl_window* window, int feature_enum) {
+  return window->logged_quirks.insert(feature_enum).second;
+}
+
+std::set<int> sl_window_logged_quirks(struct sl_window* window) {
+  return window->logged_quirks;
+}
+#endif
+
+bool sl_window_is_client_positioned(struct sl_window* window) {
+#ifdef QUIRKS_SUPPORT
+  if (window->ctx->quirks.IsEnabled(window, quirks::FEATURE_X11_MOVE_WINDOWS)) {
+    return true;
+  }
+#endif
+  return window->ctx->enable_x11_move_windows;
+}

sommelier-window.h

@@ -8,6 +8,7 @@
 #include <pixman.h>
 #include <wayland-client-protocol.h>
 #include <wayland-server-core.h>
+#include <set>
 #include <string>
 #include <xcb/xcb.h>
 
@@ -88,6 +89,12 @@ struct sl_window {
   int max_width = 0;
   int max_height = 0;
 
+#ifdef QUIRKS_SUPPORT
+  // Quirk feature flags previously applied to this window, for which log
+  // messages have already been written.
+  std::set<int> logged_quirks;
+#endif
+
   // Window rect and state from the most recent xdg_toplevel/aura_toplevel
   // configure event, to be applied when xdg_surface.configure is next received.
   struct sl_config next_config;
@@ -222,4 +229,18 @@ void sl_send_configure_notify(struct sl_window* window);
 int sl_process_pending_configure_acks(struct sl_window* window,
                                       struct sl_host_surface* host_surface);
 
+#ifdef QUIRKS_SUPPORT
+// Returns true if this function hasn't been called with this combination of
+// `window` and `feature_enum` before. In that case, the caller is expected to
+// write a log message indicating that the quirk has been applied.
+bool sl_window_should_log_quirk(struct sl_window* window, int feature_enum);
+
+// Returns all quirks ever logged against this window.
+// This "latches"; if a quirk has ever been enabled, it will stay in this list,
+// even if the quirk is no longer enabled.
+std::set<int> sl_window_logged_quirks(struct sl_window* window);
+#endif
+
+bool sl_window_is_client_positioned(struct sl_window* window);
+
 #endif  // VM_TOOLS_SOMMELIER_SOMMELIER_WINDOW_H_

sommelier.cc

@@ -5,6 +5,7 @@
 #include "sommelier.h"            // NOLINT(build/include_directory)
 #include "sommelier-tracing.h"    // NOLINT(build/include_directory)
 #include "sommelier-transform.h"  // NOLINT(build/include_directory)
+#include "sommelier-window.h"     // NOLINT(build/include_directory)
 #include "sommelier-xshape.h"     // NOLINT(build/include_directory)
 #ifdef GAMEPAD_SUPPORT
 #include "libevdev/libevdev-shim.h"
@@ -1420,8 +1421,8 @@ void sl_handle_map_request(struct sl_context* ctx,
   }
 
   // Allow user/program controlled position for transients,
-  // or for all windows if --enable_x11_move_windows was specified.
-  if (window->transient_for || ctx->enable_x11_move_windows)
+  // and other client-positioned windows.
+  if (window->transient_for || sl_window_is_client_positioned(window))
     window->size_flags |= size_hints.flags & (US_POSITION | P_POSITION);
 
   // If startup ID is not set, then try the client leader window.

testing/x11-test-base.h

@@ -5,7 +5,9 @@
 #ifndef VM_TOOLS_SOMMELIER_TESTING_X11_TEST_BASE_H_
 #define VM_TOOLS_SOMMELIER_TESTING_X11_TEST_BASE_H_
 
+#include <xcb/xproto.h>
 #include <memory>
+#include <string>
 
 #include "../xcb/mock-xcb-shim.h"
 #include "wayland-test-base.h"  // NOLINT(build/include_directory)
@@ -88,6 +90,29 @@ class X11TestBase : public WaylandTestBase {
     return window;
   }
 
+  std::string StringPropertyForTesting(xcb_window_t window_id,
+                                       xcb_atom_t property_name) {
+    xcb_get_property_cookie_t cookie = xcb.get_property(
+        nullptr, 0, window_id, property_name, XCB_ATOM_STRING, 0, 1024);
+    xcb_get_property_reply_t* reply =
+        xcb.get_property_reply(nullptr, cookie, nullptr);
+    EXPECT_TRUE(reply) << "get_property_reply() returned null. Try calling "
+                          "xcb.DelegateToFake().";
+    std::string result;
+    if (reply->format != 8) {
+      result = "error: expected X11 property format 8, got " +
+               std::to_string(reply->format);
+    } else if (reply->type != XCB_ATOM_STRING) {
+      result = "error: expected X11 property type XCB_ATOM_STRING";
+    } else {
+      void* value = xcb.get_property_value(reply);
+      result = std::string(static_cast<char*>(value), reply->length);
+      free(value);
+    }
+    free(reply);
+    return result;
+  }
+
  protected:
   NiceMock<MockXcbShim> xcb;
   std::unique_ptr<FakeWaylandClient> xwayland;