Closed Bug 1474739 Opened 3 years ago Closed 3 years ago

Kill the remaining consumers of XPT shims

Categories

(Core :: XPConnect, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: nika, Assigned: nika)

References

(Depends on 1 open bug)

Details

Attachments

(2 files)

When moving to the new XPT format, the mechanism for defining "shim" interfaces - interfaces which are not scriptable but expose an `Ci.XXX` object. was moved over to be a `shim` attribute in *.idl files. Thanks to :bz and other's hard work, many of these interfaces have been eliminated completely, taking down the [shim] attribute with them!

Currently there are a total of 4 consumers of [shim] left in tree, which seems like a small enough number that we should kill them & remove XPT shims entirely. They are:

1. nsIMessageSender https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/dom/base/nsIMessageManager.idl#10
2. nsIContentFrameMessageManager https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/dom/base/nsIMessageManager.idl#16
3. nsIDOMOfflineResourceList https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/dom/interfaces/offline/nsIDOMOfflineResourceList.idl#8
4. nsIListBoxObject https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/layout/xul/nsIListBoxObject.idl#12

For each of these messages I found the following consumers:

1. nsIMessageSender :
  a) service getter : https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/toolkit/modules/Services.jsm#64
    - Should be possible to replace with just "nsISupports" and get the same impact, as the type is WebIDL already.
  b) instanceof check : https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/toolkit/components/extensions/MessageManagerProxy.jsm#31
    - Switch to an instanceof check against the "MessageSender" webidl interface (although there may be problems with cross-compartment object references.
  c) to get parent-side messagemanager from ContentParent object : https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/toolkit/components/extensions/ExtensionParent.jsm#790
    - This appears to be the only use of the nsIInterfaceRequestor implementation on ContentParent. 
    - It's also a touch odd that the ContentParent object is exposed to JS at all - the nsIObserver nsISupports base was used in order to allow it to be the subject of the "ipc:content-created" observer notification.
    - Perhaps provide some other mechanism for getting this object beyond going through the getInterface codepath? We could add another nsIFoo interface which ContentParent implements, or even just add a ChromeUtils method which does the getting for us? I'm not sure what the best approach to take here is.

2. nsIContentFrameMessageManager :
  a) Many occurances in `docShell.getInterface()` situations. It appears that the only way to get this MessageManager right now from a docShell is to go through getInterface. 
  b) We should probably add an alternative getter method and switch to it.

3. nsIDOMOfflineResourceList : 
  a) test : https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/dom/tests/mochitest/ajax/offline/foreign2.html#49
    - Usage should be unnecessary
  b) test : https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/dom/tests/mochitest/ajax/offline/test_updatingManifest.html#159,213,262,306,309
    - Usage should be unnecessary

4. nsIListBoxObject : No consumers of this interface from JS found. Should be easy to remove.

NOTE: If the uses of the nsIDOMOfflineResourceList code could be modified to stop accessing constants on Ci.*, it would be possible to remove the bulk of the complexity of the [shim] attribute, which is forwarding XPIDL constant access to WebIDL. The GetInterface consumers could equally-well be handled by an empty `[scriptable, builtinclass, uuid(...)]` interface, which would have almost the same behaviour.
Priority: -- → P3
Depends on: 1475065
Depends on: 1475726
Depends on: 1475727
Depends on: 1475728
Comment on attachment 8996831 [details]
Bug 1474739 - Part 1: Stop using XPT shims in any xpidl interfaces, r=bzbarsky

Boris Zbarsky [:bz] (no decent commit message means r-) has approved the revision.

https://phabricator.services.mozilla.com/D2624
Attachment #8996831 - Flags: review+
Comment on attachment 8996832 [details]
Bug 1474739 - Part 2: Remove all code for XPT shims, r=bzbarsky

Boris Zbarsky [:bz] (no decent commit message means r-) has approved the revision.

https://phabricator.services.mozilla.com/D2625
Attachment #8996832 - Flags: review+
Pushed by nika@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a5c1ee48c91
Part 1: Stop using XPT shims in any xpidl interfaces, r=bzbarsky
https://hg.mozilla.org/integration/mozilla-inbound/rev/da7308cd7b0f
Part 2: Remove all code for XPT shims, r=bzbarsky
Pushed by nika@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62a0e59c7e1a
Part 3: Linting fixes on a CLOSED TREE, a=bustage
Assignee: nobody → nika
You need to log in before you can comment on or make changes to this bug.