Code review comment for lp:~raof/mir/1hz-rendering-always

Conflicts with lp:~andreas-pokorny/mir/add-timer-to-main-loop

Text conflict in include/server/mir/asio_main_loop.h
Text conflict in include/server/mir/default_server_configuration.h
Text conflict in src/server/asio_main_loop.cpp
Text conflict in tests/mir_test/CMakeLists.txt
Text conflict in tests/unit-tests/test_asio_main_loop.cpp

~~~
2146 + clock->register_time_change_callback([state_copy](mt::FakeClock::time_point now)
2147 + {
2148 + if (state_copy->state == Pending)
2149 + {
2150 + if (now > state_copy->threshold)
2151 + {
2152 + state_copy->state = Triggered;
2153 + state_copy->callback();
2154 + return false;
2155 + }
2156 + return true;
2157 + }
2158 + else
2159 + {
2160 + return false;
2161 + }
2162 + });
~~~

This indentation looks awkward. Maybe a private method for that multi-line lambda?

~~~
2568 + auto buffers_swapped = std::make_shared<mt::Signal>();
2569 +
2570 + q.client_acquire([buffers_swapped](mg::Buffer*){ buffers_swapped->raise(); });
2571 +
2572 + EXPECT_FALSE(buffers_swapped->wait_for(std::chrono::milliseconds{100}));
2573 +
2574 + policy_factory.trigger_policies();
2575 + EXPECT_TRUE(buffers_swapped->wait_for(std::chrono::milliseconds{100}));
~~~

This can be replaced with:

---
auto handle = client_acquire_async(q);
EXPECT_THAT(handle->has_acquired_buffer(), Eq(false));
policy_factory.trigger_policies();
EXPECT_THAT(handle->has_acquired_buffer(), Eq(true));
---

~~~
2598 + auto first_swap = std::make_shared<mt::Signal>();
2599 + auto second_swap = std::make_shared<mt::Signal>();
2600 +
2601 + /* Queue up two pending swaps */
2602 + mg::Buffer* client_buf;
2603 + q.client_acquire([first_swap, &client_buf](mg::Buffer* buffer)
2604 + {
2605 + client_buf = buffer;
2606 + first_swap->raise();
2607 + });
2608 + q.client_acquire([second_swap](mg::Buffer*){ second_swap->raise(); });
2609 +
2610 + ASSERT_FALSE(first_swap->raised());
2611 + ASSERT_FALSE(second_swap->raised());
2612 +
2613 + q.compositor_acquire(nullptr);
2614 +
2615 + EXPECT_TRUE(first_swap->wait_for(std::chrono::milliseconds{100}));
2616 + EXPECT_FALSE(second_swap->raised());
2617 +
2618 + /* We have to release a client buffer here; framedropping or not,
2619 + * a client can't have 2 buffers outstanding in the nbuffers = 2 case.
2620 + */
2621 + q.client_release(client_buf);
2622 +
2623 + policy_factory.trigger_policies();
2624 + EXPECT_TRUE(second_swap->wait_for(std::chrono::milliseconds{100}));
~~~

This can be replaced with:
---
/* Queue up two pending swaps */
auto handle1 = client_acquire_async(q);
auto handle2 = client_acquire_async(q);
ASSERT_THAT(handle1->has_acquired_buffer(), Eq(false));
ASSERT_THAT(handle2->has_acquired_buffer(), Eq(false));

q.compositor_acquire(this);

EXPECT_THAT(handle1->has_acquired_buffer(), Eq(true));
EXPECT_THAT(handle2->has_acquired_buffer(), Eq(false));

handle1->release_buffer();
policy_factory.trigger_policies();
EXPECT_THAT(handle2->has_acquired_buffer(), Eq(true));
---

~~~
2630 +TEST_F(BufferQueueTest, compositor_swapping_at_min_rate_gets_oldest_buffer)
~~~

That test implementation still depends on the WatchDog class.