Merge lp:~robertcarr/mir/client-focus-notifications into lp:~mir-team/mir/trunk

[Robert Carr]

Send focus notifications to client.

This time the dependency is actually removed...properly! accidentally reverse merged some stuff which was making the diff huge.

===

Another iteration. This time I've removed the dependency on connect-display-request-merge-trunk. I've adapted the handle_surface_created idea from that branch (thanks kevin :D) to enable things to work without the cloggable sink.

Currently the shell mixes responsibilities of a factory interface, and reactive behavior (largely through acting as part of the factory/builder). I hope we can run with this and split out the factory part, in to classes which do that, and be left with a sensible interface like:

mf::Shell
{
handle_session_created
handle_sesssion_destroyed
handle_surface_created
handle_surface_destroyed
etc etc
}

The session mediator (and presumably a to be coming InternalClientMediator) then acts to notify the shell after building the objects.

=== Last description ==
This branch is a new version of client-focus-notifications based on ~mir-team/mir/connect-display-request-merge-trunk which enables removal of the cloggable event sink (notice how focus is now set in handle_surface_created, executed after the surface frontend processing has completed).

I've elected to still use surface attributes for focus, for two reasons:
1. I can't think of anything else to use
2. mir_surface_get_state(mir_surface_attrib_focus...seems to make sense as client API.

I think perhaps though, the default_focus_mechanism should handle the response of surface->configure with surface_attrib_focus. This way the shell can use the surface configurator to interfere and say, actually this surface may not take focus. I'm happy to include it in this MP if it's a popular idea or propose a followup for discussion after.

[Robert Carr (robertcarr)]

There are a few open questions:

Should focus be modeled as a Surface attribute?
Is there any reason for the Surface to keep track of if it has focus.

[kevin gunn (kgunn72)]

> There are a few open questions:
>
> Should focus be modeled as a Surface attribute?
> Is there any reason for the Surface to keep track of if it has focus.

makes sense to me it would be a surface attribute. as to why it might want to keep track....optimizations if there is no focus? and possibly, other client requests that won't work/don't make sense if focused or not.

just curious...is this some convention ?
mir_surface_focus_arraysize_
didn't understand why it was in the surf attrib for focusstate

in general...might be my lack of understanding. there are some mutex/surface locks in the proposed code....but i don't see unlocks (do those happen automagically as the code goes out of scope?)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:725
http://jenkins.qa.ubuntu.com/job/mir-ci/647/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/789
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/671
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-raring-amd64-ci/132
        deb: http://jenkins.qa.ubuntu.com/job/mir-raring-amd64-ci/132/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/647/rebuild

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

> just curious...is this some convention ?
> mir_surface_focus_arraysize_
> didn't understand why it was in the surf attrib for focusstate

Yes, it is a sentinel element for the array, and its value is equal to the array size.

> in general...might be my lack of understanding. there are some mutex/surface
> locks in the proposed code....but i don't see unlocks (do those happen
> automagically as the code goes out of scope?)

std::unique_lock<> and std::lock_guard<> are scoped locks. They lock the associated mutex when created, and unlock it when going out of scope.

[Alexandros Frantzis (afrantzis)]

202 + std::unique_lock<std::mutex> lg(lock);
207 + std::unique_lock<std::mutex> lg(lock);

Are these locks necessary? Can a SessionMediator service two requests (in this case, two create_surface requests) concurrently? If it can, then the locks, as they are, have the following problem:

(t1) clog
(t2) clog
(t1) unclog, oops, any events for the surface created in (t2) will be sent to the client, too

214 + void handle_event(MirEvent const& ev)
215 + {
216 + if (clogged)

Taking into account the previous point (assuming the mediator can't service two requests from the same connection concurrently) and since this access to clogged is not protected, I suggest that we make clogged atomic<bool> and get rid of the locks.

315 + notify_change(attrib, value);
316 + break;

Indented one level deeper than required.

374 +namespace
375 +{
376 +
377 + struct ClientConfigCommon : TestingClientConfiguration
378 + {
379 + ClientConfigCommon() :
380 + connection(0),
381 + surface(0)
382 + {
383 + }
...

No need for indentation in namespace block.

415 + surface = NULL;

nullptr

505 +TEST_F(BespokeDisplayServerTestFixture, two_surfaces_are_notified_of_gaining_and_losing_focus)
552 + // We need some synchronization to ensure client two does not connect before client one.

Doesn't waiting for ready_for_second_client semaphore take care of that (it triggers only after the first client has received a surface created event, which means it has already connected)?

However, the test is still fragile. If the second client hasn't managed to connect and create a surface in the 5 seconds the first test has to finish, e.g., because of super high system load, then all_events_received.wait_for_at_most_seconds(5) in the first client will expire and the test will fail.

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

CloggableEventSink looks horribly broken:

There is no synchronization around "216 + if (clogged)" - which means it could be in different states in different threads.

"clogged" could change between "if (clogged)" being evaluated and the next statement.

There is nothing to prevent multiple calls to clog() - perhaps clogged should be an atomic counter?

I assume that clog() and unclog() are intended to be paired operations - but the calls I see don't deal with exceptions being thrown (please use RAII for paired operations).

What is the motivation for CloggableEventSink? Is this the right place for synchronization?

review: Needs Fixing

[Robert Carr (robertcarr)]

Hi all! Thanks for review...I think I have improved it some but still not found the final solution.

Starting with Alf's comments:

>> Are these locks necessary? Can a SessionMediator service two requests (in this case, two create_surface
>> requests) concurrently

As it stands the SessionMediator can not service two requests concurrently (the SocketSession only reads a new request after processing the last one). These locks are necessary however, as handle_event may be called from any thread (i.e. the shell configuring a surface from any thread), to prevent a data race on the event buffer.

I would propose we make explicit that the SessionMediator can not service two requests with locking.

>> SNIP

Indentation and nullptr and such fixed.

>> Doesn't waiting for ready_for_second_client semaphore take care of that (it triggers only after
>> the first client has received a surface created event, which means it has already connected)?

Yes sorry I moved the wait without moving the comment making this misleading. This is in fact the point of ready_for_second_client.

>> However, the test is still fragile. If the second client hasn't managed to connect and create a surface in
>> the 5 seconds the first test has to finish, e.g., because of super high system load, then
>> all_events_received.wait_for_at_most_seconds(5) in the first client will expire and the test will fail.

The test will always be fragile or have the possibility to lock indefinitely. I think 5 seconds is perhaps to short though (things have taken longer than this in valgrind on jenkins in the past) so I've bumped the timeouts to 60 seconds. We could consider a standard set of timeout values.

Alan:

>>
>> There is no synchronization around "216 + if (clogged)" - which means it could be in different states in >> different threads.
>>

This is just in error of course. Fixed.

>>
>> What is the motivation for CloggableEventSink? Is this the right place for synchronization?
>>

The motivation is that sending a surface configuration event (i.e. one generated before shell::SessionManager::create_surface returns) before replying to the create_surface request leaves client session in a situation which it can't know how to rectify. I think the SessionMediator acts to arbitrate communication between the Session and the IPC layer, and so this is in fact the right place for synchronization.

>>
>> There is nothing to prevent multiple calls to clog() - perhaps clogged should be an atomic counter?
>> I assume that clog() and unclog() are intended to be paired operations - but the calls I see don't deal
>> with exceptions being thrown (please use RAII for paired operations).
>>

I'll update the API to be safer at the beginning of next week after some feedback from the team on the high level approach.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:731
http://jenkins.qa.ubuntu.com/job/mir-ci/660/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/810
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/692
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-raring-amd64-ci/145/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/660/rebuild

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

Events should be removed from buffered_events once handled. (Consider also: what should happen if handle_event() throws.)

review: Needs Fixing

[Robert Carr (robertcarr)]

Fixed the failure to clear buffered_events (whoops!)

There is something strange going on with the acceptance tests under valgrind (intermittent failure to complete, client never receives the event). I'll stew on it (and the whole branch) overnight and revisit tomorrow.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:734
http://jenkins.qa.ubuntu.com/job/mir-ci/668/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/820
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/702
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-raring-amd64-ci/153/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/668/rebuild

review: Needs Fixing (continuous-integration)

[Robert Carr (robertcarr)]

The valgrind-induced race-triggered test-failure was due to the test client code, see the comment and change at l555 (vs the previous location of mir_surface_set_event_handler).

I think EventDelegate should be moved to the surface_create arguments. As it stands now mir_surface_create_sync should be considered dangerous.

[Robert Carr (robertcarr)]

In the process of debugging message_processor_report grew a sent_event method, as did the logging implementation (not sure how to print MirEvent from lttng...). Note the diff to add a single print line amounts to 141 lines...a bit unsatisfying imo.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:738
http://jenkins.qa.ubuntu.com/job/mir-ci/674/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/829
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/711
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-raring-amd64-ci/159
        deb: http://jenkins.qa.ubuntu.com/job/mir-raring-amd64-ci/159/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/674/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Nice to see the attribute logic being reused, but I don't think focus is an attribute of a Surface in the traditional sense. But it's kind of OK too because you're only doing notifications here.

I would like to say focus should never be an attribute because that increases the risk and confusion of potentially having multiple focused surfaces. And internally at least the attribute system is one for getting/setting simple values. But being able to set focus per surface would be dangerous.

This risk is mitigated by the absence of a mir_surface_set_focus. It's only a bit wrong internally then [1]. And I agree you need focus to be an attribute to get notifications for it. So I'm glad you're using the attribute notifications as I intended.

I'm not sure though.

If anything I'll just say that "focused" should be the attribute (boolean 0 or 1) instead of focus. And even if you disagree with renaming then please reorder the enum MirSurfaceFocusState so it's usable as a boolean.

[1] The "bit wrong internally" comment is really about this:
437 + case mir_surface_attrib_focus:
438 + notify_change(attrib, value);
439 + break;
You've implemented a stub setter for focus in msh::Surface::configure which doesn't set anything. In fact instead of notify_change and returning the default value 0 (which is wrong BTW) it should be throwing an exception on any attempt to set ("configure") focus. There must be a nicer way to do it, without modifying msh::Surface::configure (hence letting it throw on any attempt to configure focus).

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

198 + virtual ~IPCSemaphore();

Prefer a non-virtual, noexcept destructor

~~~~

549 + client->handler->handle_event(ev);

If "ev" were given the more meaningful name the sheer repetitiveness would be even more apparent:

    client->handler->handle_event(event);

You can see it here too:

948 + event_sink->handle_event(any_event);

s/handle_event/handle/ would be an improvement.

~~~~

98 + std::weak_ptr<Surface> focus_surface;

What is the purpose of this? The name isn't clear, and the usage is entirely at member function scope (so why use a member variable).

review: Needs Fixing

[Robert Carr (robertcarr)]

Thanks both!

Alan: Comments addressed. Name improved to be clear at l98

Daniel: I've flipped the enum values...good point. As for the use of configure...I'm not sure of a better way to do this until a bigger focus refactoring happens. This is "next on my list"

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:742
http://jenkins.qa.ubuntu.com/job/mir-ci/685/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/842
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/724
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-raring-amd64-ci/170
        deb: http://jenkins.qa.ubuntu.com/job/mir-raring-amd64-ci/170/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/685/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Again with:
437 + case mir_surface_attrib_focus:
438 + notify_change(attrib, value);
439 + break;

It would be easy to write reasonable test cases that now fail. Similar to:
tests/unit-tests/shell/test_surface.cpp: TEST_F(ShellSurface, states)

Because calling:
    surf.configure(mir_surface_attrib_focus, X) != X
and fails to throw an exception too.

review: Needs Fixing

[Alexandros Frantzis (afrantzis)]

> As for the use of configure...I'm not sure of a better way to do this until a bigger focus refactoring happens. This is "next on my list".

If you have in mind a better way to achieve this, perhaps we should treat this MP as a spike, drop it, and move on to a refactoring that will allow implementing a better solution?

review: Abstain

[Alan Griffiths (alan-griffiths)]

I assume that clog() and unclog() are intended to be paired operations - but the calls I see don't deal with exceptions being thrown (please use RAII for paired operations).

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

I really don't like RAII because it's too implicit and hidden from the reader. That said, you kind of need it for exception safety. But I would recommend against ever using exceptions too :)

[Alan Griffiths (alan-griffiths)]

> I really don't like RAII because it's too implicit and hidden from the reader.
> That said, you kind of need it for exception safety. But I would recommend
> against ever using exceptions too :)

You'd recommend against using C++ then?

RAAI and exceptions have been idiomatic C++ for two decades now.

[Daniel van Vugt (vanvugt)]

Well, for many people not using C++ or RAII has been idomatic for almost four decades ;)
</troll>

[Olli Ries (ories)]

you guys... ;)

On Mon, Jun 10, 2013 at 2:28 AM, Daniel van Vugt <
<email address hidden>> wrote:

> Well, for many people not using C++ or RAII has been idomatic for almost
> four decades ;)
> </troll>
> --
>
> https://code.launchpad.net/~robertcarr/mir/client-focus-notifications/+merge/166440
> Your team Mir development team is subscribed to branch lp:mir.
>

[Robert Carr (robertcarr)]

This is non-critical so I am putting it on hold for a few days so I can focus on the Unity stack.

[Robert Carr (robertcarr)]

It returns! With RAII on the event sink. I encountered some intermittent failures (both related, and somewhat unrelated but exposed):

MirConnection::released -> We don't want to remove the surface from valid surfaces until here because we may still be receiving configuration events until the surface is actually released on the server side.

input_testing_server_options: It's not enough to say we are ready for input when the input_registrar returns, because the shell may have not yet granted keyboard focus (fixes 1196744)

MirSurface::release_surface: There was a deadlock going on here:

22:42 < racarr> Client calls mir_surface_release it gets to MirSurface::release_surface which acquires the recursive lock on the MirSurface
22:42 < racarr> then calls MirConnection::release_surface
22:42 < racarr> which trys to acquire the connection lock, but blocks because an RPC thread is
22:42 < racarr> has called MirConnection::handle_event
22:42 < racarr> holding the lock
22:43 < racarr> but the RPC thread while executing MirConnection::handle_event tries to call
22:43 < racarr> MirSurface::handle_event
22:43 < racarr> but the Surface is already locked from the
22:43 < racarr> original client thread

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:746
http://jenkins.qa.ubuntu.com/job/mir-ci/957/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1237
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1121/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/195
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/195/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/597

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/957/rebuild

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

352 + class Impediment
353 + {
354 + public:
355 + ~Impediment()
356 + {
357 + remove_callback();
358 + }
359 + protected:
360 + friend class CloggableEventSink;
361 + Impediment(std::function<void()> const& remove) :
362 + remove_callback(remove)
363 + {
364 + }
365 + private:
366 + std::function<void()> const remove_callback;
367 + };
368 +
369 + std::unique_ptr<Impediment> clog()
370 + {
371 + std::unique_lock<std::mutex> lg(lock);
372 + clogged = true;
373 +
374 + return std::unique_ptr<Impediment>(new Impediment(
375 + std::bind(std::mem_fn(&CloggableEventSink::unclog), this)));
376 + }

This approach seems complicated to me. C.f.

template<class Cloggee>
class Clogger
{
public:
    Clogger(Cloggee& cloggee) : cloggee{cloggee} { cloggee.clog(); }
    ~Clogger() { cloggee.unclog(); }
private:
    Cloggee& cloggee;
    Clogger(Clogger const&) = delete;
    Clogger& operator=(Clogger const&) = delete;
};

[Alan Griffiths (alan-griffiths)]

438 === modified file 'src/server/frontend/socket_session.cpp'
445 + // TODO: It might make sense to use a pool for these.
446 + std::vector<char> whole_message;
...
451 === modified file 'src/server/frontend/socket_session.h'
458 - std::vector<char> whole_message;

This shouldn't be needed.

[Alan Griffiths (alan-griffiths)]

1321 msh::SingleVisibilityFocusMechanism focus_mechanism(mt::fake_shared(model),
1322 - std::make_shared<mtd::StubInputTargeter>());
1323 + std::make_shared<mtd::StubInputTargeter>());

Changes layout from one of our common styles to one we don't use.

[Alan Griffiths (alan-griffiths)]

I'm tempted to suggest -Wno-mismatched-tags to quiet clang's curious insistence that struct X is not a synonym for class X.

I've commented on a couple of oddities above - my biggest concern being that if we really need to move "whole_message" that there is an existing race condition (and that should be fixed instead of hacking around it).

Overall, the MP looks reasonable, but is a bit big for me to feel confident I understand it all.

review: Needs Information

[Robert Carr (robertcarr)]

Fixed clang compile.

I don't think the whole_message bit is required, that was introduced by me in an early iteration to attempt to fix a race that ended up being in the SessionMediator as I recall. It's been removed.

Fixed whitespace around 1321.

I'm not sure I like the Clogger class, the way it is now makes it clear how to call clog.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:749
http://jenkins.qa.ubuntu.com/job/mir-ci/966/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1249
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1133
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/204
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/204/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/605

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/966/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

I still think that clog()/unclog() are dubious.

What should happen if clog() is called in two threads and then unclog() is called in the first?

Why are you creating a unique_ptr at all? Surely, as you don't want to follow the model used in std (e.g. unique_lock), all that you need to return is a move-only type that calls unclog() in the destructor?

~~~~

234 +protected

line noise

review: Needs Fixing

[Robert Carr (robertcarr)]

Improved the clogging API and added exceptions for some exceptional conditions.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:751
http://jenkins.qa.ubuntu.com/job/mir-ci/1003/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1303
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1188
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/241
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/241/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/639

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1003/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

352 + ~CloggableEventSink()
353 + {
354 + std::unique_lock<std::mutex> lg(lock);
355 +
356 + if (clogged == true)
357 + BOOST_THROW_EXCEPTION(std::logic_error("EventSink destroyed while clogged leading to lost events"));
358 + }

See "C++ Coding Standards" Guideline 51 (this isn't the guideline I consider wrong).

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

606 +};
607 +struct MockEventObserver
608 +{
609 + MOCK_METHOD1(see, void(MirEvent const*));
610 +};
611 +struct EventObservingClient : ClientConfigCommon

A bit of whitespace between classes aids readability

[Robert Carr (robertcarr)]

Added some whitespace.

Removed the dtor. I thought it might be ok to throw in this case as it likely indicates a programmer error which should never happen.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:753
http://jenkins.qa.ubuntu.com/job/mir-ci/1011/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1312/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1197/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/249/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1011/rebuild

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

On 15/07/13 17:34, Robert Carr wrote:
> Removed the dtor. I thought it might be ok to throw in this case as it likely indicates a programmer error which should never happen.

Better to abort?

--
Alan Griffiths [phone]
Octopull Ltd http://www.octopull.co.uk/

[Alexandros Frantzis (afrantzis)]

219 +#include <initializer_list>
220 +#include <string>

Are these needed?

> 234 +protected
>
> line noise

+1. Just a nit, but there is no reason to add a protected block just for the deleted CopyAssign operations; we can put them under private.

