Code review comment for lp:~mir-team/mir/public-cookie-api

> Nits to fix and questions:
>
> CookieFactory looks less and less like a "factory" interface. Would
> "CookieAuthority" be a better name?
>
> ~~~~

Agreed.

>
> + virtual bool attest_timestamp(MirCookie const* cookie) = 0;
>
> This is the only place in the interface MirCookie is mentioned. Should we
> simply lose this function and require the user to call the other overload?
>
> ~~~~

The reason for keeping this, is theres no way to de construct a MirCookie in the public. So once the content hub gets a mir cookie it wont know how to *attest* it.

>
> +void mir_input_event_copy_cookie(MirInputEvent const* ev, void* cookie);
>
> I'm always cautions of APIs that take a target buffer and no buffer size -
> while not bulletproof the latter is at least a reminder to the user to obtain
> the size.
>
> ~~~~

Thats sounds quite reasonable to me.
>
> +void mir_surface_raise_with_cookie(MirSurface* surface, void const* cookie);
>
> By using void* we loose the type safety that we have with other Mir APIs. Can
> you provide some justification for this design choice? E.g. will this
> function be primarily for clients that obtained the cookie from a non-Mir API?
>
> ~~~~

I agree, and will be switching to a MirCookie* for both the copy and these functions since void* and MirCookie* will both be incomplete types.

>
> - * Copyright © 2015 Canonical Ltd.
> + * Copyright © 2016 Canonical Ltd.
>
> Don't remove the 2015 copyright claim. (I know these headers are legally
> unnecessary and PITA, but let's try to do it right.)
>
> ~~~~

Opps. Yeah Ill fix those!

>
> + /* No mac == no size! */
> + return 0;
>
> It should be a precondition that there's a MAC. I.e. the function should begin
> with something like:
>
> mir::require(mir_input_event_has_cookie(ev));
>
> ~~~~

This is true, if we have a function that tells you if an event has a cookie we better require it to actually have one when attempting to construct it!.

>
> +#include "mir/cookie.h"
> #include "mir/cookie_factory.h"
>
> The first include of the source file should be the corresponding header (to
> ensure it compiles by itself).

Will fix!