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

> +/* Queries the size needed to serialize a given cookie
> + *
> + * \params[in] cookie A cookie instance
> + * \return The size of the serialized representation of the given
> cookie
> + */
> +size_t mir_cookie_size(MirCookie const* cookie);
>
> Would mir_cookie_buffer_size() be a clearer name?
>
> ~~~~

Yes it would. Changing.

>
> +/* Create a cookie from a serialized representation
> + *
> + * \pre The size must be equal to mir_cookie_size
> + * \params[in] buffer The buffer containing a serialized cookie.
> + * The buffer may be freed immediately after this call.
> + * \return A MirCookie instance. The instance must be released
> + with a call to mir_cookie_release.
> + */
> +MirCookie const* mir_cookie_from_buffer(void const* buffer, size_t size);
>
> The precondition is impossible for the client to check - mir_cookie_size()
> takes a parameter that isn't (in the general case) available to the client.
>
> Perhaps we should drop the cookie parameter from mir_cookie_size()? Or do we
> expect different cookies to have different sizes?
>
> ~~~~

While atm the cookie size is consistent, the cookie size is dependent on the cookie authority implementation.

As far as the size, the size for the buffer *must* be mir_cookie_size() and it *must* get that size to allocate said buffer. The only way they could have gotten an allocated buffer, would be by querying the size. So I would think its up to the user to pass the size_t size around which MUST be equal to the mir_cookie_size at one point.

>
> +class Cookie
> +{
> +public:
> + virtual ~Cookie() = default;
> +
> + /**
> + * Returns the timestamp that the cookie is built with
> + *
> + * \return The timestamp
> + */
> + virtual uint64_t timestamp() const = 0;
> +
> + /**
> + * Converts the cookie into a stream of bytes.
> + *
> + * \return The stream of bytes formatted
> + */
> + virtual std::vector<uint8_t> serialize() const = 0;
> +};
>
> interfaces should delete CopyAssign
>
> ~~~~

Reasonable, Changing.

>
> - void raise_surface_with_timestamp(
> + void raise_surface(
>
> Is this API break necessary? It will break downstreams. (And is how it
> motivated by the purpose of this MP?)
>
> ~~~~

This API is new, (was removed from the API before the 0.18 release). So just trying to get the correct name for it.
>
> inline mir::cookie::Blob const& getCookie() const { return mCookie; }
>
> so getCookie() returns a blob? Confusing! "getCookieAsBlob()"?

Very confusing, and yes Ill go through and rename that.