[Nearby] Correctly set is_final_status

The final status flag is determined by the transfer status. Additionally
this CL adds a Clone() method to TransferMetadataBuilder to be able to
update fields from already build TransferMetadata objects.

Bug: 1085067
Change-Id: Ie4a3cb335897f477329bc21189c33530ee7848ec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2349848Reviewed-by: Alex Chau <alexchau@chromium.org>
Commit-Queue: Richard Knoll <knollr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#797243}

chrome/browser/nearby_sharing/nearby_notification_manager_unittest.cc

@@ -623,7 +623,6 @@ TEST_F(NearbyNotificationManagerTest, ProgressNotification_Cancelled) {
   manager()->OnTransferUpdate(
       share_target, TransferMetadataBuilder()
                         .set_status(TransferMetadata::Status::kCancelled)
-                        .set_is_final_status(true)
                         .build());
 
   // Notification should be closed.

chrome/browser/nearby_sharing/nearby_per_session_discovery_manager.cc

@@ -44,6 +44,12 @@ void NearbyPerSessionDiscoveryManager::OnTransferUpdate(
                /*token=*/base::nullopt, mojo::NullRemote());
       break;
     default:
+      if (transfer_metadata.is_final_status() &&
+          select_share_target_callback_) {
+        std::move(select_share_target_callback_)
+            .Run(nearby_share::mojom::SelectShareTargetResult::kError,
+                 /*token=*/base::nullopt, mojo::NullRemote());
+      }
       break;
   }
 }
