Mir

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

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

Nits to fix and questions:

CookieFactory looks less and less like a "factory" interface. Would "CookieAuthority" be a better name?

~~~~

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

~~~~

+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.

~~~~

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

~~~~

- * 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.)

~~~~

+ /* 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));

~~~~

+#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).

review: Needs Fixing

« Back to merge proposal