> See "C++ Coding Standards" Guideline 51 (this isn't the guideline I consider wrong).

This also applies to

401 + ~Clog()
402 + {
403 + sink.unclog();
404 + }

since sink.unclog() can throw.

[Alexandros Frantzis (afrantzis)]

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

> On 15/07/13 17:34, Robert Carr wrote:
> > Removed the dtor. I thought it might be ok to throw in this case as it
> likely indicates a programmer error which should never happen.

It is low cost to detect and leads to undefined behavior: just put an assert here.

review: Needs Fixing

[Robert Carr (robertcarr)]

assert(asserts_present_in_revision_755)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:755
http://jenkins.qa.ubuntu.com/job/mir-ci/1025/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1330
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1215/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/262
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/262/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/654

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1025/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:755
http://jenkins.qa.ubuntu.com/job/mir-ci/1035/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1343/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1228/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/273/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/663

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1035/rebuild

review: Needs Fixing (continuous-integration)

[Robert Carr (robertcarr)]

...Merged trunk!

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:757
http://jenkins.qa.ubuntu.com/job/mir-ci/1079/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1408
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1293/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/317
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/317/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/697

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1079/rebuild

review: Needs Fixing (continuous-integration)

[Robert Carr (robertcarr)]

Last jenkins failure was just intermittent jenkins infrastructure issue. I would like this to land one day because it conflicts a lot.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:758
http://jenkins.qa.ubuntu.com/job/mir-ci/1098/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1436
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1321
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/336
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/336/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1098/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Looks like my review is way out of date now. So no comment.

review: Abstain

[Kevin DuBois (kdub)]

I think lp:~kdub/mir/prepare-for-display-notifications will help with this... EventSink now is a bit more decoupled and the interface can be developed against without messing with ProtobufSocketCommunicator at all.

[Robert Carr (robertcarr)]

Is there a specific portion of the change that people don't want to land?

[Alan Griffiths (alan-griffiths)]

My previous objection is addressed, and I don't have the headspace right now.

recursive mutexes are usually a sign of design issues, but this MP doesn't introduced a new problem.

review: Abstain

[Alexandros Frantzis (afrantzis)]

Looks good overall. One question:

486 + if (current_focus)
487 + current_focus->configure(mir_surface_attrib_focus, mir_surface_unfocused);
488 + surface->configure(mir_surface_attrib_focus, mir_surface_focused);.

Is there a way for the currently focused surface to get the focus again, and receive a confusing [unfocused,focused] sequence of events? We don't seem to guard against this, at least not at this level.

review: Approve

[Alan Griffiths (alan-griffiths)]

NB this needs to be re-sync'ed with trunk before top-approving.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:759
http://jenkins.qa.ubuntu.com/job/mir-ci/1152/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1514/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1399/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/390/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1152/rebuild

review: Needs Fixing (continuous-integration)

[Robert Carr (robertcarr)]

Merged trunk

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:760
http://jenkins.qa.ubuntu.com/job/mir-ci/1153/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1516
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1401
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/391
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/391/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1153/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

> > just curious...is this some convention ?
> > mir_surface_focus_arraysize_
> > didn't understand why it was in the surf attrib for focusstate
>
> Yes, it is a sentinel element for the array, and its value is equal to the
> array size.

Just curious - was there an array when this was written? (There isn't now, and it seems unlikely there ever was.)

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-autolanding/526/
Executed test runs:
    FAILURE: http://s-jenkins:8080/job/generic-cleanup-mbs/1194/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1554/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1439/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-autolanding/180/console

review: Needs Fixing (continuous-integration)

[Robert Carr (robertcarr)]

The merge conflicts were not entirely trivial so maybe someone wants to give it a once over?

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:761
http://jenkins.qa.ubuntu.com/job/mir-ci/1181/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1555
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1440
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/419
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/419/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1181/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-autolanding/530/
Executed test runs:
    None: http://s-jenkins:8080/job/generic-cleanup-mbs/1201/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1563/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1448/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-autolanding/184/console

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

1. Conflicts...
Text conflict in include/server/mir/shell/default_focus_mechanism.h
Text conflict in src/server/shell/default_focus_mechanism.cpp
Text conflict in tests/unit-tests/shell/test_default_focus_mechanism.cpp
3 conflicts encountered.

2. I think the past tense "sent_event" doesn't feel right, and is a confusing function name despite being similar to those above it. Maybe "on_event_sent".

3. As already mentioned, I still don't think MirSurfaceFocusState should exist at all. Making "focus" an attribute of a surface leads to dangerous and impossible situations:
  (a) Many surfaces focused simultaneously; and
  (b) surf.configure(mir_surface_attrib_focus, X) != X

4. IPCSemaphore: Everyone knows semaphores as having "wait" and "signal" operations. I'd prefer those to the verbose method names you have. But it's not important.

5. Focus/blur should be an event on the connection, not the surface. This would resolve the ambiguities mentioned in 3.

Sorry to keep touching on the same things (3, 5), but I did first raise these concerns 2 months ago. At the time you seem to have agreed saying:
  "I'm not sure of a better way to do this until a bigger focus refactoring happens. This is "next on my list"
I think we should avoid a temporary solution that adds attributes which ultimately we agree should not exist.

review: Needs Fixing

[Robert Carr (robertcarr)]

I don't understand what you mean by point 5. We need to deliver focus based on surface, i.e. you have multiple terminal windows open from the same session and they need to know whether or not to blink the cursor.

[Daniel van Vugt (vanvugt)]

(5) is really an extension of (3). Given that (3) I don't think focus should be a per-surface attribute then one would conclude (5) that the "currently focused surface" should be stored in a single location, single variable. This makes it an attribute of some other class, like maybe the session.

[Chris Halse Rogers (raof)]

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

review: Needs Fixing

[Chris Halse Rogers (raof)]

On second thoughts, maybe it's ok?

We're not storing the focus state in a surface attribute, we're storing it in the FocusStrategy and projecting that to the client as a surface state.

It's possible that there's a nicer way of projecting that data, but maybe this isn't too bad.

review: Abstain

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:766
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~robertcarr/mir/client-focus-notifications/+merge/179801/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-ci/1286/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1716
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1601
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/524/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1286/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:767
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~robertcarr/mir/client-focus-notifications/+merge/179801/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-ci/1289/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1719
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1604
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/527/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1289/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:768
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~robertcarr/mir/client-focus-notifications/+merge/179801/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-ci/1290/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1720/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1605/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/528/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1290/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:767
http://jenkins.qa.ubuntu.com/job/mir-ci/1291/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1721/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1606/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/529/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1291/rebuild

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

BTW, "enum_max_" is not accurate. It goes one past the max value, used to size fixed arrays. Hence "arraysize_".

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:771
http://jenkins.qa.ubuntu.com/job/mir-ci/1295/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1726/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1611/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/533/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1295/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:772
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~robertcarr/mir/client-focus-notifications/+merge/180011/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-ci/1296/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1727/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1612/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/534/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1296/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:773
http://jenkins.qa.ubuntu.com/job/mir-ci/1298/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1730
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1615
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/536
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/536/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1298/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

486 -<<<<<<< TREE
487 MediatingDisplayChanger(
488 std::shared_ptr<graphics::Display> const& display,
489 std::shared_ptr<compositor::Compositor> const& compositor,
490 std::shared_ptr<input::InputManager> const& input_manager,
491 std::shared_ptr<graphics::DisplayConfigurationPolicy> const& display_configuration_policy);
492 -=======
493 - explicit MediatingDisplayChanger(
494 - std::shared_ptr<graphics::Display> const& display,
495 - std::shared_ptr<compositor::Compositor> const& compositor);
496 ->>>>>>> MERGE-SOURCE

The diff is a mess. It is not a good idea to have a prerequisite branch (connect-display-request-merge-trunk) that's been rejected.

review: Needs Resubmitting

[Daniel van Vugt (vanvugt)]

What Alan said.

review: Needs Resubmitting

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:773
http://jenkins.qa.ubuntu.com/job/mir-ci/1317/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1755/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1640/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/555/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1317/rebuild

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

Would it be correct to assume this might also resolve bug 1212518?

[Daniel van Vugt (vanvugt)]

1. Trivial text conflict in tests/unit-tests/client/test_mir_connection.cpp

2. The diff is messed up and seems to remove much of the recent multi-monitor additions. Please resolve that and resubmit.

review: Needs Resubmitting

[Robert Carr (robertcarr)]

Accidental reverse merge :)

[Robert Carr (robertcarr)]

Its not showing up in the thread history because of resubmit I guess. Fixed in r 775 (Merge trunk) and r776 (back out accidental revert).

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:777
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~robertcarr/mir/client-focus-notifications/+merge/180903/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-ci/1329/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1777
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1662
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/567
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/567/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1329/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:778
http://jenkins.qa.ubuntu.com/job/mir-ci/1331/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1779
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1664
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/569
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/569/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1331/rebuild

review: Approve (continuous-integration)

[Chris Halse Rogers (raof)]

This looks good to me.

Doesn't break anything, and XMir still works ☺

review: Approve

[Alan Griffiths (alan-griffiths)]

376 -
377 +
...
435 -
436 +

Introduces trailing whitespace

~~~~
437 + // TODO: We rely on done->Run() sending messages synchronously.

What is to be done?

~~~~

I don't see how this fixes Bug #1196744 or Bug #1201435 (but I'm not convinced that either is still a problem anyway).

[Alan Griffiths (alan-griffiths)]

1101 +TEST_F(ShellSurface, focus)

This an uninformative test name. What does:

[ FAILED ] ShellSurface.focus

tell anyone?

What's wrong with "when_focus_gained_and_lost_a_surface_sends_notifications"

review: Needs Fixing

[Robert Carr (robertcarr)]

Fixed white space.

Updated the comment and added a pair comment in mfd::SocketMessenger::send.

Helped test name.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:781
http://jenkins.qa.ubuntu.com/job/mir-ci/1341/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1792
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1677
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/579
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/579/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1341/rebuild

