[PATCH weston] desktop-shell: Fail if get_xdg_surface is called on an xdg_surface

[Jasper St. Pierre]

On Thu, Feb 5, 2015 at 5:11 AM, Daniel Stone <daniel at fooishbar.org> wrote:

> Hi,
>
> On 5 February 2015 at 08:58, Jonas Ådahl <jadahl at gmail.com> wrote:
> > If a client calls xdg_shell.get_xdg_surface on a surface that is already
> > an xdg_surface wold, prior to this patch, succeed, but cause weston to
> > crash later when trying to configure. This patch instead sends a role
> > error to the client complaining that it already is an xdg_surface.
> >
> > Note that .._set_role() only fails when changing roles, not when setting
> > the same role twice.
>
> Yeah, that one was interesting to me. I wonder if there's any benefit
> to making it fail unconditionally if we try to re-set the same role -
> but that's a whole different patchset.
>

Well, "set the role" means "call xdg_shell.get_xdg_surface". If you have a
wl_surface, to map it, you call xdg_shell.get_xdg_surface, and to unmap it,
you destroy the xdg_surface. If you'd rather that we fully destroy the
wl_surface and create it anew, we can do that too, but both Pekka and I
preferred this option.

This also helps simplify things like cursors, where you get the cursor role
by calling wl_pointer.set_cursor, and requiring a new wl_surface every time
you want to set the I-beam or arrow cursor seems wasteful.


> > The same is done for xdg_popup.
> >
> > Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> > ---
> >  desktop-shell/shell.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
> > index a7514f7..cc345b5 100644
> > --- a/desktop-shell/shell.c
> > +++ b/desktop-shell/shell.c
> > @@ -3959,6 +3959,14 @@ xdg_get_xdg_surface(struct wl_client *client,
> >         struct desktop_shell *shell = sc->shell;
> >         struct shell_surface *shsurf;
> >
> > +       if ((shsurf = get_shell_surface(surface)) &&
>
> Please assign shsurf in the declarations above - I hate assignments in
> if statements.
>
> > +           shell_surface_is_xdg_surface(shsurf)) {
> > +               wl_resource_post_error(resource, XDG_SHELL_ERROR_ROLE,
> > +                                      "This wl_surface is already an "
> > +                                      "xdg_surface");
> > +               return;
> > +       }
> > +
> >         if (weston_surface_set_role(surface, "xdg_surface",
> >                                     resource, XDG_SHELL_ERROR_ROLE) < 0)
> >                 return;
> > @@ -4052,6 +4060,14 @@ xdg_get_xdg_popup(struct wl_client *client,
> >         struct weston_surface *parent;
> >         struct shell_seat *seat;
> >
> > +       if ((shsurf = get_shell_surface(surface)) &&
>
> Ditto.
>
> Ultimately I'd like to see weston_surface_set_role() deal with this
> and not have to have the split codepaths, but not for 1.7. With those
> two nitpicks fixed:
> Reviewed-by: Daniel Stone <daniels at collabora.com>
>
> Cheers,
> Daniel
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>



-- 
  Jasper
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20150205/f5d2ce43/attachment.html>