Code review comment for lp:~robertcarr/mir/client-focus-notifications

High level - I concur with Daniel: having focus being a surface attribute seems dangerous. Current focus is... session local? Hm.

Focus is a per-keyboardish attribute, so it's not strictly speaking unique in a compositor session, but it's pretty close to.

===

Why are we calling the guard value of the enums *_arraysize_? That seems somewhat misleading.

===

Perhaps Clog should take a shared_ptr<CloggableEventSink> rather than a reference? It needs to keep the event sink live while the Clog is live. This is not a problem in the current code, though.

<bikeshed>I'd find "Plug" a more natural name for the Clog class; you stick a plug in your sink to clog it ☺ </bikeshed>

====

I was going to say that lttng::MPR::sent_event should be event_sent to match exception_handled above, but it turns out that above *that* is received_invocation/completed_invocation.

I think event_sent (and invocation_{received,completed}) makes more sense, but there's no consistency there at the moment.

===

812 + ~ProxyShell() noexcept(true) = default;

I *think* the default destructor is noexcept, so the noexcept(true) there is redundant?

===

949 + static int const semaphore_enable_process_shared = 1;

Why is this static? You don't need to track the value of this across entries to IPCSemaphore(), and I presume the compiler will entirely optimise it out.

===

1171 + ~MockEventSink() … {} could be ~MockEventSink() = default; ?