review: Approve (continuous-integration)

[Robert Ancell (robert-ancell)]

LGTM

review: Approve

[kevin gunn (kgunn72)]

ok, reran on a more well known machine - good results
openarena 800x600
w/ bypass 109.33 w/o bypass 103 fps
openarena 1280 x 800
w/ bypass 58.4 w/o bypass 56 fps

also, rebuilt local bypass - compared to main for egltriangle demo client
w/ bypass 264 fps
w/o bypass 173 fps

review: Approve

[kevin gunn (kgunn72)]

so sorry, wrong mp

review: Abstain

[Alan Griffiths (alan-griffiths)]

Not had time to re-review. Abstaining to avoid blocking

review: Abstain

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-autolanding/608/
Executed test runs:
    FAILURE: http://s-jenkins:8080/job/generic-cleanup-mbs/1602/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1802/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1687/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-autolanding/263/console

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

test_client_focus_notification.cpp:125:13: error: missing initializer for member ‘MirSurfaceParameters::output_id’ [-Werror=missing-field-initializers]
             };
             ^

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Also, _enum_max_ is not accurate. It's not the max value but one past it. Hence the existing _arraysize_ is more appropriate.

[Daniel van Vugt (vanvugt)]

Summary:

1. Build failure:
test_client_focus_notification.cpp:125:13: error: missing initializer for member ‘MirSurfaceParameters::output_id’ [-Werror=missing-field-initializers]
             };
             ^

2. _enum_max_ is not accurate. It's not the max value but is in fact one past the max value. Hence the existing _arraysize_ is more appropriate.

3. mir_surface_attrib_focus should not exist; focus is not a per-surface attribute. For reasons I have mentioned several times since June. Focus is per input device (for non-pointing devices: keyboard, voice...). Though could probably be simplified to one focus per session. Something like: SurfaceID Session::focussed_surface() const;

review: Needs Fixing

[Robert Carr (robertcarr)]

>> 1. Build failure:

Merged trunk

>>2._enum_mx_
mir_surface_attrib_enum_max_ is very literally the maximum valid member of the mir surface attribute enumeration :p. Chris requested this change.

3.
>> mir_surface_attrib_focus should not exist; focus is not a per-surface attribute. For reasons I have mentioned several >> times since June.

You keep saying this but haven't proposed an alternative architecture.

>> Focus is per input device (for non-pointing devices: keyboard, voice...). Though could probably be simplified to >> one focus per session. Something like: SurfaceID Session::focussed_surface() const;

Actually the design docs talk about input focus as a sort of separate concept. There is certainly some notion of a surface having a focused attribute:

1. Clients need to alter their presentation based on whether they are focused, i.e. blinking cursor. It seems like this is in some sense a 'property of the surface', i.e. has some sort of input focus.
2. Which surface does the HUD act on? This has nothing to do with pointing devices.
3. Which surface does the global menu bar display from?

I think it's likely that focus will require more refinement for some multi window, multi input device, multi monitor, etc scenarios on the desktop. So far, the design documents have talked more about a hierarchy of focus though than modelling focus only as per input device. I think these requirements are unclear and trying to over generalize the system now is fruitless. On the other hand the requirements for the phone and system compositor are very clear and satisfied here.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:782
http://jenkins.qa.ubuntu.com/job/mir-ci/1357/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1817
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1702
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/595
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/595/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1357/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

OK, I've argued enough. I don't think this is the right answer, but of course don't have an alternative branch implemented either. Someone else can veto this one.

[kevin gunn (kgunn72)]

ok, we're definitely to philosophical debates

review: Approve

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'include/server/mir/frontend/message_processor_report.h'
--- include/server/mir/frontend/message_processor_report.h	2013-04-24 05:22:20 +0000
+++ include/server/mir/frontend/message_processor_report.h	2013-08-22 22:52:58 +0000
@@ -20,6 +20,8 @@
 #ifndef MIR_FRONTEND_MESSAGE_PROCESSOR_REPORT_H_
 #define MIR_FRONTEND_MESSAGE_PROCESSOR_REPORT_H_
 
+#include "mir_toolkit/event.h"
+
 #include <string>
 
 namespace mir
@@ -42,6 +44,8 @@
     virtual void exception_handled(void const* mediator, int id, std::exception const& error) = 0;
 
     virtual void exception_handled(void const* mediator, std::exception const& error) = 0;
+    
+    virtual void sent_event(void const* mediator, MirSurfaceEvent const& ev) = 0;
 
 private:
     MessageProcessorReport(MessageProcessorReport const&) = delete;

=== modified file 'include/server/mir/frontend/null_message_processor_report.h'
--- include/server/mir/frontend/null_message_processor_report.h	2013-04-24 05:22:20 +0000
+++ include/server/mir/frontend/null_message_processor_report.h	2013-08-22 22:52:58 +0000
@@ -36,6 +36,8 @@
     void exception_handled(void const*, int, std::exception const&);
 
     void exception_handled(void const*, std::exception const&);
+    
+    void sent_event(void const*, MirSurfaceEvent const& e);
 };
 }
 }

=== modified file 'include/server/mir/frontend/shell.h'
--- include/server/mir/frontend/shell.h	2013-08-01 07:44:17 +0000
+++ include/server/mir/frontend/shell.h	2013-08-22 22:52:58 +0000
@@ -44,6 +44,8 @@
 
     virtual SurfaceId create_surface_for(std::shared_ptr<Session> const& session,
                                          shell::SurfaceCreationParameters const& params) = 0;
+    
+    virtual void handle_surface_created(std::shared_ptr<Session> const& session) = 0;
 
 protected:
     Shell() = default;

=== modified file 'include/server/mir/logging/message_processor_report.h'
--- include/server/mir/logging/message_processor_report.h	2013-04-24 05:22:20 +0000
+++ include/server/mir/logging/message_processor_report.h	2013-08-22 22:52:58 +0000
@@ -68,6 +68,8 @@
 
     void exception_handled(void const* mediator, std::exception const& error);
 
+    void sent_event(void const* mediator, MirSurfaceEvent const& event);
+
     ~MessageProcessorReport() noexcept(true);
 
 private:

=== modified file 'include/server/mir/lttng/message_processor_report.h'
--- include/server/mir/lttng/message_processor_report.h	2013-06-03 12:15:44 +0000
+++ include/server/mir/lttng/message_processor_report.h	2013-08-22 22:52:58 +0000
@@ -35,6 +35,7 @@
     void unknown_method(void const* mediator, int id, std::string const& method);
     void exception_handled(void const* mediator, int id, std::exception const& error);
     void exception_handled(void const* mediator, std::exception const& error);
+    void sent_event(void const* mediator, MirSurfaceEvent const& event);
 
 private:
     ServerTracepointProvider tp_provider;

=== modified file 'include/server/mir/shell/default_focus_mechanism.h'
--- include/server/mir/shell/default_focus_mechanism.h	2013-08-02 04:07:15 +0000
+++ include/server/mir/shell/default_focus_mechanism.h	2013-08-22 22:52:58 +0000
@@ -19,14 +19,17 @@
 #ifndef MIR_SHELL_SINGLE_VISIBILITY_FOCUS_MECHANISM_H_
 #define MIR_SHELL_SINGLE_VISIBILITY_FOCUS_MECHANISM_H_
 
+#include "mir/shell/focus_setter.h"
+
 #include <memory>
-#include "mir/shell/focus_setter.h"
+#include <mutex>
 
 namespace mir
 {
 
 namespace shell
 {
+class Surface;
 class InputTargeter;
 class SurfaceController;
 
@@ -46,6 +49,9 @@
 private:
     std::shared_ptr<InputTargeter> const input_targeter;
     std::shared_ptr<SurfaceController> const surface_controller;
+
+    std::mutex surface_focus_lock;
+    std::weak_ptr<Surface> currently_focused_surface;
 };
 
 }

=== modified file 'include/server/mir/shell/session_manager.h'
--- include/server/mir/shell/session_manager.h	2013-08-15 09:15:39 +0000
+++ include/server/mir/shell/session_manager.h	2013-08-22 22:52:58 +0000
@@ -70,6 +70,8 @@
     void focus_next();
     std::weak_ptr<Session> focussed_application() const;
     void set_focus_to(std::shared_ptr<Session> const& focus);
+    
+    void handle_surface_created(std::shared_ptr<frontend::Session> const& session);
 
 protected:
     SessionManager(const SessionManager&) = delete;

=== modified file 'include/shared/mir_toolkit/common.h'
--- include/shared/mir_toolkit/common.h	2013-07-09 10:51:59 +0000
+++ include/shared/mir_toolkit/common.h	2013-08-22 22:52:58 +0000
@@ -36,7 +36,8 @@
     mir_surface_attrib_type,
     mir_surface_attrib_state,
     mir_surface_attrib_swapinterval,
-    mir_surface_attrib_arraysize_
+    mir_surface_attrib_focus,
+    mir_surface_attrib_enum_max_
 } MirSurfaceAttrib;
 
 typedef enum MirSurfaceType
@@ -48,7 +49,7 @@
     mir_surface_type_freestyle,
     mir_surface_type_popover,
     mir_surface_type_inputmethod,
-    mir_surface_type_arraysize_
+    mir_surface_type_enum_max_
 } MirSurfaceType;
 
 typedef enum MirSurfaceState
@@ -62,8 +63,16 @@
        Omitted for now, since it's functionally a subset of vertmaximized and
        differs only in the X coordinate. */
     mir_surface_state_fullscreen,
-    mir_surface_state_arraysize_
+    mir_surface_state_enum_max_
 } MirSurfaceState;
