Bug 767640, comment 32: "Mass replace the remaining Components.interface/Components.utils uses with Ci/Cu (I'm hesitant about Components.classes -> Cc because I'm not sure we have a standard way to deal with the indent). And after doing this, covering it with eslint would make sense."

Bug 1230369 is already on file for Components.interface -> Ci etc, so linking that here.

This is produced by the CcCiCuCr.js script at https://bitbucket.org/fqueze/xpcshell-rewrites/commits/58568164a69000622267cf73b8d6e91d7fa4694c and should be quite safe. It fixes the indentation when it's confident.

The previous patch only fixes the indent when it was correct before the rewrite. This second patch was generated with the help of a modified version of the first script (to attempt to fix the indent even when it was originally wrong), but is finished by hand, so it should be reviewed as if it had been produced by hand.

This is generated by the more aggressive inCcCiCuCr.js script, which replaces all the instances that remained. I had to revert some by hand (and add them to the exclusion list of the script) to fix some test failures. This is slightly more risky than part 1 and needs review.

Enable the already existing eslint rule for this, and finish by hand what the script didn't dare touching (my script only changes CDATA section inside .xml and .xhtml files).

Try is now mostly green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=52027e6fc63629e0c871662ff17ab36b9a04e8e9 (except for one eslint error, and one JS error in robocop tests (android) that I need to fix)

Comment on attachment 8954433 [details] [diff] [review] part 1 - large scripted patch Review of attachment 8954433 [details] [diff] [review]: ----------------------------------------------------------------- Please remove the changes from npm-shrinkwrap.json

