[PATCH xserver v2 5/5] xwayland: use _XWAYLAND_ALLOW_COMMITS property

[Adam Jackson]

On Fri, 2016-12-09 at 14:24 +0200, Pekka Paalanen wrote:
> From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> 
> The X11 window manager (XWM) of a Wayland compositor can use the
> _XWAYLAND_ALLOW_COMMITS property to control when Xwayland sends
> wl_surface.commit requests. If the property is not set, the behaviour
> remains what it was.

I'm still thinking over whether I like this or whether I'd rather have
this keyed off the netwm sync props. (Did we think that was a thing
that could work? Forgive me, I'm freshly back from holidays.) Still,
some style notes below.

> +static void
> +xwl_window_set_allow_commits_from_property(struct xwl_window *xwl_window,
> +                                           PropertyPtr prop)
> +{
> +    static Bool warned = FALSE;
> +    CARD32 *propdata;
> +
> +    if (prop->propertyName != xwl_window->xwl_screen->allow_commits_prop)
> +        FatalError("Xwayland internal error: prop mismatch in %s.\n", __func__);
> +
> +    if (prop->type != XA_CARDINAL || prop->format != 32 || prop->size != 1) {
> +        /* Not properly set, so fall back to safe and glitchy */
> +        xwl_window->allow_commits = TRUE;
> +
> +        if (!warned) {
> +            LogMessage(X_WARNING, "Window manager is misusing property %s.\n",
> +                       NameForAtom(prop->propertyName));
> +            warned = TRUE;
> +        }
> +        return;
> +    }
> +
> +    propdata = prop->data;
> +    xwl_window->allow_commits = !!propdata[0];
> +    DebugF("xwayland: win %d allow_commits = %d\n",
> +           xwl_window->window->drawable.id, xwl_window->allow_commits);
> +}

This seems odd. The primitive concept here is "allow commits or not",
not "inspect this property to tell me whether commits are okay". I
think this would be better factored as:

void xwl_window_allow_commits(struct xwl_window *win, Bool allow);

And then in the callers:

> +static void
> +xwl_property_callback(CallbackListPtr *pcbl, void *closure,
> +                      void *calldata)
> +{
> +    ScreenPtr screen = closure;
> +    PropertyStateRec *rec = calldata;
> +    struct xwl_screen *xwl_screen;
> +    struct xwl_window *xwl_window;
> +    Bool old_allow_commits;
> +
> +    if (rec->win->drawable.pScreen != screen)
> +        return;
> +
> +    xwl_window = xwl_window_get(rec->win);
> +    if (!xwl_window)
> +        return;
> +
> +    xwl_screen = xwl_screen_get(screen);
> +    if (rec->prop->propertyName != xwl_screen->allow_commits_prop)
> +        return;

... change this to a table lookup for known property names? Maybe not a
thing we need yet. Either way, don't repeat this in the _allow_commits,
which you wouldn't be able to do anyway with the new signature.

> +    /* Handle _XWAYLAND_ALLOW_COMMITS property changes */
> +
> +    old_allow_commits = xwl_window->allow_commits;
> +
> +    switch (rec->state) {
> +    case PropertyNewValue:
> +        xwl_window_set_allow_commits_from_property(xwl_window, rec->prop);
> +        break;
> +
> +    case PropertyDelete:
> +        xwl_window->allow_commits = TRUE;
> +        DebugF("xwayland: win %d allow_commit reverts to uncontrolled.\n",
> +               xwl_window->window->drawable.id);
> +        break;

This would refactor as:

void xwl_property_allow_commits(struct xwl_window *, PropertyStateRec *);

which computes the allow boolean to give xwl_allow_commits.

> @@ -262,6 +346,25 @@ xwl_pixmap_get(PixmapPtr pixmap)
>  }
>  
>  static void
> +xwl_window_init_allow_commits(struct xwl_window *xwl_window)
> +{
> +    PropertyPtr prop = NULL;
> +    int ret;
> +
> +    ret = dixLookupProperty(&prop, xwl_window->window,
> +                            xwl_window->xwl_screen->allow_commits_prop,
> +                            serverClient, DixReadAccess);
> +    if (ret == Success && prop) {
> +        xwl_window_set_allow_commits_from_property(xwl_window, prop);
> +    }
> +    else {
> +        xwl_window->allow_commits = TRUE;
> +        DebugF("xwayland: win %d allow_commit is uncontrolled.\n",
> +               xwl_window->window->drawable.id);
> +    }
> +}

Likewise here you wouldn't need to look up the property first and could
just initialize. That assumes the property atom actually exists, so...

> @@ -694,6 +802,7 @@ wm_selection_callback(CallbackListPtr *p, void *data, void *arg)
>  static Bool
>  xwl_screen_init(ScreenPtr pScreen, int argc, char **argv)
>  {
> +    static const char allow_commits[] = "_XWAYLAND_ALLOW_COMMITS";
>      struct xwl_screen *xwl_screen;
>      Pixel red_mask, blue_mask, green_mask;
>      int ret, bpc, green_bpc, i;
> @@ -849,6 +958,11 @@ xwl_screen_init(ScreenPtr pScreen, int argc, char **argv)
>      pScreen->CursorWarpedTo = xwl_cursor_warped_to;
>      pScreen->CursorConfinedTo = xwl_cursor_confined_to;
>  
> +    xwl_screen->allow_commits_prop = MakeAtom(allow_commits,
> +                                              strlen(allow_commits),
> +                                              TRUE);
> +    AddCallback(&PropertyStateCallback, xwl_property_callback, pScreen);
> +
>      return ret;
>  }

... here you would check that MakeAtom didn't return BAD_RESOURCE and
return false if it did. (If MakeAtom fails in screen init you're doomed
anyway.)

- ajax