Port storage manager to MD settings.

Port storage manager to MD settings.

This CL adds an "Storage management" item on MD settings/Device, and it shows the size of each storage item.
C++ side implementation is basically copied from old setting's one.
(I don't share the implementation assuming that the old one will be removed.)

Following are not implemented.
- The UI design is temporary.
- metrics/tests will be added separately.

BUG=629438
TEST=with the flag "--enable-md-storage-manager=1",  manually tested that chrome://md-settings/storage shows the same items/values with chrome://settings/storage
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/519e0e2c9424ec070416451b0254123bdc125545
Cr-Commit-Position: refs/heads/master@{#423386}

[fukino, 2016-09-16 14:18:57]

Description was changed from

==========
(WIP) Port storage manager to MD settings.

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

to

==========
(WIP) Port storage manager to MD settings.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[fukino, 2016-09-21 08:39:22]

Description was changed from

==========
(WIP) Port storage manager to MD settings.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Port storage manager to MD settings part 1.

As a first step, this CL adds an "Storage management" item on MD
settings/Device, and it shows the size of each storage item.
C++ side implementation is basically copied from old setting's one.
(I don't share the implementation assuming that the old one will be removed.)

Following are not implemented.
- Removing Google Drive cache is not implemented yet.
- The UI design is temporary.
- metrics/tests will be added separately.

BUG=629438
TEST=manually tested that chrome://md-settings/storage shows the same
items/values with chrome://settings/storage
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[fukino, 2016-09-21 08:40:14]

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

[fukino, 2016-09-21 08:40:14]

Dan, could you take a look?

[Dan Beam, 2016-09-23 06:18:21]

can we start adding this stuff behind a flag or something? we're really trying
not to block settings on new features. this does exist in old options, but
didn't when we started. now I'm kind if worried more churn to the UI will add
more dependencies and risk to our project

[fukino, 2016-09-26 14:55:42]

Description was changed from

==========
Port storage manager to MD settings part 1.

As a first step, this CL adds an "Storage management" item on MD
settings/Device, and it shows the size of each storage item.
C++ side implementation is basically copied from old setting's one.
(I don't share the implementation assuming that the old one will be removed.)

Following are not implemented.
- Removing Google Drive cache is not implemented yet.
- The UI design is temporary.
- metrics/tests will be added separately.

BUG=629438
TEST=manually tested that chrome://md-settings/storage shows the same
items/values with chrome://settings/storage
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Port storage manager to MD settings.

This CL adds an "Storage management" item on MD settings/Device, and it shows
the size of each storage item.
C++ side implementation is basically copied from old setting's one.
(I don't share the implementation assuming that the old one will be removed.)

Following are not implemented.
- The UI design is temporary.
- metrics/tests will be added separately.

BUG=629438
TEST=manually tested that chrome://md-settings/storage shows the same
items/values with chrome://settings/storage
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[fukino, 2016-09-26 16:31:03]

On 2016/09/23 06:18:21, Dan Beam wrote:
> can we start adding this stuff behind a flag or something? we're really trying
> not to block settings on new features. this does exist in old options, but
> didn't when we started. now I'm kind if worried more churn to the UI will add
> more dependencies and risk to our project

I can add the flag, but if the MD storage manager is the blocker for Chrome OS
MD settings, the flag will not help us.
I'll wait the decision whether the MD storage manager is necessary, and if it is
a blocker, I'll focus on implementing the finalized design.

[fukino, 2016-09-26 18:17:25]

On 2016/09/26 16:31:03, fukino wrote:
> On 2016/09/23 06:18:21, Dan Beam wrote:
> > can we start adding this stuff behind a flag or something? we're really
trying
> > not to block settings on new features. this does exist in old options, but
> > didn't when we started. now I'm kind if worried more churn to the UI will
add
> > more dependencies and risk to our project
> 
> I can add the flag, but if the MD storage manager is the blocker for Chrome OS
> MD settings, the flag will not help us.
> I'll wait the decision whether the MD storage manager is necessary, and if it
is
> a blocker, I'll focus on implementing the finalized design.

Based on the agreement, I'll add this behind a flag.

[fukino, 2016-09-28 14:49:05]

The CQ bit was checked by fukino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 14:49:22]

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

[fukino, 2016-09-28 15:24:43]

Description was changed from

==========
Port storage manager to MD settings.

This CL adds an "Storage management" item on MD settings/Device, and it shows
the size of each storage item.
C++ side implementation is basically copied from old setting's one.
(I don't share the implementation assuming that the old one will be removed.)

Following are not implemented.
- The UI design is temporary.
- metrics/tests will be added separately.

BUG=629438
TEST=manually tested that chrome://md-settings/storage shows the same
items/values with chrome://settings/storage
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Port storage manager to MD settings.

This CL adds an "Storage management" item on MD settings/Device, and it shows
the size of each storage item.
C++ side implementation is basically copied from old setting's one.
(I don't share the implementation assuming that the old one will be removed.)

Following are not implemented.
- The UI design is temporary.
- metrics/tests will be added separately.

BUG=629438
TEST=with the flag "--enable-md-storage-manager=1",  manually tested that
chrome://md-settings/storage shows the same items/values with
chrome://settings/storage
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[fukino, 2016-09-28 15:27:54]

Dan, could you take a look?
I implemented the storage manager based on the latest spec behind a flag
"--enable-md-storage-manager=1".

I ported all functions at once in this CL.
If smaller CLs are better to be reviewed, please let me know.
Thanks!

[Dan Beam, 2016-09-29 02:51:55]

dbeam@chromium.org changed reviewers:
+ michaelpg@chromium.org, stevenjb@chromium.org
- dbeam@chromium.org

[Dan Beam, 2016-09-29 02:52:10]

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

[Dan Beam, 2016-09-29 02:52:11]

+michaelpg@ / stevenjb@ -- they're better CrOS reviewers

[Dan Beam, 2016-09-29 02:52:19]

dbeam@chromium.org changed reviewers:
- dbeam@chromium.org

[michaelpg, 2016-09-29 22:19:02]

michaelpg@chromium.org changed reviewers:
- stevenjb@chromium.org

[michaelpg, 2016-09-29 22:19:02]

stevenjb being OOO, I'll look over this today. You'll need an owner for metrics,
too.

[fukino, 2016-09-30 03:34:36]

On 2016/09/29 22:19:02, michaelpg wrote:
> stevenjb being OOO, I'll look over this today. You'll need an owner for
metrics,
> too.

Thanks!
I assume this is not urgent (as storage manager is only for Chrome OS), so
please take a look when you have cycles.

[michaelpg, 2016-10-01 01:41:05]

https://codereview.chromium.org/2348913002/diff/100001/chrome/app/settings_st...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2348913002/diff/100001/chrome/app/settings_st...
chrome/app/settings_strings.grdp:2350: <message
name="IDS_SETTINGS_STORAGE_ITEM_ANDROID" desc="In Device Settings > Storage,
label for the total size size of Android apps and cache.">
size size

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/device_page/device_page.html (right):

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/device_page.html:50: <template
is="dom-if" if=[[showStorageManager_]]>
quotes around attributes

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/device_page.html:90: <template
is="dom-if" if=[[showStorageManager_]]>
quotes

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/device_page/drive_cache_dialog.js
(right):

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/drive_cache_dialog.js:10: 
nit: remove newline

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/drive_cache_dialog.js:18:
properties: {},
remove

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/device_page/storage.html (right):

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.html:8: <link rel="import"
href="/settings_page/settings_subpage.html">
unused

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.html:54: background-color:
rgb(245, 245, 245);
var(--google-grey-100)

(same for any other colors that are defined in paper-styles or
settings_vars_css.html)

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.html:114: direction: rtl;
can this not be done with flex display? e.g., justify-content: flex-end

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.html:138: <div
id="critically-low-message" class="message-area">
nit throughout: use camelCase IDs for consistency w/ rest of settings/polymer

class names stay hyphen-separated though

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/device_page/storage.js (right):

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.js:16: var
StorageSpaceState = {
namespace these in 'settings' via cr.define; see
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/site_s...

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.js:17:
STORAGE_SPACE_NORMAL: 0,
why not just NORMAL, LOW, CRITICALLY_LOW?

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.js:39: /** @private
{boolean} */
the Polymer pass for closure infers type names for simple types, so the
{boolean} isn't needed

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.js:57: /** @private
{number} */
here, though, you want to provide closure with *more* type information, since
it's an enum: @private {settings.StorageSpaceState}

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.js:65: * Timer ID for
periodical update.
nit: "periodic" :-)

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.js:74: visible_: false,
what is this used for?

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.js:128:
this.startPeriodicalUpdate_();
same, "Periodic" (but "periodically" is correct. english is weird)

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.js:145: * Handler for
tapping the "Offlien files" item.
Offline

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.js:149: // TODO(fukino):
Show a dialog of Drive cache.
should this TODO go in drive_cache_dialog?

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.js:182: this.usedRatio_ =
sizeStat.usedRatio;
So, why not just have a sizeStat_ property? You can then use
sizeStat_.usedRatio, etc. in the HTML and JS. And instead of setting
textContent, you can use data binding: [[sizeStat_.usedSize]], etc

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.js:258:
chrome.send('updateStorageInfo');
I would feel better if we checked the route here, and stopped when it doesn't
match STORAGE anymore.

In particular, if this dialog gets detached, it stops observing
currentRouteChanged_:
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/route....

Meaning I think this would continue getting called indefinitely. (We don't
currently detach hidden subpages, but we might start.)

Plus if we do the check here, we lower the risk of a bug elsewhere preventing us
from clearing the interval.

It also means we can just call window.clearInterval(this.updateTimerId_) right
here -- we don't need a separate function, checking updateTimerId_, etc. And we
can also simplify currentRouteChanged_ to just:

    if (settings.getCurrentRoute() == STORAGE)
      this.onPageShown_();

and schedule the interval there.

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/chromeos/device_storage_handler.h (right):

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/chromeos/device_storage_handler.h:26: // kept
in sync with options.StorageSpaceState in JS code.
settings

https://codereview.chromium.org/2348913002/diff/100001/chromeos/chromeos_swit...
File chromeos/chromeos_switches.cc (right):

https://codereview.chromium.org/2348913002/diff/100001/chromeos/chromeos_swit...
chromeos/chromeos_switches.cc:260: const char kEnableMdStorageManager[] =
"enable-md-storage-manager";
shouldn't we only have one of these flags?

[fukino, 2016-10-03 18:38:19]

Patchset #7 (id:120001) has been deleted

[fukino, 2016-10-03 18:40:42]

Thank you for the review!
Could you take another look?

https://codereview.chromium.org/2348913002/diff/100001/chrome/app/settings_st...
File chrome/app/settings_strings.grdp (right):

https://codereview.chromium.org/2348913002/diff/100001/chrome/app/settings_st...
chrome/app/settings_strings.grdp:2350: <message
name="IDS_SETTINGS_STORAGE_ITEM_ANDROID" desc="In Device Settings > Storage,
label for the total size size of Android apps and cache.">
On 2016/10/01 01:41:04, michaelpg wrote:
> size size

Done.

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/device_page/device_page.html (right):

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/device_page.html:50: <template
is="dom-if" if=[[showStorageManager_]]>
On 2016/10/01 01:41:04, michaelpg wrote:
> quotes around attributes

Done.
I did the same fix for stylusAllowed_ too.

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/device_page.html:90: <template
is="dom-if" if=[[showStorageManager_]]>
On 2016/10/01 01:41:04, michaelpg wrote:
> quotes

Done.

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/device_page/drive_cache_dialog.js
(right):

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/drive_cache_dialog.js:10: 
On 2016/10/01 01:41:04, michaelpg wrote:
> nit: remove newline

Done.

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/drive_cache_dialog.js:18:
properties: {},
On 2016/10/01 01:41:04, michaelpg wrote:
> remove

Done.

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/device_page/storage.html (right):

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.html:8: <link rel="import"
href="/settings_page/settings_subpage.html">
On 2016/10/01 01:41:05, michaelpg wrote:
> unused

Done.

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.html:54: background-color:
rgb(245, 245, 245);
On 2016/10/01 01:41:05, michaelpg wrote:
> var(--google-grey-100)
> 
> (same for any other colors that are defined in paper-styles or
> settings_vars_css.html)

Thanks! It turned out that --google-red-500 can also be used, so I used it too.
For other colors, I couldn't find any predefined ones. I'll ask the designer if
we can share more colors.

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.html:114: direction: rtl;
On 2016/10/01 01:41:04, michaelpg wrote:
> can this not be done with flex display? e.g., justify-content: flex-end

The tricky part is that the label "Available" should be centered if the bar area
is big enough, and it should be overflow to the left if the bar area is too
small.
I tried several ways, but I could not implement the behavior only by the
flexbox.

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.html:138: <div
id="critically-low-message" class="message-area">
On 2016/10/01 01:41:04, michaelpg wrote:
> nit throughout: use camelCase IDs for consistency w/ rest of settings/polymer
> 
> class names stay hyphen-separated though

Done. Used camelCase for critically-low-message, bar-area, and bar-labels.

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/device_page/storage.js (right):

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.js:16: var
StorageSpaceState = {
On 2016/10/01 01:41:05, michaelpg wrote:
> namespace these in 'settings' via cr.define; see
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/site_s...

As far as I tried, an enum defined in a namespace is reported as Unknown type.

cr.define('settings', function() {
  /** @enum {string} */
  var Foo = {A: 'a'};

  /** @constructor */
  function Bar() {}

  return {
    Foo: Foo,
    Bar: Bar,
  };
});

/** @type {settings.Foo} */
var foo; // This doesn't compile, saying "ERROR - Bad type annotation. Unknown
type settings.Foo"

/** @type {settings.Bar} */
var bar; // This compiles.

Maybe there is an issue in ChromePass (or I missed something)?

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.js:17:
STORAGE_SPACE_NORMAL: 0,
On 2016/10/01 01:41:05, michaelpg wrote:
> why not just NORMAL, LOW, CRITICALLY_LOW?

Done.

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.js:39: /** @private
{boolean} */
On 2016/10/01 01:41:05, michaelpg wrote:
> the Polymer pass for closure infers type names for simple types, so the
> {boolean} isn't needed

Done.

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.js:57: /** @private
{number} */
On 2016/10/01 01:41:05, michaelpg wrote:
> here, though, you want to provide closure with *more* type information, since
> it's an enum: @private {settings.StorageSpaceState}

Done.

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.js:65: * Timer ID for
periodical update.
On 2016/10/01 01:41:05, michaelpg wrote:
> nit: "periodic" :-)

Oops! Done.

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.js:74: visible_: false,
On 2016/10/01 01:41:05, michaelpg wrote:
> what is this used for?

I couldn't find how to know the previous route, so I add a variable |visible_|
to know the previous state about whether the current route was STORAGE or not.

However, thanks to your suggestion about the periodic timer, I don't need
|visible_| anymore, so I removed it.

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.js:128:
this.startPeriodicalUpdate_();
On 2016/10/01 01:41:05, michaelpg wrote:
> same, "Periodic" (but "periodically" is correct. english is weird)

Done.

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.js:145: * Handler for
tapping the "Offlien files" item.
On 2016/10/01 01:41:05, michaelpg wrote:
> Offline

Done.

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.js:149: // TODO(fukino):
Show a dialog of Drive cache.
On 2016/10/01 01:41:05, michaelpg wrote:
> should this TODO go in drive_cache_dialog?

I removed the TODO comment. (The dialog was already implemented.)

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.js:182: this.usedRatio_ =
sizeStat.usedRatio;
On 2016/10/01 01:41:05, michaelpg wrote:
> So, why not just have a sizeStat_ property? You can then use
> sizeStat_.usedRatio, etc. in the HTML and JS. And instead of setting
> textContent, you can use data binding: [[sizeStat_.usedSize]], etc

Wow nice cleanup!
I totally forgot the binding to subproperties...

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.js:258:
chrome.send('updateStorageInfo');
On 2016/10/01 01:41:05, michaelpg wrote:
> I would feel better if we checked the route here, and stopped when it doesn't
> match STORAGE anymore.
> 
> In particular, if this dialog gets detached, it stops observing
> currentRouteChanged_:
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/route....
> 
> Meaning I think this would continue getting called indefinitely. (We don't
> currently detach hidden subpages, but we might start.)
> 
> Plus if we do the check here, we lower the risk of a bug elsewhere preventing
us
> from clearing the interval.
> 
> It also means we can just call window.clearInterval(this.updateTimerId_) right
> here -- we don't need a separate function, checking updateTimerId_, etc. And
we
> can also simplify currentRouteChanged_ to just:
> 
>     if (settings.getCurrentRoute() == STORAGE)
>       this.onPageShown_();
> 
> and schedule the interval there.

Oh, I didn't understand that the dialog can be detached. Thank you for pointing
it out.
I agree with your suggestion. Checking the route insider the timer function
sounds robust. I updated the code to do so.

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/chromeos/device_storage_handler.h (right):

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/chromeos/device_storage_handler.h:26: // kept
in sync with options.StorageSpaceState in JS code.
On 2016/10/01 01:41:05, michaelpg wrote:
> settings

Done.

https://codereview.chromium.org/2348913002/diff/100001/chromeos/chromeos_swit...
File chromeos/chromeos_switches.cc (right):

https://codereview.chromium.org/2348913002/diff/100001/chromeos/chromeos_swit...
chromeos/chromeos_switches.cc:260: const char kEnableMdStorageManager[] =
"enable-md-storage-manager";
On 2016/10/01 01:41:05, michaelpg wrote:
> shouldn't we only have one of these flags?

I used ENABLE_DISABLE_VALUE_TYPE to have a default value so that we can easily
change the default setting of MD settings manager back and forth.

Is it better to use single value (e.g. enable-md-storage-manager), and when we
enable it by default we should delete the flag?

[michaelpg, 2016-10-03 23:44:45]

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/device_page/storage.js (right):

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.js:16: var
StorageSpaceState = {
On 2016/10/03 18:40:41, fukino wrote:
> On 2016/10/01 01:41:05, michaelpg wrote:
> > namespace these in 'settings' via cr.define; see
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/site_s...
> 
> As far as I tried, an enum defined in a namespace is reported as Unknown type.
> 
> cr.define('settings', function() {
>   /** @enum {string} */
>   var Foo = {A: 'a'};
> 
>   /** @constructor */
>   function Bar() {}
> 
>   return {
>     Foo: Foo,
>     Bar: Bar,
>   };
> });
> 
> /** @type {settings.Foo} */
> var foo; // This doesn't compile, saying "ERROR - Bad type annotation. Unknown
> type settings.Foo"
> 
> /** @type {settings.Bar} */
> var bar; // This compiles.
> 
> Maybe there is an issue in ChromePass (or I missed something)?

You're right, I forgot about that. I think it's a problem with Closure's type
inference system.

If you want, you can use cr.exportPath('settings'); and then define these as
settings.Foo = {} outside of the cr.define:
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/people...

But that's optional, we just use "var Foo" like you have it here in several
places because of the closure bug.

https://codereview.chromium.org/2348913002/diff/100001/chromeos/chromeos_swit...
File chromeos/chromeos_switches.cc (right):

https://codereview.chromium.org/2348913002/diff/100001/chromeos/chromeos_swit...
chromeos/chromeos_switches.cc:260: const char kEnableMdStorageManager[] =
"enable-md-storage-manager";
On 2016/10/03 18:40:42, fukino wrote:
> On 2016/10/01 01:41:05, michaelpg wrote:
> > shouldn't we only have one of these flags?
> 
> I used ENABLE_DISABLE_VALUE_TYPE to have a default value so that we can easily
> change the default setting of MD settings manager back and forth.
> 
> Is it better to use single value (e.g. enable-md-storage-manager), and when we
> enable it by default we should delete the flag?

Generally yes, is it important that we be able to disable this after shipping
it?

https://codereview.chromium.org/2348913002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/device_page/device_page.html (right):

https://codereview.chromium.org/2348913002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/device_page.html:63: <div
class="middle">$i18n{storageTitle}</div>
Rebase issue: add the icon-arrow-right button here, like you see above.

https://codereview.chromium.org/2348913002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/device_page/drive_cache_dialog.html
(right):

https://codereview.chromium.org/2348913002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/drive_cache_dialog.html:2: <link
rel="import" href="chrome://resources/html/i18n_behavior.html">
import paper-button

https://codereview.chromium.org/2348913002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/device_page/storage.html (right):

https://codereview.chromium.org/2348913002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.html:4: <link rel="import"
href="chrome://resources/polymer/v1_0/paper-progress/paper-progress.html">
import iron-icon

https://codereview.chromium.org/2348913002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.html:132:
<p>$i18n{storageSpaceLowMessageLine1}</p>
do we use <p> anywhere else? it has user-agent styles that we generally don't
conform to our styles.

https://codereview.chromium.org/2348913002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.html:143:
<p>$i18n{storageSpaceCriticallyLowMessageLine1}</p>
same

https://codereview.chromium.org/2348913002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.html:204: <script
src="storage.js"></script>
use absolute paths: /device_page/storage.js

[fukino, 2016-10-04 09:03:54]

Thank you! PTAL.

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/device_page/storage.js (right):

https://codereview.chromium.org/2348913002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.js:16: var
StorageSpaceState = {
On 2016/10/03 23:44:44, michaelpg wrote:
> On 2016/10/03 18:40:41, fukino wrote:
> > On 2016/10/01 01:41:05, michaelpg wrote:
> > > namespace these in 'settings' via cr.define; see
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/site_s...
> > 
> > As far as I tried, an enum defined in a namespace is reported as Unknown
type.
> > 
> > cr.define('settings', function() {
> >   /** @enum {string} */
> >   var Foo = {A: 'a'};
> > 
> >   /** @constructor */
> >   function Bar() {}
> > 
> >   return {
> >     Foo: Foo,
> >     Bar: Bar,
> >   };
> > });
> > 
> > /** @type {settings.Foo} */
> > var foo; // This doesn't compile, saying "ERROR - Bad type annotation.
Unknown
> > type settings.Foo"
> > 
> > /** @type {settings.Bar} */
> > var bar; // This compiles.
> > 
> > Maybe there is an issue in ChromePass (or I missed something)?
> 
> You're right, I forgot about that. I think it's a problem with Closure's type
> inference system.
> 
> If you want, you can use cr.exportPath('settings'); and then define these as
> settings.Foo = {} outside of the cr.define:
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/people...
> 
> But that's optional, we just use "var Foo" like you have it here in several
> places because of the closure bug.

Thanks!
I used cr.exportPath('settings') to keep them in settings namespace.

https://codereview.chromium.org/2348913002/diff/100001/chromeos/chromeos_swit...
File chromeos/chromeos_switches.cc (right):

https://codereview.chromium.org/2348913002/diff/100001/chromeos/chromeos_swit...
chromeos/chromeos_switches.cc:260: const char kEnableMdStorageManager[] =
"enable-md-storage-manager";
On 2016/10/03 23:44:44, michaelpg wrote:
> On 2016/10/03 18:40:42, fukino wrote:
> > On 2016/10/01 01:41:05, michaelpg wrote:
> > > shouldn't we only have one of these flags?
> > 
> > I used ENABLE_DISABLE_VALUE_TYPE to have a default value so that we can
easily
> > change the default setting of MD settings manager back and forth.
> > 
> > Is it better to use single value (e.g. enable-md-storage-manager), and when
we
> > enable it by default we should delete the flag?
> 
> Generally yes, is it important that we be able to disable this after shipping
> it?

Got it. I updated the CL to use a single value flag to enable md storage
manager.

https://codereview.chromium.org/2348913002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/device_page/device_page.html (right):

https://codereview.chromium.org/2348913002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/device_page.html:63: <div
class="middle">$i18n{storageTitle}</div>
On 2016/10/03 23:44:45, michaelpg wrote:
> Rebase issue: add the icon-arrow-right button here, like you see above.

Done.

https://codereview.chromium.org/2348913002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/device_page/drive_cache_dialog.html
(right):

https://codereview.chromium.org/2348913002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/drive_cache_dialog.html:2: <link
rel="import" href="chrome://resources/html/i18n_behavior.html">
On 2016/10/03 23:44:45, michaelpg wrote:
> import paper-button

Done.

https://codereview.chromium.org/2348913002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/settings/device_page/storage.html (right):

https://codereview.chromium.org/2348913002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.html:4: <link rel="import"
href="chrome://resources/polymer/v1_0/paper-progress/paper-progress.html">
On 2016/10/03 23:44:45, michaelpg wrote:
> import iron-icon

Done.

https://codereview.chromium.org/2348913002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.html:132:
<p>$i18n{storageSpaceLowMessageLine1}</p>
On 2016/10/03 23:44:45, michaelpg wrote:
> do we use <p> anywhere else? it has user-agent styles that we generally don't
> conform to our styles.

I replaced these <p>s with <div>s, and set the same margin for those <div>s.

https://codereview.chromium.org/2348913002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.html:143:
<p>$i18n{storageSpaceCriticallyLowMessageLine1}</p>
On 2016/10/03 23:44:45, michaelpg wrote:
> same

Done.

https://codereview.chromium.org/2348913002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/settings/device_page/storage.html:204: <script
src="storage.js"></script>
On 2016/10/03 23:44:45, michaelpg wrote:
> use absolute paths: /device_page/storage.js

Done.

[fukino, 2016-10-04 09:04:52]

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

[fukino, 2016-10-04 09:04:52]

isherman@: PTAL at histograms.xml. Thanks!

[Ilya Sherman, 2016-10-05 00:32:08]

histograms.xml lgtm

[michaelpg, 2016-10-05 19:13:36]

lgtm

[fukino, 2016-10-05 22:57:46]

Thank you!

[fukino, 2016-10-05 22:57:52]

The CQ bit was checked by fukino@chromium.org

[commit-bot: I haz the power, 2016-10-05 22:58:17]

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-10-06 01:44:32]

Description was changed from

==========
Port storage manager to MD settings.

This CL adds an "Storage management" item on MD settings/Device, and it shows
the size of each storage item.
C++ side implementation is basically copied from old setting's one.
(I don't share the implementation assuming that the old one will be removed.)

Following are not implemented.
- The UI design is temporary.
- metrics/tests will be added separately.

BUG=629438
TEST=with the flag "--enable-md-storage-manager=1",  manually tested that
chrome://md-settings/storage shows the same items/values with
chrome://settings/storage
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Port storage manager to MD settings.

This CL adds an "Storage management" item on MD settings/Device, and it shows
the size of each storage item.
C++ side implementation is basically copied from old setting's one.
(I don't share the implementation assuming that the old one will be removed.)

Following are not implemented.
- The UI design is temporary.
- metrics/tests will be added separately.

BUG=629438
TEST=with the flag "--enable-md-storage-manager=1",  manually tested that
chrome://md-settings/storage shows the same items/values with
chrome://settings/storage
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-10-06 01:44:34]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2016-10-06 01:47:08]

Description was changed from

==========
Port storage manager to MD settings.

This CL adds an "Storage management" item on MD settings/Device, and it shows
the size of each storage item.
C++ side implementation is basically copied from old setting's one.
(I don't share the implementation assuming that the old one will be removed.)

Following are not implemented.
- The UI design is temporary.
- metrics/tests will be added separately.

BUG=629438
TEST=with the flag "--enable-md-storage-manager=1",  manually tested that
chrome://md-settings/storage shows the same items/values with
chrome://settings/storage
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Port storage manager to MD settings.

This CL adds an "Storage management" item on MD settings/Device, and it shows
the size of each storage item.
C++ side implementation is basically copied from old setting's one.
(I don't share the implementation assuming that the old one will be removed.)

Following are not implemented.
- The UI design is temporary.
- metrics/tests will be added separately.

BUG=629438
TEST=with the flag "--enable-md-storage-manager=1",  manually tested that
chrome://md-settings/storage shows the same items/values with
chrome://settings/storage
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/519e0e2c9424ec070416451b0254123bdc125545
Cr-Commit-Position: refs/heads/master@{#423386}
==========

[commit-bot: I haz the power, 2016-10-06 01:47:09]

Patchset 8 (id:??) landed as
https://crrev.com/519e0e2c9424ec070416451b0254123bdc125545
Cr-Commit-Position: refs/heads/master@{#423386}