Use Result<InputPublisher::Finished> instead of callback -- try 2
When the 'Finished' message is received inside InputDispatcher, we are
currently providing a callback function that gets executed with the
parameters that are matching the InputMessage fields.
This does not scale well for the case where there is more than 1 type of
InputMessage received from the InputConsumer. We would have to provide 2
callbacks, which is not user-friendly.
The calling code inside InputDispatcher is already aware of the
InputMessage struct, but InputMessage is intended to be a protocol of
communication between input channels, and is not meant to be used
elsewhere. To provide the output of 'finished' signal, we introduce a
new 'Finished' struct into InputPublisher. InputPublisher will now try
to read a message, and will provide a response in that struct.
This approach will also now force the caller to check ok(), which will
increase correctness.
Bug: 167947340
Test: atest inputflinger_tests
Test: TBD
Revert submission 13838212-revert-13780058-receiveFinishedSignal-UGCLLLUBPW
Reason for revert: Relanding with fix
Reverted Changes:
Idb3a44b4a:Revert "Update the usage of receiveFinishedSignal"...
I1e71010f5:Revert "Use Result<InputPublisher::Finished> inste...
Change-Id: I9c425bb7249d43648e558214e40fa35aeaa0bb11
diff --git a/include/input/InputTransport.h b/include/input/InputTransport.h
index f1b2258..3e5674e 100644
--- a/include/input/InputTransport.h
+++ b/include/input/InputTransport.h
@@ -33,6 +33,7 @@
#include <unordered_map>
#include <android-base/chrono_utils.h>
+#include <android-base/result.h>
#include <android-base/unique_fd.h>
#include <binder/IBinder.h>
@@ -374,20 +375,24 @@
*/
status_t publishDragEvent(uint32_t seq, int32_t eventId, float x, float y, bool isExiting);
+ struct Finished {
+ uint32_t seq;
+ bool handled;
+ nsecs_t consumeTime;
+ };
+
/* Receives the finished signal from the consumer in reply to the original dispatch signal.
- * If a signal was received, returns the message sequence number,
- * whether the consumer handled the message, and the time the event was first read by the
- * consumer.
+ * If a signal was received, returns a Finished object.
*
* The returned sequence number is never 0 unless the operation failed.
*
- * Returns OK on success.
- * Returns WOULD_BLOCK if there is no signal present.
- * Returns DEAD_OBJECT if the channel's peer has been closed.
- * Other errors probably indicate that the channel is broken.
+ * Returned error codes:
+ * OK on success.
+ * WOULD_BLOCK if there is no signal present.
+ * DEAD_OBJECT if the channel's peer has been closed.
+ * Other errors probably indicate that the channel is broken.
*/
- status_t receiveFinishedSignal(
- const std::function<void(uint32_t seq, bool handled, nsecs_t consumeTime)>& callback);
+ android::base::Result<Finished> receiveFinishedSignal();
private:
std::shared_ptr<InputChannel> mChannel;
diff --git a/libs/input/InputTransport.cpp b/libs/input/InputTransport.cpp
index 6ef0173..c2a3cf1 100644
--- a/libs/input/InputTransport.cpp
+++ b/libs/input/InputTransport.cpp
@@ -629,24 +629,26 @@
return mChannel->sendMessage(&msg);
}
-status_t InputPublisher::receiveFinishedSignal(
- const std::function<void(uint32_t seq, bool handled, nsecs_t consumeTime)>& callback) {
+android::base::Result<InputPublisher::Finished> InputPublisher::receiveFinishedSignal() {
if (DEBUG_TRANSPORT_ACTIONS) {
- ALOGD("channel '%s' publisher ~ receiveFinishedSignal", mChannel->getName().c_str());
+ ALOGD("channel '%s' publisher ~ %s", mChannel->getName().c_str(), __func__);
}
InputMessage msg;
status_t result = mChannel->receiveMessage(&msg);
if (result) {
- return result;
+ return android::base::Error(result);
}
if (msg.header.type != InputMessage::Type::FINISHED) {
ALOGE("channel '%s' publisher ~ Received unexpected %s message from consumer",
mChannel->getName().c_str(), NamedEnum::string(msg.header.type).c_str());
- return UNKNOWN_ERROR;
+ return android::base::Error(UNKNOWN_ERROR);
}
- callback(msg.header.seq, msg.body.finished.handled, msg.body.finished.consumeTime);
- return OK;
+ return Finished{
+ .seq = msg.header.seq,
+ .handled = msg.body.finished.handled,
+ .consumeTime = msg.body.finished.consumeTime,
+ };
}
// --- InputConsumer ---
diff --git a/libs/input/tests/InputPublisherAndConsumer_test.cpp b/libs/input/tests/InputPublisherAndConsumer_test.cpp
index b5ed8d7..fc31715 100644
--- a/libs/input/tests/InputPublisherAndConsumer_test.cpp
+++ b/libs/input/tests/InputPublisherAndConsumer_test.cpp
@@ -27,6 +27,8 @@
#include <utils/StopWatch.h>
#include <utils/Timers.h>
+using android::base::Result;
+
namespace android {
class InputPublisherAndConsumerTest : public testing::Test {
@@ -122,23 +124,13 @@
ASSERT_EQ(OK, status)
<< "consumer sendFinishedSignal should return OK";
- uint32_t finishedSeq = 0;
- bool handled = false;
- nsecs_t consumeTime;
- status = mPublisher->receiveFinishedSignal(
- [&finishedSeq, &handled, &consumeTime](uint32_t inSeq, bool inHandled,
- nsecs_t inConsumeTime) -> void {
- finishedSeq = inSeq;
- handled = inHandled;
- consumeTime = inConsumeTime;
- });
- ASSERT_EQ(OK, status)
- << "publisher receiveFinishedSignal should return OK";
- ASSERT_EQ(seq, finishedSeq)
- << "publisher receiveFinishedSignal should have returned the original sequence number";
- ASSERT_TRUE(handled)
- << "publisher receiveFinishedSignal should have set handled to consumer's reply";
- ASSERT_GE(consumeTime, publishTime)
+ Result<InputPublisher::Finished> result = mPublisher->receiveFinishedSignal();
+ ASSERT_TRUE(result.ok()) << "publisher receiveFinishedSignal should return OK";
+ ASSERT_EQ(seq, result->seq)
+ << "receiveFinishedSignal should have returned the original sequence number";
+ ASSERT_TRUE(result->handled)
+ << "receiveFinishedSignal should have set handled to consumer's reply";
+ ASSERT_GE(result->consumeTime, publishTime)
<< "finished signal's consume time should be greater than publish time";
}
@@ -272,23 +264,13 @@
ASSERT_EQ(OK, status)
<< "consumer sendFinishedSignal should return OK";
- uint32_t finishedSeq = 0;
- bool handled = true;
- nsecs_t consumeTime;
- status = mPublisher->receiveFinishedSignal(
- [&finishedSeq, &handled, &consumeTime](uint32_t inSeq, bool inHandled,
- nsecs_t inConsumeTime) -> void {
- finishedSeq = inSeq;
- handled = inHandled;
- consumeTime = inConsumeTime;
- });
- ASSERT_EQ(OK, status)
- << "publisher receiveFinishedSignal should return OK";
- ASSERT_EQ(seq, finishedSeq)
- << "publisher receiveFinishedSignal should have returned the original sequence number";
- ASSERT_FALSE(handled)
- << "publisher receiveFinishedSignal should have set handled to consumer's reply";
- ASSERT_GE(consumeTime, publishTime)
+ Result<InputPublisher::Finished> result = mPublisher->receiveFinishedSignal();
+ ASSERT_TRUE(result.ok()) << "receiveFinishedSignal should return OK";
+ ASSERT_EQ(seq, result->seq)
+ << "receiveFinishedSignal should have returned the original sequence number";
+ ASSERT_FALSE(result->handled)
+ << "receiveFinishedSignal should have set handled to consumer's reply";
+ ASSERT_GE(result->consumeTime, publishTime)
<< "finished signal's consume time should be greater than publish time";
}
@@ -322,22 +304,14 @@
status = mConsumer->sendFinishedSignal(seq, true);
ASSERT_EQ(OK, status) << "consumer sendFinishedSignal should return OK";
- uint32_t finishedSeq = 0;
- bool handled = false;
- nsecs_t consumeTime;
- status = mPublisher->receiveFinishedSignal(
- [&finishedSeq, &handled, &consumeTime](uint32_t inSeq, bool inHandled,
- nsecs_t inConsumeTime) -> void {
- finishedSeq = inSeq;
- handled = inHandled;
- consumeTime = inConsumeTime;
- });
- ASSERT_EQ(OK, status) << "publisher receiveFinishedSignal should return OK";
- ASSERT_EQ(seq, finishedSeq)
- << "publisher receiveFinishedSignal should have returned the original sequence number";
- ASSERT_TRUE(handled)
- << "publisher receiveFinishedSignal should have set handled to consumer's reply";
- ASSERT_GE(consumeTime, publishTime)
+ Result<InputPublisher::Finished> result = mPublisher->receiveFinishedSignal();
+
+ ASSERT_TRUE(result.ok()) << "receiveFinishedSignal should return OK";
+ ASSERT_EQ(seq, result->seq)
+ << "receiveFinishedSignal should have returned the original sequence number";
+ ASSERT_TRUE(result->handled)
+ << "receiveFinishedSignal should have set handled to consumer's reply";
+ ASSERT_GE(result->consumeTime, publishTime)
<< "finished signal's consume time should be greater than publish time";
}
@@ -369,22 +343,13 @@
status = mConsumer->sendFinishedSignal(seq, true);
ASSERT_EQ(OK, status) << "consumer sendFinishedSignal should return OK";
- uint32_t finishedSeq = 0;
- bool handled = false;
- nsecs_t consumeTime;
- status = mPublisher->receiveFinishedSignal(
- [&finishedSeq, &handled, &consumeTime](uint32_t inSeq, bool inHandled,
- nsecs_t inConsumeTime) -> void {
- finishedSeq = inSeq;
- handled = inHandled;
- consumeTime = inConsumeTime;
- });
- ASSERT_EQ(OK, status) << "publisher receiveFinishedSignal should return OK";
- ASSERT_EQ(seq, finishedSeq)
- << "publisher receiveFinishedSignal should have returned the original sequence number";
- ASSERT_TRUE(handled)
- << "publisher receiveFinishedSignal should have set handled to consumer's reply";
- ASSERT_GE(consumeTime, publishTime)
+ android::base::Result<InputPublisher::Finished> result = mPublisher->receiveFinishedSignal();
+ ASSERT_TRUE(result.ok()) << "publisher receiveFinishedSignal should return OK";
+ ASSERT_EQ(seq, result->seq)
+ << "receiveFinishedSignal should have returned the original sequence number";
+ ASSERT_TRUE(result->handled)
+ << "receiveFinishedSignal should have set handled to consumer's reply";
+ ASSERT_GE(result->consumeTime, publishTime)
<< "finished signal's consume time should be greater than publish time";
}
@@ -410,32 +375,23 @@
ASSERT_EQ(AINPUT_EVENT_TYPE_DRAG, event->getType())
<< "consumer should have returned a drag event";
- DragEvent* dragEvent = static_cast<DragEvent*>(event);
+ const DragEvent& dragEvent = static_cast<const DragEvent&>(*event);
EXPECT_EQ(seq, consumeSeq);
- EXPECT_EQ(eventId, dragEvent->getId());
- EXPECT_EQ(isExiting, dragEvent->isExiting());
- EXPECT_EQ(x, dragEvent->getX());
- EXPECT_EQ(y, dragEvent->getY());
+ EXPECT_EQ(eventId, dragEvent.getId());
+ EXPECT_EQ(isExiting, dragEvent.isExiting());
+ EXPECT_EQ(x, dragEvent.getX());
+ EXPECT_EQ(y, dragEvent.getY());
status = mConsumer->sendFinishedSignal(seq, true);
ASSERT_EQ(OK, status) << "consumer sendFinishedSignal should return OK";
- uint32_t finishedSeq = 0;
- bool handled = false;
- nsecs_t consumeTime;
- status = mPublisher->receiveFinishedSignal(
- [&finishedSeq, &handled, &consumeTime](uint32_t inSeq, bool inHandled,
- nsecs_t inConsumeTime) -> void {
- finishedSeq = inSeq;
- handled = inHandled;
- consumeTime = inConsumeTime;
- });
- ASSERT_EQ(OK, status) << "publisher receiveFinishedSignal should return OK";
- ASSERT_EQ(seq, finishedSeq)
+ android::base::Result<InputPublisher::Finished> result = mPublisher->receiveFinishedSignal();
+ ASSERT_TRUE(result.ok()) << "publisher receiveFinishedSignal should return OK";
+ ASSERT_EQ(seq, result->seq)
<< "publisher receiveFinishedSignal should have returned the original sequence number";
- ASSERT_TRUE(handled)
+ ASSERT_TRUE(result->handled)
<< "publisher receiveFinishedSignal should have set handled to consumer's reply";
- ASSERT_GE(consumeTime, publishTime)
+ ASSERT_GE(result->consumeTime, publishTime)
<< "finished signal's consume time should be greater than publish time";
}
diff --git a/services/inputflinger/dispatcher/InputDispatcher.cpp b/services/inputflinger/dispatcher/InputDispatcher.cpp
index 3e80bd7..fe46d17 100644
--- a/services/inputflinger/dispatcher/InputDispatcher.cpp
+++ b/services/inputflinger/dispatcher/InputDispatcher.cpp
@@ -80,6 +80,7 @@
#define INDENT4 " "
using android::base::HwTimeoutMultiplier;
+using android::base::Result;
using android::base::StringPrintf;
using android::os::BlockUntrustedTouchesMode;
using android::os::IInputConstants;
@@ -3283,17 +3284,17 @@
nsecs_t currentTime = now();
bool gotOne = false;
- status_t status;
+ status_t status = OK;
for (;;) {
- std::function<void(uint32_t seq, bool handled, nsecs_t consumeTime)> callback =
- std::bind(&InputDispatcher::finishDispatchCycleLocked, d, currentTime,
- connection, std::placeholders::_1, std::placeholders::_2,
- std::placeholders::_3);
-
- status = connection->inputPublisher.receiveFinishedSignal(callback);
- if (status) {
+ Result<InputPublisher::Finished> result =
+ connection->inputPublisher.receiveFinishedSignal();
+ if (!result.ok()) {
+ status = result.error().code();
break;
}
+ const InputPublisher::Finished& finished = *result;
+ d->finishDispatchCycleLocked(currentTime, connection, finished.seq,
+ finished.handled, finished.consumeTime);
gotOne = true;
}
if (gotOne) {
@@ -4998,8 +4999,7 @@
}
}
-base::Result<std::unique_ptr<InputChannel>> InputDispatcher::createInputChannel(
- const std::string& name) {
+Result<std::unique_ptr<InputChannel>> InputDispatcher::createInputChannel(const std::string& name) {
#if DEBUG_CHANNEL_CREATION
ALOGD("channel '%s' ~ createInputChannel", name.c_str());
#endif
@@ -5028,8 +5028,10 @@
return clientChannel;
}
-base::Result<std::unique_ptr<InputChannel>> InputDispatcher::createInputMonitor(
- int32_t displayId, bool isGestureMonitor, const std::string& name, int32_t pid) {
+Result<std::unique_ptr<InputChannel>> InputDispatcher::createInputMonitor(int32_t displayId,
+ bool isGestureMonitor,
+ const std::string& name,
+ int32_t pid) {
std::shared_ptr<InputChannel> serverChannel;
std::unique_ptr<InputChannel> clientChannel;
status_t result = openInputChannelPair(name, serverChannel, clientChannel);
diff --git a/services/inputflinger/host/Android.bp b/services/inputflinger/host/Android.bp
index 18d0226..743587c 100644
--- a/services/inputflinger/host/Android.bp
+++ b/services/inputflinger/host/Android.bp
@@ -67,6 +67,7 @@
cflags: ["-Wall", "-Werror"],
shared_libs: [
+ "libbase",
"libbinder",
"libinputflingerhost",
"libutils",