Code review comment for lp:~albaguirre/mir/add-menu-api

On Wed, Jan 14, 2015 at 9:27 AM, Alberto Aguirre
<email address hidden> wrote:
>> @Alan:
>> > I know it is before the time of many current team members but we
>> originally
>> envisaged an MVC arrangement
>> > where changes to the model (like creating a surface) would be
>> routed through
>> a controller
>> > (like a window manager) that is part of the shell and not go
>> directly to the
>> model (in this case the session).
>> >
>> > Were we in that hypothetical world then the window manager would
>> validate
>> this policy.
>>
>> Hm. Whereas I think this would be a part of the contract with the
>> window
>> manager - “when we ask you to create a window, we guarantee that
>> it'll have
>> all the mandatory information”. The client library needs to
>> specify what the
>> mandatory information is, because that's the only place where we
>> can specify
>> client semantics.
>
> Sure, but we are talking about malformed requests, which need to be
> validated by something. That something for me is not the frontend as
> that would couple knowledge of window management semantics (The
> implementation of the window management policy is the one that knows
> what combination of arguments are correct for a surface type for
> example).

We're (by necessity - there's no where else to put it) encoding window
management semantics into the client library and protocol. The client
library already knows what properties are mandatory for a given surface
type - we encode them in the surface_spec constructor.

The shell's window management policy may (but almost always
*shouldn't*) impose additional constraints.

I think that malformed requests are exactly the sort of thing the
protocol layer should be filtering out. Surface creation isn't a
hot-path, so a defence-in-depth, validate whatever you can at this
stage and let the next stage validate some more, is reasonable.

Note that this doesn't necessarily mean validation is baked into
SessionManager - we could have a separate MessageValidator interface
SessionManager delegates to, or (possibly a better idea) we could
specialise the SurfaceCreate protocol message for each surface type so
it's not possible to represent an invalid request.

That's not necessarily in-scope for this MP, though.