Let the description text in the preview panels managed by PreviewPanel class.

Let the description text in the preview panels managed by PreviewPanel class.

Originally the PreviewPanel class managed only the visibility of the preview
panel.

BUG=241693
TEST=manually
R=yoshiki@chromium.org

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

[hirono, 2013-09-06 06:22:22]

Could you take a look the CL?
Thank you very much!

[yoshiki, 2013-09-06 06:56:03]

https://codereview.chromium.org/23462020/diff/3001/chrome/browser/resources/f...
File chrome/browser/resources/file_manager/js/ui/preview_panel.js (right):

https://codereview.chromium.org/23462020/diff/3001/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/ui/preview_panel.js:126: set
selection(selection) {
It should be a method like setSelection(selectio) rather than setter, since this
not only just simply set the data, but also modify other related-data.

https://codereview.chromium.org/23462020/diff/3001/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/ui/preview_panel.js:270: * Animating
colculating label.
s/colculating/calculating/

And please describe what this class represents, and what this class do.
"CalculatingLabel" (or "CalculatingSizeLabel") is somewhat misleading name I
think, so we'd be better to support it with the comment.

https://codereview.chromium.org/23462020/diff/3001/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/ui/preview_panel.js:274: var
CalculatingLabel = function(element) {
This should be move to PreviewPanel class like "PreviewPanel.CalculatingLabel",
since this class is used only from PreviewPanel.

https://codereview.chromium.org/23462020/diff/3001/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/ui/preview_panel.js:274: var
CalculatingLabel = function(element) {
How about renaming it to "CalculatingSizeLabel"? Just "CalculatingLabel" is
somewhat ambiguous.

[hirono, 2013-09-09 07:15:06]

Thanks!

https://codereview.chromium.org/23462020/diff/3001/chrome/browser/resources/f...
File chrome/browser/resources/file_manager/js/ui/preview_panel.js (right):

https://codereview.chromium.org/23462020/diff/3001/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/ui/preview_panel.js:126: set
selection(selection) {
On 2013/09/06 06:56:03, yoshiki wrote:
> It should be a method like setSelection(selectio) rather than setter, since
this
> not only just simply set the data, but also modify other related-data.

Done.

https://codereview.chromium.org/23462020/diff/3001/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/ui/preview_panel.js:270: * Animating
colculating label.
On 2013/09/06 06:56:03, yoshiki wrote:
> s/colculating/calculating/
> 
> And please describe what this class represents, and what this class do.
> "CalculatingLabel" (or "CalculatingSizeLabel") is somewhat misleading name I
> think, so we'd be better to support it with the comment.

Done.

https://codereview.chromium.org/23462020/diff/3001/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/ui/preview_panel.js:274: var
CalculatingLabel = function(element) {
On 2013/09/06 06:56:03, yoshiki wrote:
> How about renaming it to "CalculatingSizeLabel"? Just "CalculatingLabel" is
> somewhat ambiguous.

Done.

https://codereview.chromium.org/23462020/diff/3001/chrome/browser/resources/f...
chrome/browser/resources/file_manager/js/ui/preview_panel.js:274: var
CalculatingLabel = function(element) {
On 2013/09/06 06:56:03, yoshiki wrote:
> This should be move to PreviewPanel class like
"PreviewPanel.CalculatingLabel",
> since this class is used only from PreviewPanel.

Done.

[yoshiki, 2013-09-09 08:28:12]

lgtm with nits

https://codereview.chromium.org/23462020/diff/12001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/ui/preview_panel.js (right):

https://codereview.chromium.org/23462020/diff/12001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/ui/preview_panel.js:231: var text = '';
nit: no need to set the default value.

https://codereview.chromium.org/23462020/diff/12001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/ui/preview_panel.js:290: * Period in
milliseconds.
Just "Period" is ambiguous for me. Either "Timer period" or "Timer interval" is
better.

https://codereview.chromium.org/23462020/diff/12001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/ui/preview_panel.js:320: /**
nit: Please add an brief comment. The function name 'onStep' itself is
ambiguous.

[hirono, 2013-09-09 08:32:29]

Thanks!

https://codereview.chromium.org/23462020/diff/12001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/ui/preview_panel.js (right):

https://codereview.chromium.org/23462020/diff/12001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/ui/preview_panel.js:231: var text = '';
On 2013/09/09 08:28:13, yoshiki wrote:
> nit: no need to set the default value.

Done.

https://codereview.chromium.org/23462020/diff/12001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/ui/preview_panel.js:290: * Period in
milliseconds.
On 2013/09/09 08:28:13, yoshiki wrote:
> Just "Period" is ambiguous for me. Either "Timer period" or "Timer interval"
is
> better.

Done.

https://codereview.chromium.org/23462020/diff/12001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/ui/preview_panel.js:320: /**
On 2013/09/09 08:28:13, yoshiki wrote:
> nit: Please add an brief comment. The function name 'onStep' itself is
> ambiguous.

Done.

[commit-bot: I haz the power, 2013-09-09 08:32:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hirono@chromium.org/23462020/23001

[hirono, 2013-09-09 10:17:01]

Committed patchset #4 manually as r221997 (presubmit successful).