Update file transfer proto for more flexibility.

This does not add any new functionality, but sets the stage for things
like requesting the host to show a file picker.

Bug: 679313
Change-Id: Id0a25d038f274584dfd6661df50658437ad29258
Reviewed-on: https://chromium-review.googlesource.com/c/1265028
Commit-Queue: Erik Jensen <rkjnsn@chromium.org>
Reviewed-by: Joe Downing <joedow@chromium.org>
Reviewed-by: Jamie Walch <jamiewalch@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605033}

remoting/host/file_proxy_wrapper.h

@@ -12,8 +12,6 @@
 
 namespace remoting {
 
-class CompoundBuffer;
-
 // FileProxyWrapper is an interface for implementing platform-specific file
 // writers for file transfers. Each operation is posted to a separate file IO
 // thread, and possibly a different process depending on the platform.
@@ -43,13 +41,11 @@ class FileProxyWrapper {
     kFailed = 5,
   };
 
-  // If an error occured while writing the file, State will be kFailed and the
-  // Optional will contain the error which occured. If the file was written to
-  // and closed successfully, State will be kClosed and the Optional will be
-  // empty.
-  typedef base::OnceCallback<
-      void(State, base::Optional<protocol::FileTransferResponse_ErrorCode>)>
-      StatusCallback;
+  // If writing the file fails, the status callback will be called with the
+  // causal error. Otherwise, the callback will be called with a nullopt once
+  // the file has been successfully written.
+  typedef base::OnceCallback<void(base::Optional<protocol::FileTransfer_Error>)>
+      ResultCallback;
 
   typedef base::OnceCallback<void(int64_t filesize)> OpenFileCallback;
 
@@ -62,18 +58,18 @@ class FileProxyWrapper {
   FileProxyWrapper();
   virtual ~FileProxyWrapper();
 
-  // |status_callback| is called either when FileProxyWrapper encounters an
+  // |result_callback| is called either when FileProxyWrapper encounters an
   // error or when Close() has been called and the file has been written
-  // successfully. |status_callback| must not immediately destroy this
+  // successfully. |result_callback| must not immediately destroy this
   // FileProxyWrapper.
-  virtual void Init(StatusCallback status_callback) = 0;
+  virtual void Init(ResultCallback result_callback) = 0;
   // Creates a new file and opens it for writing.
   virtual void CreateFile(const base::FilePath& directory,
                           const std::string& filename) = 0;
   // Opens an existing file for reading.
   virtual void OpenFile(const base::FilePath& filepath,
                         OpenFileCallback open_callback) = 0;
-  virtual void WriteChunk(std::unique_ptr<CompoundBuffer> buffer) = 0;
+  virtual void WriteChunk(std::string buffer) = 0;
   // |size| must not be greater than the remaining amount of bytes in the file
   // from the current read offset. After calling ReadChunk(), ReadChunk() cannot
   // be called again until |read_callback| is called and state() returns kReady.

remoting/host/file_proxy_wrapper_linux_unittest.cc

@@ -26,14 +26,6 @@ const std::string& kTestDataOne = "this is the first test string";
 const std::string& kTestDataTwo = "this is the second test string";
 const std::string& kTestDataThree = "this is the third test string";
 
-std::unique_ptr<remoting::CompoundBuffer> ToBuffer(const std::string& data) {
-  std::unique_ptr<remoting::CompoundBuffer> buffer =
-      std::make_unique<remoting::CompoundBuffer>();
-  buffer->Append(base::MakeRefCounted<net::WrappedIOBuffer>(data.data()),
-                 data.size());
-  return buffer;
-}
-
 }  // namespace
 
 namespace remoting {
@@ -52,9 +44,7 @@ class FileProxyWrapperLinuxTest : public testing::Test {
     return dir_.GetPath().Append(kTestFilename);
   }
 
-  void StatusCallback(
-      FileProxyWrapper::State state,
-      base::Optional<protocol::FileTransferResponse_ErrorCode> error);
+  void ResultCallback(base::Optional<protocol::FileTransfer_Error> error);
   void OpenFileCallback(int64_t filesize);
   void ReadChunkCallback(std::unique_ptr<std::vector<char>> chunk);
 
@@ -63,8 +53,7 @@ class FileProxyWrapperLinuxTest : public testing::Test {
   base::ScopedTempDir dir_;
 
   std::unique_ptr<FileProxyWrapper> file_proxy_wrapper_;
-  base::Optional<protocol::FileTransferResponse_ErrorCode> error_;
-  FileProxyWrapper::State final_state_;
+  base::Optional<protocol::FileTransfer_Error> error_;
   bool done_callback_succeeded_;
 
   base::queue<std::vector<char>> read_chunks_;
@@ -82,10 +71,9 @@ void FileProxyWrapperLinuxTest::SetUp() {
 
   file_proxy_wrapper_ = FileProxyWrapper::Create();
   file_proxy_wrapper_->Init(base::BindOnce(
-      &FileProxyWrapperLinuxTest::StatusCallback, base::Unretained(this)));
+      &FileProxyWrapperLinuxTest::ResultCallback, base::Unretained(this)));
 
-  error_ = base::Optional<protocol::FileTransferResponse_ErrorCode>();
-  final_state_ = FileProxyWrapper::kUninitialized;
+  error_ = base::nullopt;
   done_callback_succeeded_ = false;
 
   read_chunks_ = base::queue<std::vector<char>>();
@@ -96,12 +84,10 @@ void FileProxyWrapperLinuxTest::TearDown() {
   file_proxy_wrapper_.reset();
 }
 
-void FileProxyWrapperLinuxTest::StatusCallback(
-    FileProxyWrapper::State state,
-    base::Optional<protocol::FileTransferResponse_ErrorCode> error) {
-  final_state_ = state;
+void FileProxyWrapperLinuxTest::ResultCallback(
+    base::Optional<protocol::FileTransfer_Error> error) {
   error_ = error;
-  done_callback_succeeded_ = !error_.has_value();
+  done_callback_succeeded_ = !error_;
 }
 
 void FileProxyWrapperLinuxTest::OpenFileCallback(int64_t filesize) {
@@ -117,14 +103,13 @@ void FileProxyWrapperLinuxTest::ReadChunkCallback(
 // throwing any errors.
 TEST_F(FileProxyWrapperLinuxTest, WriteThreeChunks) {
   file_proxy_wrapper_->CreateFile(TestDir(), kTestFilename);
-  file_proxy_wrapper_->WriteChunk(ToBuffer(kTestDataOne));
-  file_proxy_wrapper_->WriteChunk(ToBuffer(kTestDataTwo));
-  file_proxy_wrapper_->WriteChunk(ToBuffer(kTestDataThree));
+  file_proxy_wrapper_->WriteChunk(kTestDataOne);
+  file_proxy_wrapper_->WriteChunk(kTestDataTwo);
+  file_proxy_wrapper_->WriteChunk(kTestDataThree);
   file_proxy_wrapper_->Close();
   scoped_task_environment_.RunUntilIdle();
 
   ASSERT_FALSE(error_);
-  ASSERT_EQ(final_state_, FileProxyWrapper::kClosed);
   ASSERT_TRUE(done_callback_succeeded_);
 
   std::string actual_file_data;
@@ -135,7 +120,7 @@ TEST_F(FileProxyWrapperLinuxTest, WriteThreeChunks) {
 // Verifies that calling Cancel() deletes any temporary or destination files.
 TEST_F(FileProxyWrapperLinuxTest, CancelDeletesFiles) {
   file_proxy_wrapper_->CreateFile(TestDir(), kTestFilename);
-  file_proxy_wrapper_->WriteChunk(ToBuffer(kTestDataOne));
+  file_proxy_wrapper_->WriteChunk(kTestDataOne);
   scoped_task_environment_.RunUntilIdle();
 
   file_proxy_wrapper_->Cancel();
@@ -151,7 +136,7 @@ TEST_F(FileProxyWrapperLinuxTest, FileAlreadyExists) {
   WriteFile(TestFilePath(), kTestDataOne.data(), kTestDataOne.size());
 
   file_proxy_wrapper_->CreateFile(TestDir(), kTestFilename);
-  file_proxy_wrapper_->WriteChunk(ToBuffer(kTestDataTwo));
+  file_proxy_wrapper_->WriteChunk(kTestDataTwo);
   file_proxy_wrapper_->Close();
   scoped_task_environment_.RunUntilIdle();
 
@@ -161,7 +146,6 @@ TEST_F(FileProxyWrapperLinuxTest, FileAlreadyExists) {
   ASSERT_STREQ(kTestDataTwo.data(), actual_file_data.data());
 
   ASSERT_FALSE(error_);
-  ASSERT_EQ(final_state_, FileProxyWrapper::kClosed);
 }
 
 // Verifies that FileProxyWrapper can read chunks from a file.
@@ -222,7 +206,9 @@ TEST_F(FileProxyWrapperLinuxTest, FileDoesntExist) {
                                  base::Unretained(this)));
   scoped_task_environment_.RunUntilIdle();
 
-  ASSERT_EQ(error_, protocol::FileTransferResponse_ErrorCode_FILE_IO_ERROR);
+  ASSERT_TRUE(error_);
+  ASSERT_TRUE(error_->has_type());
+  ASSERT_EQ(error_->type(), protocol::FileTransfer_Error_Type_IO_ERROR);
 }
 
 }  // namespace remoting

remoting/host/file_transfer_message_handler.cc

@@ -7,7 +7,9 @@
 #include "base/bind.h"
 #include "base/path_service.h"
 #include "base/stl_util.h"
+#include "base/strings/stringprintf.h"
 #include "remoting/base/compound_buffer.h"
+#include "remoting/protocol/file_transfer_helpers.h"
 
 namespace remoting {
 
@@ -26,26 +28,76 @@ void FileTransferMessageHandler::OnConnected() {
   // base::Unretained is safe here because |file_proxy_wrapper_| is owned by
   // this class, so the callback cannot be run after this class is destroyed.
   file_proxy_wrapper_->Init(base::BindOnce(
-      &FileTransferMessageHandler::StatusCallback, base::Unretained(this)));
+      &FileTransferMessageHandler::SaveResultCallback, base::Unretained(this)));
 }
 
 void FileTransferMessageHandler::OnIncomingMessage(
     std::unique_ptr<CompoundBuffer> buffer) {
-  FileProxyWrapper::State proxy_state = file_proxy_wrapper_->state();
-  if (proxy_state == FileProxyWrapper::kBusy ||
-      proxy_state == FileProxyWrapper::kClosed ||
-      proxy_state == FileProxyWrapper::kFailed) {
+  protocol::FileTransfer message;
+  CompoundBufferInputStream buffer_stream(buffer.get());
+  if (!message.ParseFromZeroCopyStream(&buffer_stream)) {
+    CancelAndSendError(
+        protocol::MakeFileTransferError(
+            FROM_HERE, protocol::FileTransfer_Error_Type_PROTOCOL_ERROR),
+        "Failed to parse message.");
     return;
   }
 
-  if (request_) {
-    // File transfer is already in progress, just pass the buffer to
-    // FileProxyWrapper to be written.
-    SendToFileProxy(std::move(buffer));
-  } else {
-    // A new file transfer has been started, parse the message into a request
-    // protobuf.
-    ParseNewRequest(std::move(buffer));
+  if (message.has_metadata()) {
+    StartFile(std::move(*message.mutable_metadata()));
+    return;
+  }
+
+  switch (file_proxy_wrapper_->state()) {
+    case FileProxyWrapper::kReady:
+      // This is the expected state.
+      break;
+    case FileProxyWrapper::kFailed:
+      // Ignore any messages that come in after we've returned an error.
+      return;
+    case FileProxyWrapper::kInitialized:
+      // Don't send an error in response to an error.
+      if (!message.has_error()) {
+        CancelAndSendError(
+            protocol::MakeFileTransferError(
+                FROM_HERE, protocol::FileTransfer_Error_Type_PROTOCOL_ERROR),
+            "First message must contain file metadata");
+      }
+      return;
+    case FileProxyWrapper::kBusy:
+    case FileProxyWrapper::kClosed:
+      CancelAndSendError(
+          protocol::MakeFileTransferError(
+              FROM_HERE, protocol::FileTransfer_Error_Type_PROTOCOL_ERROR),
+          "Message received after End");
+      return;
+    default:
+      CancelAndSendError(
+          protocol::MakeFileTransferError(
+              FROM_HERE, protocol::FileTransfer_Error_Type_UNEXPECTED_ERROR),
+          base::StringPrintf("Unexpected FileProxyWrapper state: %d",
+                             file_proxy_wrapper_->state()));
+      return;
+  }
+
+  switch (message.message_case()) {
+    case protocol::FileTransfer::kData:
+      file_proxy_wrapper_->WriteChunk(
+          std::move(*message.mutable_data()->mutable_data()));
+      break;
+    case protocol::FileTransfer::kEnd:
+      file_proxy_wrapper_->Close();
+      break;
+    case protocol::FileTransfer::kCancel:
+    case protocol::FileTransfer::kError:
+      file_proxy_wrapper_->Cancel();
+      break;
+    default:
+      CancelAndSendError(
+          protocol::MakeFileTransferError(
+              FROM_HERE, protocol::FileTransfer_Error_Type_PROTOCOL_ERROR),
+          "Received invalid file-transfer message.");
+      break;
   }
 }
 
@@ -58,66 +110,44 @@ void FileTransferMessageHandler::OnDisconnecting() {
   }
 }
 
-void FileTransferMessageHandler::StatusCallback(
-    FileProxyWrapper::State state,
-    base::Optional<protocol::FileTransferResponse_ErrorCode> error) {
-  protocol::FileTransferResponse response;
-  if (error.has_value()) {
-    DCHECK_EQ(state, FileProxyWrapper::kFailed);
-    response.set_error(error.value());
+void FileTransferMessageHandler::SaveResultCallback(
+    base::Optional<protocol::FileTransfer_Error> error) {
+  protocol::FileTransfer result_message;
+  if (error) {
+    *result_message.mutable_error() = std::move(*error);
   } else {
-    DCHECK_EQ(state, FileProxyWrapper::kClosed);
-    response.set_state(protocol::FileTransferResponse_TransferState_DONE);
-    response.set_total_bytes_written(request_->filesize());
+    result_message.mutable_success();
   }
-  Send(response, base::Closure());
+  Send(result_message, base::Closure());
 }
 
-void FileTransferMessageHandler::SendToFileProxy(
-    std::unique_ptr<CompoundBuffer> buffer) {
-  DCHECK_EQ(file_proxy_wrapper_->state(), FileProxyWrapper::kReady);
-
-  total_bytes_written_ += buffer->total_bytes();
-  file_proxy_wrapper_->WriteChunk(std::move(buffer));
-  if (total_bytes_written_ >= request_->filesize()) {
-    file_proxy_wrapper_->Close();
-  }
-
-  if (total_bytes_written_ > request_->filesize()) {
-    LOG(ERROR) << "File transfer received " << total_bytes_written_
-               << " bytes, but request said there would only be "
-               << request_->filesize() << " bytes.";
-  }
-}
-
-void FileTransferMessageHandler::ParseNewRequest(
-    std::unique_ptr<CompoundBuffer> buffer) {
-  std::string message;
-  message.resize(buffer->total_bytes());
-  buffer->CopyTo(base::data(message), message.size());
-
-  request_ = std::make_unique<protocol::FileTransferRequest>();
-  if (!request_->ParseFromString(message)) {
-    CancelAndSendError("Failed to parse request protobuf");
+void FileTransferMessageHandler::StartFile(
+    protocol::FileTransfer::Metadata metadata) {
+  if (file_proxy_wrapper_->state() != FileProxyWrapper::kInitialized) {
+    CancelAndSendError(
+        protocol::MakeFileTransferError(
+            FROM_HERE, protocol::FileTransfer_Error_Type_PROTOCOL_ERROR),
+        "Only one file per connection is supported.");
     return;
   }
 
   base::FilePath target_directory;
   if (!base::PathService::Get(base::DIR_USER_DESKTOP, &target_directory)) {
     CancelAndSendError(
+        protocol::MakeFileTransferError(
+            FROM_HERE, protocol::FileTransfer_Error_Type_UNEXPECTED_ERROR),
         "Failed to get DIR_USER_DESKTOP from base::PathService::Get");
     return;
   }
-
-  file_proxy_wrapper_->CreateFile(target_directory, request_->filename());
+  file_proxy_wrapper_->CreateFile(target_directory, metadata.filename());
 }
 
-void FileTransferMessageHandler::CancelAndSendError(const std::string& error) {
-  LOG(ERROR) << error;
+void FileTransferMessageHandler::CancelAndSendError(
+    protocol::FileTransfer_Error error,
+    const std::string& log_message) {
+  LOG(ERROR) << log_message;
   file_proxy_wrapper_->Cancel();
-  protocol::FileTransferResponse response;
-  response.set_error(protocol::FileTransferResponse_ErrorCode_UNEXPECTED_ERROR);
-  Send(response, base::Closure());
+  SaveResultCallback(error);
 }
 
 }  // namespace remoting

remoting/host/file_transfer_message_handler.h

@@ -9,6 +9,7 @@
 #include <memory>
 #include <string>
 
+#include "base/optional.h"
 #include "remoting/host/file_proxy_wrapper.h"
 #include "remoting/proto/file_transfer.pb.h"
 #include "remoting/protocol/named_message_pipe_handler.h"
@@ -30,16 +31,12 @@ class FileTransferMessageHandler : public protocol::NamedMessagePipeHandler {
   void OnDisconnecting() override;
 
  private:
-  void StatusCallback(
-      FileProxyWrapper::State state,
-      base::Optional<protocol::FileTransferResponse_ErrorCode> error);
-  void SendToFileProxy(std::unique_ptr<CompoundBuffer> buffer);
-  void ParseNewRequest(std::unique_ptr<CompoundBuffer> buffer);
-  void CancelAndSendError(const std::string& error);
+  void SaveResultCallback(base::Optional<protocol::FileTransfer_Error> error);
+  void StartFile(protocol::FileTransfer_Metadata metadata);
+  void CancelAndSendError(protocol::FileTransfer_Error error,
+                          const std::string& log_message);
 
   std::unique_ptr<FileProxyWrapper> file_proxy_wrapper_;
-  std::unique_ptr<protocol::FileTransferRequest> request_;
-  uint64_t total_bytes_written_ = 0;
 };
 
 }  // namespace remoting

remoting/host/file_transfer_message_handler_unittest.cc

@@ -17,6 +17,7 @@
 #include "remoting/host/file_proxy_wrapper.h"
 #include "remoting/protocol/fake_message_pipe.h"
 #include "remoting/protocol/fake_message_pipe_wrapper.h"
+#include "remoting/protocol/file_transfer_helpers.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace {
@@ -24,14 +25,27 @@ namespace {
 constexpr char kTestDatachannelName[] = "filetransfer-test";
 constexpr char kTestFilename[] = "test-file.txt";
 
-std::unique_ptr<remoting::CompoundBuffer> ToBuffer(const std::string& data) {
+std::unique_ptr<remoting::CompoundBuffer> StringToBuffer(
+    const std::string& data) {
   std::unique_ptr<remoting::CompoundBuffer> buffer =
       std::make_unique<remoting::CompoundBuffer>();
-  buffer->Append(base::MakeRefCounted<net::WrappedIOBuffer>(data.data()),
+  buffer->Append(base::MakeRefCounted<net::StringIOBuffer>(data.data()),
                  data.size());
   return buffer;
 }
 
+std::unique_ptr<remoting::CompoundBuffer> MessageToBuffer(
+    const remoting::protocol::FileTransfer& message) {
+  return StringToBuffer(message.SerializeAsString());
+}
+
+std::unique_ptr<remoting::CompoundBuffer> DataToBuffer(
+    const std::string& data) {
+  remoting::protocol::FileTransfer message;
+  message.mutable_data()->set_data(data);
+  return MessageToBuffer(message);
+}
+
 // base::queue doesn't provide operator==.
 template <typename T>
 bool QueuesEqual(const base::queue<T>& a, const base::queue<T>& b) {
@@ -59,27 +73,26 @@ class FakeFileProxyWrapper : public FileProxyWrapper {
   ~FakeFileProxyWrapper() override;
 
   // FileProxyWrapper implementation.
-  void Init(StatusCallback status_callback) override;
+  void Init(ResultCallback result_callback) override;
   void CreateFile(const base::FilePath& directory,
                   const std::string& filename) override;
   void OpenFile(const base::FilePath& filepath,
                 OpenFileCallback open_callback) override;
-  void WriteChunk(std::unique_ptr<CompoundBuffer> buffer) override;
+  void WriteChunk(std::string buffer) override;
   void ReadChunk(uint64_t chunk_size, ReadCallback read_callback) override;
   void Close() override;
   void Cancel() override;
   State state() override;
 
-  void RunStatusCallback(
-      base::Optional<protocol::FileTransferResponse_ErrorCode> error);
+  void RunResultCallback(base::Optional<protocol::FileTransfer_Error> error);
   const std::string& filename();
-  base::queue<std::vector<char>> chunks();
+  base::queue<std::string> chunks();
 
  private:
   State state_ = kUninitialized;
-  StatusCallback status_callback_;
+  ResultCallback result_callback_;
   std::string filename_;
-  base::queue<std::vector<char>> chunks_;
+  base::queue<std::string> chunks_;
 };
 
 class FileTransferMessageHandlerTest : public testing::Test {
@@ -96,18 +109,18 @@ class FileTransferMessageHandlerTest : public testing::Test {
   const std::string kTestDataTwo = "this is the second test string";
 
   std::unique_ptr<protocol::FakeMessagePipe> fake_pipe_;
-  protocol::FileTransferRequest fake_request_;
-  std::string fake_request_string_;
+  protocol::FileTransfer fake_metadata_;
+  protocol::FileTransfer fake_end_;
 };
 
 FakeFileProxyWrapper::FakeFileProxyWrapper() = default;
 FakeFileProxyWrapper::~FakeFileProxyWrapper() = default;
 
-void FakeFileProxyWrapper::Init(StatusCallback status_callback) {
+void FakeFileProxyWrapper::Init(ResultCallback result_callback) {
   ASSERT_EQ(state_, kUninitialized);
   state_ = kInitialized;
 
-  status_callback_ = std::move(status_callback);
+  result_callback_ = std::move(result_callback);
 }
 
 void FakeFileProxyWrapper::CreateFile(const base::FilePath& directory,
@@ -126,13 +139,10 @@ void FakeFileProxyWrapper::OpenFile(const base::FilePath& filepath,
   // TODO(jarhar): Implement fake file reading.
 }
 
-void FakeFileProxyWrapper::WriteChunk(std::unique_ptr<CompoundBuffer> buffer) {
+void FakeFileProxyWrapper::WriteChunk(std::string buffer) {
   ASSERT_EQ(state_, kReady);
 
-  std::vector<char> data;
-  data.resize(buffer->total_bytes());
-  buffer->CopyTo(data.data(), data.size());
-  chunks_.push(data);
+  chunks_.push(std::move(buffer));
 }
 
 void FakeFileProxyWrapper::ReadChunk(uint64_t chunk_size,
@@ -155,16 +165,16 @@ FileProxyWrapper::State FakeFileProxyWrapper::state() {
   return state_;
 }
 
-void FakeFileProxyWrapper::RunStatusCallback(
-    base::Optional<protocol::FileTransferResponse_ErrorCode> error) {
-  std::move(status_callback_).Run(state_, error);
+void FakeFileProxyWrapper::RunResultCallback(
+    base::Optional<protocol::FileTransfer_Error> error) {
+  std::move(result_callback_).Run(std::move(error));
 }
 
 const std::string& FakeFileProxyWrapper::filename() {
   return filename_;
 }
 
-base::queue<std::vector<char>> FakeFileProxyWrapper::chunks() {
+base::queue<std::string> FakeFileProxyWrapper::chunks() {
   return chunks_;
 }
 
@@ -175,10 +185,12 @@ void FileTransferMessageHandlerTest::SetUp() {
   fake_pipe_ =
       base::WrapUnique(new protocol::FakeMessagePipe(false /* asynchronous */));
 
-  fake_request_ = protocol::FileTransferRequest();
-  fake_request_.set_filename(kTestFilename);
-  fake_request_.set_filesize(kTestDataOne.size() + kTestDataTwo.size());
-  fake_request_.SerializeToString(&fake_request_string_);
+  fake_metadata_.Clear();
+  fake_metadata_.mutable_metadata()->set_filename(kTestFilename);
+  fake_metadata_.mutable_metadata()->set_size(kTestDataOne.size() +
+                                              kTestDataTwo.size());
+  fake_end_.Clear();
+  fake_end_.mutable_end();
 }
 
 void FileTransferMessageHandlerTest::TearDown() {}
@@ -196,36 +208,29 @@ TEST_F(FileTransferMessageHandlerTest, WriteTwoChunks) {
                                  std::move(file_proxy_wrapper));
 
   fake_pipe_->OpenPipe();
-  fake_pipe_->Receive(ToBuffer(fake_request_string_));
-  fake_pipe_->Receive(ToBuffer(kTestDataOne));
-  fake_pipe_->Receive(ToBuffer(kTestDataTwo));
+  fake_pipe_->Receive(MessageToBuffer(fake_metadata_));
+  fake_pipe_->Receive(DataToBuffer(kTestDataOne));
+  fake_pipe_->Receive(DataToBuffer(kTestDataTwo));
+  fake_pipe_->Receive(MessageToBuffer(fake_end_));
 
-  file_proxy_wrapper_ptr->RunStatusCallback(
-      base::Optional<protocol::FileTransferResponse_ErrorCode>());
+  file_proxy_wrapper_ptr->RunResultCallback(base::nullopt);
 
-  base::queue<std::vector<char>> actual_chunks =
-      file_proxy_wrapper_ptr->chunks();
+  base::queue<std::string> actual_chunks = file_proxy_wrapper_ptr->chunks();
 
   fake_pipe_->ClosePipe();
   file_proxy_wrapper_ptr = nullptr;
 
-  base::queue<std::vector<char>> expected_chunks;
-  expected_chunks.push(
-      std::vector<char>(kTestDataOne.begin(), kTestDataOne.end()));
-  expected_chunks.push(
-      std::vector<char>(kTestDataTwo.begin(), kTestDataTwo.end()));
+  base::queue<std::string> expected_chunks;
+  expected_chunks.push(kTestDataOne);
+  expected_chunks.push(kTestDataTwo);
   ASSERT_TRUE(QueuesEqual(expected_chunks, actual_chunks));
 
   const base::queue<std::string>& actual_sent_messages =
       fake_pipe_->sent_messages();
-  protocol::FileTransferResponse expected_response;
-  expected_response.set_state(
-      protocol::FileTransferResponse_TransferState_DONE);
-  expected_response.set_total_bytes_written(fake_request_.filesize());
-  std::string expected_response_string;
-  expected_response.SerializeToString(&expected_response_string);
+  protocol::FileTransfer expected_response;
+  expected_response.mutable_success();
   base::queue<std::string> expected_sent_messages;
-  expected_sent_messages.push(expected_response_string);
+  expected_sent_messages.push(expected_response.SerializeAsString());
   ASSERT_TRUE(QueuesEqual(expected_sent_messages, actual_sent_messages));
 }
 
@@ -237,32 +242,29 @@ TEST_F(FileTransferMessageHandlerTest, FileProxyError) {
   // |file_proxy_wrapper_ptr| is valid until fake_pipe_->ClosePipe() is called.
   FakeFileProxyWrapper* file_proxy_wrapper_ptr = file_proxy_wrapper.get();
 
-  protocol::FileTransferResponse_ErrorCode fake_error =
-      protocol::FileTransferResponse_ErrorCode_FILE_IO_ERROR;
+  protocol::FileTransfer_Error fake_error = protocol::MakeFileTransferError(
+      FROM_HERE, protocol::FileTransfer_Error_Type_IO_ERROR);
 
   // This will delete itself when fake_pipe_->ClosePipe() is called.
   new FileTransferMessageHandler(kTestDatachannelName, fake_pipe_->Wrap(),
                                  std::move(file_proxy_wrapper));
 
   fake_pipe_->OpenPipe();
-  fake_pipe_->Receive(ToBuffer(fake_request_string_));
-  fake_pipe_->Receive(ToBuffer(kTestDataOne));
+  fake_pipe_->Receive(MessageToBuffer(fake_metadata_));
+  fake_pipe_->Receive(DataToBuffer(kTestDataOne));
 
   file_proxy_wrapper_ptr->Cancel();
-  file_proxy_wrapper_ptr->RunStatusCallback(
-      base::Optional<protocol::FileTransferResponse_ErrorCode>(fake_error));
+  file_proxy_wrapper_ptr->RunResultCallback(fake_error);
 
   fake_pipe_->ClosePipe();
   file_proxy_wrapper_ptr = nullptr;
 
   const base::queue<std::string>& actual_sent_messages =
       fake_pipe_->sent_messages();
-  protocol::FileTransferResponse expected_response;
-  expected_response.set_error(fake_error);
-  std::string expected_response_string;
-  expected_response.SerializeToString(&expected_response_string);
+  protocol::FileTransfer expected_response;
+  *expected_response.mutable_error() = fake_error;
   base::queue<std::string> expected_sent_messages;
-  expected_sent_messages.push(expected_response_string);
+  expected_sent_messages.push(expected_response.SerializeAsString());
   ASSERT_TRUE(QueuesEqual(expected_sent_messages, actual_sent_messages));
 }
 

remoting/proto/file_transfer.proto

@@ -4,27 +4,75 @@ option optimize_for = LITE_RUNTIME;
 
 package remoting.protocol;
 
-// Next Id: 3
-message FileTransferRequest {
-  optional string filename = 1;
-  optional uint64 filesize = 2;
-}
-
-// Next Id: 4
-message FileTransferResponse {
-  enum TransferState {
-    IN_PROGRESS = 1;
-    DONE = 2;
+// Composite message type for messages sent over file-transfer data channels.
+// Next Id: 7
+message FileTransfer {
+  // Sender->receiver message containing file metadata. Always sent before any
+  // Data messages.
+  // Next Id: 3
+  message Metadata {
+    optional string filename = 1;
+    // Note: there may be edge cases in which the file is transmitted
+    // successfully but doesn't exactly match the number of bytes reported here.
+    // Thus, the implementation should wait for the End message to determine
+    // when the file is complete and not rely on the exact size.
+    optional uint64 size = 2;
   }
-  optional TransferState state = 1;
 
-  enum ErrorCode {
-    OUT_OF_DISK_SPACE = 1;
-    PERMISSIONS_ERROR = 2;
-    FILE_IO_ERROR = 3;
-    UNEXPECTED_ERROR = 4;
+  // Sender->receiver message containing a chunk of file data.
+  // Next Id: 2
+  message Data { optional bytes data = 1; }
+
+  // Sender->receiver message sent after the last data chunk, signaling that
+  // the transfer is complete.
+  // Next Id: 1
+  message End {}
+
+  // Receiver->sender message sent in response to the End message, indicating
+  // that the file has been successfully saved.
+  // Next Id: 1
+  message Success {}
+
+  // Bidirectional message canceling the transfer as the result of a user action
+  // or otherwise not due to an error. When Cancel is received, no more messages
+  // relating to the transfer should be sent, but the canceling end should be
+  // prepared to handle any messages that may already be on the wire.
+  // Next Id: 1
+  message Cancel {}
+
+  // Bidirectional message aborting the transfer due to an error. This may be
+  // sent by the sender to signal a read error, by the receiver to signal a
+  // write error, or by either side to signal any of the myriad of other things
+  // that can go wrong while attempting to transfer a file.
+  // Next Id: 6
+  message Error {
+    enum Type {
+      UNSPECIFIED = 0;
+      UNEXPECTED_ERROR = 1;
+      PROTOCOL_ERROR = 2;
+      PERMISSION_DENIED = 3;
+      OUT_OF_DISK_SPACE = 4;
+      IO_ERROR = 5;
+    }
+
+    // An error category to be used to select a user-displayed error message.
+    optional Type type = 1;
+    // The error code returned by the failed API call (if applicable).
+    optional int32 api_error_code = 2;
+    // The function in which the error occured.
+    optional string function = 3;
+    // The source file in which the error occured.
+    optional string source_file = 4;
+    // The line number on which the error occurred.
+    optional uint32 line_number = 5;
   }
-  optional ErrorCode error = 2;
 
-  optional uint64 total_bytes_written = 3;
+  oneof message {
+    Metadata metadata = 1;
+    Data data = 2;
+    End end = 3;
+    Success success = 4;
+    Cancel cancel = 5;
+    Error error = 6;
+  }
 }

remoting/protocol/BUILD.gn

@@ -54,6 +54,8 @@ static_library("protocol") {
     "datagram_channel_factory.h",
     "errors.cc",
     "errors.h",
+    "file_transfer_helpers.cc",
+    "file_transfer_helpers.h",
     "frame_consumer.h",
     "frame_stats.cc",
     "frame_stats.h",

remoting/protocol/file_transfer_helpers.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "remoting/protocol/file_transfer_helpers.h"
+
+namespace remoting {
+namespace protocol {
+
+FileTransfer_Error MakeFileTransferError(
+    base::Location location,
+    FileTransfer_Error_Type type,
+    base::Optional<int32_t> api_error_code) {
+  FileTransfer_Error error;
+  error.set_type(type);
+  if (api_error_code) {
+    error.set_api_error_code(*api_error_code);
+  }
+  error.set_function(location.function_name());
+  error.set_source_file(location.file_name());
+  error.set_line_number(location.line_number());
+  return error;
+}
+
+}  // namespace protocol
+}  // namespace remoting

remoting/protocol/file_transfer_helpers.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef REMOTING_PROTOCOL_FILE_TRANSFER_HELPERS_H_
+#define REMOTING_PROTOCOL_FILE_TRANSFER_HELPERS_H_
+
+#include <cstdint>
+
+#include "base/location.h"
+#include "base/optional.h"
+#include "remoting/proto/file_transfer.pb.h"
+
+namespace remoting {
+namespace protocol {
+
+FileTransfer_Error MakeFileTransferError(
+    base::Location location,
+    FileTransfer_Error_Type type,
+    base::Optional<std::int32_t> api_error_code = base::nullopt);
+
+}  // namespace protocol
+}  // namespace remoting
+
+#endif  // REMOTING_PROTOCOL_FILE_TRANSFER_HELPERS_H_