WebUI: Enable ESLint rule for missing semicolons.

WebUI: Enable ESLint rule for missing semicolons.

 - Fixing almost all existing violations
   (remaining ones are fixed at crrev.com/2903063007).
 - Enabling http://eslint.org/docs/rules/semi check during PRESUBMIT

This enforces the following section from the styleguide:
https://google.github.io/styleguide/jsguide.html#formatting-semicolons-are-required

BUG=720034
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2909503003
Cr-Commit-Position: refs/heads/master@{#475771}
Committed: https://chromium.googlesource.com/chromium/src/+/1e511b111bdebc1cdb7588d9c22e034ab70715b9

[dpapad, 2017-05-25 17:42:45]

Description was changed from

==========
WebUI: Fix violations of missing semicolons.

This is in preparation of enabling http://eslint.org/docs/rules/semi
check during PRESUBMIT.

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

to

==========
WebUI: Fix violations of missing semicolons.

This is in preparation of enabling http://eslint.org/docs/rules/semi
check during PRESUBMIT.

BUG=720034
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2017-05-25 17:46:31]

Description was changed from

==========
WebUI: Fix violations of missing semicolons.

This is in preparation of enabling http://eslint.org/docs/rules/semi
check during PRESUBMIT.

BUG=720034
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
WebUI: Fix ESLint violations of missing semicolons.

This is in preparation of enabling http://eslint.org/docs/rules/semi
check during PRESUBMIT, to enforce the styleguide.

https://google.github.io/styleguide/jsguide.html#formatting-semicolons-are-re...

BUG=720034
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2017-05-25 17:49:46]

Description was changed from

==========
WebUI: Fix ESLint violations of missing semicolons.

This is in preparation of enabling http://eslint.org/docs/rules/semi
check during PRESUBMIT, to enforce the styleguide.

https://google.github.io/styleguide/jsguide.html#formatting-semicolons-are-re...

BUG=720034
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
WebUI: Enable ESLint rule for missing semicolons.

 - Fixing existing violations
 - Enabling http://eslint.org/docs/rules/semi check during PRESUBMIT

This enforces the following section from the styleguide:
https://google.github.io/styleguide/jsguide.html#formatting-semicolons-are-re...

BUG=720034
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2017-05-25 18:45:52]

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

[commit-bot: I haz the power, 2017-05-25 18:46:27]

Dry run: 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-05-25 19:34:24]

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

[commit-bot: I haz the power, 2017-05-25 19:34:25]

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

[dpapad, 2017-05-25 21:34:00]

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

[commit-bot: I haz the power, 2017-05-25 21:34:38]

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-26 00:52:53]

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

[commit-bot: I haz the power, 2017-05-26 01:46:42]

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

[commit-bot: I haz the power, 2017-05-26 01:46:43]

Dry run: This issue passed the CQ dry run.

[dschuyler, 2017-05-26 19:00:26]

lgtm

https://codereview.chromium.org/2909503003/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/braille/expanding_braille_translator.js
(right):

https://codereview.chromium.org/2909503003/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/expanding_braille_translator.js:155:
function(accum, chunk) { return accum + chunk.cells.byteLength;}, 0);
optional:
I think a space should follow the ';'
(though I also think the line should break after the '{')

https://codereview.chromium.org/2909503003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/certificate_manager_browsertest.js (right):

https://codereview.chromium.org/2909503003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/certificate_manager_browsertest.js:251:
].forEach(CertificateManager.onPopulateTree);}));
optional: space after ;

[dpapad, 2017-05-26 19:08:39]

https://codereview.chromium.org/2909503003/diff/40001/chrome/browser/resource...
File
chrome/browser/resources/chromeos/chromevox/braille/expanding_braille_translator.js
(right):

https://codereview.chromium.org/2909503003/diff/40001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/braille/expanding_braille_translator.js:155:
function(accum, chunk) { return accum + chunk.cells.byteLength;}, 0);
On 2017/05/26 at 19:00:25, dschuyler wrote:
> optional:
> I think a space should follow the ';'
> (though I also think the line should break after the '{')

clang-format will do this (but I am not running it as part of this CL).
Specifically if one runs clang-format here it would become

var totalCells = chunks.reduce(function(accum, chunk) {
  return accum + chunk.cells.byteLength;
}, 0);

