[PATCH weston-ivi-shell v4 3/9] ivi-shell supports a type of shell for In-Vehicle Infotainment system.

[Nobuhiko Tanibata]

2014-07-07 16:34 に Pekka Paalanen さんは書きました:
> On Tue, 20 May 2014 14:05:10 +0900
> Nobuhiko Tanibata <nobuhiko_tanibata at xddp.denso.co.jp> wrote:
> 
>> Hi pq,
>> 
>> I applied your comments except for several ones,
>> 
>> >> +    struct ivi_shell *shell = wl_resource_get_user_data(resource);
>> >> +    struct ivi_shell_surface *ivisurf = NULL;
>> >> +    struct weston_layout_surface *layout_surface = NULL;
>> >> +    struct weston_surface *es =
>> >> wl_resource_get_user_data(surface_resource);
>> >> +    struct wl_resource *res;
>> >> +    int32_t warn_idx = -1;
>> >> +
>> >> +    if (es != NULL) {
>> >> +        layout_surface = weston_layout_surfaceCreate(es, id_surface);
>> >> +        if (layout_surface == NULL)
>> >> +            warn_idx = 1;
>> >> +    } else {
>> >> +        warn_idx = 0;
>> >
>> > I think this case would be a compositor internal error, not a client
>> > error. Userdata of a surface resource should never be NULL. I.e.
>> > assert() kind of thing would be more approriate.
>> >
>> 
>> I expect it would send warning to client. So I implement it like the
>> above.
> 
> Is there really a way that a client (malfunction) could cause this?
> 
> I don't think so. Weston core always creates a weston_surface for a
> wl_surface, and if it ever does not or it disappears, you have a
> Weston bug or memory corruption. So I really don't think warning
> the client is a) sufficient, nor b) relevant as the client simply
> cannot have anything to do with triggering this. There is also
> nothing the client could do to avoid or remedy it.
> 
> I believe this fits well on the definition of an internal error.
> 
> I know you have much higher demands for dealing with internal
> errors than Weston usually does, so I would suggest using your own
> assert mechanism, which just gets rid of the client via the
> wl_display::error event. I think it would simplify your code. Do
> you think it would work for you?
> 
> It would also serve a documentary purpose just like the plain
> assert() does: this condition is not expected to fail, and if it
> fails, there is a bug in *this* program somewhere, and not in its
> input (clients).
> 
> Also, Weston already uses lots of plain asserts all over. Are those
> not a concern to you?
> 

OK, I understand. I will follow the above proposal. And I need to remove 
event from ivi-application.xml as well.

>> >> +    init_ivi_shell(ec, shell);
>> >> +    weston_layout_initWithCompositor(ec);
>> >> +
>> >> +    shell->destroy_listener.notify = shell_destroy;
>> >> +    wl_signal_add(&ec->destroy_signal, &shell->destroy_listener);
>> >> +
>> >> +    if (wl_global_create(ec->wl_display, &ivi_application_interface,
>> >> 1,
>> >> +                         shell, bind_ivi_application) == NULL) {
>> >> +        return -1;
>> >> +    }
>> >
>> > You are not setting compositor->shell_interface, but OTOH those are
>> > only used for Xwayland, which probably doesn't make sense here.
>> 
>> I was advised by kritian before it shall not support other shell in
>> ivi-shell at the same time.
> 
> Sorry?
> 
> Sure, I understand you do not want/cannot to have any other shell
> loaded in Weston when ivi-shell is, but this is not about that.
> 
> This is about compositor->shell_interface, which is a set of vfuncs
> set by the shell currently loaded. However, these vfuncs are only
> used by the XWayland support (XWM), and so might not be necessary.
> It's not a different shell, it is just about XWayland using the
> current shell.
> 
> That was just an observation. I don't think you need to change
> anything here, but if you want, you could add a comment that
> XWayland is not supported here. That is all.
> 

OK I see. I also have use case of xwayland. But it may be next steps. I 
will follow the above steps.
Thank you very much!

BR,
ntanibata

> 
> Thanks,
> pq