+
+typedef enum MirSurfaceFocusState
+{
+    mir_surface_unfocused = 0,
+    mir_surface_focused,
+    mir_surface_focus_enum_max_
+} MirSurfaceFocusState;
+
 /**@}*/
 
 #endif

=== modified file 'include/test/mir_test/event_matchers.h'
--- include/test/mir_test/event_matchers.h	2013-04-26 16:31:48 +0000
+++ include/test/mir_test/event_matchers.h	2013-08-22 22:52:58 +0000
@@ -19,6 +19,8 @@
 #ifndef MIR_TEST_EVENT_MATCHERS_H_
 #define MIR_TEST_EVENT_MATCHERS_H_
 
+#include "mir_toolkit/event.h"
+
 #include <androidfw/Input.h>
 
 #include <gmock/gmock.h>
@@ -64,6 +66,18 @@
     return (coords->x == dx) && (coords->y == dy);
 }
 
+MATCHER_P2(SurfaceEvent, attrib, value, "")
+{
+    if (arg.type != mir_event_type_surface)
+        return false;
+    auto surface_ev = arg.surface;
+    if (surface_ev.attrib != attrib)
+        return false;
+    if (surface_ev.value != value)
+        return false;
+    return true;
+}
+
 }
 } // namespace mir
 

=== modified file 'include/test/mir_test_doubles/mock_shell.h'
--- include/test/mir_test_doubles/mock_shell.h	2013-08-01 07:44:17 +0000
+++ include/test/mir_test_doubles/mock_shell.h	2013-08-22 22:52:58 +0000
@@ -39,6 +39,7 @@
     MOCK_METHOD1(close_session, void(std::shared_ptr<frontend::Session> const&));
 
     MOCK_METHOD2(create_surface_for, frontend::SurfaceId(std::shared_ptr<frontend::Session> const&, shell::SurfaceCreationParameters const&));
+    MOCK_METHOD1(handle_surface_created, void(std::shared_ptr<frontend::Session> const&));
 };
 
 }

=== modified file 'include/test/mir_test_doubles/stub_shell.h'
--- include/test/mir_test_doubles/stub_shell.h	2013-08-01 07:44:17 +0000
+++ include/test/mir_test_doubles/stub_shell.h	2013-08-22 22:52:58 +0000
@@ -39,10 +39,13 @@
     {
     }
     frontend::SurfaceId create_surface_for(std::shared_ptr<frontend::Session> const& /* session */,
-                                        shell::SurfaceCreationParameters const& /* params */)
+                                        shell::SurfaceCreationParameters const& /* params */) override
     {
         return frontend::SurfaceId{0};
     }
+    void handle_surface_created(std::shared_ptr<frontend::Session> const& /* session */) override
+    {
+    }
 };
 
 }

=== modified file 'include/test/mir_test_framework/input_testing_server_configuration.h'
--- include/test/mir_test_framework/input_testing_server_configuration.h	2013-07-24 05:22:03 +0000
+++ include/test/mir_test_framework/input_testing_server_configuration.h	2013-08-22 22:52:58 +0000
@@ -63,14 +63,13 @@
     void on_exit();
     
     std::shared_ptr<mir::input::InputConfiguration> the_input_configuration() override;
-    std::shared_ptr<mir::surfaces::InputRegistrar> the_input_registrar() override;
+    std::shared_ptr<mir::frontend::Shell> the_frontend_shell() override;
 
 protected:
     virtual void inject_input() = 0;
     mir::input::android::FakeEventHub* fake_event_hub;
 
     void wait_until_client_appears(std::string const& surface_name);
-    void wait_until_client_vanishes(std::string const& surface_name);
 
 private:
     std::mutex lifecycle_lock;
@@ -81,7 +80,7 @@
     std::thread input_injection_thread;
     
     std::shared_ptr<mir::test::doubles::FakeEventHubInputConfiguration> input_configuration;
-    std::shared_ptr<mir::surfaces::InputRegistrar> input_registrar;
+    std::shared_ptr<mir::frontend::Shell> frontend_shell;
 };
 
 }

=== modified file 'src/client/mir_connection.cpp'
--- src/client/mir_connection.cpp	2013-08-20 13:43:06 +0000
+++ src/client/mir_connection.cpp	2013-08-22 22:52:58 +0000
@@ -146,6 +146,10 @@
 
 void MirConnection::released(SurfaceRelease data)
 {
+    {
+        std::lock_guard<std::recursive_mutex> lock(mutex);
+        surface_map->erase(data.surface->id());
+    }
     data.callback(data.surface, data.context);
     data.handle->result_received();
     delete data.surface;
@@ -161,7 +165,6 @@
     auto new_wait_handle = new MirWaitHandle;
 
     SurfaceRelease surf_release{surface, new_wait_handle, callback, context};
-    surface_map->erase(surface->id());
 
     mir::protobuf::SurfaceId message;
     message.set_value(surface->id());

=== modified file 'src/client/mir_surface.cpp'
--- src/client/mir_surface.cpp	2013-08-20 13:39:31 +0000
+++ src/client/mir_surface.cpp	2013-08-22 22:52:58 +0000
@@ -55,7 +55,7 @@
     
     server.create_surface(0, &message, &surface, gp::NewCallback(this, &MirSurface::created, callback, context));
 
-    for (int i = 0; i < mir_surface_attrib_arraysize_; i++)
+    for (int i = 0; i < mir_surface_attrib_enum_max_; i++)
         attrib_cache[i] = -1;
     attrib_cache[mir_surface_attrib_type] = mir_surface_type_normal;
     attrib_cache[mir_surface_attrib_state] = mir_surface_state_unknown;
@@ -223,8 +223,6 @@
         mir_surface_callback callback,
         void * context)
 {
-    std::lock_guard<std::recursive_mutex> lock(mutex);
-
     return connection->release_surface(this, callback, context);
 }
 
@@ -316,6 +314,7 @@
         {
         case mir_surface_attrib_type:
         case mir_surface_attrib_state:
+        case mir_surface_attrib_focus:
         case mir_surface_attrib_swapinterval:
             if (configure_result.has_ivalue())
                 attrib_cache[a] = configure_result.ivalue();
@@ -371,7 +370,7 @@
     if (e.type == mir_event_type_surface)
     {
         MirSurfaceAttrib a = e.surface.attrib;
-        if (a < mir_surface_attrib_arraysize_)
+        if (a < mir_surface_attrib_enum_max_)
             attrib_cache[a] = e.surface.value;
     }
 

=== modified file 'src/client/mir_surface.h'
--- src/client/mir_surface.h	2013-08-13 10:27:45 +0000
+++ src/client/mir_surface.h	2013-08-22 22:52:58 +0000
@@ -123,7 +123,7 @@
     mir::protobuf::SurfaceSetting configure_result;
 
     // Cache of latest SurfaceSettings returned from the server
-    int attrib_cache[mir_surface_attrib_arraysize_];
+    int attrib_cache[mir_surface_attrib_enum_max_];
 
     std::function<void(MirEvent const*)> handle_event_callback;
     std::shared_ptr<mir::input::receiver::InputReceiverThread> input_thread;

=== modified file 'src/server/frontend/null_message_processor_report.cpp'
--- src/server/frontend/null_message_processor_report.cpp	2013-04-24 05:22:20 +0000
+++ src/server/frontend/null_message_processor_report.cpp	2013-08-22 22:52:58 +0000
@@ -39,3 +39,7 @@
 void mf::NullMessageProcessorReport::exception_handled(void const*, std::exception const&)
 {
 }
+
+void mf::NullMessageProcessorReport::sent_event(void const*, MirSurfaceEvent const&)
+{
+}

=== modified file 'src/server/frontend/session_mediator.cpp'
--- src/server/frontend/session_mediator.cpp	2013-08-20 13:39:31 +0000
+++ src/server/frontend/session_mediator.cpp	2013-08-22 22:52:58 +0000
@@ -35,17 +35,22 @@
 #include "mir/graphics/display_configuration.h"
 #include "mir/graphics/platform_ipc_package.h"
 #include "mir/frontend/client_constants.h"
+#include "mir/frontend/event_sink.h"
+
 #include "mir/geometry/rectangles.h"
 #include "client_buffer_tracker.h"
 #include "protobuf_buffer_packer.h"
 
 #include <boost/throw_exception.hpp>
 
+#include <mutex>
+#include <functional>
+
 namespace msh = mir::shell;
 namespace mf = mir::frontend;
-namespace mfd = mir::frontend::detail;
+namespace mfd=mir::frontend::detail;
+namespace mg = mir::graphics;
 namespace geom = mir::geometry;
-namespace mg = mir::graphics;
 
 mf::SessionMediator::SessionMediator(
     std::shared_ptr<frontend::Shell> const& shell,
@@ -117,10 +122,10 @@
 
         if (session.get() == nullptr)
             BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
-
+    
         report->session_create_surface_called(session->name());
 
-        auto const id = shell->create_surface_for(session,
+        auto const id = session->create_surface(
             msh::SurfaceCreationParameters()
             .of_name(request->surface_name())
             .of_size(request->width(), request->height())
@@ -128,34 +133,34 @@
             .of_pixel_format(static_cast<geometry::PixelFormat>(request->pixel_format()))
             .with_output_id(graphics::DisplayConfigurationOutputId(request->output_id()))
             );
-
-        {
-            auto surface = session->get_surface(id);
-            response->mutable_id()->set_value(id.as_value());
-            response->set_width(surface->size().width.as_uint32_t());
-            response->set_height(surface->size().height.as_uint32_t());
-            response->set_pixel_format((int)surface->pixel_format());
-            response->set_buffer_usage(request->buffer_usage());
-
-            if (surface->supports_input())
-                response->add_fd(surface->client_input_fd());
-
-            client_buffer_resource = surface->advance_client_buffer();
-            auto const& id = client_buffer_resource->id();
-
-            auto buffer = response->mutable_buffer();
-            buffer->set_buffer_id(id.as_uint32_t());
-
-            if (!client_tracker->client_has(id))
-            {
-                auto packer = std::make_shared<mfd::ProtobufBufferPacker>(buffer);
-                graphics_platform->fill_ipc_package(packer, client_buffer_resource);
-            }
-            client_tracker->add(id);
-        }
+         {
+             auto surface = session->get_surface(id);
+             response->mutable_id()->set_value(id.as_value());
+             response->set_width(surface->size().width.as_uint32_t());
+             response->set_height(surface->size().height.as_uint32_t());
+             response->set_pixel_format((int)surface->pixel_format());
+             response->set_buffer_usage(request->buffer_usage());
+             if (surface->supports_input())
+                 response->add_fd(surface->client_input_fd());
+             client_buffer_resource = surface->advance_client_buffer();
+             auto const& id = client_buffer_resource->id();
+             auto buffer = response->mutable_buffer();
+             buffer->set_buffer_id(id.as_uint32_t());
+             if (!client_tracker->client_has(id))
+             {
+                 auto packer = std::make_shared<mfd::ProtobufBufferPacker>(buffer);
+                 graphics_platform->fill_ipc_package(packer, client_buffer_resource);
+              }
+             client_tracker->add(id);
+         }
     }
 
+    // TODO: NOTE: We use the ordering here to ensure the shell acts on the surface after the surface ID is sent over the wire.
+    // This guarantees that notifications such as, gained focus, etc, can be correctly interpreted by the client. 
+    // To achieve this order we rely on done->Run() sending messages synchronously. As documented in mfd::SocketMessenger::send.
+    // this will require additional synchronization if mfd::SocketMessenger::send changes.
     done->Run();
+    shell->handle_surface_created(session);
 }
 
 void mf::SessionMediator::next_buffer(
@@ -209,6 +214,7 @@
         session->destroy_surface(id);
     }
 
+    // TODO: We rely on this sending responses synchronously.
     done->Run();
 }
 

