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_;
diff --git a/chrome/browser/sharing/sharing_message_sender_unittest.cc b/chrome/browser/sharing/sharing_message_sender_unittest.cc
index 6114b9d..e10e48b 100644
--- a/chrome/browser/sharing/sharing_message_sender_unittest.cc
+++ b/chrome/browser/sharing/sharing_message_sender_unittest.cc
@@ -158,9 +158,8 @@
// Callback already run with result timeout, ack received for same message
// id is ignored.
- sharing_message_sender_.OnAckReceived(
- SharingPayloadCaseToMessageType(message.payload_case()),
- kSenderMessageID, /*response=*/nullptr);
+ sharing_message_sender_.OnAckReceived(kSenderMessageID,
+ /*response=*/nullptr);
};
EXPECT_CALL(
@@ -200,9 +199,8 @@
// Callback already run with result timeout, ack received for same
// message id is ignored.
- sharing_message_sender_.OnAckReceived(
- SharingPayloadCaseToMessageType(message.payload_case()),
- kSenderMessageID, /*response=*/nullptr);
+ sharing_message_sender_.OnAckReceived(kSenderMessageID,
+ /*response=*/nullptr);
};
EXPECT_CALL(
@@ -263,9 +261,8 @@
std::make_unique<chrome_browser_sharing::ResponseMessage>();
response_message->CopyFrom(expected_response_message);
- sharing_message_sender_.OnAckReceived(
- SharingPayloadCaseToMessageType(message.payload_case()),
- kSenderMessageID, std::move(response_message));
+ sharing_message_sender_.OnAckReceived(kSenderMessageID,
+ std::move(response_message));
};
EXPECT_CALL(
diff --git a/chrome/browser/sharing/sharing_metrics.cc b/chrome/browser/sharing/sharing_metrics.cc
index bd4a1bb..cce5101 100644
--- a/chrome/browser/sharing/sharing_metrics.cc
+++ b/chrome/browser/sharing/sharing_metrics.cc
@@ -121,17 +121,10 @@
}
void LogSharingMessageReceived(
- chrome_browser_sharing::MessageType original_message_type,
chrome_browser_sharing::SharingMessage::PayloadCase payload_case) {
- chrome_browser_sharing::MessageType actual_message_type =
- SharingPayloadCaseToMessageType(payload_case);
base::UmaHistogramExactLinear("Sharing.MessageReceivedType",
- actual_message_type,
+ SharingPayloadCaseToMessageType(payload_case),
chrome_browser_sharing::MessageType_ARRAYSIZE);
- base::UmaHistogramExactLinear(
- base::StrCat({"Sharing.MessageReceivedType.",
- MessageTypeToMessageSuffix(original_message_type)}),
- actual_message_type, chrome_browser_sharing::MessageType_ARRAYSIZE);
}
void LogSharingRegistrationResult(SharingDeviceRegistrationResult result) {
diff --git a/chrome/browser/sharing/sharing_metrics.h b/chrome/browser/sharing/sharing_metrics.h
index 136e4260..36df70c 100644
--- a/chrome/browser/sharing/sharing_metrics.h
+++ b/chrome/browser/sharing/sharing_metrics.h
@@ -46,11 +46,8 @@
chrome_browser_sharing::SharingMessage::PayloadCase payload_case);
// Logs the |payload_case| to UMA. This should be called when a SharingMessage
-// is received. Additionally, a suffixed version of the histogram is logged
-// using |original_message_type| which is different from the actual message type
-// for ack messages.
+// is received.
void LogSharingMessageReceived(
- chrome_browser_sharing::MessageType original_message_type,
chrome_browser_sharing::SharingMessage::PayloadCase payload_case);
// Logs the |result| to UMA. This should be called after attempting register
@@ -136,7 +133,7 @@
SharingDevicePlatform receiver_device_platform,
SharingSendMessageResult result);
-// Logs to UMA result of sendin an ack of a SharingMessage.
+// Logs to UMA result of sending an ack of a SharingMessage.
void LogSendSharingAckMessageResult(
chrome_browser_sharing::MessageType message_type,
SharingDevicePlatform ack_receiver_device_type,
diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml
index 52988f2..cd47a51 100644
--- a/tools/metrics/histograms/histograms.xml
+++ b/tools/metrics/histograms/histograms.xml
@@ -142074,8 +142074,6 @@
<histogram name="Sharing.MessageReceivedType" enum="SharingMessageType"
expires_after="M84">
-<!-- Name completed by histogram_suffixes name="SharingMessage" -->
-
<owner>[email protected]</owner>
<owner>[email protected]</owner>
<summary>
@@ -188783,7 +188781,11 @@
<affected-histogram name="Sharing.MessageAckTime.Mac"/>
<affected-histogram name="Sharing.MessageAckTime.Unknown"/>
<affected-histogram name="Sharing.MessageAckTime.Windows"/>
- <affected-histogram name="Sharing.MessageReceivedType"/>
+ <affected-histogram name="Sharing.MessageReceivedType">
+ <obsolete>
+ Removed 2020-01.
+ </obsolete>
+ </affected-histogram>
<affected-histogram name="Sharing.SendAckMessageResult"/>
<affected-histogram name="Sharing.SendAckMessageResult.Android"/>
<affected-histogram name="Sharing.SendAckMessageResult.ChromeOS"/>
