JavaScript Content Handler Version 0.0

mojo/common/data_pipe_utils.cc

Index: mojo/common/data_pipe_utils.cc
diff --git a/mojo/common/data_pipe_utils.cc b/mojo/common/data_pipe_utils.cc
index 2a6b21e6d4824ae4be3a0391d7a931beeafe7709..98d5e0d9049aae80d7c28c1317631afc48c4d2af 100644
--- a/mojo/common/data_pipe_utils.cc
+++ b/mojo/common/data_pipe_utils.cc
@@ -14,20 +14,17 @@
 
 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;
@@ -51,6 +48,40 @@ bool BlockingCopyToFile(ScopedDataPipeConsumerHandle source,
   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);