Expose computation of md5 content checksums for files via a file manager private API.

Expose computation of md5 content checksums for files via a file manager private API.

This simply takes the existing drive code for computing content hashes, and exposes it as part of the file manager private API.  The existence of this code is a temporary shim to tide us over until issue 446863 is addressed.

BUG=420680
TEST=browser_tests: FileManagerPrivateApiTest.*

Committed: https://crrev.com/32e0c3fc3ca56027343838365e742e396a8a0fd2
Cr-Commit-Position: refs/heads/master@{#310782}

[Ben Kwa, 2015-01-07 19:53:07]

kenobi@chromium.org changed reviewers:
+ mtomasz@chromium.org

[Ben Kwa, 2015-01-07 19:53:08]

Just for an initial look-over.  If this basically looks good to you, I will send
to other owners for review.

Thanks!

[mtomasz, 2015-01-08 00:38:40]

In general looks good.

https://codereview.chromium.org/840843002/diff/1/chrome/browser/chromeos/exte...
File
chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc
(right):

https://codereview.chromium.org/840843002/diff/1/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:390:
bool InitializeLocalFileSystem(std::string mount_point_name,
This can go to an anonymous namespace.

https://codereview.chromium.org/840843002/diff/1/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:391:
base::ScopedTempDir *tmp_dir,
nit: tmp_dir -> temp_dir for consistency with the type name.

https://codereview.chromium.org/840843002/diff/1/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:402:
base::FilePath test_dir = mount_point_dir->AppendASCII("test_dir");
nit: This can be const

https://codereview.chromium.org/840843002/diff/1/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:406:
base::FilePath test_file = test_dir.AppendASCII("test_file.txt");
nit: ditto

https://codereview.chromium.org/840843002/diff/1/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:433:
ASSERT_TRUE(RunExtensionTestIgnoreManifestWarnings(
Is RunComponentExtensionTest enough? We don't expect any warnings.

https://codereview.chromium.org/840843002/diff/1/chrome/browser/chromeos/exte...
File chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc
(right):

https://codereview.chromium.org/840843002/diff/1/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:735:
const std::string& md5 = drive::util::GetMd5Digest(file_url.path());
This utility makes file IO, it must not be called on UI thread. Please do
PostTask to FILE thread *or* use a blocking pool.

See:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...

https://codereview.chromium.org/840843002/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/file_browser/content_checksum_test/test.js
(right):

https://codereview.chromium.org/840843002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_browser/content_checksum_test/test.js:1:
// Copyright (c) 2013 The Chromium Authors. All rights reserved.
(c) 2013 -> 2014

https://codereview.chromium.org/840843002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_browser/content_checksum_test/test.js:15:
function initTests(callback) {
I think this can be greatly simplified. If we only care about the file system
with testing volume, not Drive.
We could simply iterate over the volume metadata list and request the file
system which has 'testing' substring. No need to sort, collect any tests, etc.

[Ben Kwa, 2015-01-08 19:29:13]

kenobi@chromium.org changed reviewers:
+ asvitkine@chromium.org, rockot@chromium.org

[Ben Kwa, 2015-01-08 19:29:14]

rockot@ for
    chrome/common/extensions/api/file_manager_private.idl

asvitkine for
    extensions/browser/extension_function_histogram_value.h
    tools/metrics/histograms/histograms.xml

mtomasz for everything else.

Thanks!

https://codereview.chromium.org/840843002/diff/1/chrome/browser/chromeos/exte...
File
chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc
(right):

https://codereview.chromium.org/840843002/diff/1/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:390:
bool InitializeLocalFileSystem(std::string mount_point_name,
On 2015/01/08 00:38:40, mtomasz wrote:
> This can go to an anonymous namespace.

Done.

https://codereview.chromium.org/840843002/diff/1/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:391:
base::ScopedTempDir *tmp_dir,
On 2015/01/08 00:38:40, mtomasz wrote:
> nit: tmp_dir -> temp_dir for consistency with the type name.

Done.

https://codereview.chromium.org/840843002/diff/1/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:402:
base::FilePath test_dir = mount_point_dir->AppendASCII("test_dir");
On 2015/01/08 00:38:40, mtomasz wrote:
> nit: This can be const

Done.

https://codereview.chromium.org/840843002/diff/1/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:406:
base::FilePath test_file = test_dir.AppendASCII("test_file.txt");
On 2015/01/08 00:38:40, mtomasz wrote:
> nit: ditto

Done.

https://codereview.chromium.org/840843002/diff/1/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:433:
ASSERT_TRUE(RunExtensionTestIgnoreManifestWarnings(
On 2015/01/08 00:38:40, mtomasz wrote:
> Is RunComponentExtensionTest enough? We don't expect any warnings.

Done.

https://codereview.chromium.org/840843002/diff/1/chrome/browser/chromeos/exte...
File chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc
(right):

https://codereview.chromium.org/840843002/diff/1/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:735:
const std::string& md5 = drive::util::GetMd5Digest(file_url.path());
On 2015/01/08 00:38:40, mtomasz wrote:
> This utility makes file IO, it must not be called on UI thread. Please do
> PostTask to FILE thread *or* use a blocking pool.
> 
> See:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...

Interesting, the doc for AsyncExtensionFunction made it sound like it runs
asynchronously w.r.t. the UI thread, which I took to mean it didn't run on the
UI thread.  So, what does it mean to say these run "asynchronously relative to
the browser's UI thread"?

I changed it to use PostTaskAndReplyWithResult.

https://codereview.chromium.org/840843002/diff/1/chrome/test/data/extensions/...
File
chrome/test/data/extensions/api_test/file_browser/content_checksum_test/test.js
(right):

https://codereview.chromium.org/840843002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_browser/content_checksum_test/test.js:1:
// Copyright (c) 2013 The Chromium Authors. All rights reserved.
On 2015/01/08 00:38:40, mtomasz wrote:
> (c) 2013 -> 2014

2015!

https://codereview.chromium.org/840843002/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/file_browser/content_checksum_test/test.js:15:
function initTests(callback) {
On 2015/01/08 00:38:40, mtomasz wrote:
> I think this can be greatly simplified. If we only care about the file system
> with testing volume, not Drive.
> We could simply iterate over the volume metadata list and request the file
> system which has 'testing' substring. No need to sort, collect any tests, etc.

Ah, ok.  Thanks for clarifying - I wasn't sure what was and wasn't needed.  Much
cleaner now.

[Alexei Svitkine (slow), 2015-01-08 19:34:00]

histograms lgtm

[Ken Rockot(use gerrit already), 2015-01-08 20:14:36]

IDL lgtm

[mtomasz, 2015-01-09 05:11:28]

Two hints for faster reviews:
1. git cl format before each patchset upload,
2. git cl try after each patchset upload.

Cheers!

https://codereview.chromium.org/840843002/diff/20001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc
(right):

https://codereview.chromium.org/840843002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:134:
base::ScopedTempDir *temp_dir,
nit: Pointer asterisk glue to the type name, not variable name. Please run git
cl format before each patchset upload.

https://codereview.chromium.org/840843002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc
(right):

https://codereview.chromium.org/840843002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:745:
const std::string& hash) {
Does it compile?

[mtomasz, 2015-01-09 05:19:02]

https://codereview.chromium.org/840843002/diff/1/chrome/browser/chromeos/exte...
File chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc
(right):

https://codereview.chromium.org/840843002/diff/1/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:735:
const std::string& md5 = drive::util::GetMd5Digest(file_url.path());
On 2015/01/08 19:29:14, Ben Kwa wrote:
> On 2015/01/08 00:38:40, mtomasz wrote:
> > This utility makes file IO, it must not be called on UI thread. Please do
> > PostTask to FILE thread *or* use a blocking pool.
> > 
> > See:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...
> 
> Interesting, the doc for AsyncExtensionFunction made it sound like it runs
> asynchronously w.r.t. the UI thread, which I took to mean it didn't run on the
> UI thread.  So, what does it mean to say these run "asynchronously relative to
> the browser's UI thread"?
> 
> I changed it to use PostTaskAndReplyWithResult.

The comment sounds very cryptic. IIUC AsyncExtensionFunction 
*can* involve other threads and return the result asynchronously with
SendResponse. In contrast to it, SyncExtensionFunction requires to set result
synchronously in RunSync(). But both RunSync and RunAsync run on UI thread.

[Ben Kwa, 2015-01-09 06:24:19]

https://codereview.chromium.org/840843002/diff/20001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc
(right):

https://codereview.chromium.org/840843002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/file_manager/file_manager_private_apitest.cc:134:
base::ScopedTempDir *temp_dir,
On 2015/01/09 05:11:28, mtomasz wrote:
> nit: Pointer asterisk glue to the type name, not variable name. Please run git
> cl format before each patchset upload.

Done.

https://codereview.chromium.org/840843002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc
(right):

https://codereview.chromium.org/840843002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/extensions/file_manager/private_api_file_system.cc:745:
const std::string& hash) {
On 2015/01/09 05:11:28, mtomasz wrote:
> Does it compile?

Yup, compiles and runs.

[mtomasz, 2015-01-09 06:25:34]

lgtm

[Ben Kwa, 2015-01-09 15:42:47]

The CQ bit was checked by kenobi@chromium.org

[commit-bot: I haz the power, 2015-01-09 15:43:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/840843002/60001

[commit-bot: I haz the power, 2015-01-09 16:54:36]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-01-09 16:55:30]

Patchset 4 (id:??) landed as
https://crrev.com/32e0c3fc3ca56027343838365e742e396a8a0fd2
Cr-Commit-Position: refs/heads/master@{#310782}