Add CDM FileIO tests.

media/cdm/ppapi/cdm_file_io_impl.cc

Index: media/cdm/ppapi/cdm_file_io_impl.cc
diff --git a/media/cdm/ppapi/cdm_file_io_impl.cc b/media/cdm/ppapi/cdm_file_io_impl.cc
new file mode 100644
index 0000000000000000000000000000000000000000..66429499b5d52371ec24e93103271cc28fd25aa8
--- /dev/null
+++ b/media/cdm/ppapi/cdm_file_io_impl.cc
@@ -0,0 +1,317 @@
+// Copyright 2013 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 "media/cdm/ppapi/cdm_file_io_impl.h"
+
+#include <algorithm>
+#include <sstream>
+
+#include "base/logging.h"
+#include "ppapi/c/pp_errors.h"
+
+namespace media {
+
+const int kReadSize = 1024;  // Arbitrary choice.
+
+// Call func_call and check the result. If the result is not
+// PP_OK_COMPLETIONPENDING, print out logs, call OnError() and return.
+#define CHECK_PP_OK_COMPLETIONPENDING(func_call, error_type)            \
+  do {                                                                  \
+    int32_t result = func_call;                                         \

[ddorwin, 2013/12/04 05:27:08]

PP_DCHECK(result != PP_OK)?

[xhwang, 2013/12/10 01:24:25]

Done.

+    if (result != PP_OK_COMPLETIONPENDING) {                            \
+      std::ostringstream error_message;                                 \
+      error_message << #func_call << " failed with result: " << result; \

[ddorwin, 2013/12/04 05:27:08]

All these strings will get included in prod binaries. I don't think we can do
that.

[xhwang, 2013/12/10 01:24:25]

Done.

+      OnError((error_type), (error_message).str());                     \
+      return;                                                           \
+    }                                                                   \
+  } while (0)
+
+// PPAPI calls should only be made on the main thread. In this file, main thread
+// checking is only performed in public APIs and the completion callback
+// (HandleCompletedTask). This ensures all functions are running on the main
+// thread since internal methods are called either by the public APIs or by the
+// completion callback.
+static bool IsMainThread() {
+  return pp::Module::Get()->core()->IsMainThread();
+}
+
+// Posts a task to run |cb| on the main thread. The task is posted even if the
+// current thread is the main thread.
+static void PostOnMain(pp::CompletionCallback cb) {
+  pp::Module::Get()->core()->CallOnMainThread(0, cb, PP_OK);
+}
+
+CdmFileIOImpl::CdmFileIOImpl(cdm::CdmFileIOClient* client,
+                             PP_Instance pp_instance)

[ddorwin, 2013/12/04 05:27:08]

fits?

[xhwang, 2013/12/10 01:24:25]

Done.

+    : state_(FILE_CLOSED),

[ddorwin, 2013/12/04 05:27:08]

Kind of odd to start out in closed, esp. since the API doesn't support
open->close->open.

[xhwang, 2013/12/10 01:24:25]

Done. Added FILE_UNOPENED.

+      client_(client),
+      pp_instance_handle_(pp_instance),
+      callback_factory_(this),
+      file_offset_(0) {
+  PP_DCHECK(pp_instance);  // 0 indicates a "NULL handle".
+}
+
+CdmFileIOImpl::~CdmFileIOImpl() {
+  if (state_ != FILE_CLOSED)
+    CloseFile();
+}
+
+// Call sequence: Open() -> OpenFileSystem() -> OpenFile() -> FILE_OPENED.
+void CdmFileIOImpl::Open(const char* file_name, uint32_t file_name_size) {
+  DVLOG(3) << __FUNCTION__;
+  PP_DCHECK(IsMainThread());
+
+  if (state_ != FILE_CLOSED) {
+    OnError(OPEN_ERROR, "Open() called in an invalid state.");
+    return;
+  }
+
+  state_ = OPENING_FILE_SYSTEM;
+
+  // File name should not contain any path separators.
+  std::string file_name_str(file_name, file_name_size);
+  if (file_name_str.find('/') != std::string::npos ||
+      file_name_str.find('\\') != std::string::npos) {
+    OnError(OPEN_ERROR, "Invalid file name.");
+    return;
+  }
+
+  // pp::FileRef only accepts path that begins with a '/' character.
+  file_name_ = '/';
+  file_name_ += file_name_str;

[ddorwin, 2013/12/04 05:27:08]

Can we merge with above line?

[xhwang, 2013/12/10 01:24:25]

Done.

+
+  OpenFileSystem();
+}
+
+// Call sequence:
+//                                       finished
+// Read() -> ReadFile() -> OnFileRead() ----------> Done.
+//               ^              |
+//               |              | not finished

[ddorwin, 2013/12/04 05:27:08]

This is a bit confusing. I guess because of the difference in layout with
"finished". Maybe put this inside the pipes on line 92 then remove this line?

[xhwang, 2013/12/10 01:24:25]

Done.

+//               |--------------|
+void CdmFileIOImpl::Read() {
+  DVLOG(3) << __FUNCTION__;
+  PP_DCHECK(IsMainThread());
+
+  if (state_ != FILE_OPENED) {
+    OnError(READ_ERROR, "Read() called in an invalid state.");
+    return;
+  }
+
+  PP_DCHECK(buffer_.empty());
+  PP_DCHECK(read_buffer_.empty());
+
+  buffer_.resize(kReadSize);
+  file_offset_ = 0;
+
+  state_ = READING_FILE;
+  ReadFile();
+}
+
+// Call sequence:
+//                                            finished
+// Write() -> WriteFile() -> OnFileWritten() ----------> Done.
+//                ^                  |
+//                |                  | not finished
+//                |------------------|
+void CdmFileIOImpl::Write(const uint8_t* data, uint32_t data_size) {
+  DVLOG(3) << __FUNCTION__;
+  PP_DCHECK(IsMainThread());
+
+  if (state_ != FILE_OPENED) {
+    OnError(WRITE_ERROR, "Write() called in an invalid state.");
+    return;
+  }
+
+  PP_DCHECK(state_ == FILE_OPENED);

[ddorwin, 2013/12/04 05:27:08]

duplicate with 122?

[xhwang, 2013/12/10 01:24:25]

Done.

+  // TODO(xhwang): Support NULL |data|.
+  PP_DCHECK(data);
+
+  buffer_.assign(data, data + data_size);
+  file_offset_ = 0;
+
+  state_ = WRITING_FILE;
+  WriteFile();
+}
+
+void CdmFileIOImpl::Close() {
+  if (state_ != FILE_CLOSED)
+    CloseFile();
+  delete this;
+}
+
+void CdmFileIOImpl::OpenFileSystem() {
+  DVLOG(3) << __FUNCTION__;
+  PP_DCHECK(state_ == OPENING_FILE_SYSTEM);
+
+  pp::CompletionCallbackWithOutput<pp::FileSystem> cb =
+      callback_factory_.NewCallbackWithOutput(&CdmFileIOImpl::OnFileSystemOpen);
+  pp::IsolatedFileSystemPrivate isolated_file_system(
+      pp_instance_handle_, PP_ISOLATEDFILESYSTEMTYPE_PRIVATE_PLUGINPRIVATE);
+
+  CHECK_PP_OK_COMPLETIONPENDING(isolated_file_system.Open(cb), OPEN_ERROR);

[ddorwin, 2013/12/04 05:27:08]

Is it okay that isolated_file_system will go out of scope during the
asynchronous call?

[xhwang, 2013/12/10 01:24:25]

Done.

+}
+
+void CdmFileIOImpl::OnFileSystemOpen(int32_t result,
+                                     pp::FileSystem file_system) {

[ddorwin, 2013/12/04 05:27:08]

fits

[xhwang, 2013/12/10 01:24:25]

doesn't fit now

+  DVLOG(3) << __FUNCTION__;

[ddorwin, 2013/12/04 05:27:08]

PP_DCHECK(IsMainThread());? Need to check callbacks per comment, right?

[xhwang, 2013/12/10 01:24:25]

Done.

+  PP_DCHECK(state_ == OPENING_FILE_SYSTEM);
+  if (result != PP_OK) {
+    OnError(OPEN_ERROR, "File system open failed asynchronously.");
+    return;
+  }
+
+  file_system_ = file_system;
+  state_ = OPENING_FILE;
+  OpenFile();
+}
+
+void CdmFileIOImpl::OpenFile() {
+  DVLOG(3) << __FUNCTION__;
+  PP_DCHECK(state_ == OPENING_FILE);
+
+  file_io_ = pp::FileIO(pp_instance_handle_);
+  file_ref_ = pp::FileRef(file_system_, file_name_.c_str());
+  // TODO(xhwang): This is not safe when we have multiple instances trying to

[ddorwin, 2013/12/11 21:16:16]

What did you do that made this safe in the new patch set?

+  // access the same file.
+  int32_t file_open_flag = PP_FILEOPENFLAG_READ |
+                           PP_FILEOPENFLAG_WRITE |
+                           PP_FILEOPENFLAG_CREATE;
+  pp::CompletionCallback cb =
+      callback_factory_.NewCallback(&CdmFileIOImpl::OnFileOpen);
+  CHECK_PP_OK_COMPLETIONPENDING(file_io_.Open(file_ref_, file_open_flag, cb),
+                                OPEN_ERROR);
+}
+
+void CdmFileIOImpl::OnFileOpen(int32_t result) {
+  PP_DCHECK(IsMainThread());
+  PP_DCHECK(state_ == OPENING_FILE);
+
+  if (result != PP_OK) {
+    OnError(OPEN_ERROR, "File open failed.");
+    return;
+  }
+
+  state_ = FILE_OPENED;
+  client_->OnOpenComplete(cdm::CdmFileIOClient::kSuccess);
+}
+
+void CdmFileIOImpl::ReadFile() {
+  PP_DCHECK(state_ == READING_FILE);
+  PP_DCHECK(!buffer_.empty());
+
+  pp::CompletionCallback cb =
+      callback_factory_.NewCallback(&CdmFileIOImpl::OnFileRead);
+  CHECK_PP_OK_COMPLETIONPENDING(
+      file_io_.Read(file_offset_, &buffer_[0], buffer_.size(), cb), READ_ERROR);
+}
+
+void CdmFileIOImpl::OnFileRead(int32_t bytes_read) {
+  PP_DCHECK(state_ == READING_FILE);

[ddorwin, 2013/12/04 05:27:08]

PP_DCHECK(IsMainThread());?

[xhwang, 2013/12/10 01:24:25]

Done.

+
+  // 0 |bytes_read| indicates end-of-file reached.
+  if (bytes_read < PP_OK) {

[ddorwin, 2013/12/04 05:27:08]

|bytes_read| is a |result|?

[xhwang, 2013/12/10 01:24:25]

yes

+    OnError(READ_ERROR, "Read file failed.");
+    return;
+  }
+
+  PP_DCHECK(!buffer_.empty());

[ddorwin, 2013/12/04 05:27:08]

This could be at 212, right? Might be unnecessary given the next line, though.

[xhwang, 2013/12/10 01:24:25]

Done.

+  PP_DCHECK(static_cast<size_t>(bytes_read) <= buffer_.size());
+  // Append |bytes_read| in |buffer_| to the end of |read_buffer_|.
+  read_buffer_.insert(read_buffer_.end(),
+                      buffer_.begin(), buffer_.begin() + bytes_read);
+
+  buffer_.erase(buffer_.begin(), buffer_.begin() + bytes_read);

[ddorwin, 2013/12/04 05:27:08]

This seems unnecessarily inefficient. Can we just clear or something?
Won't we need to resize before reading into it again?

[xhwang, 2013/12/10 01:24:25]

Done. Reuse the read buffer and this is not needed anymore.

+  file_offset_ += bytes_read;
+
+  // Still have data to read and we have not received end-of-file yet.

[ddorwin, 2013/12/04 05:27:08]

With all the complexity, we should probably include >kReadSize file accesses in
the test.

[xhwang, 2013/12/10 01:24:25]

I'll add more tests later.

+  if (!buffer_.empty() && bytes_read > 0) {

[ddorwin, 2013/12/04 05:27:08]

Why wouldn't buffer_ be empty here? Shouldn't it be? (I suppose a read could be
partial, but in most cases where we need multiple reads, we would have erased
the entire buffer.)

[xhwang, 2013/12/10 01:24:25]

For example, we ask to read 100 bytes, the only 50 bytes were read. In this
case, buffer.size() is 50.

That being said, we need to read until EOF. So this condition needs to be
removed.

+    ReadFile();
+    return;
+  }
+
+  // All requested data read or we hit end-of-file. Return read data to the
+  // client.
+  buffer_.clear();
+  file_offset_ = 0;
+  // Clear |read_buffer_| in case OnReadComplete() calls Read() or Write().
+  std::vector<char> local_buffer;
+  std::swap(read_buffer_, local_buffer);
+
+  state_ = FILE_OPENED;
+  client_->OnReadComplete(cdm::CdmFileIOClient::kSuccess,
+                          reinterpret_cast<const uint8_t*>(&local_buffer[0]),
+                          local_buffer.size());
+}
+
+void CdmFileIOImpl::WriteFile() {
+  PP_DCHECK(state_ == WRITING_FILE);
+  PP_DCHECK(!buffer_.empty());
+
+  pp::CompletionCallback cb =
+      callback_factory_.NewCallback(&CdmFileIOImpl::OnFileWritten);
+  CHECK_PP_OK_COMPLETIONPENDING(
+      file_io_.Write(file_offset_, &buffer_[0], buffer_.size(), cb),
+      WRITE_ERROR);
+}
+
+void CdmFileIOImpl::OnFileWritten(int32_t bytes_written) {
+  PP_DCHECK(state_ == WRITING_FILE);

[ddorwin, 2013/12/04 05:27:08]

PP_DCHECK(IsMainThread());?

[xhwang, 2013/12/10 01:24:25]

Done.

+
+  if (bytes_written <= PP_OK) {
+    OnError(READ_ERROR, "Write file failed.");
+    return;
+  }
+
+  PP_DCHECK(!buffer_.empty());
+  PP_DCHECK(static_cast<size_t>(bytes_written) <= buffer_.size());
+  buffer_.erase(buffer_.begin(), buffer_.begin() + bytes_written);

[ddorwin, 2013/12/04 05:27:08]

Same issues as read.
Wouldn't it be easier to just use the file_offset_ as the index to 255 (and
subtract from the size)?

[xhwang, 2013/12/10 01:24:25]

Done.

+  file_offset_ += bytes_written;
+
+  if (!buffer_.empty()) {
+    WriteFile();

[ddorwin, 2013/12/04 05:27:08]

Just to be clear, this probably won't happen, right?

[xhwang, 2013/12/10 01:24:25]

This happens when only part of the data were written. From FileIO's perspective,
this could happen.

+    return;
+  }
+
+  file_offset_ = 0;
+  state_ = FILE_OPENED;
+  client_->OnWriteComplete(cdm::CdmFileIOClient::kSuccess);
+}
+
+void CdmFileIOImpl::CloseFile() {
+  DVLOG(3) << __FUNCTION__;
+  PP_DCHECK(IsMainThread());
+
+  state_ = FILE_CLOSED;
+
+  file_io_.Close();
+  buffer_.clear();
+  file_offset_ = 0;
+  read_buffer_.clear();
+}
+
+void CdmFileIOImpl::OnError(ErrorType error_type,
+                            const std::string& error_msg) {
+  PostOnMain(callback_factory_.NewCallback(
+      &CdmFileIOImpl::NotifyClientOfError, error_type, error_msg));
+}
+
+void CdmFileIOImpl::NotifyClientOfError(int32_t /* result */,

[ddorwin, 2013/12/04 05:27:08]

DCHECK(result == PP_OK)?

[xhwang, 2013/12/10 01:24:25]

Done.

+                                        ErrorType error_type,
+                                        const std::string& error_msg) {
+  DVLOG(1) << error_msg;
+  switch (error_type) {

[ddorwin, 2013/12/04 05:27:08]

I'm not wild about this conversion, but I can't think of a better way.

[xhwang, 2013/12/10 01:24:25]

Done.

+    case OPEN_ERROR:
+      client_->OnOpenComplete(cdm::CdmFileIOClient::kError);
+      break;
+    case READ_ERROR:
+      client_->OnReadComplete(cdm::CdmFileIOClient::kError, NULL, 0);
+      break;
+    case WRITE_ERROR:
+      client_->OnWriteComplete(cdm::CdmFileIOClient::kError);
+      break;
+  }
+}
+
+}  // namespace media