Comment on attachment 8954436 [details] [diff] [review] part 3 - more aggressive scripted patch Review of attachment 8954436 [details] [diff] [review]: ----------------------------------------------------------------- So in a bunch of places we just do things like "CC = Cc", "CI = Ci" etc. I'm inclined to change them where capitalisation is the only difference, but I'm open to discussing how strict/lenient to be there. There are a few clear bugs in this patch though. ::: docshell/test/unit/test_pb_notification.js @@ +3,2 @@ > if (typeof Ci === "undefined") > + Ci = Ci; This looks wrong! ::: dom/base/test/unit/test_range.js @@ +3,5 @@ > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > Cu.importGlobalProperties(["NodeFilter"]); > > +const C_i = Ci; I think it would be best to rename C_i to Ci in this file unless there is some reason against it. ::: dom/tests/mochitest/chrome/test_sandbox_eventhandler.xul @@ +18,5 @@ > <script type="application/javascript"> > <![CDATA[ > > /** Test for Bug 817284 **/ > + var cu = Cu; And can we change cu to Cu here? ::: dom/tests/mochitest/general/test_innerScreen.xul @@ +49,5 @@ > > var f = document.getElementById("f"); > var fBounds = f.getBoundingClientRect(); > > + const CI = Ci; And CI to Ci here? ::: intl/uconv/tests/unit/test_input_stream.js @@ +1,2 @@ > +var Ci = Ci, > + Cc = Cc, Probably best not ::: js/xpconnect/tests/unit/test_allowedDomains.js @@ +1,2 @@ > function run_test() { > + var cu = Cu; Change to Cu throughout ::: js/xpconnect/tests/unit/test_allowedDomainsXHR.js @@ +1,2 @@ > > +var cu = Cu; Change to Cu throughout ::: js/xpconnect/tests/unit/test_want_components.js @@ +1,2 @@ > function run_test() { > + var cu = Cu; Change to Cu throughout ::: layout/inspector/tests/chrome/test_bug467669.xul @@ +18,5 @@ > SimpleTest.waitForExplicitFinish(); > > function RunTest() { > + const CI = Ci; > + const CC = Cc; Change to Ci and Cc throughout ::: layout/inspector/tests/chrome/test_bug695639.xul @@ +16,5 @@ > SimpleTest.waitForExplicitFinish(); > > function RunTest() { > + const CI = Ci; > + const CC = Cc; Change to Ci and Cc throughout ::: layout/tools/recording/recording.js @@ +4,5 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > +var CC = Cc; > +const CI = Ci; Change to Ci and Cc throughout ::: layout/tools/reftest/manifest.jsm @@ +8,5 @@ > var EXPORTED_SYMBOLS = ["ReadTopManifest", "CreateUrls"]; > > +var CC = Cc; > +const CI = Ci; > +const CU = Cu; Change to Ci, Cu and Cc throughout ::: layout/tools/reftest/reftest-content.js @@ +6,5 @@ > > +var CC = Cc; > +const CI = Ci; > +const CR = Cr; > +const CU = Cu; Change to Ci, Cu, Cr and Cc throughout ::: layout/tools/reftest/reftest.jsm @@ +13,5 @@ > > +var CC = Cc; > +const CI = Ci; > +const CR = Cr; > +const CU = Cu; Change to Ci, Cu, Cr and Cc throughout ::: mobile/android/chrome/content/about.js @@ +1,5 @@ > /* This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > +var Ci = Ci, Cc = Cc, Cu = Cu, Cr = Cr; Unnecessary ::: services/sync/tps/extensions/tps/components/tps-cmdline.js @@ +2,5 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > +const CC = Cc; > +const CI = Ci; Change to Ci and Cc throughout ::: testing/mochitest/tests/SimpleTest/SimpleTest.js @@ +867,5 @@ > // be forwarded to the child process. > // XXXndeakin now sure what this issue with Components.utils is about, but > // browser tests require the former and plain tests require the latter. > + var Cu = Cu || SpecialPowers.Cu; > + var Ci = Ci || SpecialPowers.Ci; This looks suspicious to me, I suspect since the var is hoisted by the time the assignment happens you're already assigning the local variable to itself which would give you undefined. ::: toolkit/content/widgets/textbox.xml @@ +525,5 @@ > <getter><![CDATA[ > if (!this._spellCheckInitialized) { > this._spellCheckInitialized = true; > > + const CI = Ci; Change to Ci throughout.

Comment on attachment 8954437 [details] [diff] [review] part 4 - enable the use-cc-etc eslint rule Review of attachment 8954437 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/test/file.js @@ +108,5 @@ > } > > function verifyBlob(blob1, blob2, fileId, blobReadHandler) > { > + // eslint-disable-next-line mozilla/use-cc-etc Why is this needed? ::: toolkit/components/ctypes/tests/unit/test_jsctypes.js @@ +1923,5 @@ > let t4_t = f4_t.ptr.ptr.array(8).array(); > Assert.equal(t4_t.name, "char*(*(**[][8])(int32_t, g_t(*)()))[]"); > > // Not available in a Worker > + // eslint-disable-next-line mozilla/use-cc-etc Why is this needed?

(In reply to Dave Townsend [:mossop] from comment #10) > Comment on attachment 8954437 [details] [diff] [review] > part 4 - enable the use-cc-etc eslint rule > > Review of attachment 8954437 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/indexedDB/test/file.js > @@ +108,5 @@ > > } > > > > function verifyBlob(blob1, blob2, fileId, blobReadHandler) > > { > > + // eslint-disable-next-line mozilla/use-cc-etc > > Why is this needed? > > ::: toolkit/components/ctypes/tests/unit/test_jsctypes.js > @@ +1923,5 @@ > > let t4_t = f4_t.ptr.ptr.array(8).array(); > > Assert.equal(t4_t.name, "char*(*(**[][8])(int32_t, g_t(*)()))[]"); > > > > // Not available in a Worker > > + // eslint-disable-next-line mozilla/use-cc-etc > > Why is this needed? Ci, Cc, Cu don't exist in the content JS scopes of plain mochitests. 'Components' exists there as a shim. These 2 places are things that the scripts replaced, and that I had to revert to fix test failures.

This is changes done by hand to address comment 9.

This is similar to attachment 8954436 [details] [diff] [review] but all the parts mentionned in Mossop's comment 9 have been removed (either by doing changes by hand in the previous patch, or by adding the files in the script's exclusion list).

This is an updated version of part 4 that Mossop already r+'d. The changes are small enough that I would be ok carrying forward his r+, but I figured you may want to have a look at the (hopefully) final version. The changes since the previous version are: - the manual changes in tabbrowser.xml are no longer needed as that code has been cleaned up and moved to tabbrowser.js already. - eslint-disable mozilla/use-cc-etc in robocop_head.js (I reverted all scripted changes to that file as I had robocop test failures saying "Cc is undefined"). - eslint-disable-line mozilla/use-cc-etc in testing/xpcshell/dbg-actors.js (I also reverted scripted changes to that file to fix test failures).

Comment on attachment 8954784 [details] [diff] [review] part 4 - enable the use-cc-etc eslint rule, v2 Review of attachment 8954784 [details] [diff] [review]: ----------------------------------------------------------------- Nice!

Pushed by florian@queze.net: https://hg.mozilla.org/mozilla-central/rev/7bbd1a09eacb scripted patch to replace Components.classes[, Components.interfaces.nsI, Components.utils. and Components.results. with Cc, Ci, Cu and Cr, r=Mossop. https://hg.mozilla.org/mozilla-central/rev/3c6de76d1855 semi-automated indent fix, r=Mossop. https://hg.mozilla.org/mozilla-central/rev/3e44c465d268 remove by hands some variations of Cc,Ci,Cu definitions, r=Standard8. https://hg.mozilla.org/mozilla-central/rev/3c2b0756aa87 more aggressive scripted patch to replace remaining Components.classes, Components.interfaces, Components.utils and Components.results uses with Cc, Ci, Cu and Cr, r=Mossop. https://hg.mozilla.org/mozilla-central/rev/3f83ced4a8a5 enable the use-cc-etc eslint rule, r=Standard8. https://hg.mozilla.org/mozilla-central/rev/f9cbd1358fb8 Fix xpcshell tests, 'Cc' isn't defined in that scope, so use _Services.tm directly, r=Mossop, a=Aryx on CLOSED TREE