@@ -64,29 +70,29 @@ void NearbyPerSessionDiscoveryManager::OnShareTargetLost(
 void NearbyPerSessionDiscoveryManager::StartDiscovery(
     mojo::PendingRemote<nearby_share::mojom::ShareTargetListener> listener,
     StartDiscoveryCallback callback) {
-  DCHECK(!share_target_listener_.is_bound());
+  share_target_listener_.Bind(std::move(listener));
+  // |share_target_listener_| owns the callbacks and is guaranteed to be
+  // destroyed before |this|, therefore making base::Unretained() safe to use.
+  share_target_listener_.set_disconnect_handler(
+      base::BindOnce(&NearbyPerSessionDiscoveryManager::UnregisterSendSurface,
+                     base::Unretained(this)));
 
   if (nearby_sharing_service_->RegisterSendSurface(
           this, this, NearbySharingService::SendSurfaceState::kForeground) !=
       NearbySharingService::StatusCodes::kOk) {
     NS_LOG(WARNING) << "Failed to register send surface";
+    share_target_listener_.reset();
     std::move(callback).Run(/*success=*/false);
     return;
   }
 
-  share_target_listener_.Bind(std::move(listener));
-  // |share_target_listener_| owns the callbacks and is guaranteed to be
-  // destroyed before |this|, therefore making base::Unretained() safe to use.
-  share_target_listener_.set_disconnect_handler(
-      base::BindOnce(&NearbyPerSessionDiscoveryManager::UnregisterSendSurface,
-                     base::Unretained(this)));
-
   std::move(callback).Run(/*success=*/true);
 }
 
 void NearbyPerSessionDiscoveryManager::SelectShareTarget(
     const base::UnguessableToken& share_target_id,
     SelectShareTargetCallback callback) {
+  DCHECK(share_target_listener_.is_bound());
   DCHECK(!select_share_target_callback_);
 
   auto iter = discovered_share_targets_.find(share_target_id);
@@ -108,7 +114,10 @@ void NearbyPerSessionDiscoveryManager::SelectShareTarget(
 
 void NearbyPerSessionDiscoveryManager::OnSend(
     NearbySharingService::StatusCodes status) {
-  DCHECK(select_share_target_callback_);
+  // Nothing to do if the result has been returned already.
+  if (!select_share_target_callback_)
+    return;
+
   // If the send call succeeded, we expect OnTransferUpdate() to be called next.
   if (status == NearbySharingService::StatusCodes::kOk)
     return;

chrome/browser/nearby_sharing/nearby_per_session_discovery_manager_unittest.cc

@@ -146,6 +146,12 @@ TEST_F(NearbyPerSessionDiscoveryManagerTest, OnShareTargetLost) {
 }
 
 TEST_F(NearbyPerSessionDiscoveryManagerTest, SelectShareTarget_Invalid) {
+  MockShareTargetListener listener;
+  EXPECT_CALL(sharing_service(),
+              RegisterSendSurface(testing::_, testing::_, testing::_))
+      .WillOnce(testing::Return(NearbySharingService::StatusCodes::kOk));
+  manager().StartDiscovery(listener.Bind(), base::DoNothing());
+
   MockSelectShareTargetCallback callback;
   EXPECT_CALL(
       callback,

chrome/browser/nearby_sharing/nearby_sharing_service_impl.cc

@@ -1129,7 +1129,10 @@ void NearbySharingServiceImpl::OnIncomingTransferUpdate(
     TransferMetadata metadata) {
   if (metadata.status() != TransferMetadata::Status::kCancelled &&
       metadata.status() != TransferMetadata::Status::kRejected) {
-    last_incoming_metadata_ = std::make_pair(share_target, metadata);
+    last_incoming_metadata_ =
+        std::make_pair(share_target, TransferMetadataBuilder::Clone(metadata)
+                                         .set_is_original(false)
+                                         .build());
   } else {
     last_incoming_metadata_ = base::nullopt;
   }

chrome/browser/nearby_sharing/transfer_metadata.cc

@@ -6,6 +6,25 @@
 
 #include "chrome/browser/nearby_sharing/transfer_metadata.h"
 
+// static
+bool TransferMetadata::IsFinalStatus(Status status) {
+  switch (status) {
+    case Status::kAwaitingRemoteAcceptanceFailed:
+    case Status::kComplete:
+    case Status::kFailed:
+    case Status::kRejected:
+    case Status::kCancelled:
+    case Status::kTimedOut:
+    case Status::kMediaUnavailable:
+    case Status::kNotEnoughSpace:
+    case Status::kUnsupportedAttachmentType:
+    case Status::kExternalProviderLaunched:
+      return true;
+    default:
+      return false;
+  }
+}
+
 TransferMetadata::TransferMetadata(Status status,
                                    float progress,
                                    base::Optional<std::string> token,

chrome/browser/nearby_sharing/transfer_metadata.h

@@ -34,6 +34,8 @@ class TransferMetadata {
     kMaxValue = kExternalProviderLaunched
   };
 
+  static bool IsFinalStatus(Status status);
+
   TransferMetadata(Status status,
                    float progress,
                    base::Optional<std::string> token,

chrome/browser/nearby_sharing/transfer_metadata_builder.cc

@@ -4,13 +4,30 @@
 
 #include "chrome/browser/nearby_sharing/transfer_metadata_builder.h"
 
+// static
+TransferMetadataBuilder TransferMetadataBuilder::Clone(
+    const TransferMetadata& metadata) {
+  TransferMetadataBuilder builder;
+  builder.is_original_ = metadata.is_original();
+  builder.progress_ = metadata.progress();
+  builder.status_ = metadata.status();
+  builder.token_ = metadata.token();
+  return builder;
+}
+
 TransferMetadataBuilder::TransferMetadataBuilder() = default;
 
+TransferMetadataBuilder::TransferMetadataBuilder(TransferMetadataBuilder&&) =
+    default;
+
+TransferMetadataBuilder& TransferMetadataBuilder::operator=(
+    TransferMetadataBuilder&&) = default;
+
 TransferMetadataBuilder::~TransferMetadataBuilder() = default;
 
-TransferMetadataBuilder& TransferMetadataBuilder::set_is_final_status(
-    bool is_final_status) {
-  is_final_status_ = is_final_status;
+TransferMetadataBuilder& TransferMetadataBuilder::set_is_original(
+    bool is_original) {
+  is_original_ = is_original;
   return *this;
 }
 
@@ -33,6 +50,6 @@ TransferMetadataBuilder& TransferMetadataBuilder::set_token(
 }
 
 TransferMetadata TransferMetadataBuilder::build() const {
-  return TransferMetadata(status_, progress_, token_,
-                          /*is_original=*/false, is_final_status_);
+  return TransferMetadata(status_, progress_, token_, is_original_,
+                          TransferMetadata::IsFinalStatus(status_));
 }

chrome/browser/nearby_sharing/transfer_metadata_builder.h

@@ -12,10 +12,14 @@
 
 class TransferMetadataBuilder {
  public:
+  static TransferMetadataBuilder Clone(const TransferMetadata& metadata);
+
   TransferMetadataBuilder();
+  TransferMetadataBuilder(TransferMetadataBuilder&&);
+  TransferMetadataBuilder& operator=(TransferMetadataBuilder&&);
   ~TransferMetadataBuilder();
 
-  TransferMetadataBuilder& set_is_final_status(bool is_final_status);
+  TransferMetadataBuilder& set_is_original(bool is_original);
 
   TransferMetadataBuilder& set_progress(double progress);
 
@@ -26,7 +30,7 @@ class TransferMetadataBuilder {
   TransferMetadata build() const;
 
  private:
-  bool is_final_status_ = false;
+  bool is_original_ = false;
   double progress_ = 0;
   TransferMetadata::Status status_ = TransferMetadata::Status::kInProgress;
   base::Optional<std::string> token_;