=== modified file 'src/server/frontend/socket_messenger.cpp'
--- src/server/frontend/socket_messenger.cpp	2013-08-01 09:55:02 +0000
+++ src/server/frontend/socket_messenger.cpp	2013-08-22 22:52:58 +0000
@@ -57,6 +57,8 @@
     // TODO: This should be asynchronous, but we are not making sure
     // that a potential call to send_fds is executed _after_ this
     // function has completed (if it would be executed asynchronously.
+    // NOTE: we rely on this synchronous behavior as per the comment in
+    // mf::SessionMediator::create_surface
     ba::write(*socket, ba::buffer(whole_message));
 }
 

=== modified file 'src/server/logging/message_processor_report.cpp'
--- src/server/logging/message_processor_report.cpp	2013-04-24 05:22:20 +0000
+++ src/server/logging/message_processor_report.cpp	2013-08-22 22:52:58 +0000
@@ -162,6 +162,9 @@
         mediators.erase(mediator);
 }
 
-
-
-
+void ml::MessageProcessorReport::sent_event(void const* mediator, MirSurfaceEvent const& event)
+{
+    std::ostringstream out;
+    out << "mediator=" << mediator << ", sent event, surface id=" << event.id;
+    log->log<Logger::debug>(out.str(), component);
+}

=== modified file 'src/server/lttng/message_processor_report.cpp'
--- src/server/lttng/message_processor_report.cpp	2013-06-03 12:15:44 +0000
+++ src/server/lttng/message_processor_report.cpp	2013-08-22 22:52:58 +0000
@@ -49,3 +49,9 @@
     void const* /*mediator*/, std::exception const& /*error*/)
 {
 }
+
+void mir::lttng::MessageProcessorReport::sent_event(
+    void const* /*mediator*/, MirSurfaceEvent const&)
+{
+    // TODO
+}

=== modified file 'src/server/shell/default_focus_mechanism.cpp'
--- src/server/shell/default_focus_mechanism.cpp	2013-08-02 04:07:15 +0000
+++ src/server/shell/default_focus_mechanism.cpp	2013-08-22 22:52:58 +0000
@@ -45,6 +45,13 @@
     auto surface = focus_session->default_surface();
     if (surface)
     {
+        std::lock_guard<std::mutex> lg(surface_focus_lock);
+        auto current_focus = currently_focused_surface.lock();
+        if (current_focus)
+            current_focus->configure(mir_surface_attrib_focus, mir_surface_unfocused);
+        surface->configure(mir_surface_attrib_focus, mir_surface_focused);
+        currently_focused_surface = surface;
+        
         surface->raise(surface_controller);
         surface->take_input_focus(input_targeter);
     }

=== modified file 'src/server/shell/session_manager.cpp'
--- src/server/shell/session_manager.cpp	2013-08-15 09:15:39 +0000
+++ src/server/shell/session_manager.cpp	2013-08-22 22:52:58 +0000
@@ -150,13 +150,24 @@
     return focus_application;
 }
 
+// TODO: We use this to work around the lack of a SessionMediator-like object for internal clients.
+// we could have an internal client mediator which acts as a factory for internal clients, taking responsibility
+// for invoking handle_surface_created.
 mf::SurfaceId msh::SessionManager::create_surface_for(std::shared_ptr<mf::Session> const& session,
     msh::SurfaceCreationParameters const& params)
 {
     auto shell_session = std::dynamic_pointer_cast<Session>(session);
     auto id = shell_session->create_surface(params);
+    
+    handle_surface_created(session);
+
+    return id;
+}
+
+void msh::SessionManager::handle_surface_created(std::shared_ptr<mf::Session> const& session)
+{
+    auto shell_session = std::dynamic_pointer_cast<Session>(session);
 
     set_focus_to(shell_session);
-
-    return id;
 }
+                                                 

=== modified file 'src/server/shell/surface.cpp'
--- src/server/shell/surface.cpp	2013-08-09 03:39:35 +0000
+++ src/server/shell/surface.cpp	2013-08-22 22:52:58 +0000
@@ -242,6 +242,9 @@
             BOOST_THROW_EXCEPTION(std::logic_error("Invalid surface state."));
         result = state();
         break;
+    case mir_surface_attrib_focus:
+        notify_change(attrib, value);
+        break;
     case mir_surface_attrib_swapinterval:
         allow_dropping = (value == 0);
         allow_framedropping(allow_dropping);
