Turn on verbose flag for compiling file_manager.

Turn on verbose flag for compiling file_manager.

BUG=543416
TEST=run closure compiler

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

[fukino, 2015-10-15 09:01:07]

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

[fukino, 2015-10-15 09:01:08]

yawano@: PTAL at ui/file_manager
dbeam@: PTAL at ui/webui

https://codereview.chromium.org/1408533002/diff/20001/ui/file_manager/gallery...
File ui/file_manager/gallery/js/gallery_item.js (right):

https://codereview.chromium.org/1408533002/diff/20001/ui/file_manager/gallery...
ui/file_manager/gallery/js/gallery_item.js:16: function GalleryItem(
Gallery.Item and Gallery had circular dependency. Gallery.Item needs predefined
Gallery, and Gallery uses definition of Gallery.Item.
I renamed Gallery.Item to GalleryItem to keep these two files separated.

[yawano, 2015-10-15 11:09:09]

https://codereview.chromium.org/1408533002/diff/20001/ui/file_manager/audio_p...
File ui/file_manager/audio_player/js/compiled_resources.gyp (right):

https://codereview.chromium.org/1408533002/diff/20001/ui/file_manager/audio_p...
ui/file_manager/audio_player/js/compiled_resources.gyp:97: ],
Why don't you enable closure_args:warning_level=VERBOSE for foreground page?

https://codereview.chromium.org/1408533002/diff/20001/ui/file_manager/gallery...
File ui/file_manager/gallery/js/compiled_resources.gyp (right):

https://codereview.chromium.org/1408533002/diff/20001/ui/file_manager/gallery...
ui/file_manager/gallery/js/compiled_resources.gyp:147: ],
Same with compiled_resources.gyp of audio_player.

https://codereview.chromium.org/1408533002/diff/20001/ui/file_manager/video_p...
File ui/file_manager/video_player/js/compiled_resources.gyp (right):

https://codereview.chromium.org/1408533002/diff/20001/ui/file_manager/video_p...
ui/file_manager/video_player/js/compiled_resources.gyp:106: ],
Same with compiled_resources.gyp of audio_player.

[fukino, 2015-10-15 11:23:54]

Thank you!

https://codereview.chromium.org/1408533002/diff/20001/ui/file_manager/audio_p...
File ui/file_manager/audio_player/js/compiled_resources.gyp (right):

https://codereview.chromium.org/1408533002/diff/20001/ui/file_manager/audio_p...
ui/file_manager/audio_player/js/compiled_resources.gyp:97: ],
On 2015/10/15 11:09:09, yawano wrote:
> Why don't you enable closure_args:warning_level=VERBOSE for foreground page?

Oops, I missed it. Done.

https://codereview.chromium.org/1408533002/diff/20001/ui/file_manager/gallery...
File ui/file_manager/gallery/js/compiled_resources.gyp (right):

https://codereview.chromium.org/1408533002/diff/20001/ui/file_manager/gallery...
ui/file_manager/gallery/js/compiled_resources.gyp:147: ],
On 2015/10/15 11:09:09, yawano wrote:
> Same with compiled_resources.gyp of audio_player.

Done.

https://codereview.chromium.org/1408533002/diff/20001/ui/file_manager/video_p...
File ui/file_manager/video_player/js/compiled_resources.gyp (right):

https://codereview.chromium.org/1408533002/diff/20001/ui/file_manager/video_p...
ui/file_manager/video_player/js/compiled_resources.gyp:106: ],
On 2015/10/15 11:09:09, yawano wrote:
> Same with compiled_resources.gyp of audio_player.

Done.

[yawano, 2015-10-15 11:26:44]

lgtm with small nit. Thank you!

https://codereview.chromium.org/1408533002/diff/40001/ui/file_manager/audio_p...
File ui/file_manager/audio_player/js/compiled_resources.gyp (right):

https://codereview.chromium.org/1408533002/diff/40001/ui/file_manager/audio_p...
ui/file_manager/audio_player/js/compiled_resources.gyp:100: ]
Small nit: In gyp file, we add "," even if it's last element. Please also check
other gyp files.

[fukino, 2015-10-15 11:45:40]

Thank you!