https://codereview.chromium.org/2909503003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/certificate_manager_browsertest.js (right):

https://codereview.chromium.org/2909503003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/certificate_manager_browsertest.js:251:
].forEach(CertificateManager.onPopulateTree);}));
On 2017/05/26 at 19:00:25, dschuyler wrote:
> optional: space after ;

Similar to previous comment. clang-format will handle the whitespace
distribution, as follows:

 ]].forEach(CertificateManager.onPopulateTree);
}));

I prefer not to mix "eslint --fix" and "clang-format" in the same CL. It will be
addressed if/when we decide to run clang-format across our JS codebase.

[dpapad, 2017-05-26 19:17:30]

Description was changed from

==========
WebUI: Enable ESLint rule for missing semicolons.

 - Fixing existing violations
 - Enabling http://eslint.org/docs/rules/semi check during PRESUBMIT

This enforces the following section from the styleguide:
https://google.github.io/styleguide/jsguide.html#formatting-semicolons-are-re...

BUG=720034
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
WebUI: Enable ESLint rule for missing semicolons.

 - Fixing almost all existing violations
   (remaining ones are fixed at crrev.com/2903063007).
 - Enabling http://eslint.org/docs/rules/semi check during PRESUBMIT

This enforces the following section from the styleguide:
https://google.github.io/styleguide/jsguide.html#formatting-semicolons-are-re...

BUG=720034
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2017-05-26 19:17:56]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2017-05-26 19:18:52]

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-05-26 20:08:29]

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

[commit-bot: I haz the power, 2017-05-26 20:08:32]

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...)

[dpapad, 2017-05-26 20:41:06]

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

[dpapad, 2017-05-26 20:41:08]

@dbeam: OWNER review needed for
chrome/browser/ui/webui/options/certificate_manager_browsertest.js
(that folder has a noparent,
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/options/OWNERS?l=2)

[dpapad, 2017-05-26 20:49:00]

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

[dpapad, 2017-05-26 20:49:01]

Redirecting to stevenjb, since dbeam is OOO:

@stevenjb: OWNER review needed for
chrome/browser/ui/webui/options/certificate_manager_browsertest.js

(that folder has a noparent,
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/options/OWNERS?l=2)

[michaelpg, 2017-05-26 22:13:30]

chromevox is an extension, not WebUI

On Fri, May 26, 2017, 1:49 PM dpapad@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:

> Redirecting to stevenjb, since dbeam is OOO:
>
> @stevenjb: OWNER review needed for
>
> chrome/browser/ui/webui/options/certificate_manager_browsertest.js
>
> (that folder has a noparent,
>
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/options/OWNERS?l=2
> )
>
> https://codereview.chromium.org/2909503003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dpapad, 2017-05-26 22:23:14]

On 2017/05/26 at 22:13:30, michaelpg wrote:
> chromevox is an extension, not WebUI

chromevox is subject to our existing web_dev_styleguide checks because of
https://cs.chromium.org/chromium/src/chrome/browser/resources/PRESUBMIT.py?l=128.
ESLint checks run exactly on the same files where our other existing files.

Is your objection about running those checks in ChromeVox code? Or just about
the CL description not being 100% accurate because of the "WebUI" prefix?

[dpapad, 2017-05-26 22:24:50]

On 2017/05/26 at 22:23:14, dpapad wrote:
> On 2017/05/26 at 22:13:30, michaelpg wrote:
> > chromevox is an extension, not WebUI
> 
> chromevox is subject to our existing web_dev_styleguide checks because of
https://cs.chromium.org/chromium/src/chrome/browser/resources/PRESUBMIT.py?l=128.
ESLint checks run exactly on the same files where our other existing files.
> 
> Is your objection about running those checks in ChromeVox code? Or just about
the CL description not being 100% accurate because of the "WebUI" prefix?

