Quick View: show device model metadata.

Quick View: show device model metadata.

TEST=manually.
BUG=636191
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[oka, 2016-11-29 09:21:34]

Description was changed from

==========
Quick View: show device model metadata.

TEST=manually.
BUG=636191
==========

to

==========
Quick View: show device model metadata.

TEST=manually.
BUG=636191
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[oka, 2016-11-29 09:34:11]

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

[commit-bot: I haz the power, 2016-11-29 09:34:28]

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

[oka, 2016-11-29 09:35:38]

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

[oka, 2016-11-29 09:35:39]

PTAL.

[commit-bot: I haz the power, 2016-11-29 09:41:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-29 09:41:20]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[fukino, 2016-11-30 05:37:28]

https://codereview.chromium.org/2532003005/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/elements/files_metadata_box.js
(right):

https://codereview.chromium.org/2532003005/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_metadata_box.js:154:
deviceModel_: function(ifd) {
Add an annotation.

https://codereview.chromium.org/2532003005/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_metadata_box.js:154:
deviceModel_: function(ifd) {
Please make sure this function returns a string value.

[oka, 2016-11-30 05:56:43]

PTAL

https://codereview.chromium.org/2532003005/diff/20001/ui/file_manager/file_ma...
File ui/file_manager/file_manager/foreground/elements/files_metadata_box.js
(right):

https://codereview.chromium.org/2532003005/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_metadata_box.js:154:
deviceModel_: function(ifd) {
On 2016/11/30 05:37:28, fukino wrote:
> Add an annotation.

Done.

https://codereview.chromium.org/2532003005/diff/20001/ui/file_manager/file_ma...
ui/file_manager/file_manager/foreground/elements/files_metadata_box.js:154:
deviceModel_: function(ifd) {
On 2016/11/30 05:37:28, fukino wrote:
> Please make sure this function returns a string value.

Done.

[fukino, 2016-11-30 06:17:35]

lgtm with nits.

https://codereview.chromium.org/2532003005/diff/100001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/elements/files_metadata_box.js
(right):

https://codereview.chromium.org/2532003005/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/elements/files_metadata_box.js:52: *
@type {?Object}
Please remove type annotation, as it is redundant here.

https://codereview.chromium.org/2532003005/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/elements/files_metadata_box.js:158: *
@param {?Object} ifd
nit: Remove '?'. Object is nullable by default.

https://codereview.chromium.org/2532003005/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/elements/files_metadata_box.js:164:
return (ifd && ifd.image && ifd.image[272].value) || '';
Is it guaranteed that image has key:272? If no, maybe you need to check
ifd.image[272].

[oka, 2016-11-30 06:36:26]

PTAL.

https://codereview.chromium.org/2532003005/diff/100001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/elements/files_metadata_box.js
(right):

https://codereview.chromium.org/2532003005/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/elements/files_metadata_box.js:52: *
@type {?Object}
On 2016/11/30 06:17:35, fukino wrote:
> Please remove type annotation, as it is redundant here.

Removing this fails closure compiler.
##
/bt/cr/1/src/ui/file_manager/file_manager/foreground/elements/files_metadata_box.js:79:
ERROR - assignment to property ifd of FilesMetadataBox
## found   : null
## required: Object
##     this.ifd = null;
##     ^
ninja: build stopped: subcommand failed.

https://codereview.chromium.org/2532003005/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/elements/files_metadata_box.js:158: *
@param {?Object} ifd
On 2016/11/30 06:17:35, fukino wrote:
> nit: Remove '?'. Object is nullable by default.

prepending? is redundant, but is recommended so that the intent is clear and
explicit:
https://github.com/google/closure-compiler/wiki/Types-in-the-Closure-Type-System

https://codereview.chromium.org/2532003005/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/elements/files_metadata_box.js:164:
return (ifd && ifd.image && ifd.image[272].value) || '';
On 2016/11/30 06:17:35, fukino wrote:
> Is it guaranteed that image has key:272? If no, maybe you need to check
> ifd.image[272].

Done. Good catch! Thank you.

[fukino, 2016-11-30 07:11:01]

https://codereview.chromium.org/2532003005/diff/100001/ui/file_manager/file_m...
File ui/file_manager/file_manager/foreground/elements/files_metadata_box.js
(right):

https://codereview.chromium.org/2532003005/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/elements/files_metadata_box.js:52: *
@type {?Object}
On 2016/11/30 06:36:26, oka wrote:
> On 2016/11/30 06:17:35, fukino wrote:
> > Please remove type annotation, as it is redundant here.
> 
> Removing this fails closure compiler.
> ##
>
/bt/cr/1/src/ui/file_manager/file_manager/foreground/elements/files_metadata_box.js:79:
> ERROR - assignment to property ifd of FilesMetadataBox
> ## found   : null
> ## required: Object
> ##     this.ifd = null;
> ##     ^
> ninja: build stopped: subcommand failed.

I see.
I guess it is a PolymerPass's issue or assigning null to type:Object property is
not supported in Polymer. (like, due to attribute serialize/deserialize
capability?)

Using {?Object} here is OK. Assigning an empty object in line 84 is also OK.

https://codereview.chromium.org/2532003005/diff/100001/ui/file_manager/file_m...
ui/file_manager/file_manager/foreground/elements/files_metadata_box.js:158: *
@param {?Object} ifd
On 2016/11/30 06:36:26, oka wrote:
> On 2016/11/30 06:17:35, fukino wrote:
> > nit: Remove '?'. Object is nullable by default.
> 
> prepending? is redundant, but is recommended so that the intent is clear and
> explicit:
>
https://github.com/google/closure-compiler/wiki/Types-in-the-Closure-Type-System

sgtm.
It's also mentioned in
https://www.chromium.org/developers/web-development-style-guide.

[oka, 2016-11-30 07:36:35]

The CQ bit was checked by oka@chromium.org

[oka, 2016-11-30 07:36:35]

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/2532003005/#ps120001
(title: "Addressed comments.")

[commit-bot: I haz the power, 2016-11-30 07:37:34]

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-11-30 08:15:44]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1480491395491620,
"parent_rev": "d76fb0da3c44854d982622a45c2ec6c08359af62", "commit_rev":
"f189d74a9156743ff595155dc34426abd7d4fad9"}

[commit-bot: I haz the power, 2016-11-30 08:16:14]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-11-30 08:20:12]

Description was changed from

==========
Quick View: show device model metadata.

TEST=manually.
BUG=636191
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Quick View: show device model metadata.

TEST=manually.
BUG=636191
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/b540f4e1ae8480e039530c62c0656baba0785317
Cr-Commit-Position: refs/heads/master@{#435158}
==========

[commit-bot: I haz the power, 2016-11-30 08:20:13]

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