As we're moving to a no-XUL no-chrome world, we need to make sure we don't introduce new usages of inIDOMUtils in our client code. We have a number of them now: https://dxr.mozilla.org/mozilla-central/search?q=inIDOMUtils+path%3Adevtools%2Fclient&redirect=false&case=false All of which will need to be removed during the course of the devtools.html project. We should make sure we don't re-introduce new usages in the future. To do this, I suggest an ESLint rule that checks for inIDOMUtils in files. And each line that use it today can be annotated with // eslint-disable-line Then it'd be up to reviewers to check that new usages with this annotations aren't added.

This should also check for Ci.nsIDOM* constants.

We should create a more general rule. Looking at the inspector code (as an example), there are things like: Cu.import, Ci.nsIClipboardHelper, Ci.nsIThread, Ci.nsIFocusManager, Ci.nsIXULRuntime, Ci.nsIPrincipal, Ci.nsIDOMCSSRule, Ci.nsIIOService, Ci.nsIXULChromeRegistry, Ci.nsIPrefLocalizedString, Ci.nsIDOMParser, ... Basically everything we created a shim for in the devtools.html prototype in 2015: https://github.com/jlongster/ff-devtools-libs/blob/master/sham/chrome.js

When going through the files I think we've been looking for uses of require("chrome"), Ci, Cu, Cc, Cr, Services, imports of jsms, and requires of things from sdk/.

We probably also must reject chrome: and resource: URLs, and require()s of .jsm files.

I have a rule to selectively reject require()s based on what is being required. It uses: /^(chrome|chrome:.*|resource:.*|devtools\/server\/.*)/.test(path) However the difficulty is that we can't actually enable it until we're further along.

