Remove original_message_type from Sharing AckMessage
This field was only used to collect metrics, however, we can actually
just store that information on the sender side and avoid sending it back
via FCM.
We never used the suffixed version of Sharing.MessageReceivedType so
this also removes that while keeping the aggregate one.
Bug: None
Change-Id: Ie598d33e60381341170bbca2716cb70be09be0e0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2000764
Reviewed-by: Alexei Svitkine <[email protected]>
Reviewed-by: Michael van Ouwerkerk <[email protected]>
Commit-Queue: Richard Knoll <[email protected]>
Cr-Commit-Position: refs/heads/master@{#732025}
diff --git a/chrome/browser/sharing/ack_message_handler.cc b/chrome/browser/sharing/ack_message_handler.cc
index 5224123..1f205426 100644
--- a/chrome/browser/sharing/ack_message_handler.cc
+++ b/chrome/browser/sharing/ack_message_handler.cc
@@ -24,8 +24,7 @@
if (ack_message->has_response_message())
response = base::WrapUnique(ack_message->release_response_message());
- sharing_message_sender_->OnAckReceived(ack_message->original_message_type(),
- ack_message->original_message_id(),
+ sharing_message_sender_->OnAckReceived(ack_message->original_message_id(),
std::move(response));
std::move(done_callback).Run(/*response=*/nullptr);
diff --git a/chrome/browser/sharing/ack_message_handler_unittest.cc b/chrome/browser/sharing/ack_message_handler_unittest.cc
index 9d937da..2e8b832 100644
--- a/chrome/browser/sharing/ack_message_handler_unittest.cc
+++ b/chrome/browser/sharing/ack_message_handler_unittest.cc
@@ -24,10 +24,9 @@
/*local_device_info_provider=*/nullptr) {}
~MockSharingMessageSender() override = default;
- MOCK_METHOD3(
+ MOCK_METHOD2(
OnAckReceived,
- void(chrome_browser_sharing::MessageType message_type,
- const std::string& fcm_message_id,
+ void(const std::string& fcm_message_id,
std::unique_ptr<chrome_browser_sharing::ResponseMessage> response));
private:
@@ -59,16 +58,12 @@
chrome_browser_sharing::SharingMessage sharing_message;
sharing_message.mutable_ack_message()->set_original_message_id(
kTestMessageId);
- sharing_message.mutable_ack_message()->set_original_message_type(
- chrome_browser_sharing::CLICK_TO_CALL_MESSAGE);
base::MockCallback<SharingMessageHandler::DoneCallback> done_callback;
EXPECT_CALL(done_callback, Run(testing::Eq(nullptr)));
- EXPECT_CALL(
- mock_response_callback_helper_,
- OnAckReceived(testing::Eq(chrome_browser_sharing::CLICK_TO_CALL_MESSAGE),
- testing::Eq(kTestMessageId), testing::Eq(nullptr)));
+ EXPECT_CALL(mock_response_callback_helper_,
+ OnAckReceived(testing::Eq(kTestMessageId), testing::Eq(nullptr)));
ack_message_handler_.OnMessage(std::move(sharing_message),
done_callback.Get());
@@ -78,8 +73,6 @@
chrome_browser_sharing::SharingMessage sharing_message;
sharing_message.mutable_ack_message()->set_original_message_id(
kTestMessageId);
- sharing_message.mutable_ack_message()->set_original_message_type(
- chrome_browser_sharing::CLICK_TO_CALL_MESSAGE);
sharing_message.mutable_ack_message()->mutable_response_message();
chrome_browser_sharing::ResponseMessage response_message_copy =
@@ -88,11 +81,9 @@
base::MockCallback<SharingMessageHandler::DoneCallback> done_callback;
EXPECT_CALL(done_callback, Run(testing::Eq(nullptr)));
- EXPECT_CALL(
- mock_response_callback_helper_,
- OnAckReceived(testing::Eq(chrome_browser_sharing::CLICK_TO_CALL_MESSAGE),
- testing::Eq(kTestMessageId),
- ProtoEquals(response_message_copy)));
+ EXPECT_CALL(mock_response_callback_helper_,
+ OnAckReceived(testing::Eq(kTestMessageId),
+ ProtoEquals(response_message_copy)));
ack_message_handler_.OnMessage(std::move(sharing_message),
done_callback.Get());
diff --git a/chrome/browser/sharing/proto/sharing_message.proto b/chrome/browser/sharing/proto/sharing_message.proto
index d1c57271..70a4de2 100644
--- a/chrome/browser/sharing/proto/sharing_message.proto
+++ b/chrome/browser/sharing/proto/sharing_message.proto
@@ -65,13 +65,13 @@
}
// Message for acknowledging the sender after a non-AckMessage is received.
+// Next tag: 4
message AckMessage {
+ reserved 2;
+
// required.
string original_message_id = 1;
- // The type of message that this is acknowledging. optional.
- MessageType original_message_type = 2;
-
// Response of the message, optional.
ResponseMessage response_message = 3;
}
diff --git a/chrome/browser/sharing/sharing_fcm_handler.cc b/chrome/browser/sharing/sharing_fcm_handler.cc
index 8ed590ed..9f78d706 100644
--- a/chrome/browser/sharing/sharing_fcm_handler.cc
+++ b/chrome/browser/sharing/sharing_fcm_handler.cc
@@ -95,16 +95,8 @@
<< "No payload set in SharingMessage received";
chrome_browser_sharing::MessageType message_type =
- chrome_browser_sharing::UNKNOWN_MESSAGE;
- if (sharing_message.payload_case() ==
- chrome_browser_sharing::SharingMessage::kAckMessage) {
- DCHECK(sharing_message.has_ack_message());
- message_type = sharing_message.ack_message().original_message_type();
- } else {
- message_type =
- SharingPayloadCaseToMessageType(sharing_message.payload_case());
- }
- LogSharingMessageReceived(message_type, sharing_message.payload_case());
+ SharingPayloadCaseToMessageType(sharing_message.payload_case());
+ LogSharingMessageReceived(sharing_message.payload_case());
SharingMessageHandler* handler =
handler_registry_->GetSharingHandler(sharing_message.payload_case());
@@ -170,7 +162,6 @@
chrome_browser_sharing::AckMessage* ack_message =
sharing_message.mutable_ack_message();
ack_message->set_original_message_id(original_message_id);
- ack_message->set_original_message_type(original_message_type);
if (response)
ack_message->set_allocated_response_message(response.release());
diff --git a/chrome/browser/sharing/sharing_fcm_handler_unittest.cc b/chrome/browser/sharing/sharing_fcm_handler_unittest.cc
index 3f945d7..1e4d225 100644
--- a/chrome/browser/sharing/sharing_fcm_handler_unittest.cc
+++ b/chrome/browser/sharing/sharing_fcm_handler_unittest.cc
@@ -177,8 +177,6 @@
SharingMessage sharing_ack_message;
sharing_ack_message.mutable_ack_message()->set_original_message_id(
kTestMessageId);
- sharing_ack_message.mutable_ack_message()->set_original_message_type(
- chrome_browser_sharing::PING_MESSAGE);
// Tests OnMessage flow in SharingFCMHandler when no handler is registered.
EXPECT_CALL(mock_sharing_message_handler_, OnMessage(_, _)).Times(0);
@@ -225,8 +223,6 @@
SharingMessage sharing_ack_message;
sharing_ack_message.mutable_ack_message()->set_original_message_id(
kTestMessageId);
- sharing_ack_message.mutable_ack_message()->set_original_message_type(
- chrome_browser_sharing::PING_MESSAGE);
sharing_ack_message.mutable_ack_message()->mutable_response_message();
// Tests OnMessage flow in SharingFCMHandler after handler is added.
@@ -263,8 +259,6 @@
SharingMessage sharing_ack_message;
sharing_ack_message.mutable_ack_message()->set_original_message_id(
kTestMessageId);
- sharing_ack_message.mutable_ack_message()->set_original_message_type(
- chrome_browser_sharing::PING_MESSAGE);
// Tests OnMessage flow in SharingFCMHandler after handler is added.
ON_CALL(mock_sharing_message_handler_,
@@ -300,8 +294,6 @@
SharingMessage sharing_ack_message;
sharing_ack_message.mutable_ack_message()->set_original_message_id(
kTestMessageId);
- sharing_ack_message.mutable_ack_message()->set_original_message_type(
- chrome_browser_sharing::PING_MESSAGE);
ON_CALL(mock_sharing_message_handler_,
OnMessage(ProtoEquals(sharing_message), _))
diff --git a/chrome/browser/sharing/sharing_message_sender.cc b/chrome/browser/sharing/sharing_message_sender.cc
index d8acb873..eaf0dce 100644
--- a/chrome/browser/sharing/sharing_message_sender.cc
+++ b/chrome/browser/sharing/sharing_message_sender.cc
@@ -109,10 +109,10 @@
message_guids_.emplace(*message_id, message_guid);
receiver_device_platform_.emplace(*message_id, receiver_device_platform);
receiver_last_updated_age_.emplace(*message_id, last_updated_age);
+ message_types_.emplace(*message_id, message_type);
}
void SharingMessageSender::OnAckReceived(
- chrome_browser_sharing::MessageType message_type,
const std::string& message_id,
std::unique_ptr<chrome_browser_sharing::ResponseMessage> response) {
auto guid_iter = message_guids_.find(message_id);
@@ -122,6 +122,12 @@
std::string message_guid = std::move(guid_iter->second);
message_guids_.erase(guid_iter);
+ auto message_types_iter = message_types_.find(message_id);
+ DCHECK(message_types_iter != message_types_.end());
+
+ chrome_browser_sharing::MessageType message_type = message_types_iter->second;
+ message_types_.erase(message_types_iter);
+
auto times_iter = send_message_times_.find(message_id);
DCHECK(times_iter != send_message_times_.end());
diff --git a/chrome/browser/sharing/sharing_message_sender.h b/chrome/browser/sharing/sharing_message_sender.h
index b1261a5..a70719a4 100644
--- a/chrome/browser/sharing/sharing_message_sender.h
+++ b/chrome/browser/sharing/sharing_message_sender.h
@@ -70,7 +70,6 @@
ResponseCallback callback);
virtual void OnAckReceived(
- chrome_browser_sharing::MessageType message_type,
const std::string& message_id,
std::unique_ptr<chrome_browser_sharing::ResponseMessage> response);
@@ -109,6 +108,8 @@
std::map<std::string, SharingDevicePlatform> receiver_device_platform_;
// Map of FCM message_id to age of last updated timestamp of receiver device.
std::map<std::string, base::TimeDelta> receiver_last_updated_age_;
+ // Map of FCM message_id to message type for metrics.
+ std::map<std::string, chrome_browser_sharing::MessageType> message_types_;
// Registered delegates to send messages.
std::map<DelegateType, std::unique_ptr<SendMessageDelegate>> send_delegates_;
