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

[Daniel Stone]

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.

> 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