Issue 801533006: Add type annotations to gallery/js/gallery.js.

Issue 801533006: Add type annotations to gallery/js/gallery.js. (Closed)

Created:
6 years ago by yawano

Modified:
6 years ago

Reviewers:
fukino

CC:
chromium-reviews, vitalyp+closure_chromium.org, rginda+watch_chromium.org, mtomasz+watch_chromium.org, dbeam+watch-closure_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Add type annotations to gallery/js/gallery.js. BUG=433728 TEST=GYP_GENERATORS=ninja tools/gyp/gyp --depth . ui/file_manager/gallery/js/compiled_resources.gyp && ninja -C out/Default | grep "gallery.js" Committed: https://crrev.com/e15fd7e09d0d051421554a0d242442e51bb9c122 Cr-Commit-Position: refs/heads/master@{#308307}

Patch Set 1 #

Patch Set 2 : Removed unnecessary annotations. #

Total comments: 13

Patch Set 3 : Fixed false-positive lint errors. #

Created: 6 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+194 lines, -187 lines)			Patch
M	ui/file_manager/externs/chrome_app_window.js	View		1 chunk	+3 lines, -0 lines	0 comments	Download
D	ui/file_manager/externs/gallery.js	View		1 chunk	+0 lines, -27 lines	0 comments	Download
A +	ui/file_manager/externs/gallery_background.js	View		1 chunk	+0 lines, -5 lines	0 comments	Download
A	ui/file_manager/externs/gallery_foreground.js	View		1 chunk	+25 lines, -0 lines	0 comments	Download
M	ui/file_manager/file_manager/common/js/util.js	View	1 2	2 chunks	+4 lines, -4 lines	0 comments	Download
M	ui/file_manager/file_manager/foreground/js/metadata/metadata_cache.js	View		6 chunks	+10 lines, -9 lines	0 comments	Download
M	ui/file_manager/gallery/js/compiled_resources.gyp	View		2 chunks	+2 lines, -2 lines	0 comments	Download
M	ui/file_manager/gallery/js/gallery.js	View	1 2	23 chunks	+146 lines, -130 lines	0 comments	Download
M	ui/file_manager/gallery/js/mosaic_mode.js	View		1 chunk	+1 line, -1 line	0 comments	Download
M	ui/file_manager/gallery/js/slide_mode.js	View		4 chunks	+3 lines, -9 lines	0 comments	Download

Messages

Total messages: 10 (2 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

fukino

Thanks! https://codereview.chromium.org/801533006/diff/20001/ui/file_manager/file_manager/common/js/util.js File ui/file_manager/file_manager/common/js/util.js (right): https://codereview.chromium.org/801533006/diff/20001/ui/file_manager/file_manager/common/js/util.js#newcode63 ui/file_manager/file_manager/common/js/util.js:63: ENCODING_ERR: 'EncodingError' Sorry for forgetting to share this: ...

6 years ago (2014-12-12 15:15:26 UTC) #3

yawano

Thank you for the review! PTAL. https://codereview.chromium.org/801533006/diff/20001/ui/file_manager/file_manager/common/js/util.js File ui/file_manager/file_manager/common/js/util.js (right): https://codereview.chromium.org/801533006/diff/20001/ui/file_manager/file_manager/common/js/util.js#newcode63 ui/file_manager/file_manager/common/js/util.js:63: ENCODING_ERR: 'EncodingError' On ...

6 years ago (2014-12-15 01:45:24 UTC) #4

fukino

lgtm! https://codereview.chromium.org/801533006/diff/20001/ui/file_manager/file_manager/common/js/util.js File ui/file_manager/file_manager/common/js/util.js (right): https://codereview.chromium.org/801533006/diff/20001/ui/file_manager/file_manager/common/js/util.js#newcode291 ui/file_manager/file_manager/common/js/util.js:291: * @param {?string} currentDirectoryURL Currently opened directory as ...

6 years ago (2014-12-15 02:15:28 UTC) #5

lgtm!

https://codereview.chromium.org/801533006/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/common/js/util.js (right):

https://codereview.chromium.org/801533006/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/common/js/util.js:291: * @param {?string}
currentDirectoryURL Currently opened directory as an URL.
On 2014/12/15 01:45:24, yawano wrote:
> On 2014/12/12 15:15:25, fukino wrote:
> > Could you change the method invocation like this?
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/file_manager/fi...
> 
> In the comment of first argument of util.updateAppState method invocation in
> gallery.js, it says it keeps the current directory. If we change it to pass ''
> instead of null, it resets the value.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/file_manager/ga...

Thanks! Got it (a little bit confusing interface, though...)

https://codereview.chromium.org/801533006/diff/20001/ui/file_manager/file_man...
File ui/file_manager/file_manager/foreground/js/metadata/metadata_cache.js
(right):

https://codereview.chromium.org/801533006/diff/20001/ui/file_manager/file_man...
ui/file_manager/file_manager/foreground/js/metadata/metadata_cache.js:958: *
@type {string}
On 2014/12/15 01:45:24, yawano wrote:
> On 2014/12/12 15:15:25, fukino wrote:
> > Didn't the compiler accept this as const?
> 
> We assign new value at gallery/js/gallery.js, and the compiler complains that
> the value is assigned more than once if we annotate for both of WORKER_SCRIPT.
> 
> After the script is combined, assignments look like below.
> 
> // From metadata_cache.js
> ContentProvider.WORKER_SCRIPT = ...
> 
> ... some codes ...
> 
> // From gallery.js
> ContentProvider.WORKER_SCRIPT = ...
> 
> While the compiler accepts to annotate only for WORKER_SCRIPT of
> metadata_cache.js as @const, I think it's wrong behavior. Since even in that
> case, we assign(define) WORKER_SCRIPT twice.

Thanks.
I agree that both WORKER_SCRIPT shouldn't be @const.

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

https://codereview.chromium.org/801533006/diff/20001/ui/file_manager/gallery/...
ui/file_manager/gallery/js/gallery.js:308: this.toggleMode_.bind(this,
undefined));
On 2014/12/15 01:45:24, yawano wrote:
> On 2014/12/12 15:15:25, fukino wrote:
> > 'undefined' looks redundant
> 
> Interface of toggleMode_ is function(opt_callback, opt_event). In order to
pass
> the event to opt_event, I think we need to bind undefined to opt_callback
here.

Sorry, I misunderstood the code. Thank you  for explanation!

commit-bot: I haz the power

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

6 years ago (2014-12-15 02:17:20 UTC) #8

commit-bot: I haz the power

6 years ago (2014-12-15 03:08:40 UTC) #10

Message was sent while issue was closed.

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

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages