Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
418663002:
Typecheck JS files for chrome://help before doing import transition (Closed)
|
Created:
6 years, 5 months ago by Vitaly Pavlenko Modified:
6 years, 4 months ago Reviewers:
Dan Beam CC:
chromium-reviews, arv+watch_chromium.org, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@json_to_pydict Project:
chromium Visibility:
Public. |
DescriptionTypecheck JS files for chrome://help before doing import transition
BUG=393873
R=dbeam@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288504
Patch Set 1 #
Total comments: 12
Patch Set 2 : Fixed previous comments #
Total comments: 6
Patch Set 3 : Resolve 3 comments #
Total comments: 4
Patch Set 4 : Last fixes #Patch Set 5 : Yet last fixes #
Total comments: 10
Patch Set 6 : split to js and checker config, will commit js #Patch Set 7 : revert assert.js to master #
Total comments: 2
Patch Set 8 : added missing space #Patch Set 9 : fixed wrong use of assert() #
Messages
Total messages: 23 (0 generated)
https://codereview.chromium.org/418663002/diff/1/chrome/browser/resources/ube... File chrome/browser/resources/uber/uber.js (right): https://codereview.chromium.org/418663002/diff/1/chrome/browser/resources/ube... chrome/browser/resources/uber/uber.js:152: e = /** @type{!MessageEvent.<!{method: string, params: *}>} */ (e); we should stay consistent regarding /** @type {T} */(t) vs. /** @type {T} */ (t) ^ i'm in favor of no space https://codereview.chromium.org/418663002/diff/1/compiled_resources.py File compiled_resources.py (right): https://codereview.chromium.org/418663002/diff/1/compiled_resources.py#newcode28 compiled_resources.py:28: # }, probably shouldn't commit commented out targets. probably make more sense to just: # TODO(vitalyp): compile chrome/browser/resources/uber/uber.js https://codereview.chromium.org/418663002/diff/1/third_party/closure_compiler... File third_party/closure_compiler/checker.py (right): https://codereview.chromium.org/418663002/diff/1/third_party/closure_compiler... third_party/closure_compiler/checker.py:75: "--jscomp_warning=checkTypes", did @suppress {checkTypes} not work? i don't think we want to do this, as no type errors will "break the build" so to speak (they wouldn't return a non-zero exit code) https://codereview.chromium.org/418663002/diff/1/third_party/closure_compiler... third_party/closure_compiler/checker.py:84: # TODO(vitalyp): switch to jscomp_error after transition to new imports maybe we could stand for this, but why can't we just suppress or hack around? https://codereview.chromium.org/418663002/diff/1/third_party/closure_compiler... File third_party/closure_compiler/externs/pending_compiler_externs.js (right): https://codereview.chromium.org/418663002/diff/1/third_party/closure_compiler... third_party/closure_compiler/externs/pending_compiler_externs.js:13: /** document these params https://codereview.chromium.org/418663002/diff/1/ui/webui/resources/js/cr/ui/... File ui/webui/resources/js/cr/ui/focus_manager.js (right): https://codereview.chromium.org/418663002/diff/1/ui/webui/resources/js/cr/ui/... ui/webui/resources/js/cr/ui/focus_manager.js:158: event.target = /** @type {Element} */(event.target); this can be (and very well might be) a Document. why can't we use EventTarget instead?
https://chromiumcodereview.appspot.com/418663002/diff/1/third_party/closure_c... File third_party/closure_compiler/checker.py (right): https://chromiumcodereview.appspot.com/418663002/diff/1/third_party/closure_c... third_party/closure_compiler/checker.py:75: "--jscomp_warning=checkTypes", On 2014/07/23 22:09:24, Dan Beam wrote: > did @suppress {checkTypes} not work? i don't think we want to do this, as no > type errors will "break the build" so to speak (they wouldn't return a non-zero > exit code) I'll enable it in the next CL where I switch to cr.exportPath() and references to module functions no longer cause compiler error.
https://chromiumcodereview.appspot.com/418663002/diff/1/chrome/browser/resour... File chrome/browser/resources/uber/uber.js (right): https://chromiumcodereview.appspot.com/418663002/diff/1/chrome/browser/resour... chrome/browser/resources/uber/uber.js:152: e = /** @type{!MessageEvent.<!{method: string, params: *}>} */ (e); On 2014/07/23 22:09:24, Dan Beam wrote: > we should stay consistent regarding > > /** @type {T} */(t) > > vs. > > /** @type {T} */ (t) > ^ > i'm in favor of no space Done. https://chromiumcodereview.appspot.com/418663002/diff/1/compiled_resources.py File compiled_resources.py (right): https://chromiumcodereview.appspot.com/418663002/diff/1/compiled_resources.py... compiled_resources.py:28: # }, On 2014/07/23 22:09:24, Dan Beam wrote: > probably shouldn't commit commented out targets. probably make more sense to > just: > > # TODO(vitalyp): compile chrome/browser/resources/uber/uber.js Done. https://chromiumcodereview.appspot.com/418663002/diff/1/third_party/closure_c... File third_party/closure_compiler/checker.py (right): https://chromiumcodereview.appspot.com/418663002/diff/1/third_party/closure_c... third_party/closure_compiler/checker.py:84: # TODO(vitalyp): switch to jscomp_error after transition to new imports On 2014/07/23 22:09:24, Dan Beam wrote: > maybe we could stand for this, but why can't we just suppress or hack around? I caught a situation when the suppress declaration didn't work. https://chromiumcodereview.appspot.com/418663002/diff/1/third_party/closure_c... File third_party/closure_compiler/externs/pending_compiler_externs.js (right): https://chromiumcodereview.appspot.com/418663002/diff/1/third_party/closure_c... third_party/closure_compiler/externs/pending_compiler_externs.js:13: /** On 2014/07/23 22:09:24, Dan Beam wrote: > document these params Done. https://chromiumcodereview.appspot.com/418663002/diff/1/ui/webui/resources/js... File ui/webui/resources/js/cr/ui/focus_manager.js (right): https://chromiumcodereview.appspot.com/418663002/diff/1/ui/webui/resources/js... ui/webui/resources/js/cr/ui/focus_manager.js:158: event.target = /** @type {Element} */(event.target); On 2014/07/23 22:09:24, Dan Beam wrote: > this can be (and very well might be) a Document. why can't we use EventTarget > instead? Done.
https://chromiumcodereview.appspot.com/418663002/diff/20001/chrome/browser/re... File chrome/browser/resources/uber/uber.js (right): https://chromiumcodereview.appspot.com/418663002/diff/20001/chrome/browser/re... chrome/browser/resources/uber/uber.js:221: assert(element, 'expected an element'); why don't we just update assert() to something like: /** * Verify |cond| is truthy and return a non-null result if so. * @template {T} * @param {T} cond A condition to check for truthiness. * @param {string=} opt_message A message to show on failure. * @return {!T} A non-null |cond|. */ function assert(cond, opt_message) { if (!cond) throw new Error(opt_message || "Assertion failed"); return cond; } and then the calling code could look like: var element = assert(document.querySelector(query)); existing code that does: assert(blah, 'message'); should remain unaffected. https://chromiumcodereview.appspot.com/418663002/diff/20001/chrome/browser/re... File chrome/browser/resources/uber/uber_utils.js (right): https://chromiumcodereview.appspot.com/418663002/diff/20001/chrome/browser/re... chrome/browser/resources/uber/uber_utils.js:61: e = /** @type{!MessageEvent.<!{method: string, params: *}>} */ (e); /* ... */(e) https://chromiumcodereview.appspot.com/418663002/diff/20001/ui/webui/resource... File ui/webui/resources/js/cr/ui/focus_manager.js (right): https://chromiumcodereview.appspot.com/418663002/diff/20001/ui/webui/resource... ui/webui/resources/js/cr/ui/focus_manager.js:157: onDocumentFocus_: function(event) { var targetNode = /** @type {Node} */(event.target); is mildly prettier, IMO
https://chromiumcodereview.appspot.com/418663002/diff/20001/chrome/browser/re... File chrome/browser/resources/uber/uber.js (right): https://chromiumcodereview.appspot.com/418663002/diff/20001/chrome/browser/re... chrome/browser/resources/uber/uber.js:221: assert(element, 'expected an element'); On 2014/07/24 04:27:54, Dan Beam wrote: > why don't we just update assert() to something like: > > /** > * Verify |cond| is truthy and return a non-null result if so. > * @template {T} > * @param {T} cond A condition to check for truthiness. > * @param {string=} opt_message A message to show on failure. > * @return {!T} A non-null |cond|. > */ > function assert(cond, opt_message) { > if (!cond) > throw new Error(opt_message || "Assertion failed"); > return cond; > } > > and then the calling code could look like: > > var element = assert(document.querySelector(query)); > > existing code that does: > > assert(blah, 'message'); > > should remain unaffected. Acknowledged. https://chromiumcodereview.appspot.com/418663002/diff/20001/chrome/browser/re... File chrome/browser/resources/uber/uber_utils.js (right): https://chromiumcodereview.appspot.com/418663002/diff/20001/chrome/browser/re... chrome/browser/resources/uber/uber_utils.js:61: e = /** @type{!MessageEvent.<!{method: string, params: *}>} */ (e); On 2014/07/24 04:27:54, Dan Beam wrote: > /* ... */(e) Acknowledged. https://chromiumcodereview.appspot.com/418663002/diff/20001/ui/webui/resource... File ui/webui/resources/js/cr/ui/focus_manager.js (right): https://chromiumcodereview.appspot.com/418663002/diff/20001/ui/webui/resource... ui/webui/resources/js/cr/ui/focus_manager.js:157: onDocumentFocus_: function(event) { On 2014/07/24 04:27:54, Dan Beam wrote: > var targetNode = /** @type {Node} */(event.target); > > is mildly prettier, IMO Acknowledged.
https://chromiumcodereview.appspot.com/418663002/diff/40001/chrome/browser/re... File chrome/browser/resources/uber/uber.js (right): https://chromiumcodereview.appspot.com/418663002/diff/40001/chrome/browser/re... chrome/browser/resources/uber/uber.js:220: var element = assert(document.querySelector(query), 'expected an element'); just remove the 'expected an element' message https://chromiumcodereview.appspot.com/418663002/diff/40001/compiled_resource... File compiled_resources.py (right): https://chromiumcodereview.appspot.com/418663002/diff/40001/compiled_resource... compiled_resources.py:45: }, in the grand scheme of things, these should probably go on the bottom of the file, not the top
you need to rebase in changes, but other than that lgtm
https://chromiumcodereview.appspot.com/418663002/diff/40001/chrome/browser/re... File chrome/browser/resources/uber/uber.js (right): https://chromiumcodereview.appspot.com/418663002/diff/40001/chrome/browser/re... chrome/browser/resources/uber/uber.js:220: var element = assert(document.querySelector(query), 'expected an element'); On 2014/07/24 20:11:53, Dan Beam wrote: > just remove the 'expected an element' message Done. https://chromiumcodereview.appspot.com/418663002/diff/40001/compiled_resource... File compiled_resources.py (right): https://chromiumcodereview.appspot.com/418663002/diff/40001/compiled_resource... compiled_resources.py:45: }, On 2014/07/24 20:11:53, Dan Beam wrote: > in the grand scheme of things, these should probably go on the bottom of the > file, not the top Acknowledged.
The CQ bit was checked by vitalyp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vitalyp@chromium.org/418663002/100001
The CQ bit was unchecked by dbeam@chromium.org
https://codereview.chromium.org/418663002/diff/80001/chrome/browser/resources... File chrome/browser/resources/uber/uber_utils.js (right): https://codereview.chromium.org/418663002/diff/80001/chrome/browser/resources... chrome/browser/resources/uber/uber_utils.js:64: else if (e.data.method === 'mouseWheel') add curlies to every if/else now https://codereview.chromium.org/418663002/diff/80001/compiled.resources File compiled.resources (right): https://codereview.chromium.org/418663002/diff/80001/compiled.resources#newco... compiled.resources:132: }, just remove this diff and keep the file local https://codereview.chromium.org/418663002/diff/80001/third_party/closure_comp... File third_party/closure_compiler/checker.py (right): https://codereview.chromium.org/418663002/diff/80001/third_party/closure_comp... third_party/closure_compiler/checker.py:74: # TODO(vitalyp): switch to jscomp_error after transition to new imports do we still need this? https://codereview.chromium.org/418663002/diff/80001/ui/webui/resources/js/as... File ui/webui/resources/js/assert.js (right): https://codereview.chromium.org/418663002/diff/80001/ui/webui/resources/js/as... ui/webui/resources/js/assert.js:16: * @template T this didn't work :( https://codereview.chromium.org/418663002/diff/80001/ui/webui/resources/js/cr.js File ui/webui/resources/js/cr.js (right): https://codereview.chromium.org/418663002/diff/80001/ui/webui/resources/js/cr... ui/webui/resources/js/cr.js:132: * @param {function(*, *):void=} opt_setHook A function to run after the this is fixed here: https://chromiumcodereview.appspot.com/405743002/
Dan, please, set commit bit if everything is OK. https://codereview.chromium.org/418663002/diff/80001/chrome/browser/resources... File chrome/browser/resources/uber/uber_utils.js (right): https://codereview.chromium.org/418663002/diff/80001/chrome/browser/resources... chrome/browser/resources/uber/uber_utils.js:64: else if (e.data.method === 'mouseWheel') On 2014/08/06 16:06:59, Dan Beam wrote: > add curlies to every if/else now Is it because the lines 65-66 are two physical lines? (although one logical) https://codereview.chromium.org/418663002/diff/80001/compiled.resources File compiled.resources (right): https://codereview.chromium.org/418663002/diff/80001/compiled.resources#newco... compiled.resources:132: }, On 2014/08/06 16:06:59, Dan Beam wrote: > just remove this diff and keep the file local I removed it in Patch Set #6. https://codereview.chromium.org/418663002/diff/80001/third_party/closure_comp... File third_party/closure_compiler/checker.py (right): https://codereview.chromium.org/418663002/diff/80001/third_party/closure_comp... third_party/closure_compiler/checker.py:74: # TODO(vitalyp): switch to jscomp_error after transition to new imports On 2014/08/06 16:06:59, Dan Beam wrote: > do we still need this? I removed it in Patch Set #6. https://codereview.chromium.org/418663002/diff/80001/ui/webui/resources/js/as... File ui/webui/resources/js/assert.js (right): https://codereview.chromium.org/418663002/diff/80001/ui/webui/resources/js/as... ui/webui/resources/js/assert.js:16: * @template T On 2014/08/06 16:06:59, Dan Beam wrote: > this didn't work :( Reverted. https://codereview.chromium.org/418663002/diff/80001/ui/webui/resources/js/cr.js File ui/webui/resources/js/cr.js (right): https://codereview.chromium.org/418663002/diff/80001/ui/webui/resources/js/cr... ui/webui/resources/js/cr.js:132: * @param {function(*, *):void=} opt_setHook A function to run after the On 2014/08/06 16:06:59, Dan Beam wrote: > this is fixed here: https://chromiumcodereview.appspot.com/405743002/ This file is removed in my Patch Set #6.
https://codereview.chromium.org/418663002/diff/120001/chrome/browser/resource... File chrome/browser/resources/uber/uber_utils.js (right): https://codereview.chromium.org/418663002/diff/120001/chrome/browser/resource... chrome/browser/resources/uber/uber_utils.js:61: e = /** @type{!MessageEvent.<!{method: string, params: *}>} */(e); type{ => type {
https://codereview.chromium.org/418663002/diff/120001/chrome/browser/resource... File chrome/browser/resources/uber/uber_utils.js (right): https://codereview.chromium.org/418663002/diff/120001/chrome/browser/resource... chrome/browser/resources/uber/uber_utils.js:61: e = /** @type{!MessageEvent.<!{method: string, params: *}>} */(e); On 2014/08/07 20:50:46, Dan Beam wrote: > type{ => type { Done.
The CQ bit was checked by dbeam@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vitalyp@chromium.org/418663002/140001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_swarming on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by vitalyp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vitalyp@chromium.org/418663002/160001
Message was sent while issue was closed.
Change committed as 288504 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