https://codereview.chromium.org/1408533002/diff/40001/ui/file_manager/audio_p...
File ui/file_manager/audio_player/js/compiled_resources.gyp (right):

https://codereview.chromium.org/1408533002/diff/40001/ui/file_manager/audio_p...
ui/file_manager/audio_player/js/compiled_resources.gyp:100: ]
On 2015/10/15 11:26:43, yawano wrote:
> Small nit: In gyp file, we add "," even if it's last element. Please also
check
> other gyp files.

Done.

[Dan Beam, 2015-10-16 07:15:02]

https://codereview.chromium.org/1408533002/diff/60001/ui/file_manager/audio_p...
File ui/file_manager/audio_player/js/compiled_resources.gyp (right):

https://codereview.chromium.org/1408533002/diff/60001/ui/file_manager/audio_p...
ui/file_manager/audio_player/js/compiled_resources.gyp:45: ],
please make 1 ui/file_manager/compile_js.gypi with these args and use it instead
of third_party/closure_compiler/compile_js.gypi

see remoting/compile_js.gypi for an example

[Dan Beam, 2015-10-16 07:20:48]

see also:
https://codereview.chromium.org/1409843002/

sorry, i didn't see your CL first

https://codereview.chromium.org/1408533002/diff/60001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/common/js/volume_manager_common.js (right):

https://codereview.chromium.org/1408533002/diff/60001/ui/file_manager/file_ma...
ui/file_manager/file_manager/common/js/volume_manager_common.js:179: return
VolumeManagerCommon.VolumeType.UNKNOWN;
this shouldn't be necessary with my recent change

https://codereview.chromium.org/1408533002/diff/60001/ui/file_manager/gallery...
File ui/file_manager/gallery/js/image_editor/image_encoder.js (right):

https://codereview.chromium.org/1408533002/diff/60001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_encoder.js:223: return
assert(mimeType);
return assert(metadata.mediaMimeType || metadata.contentMimeType);

https://codereview.chromium.org/1408533002/diff/60001/ui/file_manager/gallery...
File ui/file_manager/gallery/js/image_editor/image_transform.js (right):

https://codereview.chromium.org/1408533002/diff/60001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_transform.js:85: this.domOverlay_
= assertInstanceof(doc.createElement('div'), HTMLDivElement);
why do these need to be HTMLDivElement rather than HTMLElement?  do you use
this.domOverlay_.align somewhere?

https://codereview.chromium.org/1408533002/diff/60001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr/ui/table.js (right):

https://codereview.chromium.org/1408533002/diff/60001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/table.js:224: return
this.list_.scrollIndexIntoView(i);
nobody actually uses this, afaict

[fukino, 2015-10-19 09:55:14]

Thank you Dan!
Could you take another look?

https://codereview.chromium.org/1408533002/diff/60001/ui/file_manager/audio_p...
File ui/file_manager/audio_player/js/compiled_resources.gyp (right):

https://codereview.chromium.org/1408533002/diff/60001/ui/file_manager/audio_p...
ui/file_manager/audio_player/js/compiled_resources.gyp:45: ],
On 2015/10/16 07:15:02, Dan Beam wrote:
> please make 1 ui/file_manager/compile_js.gypi with these args and use it
instead
> of third_party/closure_compiler/compile_js.gypi
> 
> see remoting/compile_js.gypi for an example

I didn't know how to share the compile option. Thanks!
Done.

https://codereview.chromium.org/1408533002/diff/60001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/common/js/volume_manager_common.js (right):

https://codereview.chromium.org/1408533002/diff/60001/ui/file_manager/file_ma...
ui/file_manager/file_manager/common/js/volume_manager_common.js:179: return
VolumeManagerCommon.VolumeType.UNKNOWN;
On 2015/10/16 07:20:47, Dan Beam wrote:
> this shouldn't be necessary with my recent change

Reverted adding UNKNOWN. Thank you for taking care of assertNotReached()!

https://codereview.chromium.org/1408533002/diff/60001/ui/file_manager/gallery...
File ui/file_manager/gallery/js/image_editor/image_encoder.js (right):

