Fix trivial type-check errors in file_manager.

Fix trivial type-check errors in file_manager.

Changes in this CL are many, but they are basically about type annotations.
The actual code doesn't change substantially.
Changes which affect actual code will be separate CLs.

BUG=406995
TEST=run browser_tests

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

[fukino, 2014-10-15 09:25:02]

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

[fukino, 2014-10-15 09:25:04]

hirono@, could you take a look? Thanks!

https://codereview.chromium.org/651403002/diff/2/ui/file_manager/file_manager...
File ui/file_manager/file_manager/common/js/externs.js (right):

https://codereview.chromium.org/651403002/diff/2/ui/file_manager/file_manager...
ui/file_manager/file_manager/common/js/externs.js:9: var BackgroundWindow =
function() {};
This interface is for defining global properties in window of background page.

https://codereview.chromium.org/651403002/diff/2/ui/file_manager/file_manager...
ui/file_manager/file_manager/common/js/externs.js:26: var
FileOperationProgressEvent = function() {};
We can make customized Event object and dispatch it, but for now I defined a
interface here not to change the actual code.

https://codereview.chromium.org/651403002/diff/2/ui/file_manager/file_manager...
File ui/file_manager/file_manager/foreground/js/compiled_resources.gyp (right):

https://codereview.chromium.org/651403002/diff/2/ui/file_manager/file_manager...
ui/file_manager/file_manager/foreground/js/compiled_resources.gyp:7:
'target_name': 'main',
After fixing circular dependency between foreground and background, I'll revert
this target to main_scripts and remove scripts described in main_scripts from
following 'depends' section.

[hirono, 2014-10-15 09:40:35]

Up to file_manager.js.

https://codereview.chromium.org/651403002/diff/2/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/device_handler.js (right):

https://codereview.chromium.org/651403002/diff/2/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/device_handler.js:163: * @param
{(string|undefined)} devicePath Device path.
Should it need to accept undefined?

https://codereview.chromium.org/651403002/diff/2/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/file_operation_handler.js
(right):

https://codereview.chromium.org/651403002/diff/2/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/file_operation_handler.js:70: *
@param {Event} event Progress event.
Can we change the argument type instead of cast?

https://codereview.chromium.org/651403002/diff/2/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/volume_manager.js (right):

https://codereview.chromium.org/651403002/diff/2/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/volume_manager.js:113: * @return
{(string|undefined)} Error identifier.
string cannot be nullable?

