Add 'App Size' to App Info Dialog

Add 'App Size' to App Info Dialog

Added the size of the app to the App Info dialog, which is computed by
asynchronously calculating the recursive size of all files in the app's
directory on disk. Displays 'Calculating' while the calculation is in
progress, and then the app's size when it's complete, formatted in a
human-readable way.

Based on CL 425933002, which adds the Installed and Last Run dates to
the dialog.

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

[sashab, 2014-07-30 05:15:40]

tapted@ please review first, and advise on the lifetime stuff if possible :-)

benwells@ to review after, mainly on UI and code struct.

[tapted, 2014-07-31 00:42:04]

Have you discussed whether this is an accurate measure of size an app takes up?

I think things an app stores in indexdb or HTM5 sandboxed filesystems are stored
elsewhere, but are still removed on uninstall. That should probably be a
separate field though - I think that's what android does

https://codereview.chromium.org/423363002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_panel.cc
(right):

https://codereview.chromium.org/423363002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_panel.cc:336:
content::BrowserThread::PostTaskAndReply(
I think you want PostTaskAndReplyWithResult. Also the FILE thread is pretty
clogged - I think new code is meant to use the blocking pool. I think it would
look something like

  base::PostTaskAndReplyWithResult(
      BrowserThread::GetBlockingPool(),
      FROM_HERE,
      base::Bind(&base::ComputeDirectorySize, app_->path()),
      base::Bind(&AppInfoSummaryPanel::OnAppSizeCalculated, AsWeakPtr()));

https://codereview.chromium.org/423363002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_panel.cc:345: void
AppInfoSummaryPanel::OnAppSizeCalculated(
Then this can just take an an int64 argument

https://codereview.chromium.org/423363002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_panel.h
(right):

https://codereview.chromium.org/423363002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_panel.h:84: int64
app_size_in_bytes_;
I don't think you use this

[sashab, 2014-07-31 05:00:46]

https://codereview.chromium.org/423363002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_panel.cc
(right):

https://codereview.chromium.org/423363002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_panel.cc:336:
content::BrowserThread::PostTaskAndReply(
On 2014/07/31 00:42:04, tapted wrote:
> I think you want PostTaskAndReplyWithResult. Also the FILE thread is pretty
> clogged - I think new code is meant to use the blocking pool. I think it would
> look something like
> 
>   base::PostTaskAndReplyWithResult(
>       BrowserThread::GetBlockingPool(),
>       FROM_HERE,
>       base::Bind(&base::ComputeDirectorySize, app_->path()),
>       base::Bind(&AppInfoSummaryPanel::OnAppSizeCalculated, AsWeakPtr()));
> 

Nice! Saves so much code. Strange how there are different base:: and
BrowerThread:: PostTaskAndReplyWithResult... Anyway, done :)

https://codereview.chromium.org/423363002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_panel.cc:345: void
AppInfoSummaryPanel::OnAppSizeCalculated(
On 2014/07/31 00:42:04, tapted wrote:
> Then this can just take an an int64 argument

Yup, done.

https://codereview.chromium.org/423363002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_panel.h
(right):

https://codereview.chromium.org/423363002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_panel.h:84: int64
app_size_in_bytes_;
On 2014/07/31 00:42:04, tapted wrote:
> I don't think you use this

Removed.

[sashab, 2014-07-31 05:01:10]

Oops, and I meant to say - yes this has been discussed. See bug but basically
they will be displayed in separate sections.

[tapted, 2014-07-31 06:26:42]

I think you upstream branch is pointing to the wrong thing

[sashab, 2014-07-31 07:13:42]

Oops, thanks. Fixed. :)

[tapted, 2014-07-31 07:39:57]

lgtm (with a nit)

https://codereview.chromium.org/423363002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_panel.cc
(right):

https://codereview.chromium.org/423363002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_panel.cc:296: if
(size_title_ && size_value_)
nit: all these `if`s should have curly braces since the bodies are multi-line.
(although I learned recently that if only the condition is multi-line, you can
skip curlies). I know this new one is consistent wiht others, but they should
all be updated really :/

[sashab, 2014-07-31 10:06:05]

https://codereview.chromium.org/423363002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_panel.cc
(right):

https://codereview.chromium.org/423363002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/apps/app_info_dialog/app_info_summary_panel.cc:296: if
(size_title_ && size_value_)
On 2014/07/31 07:39:57, tapted wrote:
> nit: all these `if`s should have curly braces since the bodies are multi-line.
> (although I learned recently that if only the condition is multi-line, you can
> skip curlies). I know this new one is consistent wiht others, but they should
> all be updated really :/

Done :) Updated the other ones in the CL this one is based on, too, since it
hasn't landed yet :)

[sashab, 2014-07-31 10:06:09]

The CQ bit was checked by sashab@chromium.org

[sashab, 2014-07-31 10:06:13]

The CQ bit was unchecked by sashab@chromium.org

[sashab, 2014-08-01 01:00:07]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2014-08-01 01:04:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sashab@chromium.org/423363002/120001

[commit-bot: I haz the power, 2014-08-01 08:33:02]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-01 15:44:39]

Change committed as 287007