https://codereview.chromium.org/1408533002/diff/60001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_encoder.js:223: return
assert(mimeType);
On 2015/10/16 07:20:47, Dan Beam wrote:
> return assert(metadata.mediaMimeType || metadata.contentMimeType);

Done.

https://codereview.chromium.org/1408533002/diff/60001/ui/file_manager/gallery...
File ui/file_manager/gallery/js/image_editor/image_transform.js (right):

https://codereview.chromium.org/1408533002/diff/60001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_transform.js:85: this.domOverlay_
= assertInstanceof(doc.createElement('div'), HTMLDivElement);
On 2015/10/16 07:20:47, Dan Beam wrote:
> why do these need to be HTMLDivElement rather than HTMLElement?  do you use
> this.domOverlay_.align somewhere?

As we call createElement('div'), we expect the returned element is
HTMLDivElement. In general, should we use HTMLElement as long as we don't touch
specific properties of descendent classes.

https://codereview.chromium.org/1408533002/diff/60001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr/ui/table.js (right):

https://codereview.chromium.org/1408533002/diff/60001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/table.js:224: return
this.list_.scrollIndexIntoView(i);
On 2015/10/16 07:20:48, Dan Beam wrote:
> nobody actually uses this, afaict

Reverted this change.
I also removed return value from ui/webui/resources/js/cr/ui/list.js.

[Dan Beam, 2015-10-19 16:10:28]

https://codereview.chromium.org/1408533002/diff/80001/ui/file_manager/compile...
File ui/file_manager/compile_js.gypi (right):

https://codereview.chromium.org/1408533002/diff/80001/ui/file_manager/compile...
ui/file_manager/compile_js.gypi:12: '<(EXTERNS_DIR)/metrics_private.js',
is this common to ALL compiled_resources.gyp files in ui/file_manager? it
doesn't seem like it

[Dan Beam, 2015-10-20 00:33:38]

https://codereview.chromium.org/1408533002/diff/60001/ui/file_manager/gallery...
File ui/file_manager/gallery/js/image_editor/image_transform.js (right):

https://codereview.chromium.org/1408533002/diff/60001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_transform.js:85: this.domOverlay_
= assertInstanceof(doc.createElement('div'), HTMLDivElement);
On 2015/10/19 09:55:14, fukino wrote:
> On 2015/10/16 07:20:47, Dan Beam wrote:
> > why do these need to be HTMLDivElement rather than HTMLElement?  do you use
> > this.domOverlay_.align somewhere?
> 
> As we call createElement('div'), we expect the returned element is
> HTMLDivElement. In general, should we use HTMLElement as long as we don't
touch
> specific properties of descendent classes.

yes, i understand.  HTMLDivElement#align is the /only/ thing that <div> has that
HTMLElement doesn't.  that's why I ask.

these should all be HTMLElement.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/1408533002/diff/80001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/navigation_list_model.js
(right):

https://codereview.chromium.org/1408533002/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/navigation_list_model.js:291: *
@return {NavigationModelItem} The item at the given index.
can this be NavigationModelItem|undefined or ?NavigationModelItem to be more
explicit?

https://codereview.chromium.org/1408533002/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/navigation_list_model.js:295: return
this.volumeList_[index];
doesn't this return undefined if index < 0?

https://codereview.chromium.org/1408533002/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/navigation_list_model.js:300: return
null;
is this necessary?  should it be return undefined?

https://codereview.chromium.org/1408533002/diff/80001/ui/file_manager/gallery...
File ui/file_manager/gallery/js/gallery_item.js (right):