@@ -267,7 +270,7 @@
 {
     bool valid = false;
 
-    if (t >= 0 && t < mir_surface_type_arraysize_)
+    if (t >= 0 && t < mir_surface_type_enum_max_)
     {
         type_value = t;
         valid = true;
@@ -286,7 +289,7 @@
     bool valid = false;
 
     if (s > mir_surface_state_unknown &&
-        s < mir_surface_state_arraysize_)
+        s < mir_surface_state_enum_max_)
     {
         state_value = s;
         valid = true;

=== modified file 'tests/acceptance-tests/CMakeLists.txt'
--- tests/acceptance-tests/CMakeLists.txt	2013-08-20 13:39:31 +0000
+++ tests/acceptance-tests/CMakeLists.txt	2013-08-22 22:52:58 +0000
@@ -10,6 +10,7 @@
   test_test_framework.cpp
   test_focus_selection.cpp
   test_server_shutdown.cpp
+  test_client_focus_notification.cpp
   test_client_authorization.cpp
   test_shell_control_of_surface_configuration.cpp
   test_nested_mir.cpp

=== added file 'tests/acceptance-tests/test_client_focus_notification.cpp'
--- tests/acceptance-tests/test_client_focus_notification.cpp	1970-01-01 00:00:00 +0000
+++ tests/acceptance-tests/test_client_focus_notification.cpp	2013-08-22 22:52:58 +0000
@@ -0,0 +1,232 @@
+/*
+ * Copyright © 2013 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Robert Carr <[e-mail]>
+ */
+
+#include "mir_toolkit/mir_client_library.h"
+
+#include "mir_test/wait_condition.h"
+#include "mir_test/event_matchers.h"
+
+#include "mir_test_framework/display_server_test_fixture.h"
+#include "mir_test_framework/cross_process_sync.h"
+
+#include <gtest/gtest.h>
+#include <gmock/gmock.h>
+
+namespace mt = mir::test;
+namespace mtf = mir_test_framework;
+
+namespace
+{
+    char const* const mir_test_socket = mtf::test_socket_file().c_str();
+}
+
+namespace
+{
+struct ClientConfigCommon : TestingClientConfiguration
+{
+    ClientConfigCommon() :
+        connection(0),
+        surface(0)
+    {
+    }
+    static void connection_callback(MirConnection* connection, void* context)
+    {
+        ClientConfigCommon* config = reinterpret_cast<ClientConfigCommon *>(context);
+        config->connection = connection;
+    }
+    static void create_surface_callback(MirSurface* surface, void* context)
+    {
+        ClientConfigCommon* config = reinterpret_cast<ClientConfigCommon *>(context);
+        config->surface_created(surface);
+    }
+    static void release_surface_callback(MirSurface* surface, void* context)
+    {
+        ClientConfigCommon* config = reinterpret_cast<ClientConfigCommon *>(context);
+        config->surface_released(surface);
+    }
+    virtual void connected(MirConnection* new_connection)
+    {
+        connection = new_connection;
+    }
+    virtual void surface_created(MirSurface* new_surface)
+    {
+        surface = new_surface;
+    }
+    virtual void surface_released(MirSurface* /* released_surface */)
+    {
+        surface = nullptr;
+    }
+    MirConnection* connection;
+    MirSurface* surface;
+};
+
+struct MockEventObserver
+{
+    MOCK_METHOD1(see, void(MirEvent const*));
+};
+
+struct EventObservingClient : ClientConfigCommon
+{
+    EventObservingClient()
+        : observer(std::make_shared<MockEventObserver>())
+    {
+    }
+
+    static void handle_event(MirSurface* /* surface */, MirEvent const* ev, void* context)
+    {
+        auto client = static_cast<EventObservingClient *>(context);
+        client->observer->see(ev);
+    }
+ 
+    virtual void expect_events(mt::WaitCondition* /* all_events_received */) = 0;
+
+    void surface_created(MirSurface *surface_) override
+    {
+        // We need to set the event delegate from the surface_created
+        // callback so we can block the reading of new events
+        // until we are ready
+        MirEventDelegate const event_delegate =
+            {
+                handle_event,
+                this
+            };
+        surface = surface_;
+        mir_surface_set_event_handler(surface, &event_delegate);
+    }
+
+    void exec()
+    {
+        mt::WaitCondition all_events_received;
+        expect_events(&all_events_received);
+        mir_wait_for(mir_connect(mir_test_socket,
+            __PRETTY_FUNCTION__, connection_callback, this));
+        ASSERT_TRUE(connection != NULL);
+        MirSurfaceParameters const request_params =
+            {
+                __PRETTY_FUNCTION__,
+                surface_width, surface_height,
+                mir_pixel_format_abgr_8888,
+                mir_buffer_usage_hardware,
+                mir_display_output_id_invalid
+            };
+        mir_wait_for(mir_connection_create_surface(connection, &request_params, create_surface_callback, this));
+        all_events_received.wait_for_at_most_seconds(60);
+        mir_surface_release_sync(surface);
+        mir_connection_release(connection);
+
+        // The ClientConfig is not destroyed before the testing process 
+        // exits.
+        observer.reset();
+    }
+    std::shared_ptr<MockEventObserver> observer;
+    static int const surface_width = 100;
+    static int const surface_height = 100;
+};
+
+}
+
+TEST_F(BespokeDisplayServerTestFixture, a_surface_is_notified_of_receiving_focus)
+{
+    using namespace ::testing;
+
+    TestingServerConfiguration server_config;
+    launch_server_process(server_config);
+
+    struct FocusObservingClient : public EventObservingClient
+    {
+        void expect_events(mt::WaitCondition* all_events_received) override
+        {
+            EXPECT_CALL(*observer, see(Pointee(mt::SurfaceEvent(mir_surface_attrib_focus, mir_surface_focused)))).Times(1)
+                .WillOnce(mt::WakeUp(all_events_received));
+        }
+    } client_config;
+    launch_client_process(client_config);
+}
+
+namespace
+{
+
+ACTION_P(SignalFence, fence)
+{
+    fence->try_signal_ready_for();
+}
+
+}
+
+TEST_F(BespokeDisplayServerTestFixture, two_surfaces_are_notified_of_gaining_and_losing_focus)
+{
+    using namespace ::testing;
+    
+    TestingServerConfiguration server_config;
+    launch_server_process(server_config);
+
+    // We use this for synchronization to ensure the two clients
+    // are launched in a defined order.
+    mtf::CrossProcessSync ready_for_second_client;
+
+    struct FocusObservingClientOne : public EventObservingClient
+    {
+        mtf::CrossProcessSync ready_for_second_client;
+        FocusObservingClientOne(mtf::CrossProcessSync const& ready_for_second_client)
+            : ready_for_second_client(ready_for_second_client)
+        {
+        }
+
+        void expect_events(mt::WaitCondition* all_events_received) override
+        {
+            InSequence seq;
+            // We should receive focus as we are created
+            EXPECT_CALL(*observer, see(Pointee(mt::SurfaceEvent(mir_surface_attrib_focus,
+                mir_surface_focused)))).Times(1)
+                    .WillOnce(SignalFence(&ready_for_second_client));
+
+            // And lose it as the second surface is created
+            EXPECT_CALL(*observer, see(Pointee(mt::SurfaceEvent(mir_surface_attrib_focus,
+                mir_surface_unfocused)))).Times(1);
+            // And regain it when the second surface is closed
+            EXPECT_CALL(*observer, see(Pointee(mt::SurfaceEvent(mir_surface_attrib_focus,
+                mir_surface_focused)))).Times(1).WillOnce(mt::WakeUp(all_events_received));
+        }
+
+    } client_one_config(ready_for_second_client);
+    launch_client_process(client_one_config);
+
+    struct FocusObservingClientTwo : public EventObservingClient
+    {
+        mtf::CrossProcessSync ready_for_second_client;
+        FocusObservingClientTwo(mtf::CrossProcessSync const& ready_for_second_client)
+            : ready_for_second_client(ready_for_second_client)
+        {
+        }
+        void exec() override
+        {
+            // We need some synchronization to ensure client two does not connect before client one.
+            ready_for_second_client.wait_for_signal_ready_for();
+            EventObservingClient::exec();
+        }
+
+        void expect_events(mt::WaitCondition* all_events_received) override
+        {
+            EXPECT_CALL(*observer, see(Pointee(
+                mt::SurfaceEvent(mir_surface_attrib_focus, mir_surface_focused))))
+                    .Times(1).WillOnce(mt::WakeUp(all_events_received));
+        }
+    } client_two_config(ready_for_second_client);
+
+    launch_client_process(client_two_config);
+}

=== modified file 'tests/acceptance-tests/test_client_input.cpp'
--- tests/acceptance-tests/test_client_input.cpp	2013-08-20 13:39:31 +0000
+++ tests/acceptance-tests/test_client_input.cpp	2013-08-22 22:52:58 +0000
@@ -154,8 +154,10 @@
 
     static void handle_input(MirSurface* /* surface */, MirEvent const* ev, void* context)
     {
+        if (ev->type == mir_event_type_surface)
+            return;
+
         auto client = static_cast<InputClient *>(context);
-
         client->handler->handle_input(ev);
     }
 

=== modified file 'tests/acceptance-tests/test_focus_selection.cpp'
--- tests/acceptance-tests/test_focus_selection.cpp	2013-08-20 13:39:31 +0000
+++ tests/acceptance-tests/test_focus_selection.cpp	2013-08-22 22:52:58 +0000
@@ -38,6 +38,7 @@
 namespace mi = mir::input;
 namespace mt = mir::test;
 namespace mtd = mt::doubles;
+namespace mt = mir::test;
 namespace mtf = mir_test_framework;
 
 namespace

=== modified file 'tests/acceptance-tests/test_shell_control_of_surface_configuration.cpp'
--- tests/acceptance-tests/test_shell_control_of_surface_configuration.cpp	2013-08-20 13:39:31 +0000
+++ tests/acceptance-tests/test_shell_control_of_surface_configuration.cpp	2013-08-22 22:52:58 +0000
@@ -82,6 +82,9 @@
         void exec() override
         {
             using namespace ::testing;
+            
+            EXPECT_CALL(mock_configurator, select_attribute_value(_, mir_surface_attrib_focus, _)).Times(AnyNumber());
+            EXPECT_CALL(mock_configurator, attribute_set(_, mir_surface_attrib_focus, _)).Times(AnyNumber());
 
             ON_CALL(mock_configurator, select_attribute_value(_, _, _)).WillByDefault(Return(mir_surface_type_freestyle));
             EXPECT_CALL(mock_configurator, select_attribute_value(_, mir_surface_attrib_type, Eq(mir_surface_type_freestyle))).Times(1);
@@ -117,6 +120,10 @@
         void exec() override
         {
             using namespace ::testing;
+
+            EXPECT_CALL(mock_configurator, select_attribute_value(_, mir_surface_attrib_focus, _)).Times(AnyNumber());
+            EXPECT_CALL(mock_configurator, attribute_set(_, mir_surface_attrib_focus, _)).Times(AnyNumber());
+
             EXPECT_CALL(mock_configurator, select_attribute_value(_, mir_surface_attrib_type, Eq(mir_surface_type_freestyle))).Times(1)
                 .WillOnce(Return(mir_surface_type_normal));
             EXPECT_CALL(mock_configurator, attribute_set(_, mir_surface_attrib_type, Eq(mir_surface_type_normal))).Times(1);

=== modified file 'tests/mir_test_framework/input_testing_server_options.cpp'
--- tests/mir_test_framework/input_testing_server_options.cpp	2013-08-07 07:18:03 +0000
+++ tests/mir_test_framework/input_testing_server_options.cpp	2013-08-22 22:52:58 +0000
@@ -21,6 +21,9 @@
 #include "mir/input/input_channel.h"
 #include "mir/surfaces/input_registrar.h"
 #include "mir/input/surface.h"
+#include "mir/shell/surface_creation_parameters.h"
+#include "mir/frontend/shell.h"
+#include "mir/frontend/session.h"
 #include "mir/input/composite_event_filter.h"
 
 #include "mir_test/fake_event_hub.h"
@@ -34,6 +37,8 @@
 namespace mtf = mir_test_framework;
 
 namespace ms = mir::surfaces;
+namespace msh = mir::shell;
+namespace mf = mir::frontend;
 namespace mg = mir::graphics;
 namespace mi = mir::input;
 namespace mia = mi::android;
@@ -48,7 +53,6 @@
     virtual ~SurfaceReadinessListener() = default;
     
     virtual void channel_ready_for_input(std::string const& channel_name) = 0;
-    virtual void channel_finished_for_input(std::string const& channel_name) = 0;
 
 protected:
     SurfaceReadinessListener() = default;
@@ -56,36 +60,43 @@
     SurfaceReadinessListener& operator=(SurfaceReadinessListener const&) = delete;
 };
 
-class ProxyInputRegistrar : public ms::InputRegistrar
+class ProxyShell : public mf::Shell
 {
 public:
-    ProxyInputRegistrar(std::shared_ptr<ms::InputRegistrar> const underlying_registrar,
-                        std::shared_ptr<SurfaceReadinessListener> const listener)
-        : underlying_registrar(underlying_registrar),
+    ProxyShell(std::shared_ptr<mf::Shell> const& underlying_shell,
+               std::shared_ptr<SurfaceReadinessListener> const listener)
+        : underlying_shell(underlying_shell),
           listener(listener)
     {   
     }
 
-    ~ProxyInputRegistrar() noexcept(true) = default;
-    
-    void input_channel_opened(std::shared_ptr<mi::InputChannel> const& opened_channel,
-                              std::shared_ptr<mi::Surface> const& surface,
-                              mi::InputReceptionMode input_mode)
-    {
-        outstanding_channels[opened_channel] = surface->name();
-        underlying_registrar->input_channel_opened(opened_channel, surface, input_mode);
-        listener->channel_ready_for_input(surface->name());
-    }
-    void input_channel_closed(std::shared_ptr<mi::InputChannel> const& closed_channel)
-    {
-        auto channel = outstanding_channels[closed_channel];
-        underlying_registrar->input_channel_closed(closed_channel);
-        listener->channel_finished_for_input(channel);
+    ~ProxyShell() noexcept(true) = default;
+    
+    mf::SurfaceId create_surface_for(std::shared_ptr<mf::Session> const& session,
+        msh::SurfaceCreationParameters const& params)
+    {
+        return underlying_shell->create_surface_for(session, params);
+    }
+
+    std::shared_ptr<mf::Session> open_session(std::string const& name, 
+                                              std::shared_ptr<mf::EventSink> const& sink)
+    {
+        return underlying_shell->open_session(name, sink);
+    }
+
+    void close_session(std::shared_ptr<mf::Session> const& session)
+    {
+        underlying_shell->close_session(session);
+    }
+    
+    void handle_surface_created(std::shared_ptr<mf::Session> const& session)
+    {
+        underlying_shell->handle_surface_created(session);
+        listener->channel_ready_for_input(session->name());
     }
 
 private:
-    std::map<std::shared_ptr<mi::InputChannel>, std::string> outstanding_channels;
-    std::shared_ptr<ms::InputRegistrar> const underlying_registrar;
+    std::shared_ptr<mf::Shell> const underlying_shell;
     std::shared_ptr<SurfaceReadinessListener> const listener;
 };
 
@@ -126,7 +137,7 @@
     return input_configuration;
 }
 
-std::shared_ptr<ms::InputRegistrar> mtf::InputTestingServerConfiguration::the_input_registrar()
+std::shared_ptr<mf::Shell> mtf::InputTestingServerConfiguration::the_frontend_shell()
 {
     struct LifecycleTracker : public SurfaceReadinessListener
     {
@@ -145,27 +156,20 @@
             lifecycle_condition.notify_all();
         }
 
-        void channel_finished_for_input(std::string const& channel_name)
-        {
-            std::unique_lock<std::mutex> lg(lifecycle_lock);
-            client_lifecycles[channel_name] = mtf::ClientLifecycleState::vanished;
-            lifecycle_condition.notify_all();
-        }
         std::mutex &lifecycle_lock;
         std::condition_variable &lifecycle_condition;
         std::map<std::string, mtf::ClientLifecycleState> &client_lifecycles;
     };
     
-    if (!input_registrar)
+    if (!frontend_shell)
     {
-        auto registrar_listener = std::make_shared<LifecycleTracker>(lifecycle_lock,
+        auto readiness_listener = std::make_shared<LifecycleTracker>(lifecycle_lock,
             lifecycle_condition,
             client_lifecycles);
-        input_registrar = std::make_shared<ProxyInputRegistrar>(the_input_configuration()->the_input_registrar(),
-            registrar_listener);
+        frontend_shell = std::make_shared<ProxyShell>(DefaultServerConfiguration::the_frontend_shell(), readiness_listener);
     }
 
-    return input_registrar;
+    return frontend_shell;
 }
 
 void mtf::InputTestingServerConfiguration::wait_until_client_appears(std::string const& channel_name)
@@ -185,22 +189,3 @@
             BOOST_THROW_EXCEPTION(std::runtime_error("Timed out waiting for client (" + channel_name + ") to appear"));
     }
 }
-
-void mtf::InputTestingServerConfiguration::wait_until_client_vanishes(std::string const& channel_name)
-{
-    std::unique_lock<std::mutex> lg(lifecycle_lock);
-
-    std::chrono::seconds timeout(60);
-    auto end_time = std::chrono::system_clock::now() + timeout;
-    
-
-    if (client_lifecycles[channel_name] == appeared)
-    {
-        BOOST_THROW_EXCEPTION(std::runtime_error("Waiting for a client (" + channel_name + ") to vanish but it has already appeared"));
-    }
-    while (client_lifecycles[channel_name] != vanished)
-    {
-        if (lifecycle_condition.wait_until(lg, end_time) == std::cv_status::timeout)
-            BOOST_THROW_EXCEPTION(std::runtime_error("Timed out waiting for client (" + channel_name + ") to vanish"));
-    }
-}

=== modified file 'tests/unit-tests/shell/test_default_focus_mechanism.cpp'
--- tests/unit-tests/shell/test_default_focus_mechanism.cpp	2013-08-09 22:48:27 +0000
+++ tests/unit-tests/shell/test_default_focus_mechanism.cpp	2013-08-22 22:52:58 +0000
@@ -67,6 +67,32 @@
     focus_mechanism.set_focus_to(mt::fake_shared(app1));
 }
 
