[PATCH] xwayland: Avoid repeatedly looping through window ancestor chain

[Pekka Paalanen]

On Sat, 12 Aug 2017 03:45:39 +0200
Roman Gilg <subdiff at gmail.com> wrote:

> On Thu, Aug 10, 2017 at 2:49 PM, Pekka Paalanen <ppaalanen at gmail.com> wrote:

> > Assuming the theory is sound, how about you kept the old names and
> > semantics of xwl_window_get() and xwl_window_from_window() but just
> > used the stored private in xwl_window_from_window()? You would need to
> > have a xwl_window_find_from_window() for the single use in
> > xwl_realize_window(), but I think that would allow you to put the test
> >
> >         if (win != xwl_window->window)
> >
> > in xwl_window_get() rather than open-coding it in the users. I believe
> > keeping the semantics would be less surprising to the reader,
> > especially if they know the old code.
> >  
> 
> It's a good idea to not open-code the condition for the users. I would
> prefer though to risk the short-term confusion for the users, because
> otherwise in the long term the naming is ambiguous:
> 
> A Window has exactly one xwl_window associated (or none): The one in its
> private field. Calling xwl_window_get() therefore is the natural way to
> receive this one xwl_window. The function naming becomes really confusing
> when we call xwl_window_from_window() inside xwl_window_get() to receive
> the private field and afterwards do the test or when we call
> xwl_window_from_window() in xwl_window_find_from_window() on the Window's
> parent Window.

Hi Roman,

are you seeing xwl_window_get() as "get the private" while I think of
it as "get the owned xwl_window"? In that case, and as it has not been
documented which one it really meant before, I think we should just not
use the name xwl_window_get() anymore at all to get rid of the
ambiguity. This is the most important point in my comments and even
that is not too important.

> How do you like this approach:
> * xwl_window_get(WindowPtr) returns the private field
> * New function xwl_window_own(WindowPtr win) calls xwl_window_get and
> forwards the xwl_window if (win == xwl_window->window), otherwise NULL.
> Maybe there is a better name than xwl_window_own though for this function?
> * xwl_window_from_window() is renamed to xwl_window_find() and as it's now
> will only be called in xwl_realize_window.
> 
> But if you prefer the other naming, it's also fine with me.

Yes, that sounds better. Since the semantics of xwl_window_get() change
due to setting the window private for more windows than the one
actually owning the xwl_window, it requires touching all the current
callers. That's why I previously suggested to keep its semantics intact.

Would it be nice to rename xwl_window_get() to
xwl_window_from_priv() to denote this change in semantics and
ensure that no callers go unnoticed? Or to_xwl_window()?

xwl_window_own() would be the replacement for the old xwl_window_get(),
or maybe call it... xwl_window_from_owner()? Returns NULL if the Window
doesn't own any xwl_window.

How does your Present and sub-surface work change the relationships
between Windows and xwl_windows? Does the child Window getting a
sub-surface have its own xwl_window? Are there cases for fetching from
the child Window (or from its descendant) both its own xwl_window and
the respective toplevel Window's xwl_window?

If the above doesn't feel particularly fitting, then let's go with your
suggestion.


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