Correction: ESLint checks run exactly on the same files where our other existing
checks
(https://cs.chromium.org/chromium/src/tools/web_dev_style/js_checker.py?type=c...).

[michaelpg, 2017-05-26 23:35:36]

On 2017/05/26 22:24:50, dpapad wrote:
> On 2017/05/26 at 22:23:14, dpapad wrote:
> > On 2017/05/26 at 22:13:30, michaelpg wrote:
> > > chromevox is an extension, not WebUI
> > 
> > chromevox is subject to our existing web_dev_styleguide checks because of
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/PRESUBMIT.py?l=128.
> ESLint checks run exactly on the same files where our other existing files.
> > 
> > Is your objection about running those checks in ChromeVox code? Or just
about
> the CL description not being 100% accurate because of the "WebUI" prefix?
> 
> Correction: ESLint checks run exactly on the same files where our other
existing
> checks
>
(https://cs.chromium.org/chromium/src/tools/web_dev_style/js_checker.py?type=c...).

Sorry for the terseness, was on my phone. Honestly it was just something to be
aware of. We have other non-WebUI JS that will probably get included in the same
ESLint checks, especially component extensions. AFAICR the PSA's, Intent to
Implement and docs have only referred to WebUI. If, say, Hangouts or File
Manager files will undergo churn from linting they might want a heads up.

Just adding a note to the doc saying that other JS like component extensions may
be linted would LGTM.

[michaelpg, 2017-05-26 23:37:34]

https://codereview.chromium.org/2909503003/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/certificate_manager_browsertest.js (right):

https://codereview.chromium.org/2909503003/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/certificate_manager_browsertest.js:251:
].forEach(CertificateManager.onPopulateTree);}));
On 2017/05/26 19:08:39, dpapad wrote:
> On 2017/05/26 at 19:00:25, dschuyler wrote:
> > optional: space after ;
> 
> Similar to previous comment. clang-format will handle the whitespace
> distribution, as follows:
> 
>  ]].forEach(CertificateManager.onPopulateTree);
> }));
> 
> I prefer not to mix "eslint --fix" and "clang-format" in the same CL. It will
be
> addressed if/when we decide to run clang-format across our JS codebase.

+1, keeping them separate should make reviews and rebases easier.

[dpapad, 2017-05-27 00:02:16]

> Just adding a note to the doc saying that other JS like component extensions
may be linted would LGTM.

Thanks for pointing this out. I clarified this in the document, by changing the
title and adding an explanatory sentence.

Still need either Steven's or Dan's LGTM though, because of the noparent in
options/.

[dpapad, 2017-05-30 20:28:57]

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

[michaelpg, 2017-05-30 21:12:07]

On 2017/05/27 00:02:16, dpapad wrote:
> > Just adding a note to the doc saying that other JS like component extensions
> may be linted would LGTM.
> 
> Thanks for pointing this out. I clarified this in the document, by changing
the
> title and adding an explanatory sentence.
> 
> Still need either Steven's or Dan's LGTM though, because of the noparent in
> options/.

the noparent is just to prevent people from adding things to old options. Up to
you if you care to TBR though.

[stevenjb, 2017-05-30 23:25:14]

lgtm

[dpapad, 2017-05-31 00:22:34]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2017-05-31 00:23:14]

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-05-31 01:51:48]

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

[commit-bot: I haz the power, 2017-05-31 01:51:50]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[dpapad, 2017-05-31 02:41:10]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2017-05-31 02:41:43]

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-05-31 03:31:28]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496198470196660,
"parent_rev": "ad6d7db1af8ce80131088c20bd5480d413e9c3d7", "commit_rev":
"1e511b111bdebc1cdb7588d9c22e034ab70715b9"}

[commit-bot: I haz the power, 2017-05-31 03:31:39]

Description was changed from

==========
WebUI: Enable ESLint rule for missing semicolons.

 - Fixing almost all existing violations
   (remaining ones are fixed at crrev.com/2903063007).
 - Enabling http://eslint.org/docs/rules/semi check during PRESUBMIT

This enforces the following section from the styleguide:
https://google.github.io/styleguide/jsguide.html#formatting-semicolons-are-re...

BUG=720034
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
WebUI: Enable ESLint rule for missing semicolons.

 - Fixing almost all existing violations
   (remaining ones are fixed at crrev.com/2903063007).
 - Enabling http://eslint.org/docs/rules/semi check during PRESUBMIT

This enforces the following section from the styleguide:
https://google.github.io/styleguide/jsguide.html#formatting-semicolons-are-re...

BUG=720034
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2909503003
Cr-Commit-Position: refs/heads/master@{#475771}
Committed:
https://chromium.googlesource.com/chromium/src/+/1e511b111bdebc1cdb7588d9c22e...
==========

[commit-bot: I haz the power, 2017-05-31 03:31:41]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/1e511b111bdebc1cdb7588d9c22e...