mediaview: Implement ArcDocumentsProviderBackendDelegate.

chrome/browser/chromeos/arc/fileapi/arc_documents_provider_file_stream_reader.cc

+// Copyright 2016 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 "chrome/browser/chromeos/arc/fileapi/arc_documents_provider_file_stream_reader.h"
+
+#include "base/bind.h"
+#include "base/memory/ptr_util.h"
+#include "chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.h"
+#include "content/public/browser/browser_thread.h"
+#include "net/base/net_errors.h"
+#include "url/gurl.h"
+
+using content::BrowserThread;
+
+namespace arc {
+
+ArcDocumentsProviderFileStreamReader::ArcDocumentsProviderFileStreamReader(
+    int64_t offset)
+    : offset_(offset), resolved_(false), weak_ptr_factory_(this) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+}
+
+ArcDocumentsProviderFileStreamReader::~ArcDocumentsProviderFileStreamReader() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+}
+
+void ArcDocumentsProviderFileStreamReader::SetContentUrl(
+    const GURL& content_url) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK(!resolved_);
+
+  if (content_url.is_valid()) {
+    underlying_reader_ = base::MakeUnique<ArcContentFileSystemFileStreamReader>(
+        content_url, offset_);
+  }
+  resolved_ = true;
+
+  for (const base::Closure& callback : pending_operations_) {
+    callback.Run();
+  }
+  pending_operations_.clear();

[hashimoto, 2017/01/05 09:21:22]

How about std::move'ing pending_operations_ to a temporary variable before the
for loop, instead of calling clear() here?
This way, you don't have to worry about what happens when pending_operations_
gets modified as a result of running callbacks (it doesn't happen with the
current code, but someone may carelessly modify this file in future).

[Shuhei Takahashi, 2017/01/05 11:08:28]

Well, since |resolved_| = true at this moment, further elements are never added
to |pending_operations_|. But anyway it sounds okay to me to use a temporary
variable here.

Note that I did use .swap() instead of std::move() because std::move() puts a
vector valid but unspecified state, thus usually should be used for
soon-expiring objects.

+}
+
+base::WeakPtr<ArcDocumentsProviderFileStreamReader>
+ArcDocumentsProviderFileStreamReader::GetWeakPtr() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  return weak_ptr_factory_.GetWeakPtr();
+}
+
+int ArcDocumentsProviderFileStreamReader::Read(
+    net::IOBuffer* buffer,
+    int buffer_length,
+    const net::CompletionCallback& callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  if (!resolved_) {
+    pending_operations_.emplace_back(base::Bind(
+        base::IgnoreResult(&ArcDocumentsProviderFileStreamReader::Read),

[hashimoto, 2017/01/05 09:21:22]

IgnoreResult here feels a bit dangerous to me.
ATM ArcDocumentsProviderFileStreamReader::Read() doesn't return values other
than ERR_IO_PENDING, but it's not guaranteed to be true for future.
The same goes for GetLength().

[Shuhei Takahashi, 2017/01/05 11:08:28]

True. I added proper handling.

+        weak_ptr_factory_.GetWeakPtr(), buffer, buffer_length, callback));

[hashimoto, 2017/01/05 09:21:22]

Without holding a refptr, this buffer may die before
ArcDocumentsProviderFileStreamReader::Read gets called.

[Shuhei Takahashi, 2017/01/05 11:08:28]

Good catch!

+    return net::ERR_IO_PENDING;
+  }
+  if (!underlying_reader_)
+    return net::ERR_FILE_NOT_FOUND;
+  return underlying_reader_->Read(buffer, buffer_length, callback);
+}
+
+int64_t ArcDocumentsProviderFileStreamReader::GetLength(
+    const net::Int64CompletionCallback& callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  if (!resolved_) {
+    pending_operations_.emplace_back(base::Bind(
+        base::IgnoreResult(&ArcDocumentsProviderFileStreamReader::GetLength),
+        weak_ptr_factory_.GetWeakPtr(), callback));
+    return net::ERR_IO_PENDING;
+  }
+  if (!underlying_reader_)
+    return net::ERR_FILE_NOT_FOUND;
+  return underlying_reader_->GetLength(callback);
+}
+
+}  // namespace arc