Remove definitions of Ci, Cr, Cc, and Cu

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P2
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

unspecified
Firefox 60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(5 attachments, 5 obsolete attachments)

Assignee

Description

a year ago
In bug 767640, I made Ci, Cr, Cc, and Cu to be defined on chrome scopes. I also removed many definitions of these variables from .jsms.

I only removed the simplest form, which is stuff like
  const Cc = Components.classes;

Specifically, I used the regexp:
  ^(const|let|var)\s+(Cc|Ci|Cr|Cu)\s*=\s*Components.(classes|interfaces|results|utils)\s*;\s*$

So, stuff needs to be removed from files that don't end in .jsm.

The simple form also needs to be removed when it starts with whitespace. I was having problems with that, but the version of my patch that I was using was broken so maybe it will actually work now.

Another common form of definitions that should be removed look like this:
  const {classes: Cc, utils: Cu, interfaces: Ci} = Components;
Assignee

Comment 1

a year ago
My script in bug 767640 could probably be modified to handle these extra cases. There may be other cases I haven't come across.
Assignee

Comment 2

a year ago
I guess I'll take a crack at this...
Assignee: nobody → continuation
Assignee

Comment 3

a year ago
A few places use Cm. Maybe I should add that first just to make things a little cleaner.
(In reply to Andrew McCreight [:mccr8] from comment #3)
> A few places use Cm. Maybe I should add that first just to make things a
> little cleaner.

There are also a few uses of CC (Components.Constructor) https://searchfox.org/mozilla-central/search?q=+CC%28&case=true&path=.js
Assignee

Updated

a year ago
Depends on: 1431533
Priority: -- → P2
Assignee

Updated

a year ago
Blocks: 1433175
Assignee

Comment 5

a year ago
The set of file extensions that matched my regexes is: js, jsm, html, py, sjs, xhtml, xul

Additionally, there's is an instance of it in testing/marionette/doc/CodeStyle.md which I guess needs to be updated at some point.
Assignee

Comment 7

a year ago
This patch is entirely generated by my script.

- It covers every file with extension js, jsm, html, py, sjs, xhtml, xul.
- It removes blank lines after removed lines, when the removed lines are preceded by either blank lines or the start of a new block. The "start of a new block" is defined fairly hackily: either the line starts with //, ends with */, ends with {, """ or '''. The first two cover comments, the third one covers JS, and the final two cover JS embedded in Python. This also applies if the removed line was the first line of the file.
- It covers the pattern matching cases like "var {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;". It'll remove the entire thing if they are all either Ci, Cr, Cc or Cu, or it will remove the appropriate ones and leave the residue behind. If there's only one behind, then it will turn it into a normal, non-pattern matching variable definition. (For instance, "const { classes: Cc, Constructor: CC, interfaces: Ci, utils: Cu } = Components" becomes "const CC = Components.Constructor".)

This mostly passes eslint, but there are a few places where it complains about an empty block. I'll fix those by hand in a separate patch.

I did a try run on an earlier version of the patch and there were some test failures I need to investigate.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4908edeb447efad48e0c6c139644de2b0e59b4b4
Assignee

Comment 8

a year ago
Posted file fixing script (obsolete) —
Assignee

Comment 9

a year ago
The updated version of my autogenerated patch has:
1919 files changed, 56 insertions(+), 4325 deletions(-)
Assignee

Comment 10

a year ago
I had to blacklist test_bug790732.html and dbg-actors.js. The former because it is defining Ci in content, so the behavior isn't affected by my patch. I'm not sure why the latter doesn't work. There must be something weird about the way it gets loaded (as part of some XPCShell thing, via the devtools loader stuff), but I don't know what.
Blocks: 406371
Assignee

Comment 11

a year ago
Posted file script used to generate part 1 (obsolete) —
Attachment #8945656 - Attachment is obsolete: true
Assignee

Updated

a year ago
Attachment #8945644 - Attachment is obsolete: true
Assignee

Comment 13

a year ago
opt try run, including Talos:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=b26403a1bfd0340e8875486a5d3328f24b19c649
(debug had previously only shown the same problems as opt builds)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 17

a year ago
mozreview-review
Comment on attachment 8947903 [details]
Bug 1432992, part 2 - Manually remove some empty blocks.

https://reviewboard.mozilla.org/r/217580/#review223602

There's a similar empty block in these 3 files: testing/mochitest/browser-harness.xul testing/mochitest/harness.xul dom/events/test/test_bug415498.xul
Attachment #8947903 - Flags: review?(florian) → review+

Comment 18

a year ago
mozreview-review
Comment on attachment 8947904 [details]
Bug 1432992, part 3 - Adjust some line numbers in tests.

https://reviewboard.mozilla.org/r/217582/#review223606

rs=me if try is happy.
Attachment #8947904 - Flags: review?(florian) → review+

Comment 19

a year ago
mozreview-review
Comment on attachment 8947902 [details]
Bug 1432992, part 1 - Remove definitions of Ci, Cr, Cc, and Cu.

https://reviewboard.mozilla.org/r/217578/#review223588

This looks good, thanks for doing it!

I reviewed browser/base/content, toolkit/components/search, toolkit/content/, all the files with modified lines rather than just plain removals, all the head*.js files, and a few random files here and there.

Comments:

- tools/code-coverage/tests/xpcshell/head.js now contains only a license header, and netwerk/test/unit_ipc/head_cc.js is now empty. Please remove these 2 files as part of a manual cleanup.

- some files now have a CDATA block that starts with an empty line, when it wasn't the case before. Affected files: browser/base/content/test/chrome/test_aboutCrashed.xul dom/base/test/chrome/file_bug1139964.xul dom/base/test/chrome/file_bug549682.xul dom/base/test/chrome/test_permission_isHandlingUserInput.xul editor/libeditor/tests/test_bug1397412.xul js/xpconnect/tests/chrome/test_APIExposer.xul toolkit/content/tests/widgets/test_popupreflows.xul toolkit/mozapps/downloads/tests/chrome/test_unknownContentType_delayedbutton.xul toolkit/mozapps/downloads/tests/chrome/test_unknownContentType_dialog_layout.xul widget/tests/test_system_status_bar.xul
This is the only correctness issue I've found in the output of your script. Given that it only affects test files and that other test files already had this pattern, I'm not requiring a new version of the script or a cleanup here. I'll leave it up to you to decide if you want to cleanup these 10 files by hand.

- browser/components/places/content/placesOverlay.xul has a comment that should be removed (but you can ask Marco to do it for you in bug 406371).
Attachment #8947902 - Flags: review?(florian) → review+
Assignee

Comment 20

a year ago
(In reply to Florian Quèze [:florian] from comment #17)
> There's a similar empty block in these 3 files:

Fixed.
Assignee

Comment 21

a year ago
Attachment #8947900 - Attachment is obsolete: true
Assignee

Comment 22

a year ago
(In reply to Florian Quèze [:florian] from comment #19)
> This looks good, thanks for doing it!
Thanks for the review!

> - tools/code-coverage/tests/xpcshell/head.js now contains only a license
> header, and netwerk/test/unit_ipc/head_cc.js is now empty. Please remove
> these 2 files as part of a manual cleanup.
Fixed, in part 2.

> - some files now have a CDATA block that starts with an empty line, when it
> wasn't the case before.
Good catch. I added a CDATA case to my script's code for removing blank lines, and made sure that every file you mentioned was fixed. There were two additional instances of this issue not in your list (docshell/test/chrome/test_bug789773.xul and js/xpconnect/tests/chrome/test_cows.xul).

> - browser/components/places/content/placesOverlay.xul has a comment that
> should be removed (but you can ask Marco to do it for you in bug 406371).
Fixed in part 2.

I'm going to do a full try run to make sure there aren't any lingering issues I failed to notice so far.
Assignee

Comment 23

a year ago
(I'm going to wait to push my updated version of the patch to the bug until I'm ready to land it, to avoid churning on this huge patch, but I can upload it sooner if you prefer.)
Assignee

Comment 24

a year ago
Unfortunately, my patches have a number of problems on Android. The biggest one is that httpd.js is run from xpcshell to run reftests, and for some reason Cu is not defined, so reftests and crash tests all fail. There are also three Mochitests that seem to fail due to my patches. I don't know why the failures are Android-only.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a9a215209cfd9076c0762a754dcae3f02ee3e80
(the XPCShell failures present on all platforms are not related to my patch)

Comment 25

a year ago
(In reply to Andrew McCreight [:mccr8] from comment #24)
> Unfortunately, my patches have a number of problems on Android. The biggest
> one is that httpd.js is run from xpcshell to run reftests, and for some
> reason Cu is not defined, so reftests and crash tests all fail. There are
> also three Mochitests that seem to fail due to my patches. I don't know why
> the failures are Android-only.
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=7a9a215209cfd9076c0762a754dcae3f02ee3e80
> (the XPCShell failures present on all platforms are not related to my patch)

Could this be the same hostutils stuff that plagued kmag's patches in bug 1431533? I didn't follow closely, but off-hand it sounds like it could be similar.

Comment 26

a year ago
(In reply to :Gijs from comment #25)
> the same hostutils stuff

x-ref bug 1433279.
Assignee

Comment 27

a year ago
(In reply to :Gijs from comment #25)
> Could this be the same hostutils stuff that plagued kmag's patches in bug
> 1431533? I didn't follow closely, but off-hand it sounds like it could be
> similar.

Oh, it sounds like the version of XPCShell used to run the server code on Android is different than the actual version of the tree? That's surprising. Thanks for pointing that out, as I would never have figured that out on my own. Maybe I'll split off the httpd.js and *.sjs changes into a separate bug, and hope there's nothing else in the 2000 files I'm changing that runs in the old version.
(In reply to Andrew McCreight [:mccr8] from comment #27)
> Maybe I'll split off the httpd.js and *.sjs changes into a separate
> bug, and hope there's nothing else in the 2000 files I'm changing that runs
> in the old version.

Unfortunately it's not that simple. I almost tried that myself, but the mochitest server pulls in a bunch of JSMs, starting with NetUtil.jsm, XPCOMUtils.jsm, Services.jsm, ...
Assignee

Comment 29

a year ago
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #28)
> Unfortunately it's not that simple. I almost tried that myself, but the
> mochitest server pulls in a bunch of JSMs, starting with NetUtil.jsm,
> XPCOMUtils.jsm, Services.jsm, ...

Mochitest itself was fine, for whatever reason. The problem was reftests and a few SJS files.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a677790fb3f41330678008cae7c7028ee10adfa5

Of course, test_bootstrap.js seems to be failing now. At least that's on Linux where I can investigate it more easily.
Assignee

Comment 30

a year ago
I couldn't repro the test_bootstrap.js failure locally, not even against the same revision, and when I rebased it, it seems to have gone away, so I'm going to hope it was a fluke or something that got fixed in the mean time...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=21b551ff59001fbca8d1a17c26bb23737a12e22c
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 6c752e9f4403aac6276c1e837b7d561e86e6a572 -d 828f63e2d52a: rebasing 446027:6c752e9f4403 "Bug 1432992, part 1 - Remove definitions of Ci, Cr, Cc, and Cu. r=florian"
merging browser/base/content/browser.js
merging browser/components/nsBrowserGlue.js
merging browser/components/places/PlacesUIUtils.jsm
merging browser/components/preferences/SiteDataManager.jsm
merging browser/components/preferences/in-content/preferences.js
merging browser/components/preferences/in-content/tests/browser_clearSiteData.js and browser/components/preferences/in-content/tests/browser_siteData.js to browser/components/preferences/in-content/tests/browser_clearSiteData.js
merging browser/components/preferences/in-content/tests/browser_siteData.js
merging browser/components/preferences/siteDataSettings.js
merging browser/extensions/webcompat-reporter/content/WebCompatReporter.jsm
merging toolkit/components/places/PlacesUtils.jsm
merging toolkit/components/places/SyncedBookmarksMirror.jsm
merging toolkit/components/url-classifier/SafeBrowsing.jsm
merging toolkit/content/aboutTelemetry.js
warning: conflicts while merging browser/components/preferences/in-content/preferences.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/preferences/in-content/tests/browser_clearSiteData.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/preferences/in-content/tests/browser_siteData.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Assignee

Comment 36

a year ago
I guess I'll probably have to land this myself on inbound.
Assignee

Updated

a year ago
Blocks: 1436184
Assignee

Updated

a year ago
Attachment #8947901 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 40

a year ago
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b38d59f71915
part 1 - Remove definitions of Ci, Cr, Cc, and Cu. r=florian
https://hg.mozilla.org/integration/autoland/rev/f06620ef048d
part 2 - Manually remove some empty blocks. r=florian
https://hg.mozilla.org/integration/autoland/rev/f033d62a90ad
part 3 - Adjust some line numbers in tests. r=florian

Comment 42

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b38d59f71915
https://hg.mozilla.org/mozilla-central/rev/f06620ef048d
https://hg.mozilla.org/mozilla-central/rev/f033d62a90ad
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Do we have eslint rules to prevent re-introducing them?
(In reply to Masatoshi Kimura [:emk] from comment #43)
> Do we have eslint rules to prevent re-introducing them?

Please see the blocking list. There's a couple of bugs to tidy up on the ESLint front.
Depends on: 1436310
You need to log in before you can comment on or make changes to this bug.