These scripts only have access to globals explicitly exported via the `global` object, not every global defined in every other API script.

I do prefer the way the code looks after this patch, but this is effectively a revert of part of bug 1371292, lets make sure we've got consensus on how we want this to look instead of just going back and forth. Mark/Dave, bug 1371292 describes a "what" without a "why". Like I said above, I think the way we handle shared globals in webextensions is nicer with this patch than with all the import-globals-from directives, any objections?

There's several reasons I prefer not to include globals in .eslintrc.js files wherever possible: - If globals are removed, the .eslintrc.js files can be easily forgotten, leading to later updates. Additionally, if a global is added, the developer may not realise straight away they need to add it there. - In extensions/ case it is very clear that ESLint will pretend globals are available in the non ext-*.js files. - The globals defined here will also apply to the test directories as well. Generally, I think this adds up to: 1) harder to maintain, 2) more likely for developers to make mistakes and take longer to find those mistakes (e.g. being altered in the editor vs manual/automated test failing). Although testing should cover all cases, we know from history it doesn't always. That's the main reasons I wanted to change. However, the fact that only the items on `global` are exported would make it harder for ESLint. We could possibly write a custom rule to manage this, as we have done with other globals, though that will likely take a bit of time as there's other ESLint things we need to deal with at the moment. In the meantime, I personally would be slightly happier if the ext-* files were in their own sub-directories, this would avoid "leaking" the globals to other files - though this is really a call for you who touch the code frequently. In any case, the reasons for adding the globals to the .eslintrc.js files (why there are there, where they come from) needs to be clearly documented in each of the files - it is easy to forget, and new developers need to know.

I'll defer to Mark here as the de-facto owner of eslint.

I think we just have a simple difference of opinion. I found having the list of globals centralized to be much neater and I the things you mentioned in comment 3 weren't actually problems in practice. I think there's also a bigger philosophical difference (eg, relying on lint to catch things that unit tests don't catch seems exactly backward to me). As a larger policy matter, I think that this type of conflict is likely to keep occurring if there is a strong push to enforce a single standard across the entire source tree. That all said, I suspect Kris has the strongest opinions about what to do with the webextensions code. Kris, where do you want to go with this bug?

(In reply to Andrew Swan [:aswan] from comment #5) > I think there's also a > bigger philosophical difference (eg, relying on lint to catch things that > unit tests don't catch seems exactly backward to me). If we had 100% test coverage across the whole tree or even parts of it I'd agree with you. Unfortunately we don't, and as we were enabling no-undef we found various bugs in lesser used bits of the tree that weren't covered by tests. For bits that are covered by tests then my personal opinion tends to revert to make it quicker for developers to see issues, e.g. something being highlighted before they leave the editor to run something. > As a larger policy matter, I think that this type of conflict is likely to > keep occurring if there is a strong push to enforce a single standard across > the entire source tree. To clarify: I'm fine for reverting the WebExtensions code. This is the first case where we've needed to do something slightly different, but there's good reasons for that which I hadn't realised before (the assigning to `global.`). I still think there's some potential pitfalls (as explained), but if the team is happy accepting those then I'm happy for this to be reverted - as long as the globals list is documented as to where it comes from and why it is specified there.

I agree that Mark's request to document the handling of the globals is a good idea, Kris can you update the patch to do that?

Comment on attachment 8962238 [details] Bug 1372406: Stop misusing import-globals-from in extension API scripts. https://reviewboard.mozilla.org/r/231078/#review237302

https://hg.mozilla.org/integration/mozilla-inbound/rev/6af40fb76692ad647645194c7458c1be228378aa Bug 1372406: Stop misusing import-globals-from in extension API scripts. r=aswan

Backed out changeset 6af40fb76692 (bug 1372406) for Doc lint failure in builds/worker/checkouts/gecko/docs-out/html/main/_staging/python/mach.commands.rst Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=6af40fb76692ad647645194c7458c1be228378aa&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=success&selectedJob=170716248 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=170716248&repo=mozilla-inbound&lineNumber=446 Backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=dea16926510733a02d98073d3a64bf7279462b99&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=success There are also some 1 failures: https://treeherder.mozilla.org/logviewer.html#?job_id=170720761&repo=mozilla-inbound&lineNumber=2468

That failure is almost certainly unrelated to this patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8709491f23da3f8d7db26ba1bab470bcd5e0de6c Bug 1372406: Stop misusing import-globals-from in extension API scripts. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/2107d3d5b82265a1d4b243149b76c09c8873d260 Bug 1372406: Follow-up: Disable test_ext_all_apis for being flaky. r=bustage DONTBUILD https://hg.mozilla.org/integration/mozilla-inbound/rev/1d0d349cd2c4ec4b42766194b5802fb1505fa175 Bug 1372406: Follow-up: Add missing source directory to docs config. r=bustage

https://hg.mozilla.org/integration/mozilla-inbound/rev/19cd462bbc4e3efd3e905bd6ce8ef036d593577a Bug 1372406: Follow-up: Fix rebase botch. r=bustage

https://hg.mozilla.org/mozilla-central/rev/8709491f23da https://hg.mozilla.org/mozilla-central/rev/2107d3d5b822 https://hg.mozilla.org/mozilla-central/rev/1d0d349cd2c4 https://hg.mozilla.org/mozilla-central/rev/19cd462bbc4e