Skip to content

Commit

Permalink
[Nearby] Callback when ReceiveMessagesExpress completes.
Browse files Browse the repository at this point in the history 
This adds a callback to be invoked when the download stream ends in
ReceiveMessagesExpress::OnComplete(). This will ultimately notify the
WebRTC medium so that we can move on quickly rather than waiting on a
timeout to fire.

This change includes an uprev for Nearby Connections with the following
CLs...

cl/359161492: Add nearby-google3 comments to blocks which shouldn't
  appear in GitHub.
cl/359631402: Completion callback for WebRtcSignalingMessenger.
cl/359771717: Get data how many user dismiss half sheet actually pair
  the device.

Fixed: 1165387
Change-Id: I7d44ee2b015af210f365c185e77e829b164f3dec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2713883
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Reviewed-by: James Vecore <vecore@google.com>
Reviewed-by: Alex Gough <ajgo@chromium.org>
Commit-Queue: Michael Hansen <hansenmichael@google.com>
Cr-Commit-Position: refs/heads/master@{#859140}
  • Loading branch information
Michael Hansen authored and Chromium LUCI CQ committed Mar 2, 2021
1 parent 5bfbe31 commit 3241585
Show file tree
Hide file tree
Showing 7 changed files with 101 additions and 26 deletions.
2 changes: 1 addition & 1 deletion DEPS
View file
Expand Up @@ -351,7 +351,7 @@ vars = {
# Three lines of non-changing comments so that
# the commit queue can handle CLs rolling nearby
# and whatever else without interference from each other.
'nearby_revision': '7c0841b18cfe239453aa65cc9504ac0535d8f756',
'nearby_revision': '773ea15f5e3db5cd952e3916e828da55b098d9f3',
# Three lines of non-changing comments so that
# the commit queue can handle CLs rolling securemessage
# and whatever else without interference from each other.
Expand Down
10 changes: 8 additions & 2 deletions chrome/browser/nearby_sharing/instantmessaging/receive_messages_express.cc
View file
Expand Up @@ -212,7 +212,8 @@ void ReceiveMessagesExpress::StopReceivingMessages() {
// Cancel any pending calls into this object.
weak_ptr_factory_.InvalidateWeakPtrs();
// This implicitly cancels the download stream.
// This implicitly cancels the download stream. We intentionally don't call
// OnComplete() when the other side calls StopReceivingMessages().
url_loader_.reset();
NS_LOG(VERBOSE) << __func__ << ": callback already invoked? "
Expand Down Expand Up @@ -241,7 +242,8 @@ void ReceiveMessagesExpress::OnComplete(bool success) {
NS_LOG(VERBOSE) << __func__ << ": success? " << (success ? "yes" : "no")
<< ", start calback invoked? "
<< (start_receiving_messages_callback_ ? "no" : "yes");
<< (start_receiving_messages_callback_ ? "no" : "yes")
<< ", net::Error " << url_loader_->NetError();
if (start_receiving_messages_callback_) {
LogReceiveResult(success, http_status);
Expand All @@ -251,6 +253,10 @@ void ReceiveMessagesExpress::OnComplete(bool success) {
FailSessionAndDestruct("Download stream ended before fast path ready");
// |this| will be destroyed here.
return;
} else {
// Only call OnComplete() if the start callback has been invoked, meaning
// the stream has opened and we have received "fast path ready".
incoming_messages_listener_->OnComplete(success);
}
}
Expand Down
40 changes: 40 additions & 0 deletions chrome/browser/nearby_sharing/instantmessaging/receive_messages_express_unittest.cc
View file
Expand Up @@ -70,12 +70,17 @@ class FakeIncomingMessagesListener
messages_received_.push_back(message);
}

void OnComplete(bool success) override { on_complete_result_ = success; }

const std::vector<std::string>& messages_received() {
return messages_received_;
}

base::Optional<bool> on_complete_result() { return on_complete_result_; }

private:
std::vector<std::string> messages_received_;
base::Optional<bool> on_complete_result_;
};

class ReceiveMessagesExpressTest : public testing::Test {
Expand Down Expand Up @@ -111,6 +116,10 @@ class ReceiveMessagesExpressTest : public testing::Test {
return message_listener_.messages_received();
}

base::Optional<bool> OnCompleteResult() {
return message_listener_.on_complete_result();
}

std::string GetFastPathOnlyResponse() {
return BuildResponseProto({}, /*include_fast_path_ready=*/true)
.SerializeAsString();
Expand Down Expand Up @@ -284,3 +293,34 @@ TEST_F(ReceiveMessagesExpressTest, PendingRemoteCleanupDisconnects) {
session_pending_remote_.reset();
run_loop_2.Run();
}

TEST_F(ReceiveMessagesExpressTest, OnCompleteAfterSuccess) {
base::RunLoop run_loop;
StartReceivingMessages(&run_loop, /*token_success=*/true);

std::vector<std::string> messages = {"quick brown", "fox"};
std::string response = BuildResponseProto(messages).SerializeAsString();
GetTestUrlLoaderFactory().AddResponse(kInstantMessagingReceiveMessageAPI,
response, net::HTTP_OK);
run_loop.Run();

ASSERT_EQ(0, GetTestUrlLoaderFactory().NumPending());
ASSERT_TRUE(OnCompleteResult().has_value());
EXPECT_TRUE(OnCompleteResult().value());
}

TEST_F(ReceiveMessagesExpressTest, NoOnCompleteWithoutFastPathReady) {
base::RunLoop run_loop;
StartReceivingMessages(&run_loop, /*token_success=*/true);

std::vector<std::string> messages = {"quick brown", "fox"};
std::string response =
BuildResponseProto(messages, /*include_fast_path_ready=*/false)
.SerializeAsString();
GetTestUrlLoaderFactory().AddResponse(kInstantMessagingReceiveMessageAPI,
response, net::HTTP_FORBIDDEN);
run_loop.Run();

ASSERT_EQ(0, GetTestUrlLoaderFactory().NumPending());
ASSERT_FALSE(OnCompleteResult().has_value());
}
30 changes: 24 additions & 6 deletions chrome/services/sharing/nearby/platform/webrtc.cc
View file
Expand Up @@ -87,9 +87,13 @@ class IncomingMessageListener
public:
explicit IncomingMessageListener(
api::WebRtcSignalingMessenger::OnSignalingMessageCallback
signaling_message_callback)
: signaling_message_callback_(std::move(signaling_message_callback)) {
signaling_message_callback,
api::WebRtcSignalingMessenger::OnSignalingCompleteCallback
signaling_complete_callback)
: signaling_message_callback_(std::move(signaling_message_callback)),
signaling_complete_callback_(std::move(signaling_complete_callback)) {
DCHECK(signaling_message_callback_);
DCHECK(signaling_complete_callback_);
}

~IncomingMessageListener() override = default;
Expand All @@ -99,9 +103,16 @@ class IncomingMessageListener
signaling_message_callback_(ByteArray(message));
}

// mojom::IncomingMessagesListener:
void OnComplete(bool success) override {
signaling_complete_callback_(success);
}

private:
api::WebRtcSignalingMessenger::OnSignalingMessageCallback
signaling_message_callback_;
api::WebRtcSignalingMessenger::OnSignalingCompleteCallback
signaling_complete_callback_;
};

// Used as a messenger in sending and receiving WebRTC messages between devices.
Expand Down Expand Up @@ -169,14 +180,20 @@ class WebRtcSignalingMessengerImpl : public api::WebRtcSignalingMessenger {
void BindIncomingReceiver(
mojo::PendingReceiver<sharing::mojom::IncomingMessagesListener>
pending_receiver,
api::WebRtcSignalingMessenger::OnSignalingMessageCallback callback) {
api::WebRtcSignalingMessenger::OnSignalingMessageCallback
message_callback,
api::WebRtcSignalingMessenger::OnSignalingCompleteCallback
complete_callback) {
mojo::MakeSelfOwnedReceiver(
std::make_unique<IncomingMessageListener>(std::move(callback)),
std::make_unique<IncomingMessageListener>(std::move(message_callback),
std::move(complete_callback)),
std::move(pending_receiver), task_runner_);
}

// api::WebRtcSignalingMessenger:
bool StartReceivingMessages(OnSignalingMessageCallback callback) override {
bool StartReceivingMessages(
OnSignalingMessageCallback message_callback,
OnSignalingCompleteCallback complete_callback) override {
bool success = false;
mojo::PendingRemote<sharing::mojom::IncomingMessagesListener>
pending_remote;
Expand All @@ -199,7 +216,8 @@ class WebRtcSignalingMessengerImpl : public api::WebRtcSignalingMessenger {
FROM_HERE,
base::BindOnce(&WebRtcSignalingMessengerImpl::BindIncomingReceiver,
weak_ptr_factory_.GetWeakPtr(),
std::move(pending_receiver), std::move(callback)));
std::move(pending_receiver), std::move(message_callback),
std::move(complete_callback)));

receiving_messages_ = true;
return true;
Expand Down
41 changes: 25 additions & 16 deletions chrome/services/sharing/nearby/platform/webrtc_test.cc
View file
Expand Up @@ -224,17 +224,20 @@ TEST_F(WebRtcMediumTest, GetMessengerAndStartReceivingMessages) {
mojo::Remote<sharing::mojom::IncomingMessagesListener> remote(
std::move(listener));
remote->OnMessage(std::string(message));
remote->OnComplete(true);
}));

std::unique_ptr<api::WebRtcSignalingMessenger> messenger =
GetMedium().GetSignalingMessenger(from, GetCountryCodeLocationHint("ZZ"));
EXPECT_TRUE(messenger);

base::RunLoop loop;
EXPECT_TRUE(messenger->StartReceivingMessages([&](const ByteArray& msg) {
EXPECT_EQ(message, msg);
loop.Quit();
}));
EXPECT_TRUE(messenger->StartReceivingMessages(
[&](const ByteArray& msg) { EXPECT_EQ(message, msg); },
[&](bool success) {
EXPECT_TRUE(success);
loop.Quit();
}));
loop.Run();
}

Expand Down Expand Up @@ -274,10 +277,12 @@ TEST_F(WebRtcMediumTest, DISABLED_GetMessenger_StartAndStopReceivingMessages) {
EXPECT_TRUE(messenger);

base::RunLoop loop;
EXPECT_TRUE(messenger->StartReceivingMessages([&](const ByteArray& msg) {
EXPECT_EQ(message, msg);
loop.Quit();
}));
EXPECT_TRUE(messenger->StartReceivingMessages(
[&](const ByteArray& msg) {
EXPECT_EQ(message, msg);
loop.Quit();
},
[](bool success) {}));
loop.Run();

EXPECT_TRUE(remote.is_connected());
Expand Down Expand Up @@ -320,10 +325,12 @@ TEST_F(WebRtcMediumTest, GetMessengerAndStartReceivingMessagesTwice) {
EXPECT_TRUE(messenger);

base::RunLoop loop;
EXPECT_TRUE(messenger->StartReceivingMessages([&](const ByteArray& msg) {
EXPECT_EQ(message, msg);
loop.Quit();
}));
EXPECT_TRUE(messenger->StartReceivingMessages(
[&](const ByteArray& msg) {
EXPECT_EQ(message, msg);
loop.Quit();
},
[](bool success) {}));
loop.Run();

// Create a second receiver sessions to return
Expand Down Expand Up @@ -358,10 +365,12 @@ TEST_F(WebRtcMediumTest, GetMessengerAndStartReceivingMessagesTwice) {
}));

