Fix Closure Compilation for chrome://downloads and chrome://history

Fix Closure Compilation for chrome://downloads and chrome://history

Changes in https://codereview.chromium.org/807593005/ broke Closure Compilation.

BUG=427670, 452870
Committed: https://crrev.com/1d1405a2c7309bec10bbf837e27e0b1cab82b1ff
Cr-Commit-Position: refs/heads/master@{#314718}

[hcarmona, 2015-02-04 02:37:00]

hcarmona@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2015-02-04 17:08:05]

... how do you know any of these casts are valid?

https://codereview.chromium.org/895283002/diff/1/ui/webui/resources/js/cr/ui/...
File ui/webui/resources/js/cr/ui/focus_grid.js (right):

https://codereview.chromium.org/895283002/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/focus_grid.js:66: row.makeRowActive(row.contains(/**
@type {Node} */ (e.target)));
how do you know this is a node?

https://codereview.chromium.org/895283002/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/focus_grid.js:89: if (this.rows[i].contains(/**
@type {Node} */ (target)))
how do you know this is a node?

[hcarmona, 2015-02-04 19:19:55]

https://codereview.chromium.org/895283002/diff/1/ui/webui/resources/js/cr/ui/...
File ui/webui/resources/js/cr/ui/focus_grid.js (right):

https://codereview.chromium.org/895283002/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/focus_grid.js:66: row.makeRowActive(row.contains(/**
@type {Node} */ (e.target)));
On 2015/02/04 17:08:05, Dan Beam wrote:
> how do you know this is a node?

Changed casts to assertInstanceof and verified that the functions are listening
to elements instead of document or window.

https://codereview.chromium.org/895283002/diff/1/ui/webui/resources/js/cr/ui/...
ui/webui/resources/js/cr/ui/focus_grid.js:89: if (this.rows[i].contains(/**
@type {Node} */ (target)))
On 2015/02/04 17:08:05, Dan Beam wrote:
> how do you know this is a node?

This function doesn't know where it will be called. Updated it so it expects a
node. Also updated where it is called since those functions will know where the
event came from so it can be casted safely.

[Dan Beam, 2015-02-04 19:31:48]

lgtm

https://codereview.chromium.org/895283002/diff/20001/ui/webui/resources/js/cr...
File ui/webui/resources/js/cr/ui/focus_grid.js (right):

https://codereview.chromium.org/895283002/diff/20001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/ui/focus_grid.js:35: this.eventTracker_.add(cr.doc,
'keydown', this.onKeydown_.bind(this));
I didn't see how this is rigged -- basically because you're filtering from
document down, casting is probably OK but might as well leave assertInstanceof()
as you've already done the work and it probably isn't killing the perf all that
much

https://codereview.chromium.org/895283002/diff/20001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/ui/focus_grid.js:103: var rowIndex =
this.getRowIndexForTarget(target);
nit: inline assertInstanceof() if you have enough space

https://codereview.chromium.org/895283002/diff/20001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/ui/focus_grid.js:139: this.lastFocused = e.target;
nit: re-use target

https://codereview.chromium.org/895283002/diff/20001/ui/webui/resources/js/cr...
File ui/webui/resources/js/cr/ui/focus_row.js (right):

https://codereview.chromium.org/895283002/diff/20001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/ui/focus_row.js:164: if
(!this.contains(assertInstanceof(e.target, Node)))
var element = assertInstanceof(e.target, Element);
... reuse element when accessing focusableElements ...

[hcarmona, 2015-02-04 22:02:14]

New patchsets have been uploaded after l-g-t-m from dbeam@chromium.org

[hcarmona, 2015-02-04 22:05:52]

Applied feedback. Will click commit when tree is open.

https://codereview.chromium.org/895283002/diff/20001/ui/webui/resources/js/cr...
File ui/webui/resources/js/cr/ui/focus_grid.js (right):

https://codereview.chromium.org/895283002/diff/20001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/ui/focus_grid.js:35: this.eventTracker_.add(cr.doc,
'keydown', this.onKeydown_.bind(this));
On 2015/02/04 19:31:48, Dan Beam wrote:
> I didn't see how this is rigged -- basically because you're filtering from
> document down, casting is probably OK but might as well leave
assertInstanceof()
> as you've already done the work and it probably isn't killing the perf all
that
> much

Acknowledged.

https://codereview.chromium.org/895283002/diff/20001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/ui/focus_grid.js:103: var rowIndex =
this.getRowIndexForTarget(target);
On 2015/02/04 19:31:48, Dan Beam wrote:
> nit: inline assertInstanceof() if you have enough space

Acknowledged. Not enough room.

https://codereview.chromium.org/895283002/diff/20001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/ui/focus_grid.js:139: this.lastFocused = e.target;
On 2015/02/04 19:31:48, Dan Beam wrote:
> nit: re-use target

Done.

https://codereview.chromium.org/895283002/diff/20001/ui/webui/resources/js/cr...
File ui/webui/resources/js/cr/ui/focus_row.js (right):

https://codereview.chromium.org/895283002/diff/20001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/ui/focus_row.js:164: if
(!this.contains(assertInstanceof(e.target, Node)))
On 2015/02/04 19:31:48, Dan Beam wrote:
> var element = assertInstanceof(e.target, Element);
> ... reuse element when accessing focusableElements ...

Done.

[Dan Beam, 2015-02-04 22:06:59]

https://codereview.chromium.org/895283002/diff/20001/ui/webui/resources/js/cr...
File ui/webui/resources/js/cr/ui/focus_row.js (right):

https://codereview.chromium.org/895283002/diff/20001/ui/webui/resources/js/cr...
ui/webui/resources/js/cr/ui/focus_row.js:164: if
(!this.contains(assertInstanceof(e.target, Node)))
On 2015/02/04 22:05:52, Hector Carmona wrote:
> On 2015/02/04 19:31:48, Dan Beam wrote:
> > var element = assertInstanceof(e.target, Element);
> > ... reuse element when accessing focusableElements ...
> 
> Done.

Node -> Element

[hcarmona, 2015-02-05 01:08:54]

The CQ bit was checked by hcarmona@chromium.org

[commit-bot: I haz the power, 2015-02-05 01:10:07]

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

[commit-bot: I haz the power, 2015-02-05 02:06:21]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-05 02:07:31]

Patchset 4 (id:??) landed as
https://crrev.com/1d1405a2c7309bec10bbf837e27e0b1cab82b1ff
Cr-Commit-Position: refs/heads/master@{#314718}