(In reply to Tom Tromey :tromey from comment #5) > I have a rule to selectively reject require()s based on what is being > required. > It uses: > > /^(chrome|chrome:.*|resource:.*|devtools\/server\/.*)/.test(path) > > However the difficulty is that we can't actually enable it until we're > further along. Or we could only enable it for the few files that don't require chrome stuff anymore. Since this is only ever going to be important for client code, we could add an .eslintrc file in devtools/client that enables it. And then add comments that disable the rule in the files that haven't been converted yet. Or if it's too soon/too many files, maybe we can add the .eslintrc just in the devtools/client/inspector subfolder for the moment.

Review commit: https://reviewboard.mozilla.org/r/66058/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/66058/

Review commit: https://reviewboard.mozilla.org/r/66060/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/66060/

Just a reminder that adding new rules need to be packaged for automation: 1. Bump the version number[1] of the ESLint plugin 2. Update the version the mach command checks for to match (so local users know to update) 3. Update the ESLint archive in tooltool using the update script[3] so automation uses the new code [1]: https://dxr.mozilla.org/mozilla-central/source/tools/lint/eslint/eslint-plugin-mozilla/package.json [2]: https://dxr.mozilla.org/mozilla-central/source/tools/lint/mach_commands.py#36 [3]: https://dxr.mozilla.org/mozilla-central/source/tools/lint/eslint/update The tooltool steps need special access. Ping me or Mike for help with this.

Comment on attachment 8773314 [details] Bug 1264649 - add reject-some-requires eslint rule; https://reviewboard.mozilla.org/r/66058/#review63152 ::: devtools/.eslintrc:39 (Diff revision 1) > "mozilla/no-aArgs": 1, > "mozilla/no-cpows-in-tests": 2, > "mozilla/no-single-arg-cu-import": 2, > // See bug 1224289. > "mozilla/reject-importGlobalProperties": 2, > + "mozilla/reject-some-requires": [2, "^(chrome|chrome:.*|resource:.*|devtools/server/.*|\\.jsm)$"], You're removing this in the next commit. So there's no need to add it here in the first place. ::: tools/lint/eslint/eslint-plugin-mozilla/lib/rules/reject-some-requires.js:35 (Diff revision 1) > + node.arguments.length == 1 && > + node.arguments[0].type == "Literal") { > + let path = node.arguments[0].value; > + > + if (RX.test(path)) { > + context.report(node, `invalid require(${path}) for content code`); I think this needs rephrasing. This rule is a general one that people may find useful for all sorts of stuff. We happen to use it to prevent some kinds of require in "content" code, but not everyone is going to use it for this. So, maybe something like: `requiring a module from ${path} is not allowed`

Comment on attachment 8773315 [details] Bug 1264649 - enable reject-some-requires rule for inspector; https://reviewboard.mozilla.org/r/66060/#review63156 ::: devtools/client/inspector/.eslintrc:6 (Diff revision 1) > +{ > + // Extend from the devtools eslintrc. > + "extends": "../../.eslintrc", > + > + "rules": { > + "mozilla/reject-some-requires": [2, "^(chrome|chrome:.*|resource:.*|devtools/server/.*|\\.jsm)$"], I think we should add a comment here because this is an ongoing effort. Only 9 out of 22 files in the inspector already comply to this rule, so there's still a lot to do. Don't know if this is the best place, but I couldn't think of any other. I'd add something like: // The inspector is being migrated to HTML and cleaned of chrome-privileged code, so this rule disallows requiring chrome code. Some files in the inspector disable this rule still. The goal is to enable the rule globally on all files.

(In reply to Patrick Brosset <:pbro> (PTO until August 16, not doing reviews) from comment #10) > > + "mozilla/reject-some-requires": [2, "^(chrome|chrome:.*|resource:.*|devtools/server/.*|\\.jsm)$"], > > You're removing this in the next commit. So there's no need to add it here > in the first place. Thanks for noticing that. It was a leftover from some testing, oops.

(In reply to Patrick Brosset <:pbro> (PTO until August 16, not doing reviews) from comment #11) > I think we should add a comment here because this is an ongoing effort. Only > 9 out of 22 files in the inspector already comply to this rule, so there's > still a lot to do. I've also updated the patch to re-enable the rule after each bad require. So now a given require might look like: /* eslint-disable mozilla/reject-some-requires */ var { XPCOMUtils } = require("resource://gre/modules/XPCOMUtils.jsm"); /* eslint-enable mozilla/reject-some-requires */ The idea here is that it will be obvious now that removing the require should also mean removing the eslint comment.

(In reply to Tom Tromey :tromey from comment #13) > /* eslint-disable mozilla/reject-some-requires */ > var { XPCOMUtils } = require("resource://gre/modules/XPCOMUtils.jsm"); > /* eslint-enable mozilla/reject-some-requires */ Great idea!

I belatedly realized that this has to check lazyRequireGetter as well.

Comment on attachment 8773314 [details] Bug 1264649 - add reject-some-requires eslint rule; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66058/diff/1-2/

Comment on attachment 8773315 [details] Bug 1264649 - enable reject-some-requires rule for inspector; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66060/diff/1-2/

:jryans - pinging about tooltool

Please fold this additional patch into your new rule patch. With this added in, you can see the effect of your changes on try and should be ready to land if it's green.

Comment on attachment 8773314 [details] Bug 1264649 - add reject-some-requires eslint rule; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66058/diff/2-3/

Comment on attachment 8773315 [details] Bug 1264649 - enable reject-some-requires rule for inspector; Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66060/diff/2-3/

Comment on attachment 8773857 [details] [diff] [review] tooltool upload addendum Rolled in.

Pushed by ttromey@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a81edfa43440 add reject-some-requires eslint rule; r=pbro https://hg.mozilla.org/integration/autoland/rev/eb5d2f565ba4 enable reject-some-requires rule for inspector; r=pbro

https://hg.mozilla.org/mozilla-central/rev/a81edfa43440 https://hg.mozilla.org/mozilla-central/rev/eb5d2f565ba4