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

[Pekka Paalanen]

On Mon, 02 Jan 2017 16:17:27 -0500
Adam Jackson <ajax at nwnk.net> wrote:

> 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.

Hi Adam,

I'll continue that discussion in a reply to Olivier, replies on the
mechanics below. Just fresh (all paged out) from the holidays myself.

> > +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__);

This check is purely defensive programming in case someone later edits
the code. It's essentially an assert() to say "never call this function
with any other property". Do you not like such "documentation"?

> > +
> > +    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

Well, every place where this is called, really needs to inspect the
property, because it may or may not be set, and it may or may not be set
with the correct types etc.

> think this would be better factored as:
> 
> void xwl_window_allow_commits(struct xwl_window *win, Bool allow);

That function would be a single assignment, and I was lazy writing it,
but I can do that - it will be easier to maintain with a function
instead of a plain assignment. I would still prefer keeping the
different debug messages for the cases where the value is set
implicitly instead of from the property.

> 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.

Indeed, I did think of something to allow easily adding more properties
we might want to watch, but since we don't yet, I decided to leave that for
when (if) we actually do. When we do, the below stuff would be split into a
separate function "handle the allow_commits_prop stuff". I can do that
already, too.

The repeated check however I explained above, since this is not the
only call site of xwl_window_set_allow_commits_from_property().

> > +    /* 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.

Yup.

> > @@ -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.

I'm not sure what you mean, because I really want to initialize from
the current value of the property if it exists, not unconditionally to
a constant. The WM can (and IIRC normally will) set the property before
the window is realized.

Ah, it must be set before the window is realized because otherwise the
first commit would race against setting ALLOW_COMMITS to false.

Struct xwl_window gets created when the window is realized, so it
cannot be initialized earlier.

> 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.)

I did wonder if that might fail, I'll add the check.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.x.org/archives/xorg-devel/attachments/20170112/5eb626d4/attachment-0001.sig>