Show a local folder's size in QuickView.

Show a local folder's size in QuickView.

BUG=474318
TEST=none

Committed: https://crrev.com/2921d494e124e896f15b7b5ba8c6bbd8de965636
Cr-Commit-Position: refs/heads/master@{#416618}

[harukam, 2016-09-06 03:50:56]

harukam@google.com changed reviewers:
+ fukino@chromium.org

[harukam, 2016-09-06 03:50:57]

PTAL

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

https://codereview.chromium.org/2313763003/diff/1/ui/file_manager/file_manage...
File ui/file_manager/file_manager/foreground/elements/files_metadata_box.js
(right):

https://codereview.chromium.org/2313763003/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/elements/files_metadata_box.js:23:
loading: Boolean,
'loading' sounds like all the properties are loading.
How about using 'laodingSize' or 'isSizeLoading'?

https://codereview.chromium.org/2313763003/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/elements/files_metadata_box.js:97:
setLoading: function(loading) {
You can simply call, for example, 'metadataBox_.loading = true'

https://codereview.chromium.org/2313763003/diff/1/ui/file_manager/file_manage...
File ui/file_manager/file_manager/foreground/elements/files_metadata_entry.html
(right):

https://codereview.chromium.org/2313763003/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/elements/files_metadata_entry.html:46:
.loading {

instead of the '.loading' class, #value[loading] should work.

https://codereview.chromium.org/2313763003/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/elements/files_metadata_entry.html:51:
overflow: hidden;
Sort properties.

https://codereview.chromium.org/2313763003/diff/1/ui/file_manager/file_manage...
File ui/file_manager/file_manager/foreground/elements/files_metadata_entry.js
(right):

https://codereview.chromium.org/2313763003/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/elements/files_metadata_entry.js:31:
setLoadingClass: function(loading) {
This function looks unnecessary.

https://codereview.chromium.org/2313763003/diff/1/ui/file_manager/file_manage...
File ui/file_manager/file_manager/foreground/js/metadata_box_controller.js
(right):

https://codereview.chromium.org/2313763003/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/metadata_box_controller.js:44: *
@type {Promise.<number>}
We don't need '.'. Use Promise<number>

https://codereview.chromium.org/2313763003/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/metadata_box_controller.js:115:
this.setDirectorySize_(new Promise(function(resolve, reject) {
I think it is better to simply call this.setDirectorySize_(entry) and move the
implementation inside setDirectorySize_.

https://codereview.chromium.org/2313763003/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/metadata_box_controller.js:178:
this.directorySizePromise_ = directorySizePromise;
If you avoid showing the size of the wrong entry, I think keeping track of Entry
instead of the promise will be easier to understand.

[harukam, 2016-09-06 11:25:24]

Thanks for the review, Fukino-san.

Please take another look.

https://codereview.chromium.org/2313763003/diff/1/ui/file_manager/file_manage...
File ui/file_manager/file_manager/foreground/elements/files_metadata_box.js
(right):

https://codereview.chromium.org/2313763003/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/elements/files_metadata_box.js:23:
loading: Boolean,
On 2016/09/06 09:12:44, fukino wrote:
> 'loading' sounds like all the properties are loading.
> How about using 'laodingSize' or 'isSizeLoading'?

Agree. I like 'isSizeLoading' better.

https://codereview.chromium.org/2313763003/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/elements/files_metadata_box.js:97:
setLoading: function(loading) {
On 2016/09/06 09:12:44, fukino wrote:
> You can simply call, for example, 'metadataBox_.loading = true' 

Acknowledged.

https://codereview.chromium.org/2313763003/diff/1/ui/file_manager/file_manage...
File ui/file_manager/file_manager/foreground/elements/files_metadata_entry.html
(right):

https://codereview.chromium.org/2313763003/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/elements/files_metadata_entry.html:46:
.loading {
On 2016/09/06 09:12:45, fukino wrote:
> 
> instead of the '.loading' class, #value[loading] should work.

Thanks.

https://codereview.chromium.org/2313763003/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/elements/files_metadata_entry.html:51:
overflow: hidden;
On 2016/09/06 09:12:45, fukino wrote:
> Sort properties.

Done.

https://codereview.chromium.org/2313763003/diff/1/ui/file_manager/file_manage...
File ui/file_manager/file_manager/foreground/elements/files_metadata_entry.js
(right):

https://codereview.chromium.org/2313763003/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/elements/files_metadata_entry.js:31:
setLoadingClass: function(loading) {
On 2016/09/06 09:12:45, fukino wrote:
> This function looks unnecessary.

Acknowledged.

https://codereview.chromium.org/2313763003/diff/1/ui/file_manager/file_manage...
File ui/file_manager/file_manager/foreground/js/metadata_box_controller.js
(right):

https://codereview.chromium.org/2313763003/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/metadata_box_controller.js:44: *
@type {Promise.<number>}
On 2016/09/06 09:12:45, fukino wrote:
> We don't need '.'. Use Promise<number>

Acknowledged.

https://codereview.chromium.org/2313763003/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/metadata_box_controller.js:115:
this.setDirectorySize_(new Promise(function(resolve, reject) {
On 2016/09/06 09:12:45, fukino wrote:
> I think it is better to simply call this.setDirectorySize_(entry) and move the
> implementation inside setDirectorySize_.

OK. Thanks.

https://codereview.chromium.org/2313763003/diff/1/ui/file_manager/file_manage...
ui/file_manager/file_manager/foreground/js/metadata_box_controller.js:178:
this.directorySizePromise_ = directorySizePromise;
On 2016/09/06 09:12:45, fukino wrote:
> If you avoid showing the size of the wrong entry, I think keeping track of
Entry
> instead of the promise will be easier to understand. 

Acknowledged.

[fukino, 2016-09-06 11:30:59]

lgtm with a nit. Thanks!

https://codereview.chromium.org/2313763003/diff/40001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/metadata_box_controller.js
(right):

https://codereview.chromium.org/2313763003/diff/40001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/metadata_box_controller.js:179:
}.bind(this));
indent level should be same as function(size)

[harukam, 2016-09-06 11:35:09]

The CQ bit was checked by harukam@google.com

[harukam, 2016-09-06 11:35:09]

The patchset sent to the CQ was uploaded after l-g-t-m from fukino@chromium.org
Link to the patchset: https://codereview.chromium.org/2313763003/#ps60001
(title: "Show a local folder's size in QuickView.")

[commit-bot: I haz the power, 2016-09-06 11:35:18]

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-09-06 12:19:57]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-06 12:22:02]

Description was changed from

==========
Show a local folder's size in QuickView.

BUG=474318
TEST=none
==========

to

==========
Show a local folder's size in QuickView.

BUG=474318
TEST=none

Committed: https://crrev.com/2921d494e124e896f15b7b5ba8c6bbd8de965636
Cr-Commit-Position: refs/heads/master@{#416618}
==========

[commit-bot: I haz the power, 2016-09-06 12:22:05]

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