Storage manager: Add site data size on the amount of browsing data.

Storage manager: Add site data size on the amount of browsing data.

The size of browsing data used to contain only HTTP cache.
This CL adds the total size of site data as a part of browsing data.

BUG=625072
TEST=manually tested

Committed: https://crrev.com/bace6ed0fd91cd357569dc9ad1d285ff2390520e
Cr-Commit-Position: refs/heads/master@{#404320}

[fukino, 2016-07-06 08:56:57]

Description was changed from

==========
Storage manager: Add site data size on the amount of browsing data.

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

to

==========
Storage manager: Add site data size on the amount of browsing data.

The size of browsing data used to contain only HTTP cache.
This CL adds the total size of site data as a part of browsing data.

BUG=625072
TEST=manually tested
==========

[fukino, 2016-07-06 08:57:16]

fukino@chromium.org changed reviewers:
+ dbeam@chromium.org

[fukino, 2016-07-06 08:57:16]

Dan, could you take a look? Thanks!

[Dan Beam, 2016-07-06 18:29:22]

lgtm w/nits

https://codereview.chromium.org/2123133002/diff/1/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/storage_manager_handler.cc
(right):

https://codereview.chromium.org/2123133002/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/storage_manager_handler.cc:317: if
(browser_site_data_size_ >= 0 && browser_cache_size_ >= 0) {
what if there was a valid size and now it fails?  do you want the stale invalid
size to show?

https://codereview.chromium.org/2123133002/diff/1/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/storage_manager_handler.h (right):

https://codereview.chromium.org/2123133002/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/storage_manager_handler.h:9: 
#include <memory>

[fukino, 2016-07-07 11:08:18]

Could you take another look?
I made a non-trivial change on this CL to update the size to "Unknown" when
fetching browsing data fails.

https://codereview.chromium.org/2123133002/diff/1/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/storage_manager_handler.cc
(right):

https://codereview.chromium.org/2123133002/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/storage_manager_handler.cc:317: if
(browser_site_data_size_ >= 0 && browser_cache_size_ >= 0) {
On 2016/07/06 18:29:22, Dan Beam wrote:
> what if there was a valid size and now it fails?  do you want the stale
invalid
> size to show?

Ah, yes, when fetching browsing data fails, I think it should be better to show
"Unknown". Thanks!
I updated the CL to do so.

https://codereview.chromium.org/2123133002/diff/1/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/chromeos/storage_manager_handler.h (right):

https://codereview.chromium.org/2123133002/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/chromeos/storage_manager_handler.h:9: 
On 2016/07/06 18:29:22, Dan Beam wrote:
> #include <memory>

Done.

[Dan Beam, 2016-07-07 18:46:23]

i'm fine with this implementation, but 2 bools and 2 ints sure did seem
simpler...  i assume you're adding a whole bunch more types soon or something?

lgtm

https://codereview.chromium.org/2123133002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/storage_manager_handler.cc
(right):

https://codereview.chromium.org/2123133002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/storage_manager_handler.cc:314:
browsing_data_size_[BROWSING_DATA_TYPE_SITE_DATA] >= 0) {
you treat everything else like a map then expand out all the keys here ;)

https://codereview.chromium.org/2123133002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/storage_manager_handler.cc:318: }
nit:

base::string16 size_string;

if (browsing_data_size_[BROWSING_DATA_TYPE_HTTP_CACHE] >= 0
    browsing_data_size_[BROWSING_DATA_TYPE_SITE_DATA] >= 0) {
  size_string = ui::FormatBytes(
      browsing_data_size_[BROWSING_DATA_TYPE_HTTP_CACHE] +
      browsing_data_size_[BROWSING_DATA_TYPE_SITE_DATA]);
} else {
  size_string = l10n_util::GetStringUTF16(
      IDS_OPTIONS_SETTINGS_STORAGE_SIZE_UNKNOWN);
}

web_ui()->CallJavascriptFunctionUnsafe(
    "options.StorageManager.setBrowsingDataSize",
    base::StringValue(size_string));

avoids some duplication and a little bit of extra work

[fukino, 2016-07-08 03:47:01]

Though I was thinking of a possibility of future expansion, 
I don't have any plan to expand the type of browsing data right now.
Let me go back to the old approach and use 2 bools and 2 ints to track the
fetched browsing data size to keep it simple.

https://codereview.chromium.org/2123133002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/chromeos/storage_manager_handler.cc
(right):

https://codereview.chromium.org/2123133002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/storage_manager_handler.cc:314:
browsing_data_size_[BROWSING_DATA_TYPE_SITE_DATA] >= 0) {
On 2016/07/07 18:46:23, Dan Beam wrote:
> you treat everything else like a map then expand out all the keys here ;)

I don't use map in the latest patch.
I'll iterate through the map next time!

https://codereview.chromium.org/2123133002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/chromeos/storage_manager_handler.cc:318: }
On 2016/07/07 18:46:22, Dan Beam wrote:
> nit:
> 
> base::string16 size_string;
> 
> if (browsing_data_size_[BROWSING_DATA_TYPE_HTTP_CACHE] >= 0
>     browsing_data_size_[BROWSING_DATA_TYPE_SITE_DATA] >= 0) {
>   size_string = ui::FormatBytes(
>       browsing_data_size_[BROWSING_DATA_TYPE_HTTP_CACHE] +
>       browsing_data_size_[BROWSING_DATA_TYPE_SITE_DATA]);
> } else {
>   size_string = l10n_util::GetStringUTF16(
>       IDS_OPTIONS_SETTINGS_STORAGE_SIZE_UNKNOWN);
> }
> 
> web_ui()->CallJavascriptFunctionUnsafe(
>     "options.StorageManager.setBrowsingDataSize",
>     base::StringValue(size_string));
> 
> avoids some duplication and a little bit of extra work

Done. Thank you for the suggestion!
I took the same way in OnGetOtherUserSize() and OnGetArcSize().

[fukino, 2016-07-08 04:24:40]

The CQ bit was checked by fukino@chromium.org

[fukino, 2016-07-08 04:24:40]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2123133002/#ps60001
(title: "Use 2 ints and 2 bools to track fetched browsing data size.")

[commit-bot: I haz the power, 2016-07-08 04:25:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-08 09:28:57]

Description was changed from

==========
Storage manager: Add site data size on the amount of browsing data.

The size of browsing data used to contain only HTTP cache.
This CL adds the total size of site data as a part of browsing data.

BUG=625072
TEST=manually tested
==========

to

==========
Storage manager: Add site data size on the amount of browsing data.

The size of browsing data used to contain only HTTP cache.
This CL adds the total size of site data as a part of browsing data.

BUG=625072
TEST=manually tested
==========

[commit-bot: I haz the power, 2016-07-08 09:28:58]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-08 09:32:24]

Description was changed from

==========
Storage manager: Add site data size on the amount of browsing data.

The size of browsing data used to contain only HTTP cache.
This CL adds the total size of site data as a part of browsing data.

BUG=625072
TEST=manually tested
==========

to

==========
Storage manager: Add site data size on the amount of browsing data.

The size of browsing data used to contain only HTTP cache.
This CL adds the total size of site data as a part of browsing data.

BUG=625072
TEST=manually tested

Committed: https://crrev.com/bace6ed0fd91cd357569dc9ad1d285ff2390520e
Cr-Commit-Position: refs/heads/master@{#404320}
==========

[commit-bot: I haz the power, 2016-07-08 09:32:25]

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