Refactor FidoHidDevice::MessageReceived()

|is_success| parameter in FidoHidDevice::MessageReceived() can be
represented by nullptr |message| parameter.

Bug: 828088
Change-Id: Ia13439acc98f43af1741308063ac7c2ab1b62af9
Reviewed-on: https://chromium-review.googlesource.com/1014655Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Commit-Queue: Jun Choi <hongjunchoi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551741}

device/fido/fido_hid_device.cc

@@ -148,13 +148,13 @@ void FidoHidDevice::AllocateChannel(std::vector<uint8_t> command,
 void FidoHidDevice::OnAllocateChannel(std::vector<uint8_t> nonce,
                                       std::vector<uint8_t> command,
                                       DeviceCallback callback,
-                                      bool success,
-                                      std::unique_ptr<FidoHidMessage> message) {
+                                      base::Optional<FidoHidMessage> message) {
   if (state_ == State::kDeviceError)
     return;
+
   timeout_callback_.Cancel();
 
-  if (!success || !message) {
+  if (!message) {
     state_ = State::kDeviceError;
     Transition(std::vector<uint8_t>(), std::move(callback));
     return;
@@ -198,11 +198,11 @@ void FidoHidDevice::OnAllocateChannel(std::vector<uint8_t> nonce,
   Transition(std::move(command), std::move(callback));
 }
 
-void FidoHidDevice::WriteMessage(std::unique_ptr<FidoHidMessage> message,
+void FidoHidDevice::WriteMessage(base::Optional<FidoHidMessage> message,
                                  bool response_expected,
                                  HidMessageCallback callback) {
   if (!connection_ || !message || message->NumPackets() == 0) {
-    std::move(callback).Run(false, nullptr);
+    std::move(callback).Run(base::nullopt);
     return;
   }
   const auto& packet = message->PopNextPacket();
@@ -213,7 +213,7 @@ void FidoHidDevice::WriteMessage(std::unique_ptr<FidoHidMessage> message,
                      std::move(callback)));
 }
 
-void FidoHidDevice::PacketWritten(std::unique_ptr<FidoHidMessage> message,
+void FidoHidDevice::PacketWritten(base::Optional<FidoHidMessage> message,
                                   bool response_expected,
                                   HidMessageCallback callback,
                                   bool success) {
@@ -222,13 +222,13 @@ void FidoHidDevice::PacketWritten(std::unique_ptr<FidoHidMessage> message,
   } else if (success && response_expected) {
     ReadMessage(std::move(callback));
   } else {
-    std::move(callback).Run(success, nullptr);
+    std::move(callback).Run(base::nullopt);
   }
 }
 
 void FidoHidDevice::ReadMessage(HidMessageCallback callback) {
   if (!connection_) {
-    std::move(callback).Run(false, nullptr);
+    std::move(callback).Run(base::nullopt);
     return;
   }
 
@@ -241,14 +241,14 @@ void FidoHidDevice::OnRead(HidMessageCallback callback,
                            uint8_t report_id,
                            const base::Optional<std::vector<uint8_t>>& buf) {
   if (!success) {
-    std::move(callback).Run(success, nullptr);
+    std::move(callback).Run(base::nullopt);
     return;
   }
 
   DCHECK(buf);
   auto read_message = FidoHidMessage::CreateFromSerializedData(*buf);
   if (!read_message) {
-    std::move(callback).Run(false, nullptr);
+    std::move(callback).Run(base::nullopt);
     return;
   }
 
@@ -261,7 +261,7 @@ void FidoHidDevice::OnRead(HidMessageCallback callback,
   }
 
   if (read_message->MessageComplete()) {
-    std::move(callback).Run(success, std::move(read_message));
+    std::move(callback).Run(std::move(read_message));
     return;
   }
 
@@ -272,20 +272,20 @@ void FidoHidDevice::OnRead(HidMessageCallback callback,
 }
 
 void FidoHidDevice::OnReadContinuation(
-    std::unique_ptr<FidoHidMessage> message,
+    base::Optional<FidoHidMessage> message,
     HidMessageCallback callback,
     bool success,
     uint8_t report_id,
     const base::Optional<std::vector<uint8_t>>& buf) {
   if (!success) {
-    std::move(callback).Run(success, nullptr);
+    std::move(callback).Run(base::nullopt);
     return;
   }
 
   DCHECK(buf);
   message->AddContinuationPacket(*buf);
   if (message->MessageComplete()) {
-    std::move(callback).Run(success, std::move(message));
+    std::move(callback).Run(std::move(message));
     return;
   }
   connection_->Read(base::BindOnce(&FidoHidDevice::OnReadContinuation,
@@ -294,13 +294,12 @@ void FidoHidDevice::OnReadContinuation(
 }
 
 void FidoHidDevice::MessageReceived(DeviceCallback callback,
-                                    bool success,
-                                    std::unique_ptr<FidoHidMessage> message) {
+                                    base::Optional<FidoHidMessage> message) {
   if (state_ == State::kDeviceError)
     return;
 
   timeout_callback_.Cancel();
-  if (!success || !message) {
+  if (!message) {
     state_ = State::kDeviceError;
     Transition(std::vector<uint8_t>(), std::move(callback));
     return;
@@ -329,8 +328,8 @@ void FidoHidDevice::MessageReceived(DeviceCallback callback,
   state_ = State::kReady;
   base::WeakPtr<FidoHidDevice> self = weak_factory_.GetWeakPtr();
   std::move(callback).Run(
-      (success && message) ? base::make_optional(message->GetMessagePayload())
-                           : base::nullopt);
+      message ? base::make_optional(message->GetMessagePayload())
+              : base::nullopt);
 
   // Executing |callback| may have freed |this|. Check |self| first.
   if (self && !pending_transactions_.empty()) {
@@ -366,8 +365,7 @@ void FidoHidDevice::OnKeepAlive(DeviceCallback callback) {
 }
 
 void FidoHidDevice::OnWink(WinkCallback callback,
-                           bool success,
-                           std::unique_ptr<FidoHidMessage> response) {
+                           base::Optional<FidoHidMessage> response) {
   std::move(callback).Run();
 }
 

device/fido/fido_hid_device.h

@@ -5,7 +5,6 @@
 #ifndef DEVICE_FIDO_FIDO_HID_DEVICE_H_
 #define DEVICE_FIDO_FIDO_HID_DEVICE_H_
 
-#include <memory>
 #include <queue>
 #include <string>
 #include <utility>
@@ -15,6 +14,7 @@
 #include "base/cancelable_callback.h"
 #include "base/component_export.h"
 #include "base/macros.h"
+#include "base/optional.h"
 #include "components/apdu/apdu_command.h"
 #include "components/apdu/apdu_response.h"
 #include "device/fido/fido_device.h"
@@ -58,7 +58,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoHidDevice : public FidoDevice {
   static constexpr uint32_t kBroadcastChannel = 0xffffffff;
 
   using HidMessageCallback =
-      base::OnceCallback<void(bool, std::unique_ptr<FidoHidMessage>)>;
+      base::OnceCallback<void(base::Optional<FidoHidMessage>)>;
   using ConnectCallback = device::mojom::HidManager::ConnectCallback;
 
   // Open a connection to this device.
@@ -71,35 +71,31 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FidoHidDevice : public FidoDevice {
   void OnAllocateChannel(std::vector<uint8_t> nonce,
                          std::vector<uint8_t> command,
                          DeviceCallback callback,
-                         bool success,
-                         std::unique_ptr<FidoHidMessage> message);
+                         base::Optional<FidoHidMessage> message);
   void Transition(std::vector<uint8_t> command, DeviceCallback callback);
   // Write all message packets to device, and read response if expected.
-  void WriteMessage(std::unique_ptr<FidoHidMessage> message,
+  void WriteMessage(base::Optional<FidoHidMessage> message,
                     bool response_expected,
                     HidMessageCallback callback);
-  void PacketWritten(std::unique_ptr<FidoHidMessage> message,
+  void PacketWritten(base::Optional<FidoHidMessage> message,
                      bool response_expected,
                      HidMessageCallback callback,
                      bool success);
   // Read all response message packets from device.
   void ReadMessage(HidMessageCallback callback);
   void MessageReceived(DeviceCallback callback,
-                       bool success,
-                       std::unique_ptr<FidoHidMessage> message);
+                       base::Optional<FidoHidMessage> message);
   void OnRead(HidMessageCallback callback,
               bool success,
               uint8_t report_id,
               const base::Optional<std::vector<uint8_t>>& buf);
-  void OnReadContinuation(std::unique_ptr<FidoHidMessage> message,
+  void OnReadContinuation(base::Optional<FidoHidMessage> message,
                           HidMessageCallback callback,
                           bool success,
                           uint8_t report_id,
                           const base::Optional<std::vector<uint8_t>>& buf);
   void OnKeepAlive(DeviceCallback callback);
-  void OnWink(WinkCallback callback,
-              bool success,
-              std::unique_ptr<FidoHidMessage> response);
+  void OnWink(WinkCallback callback, base::Optional<FidoHidMessage> response);
   void ArmTimeout(DeviceCallback callback);
   void OnTimeout(DeviceCallback callback);
 

device/fido/fido_hid_message.cc

@@ -15,12 +15,12 @@
 namespace device {
 
 // static
-std::unique_ptr<FidoHidMessage> FidoHidMessage::Create(
+base::Optional<FidoHidMessage> FidoHidMessage::Create(
     uint32_t channel_id,
     FidoHidDeviceCommand type,
     base::span<const uint8_t> data) {
   if (data.size() > kHidMaxMessageSize)
-    return nullptr;
+    return base::nullopt;
 
   switch (type) {
     case FidoHidDeviceCommand::kPing:
@@ -28,53 +28,56 @@ std::unique_ptr<FidoHidMessage> FidoHidMessage::Create(
     case FidoHidDeviceCommand::kMsg:
     case FidoHidDeviceCommand::kCbor: {
       if (data.empty())
-        return nullptr;
+        return base::nullopt;
       break;
     }
 
     case FidoHidDeviceCommand::kCancel:
     case FidoHidDeviceCommand::kWink: {
       if (!data.empty())
-        return nullptr;
+        return base::nullopt;
       break;
     }
     case FidoHidDeviceCommand::kLock: {
       if (data.size() != 1 || data[0] > kHidMaxLockSeconds)
-        return nullptr;
+        return base::nullopt;
       break;
     }
     case FidoHidDeviceCommand::kInit: {
       if (data.size() != 8)
-        return nullptr;
+        return base::nullopt;
       break;
     }
     case FidoHidDeviceCommand::kKeepAlive:
     case FidoHidDeviceCommand::kError:
       if (data.size() != 1)
-        return nullptr;
+        return base::nullopt;
   }
 
-  return base::WrapUnique(new FidoHidMessage(channel_id, type, data));
+  return FidoHidMessage(channel_id, type, data);
 }
 
 // static
-std::unique_ptr<FidoHidMessage> FidoHidMessage::CreateFromSerializedData(
+base::Optional<FidoHidMessage> FidoHidMessage::CreateFromSerializedData(
     base::span<const uint8_t> serialized_data) {
   size_t remaining_size = 0;
   if (serialized_data.size() > kHidPacketSize ||
       serialized_data.size() < kHidInitPacketHeaderSize)
-    return nullptr;
+    return base::nullopt;
 
   auto init_packet = FidoHidInitPacket::CreateFromSerializedData(
       serialized_data, &remaining_size);
 
   if (init_packet == nullptr)
-    return nullptr;
+    return base::nullopt;
 
-  return base::WrapUnique(
-      new FidoHidMessage(std::move(init_packet), remaining_size));
+  return FidoHidMessage(std::move(init_packet), remaining_size);
 }
 
+FidoHidMessage::FidoHidMessage(FidoHidMessage&& that) = default;
+
+FidoHidMessage& FidoHidMessage::operator=(FidoHidMessage&& other) = default;
+
 FidoHidMessage::~FidoHidMessage() = default;
 
 bool FidoHidMessage::MessageComplete() const {

device/fido/fido_hid_message.h

@@ -17,6 +17,7 @@
 #include "base/containers/queue.h"
 #include "base/containers/span.h"
 #include "base/macros.h"
+#include "base/optional.h"
 #include "device/fido/fido_constants.h"
 #include "device/fido/fido_hid_packet.h"
 
@@ -27,14 +28,16 @@ namespace device {
 class COMPONENT_EXPORT(DEVICE_FIDO) FidoHidMessage {
  public:
   // Static functions to create CTAP/U2F HID commands.
-  static std::unique_ptr<FidoHidMessage> Create(uint32_t channel_id,
-                                                FidoHidDeviceCommand cmd,
-                                                base::span<const uint8_t> data);
+  static base::Optional<FidoHidMessage> Create(uint32_t channel_id,
+                                               FidoHidDeviceCommand cmd,
+                                               base::span<const uint8_t> data);
 
   // Reconstruct a message from serialized message data.
-  static std::unique_ptr<FidoHidMessage> CreateFromSerializedData(
+  static base::Optional<FidoHidMessage> CreateFromSerializedData(
       base::span<const uint8_t> serialized_data);
 
+  FidoHidMessage(FidoHidMessage&& that);
+  FidoHidMessage& operator=(FidoHidMessage&& other);
   ~FidoHidMessage();
 
   bool MessageComplete() const;

device/fido/fido_hid_message_unittest.cc

@@ -160,7 +160,7 @@ TEST(FidoHidMessageTest, TestMaxSize) {
   std::vector<uint8_t> data(kHidMaxMessageSize + 1);
   auto oversize_message =
       FidoHidMessage::Create(channel_id, FidoHidDeviceCommand::kPing, data);
-  EXPECT_EQ(nullptr, oversize_message);
+  EXPECT_FALSE(oversize_message);
 }
 
 TEST(FidoHidMessageTest, TestDeconstruct) {