Reland of 260688: [Files.app] Use getDriveEntryProperties to retrieve metadata

Reland of 260688: [Files.app] Use getDriveEntryProperties() to retrieve metadata

This is a reland of 260688, which is reverted due to false charge of build failure.

Previously, we used FileEntry.getMetadata() to retrieve filesystem metadata and getDriveEntryProperties() to get Drive metadata, so we need 2 calls for 1 file on Drive.

With this patch, getDriveEntryProperties() returns not only Drive metadata but also filesystem metadata. It's enough to call only getDriveEntryProperties() and we can reduce a number of calls by half.

BUG=345196
TEST=browser_test passes.
R=asargent@chromium.org, hashimoto@chromium.org, hirono@chromium.org
TBR=asargent@chromium.org, hashimoto@chromium.org, hirono@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266437

[yoshiki, 2014-03-14 08:28:18]

PTAL, Thanks,

hashimoto: C++ code
hirono: JS code.

[hashimoto, 2014-03-14 08:31:18]

chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc lgtm

[hirono, 2014-03-14 08:47:29]

https://codereview.chromium.org/195763004/diff/60001/chrome/browser/resources...
File
chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js
(right):

https://codereview.chromium.org/195763004/diff/60001/chrome/browser/resources...
chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js:845:
size: (entry.isFile ? (data.fileSize || 0) : -1),
Currently the size of directories on the drive are 0.
Maybe keeping 0 is better for avoiding regression.

[yoshiki, 2014-03-14 10:03:11]

@hirono, PTAL

https://codereview.chromium.org/195763004/diff/60001/chrome/browser/resources...
File
chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js
(right):

https://codereview.chromium.org/195763004/diff/60001/chrome/browser/resources...
chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js:845:
size: (entry.isFile ? (data.fileSize || 0) : -1),
On 2014/03/14 08:47:29, hirono wrote:
> Currently the size of directories on the drive are 0.
> Maybe keeping 0 is better for avoiding regression.

Yes, I completely agree with you. Done.

[hirono, 2014-03-17 02:28:00]

https://codereview.chromium.org/195763004/diff/80001/chrome/browser/resources...
File
chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js
(right):

https://codereview.chromium.org/195763004/diff/80001/chrome/browser/resources...
chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js:845:
size: (entry.isFile ? (data.fileSize || 0) : -1),
How about this line?

[yoshiki, 2014-03-17 04:06:14]

https://codereview.chromium.org/195763004/diff/80001/chrome/browser/resources...
File
chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js
(right):

https://codereview.chromium.org/195763004/diff/80001/chrome/browser/resources...
chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js:845:
size: (entry.isFile ? (data.fileSize || 0) : -1),
On 2014/03/17 02:28:00, hirono wrote:
> How about this line?

Sorry, I was misunderstood and changed my mind. I thought a size value of 
directory would be ignored, but it's wrong.

I think we should keep it returning '-1' if it's directory. In Files.app, we
distinguish looks between a 0-byte file and a 0-byte directory ('0' vs '---'), 
and '-1' is used as the size value representing directory, so it is reasonable
to keep use '-1' as a size for directory.

[hirono, 2014-03-17 08:49:11]

On 2014/03/17 04:06:14, yoshiki wrote:
>
https://codereview.chromium.org/195763004/diff/80001/chrome/browser/resources...
> File
> chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js
> (right):
> 
>
https://codereview.chromium.org/195763004/diff/80001/chrome/browser/resources...
>
chrome/browser/resources/file_manager/foreground/js/metadata/metadata_cache.js:845:
> size: (entry.isFile ? (data.fileSize || 0) : -1),
> On 2014/03/17 02:28:00, hirono wrote:
> > How about this line?
> 
> Sorry, I was misunderstood and changed my mind. I thought a size value of 
> directory would be ignored, but it's wrong.
> 
> I think we should keep it returning '-1' if it's directory. In Files.app, we
> distinguish looks between a 0-byte file and a 0-byte directory ('0' vs '---'),

> and '-1' is used as the size value representing directory, so it is reasonable
> to keep use '-1' as a size for directory.

Got it. Thanks! lgtm.

[yoshiki, 2014-03-17 09:05:13]

Thanks

[yoshiki, 2014-03-24 16:33:31]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2014-03-24 16:34:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/195763004/100001

[commit-bot: I haz the power, 2014-03-24 17:13:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-24 17:13:15]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[yoshiki, 2014-03-28 09:32:52]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2014-03-28 09:33:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/195763004/100001

[commit-bot: I haz the power, 2014-03-28 10:13:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-28 10:13:01]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[yoshiki, 2014-03-31 07:09:02]

@asargent: Could you take a look at the change in file_browser_private.idl?
Thanks.

[yoshiki, 2014-03-31 17:26:36]

The CQ bit was checked by yoshiki@chromium.org

[yoshiki, 2014-03-31 17:26:38]

The CQ bit was unchecked by yoshiki@chromium.org

[yoshiki, 2014-03-31 17:27:10]

On 2014/03/31 07:09:02, yoshiki wrote:
> @asargent: Could you take a look at the change in file_browser_private.idl?
> Thanks.

I'll add you as TBR, since this is a bit urgent. Thanks.

[yoshiki, 2014-03-31 17:27:17]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2014-03-31 17:28:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/195763004/100001

[commit-bot: I haz the power, 2014-03-31 18:22:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-31 18:22:15]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests,
check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests,
components_unittests, compositor_unittests, content_browsertests,
content_unittests, crypto_unittests, events_unittests, gfx_unittests,
google_apis_unittests, gpu_unittests, installer_util_unittests, ipc_tests,
jingle_unittests, media_unittests, mini_installer_test, nacl_integration,
ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, telemetry_perf_unittests,
telemetry_unittests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[asargent_no_longer_on_chrome, 2014-03-31 19:39:03]

lgtm

[yoshiki, 2014-03-31 22:53:55]

Committed patchset #3 manually as r260688 (presubmit successful).

[yoshiki, 2014-04-28 04:31:57]

The CQ bit was checked by yoshiki@chromium.org

[commit-bot: I haz the power, 2014-04-28 04:33:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/195763004/140001

[commit-bot: I haz the power, 2014-04-28 05:22:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-28 05:22:49]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[yoshiki, 2014-04-28 06:00:42]

Committed patchset #4 manually as r266437 (presubmit successful).