Move implementation of QuotaManager.getVolumeInfo to base::SysInfo.

Move implementation of QuotaManager.getVolumeInfo to base::SysInfo.

We get both total disk space and available disk space in multiple places.
I'd like to move the QuotaManager's implementation to base::SysInfo so that we can share the implementation.
This CL also changes the type of disk size from uint64_t to int64_t to be consistent with base::SysInfo::AmountOfFreeDiskSpace().

BUG=none

Committed: https://crrev.com/50a448115ed04d2433e903affb17c8bcd617b367
Cr-Commit-Position: refs/heads/master@{#399252}

[fukino, 2016-06-09 16:59:38]

Description was changed from

==========
Move implementation of QuotaManager.getVolumeInfo to base::SysInfo.

BUG=
==========

to

==========
Move implementation of QuotaManager.getVolumeInfo to base::SysInfo.

We get both total disk space and available disk space in multiple places.
I'd like to move the QuotaManager's implementation to base::SysInfo so that we
can share the implementation.
This CL also changes the type of disk size from uint64_t to int64_t to be
consistent with base::SysInfo::AmountOfFreeDiskSpace().

BUG=none
==========

[fukino, 2016-06-09 17:02:42]

fukino@chromium.org changed reviewers:
+ thestig@chromium.org, tzik@chromium.org

[fukino, 2016-06-09 17:02:42]

thestig@, PTAL at sys_info.
tzik@, PTAL at quota_manager.
Thanks!

[Dan Beam, 2016-06-09 17:20:13]

why do we need AmountOfFreeDiskSpace if we have GetDiskSpaceInfo?  can we just
change everything to GetDiskSpaceInfo and pass an unused (or nullptr) param if
we don't care about one of the numbers?

https://codereview.chromium.org/2052663003/diff/20001/base/sys_info_posix.cc
File base/sys_info_posix.cc (right):

https://codereview.chromium.org/2052663003/diff/20001/base/sys_info_posix.cc#...
base/sys_info_posix.cc:98: return static_cast<int64_t>(stats.f_bavail) *
stats.f_frsize;
nit: maybe use GetDiskSpaceInfo() to implement AmountOfFreeDiskSpace?

int64_t available, unused;
if (!GetDiskSpaceInfo(path, &available, &unused))
  return -1;
return available;

OR

share the stat'ing code?

bool Stat(const char* path, statvfs* stats) {
  return HANDLE_EINTR(statvfs(path, stats)) != 0;
}

and from both

struct statvfs stats;
if (!Stat(path.value().c_str(), &stats))
  return false;  // or -1

https://codereview.chromium.org/2052663003/diff/20001/base/sys_info_unittest.cc
File base/sys_info_unittest.cc (right):

https://codereview.chromium.org/2052663003/diff/20001/base/sys_info_unittest....
base/sys_info_unittest.cc:54: EXPECT_GT(available, 0) << tmp_path.value();
what if the disk is full?  do we want to handle that by changing to _GE?  is
that nuts?

https://codereview.chromium.org/2052663003/diff/20001/base/sys_info_win.cc
File base/sys_info_win.cc (right):

https://codereview.chromium.org/2052663003/diff/20001/base/sys_info_win.cc#ne...
base/sys_info_win.cc:66: return rv < 0 ? std::numeric_limits<int64_t>::max() :
rv;
what about this logic?

[fukino, 2016-06-10 01:33:37]

Thank you for the review!
As AmountOfFreeDiskSpace() looks handy for users who need only available size,
I'd like too keep it and make its implementation use GetDiskSpaceInfo().

https://codereview.chromium.org/2052663003/diff/20001/base/sys_info_posix.cc
File base/sys_info_posix.cc (right):

https://codereview.chromium.org/2052663003/diff/20001/base/sys_info_posix.cc#...
base/sys_info_posix.cc:98: return static_cast<int64_t>(stats.f_bavail) *
stats.f_frsize;
On 2016/06/09 17:20:13, Dan Beam wrote:
> nit: maybe use GetDiskSpaceInfo() to implement AmountOfFreeDiskSpace?
> 
> int64_t available, unused;
> if (!GetDiskSpaceInfo(path, &available, &unused))
>   return -1;
> return available;
> 
> OR
> 
> share the stat'ing code?
> 
> bool Stat(const char* path, statvfs* stats) {
>   return HANDLE_EINTR(statvfs(path, stats)) != 0;
> }
> 
> and from both
> 
> struct statvfs stats;
> if (!Stat(path.value().c_str(), &stats))
>   return false;  // or -1

Done.
I updated this to use GetDiskSpaceInfo() in AmountOfFreeDiskSpace.

https://codereview.chromium.org/2052663003/diff/20001/base/sys_info_unittest.cc
File base/sys_info_unittest.cc (right):

https://codereview.chromium.org/2052663003/diff/20001/base/sys_info_unittest....
base/sys_info_unittest.cc:54: EXPECT_GT(available, 0) << tmp_path.value();
On 2016/06/09 17:20:13, Dan Beam wrote:
> what if the disk is full?  do we want to handle that by changing to _GE?  is
> that nuts?

I updated it to _GE, and I changed the initial values of |available| and |total|
to -1 to check that the |available| is changed from initial value.

https://codereview.chromium.org/2052663003/diff/20001/base/sys_info_win.cc
File base/sys_info_win.cc (right):

https://codereview.chromium.org/2052663003/diff/20001/base/sys_info_win.cc#ne...
base/sys_info_win.cc:66: return rv < 0 ? std::numeric_limits<int64_t>::max() :
rv;
On 2016/06/09 17:20:13, Dan Beam wrote:
> what about this logic?

Added the logic inside GetDiskSpaceInfo to cover the case when the
|available.QuadPart| is bigger than INT64_MAX.

[Lei Zhang, 2016-06-10 05:46:37]

https://codereview.chromium.org/2052663003/diff/40001/base/sys_info.h
File base/sys_info.h (right):

https://codereview.chromium.org/2052663003/diff/40001/base/sys_info.h#newcode55
base/sys_info.h:55: static bool GetDiskSpaceInfo(const FilePath& path,
Most of the functions here just retrieves one piece of information. Can we just
add GetVolumeSize() instead for consistency? We'll need to retain
QuotaManager::GetVolumeInfo() but it will be simpler. I understand it's slightly
less efficient to call stat() twice, but I don't think this is performance
critical.

https://codereview.chromium.org/2052663003/diff/40001/storage/browser/quota/q...
File storage/browser/quota/quota_manager.cc (right):

https://codereview.chromium.org/2052663003/diff/40001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:1836: int64_t available, total;
style: One variable declaration per line please.

[fukino, 2016-06-10 12:10:09]

Thank you!

https://codereview.chromium.org/2052663003/diff/40001/base/sys_info.h
File base/sys_info.h (right):

https://codereview.chromium.org/2052663003/diff/40001/base/sys_info.h#newcode55
base/sys_info.h:55: static bool GetDiskSpaceInfo(const FilePath& path,
On 2016/06/10 05:46:37, Lei Zhang (Slow) wrote:
> Most of the functions here just retrieves one piece of information. Can we
just
> add GetVolumeSize() instead for consistency? We'll need to retain
> QuotaManager::GetVolumeInfo() but it will be simpler. I understand it's
slightly
> less efficient to call stat() twice, but I don't think this is performance
> critical.

I added the function which returns only total size, but I named it as
AmountOfTotalDiskSpace to be consistent with AmountOfFreeDiskSpace. WDYT?

https://codereview.chromium.org/2052663003/diff/40001/storage/browser/quota/q...
File storage/browser/quota/quota_manager.cc (right):

https://codereview.chromium.org/2052663003/diff/40001/storage/browser/quota/q...
storage/browser/quota/quota_manager.cc:1836: int64_t available, total;
On 2016/06/10 05:46:37, Lei Zhang (Slow) wrote:
> style: One variable declaration per line please.

In the new patch, this line doesn't need to be changed, so I keep it as it is.

[Lei Zhang, 2016-06-10 17:43:11]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2016-06-10 17:43:11]

lgtm

https://codereview.chromium.org/2052663003/diff/60001/base/sys_info_win.cc
File base/sys_info_win.cc (right):

https://codereview.chromium.org/2052663003/diff/60001/base/sys_info_win.cc#ne...
base/sys_info_win.cc:36: ULARGE_INTEGER available, total, free;
One variable per line here too, but don't worry about it.

[commit-bot: I haz the power, 2016-06-10 17:43:49]

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

[commit-bot: I haz the power, 2016-06-10 17:51:06]

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

[commit-bot: I haz the power, 2016-06-10 17:51:07]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[tzik, 2016-06-10 17:58:43]

lgtm

[Lei Zhang, 2016-06-10 17:59:34]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2016-06-10 17:59:55]

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

[commit-bot: I haz the power, 2016-06-10 19:36:06]

Description was changed from

==========
Move implementation of QuotaManager.getVolumeInfo to base::SysInfo.

We get both total disk space and available disk space in multiple places.
I'd like to move the QuotaManager's implementation to base::SysInfo so that we
can share the implementation.
This CL also changes the type of disk size from uint64_t to int64_t to be
consistent with base::SysInfo::AmountOfFreeDiskSpace().

BUG=none
==========

to

==========
Move implementation of QuotaManager.getVolumeInfo to base::SysInfo.

We get both total disk space and available disk space in multiple places.
I'd like to move the QuotaManager's implementation to base::SysInfo so that we
can share the implementation.
This CL also changes the type of disk size from uint64_t to int64_t to be
consistent with base::SysInfo::AmountOfFreeDiskSpace().

BUG=none
==========

[commit-bot: I haz the power, 2016-06-10 19:36:07]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-10 19:36:11]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-10 19:37:16]

Description was changed from

==========
Move implementation of QuotaManager.getVolumeInfo to base::SysInfo.

We get both total disk space and available disk space in multiple places.
I'd like to move the QuotaManager's implementation to base::SysInfo so that we
can share the implementation.
This CL also changes the type of disk size from uint64_t to int64_t to be
consistent with base::SysInfo::AmountOfFreeDiskSpace().

BUG=none
==========

to

==========
Move implementation of QuotaManager.getVolumeInfo to base::SysInfo.

We get both total disk space and available disk space in multiple places.
I'd like to move the QuotaManager's implementation to base::SysInfo so that we
can share the implementation.
This CL also changes the type of disk size from uint64_t to int64_t to be
consistent with base::SysInfo::AmountOfFreeDiskSpace().

BUG=none

Committed: https://crrev.com/50a448115ed04d2433e903affb17c8bcd617b367
Cr-Commit-Position: refs/heads/master@{#399252}
==========

[commit-bot: I haz the power, 2016-06-10 19:37:17]

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