[PATCH weston v2 04/21] desktop-shell: Make activate_binding take a view instead of surface

[Jonas Ådahl]

On Thu, Jun 04, 2015 at 04:00:27PM -0500, Derek Foreman wrote:
> On 13/05/15 05:26 AM, Jonas Ådahl wrote:
> > In preparation for further refactoring.
> 
> The intended change seems harmless enough, however see below...
> 
> > 
> > Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> > ---
> >  desktop-shell/shell.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
> > index ff17b04..1ac1340 100644
> > --- a/desktop-shell/shell.c
> > +++ b/desktop-shell/shell.c
> > @@ -5153,12 +5153,14 @@ is_black_surface (struct weston_surface *es, struct weston_surface **fs_surface)
> >  static void
> >  activate_binding(struct weston_seat *seat,
> >  		 struct desktop_shell *shell,
> > -		 struct weston_surface *focus)
> > +		 struct weston_view *focus_view)
> >  {
> > +	struct weston_surface *focus;
> >  	struct weston_surface *main_surface;
> >  
> > -	if (!focus)
> > +	if (!focus_view)
> >  		return;
> 
> focus_view being NULL would have caused a segfault previously, since all
> callers passed focus_view->surface, so this test will never cause a return?

Seems all callers of this function does a NULL check on the view before
calling this function so the check here is redundant. I'll remove it.

> 
> > +	focus = focus_view->surface;
> 
> And we used to test this for NULL but we don't anymore?
> 
> (I guess just leave the if statement alone and move the focus = line
> above it to keep the old behaviour?)

The surface of a view can never be NULL, so there is no need to check
it. The original check seems to have been completely unnecessary, so no
use of reinstating it.

Thanks for taking a look.


Jonas

> 
> >  	if (is_black_surface(focus, &main_surface))
> >  		focus = main_surface;
> > @@ -5171,7 +5173,8 @@ activate_binding(struct weston_seat *seat,
> >  }
> >  
> >  static void
> > -click_to_activate_binding(struct weston_seat *seat, uint32_t time, uint32_t button,
> > +click_to_activate_binding(struct weston_seat *seat,
> > +			  uint32_t time, uint32_t button,
> >  			  void *data)
> >  {
> >  	if (seat->pointer->grab != &seat->pointer->default_grab)
> > @@ -5179,7 +5182,7 @@ click_to_activate_binding(struct weston_seat *seat, uint32_t time, uint32_t butt
> >  	if (seat->pointer->focus == NULL)
> >  		return;
> >  
> > -	activate_binding(seat, data, seat->pointer->focus->surface);
> > +	activate_binding(seat, data, seat->pointer->focus);
> >  }
> >  
> >  static void
> > @@ -5190,7 +5193,7 @@ touch_to_activate_binding(struct weston_seat *seat, uint32_t time, void *data)
> >  	if (seat->touch->focus == NULL)
> >  		return;
> >  
> > -	activate_binding(seat, data, seat->touch->focus->surface);
> > +	activate_binding(seat, data, seat->touch->focus);
> >  }
> >  
> >  static void
> > 
>