Files.app: Implement md5 hashing over file streams.

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

 // 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 "chrome/browser/chromeos/extensions/file_manager/private_api_file_system.h"
 
 #include <sys/statvfs.h>
 #include <set>
 
 #include "base/posix/eintr_wrapper.h"
@@ skipping 16 matching lines @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/common/extensions/api/file_manager_private.h"
 #include "chrome/common/extensions/api/file_manager_private_internal.h"
 #include "chromeos/disks/disk_mount_manager.h"
 #include "content/public/browser/child_process_security_policy.h"
 #include "content/public/browser/render_process_host.h"
 #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"
 #include "storage/browser/fileapi/file_system_operation_runner.h"
 #include "storage/browser/fileapi/file_system_url.h"
 #include "storage/common/fileapi/file_system_info.h"
 #include "storage/common/fileapi/file_system_types.h"
 #include "storage/common/fileapi/file_system_util.h"
 
 using chromeos::disks::DiskMountManager;
@@ skipping 665 matching lines @@
       ResolveIsolatedEntries::Results::Create(entries);
   SendResponse(true);
 }
 
 bool FileManagerPrivateComputeChecksumFunction::RunAsync() {
   using extensions::api::file_manager_private::ComputeChecksum::Params;
   const scoped_ptr<Params> params(Params::Create(*args_));
   EXTENSION_FUNCTION_VALIDATE(params);
 
   if (params->file_url.empty()) {
-    // TODO(kenobi): call SetError()
+    SetError("File URL must be provided");
     return false;
   }
 
   scoped_refptr<storage::FileSystemContext> file_system_context =
       file_manager::util::GetFileSystemContextForRenderViewHost(
           GetProfile(), render_view_host());
 
   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);
 }
 
 bool FileManagerPrivateSearchFilesByHashesFunction::RunAsync() {
   using api::file_manager_private::SearchFilesByHashes::Params;
   const scoped_ptr<Params> params(Params::Create(*args_));
   EXTENSION_FUNCTION_VALIDATE(params);
 
   // TODO(hirono): Check the volume ID and fail the function for volumes other
@@ skipping 47 matching lines @@
     result->GetListWithoutPathExpansion(hashAndPath.hash, &list);
     list->AppendString(
         file_manager::util::ConvertDrivePathToFileSystemUrl(
             GetProfile(), hashAndPath.path, extension_id()).spec());
   }
   SetResult(result.release());
   SendResponse(true);
 }
 
 }  // namespace extensions