base::RunLoop loop_2;
EXPECT_TRUE(messenger->StartReceivingMessages([&](const ByteArray& msg) {
EXPECT_EQ(message, msg);
loop_2.Quit();
}));
EXPECT_TRUE(messenger->StartReceivingMessages(
[&](const ByteArray& msg) {
EXPECT_EQ(message, msg);
loop_2.Quit();
},
[](bool success) {}));
loop_2.Run();
}

Expand Down
2 changes: 2 additions & 0 deletions chromeos/services/nearby/public/mojom/webrtc_signaling_messenger.mojom
View file
Expand Up @@ -8,6 +8,8 @@ module sharing.mojom;
interface IncomingMessagesListener {
// Called by the browser process when a new message is received.
OnMessage(string message);
// Called by the browser process when message receiving is complete.
OnComplete(bool success);
};

// Runs in the browser process and called by the Nearby utility process.
Expand Down
2 changes: 1 addition & 1 deletion third_party/nearby/README.chromium
View file
@@ -1,7 +1,7 @@
Name: Nearby Connections Library
Short Name: Nearby
URL: https://github.com/google/nearby-connections
Version: 7c0841b18cfe239453aa65cc9504ac0535d8f756
Version: 773ea15f5e3db5cd952e3916e828da55b098d9f3
License: Apache 2.0
License File: LICENSE
Security Critical: yes
Expand Down

0 comments on commit 3241585

Please sign in to comment.