+TEST(DefaultFocusMechanism, mechanism_notifies_default_surface_of_focus_changes)
+{
+    using namespace ::testing;
+
+    NiceMock<mtd::MockShellSession> app1, app2;
+    mtd::MockSurface mock_surface1(&app1, std::make_shared<mtd::StubSurfaceBuilder>());
+    mtd::MockSurface mock_surface2(&app2, std::make_shared<mtd::StubSurfaceBuilder>());
+    
+    ON_CALL(app1, default_surface()).WillByDefault(Return(mt::fake_shared(mock_surface1)));
+    ON_CALL(app2, default_surface()).WillByDefault(Return(mt::fake_shared(mock_surface2)));
+
+    
+    {
+        InSequence seq;
+        EXPECT_CALL(mock_surface1, configure(mir_surface_attrib_focus, mir_surface_focused)).Times(1);
+        EXPECT_CALL(mock_surface1, configure(mir_surface_attrib_focus, mir_surface_unfocused)).Times(1);
+        EXPECT_CALL(mock_surface2, configure(mir_surface_attrib_focus, mir_surface_focused)).Times(1);
+    }
+
+    msh::DefaultFocusMechanism focus_mechanism(std::make_shared<mtd::StubInputTargeter>(),
+                                                        std::make_shared<mtd::StubSurfaceController>());
+
+    focus_mechanism.set_focus_to(mt::fake_shared(app1));
+    focus_mechanism.set_focus_to(mt::fake_shared(app2));
+}
+
 TEST(DefaultFocusMechanism, sets_input_focus)
 {
     using namespace ::testing;

=== modified file 'tests/unit-tests/shell/test_surface.cpp'
--- tests/unit-tests/shell/test_surface.cpp	2013-08-09 03:39:35 +0000
+++ tests/unit-tests/shell/test_surface.cpp	2013-08-22 22:52:58 +0000
@@ -21,6 +21,8 @@
 #include "mir/shell/surface_creation_parameters.h"
 #include "mir/surfaces/surface_stack_model.h"
 #include "mir/shell/surface_builder.h"
+#include "mir/frontend/event_sink.h"
+#include "mir/graphics/display_configuration.h"
 
 #include "mir_test_doubles/stub_surface_controller.h"
 #include "mir_test_doubles/mock_surface_controller.h"
@@ -35,6 +37,7 @@
 #include "mir_test_doubles/null_surface_configurator.h"
 #include "mir_test_doubles/mock_surface_configurator.h"
 #include "mir_test/fake_shared.h"
+#include "mir_test/event_matchers.h"
 
 #include <stdexcept>
 #include <gmock/gmock.h>
@@ -52,6 +55,14 @@
 
 namespace
 {
+
+struct MockEventSink : public mf::EventSink
+{
+    ~MockEventSink() noexcept(true) {}
+    MOCK_METHOD1(handle_event, void(MirEvent const&));
+    MOCK_METHOD1(handle_display_config_change, void(mg::DisplayConfiguration const&));
+};
+
 class StubSurfaceBuilder : public msh::SurfaceBuilder
 {
 public:
@@ -454,6 +465,29 @@
     EXPECT_EQ(mir_surface_state_fullscreen, surf.state());
 }
 
+TEST_F(ShellSurface, sends_focus_notifications_when_focus_gained_and_lost)
+{
+    using namespace testing;
+
+    MockEventSink sink;
+    
+    {
+        InSequence seq;
+        EXPECT_CALL(sink, handle_event(mt::SurfaceEvent(mir_surface_attrib_focus, mir_surface_focused)))
+            .Times(1);
+        EXPECT_CALL(sink, handle_event(mt::SurfaceEvent(mir_surface_attrib_focus, mir_surface_unfocused)))
+            .Times(1);
+    }
+
+    msh::Surface surf(
+            nullptr,
+            mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
+            msh::a_surface(), stub_id, mt::fake_shared(sink));
+
+    surf.configure(mir_surface_attrib_focus, mir_surface_focused);
+    surf.configure(mir_surface_attrib_focus, mir_surface_unfocused);
+}
+
 TEST_F(ShellSurface, configurator_selects_attribute_values)
 {
     using namespace testing;