https://codereview.chromium.org/651403002/diff/2/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/volume_manager.js:462:
this.driveConnectionState_ = {
Can we add JSDoc for the property?

[fukino, 2014-10-15 11:46:02]

https://codereview.chromium.org/651403002/diff/2/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/device_handler.js (right):

https://codereview.chromium.org/651403002/diff/2/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/device_handler.js:163: * @param
{(string|undefined)} devicePath Device path.
On 2014/10/15 09:40:34, hirono wrote:
> Should it need to accept undefined?

For now, this devicePath can be undefined (because event.undefined in
onDeviceChange_ is optional), and I reflected it in the type annotation.
When the device path is undefined, notification's id will be '***undefined'.
This may be unintended behavior.
I'm keeping the actual code as is, but we can fix it by inserting null check.
Should I do it in this CL?

https://codereview.chromium.org/651403002/diff/2/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/file_operation_handler.js
(right):

https://codereview.chromium.org/651403002/diff/2/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/file_operation_handler.js:70: *
@param {Event} event Progress event.
On 2014/10/15 09:40:34, hirono wrote:
> Can we change the argument type instead of cast?

Oh, this cast was unnecessary. Thanks! Done.

https://codereview.chromium.org/651403002/diff/2/ui/file_manager/file_manager...
File ui/file_manager/file_manager/background/js/volume_manager.js (right):

https://codereview.chromium.org/651403002/diff/2/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/volume_manager.js:113: * @return
{(string|undefined)} Error identifier.
On 2014/10/15 09:40:34, hirono wrote:
> string cannot be nullable?

string cannot be null, but this.error_ can be undefined (when there's no error).

https://codereview.chromium.org/651403002/diff/2/ui/file_manager/file_manager...
ui/file_manager/file_manager/background/js/volume_manager.js:462:
this.driveConnectionState_ = {
On 2014/10/15 09:40:34, hirono wrote:
> Can we add JSDoc for the property?

Done.
I defined shape of the object as VolumeManagerCommon.DriveConnectionState. It's
defined in common/js/volume_manager_common.js

[hirono, 2014-10-16 04:36:51]

https://codereview.chromium.org/651403002/diff/30001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/ui/drive_banners.js (right):

https://codereview.chromium.org/651403002/diff/30001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/ui/drive_banners.js:607: * Create an
element using given parameters.
nit: Create -> Creates

https://codereview.chromium.org/651403002/diff/30001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/ui/scrollbar.js (right):

https://codereview.chromium.org/651403002/diff/30001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/ui/scrollbar.js:63: * @param {Node}
parent Parent node, must have a relative or absolute
Why should it handle Node except for Element?

https://codereview.chromium.org/651403002/diff/30001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/ui/share_dialog.js (right):

https://codereview.chromium.org/651403002/diff/30001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/ui/share_dialog.js:177: * @param
{(Function|null)=} opt_onHide Called when the dialog is hidden.
Should it handle null?

[fukino, 2014-10-16 05:17:13]

https://codereview.chromium.org/651403002/diff/30001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/ui/drive_banners.js (right):

https://codereview.chromium.org/651403002/diff/30001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/ui/drive_banners.js:607: * Create an
element using given parameters.
On 2014/10/16 04:36:51, hirono wrote:
> nit: Create -> Creates

Done.

https://codereview.chromium.org/651403002/diff/30001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/ui/scrollbar.js (right):

https://codereview.chromium.org/651403002/diff/30001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/ui/scrollbar.js:63: * @param {Node}
parent Parent node, must have a relative or absolute
On 2014/10/16 04:36:51, hirono wrote:
> Why should it handle Node except for Element?

In this function, the only use of the parameter |parent| is
'parent.appendChild()'. It is a member of Node interface, so Node is sufficient
to specify parameter type.

Actually, in directory_tree.js, there is a code
'this.scrollBar_.initialize(this.parentNode, this)', in which the
|this.parentNode| is Node.

https://codereview.chromium.org/651403002/diff/30001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/ui/share_dialog.js (right):

https://codereview.chromium.org/651403002/diff/30001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/ui/share_dialog.js:177: * @param
{(Function|null)=} opt_onHide Called when the dialog is hidden.
On 2014/10/16 04:36:51, hirono wrote:
> Should it handle null?

Done.
The type {(Function|null)=} was inherited from FileManagerDialogBase, but the
cr.ui.dialog.BaseDialog has type {Function=} for its hide() member. I changed
the hide()'s type to {Function=} in ShareDialog, SuggestAppDialog, and
FileManagerBrowserBase.

[hirono, 2014-10-16 06:13:00]

https://codereview.chromium.org/651403002/diff/30001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/ui/scrollbar.js (right):

https://codereview.chromium.org/651403002/diff/30001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/ui/scrollbar.js:63: * @param {Node}
parent Parent node, must have a relative or absolute
On 2014/10/16 05:17:13, fukino wrote:
> On 2014/10/16 04:36:51, hirono wrote:
> > Why should it handle Node except for Element?
> 
> In this function, the only use of the parameter |parent| is
> 'parent.appendChild()'. It is a member of Node interface, so Node is
sufficient
> to specify parameter type.
> 
> Actually, in directory_tree.js, there is a code
> 'this.scrollBar_.initialize(this.parentNode, this)', in which the
> |this.parentNode| is Node. 

I think Element is better here. Attaching the scroll bar to Node except for
Element does not make sense (e.g. TextNode).
Maybe 'this.scrollBar_.initialize(this.parentNode, this)' should be
'this.scrollBar_.initialize(this.parentElement, this)'.
WDYT?

https://codereview.chromium.org/651403002/diff/50001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/ui/drive_banners.js (right):

https://codereview.chromium.org/651403002/diff/50001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/ui/drive_banners.js:233:
opt_sizeStats.totalSize && // Quota had not changed
Can we ensure opt_sizeStats is not undefined here?

https://codereview.chromium.org/651403002/diff/50001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/ui/file_manager_dialog_base.js
(right):

https://codereview.chromium.org/651403002/diff/50001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/ui/file_manager_dialog_base.js:111: *
@param {(Function|null)=} opt_onHide Called when the dialog is hidden.
Can we remove the null?

[fukino, 2014-10-16 06:35:07]

https://codereview.chromium.org/651403002/diff/30001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/ui/scrollbar.js (right):

https://codereview.chromium.org/651403002/diff/30001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/ui/scrollbar.js:63: * @param {Node}
parent Parent node, must have a relative or absolute
On 2014/10/16 06:13:00, hirono wrote:
> On 2014/10/16 05:17:13, fukino wrote:
> > On 2014/10/16 04:36:51, hirono wrote:
> > > Why should it handle Node except for Element?
> > 
> > In this function, the only use of the parameter |parent| is
> > 'parent.appendChild()'. It is a member of Node interface, so Node is
> sufficient
> > to specify parameter type.
> > 
> > Actually, in directory_tree.js, there is a code
> > 'this.scrollBar_.initialize(this.parentNode, this)', in which the
> > |this.parentNode| is Node. 
> 
> I think Element is better here. Attaching the scroll bar to Node except for
> Element does not make sense (e.g. TextNode).
> Maybe 'this.scrollBar_.initialize(this.parentNode, this)' should be
> 'this.scrollBar_.initialize(this.parentElement, this)'.
> WDYT?

Oh, I didn't know the Node.parentElement. I strongly agree with you! Done.

https://codereview.chromium.org/651403002/diff/50001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/ui/drive_banners.js (right):

https://codereview.chromium.org/651403002/diff/50001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/ui/drive_banners.js:233:
opt_sizeStats.totalSize && // Quota had not changed
On 2014/10/16 06:13:00, hirono wrote:
> Can we ensure opt_sizeStats is not undefined here?

Done.

https://codereview.chromium.org/651403002/diff/50001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/ui/file_manager_dialog_base.js
(right):

https://codereview.chromium.org/651403002/diff/50001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/ui/file_manager_dialog_base.js:111: *
@param {(Function|null)=} opt_onHide Called when the dialog is hidden.
On 2014/10/16 06:13:00, hirono wrote:
> Can we remove the null?

Oops, I failed to save the latest file. Done.

[hirono, 2014-10-16 07:24:47]

lgtm, thanks!

https://codereview.chromium.org/651403002/diff/70001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/ui/scrollbar.js (right):

https://codereview.chromium.org/651403002/diff/70001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/ui/scrollbar.js:63: * @param
{Element} parent Parent node, must have a relative or absolute
nit: Parent node -> Parent element

[fukino, 2014-10-16 07:43:43]

Thanks!

https://codereview.chromium.org/651403002/diff/70001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/ui/scrollbar.js (right):

https://codereview.chromium.org/651403002/diff/70001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/ui/scrollbar.js:63: * @param
{Element} parent Parent node, must have a relative or absolute
On 2014/10/16 07:24:47, hirono wrote:
> nit: Parent node -> Parent element

Done.

[fukino, 2014-10-16 07:43:56]

The CQ bit was checked by fukino@chromium.org

[commit-bot: I haz the power, 2014-10-16 07:44:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/651403002/90001

[commit-bot: I haz the power, 2014-10-16 08:13:33]

Committed patchset #6 (id:90001)

[commit-bot: I haz the power, 2014-10-16 08:14:18]

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