Files.app: Implement md5 hashing over file streams.

chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc

Index: chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc
diff --git a/chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc b/chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc
index b7b52fac6cfea2125c6d5f1404114165293f3e4c..bcf2034bf8b441dc58efb9b21514c24b4bb9c53f 100644
--- a/chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc
+++ b/chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc
@@ -34,6 +34,7 @@
 #include "content/public/browser/render_view_host.h"
 #include "content/public/common/url_constants.h"
 #include "net/base/escape.h"
+#include "storage/browser/blob/file_stream_reader.h"
 #include "storage/browser/fileapi/file_system_context.h"
 #include "storage/browser/fileapi/file_system_file_util.h"
 #include "storage/browser/fileapi/file_system_operation_context.h"
@@ -719,7 +720,7 @@ bool FileManagerPrivateComputeChecksumFunction::RunAsync() {
   EXTENSION_FUNCTION_VALIDATE(params);
 
   if (params->file_url.empty()) {
-    // TODO(kenobi): call SetError()
+    SetError("File URL must be provided");
     return false;
   }
 
@@ -730,20 +731,39 @@ bool FileManagerPrivateComputeChecksumFunction::RunAsync() {
   storage::FileSystemURL file_url(
       file_system_context->CrackURL(GURL(params->file_url)));
   if (!file_url.is_valid()) {
-    // TODO(kenobi): Call SetError()
+    SetError("File URL was invalid");
     return false;
   }
 
-  BrowserThread::PostTaskAndReplyWithResult(
-      BrowserThread::FILE, FROM_HERE,
-      base::Bind(&drive::util::GetMd5Digest, file_url.path()),
-      base::Bind(&FileManagerPrivateComputeChecksumFunction::Respond, this));
+  scoped_ptr<storage::FileStreamReader> reader =
+      file_system_context->CreateFileStreamReader(
+          file_url, 0, storage::kMaximumLength, base::Time());
+
+  scoped_refptr<drive::util::FileStreamMd5Digester> digester(

[mtomasz, 2015/02/02 23:47:07]

You don't need to make it a ref counter class. We can keep it normal, and do
base::Bind(&A::b, base::Unretained(b), ..., callback) if the "callback" outlives
the "b" object. It is this case, as FileManagerPrivateComputeChecksumFunction is
already ref counted.

In general we try to avoid ref counted classes if not necessary in Chromium.

http://www.chromium.org/developers/smart-pointer-guidelines

[Ben Kwa, 2015/02/03 06:28:54]

So, you're suggesting making the FileStreamMd5Digester a member of the
FileManagerPrivateComputeChecksumFunction?  Otherwise it goes out of scope
before the bound closure gets executed.  I figured it was cleaner to just
construct it here and have it live only as long as necessary, but I can do the
member thing if you prefer that.

[mtomasz, 2015/02/03 10:36:53]

We use members for that, as the FileManagerPrivateComputeChecksumFunction
instance is created per call.

[Ben Kwa, 2015/02/03 16:32:19]

OK, done.

+      new drive::util::FileStreamMd5Digester());
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&drive::util::FileStreamMd5Digester::GetMd5Digest, digester,
+                 base::Passed(&reader),
+                 base::Bind(&FileManagerPrivateComputeChecksumFunction::Respond,
+                            this)));
 
   return true;
 }
 
 void FileManagerPrivateComputeChecksumFunction::Respond(
     const std::string& hash) {
+  if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {

[mtomasz, 2015/02/02 23:47:07]

This is a little bit hacky. Each method should be called on one thread, or any
thread. I'd suggest to use PostTaskAndReply* which would call the callback on
the same thread as caller.

For reference see
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/uWGNk-hVlGc/dis...

[Ben Kwa, 2015/02/03 06:28:54]

I don't think I can use PostTaskAndReply, because the MD5 hashing occurs
asynchronously.  Is there some utility for waiting on the async reads and
returning synchronously?  I don't know that that's really the best solution
anyway - it seems cleaner to me to write code that accounts for the asynchronous
nature of what's happening.  Would it satisfy you to just take out this if
statement and just always explicitly specify which function is occurring on
which thread?

[mtomasz, 2015/02/03 10:36:53]

You're right. I'd suggest to repost then in an anonymous namespace.

Eg.

void OnMd5ChecksumOnIOThread(const std::string& checksum, Callback callback) {
  PostTask(UI, FROM_HERE, checksum, callback);
}

or sth like that, then:

PostTask(IO, FROM_HERE, base::Bind(&Digester::GetMd5Digest,
base::Unretained(digester_), base::Bind(&OnMd5ChecksumOnIOThread,
base::Bind(&FileManagerPrivateChecksumFunction::Respond, this)));

It's a pretty common pattern in Chromium.

[Ben Kwa, 2015/02/03 16:32:19]

Done.

+    // If not called from the UI thread, repost to the UI thread.  The only
+    // other thread that this should be getting called from is the IO thread.
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    BrowserThread::PostTask(
+        BrowserThread::UI, FROM_HERE,
+        base::Bind(&FileManagerPrivateComputeChecksumFunction::Respond, this,
+                   hash));
+    return;
+  }
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   SetResult(new base::StringValue(hash));
   SendResponse(true);
 }