js_checker.py: Replace custom WrapperTypeCheck with ESLint's no-new-wrappers.

js_checker.py: Replace custom WrapperTypeCheck with ESLint's no-new-wrappers.

BUG=720034
Review-Url: https://codereview.chromium.org/2913453002
Cr-Commit-Position: refs/heads/master@{#476121}
Committed: https://chromium.googlesource.com/chromium/src/+/728cff0c8b3f6e205d5ab5d26e63df7e67e55d29

[dpapad, 2017-05-27 01:15:05]

Description was changed from

==========
js_checker.py: Replace custom checks for String,Boolean,Number, with
no-new-wrappers.

BUG=720034
==========

to

==========
js_checker.py: Replace custom WrapperTypeCheck with ESLint's no-new-wrappers.

BUG=720034
==========

[dpapad, 2017-05-27 01:15:24]

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

[commit-bot: I haz the power, 2017-05-27 01:15:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dpapad, 2017-05-27 01:26:24]

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

[dpapad, 2017-05-27 01:26:25]

Example error message:

/workspage/chromium1/src/chrome/browser/resources/settings/direction_delegate.js
  32:12  error  Do not use String as a constructor  no-new-wrappers

✖ 1 problem (1 error, 0 warnings)


So, no-new-wrappers is not 100% equivalent to our custom check. Perhaps we can
keep both, but I think it is unnecessary. The differences AFAIU are the
following:

 - WrapperTypeCheck actually checked for Boolean,Number,String appearing only
within an @type/@param/@return annotations, but it did not look for "new
String(", "new Boolean(" etc.

 - no-new-wrappers is checking the code for "new String(...)" etc, and ignores
type annotations.

Now the JSCompiler (correctly) differentiates between |string| and |String|
types, so if someone where to write a function like this

/** @return {!String} */
function() { return 'foo'; }

it would produce the following error.
JSC_TYPE_MISMATCH: inconsistent return type
found   : string
required: String at line 2 character 26
function hello() { return 'foo'; }


So to summarize:
- js_checker.py checked JSDoc only
- ESLint checks the actual code only

We could (a) keep both, (b) only keep Python check (current state), or (c) only
keep ESLint check.

[commit-bot: I haz the power, 2017-05-27 01:52:48]

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

[commit-bot: I haz the power, 2017-05-27 01:52:49]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2017-05-30 16:16:21]

https://codereview.chromium.org/2913453002/diff/1/tools/web_dev_style/js_chec...
File tools/web_dev_style/js_checker_test.py (left):

https://codereview.chromium.org/2913453002/diff/1/tools/web_dev_style/js_chec...
tools/web_dev_style/js_checker_test.py:270: def ShouldFailWrapperTypeCheck(self,
line):
can we leave these?  i know there's likely tests for ESLint but if were ever to
change the implementation it might be hard to know there were ever tests like
this

[Dan Beam, 2017-05-30 17:24:59]

lgtm

[dpapad, 2017-05-31 22:30:20]

PTAL

https://codereview.chromium.org/2913453002/diff/1/tools/web_dev_style/js_chec...
File tools/web_dev_style/js_checker_test.py (left):

https://codereview.chromium.org/2913453002/diff/1/tools/web_dev_style/js_chec...
tools/web_dev_style/js_checker_test.py:270: def ShouldFailWrapperTypeCheck(self,
line):
On 2017/05/30 at 16:16:20, Dan Beam wrote:
> can we leave these?  i know there's likely tests for ESLint but if were ever
to change the implementation it might be hard to know there were ever tests like
this

Added equivalent smoke tests in js_checker_eslint_test.py.

https://codereview.chromium.org/2913453002/diff/20001/tools/web_dev_style/PRE...
File tools/web_dev_style/PRESUBMIT.py (right):

https://codereview.chromium.org/2913453002/diff/20001/tools/web_dev_style/PRE...
tools/web_dev_style/PRESUBMIT.py:30: tests.append(path.join(cwd,
'js_checker_eslint_test.py'))
I am planning to detect .eslintrc.js changes in a  follow up CL.

[Dan Beam, 2017-05-31 22:54:34]

lgtm

https://codereview.chromium.org/2913453002/diff/20001/tools/web_dev_style/PRE...
File tools/web_dev_style/PRESUBMIT.py (right):

https://codereview.chromium.org/2913453002/diff/20001/tools/web_dev_style/PRE...
tools/web_dev_style/PRESUBMIT.py:30: tests.append(path.join(cwd,
'js_checker_eslint_test.py'))
On 2017/05/31 22:30:20, dpapad wrote:
> I am planning to detect .eslintrc.js changes in a  follow up CL.

Acknowledged.

https://codereview.chromium.org/2913453002/diff/20001/tools/web_dev_style/PRE...
tools/web_dev_style/PRESUBMIT.py:30: tests.append(path.join(cwd,
'js_checker_eslint_test.py'))
nit:

tests += [path.join(cwd, 'js_checker_%stest.py' % e) for e in '', 'eslint_']

[dpapad, 2017-05-31 22:58:10]

https://codereview.chromium.org/2913453002/diff/20001/tools/web_dev_style/PRE...
File tools/web_dev_style/PRESUBMIT.py (right):

https://codereview.chromium.org/2913453002/diff/20001/tools/web_dev_style/PRE...
tools/web_dev_style/PRESUBMIT.py:30: tests.append(path.join(cwd,
'js_checker_eslint_test.py'))
On 2017/05/31 at 22:54:33, Dan Beam wrote:
> nit:
> 
> tests += [path.join(cwd, 'js_checker_%stest.py' % e) for e in '', 'eslint_']

Even though I acknowledge that this is shorter and more python-ic way of doing
things, prefer not to do this, because it reduces grep-ability, for example
grepping the code for "js_checker_test.py" will return no results.

[dpapad, 2017-06-01 00:48:42]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2017-06-01 00:49: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, 2017-06-01 01:06:45]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1496278122150870,
"parent_rev": "83ad1eda722e5e35b9d0a943617036958c6b4b4e", "commit_rev":
"728cff0c8b3f6e205d5ab5d26e63df7e67e55d29"}

[commit-bot: I haz the power, 2017-06-01 01:06:59]

Description was changed from

==========
js_checker.py: Replace custom WrapperTypeCheck with ESLint's no-new-wrappers.

BUG=720034
==========

to

==========
js_checker.py: Replace custom WrapperTypeCheck with ESLint's no-new-wrappers.

BUG=720034

Review-Url: https://codereview.chromium.org/2913453002
Cr-Commit-Position: refs/heads/master@{#476121}
Committed:
https://chromium.googlesource.com/chromium/src/+/728cff0c8b3f6e205d5ab5d26e63...
==========

[commit-bot: I haz the power, 2017-06-01 01:07:00]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/728cff0c8b3f6e205d5ab5d26e63...