https://codereview.chromium.org/1408533002/diff/80001/ui/file_manager/gallery...
ui/file_manager/gallery/js/gallery_item.js:176:
GalleryItem.prototype.createCopyName_ = function(
nit: you'd have needed to change Gallery.Item -> GalleryItem far less in this CL
if instead of

  Class.prototype.foo = function() {...};
  Class.prototype.bar = function() {...};

the code did

  Class.prototype = {
    foo: function() {...},
    bar: function() {...},
  };

#justsayin

https://codereview.chromium.org/1408533002/diff/80001/ui/file_manager/gallery...
File ui/file_manager/gallery/js/image_editor/image_adjust.js (left):

https://codereview.chromium.org/1408533002/diff/80001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_adjust.js:237:
toolbar.addButton('Apply', 'Apply', this.apply.bind(this));
i have very little clue what this did, but you're removing it... so yay!

https://codereview.chromium.org/1408533002/diff/80001/ui/file_manager/gallery...
File ui/file_manager/gallery/js/image_editor/image_transform.js (right):

https://codereview.chromium.org/1408533002/diff/80001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_transform.js:85: this.domOverlay_
= assertInstanceof(doc.createElement('div'), HTMLDivElement);
just change these to HTMLElement instead

https://codereview.chromium.org/1408533002/diff/80001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr/ui/list.js (left):

https://codereview.chromium.org/1408533002/diff/80001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/list.js:744: };
while you're here, can you fix this indent and remove the ; after function ()
{}?

[fukino, 2015-10-20 04:13:08]

Thank you! Please take another look.

https://codereview.chromium.org/1408533002/diff/80001/ui/file_manager/compile...
File ui/file_manager/compile_js.gypi (right):

https://codereview.chromium.org/1408533002/diff/80001/ui/file_manager/compile...
ui/file_manager/compile_js.gypi:12: '<(EXTERNS_DIR)/metrics_private.js',
On 2015/10/19 16:10:28, Dan Beam wrote:
> is this common to ALL compiled_resources.gyp files in ui/file_manager? it
> doesn't seem like it

Actually, none of these externs are not common.
I listed minimum set of externs in each compiled_resources.js

https://codereview.chromium.org/1408533002/diff/80001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/js/navigation_list_model.js
(right):

https://codereview.chromium.org/1408533002/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/navigation_list_model.js:291: *
@return {NavigationModelItem} The item at the given index.
On 2015/10/20 00:33:37, Dan Beam wrote:
> can this be NavigationModelItem|undefined or ?NavigationModelItem to be more
> explicit?

Changed to {NavigationModelItem|undefined}

https://codereview.chromium.org/1408533002/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/navigation_list_model.js:295: return
this.volumeList_[index];
On 2015/10/20 00:33:37, Dan Beam wrote:
> doesn't this return undefined if index < 0?

You're right. This can return undefined.

https://codereview.chromium.org/1408533002/diff/80001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/js/navigation_list_model.js:300: return
null;
On 2015/10/20 00:33:37, Dan Beam wrote:
> is this necessary?  should it be return undefined?

I changed it to "return undefined" to be consistent with ArrayDataModel.item().

https://codereview.chromium.org/1408533002/diff/80001/ui/file_manager/gallery...
File ui/file_manager/gallery/js/gallery_item.js (right):

https://codereview.chromium.org/1408533002/diff/80001/ui/file_manager/gallery...
ui/file_manager/gallery/js/gallery_item.js:176:
GalleryItem.prototype.createCopyName_ = function(
On 2015/10/20 00:33:37, Dan Beam wrote:
> nit: you'd have needed to change Gallery.Item -> GalleryItem far less in this
CL
> if instead of
> 
>   Class.prototype.foo = function() {...};
>   Class.prototype.bar = function() {...};
> 
> the code did
> 
>   Class.prototype = {
>     foo: function() {...},
>     bar: function() {...},
>   };
> 
> #justsayin

For consistency with other code under file_manager, I'd like to keep using
"Class.prototype.foo = ..." style for now.
We'll revisit the style to define prototype later.

https://codereview.chromium.org/1408533002/diff/80001/ui/file_manager/gallery...
File ui/file_manager/gallery/js/image_editor/image_adjust.js (left):

https://codereview.chromium.org/1408533002/diff/80001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_adjust.js:237:
toolbar.addButton('Apply', 'Apply', this.apply.bind(this));
On 2015/10/20 00:33:38, Dan Beam wrote:
> i have very little clue what this did, but you're removing it... so yay!

Yeah, this code was wrong, but this function is not used.

https://codereview.chromium.org/1408533002/diff/80001/ui/file_manager/gallery...
File ui/file_manager/gallery/js/image_editor/image_transform.js (right):

https://codereview.chromium.org/1408533002/diff/80001/ui/file_manager/gallery...
ui/file_manager/gallery/js/image_editor/image_transform.js:85: this.domOverlay_
= assertInstanceof(doc.createElement('div'), HTMLDivElement);
On 2015/10/20 00:33:38, Dan Beam wrote:
> just change these to HTMLElement instead

Done.

https://codereview.chromium.org/1408533002/diff/80001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr/ui/list.js (left):

https://codereview.chromium.org/1408533002/diff/80001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/list.js:744: };
On 2015/10/20 00:33:38, Dan Beam wrote:
> while you're here, can you fix this indent and remove the ; after function ()
> {}?

Done.

[Dan Beam, 2015-10-21 01:35:26]

lgtm

https://codereview.chromium.org/1408533002/diff/80001/ui/webui/resources/js/c...
File ui/webui/resources/js/cr/ui/list.js (left):

https://codereview.chromium.org/1408533002/diff/80001/ui/webui/resources/js/c...
ui/webui/resources/js/cr/ui/list.js:744: };
On 2015/10/20 04:13:08, fukino wrote:
> On 2015/10/20 00:33:38, Dan Beam wrote:
> > while you're here, can you fix this indent and remove the ; after function
()
> > {}?
> 
> Done.

what about the indent?

https://codereview.chromium.org/1408533002/diff/100001/ui/file_manager/galler...
File ui/file_manager/gallery/js/image_editor/image_transform.js (right):

https://codereview.chromium.org/1408533002/diff/100001/ui/file_manager/galler...
ui/file_manager/gallery/js/image_editor/image_transform.js:85: this.domOverlay_
= assertInstanceof(doc.createElement('div'), HTMLElement);
assertInstanceof is not required, createElement only returns !Element

https://github.com/google/closure-compiler/blob/04dd916be73f961f489906231cfb1...

https://codereview.chromium.org/1408533002/diff/100001/ui/webui/resources/js/...
File ui/webui/resources/js/cr/ui/list.js (right):

https://codereview.chromium.org/1408533002/diff/100001/ui/webui/resources/js/...
ui/webui/resources/js/cr/ui/list.js:741: self.scrollTop = top;
these should be 2 spaces less indented

[fukino, 2015-10-21 04:00:28]

Thank you!

https://codereview.chromium.org/1408533002/diff/100001/ui/file_manager/galler...
File ui/file_manager/gallery/js/image_editor/image_transform.js (right):

https://codereview.chromium.org/1408533002/diff/100001/ui/file_manager/galler...
ui/file_manager/gallery/js/image_editor/image_transform.js:85: this.domOverlay_
= assertInstanceof(doc.createElement('div'), HTMLElement);
On 2015/10/21 01:35:26, Dan Beam wrote:
> assertInstanceof is not required, createElement only returns !Element
> 
>
https://github.com/google/closure-compiler/blob/04dd916be73f961f489906231cfb1...

Do you suggest to use /** @type {!HTMLElement} */ instead of assertInstanceof()?
I agree with it, since check using instanceof looks redundant in this case.

I changed these assertInstanceof to type cast.
Please let me know if you suggest to change the type of this.domOverlay_ from
HTMLElement to Element.

https://codereview.chromium.org/1408533002/diff/100001/ui/webui/resources/js/...
File ui/webui/resources/js/cr/ui/list.js (right):

https://codereview.chromium.org/1408533002/diff/100001/ui/webui/resources/js/...
ui/webui/resources/js/cr/ui/list.js:741: self.scrollTop = top;
On 2015/10/21 01:35:26, Dan Beam wrote:
> these should be 2 spaces less indented

Sorry! I forgot to fix these indents. Done.

[fukino, 2015-10-21 06:04:19]

The CQ bit was checked by fukino@chromium.org

[fukino, 2015-10-21 06:04:19]

The patchset sent to the CQ was uploaded after l-g-t-m from yawano@chromium.org,
dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1408533002/#ps120001
(title: "Use type cast for createElement, fix indent.")

[commit-bot: I haz the power, 2015-10-21 06:04:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1408533002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1408533002/120001

[commit-bot: I haz the power, 2015-10-21 07:05:16]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-10-21 07:05:56]

Failed to apply the patch.

[commit-bot: I haz the power, 2015-10-21 07:06:12]

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