Mir

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

Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

+/* 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?

~~~~

+/* 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?

~~~~

+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

~~~~

- 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?)

~~~~

inline mir::cookie::Blob const& getCookie() const { return mCookie; }

so getCookie() returns a blob? Confusing! "getCookieAsBlob()"?

review: Needs Fixing

« Back to merge proposal