JavaScript Content Handler Version 0.0

mojo/common/data_pipe_utils.cc

 // Copyright 2014 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 "mojo/common/data_pipe_utils.h"
 
 #include <stdio.h>
 
 #include "base/file_util.h"
 #include "base/files/file_path.h"
 #include "base/files/scoped_file.h"
 #include "base/message_loop/message_loop.h"
 #include "base/task_runner_util.h"
 
 namespace mojo {
 namespace common {
+namespace {
 
-bool BlockingCopyToFile(ScopedDataPipeConsumerHandle source,
-                        const base::FilePath& destination) {
-  base::ScopedFILE fp(base::OpenFile(destination, "wb"));
-  if (!fp)
-    return false;
-
+bool BlockingCopyHelper(ScopedDataPipeConsumerHandle source,

[Aaron Boodman, 2014/08/25 17:25:37]

I think the callback-based refactor makes sense, but you should get Darin's
opinion too.

+    const base::Callback<size_t(const void*, uint32_t)>& write_bytes) {
   for (;;) {
     const void* buffer;
     uint32_t num_bytes;
-    MojoResult result = BeginReadDataRaw(source.get(), &buffer, &num_bytes,
-                                         MOJO_READ_DATA_FLAG_NONE);
+    MojoResult result = BeginReadDataRaw(
+        source.get(), &buffer, &num_bytes, MOJO_READ_DATA_FLAG_NONE);
     if (result == MOJO_RESULT_OK) {
-      size_t bytes_written = fwrite(buffer, 1, num_bytes, fp.get());
+      size_t bytes_written = write_bytes.Run(buffer, num_bytes);
       result = EndReadDataRaw(source.get(), num_bytes);
       if (bytes_written < num_bytes || result != MOJO_RESULT_OK)
         return false;
     } else if (result == MOJO_RESULT_SHOULD_WAIT) {
       result = Wait(source.get(),
                     MOJO_HANDLE_SIGNAL_READABLE,
                     MOJO_DEADLINE_INDEFINITE);
       if (result != MOJO_RESULT_OK) {
         // If the producer handle was closed, then treat as EOF.
         return result == MOJO_RESULT_FAILED_PRECONDITION;
       }
     } else if (result == MOJO_RESULT_FAILED_PRECONDITION) {
       // If the producer handle was closed, then treat as EOF.
       return true;
     } else {
       // Some other error occurred.
       break;
     }
   }
 
   return false;
 }
 
+size_t CopyToStringHelper(

[darin (slow to review), 2014/08/26 05:23:22]

nit: Append{Bytes}ToString ?

[hansmuller, 2014/08/26 23:39:12]

I thought there was some value in repeating most of the name of the function
being helped; so CopyToStringHelper() goes with BlockingCopyToString(). If you
think AppendBytesToString() is clearer, I'll be happy to change it. Likewise for
CopyToFileHelper().

+    std::string* result, const void* buffer, uint32_t num_bytes) {
+  if (result)
+      result->append(static_cast<const char*>(buffer), num_bytes);
+  return num_bytes;
+}
+
+size_t CopyToFileHelper(FILE* fp, const void* buffer, uint32_t num_bytes) {

[darin (slow to review), 2014/08/26 05:23:23]

nit: Append{Bytes}ToFile ?

[hansmuller, 2014/08/26 23:39:12]

[see previous comment]

+  return fwrite(buffer, 1, num_bytes, fp);
+}
+
+} // namespace
+
+
+// Similar to base::ReadFileToString().
+// TODO(hansmuller): Add a max_size parameter.
+bool BlockingCopyToString(ScopedDataPipeConsumerHandle source,
+                          std::string* result) {
+  if (result)

[Aaron Boodman, 2014/08/25 17:25:38]

Is there a reason to not pass |result|, or is this defensive programming? If it
is defensive, don't be... just let it crash. Better to crash than carry on in a
weird state.

http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-

[hansmuller, 2014/08/26 23:39:12]

I was emulating  base::ReadFileToString() which also uses a NULL return
parameter to indicate that the input is to be read and discarded. I added a
comment to the header file to explain the function's semantics a little better.

[Aaron Boodman, 2014/08/27 05:55:53]

OK, that functions gives the example of priming a disk cache as a reason you
might want to read and discard. Would that example apply here too? I guess...
Are there any better examples for why you might want to do this?

It seems a little unfortunate to me to have this bear trap here for such a teeny
potential use case. Crashing on NULL seems like what you want to happen most of
the time.

But... I defer to Darin.

+    result->clear();
+  return BlockingCopyHelper(
+      source.Pass(), base::Bind(&CopyToStringHelper, result));
+}
+
+bool BlockingCopyToFile(ScopedDataPipeConsumerHandle source,
+                        const base::FilePath& destination) {
+  base::ScopedFILE fp(base::OpenFile(destination, "wb"));
+  if (!fp)
+    return false;
+  return BlockingCopyHelper(
+      source.Pass(), base::Bind(&CopyToFileHelper, fp.get()));
+}
+
+// TODO(hansmuller): Should this be deleted?
 void CompleteBlockingCopyToFile(const base::Callback<void(bool)>& callback,

[Aaron Boodman, 2014/08/25 17:25:38]

Yeah, looks like dead code.

[hansmuller, 2014/08/26 23:39:12]

OK, I deleted it.

                                 bool result) {
   callback.Run(result);
 }
 
 void CopyToFile(ScopedDataPipeConsumerHandle source,
                 const base::FilePath& destination,
                 base::TaskRunner* task_runner,
                 const base::Callback<void(bool)>& callback) {
   base::PostTaskAndReplyWithResult(
       task_runner,
       FROM_HERE,
       base::Bind(&BlockingCopyToFile, base::Passed(&source), destination),
       callback);
 }
 
 }  // namespace common
 }  // namespace mojo