WebUI: Fix/suppress some existing violations of no-restricted-globals.

WebUI: Fix/suppress some existing violations of no-restricted-globals.

Adding suppressions for cases where util.js document.getElementById can't
be used because the code runs on an extension or content_script, or just
because a dependency to util.js is unwanted.

For remaining cases, actually replacing document.getElementById() with $().

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

Review-Url: https://codereview.chromium.org/2921783003
Cr-Commit-Position: refs/heads/master@{#477419}
Committed: https://chromium.googlesource.com/chromium/src/+/4bcb1a848a99744f64f48301f5f797fb09b30e2c

[dpapad, 2017-06-02 01:36:55]

Description was changed from

==========
WebUI: Fix some existing violations of no-restricted-globals.

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

to

==========
WebUI: Fix some existing violations of no-restricted-globals.

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

[dpapad, 2017-06-02 21:23:04]

Description was changed from

==========
WebUI: Fix some existing violations of no-restricted-globals.

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

to

==========
WebUI: Fix/suppress some existing violations of no-restricted-globals.

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

[dpapad, 2017-06-02 21:41:38]

Description was changed from

==========
WebUI: Fix/suppress some existing violations of no-restricted-globals.

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

to

==========
WebUI: Fix/suppress some existing violations of no-restricted-globals.

Adding suppressions for cases where util.js can't be used because the code
runs on an extension or content_script, or just because a dependency to
util.js is unwanted.

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

[dpapad, 2017-06-02 21:41:43]

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

[commit-bot: I haz the power, 2017-06-02 21:42:17]

Dry run: CQ is trying da patch.

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

[dpapad, 2017-06-02 21:47:40]

Description was changed from

==========
WebUI: Fix/suppress some existing violations of no-restricted-globals.

Adding suppressions for cases where util.js can't be used because the code
runs on an extension or content_script, or just because a dependency to
util.js is unwanted.

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

to

==========
WebUI: Fix/suppress some existing violations of no-restricted-globals.

Adding suppressions for cases where util.js can't be used because the code
runs on an extension or content_script, or just because a dependency to
util.js is unwanted.

For remaining cases, actually replacing document.getElementById() with $().

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

[dpapad, 2017-06-02 22:27:08]

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

[dpapad, 2017-06-02 22:27:08]

The amount of exceptions needed for this rule is a bit unsatisfactory, but
fortunately we have now a way to customize per-folder or even inline.

In general, I think as we move more and more code within Custom Elements, where
document.getElementById is useless, this restriction will become less important,
and perhaps we could even remove it completely.

[dpapad, 2017-06-02 22:27:52]

Description was changed from

==========
WebUI: Fix/suppress some existing violations of no-restricted-globals.

Adding suppressions for cases where util.js can't be used because the code
runs on an extension or content_script, or just because a dependency to
util.js is unwanted.

For remaining cases, actually replacing document.getElementById() with $().

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

to

==========
WebUI: Fix/suppress some existing violations of no-restricted-globals.

Adding suppressions for cases where util.js document.getElementById can't
be used because the code runs on an extension or content_script, or just
because a dependency to util.js is unwanted.

For remaining cases, actually replacing document.getElementById() with $().

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

[dpapad, 2017-06-02 22:35:05]

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

[Dan Beam, 2017-06-02 23:01:58]

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

[Dan Beam, 2017-06-02 23:01:59]

https://codereview.chromium.org/2921783003/diff/60001/ui/webui/resources/js/u...
File ui/webui/resources/js/util.js (right):

https://codereview.chromium.org/2921783003/diff/60001/ui/webui/resources/js/u...
ui/webui/resources/js/util.js:15: /* eslint-disable no-restricted-properties */
why not

var el = document.getElementById(id);  // eslist-disable-line

or any number of ways documented here that might be less crufty?
http://eslint.org/docs/user-guide/configuring

[commit-bot: I haz the power, 2017-06-02 23:26:16]

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

[commit-bot: I haz the power, 2017-06-02 23:26:19]

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

[dpapad, 2017-06-02 23:30:03]

https://codereview.chromium.org/2921783003/diff/60001/ui/webui/resources/js/u...
File ui/webui/resources/js/util.js (right):

https://codereview.chromium.org/2921783003/diff/60001/ui/webui/resources/js/u...
ui/webui/resources/js/util.js:15: /* eslint-disable no-restricted-properties */
On 2017/06/02 at 23:01:58, Dan Beam wrote:
> why not
> 
> var el = document.getElementById(id);  // eslist-disable-line
> 
> or any number of ways documented here that might be less crufty?
> http://eslint.org/docs/user-guide/configuring

Done. I was not aware of eslint-disable-line and eslint-disable-next-line

[michaelpg, 2017-06-03 00:04:49]

I must've missed a prior CL, but what's wrong with document.getElementById?

[dpapad, 2017-06-03 00:08:18]

On 2017/06/03 at 00:04:49, michaelpg wrote:
> I must've missed a prior CL, but what's wrong with document.getElementById?

You only missed the fact that I migrated an existing (and fairly old check) to
be implemented via ESlint at https://codereview.chromium.org/2907763004). The
restriction was also mentioned in the styleguide at
https://chromium.googlesource.com/chromium/src/+/master/styleguide/web/web.md....

My guess is that this check had been added because $() is shorter. I can't think
of any other reason it would be preferable.

[Dan Beam, 2017-06-03 00:08:49]

On 2017/06/03 00:08:18, dpapad wrote:
> On 2017/06/03 at 00:04:49, michaelpg wrote:
> > I must've missed a prior CL, but what's wrong with document.getElementById?
> 
> You only missed the fact that I migrated an existing (and fairly old check) to
> be implemented via ESlint at https://codereview.chromium.org/2907763004). The
> restriction was also mentioned in the styleguide at
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/web/web.md....
> 
> My guess is that this check had been added because $() is shorter. I can't
think
> of any other reason it would be preferable.

dpapad@ and I are kinddda wondering that ourselves

[dpapad, 2017-06-05 19:01:07]

On 2017/06/03 at 00:08:49, dbeam wrote:
> On 2017/06/03 00:08:18, dpapad wrote:
> > On 2017/06/03 at 00:04:49, michaelpg wrote:
> > > I must've missed a prior CL, but what's wrong with
document.getElementById?
> > 
> > You only missed the fact that I migrated an existing (and fairly old check)
to
> > be implemented via ESlint at https://codereview.chromium.org/2907763004).
The
> > restriction was also mentioned in the styleguide at
> >
https://chromium.googlesource.com/chromium/src/+/master/styleguide/web/web.md....
> > 
> > My guess is that this check had been added because $() is shorter. I can't
think
> > of any other reason it would be preferable.
> 
> dpapad@ and I are kinddda wondering that ourselves

Friendly ping, since this is blocking others who are trying to modify those
files. We can evaluate whether we want to completely remove this check, or make
it a warning separately.

[Dan Beam, 2017-06-05 19:04:01]

https://codereview.chromium.org/2921783003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/.eslintrc.js (right):

https://codereview.chromium.org/2921783003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/.eslintrc.js:13:
'no-restricted-properties': 'off',
what if we write other checked based on no-restricted properties in the future
other than getElementById vs $?

[dpapad, 2017-06-05 19:11:58]

https://codereview.chromium.org/2921783003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/.eslintrc.js (right):

https://codereview.chromium.org/2921783003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/.eslintrc.js:13:
'no-restricted-properties': 'off',
On 2017/06/05 at 19:04:01, Dan Beam wrote:
> what if we write other checked based on no-restricted properties in the future
other than getElementById vs $?

It would be disabled also. The granularity we have from within this config file
is at the rule level. A rule can be turned on/off as a whole.

[Dan Beam, 2017-06-05 19:23:10]

lgtm

https://codereview.chromium.org/2921783003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/.eslintrc.js (right):

https://codereview.chromium.org/2921783003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/.eslintrc.js:13:
'no-restricted-properties': 'off',
On 2017/06/05 19:11:58, dpapad wrote:
> On 2017/06/05 at 19:04:01, Dan Beam wrote:
> > what if we write other checked based on no-restricted properties in the
future
> other than getElementById vs $?
> 
> It would be disabled also. The granularity we have from within this config
file
> is at the rule level. A rule can be turned on/off as a whole.

i was mainly asking to see if we have a plan.

there's 51 of occurrences of `document.getElementById` in
chrome/browser/resources/chromeos/*vox so that's probably not work individually
disabling.

[dpapad, 2017-06-05 19:32:40]

https://codereview.chromium.org/2921783003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/chromeos/chromevox/.eslintrc.js (right):

https://codereview.chromium.org/2921783003/diff/80001/chrome/browser/resource...
chrome/browser/resources/chromeos/chromevox/.eslintrc.js:13:
'no-restricted-properties': 'off',
> i was mainly asking to see if we have a plan.
> 
> there's 51 of occurrences of `document.getElementById` in
chrome/browser/resources/chromeos/*vox so that's probably not work individually
disabling.

Yes, already checked number of occurrences before deciding to go with the config
file approach. I don't have a great plan for this yet. Issue
https://github.com/eslint/eslint/issues/7957 seems related. Eventually I think
we need help from ESLint to address this problem, by being able to override
sub-rule properties, instead of turning off the entire rule. I'll ask on their
tracker for this.

[michaelpg, 2017-06-05 21:05:35]

Description was changed from

==========
WebUI: Fix/suppress some existing violations of no-restricted-globals.

Adding suppressions for cases where util.js document.getElementById can't
be used because the code runs on an extension or content_script, or just
because a dependency to util.js is unwanted.

For remaining cases, actually replacing document.getElementById() with $().

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

to

==========
WebUI: Fix/suppress some existing violations of no-restricted-globals.

Adding suppressions for cases where util.js document.getElementById can't
be used because the code runs on an extension or content_script, or just
because a dependency to util.js is unwanted.

For remaining cases, actually replacing document.getElementById() with $().

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

[michaelpg, 2017-06-05 21:05:36]

michaelpg@chromium.org changed reviewers:
- michaelpg@chromium.org

[dpapad, 2017-06-05 21:06:20]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2017-06-05 21:07:16]

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-05 21:54:57]

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

[commit-bot: I haz the power, 2017-06-05 21:54:58]

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

[dpapad, 2017-06-06 19:25:51]

https://codereview.chromium.org/2921783003/diff/80001/ui/webui/resources/js/u...
File ui/webui/resources/js/util.js (right):

https://codereview.chromium.org/2921783003/diff/80001/ui/webui/resources/js/u...
ui/webui/resources/js/util.js:28: var el = $(id);
Well, that was the culprit for the failing tests. Justified, since getSVGElement
can't delegate to $(), because the latter asserts for HTMLELement, which SVG
elements do not inherit from.

[dpapad, 2017-06-06 19:27:58]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2017-06-06 19:27:59]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2921783003/#ps100001
(title: "Fix svg")

[commit-bot: I haz the power, 2017-06-06 19:28:26]

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-06 20:52:50]

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

[commit-bot: I haz the power, 2017-06-06 20:52:51]

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-06-06 21:06:58]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2017-06-06 21:07:35]

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-06 21:41:04]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1496783218019540,
"parent_rev": "901bfdd1ba474723ae625a0472ef19155339f779", "commit_rev":
"4bcb1a848a99744f64f48301f5f797fb09b30e2c"}

[commit-bot: I haz the power, 2017-06-06 21:41:17]

Description was changed from

==========
WebUI: Fix/suppress some existing violations of no-restricted-globals.

Adding suppressions for cases where util.js document.getElementById can't
be used because the code runs on an extension or content_script, or just
because a dependency to util.js is unwanted.

For remaining cases, actually replacing document.getElementById() with $().

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

to

==========
WebUI: Fix/suppress some existing violations of no-restricted-globals.

Adding suppressions for cases where util.js document.getElementById can't
be used because the code runs on an extension or content_script, or just
because a dependency to util.js is unwanted.

For remaining cases, actually replacing document.getElementById() with $().

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

Review-Url: https://codereview.chromium.org/2921783003
Cr-Commit-Position: refs/heads/master@{#477419}
Committed:
https://chromium.googlesource.com/chromium/src/+/4bcb1a848a99744f64f48301f5f7...
==========

[commit-bot: I haz the power, 2017-06-06 21:41:19]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/4bcb1a848a99744f64f48301f5f7...