Commit 3385c591 authored by Curt Clemens's avatar Curt Clemens Committed by Commit Bot

[NearbyShare] Add ClearForegroundReceiveSurfaces to NearbySharingService

Before, the NearbyShareDelegate was turning off high visibility mode by
invoking the settings page with ?stop_receiving. That wasn't working
because it was causing the settings page to reload and the state was
lost. Instead, we go directly to the NearbySharingService and remove the
foreground receivers.

Bug: 1133799
Change-Id: I6cb35799dc8ee39db3d20cdd8865b45fccccf4e9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2441114Reviewed-by: default avatarAlex Gough <ajgo@chromium.org>
Reviewed-by: default avatarKyle Horimoto <khorimoto@chromium.org>
Reviewed-by: default avatarJames Vecore <vecore@google.com>
Commit-Queue: Curt Clemens <cclem@google.com>
Cr-Commit-Position: refs/heads/master@{#815384}
parent 1ab80e53
......@@ -38,6 +38,7 @@ class MockNearbySharingService : public NearbySharingService {
UnregisterReceiveSurface,
(TransferUpdateCallback*),
(override));
MOCK_METHOD(StatusCodes, ClearForegroundReceiveSurfaces, (), (override));
MOCK_METHOD(bool, IsInHighVisibility, (), (override));
MOCK_METHOD(StatusCodes,
SendAttachments,
......
......@@ -10,11 +10,13 @@ NearbyReceiveManager::NearbyReceiveManager(
NearbySharingService* nearby_sharing_service)
: nearby_sharing_service_(nearby_sharing_service) {
DCHECK(nearby_sharing_service_);
nearby_sharing_service_->AddObserver(this);
}
NearbyReceiveManager::~NearbyReceiveManager() {
ExitHighVisibility(base::DoNothing());
UnregisterForegroundReceiveSurface(base::DoNothing());
observers_set_.Clear();
nearby_sharing_service_->RemoveObserver(this);
}
void NearbyReceiveManager::OnTransferUpdate(
......@@ -42,26 +44,22 @@ void NearbyReceiveManager::AddReceiveObserver(
void NearbyReceiveManager::IsInHighVisibility(
IsInHighVisibilityCallback callback) {
std::move(callback).Run(in_high_visibility_);
std::move(callback).Run(nearby_sharing_service_->IsInHighVisibility());
}
void NearbyReceiveManager::EnterHighVisibility(
EnterHighVisibilityCallback callback) {
void NearbyReceiveManager::RegisterForegroundReceiveSurface(
RegisterForegroundReceiveSurfaceCallback callback) {
bool success =
NearbySharingService::StatusCodes::kOk ==
nearby_sharing_service_->RegisterReceiveSurface(
this, NearbySharingService::ReceiveSurfaceState::kForeground);
// We are in high-visibility only if the call was successful.
SetInHighVisibility(success);
std::move(callback).Run(success);
}
void NearbyReceiveManager::ExitHighVisibility(
ExitHighVisibilityCallback callback) {
void NearbyReceiveManager::UnregisterForegroundReceiveSurface(
UnregisterForegroundReceiveSurfaceCallback callback) {
bool success = NearbySharingService::StatusCodes::kOk ==
nearby_sharing_service_->UnregisterReceiveSurface(this);
// We have only exited high visibility if the call was successful.
SetInHighVisibility(success ? false : this->in_high_visibility_);
std::move(callback).Run(success);
}
......@@ -105,15 +103,7 @@ void NearbyReceiveManager::Reject(const base::UnguessableToken& share_target_id,
std::move(callback)));
}
void NearbyReceiveManager::SetInHighVisibility(bool in_high_visibility) {
if (in_high_visibility_ != in_high_visibility) {
in_high_visibility_ = in_high_visibility;
NotifyOnHighVisibilityChanged(in_high_visibility_);
}
}
void NearbyReceiveManager::NotifyOnHighVisibilityChanged(
bool in_high_visibility) {
void NearbyReceiveManager::OnHighVisibilityChanged(bool in_high_visibility) {
for (auto& remote : observers_set_) {
remote->OnHighVisibilityChanged(in_high_visibility);
}
......
......@@ -21,7 +21,8 @@
// share request has come in. The client can then accept or reject the share.
// This is a transient object and only lives while os-settings has it bound.
class NearbyReceiveManager : public nearby_share::mojom::ReceiveManager,
public TransferUpdateCallback {
public TransferUpdateCallback,
public NearbySharingService::Observer {
public:
explicit NearbyReceiveManager(NearbySharingService* nearby_sharing_service);
~NearbyReceiveManager() override;
......@@ -35,23 +36,26 @@ class NearbyReceiveManager : public nearby_share::mojom::ReceiveManager,
::mojo::PendingRemote<nearby_share::mojom::ReceiveObserver> observer)
override;
void IsInHighVisibility(IsInHighVisibilityCallback callback) override;
void EnterHighVisibility(EnterHighVisibilityCallback callback) override;
void ExitHighVisibility(ExitHighVisibilityCallback callback) override;
void RegisterForegroundReceiveSurface(
RegisterForegroundReceiveSurfaceCallback callback) override;
void UnregisterForegroundReceiveSurface(
UnregisterForegroundReceiveSurfaceCallback callback) override;
void Accept(const base::UnguessableToken& share_target_id,
AcceptCallback callback) override;
void Reject(const base::UnguessableToken& share_target_id,
RejectCallback callback) override;
// NearbySharingService::Observer
void OnHighVisibilityChanged(bool in_high_visibility) override;
void OnShutdown() override {}
private:
void SetInHighVisibility(bool in_high_visibility);
void NotifyOnHighVisibilityChanged(bool in_high_visibility);
void NotifyOnIncomingShare(
const ShareTarget& share_target,
const base::Optional<std::string>& connection_token);
NearbySharingService* nearby_sharing_service_;
bool in_high_visibility_ = false;
base::flat_map<base::UnguessableToken, ShareTarget> share_targets_map_;
mojo::RemoteSet<nearby_share::mojom::ReceiveObserver> observers_set_;
};
......
......@@ -73,6 +73,11 @@ class NearbyReceiveManagerTest : public testing::Test {
[=](TransferUpdateCallback* transfer_callback) { return code; });
}
void ExpectIsInHighVisibility(bool in_high_visibility) {
EXPECT_CALL(sharing_service_, IsInHighVisibility())
.WillOnce(testing::Return(in_high_visibility));
}
void ExpectAccept(StatusCodes code = StatusCodes::kOk) {
EXPECT_CALL(sharing_service_, Accept(testing::_, testing::_))
.WillOnce([=](const ShareTarget& share_target,
......@@ -103,76 +108,40 @@ class NearbyReceiveManagerTest : public testing::Test {
} // namespace
TEST_F(NearbyReceiveManagerTest, Default_State) {
bool on = false;
receive_manager_waiter_.IsInHighVisibility(&on);
EXPECT_FALSE(on);
ExpectUnregister();
}
TEST_F(NearbyReceiveManagerTest, Enter_Exit_Success) {
ExpectRegister();
bool success = false;
receive_manager_waiter_.EnterHighVisibility(&success);
receive_manager_waiter_.RegisterForegroundReceiveSurface(&success);
EXPECT_TRUE(success);
FlushMojoMessages();
ASSERT_TRUE(observer_.in_high_visibility_.has_value());
EXPECT_TRUE(*observer_.in_high_visibility_);
bool on = false;
receive_manager_waiter_.IsInHighVisibility(&on);
EXPECT_TRUE(on);
ExpectUnregister();
bool exited = false;
receive_manager_waiter_.ExitHighVisibility(&exited);
receive_manager_waiter_.UnregisterForegroundReceiveSurface(&exited);
EXPECT_TRUE(exited);
FlushMojoMessages();
ASSERT_TRUE(observer_.in_high_visibility_.has_value());
EXPECT_FALSE(*observer_.in_high_visibility_);
ExpectUnregister();
}
TEST_F(NearbyReceiveManagerTest, Enter_Failed) {
bool success = true;
ExpectRegister(StatusCodes::kError);
receive_manager_waiter_.EnterHighVisibility(&success);
receive_manager_waiter_.RegisterForegroundReceiveSurface(&success);
EXPECT_FALSE(success);
bool on = true;
receive_manager_waiter_.IsInHighVisibility(&on);
EXPECT_FALSE(on);
ExpectUnregister();
}
TEST_F(NearbyReceiveManagerTest, Multiple_Enter_Successful) {
bool success = false;
ExpectRegister(StatusCodes::kOk);
receive_manager_waiter_.EnterHighVisibility(&success);
receive_manager_waiter_.RegisterForegroundReceiveSurface(&success);
FlushMojoMessages();
EXPECT_TRUE(success);
bool on = false;
receive_manager_waiter_.IsInHighVisibility(&on);
EXPECT_TRUE(on);
ASSERT_TRUE(observer_.in_high_visibility_.has_value());
EXPECT_TRUE(*observer_.in_high_visibility_);
// Reset this so we validate the we don't get duplicate events
observer_.in_high_visibility_ = base::nullopt;
success = false;
ExpectRegister(StatusCodes::kOk);
receive_manager_waiter_.EnterHighVisibility(&success);
receive_manager_waiter_.RegisterForegroundReceiveSurface(&success);
FlushMojoMessages();
EXPECT_TRUE(success);
// Verify we did not get notified twice.
EXPECT_FALSE(observer_.in_high_visibility_.has_value());
on = false;
receive_manager_waiter_.IsInHighVisibility(&on);
EXPECT_TRUE(on);
ExpectUnregister();
}
......@@ -180,23 +149,14 @@ TEST_F(NearbyReceiveManagerTest, Multiple_Enter_Successful) {
TEST_F(NearbyReceiveManagerTest, Exit_Failed) {
bool success = false;
ExpectRegister();
receive_manager_waiter_.EnterHighVisibility(&success);
receive_manager_waiter_.RegisterForegroundReceiveSurface(&success);
EXPECT_TRUE(success);
ExpectUnregister(StatusCodes::kError);
success = false;
receive_manager_waiter_.ExitHighVisibility(&success);
receive_manager_waiter_.UnregisterForegroundReceiveSurface(&success);
EXPECT_FALSE(success);
// If exit failed, we are still in high visibility.
bool on = false;
receive_manager_waiter_.IsInHighVisibility(&on);
EXPECT_TRUE(on);
FlushMojoMessages();
ASSERT_TRUE(observer_.in_high_visibility_.has_value());
EXPECT_TRUE(*observer_.in_high_visibility_);
ExpectUnregister();
}
......@@ -273,3 +233,35 @@ TEST_F(NearbyReceiveManagerTest,
ExpectUnregister();
}
TEST_F(NearbyReceiveManagerTest, IsInHighVisibility) {
ExpectIsInHighVisibility(true);
bool on = false;
receive_manager_waiter_.IsInHighVisibility(&on);
FlushMojoMessages();
EXPECT_TRUE(on);
ExpectIsInHighVisibility(false);
on = true;
receive_manager_waiter_.IsInHighVisibility(&on);
FlushMojoMessages();
EXPECT_FALSE(on);
ExpectUnregister();
}
TEST_F(NearbyReceiveManagerTest, OnHighVisibilityChangedObserver) {
receive_manager_.OnHighVisibilityChanged(false);
FlushMojoMessages();
ASSERT_TRUE(observer_.in_high_visibility_.has_value());
EXPECT_FALSE(*observer_.in_high_visibility_);
observer_.in_high_visibility_ = base::nullopt;
receive_manager_.OnHighVisibilityChanged(true);
FlushMojoMessages();
ASSERT_TRUE(observer_.in_high_visibility_.has_value());
EXPECT_TRUE(*observer_.in_high_visibility_);
ExpectUnregister();
}
......@@ -18,7 +18,6 @@
namespace {
const char kStartReceivingQueryParam[] = "receive";
const char kStopReceivingQueryParam[] = "stop_receiving";
constexpr base::TimeDelta kShutoffTimeout = base::TimeDelta::FromMinutes(5);
constexpr base::TimeDelta kOnboardingWaitTimeout =
......@@ -81,7 +80,7 @@ void NearbyShareDelegateImpl::DisableHighVisibility() {
shutoff_timer_.Stop();
settings_opener_->ShowSettingsPage(kStopReceivingQueryParam);
nearby_share_service_->ClearForegroundReceiveSurfaces();
}
void NearbyShareDelegateImpl::OnLockStateChanged(bool locked) {
......
......@@ -110,7 +110,7 @@ TEST_F(NearbyShareDelegateImplTest, StartHighVisibilityAndTimeout) {
delegate_.EnableHighVisibility();
SetHighVisibilityOn(true);
EXPECT_CALL(*settings_opener_, ShowSettingsPage(_));
EXPECT_CALL(nearby_share_service_, ClearForegroundReceiveSurfaces());
EXPECT_CALL(controller_, HighVisibilityEnabledChanged(false));
// DisableHighVisibility will be called automatically after the timer fires.
......@@ -127,7 +127,7 @@ TEST_F(NearbyShareDelegateImplTest, StartStopHighVisibility) {
delegate_.EnableHighVisibility();
SetHighVisibilityOn(true);
EXPECT_CALL(*settings_opener_, ShowSettingsPage(_));
EXPECT_CALL(nearby_share_service_, ClearForegroundReceiveSurfaces());
EXPECT_CALL(controller_, HighVisibilityEnabledChanged(false));
delegate_.DisableHighVisibility();
......@@ -137,9 +137,8 @@ TEST_F(NearbyShareDelegateImplTest, StartStopHighVisibility) {
TEST_F(NearbyShareDelegateImplTest, ShowOnboardingAndTurnOnHighVisibility) {
settings_.SetEnabled(false);
// Called once to start onboarding, once to enter high visibility, and once to
// exit high visibility.
EXPECT_CALL(*settings_opener_, ShowSettingsPage(_)).Times(3);
// Called once to start onboarding and once to enter high visibility
EXPECT_CALL(*settings_opener_, ShowSettingsPage(_)).Times(2);
delegate_.EnableHighVisibility();
......@@ -150,6 +149,7 @@ TEST_F(NearbyShareDelegateImplTest, ShowOnboardingAndTurnOnHighVisibility) {
settings_.SetEnabled(true);
SetHighVisibilityOn(true);
EXPECT_CALL(nearby_share_service_, ClearForegroundReceiveSurfaces());
EXPECT_CALL(controller_, HighVisibilityEnabledChanged(false));
// DisableHighVisibility will be called automatically after the timer fires.
......@@ -186,7 +186,7 @@ TEST_F(NearbyShareDelegateImplTest, StopHighVisibilityOnScreenLock) {
SetHighVisibilityOn(true);
EXPECT_CALL(controller_, HighVisibilityEnabledChanged(false));
EXPECT_CALL(*settings_opener_, ShowSettingsPage(_));
EXPECT_CALL(nearby_share_service_, ClearForegroundReceiveSurfaces());
// DisableHighVisibility will be called when the screen locks.
delegate_.OnLockStateChanged(/*locked=*/true);
......
......@@ -101,6 +101,9 @@ class NearbySharingService : public KeyedService {
virtual StatusCodes UnregisterReceiveSurface(
TransferUpdateCallback* transfer_callback) = 0;
// Unregisters all foreground receive surfaces.
virtual StatusCodes ClearForegroundReceiveSurfaces() = 0;
// Returns true if a foreground receive surface is registered.
virtual bool IsInHighVisibility() = 0;
......
......@@ -573,6 +573,20 @@ NearbySharingServiceImpl::UnregisterReceiveSurface(
return StatusCodes::kOk;
}
NearbySharingService::StatusCodes
NearbySharingServiceImpl::ClearForegroundReceiveSurfaces() {
std::vector<TransferUpdateCallback*> fg_receivers;
for (auto& callback : foreground_receive_callbacks_)
fg_receivers.push_back(&callback);
StatusCodes status = StatusCodes::kOk;
for (TransferUpdateCallback* callback : fg_receivers) {
if (UnregisterReceiveSurface(callback) != StatusCodes::kOk)
status = StatusCodes::kError;
}
return status;
}
bool NearbySharingServiceImpl::IsInHighVisibility() {
return in_high_visibility;
}
......
......@@ -89,6 +89,7 @@ class NearbySharingServiceImpl
ReceiveSurfaceState state) override;
StatusCodes UnregisterReceiveSurface(
TransferUpdateCallback* transfer_callback) override;
StatusCodes ClearForegroundReceiveSurfaces() override;
bool IsInHighVisibility() override;
StatusCodes SendAttachments(
const ShareTarget& share_target,
......
......@@ -89,6 +89,9 @@ Polymer({
/** @private {?nearbyShare.mojom.ReceiveObserverReceiver} */
observerReceiver_: null,
/** @private {number} */
closeTimeoutId_: 0,
/** @override */
attached() {
this.closing_ = false;
......@@ -112,8 +115,16 @@ Polymer({
*/
onHighVisibilityChanged(inHighVisibility) {
if (inHighVisibility == false) {
// TODO(vecore): Show error state to user
this.close_();
// TODO(crbug/1134745): Exiting high visibility can happen for multiple
// reasons (timeout, user cancel, etc). During a receive transfer, it
// happens before we start connecting (because we need to stop
// advertising) so we need to wait a bit to see if we see an
// onIncomingShare event within a reasonable timeout. This is the normal
// case and it should happen quickly when it is a real connection. In the
// timeout case, we are just exiting high visibility normally and can
// close for now, and the small timeout won't impact UX. Ideally we should
// refactor to not require the use of a timeout.
this.closeTimeoutId_ = setTimeout(this.close_.bind(this), 25);
}
},
......@@ -124,6 +135,7 @@ Polymer({
* @param {?string} connectionToken
*/
onIncomingShare(shareTarget, connectionToken) {
clearTimeout(this.closeTimeoutId_);
this.shareTarget = shareTarget;
this.connectionToken = connectionToken;
this.showConfirmPage();
......@@ -161,7 +173,7 @@ Polymer({
}
this.closing_ = true;
this.receiveManager_.exitHighVisibility().then(() => {
this.receiveManager_.unregisterForegroundReceiveSurface().then(() => {
const dialog = /** @type {!CrDialogElement} */ (this.$.dialog);
if (dialog.open) {
dialog.close();
......@@ -216,8 +228,8 @@ Polymer({
return;
}
// Request to enter high visibility mode and show the page.
this.receiveManager_.enterHighVisibility();
// Register a receive surface to enter high visibility and show the page.
this.receiveManager_.registerForegroundReceiveSurface();
this.getViewManager_().switchView(Page.HIGH_VISIBILITY);
},
......
......@@ -281,10 +281,6 @@ Polymer({
this.$$('#receiveDialog').showHighVisibilityPage();
}
if (queryParams.has('stop_receiving')) {
this.showReceiveDialog_ = false;
}
if (queryParams.has('confirm')) {
this.showReceiveDialog_ = true;
Polymer.dom.flush();
......
......@@ -148,9 +148,10 @@ interface ReceiveObserver {
OnIncomingShare(ShareTarget share_target, string? connection_token);
};
// Allows the caller to observe changes to, query, or explicitly enter or exit
// high visibility. Once in high visibility, the caller may observe incoming
// shares and accept or reject them.
// Allows the caller to observe changes to or query high visibility, or
// register/unregister a foreground receive surface, which will cause high
// visibility to start/stop respectively. Once in high visibility, the caller
// may observe incoming shares and accept or reject them.
//
// This interface runs in the browser process and is bound by os-settings which
// hosts the ui surface and implements the |ReceiveObserver|.
......@@ -161,15 +162,20 @@ interface ReceiveManager {
// Determine if the user is currently in high visibility.
IsInHighVisibility() => (bool in_high_visibility);
// Attempt to explicitly enter high visibility. If |success| is true the
// receive manager is in high visibility state. If |success| is false the
// receive manager is not in high visibility state.
EnterHighVisibility() => (bool success);
// Attempt to explicitly exit high visibility. If |success| is true the
// receive manager is not in high visibility state. If |success| is false the
// receive manager is still in high visibility state.
ExitHighVisibility() => (bool success);
// Attempt to register the receive manager as a foreground receive surface.
// If |success| is true, then the surface has been registered; otherwise it
// has not been registered. The foreground surface receives incoming
// shares, and once one is registered, high visibility will become active
// after a short while. Calling multiple times will have no additional effect.
RegisterForegroundReceiveSurface() => (bool success);
// Attempt to unregister the receive manager as a foreground receive surface.
// If |success| is true, then the surface is no longer registered; otherwise,
// it is still registered. The foreground surface receives incoming shares,
// and once it is no longer registered, high visibility will no longer be
// active after a short while. Calling unregister before register will have
// no effect.
UnregisterForegroundReceiveSurface() => (bool success);
// Accept the incoming share request by the share target id.
Accept(mojo_base.mojom.UnguessableToken share_target_id) => (bool success);
......
......@@ -18,8 +18,8 @@ cr.define('nearby_share', function() {
super([
'addReceiveObserver',
'isInHighVisibility',
'enterHighVisibility',
'exitHighVisibility',
'registerForegroundReceiveSurface',
'unregisterForegroundReceiveSurface',
'accept',
'reject',
]);
......@@ -63,24 +63,24 @@ cr.define('nearby_share', function() {
/**
* @return {!Promise<{success: !boolean}>}
*/
async enterHighVisibility() {
async registerForegroundReceiveSurface() {
this.inHighVisibility_ = true;
if (this.observer_) {
this.observer_.onHighVisibilityChanged(this.inHighVisibility_);
}
this.methodCalled('enterHighVisibility');
this.methodCalled('registerForegroundReceiveSurface');
return {success: this.nextResult_};
}
/**
* @return {!Promise<{success: !boolean}>}
*/
async exitHighVisibility() {
async unregisterForegroundReceiveSurface() {
this.inHighVisibility_ = false;
if (this.observer_) {
this.observer_.onHighVisibilityChanged(this.inHighVisibility_);
}
this.methodCalled('exitHighVisibility');
this.methodCalled('unregisterForegroundReceiveSurface');
return {success: this.nextResult_};
}
......
......@@ -116,16 +116,30 @@ suite('NearbyShare', function() {
});
test(
'show high visibility page, exitHighVisibility, closes dialog',
'show high visibility page, unregister surface, closes dialog',
async function() {
// When attached we enter high visibility mode by default
assertTrue(isVisible('nearby-share-high-visibility-page'));
assertFalse(isVisible('nearby-share-confirm-page'));
// If a share target comes in, we show it.
await fakeReceiveManager.exitHighVisibility();
await fakeReceiveManager.unregisterForegroundReceiveSurface();
Polymer.dom.flush();
assertFalse(isVisible('cr-dialog'));
});
test(
'unregister surface, onIncomingShare, does not close dialog',
async function() {
// When attached we enter high visibility mode by default
assertTrue(isVisible('nearby-share-high-visibility-page'));
assertFalse(isVisible('nearby-share-confirm-page'));
// If a share target comes in, we show it.
await fakeReceiveManager.unregisterForegroundReceiveSurface();
const target =
fakeReceiveManager.simulateShareTargetArrival('testName', '1234');
Polymer.dom.flush();
assertFalse(dialog.closing_);
});
});
suite('DisabledTests', function() {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment