1437605 - tidy up QI implementations in xpconnect

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement, P2)

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - use NS_IMPL_QUERY_INTERFACE in js/xpconnect/

This construct is nicer than NS_INTERFACE_MAP_BEGIN and assures the
reader there's no weirdness in the QI implementation.  This change does
mean that PGO doesn't get an opportunity to measure the frequency of
which interfaces are QI'd most often.  I think this is probably an OK
tradeoff to make, given the prevalence of NS_IMPL_QUERY_INTERFACE
elsewhere in the codebase.

The one thing we have to ensure with this change is that the ambiguous
QI to nsISupports uses the proper class after the change.  The
NS_IMPL_QUERY_INTERFACE macro chooses the first interface listed to
disambiguate the cast to nsISupports.  In many cases, the ordering of
the interfaces was already correct, but a few cases required reordering
the interfaces.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - use NS_IMPL_QUERY_INTERFACE_CI in js/xpconnect/

Just like part 1, only for classes that also require classinfo.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 3 - use NS_IMPL_ISUPPORTS in js/xpconnect/

Now that we've used the standard NS_IMPL_QUERY_INTERFACE macro, we can
start using the even more standard NS_IMPL_ISUPPORTS macro.  There's a
standard NS_IMPL_ISUPPORTS_CI macro that we could use for the bits in
XPCJSID.cpp, but said macro assumes that all the interfaces listed are
exported to classinfo.  nsJSIID and nsJSCID expose nsIXPCScritable
through QI, but not through classinfo, so I've chosen to leave those
alone.

Comment 4

[Andrew McCreight [:mccr8]]

Comment on attachment 8950289 [details] [diff] [review]
part 1 - use NS_IMPL_QUERY_INTERFACE in js/xpconnect/

Review of attachment 8950289 [details] [diff] [review]:
-----------------------------------------------------------------

FWIW, table driven QIs have shown up in profiles before, though mostly due to CC. Hopefully these aren't QId enough for it to matter. Ci, etc. are used frequently, but we only define them once per compartment, so hopefully there's not too much in the way of QIing.

Comment 5

[Andrew McCreight [:mccr8]]

Comment on attachment 8950291 [details] [diff] [review]
part 3 - use NS_IMPL_ISUPPORTS in js/xpconnect/

Review of attachment 8950291 [details] [diff] [review]:
-----------------------------------------------------------------

nit: "nsIXPCScritable" in patch description is missing a p.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/39aadaccd235
part 1 - use NS_IMPL_QUERY_INTERFACE in js/xpconnect/; r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/281b91e31224
part 2 - use NS_IMPL_QUERY_INTERFACE_CI in js/xpconnect/; r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e563407a9e9
part 3 - use NS_IMPL_ISUPPORTS in js/xpconnect/; r=mccr8

Comment 7

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/39aadaccd235
https://hg.mozilla.org/mozilla-central/rev/281b91e31224
https://hg.mozilla.org/mozilla-central/rev/7e563407a9e9