Remove classes, interfaces, results, and utils from Components for a chrome scope

NEW
Unassigned

Status

()

enhancement
P3
normal
a year ago
3 months ago

People

(Reporter: emk, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

a year ago
Because we no longer use them. We even have a eslint rule to force it. There is no point in keeping dead properties on Components.
This should wait until the Cu/... globals are on all channels for the sake of system add-ons. And it will break comm-central, which didn't get the auto-rewrite.
(Reporter)

Comment 2

a year ago
Looks like c-c is working on bug 1436605.
Depends on: 1436605
I offered patches in bug 1436605 for comm-central, so this should become a non-issue soon. But there's still some code that needs rewriting, eg. JS code embedded in python scripts: https://searchfox.org/mozilla-central/search?q=Components.classes&case=false&regexp=false&path=py

Also, one thing that really needs fixing before we can remove the properties from 'Components' is that in a handful of places, we had to revert to using Components.classes because 'Cc' wasn't defined. This seems to be mostly in content scopes in plain mochitests.
(In reply to Florian Quèze [:florian] from comment #3)
> Also, one thing that really needs fixing before we can remove the properties
> from 'Components' is that in a handful of places, we had to revert to using
> Components.classes because 'Cc' wasn't defined. This seems to be mostly in
> content scopes in plain mochitests.

Are they using UniversalXPConnect? If so, they should probably be updated to use SpecialPowers. Though I though mccr8's patch defined those properties for UniversalXPConnect scopes too...
I'm not completely sure. If you want to investigate, the files I had to exclude from my rewrite scripts are:
  "toolkit/modules/Promise-backend.js",
  "testing/xpcshell/dbg-actors.js",
  "toolkit/components/ctypes/tests/unit/test_jsctypes.js",
  "js/xpconnect/tests/mochitest/file_bug795275.html",
  "js/xpconnect/tests/mochitest/test_bug790732.html",
  "mobile/android/tests/browser/robocop/robocop_head.js",
  "testing/mochitest/tests/SimpleTest/SimpleTest.js",
  "dom/events/test/test_bug238987.html",
  "dom/indexedDB/test/file.js",
Some of the XPConnect tests are actually checking to make sure that Components isn't defined, so that should be fine.
(In reply to Florian Quèze [:florian] from comment #5)
> I'm not completely sure. If you want to investigate, the files I had to
> exclude from my rewrite scripts are:
>   "toolkit/modules/Promise-backend.js",
>   "testing/xpcshell/dbg-actors.js",

These are loaded devtools scopes, which don't have any Components/Cc/Ci/... globals by default.

We should probably get rid of Promise-backend.js at this point, and have devtools either switch to JS Promises or move the code directly to their promise module.

>   "toolkit/components/ctypes/tests/unit/test_jsctypes.js",

This seems to work for me.

>   "js/xpconnect/tests/mochitest/file_bug795275.html",
>   "js/xpconnect/tests/mochitest/test_bug790732.html",

As Andrew said, these are probably fine, but they suggest we won't be able to remove Components.interfaces for content scopes immediately.

>   "mobile/android/tests/browser/robocop/robocop_head.js",

OK, this is pretty gross. So, this script runs in either a chrome-privileged sandbox or in a content-privileged HTML page.

In the sandbox configuration, the Cc/Ci/... globals should work, although for some reason the harness also copies the SpecialPowers.Components object to the sandbox global.

In the HTML version, the file just creates a SpecialPowers-wrapped Components global. That can be changed to just destructure the SpecialPowers.Cc/... properties, I guess.

>   "testing/mochitest/tests/SimpleTest/SimpleTest.js",

This is also kind of gross.

I suppose one issue is here:

    var Cu = Components.utils || SpecialPowers.Cu;
    var Ci = Components.interfaces || SpecialPowers.Ci;

where we'll wind up always using the SpecialPowers Cu and Ci objects, because the var declarations mask the globals. And where we'd throw if we didn't mask those globals, but tried to access them when they didn't exist.

There are some other places where we use Components.interfaces, which exists in non-chrome scopes (as opposed to Ci which doesn't), but they also touch Components.classes in the same place, so probably not an issue.

>   "dom/events/test/test_bug238987.html",

This uses Components.interfaces in a content scope where it exists but Ci doesn't. It needs to be updated to use Event.CAPTURING_PHASE rather than nsIDOMEvent.

But that also suggests that removing Components.interfaces from content scopes may break code in the wild. :(

>   "dom/indexedDB/test/file.js",

This also uses Components.interfaces in a content scope. It can be updated to use SpecialPowers.Ci.
(Reporter)

Comment 8

a year ago
I will not remove Components.interfaces from the Components shim (at least in this bug) because it is exposed to the web.
Summary: Remove classes, interfaces, results, and utils from Components → Remove classes, interfaces, results, and utils from Components for a chrome scope

Comment 9

a year ago
Setting priority to remove from triage.
Priority: -- → P3
(Reporter)

Comment 10

a year ago
Bug 1448048 fixed Components.interfaces usages in content scope :)
(Reporter)

Updated

8 months ago
Depends on: 1458367
(Reporter)

Comment 11

3 months ago

Now we have Cc,Ci,Cu,... everywhere and c-c finished rewriting. Can we start working on this?

You need to log in before you can comment on or make changes to this bug.