399237 - deCOMtaminate parts of nsISupportsPrimitives

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

In quite a few places in Gecko we create and use nsISupportsPrimitive subclasses, namely nsSupports...Impl. These need to keep an XPCOM interface because they are often used from script, but there are two improvements we could make:

1) Instead of creating them using do_CreateInstance, we could just "new nsSupports...Impl()" directly, and add a constructor that takes a parameter to initialize the value, so we don't have to make a separate virtual method call to set the value.

2) We could add IIDs to the nsSupports...Impl classes to make it possible to QueryInterface directly to an nsSupports...Impl class. Then we could add inline, non-COM getters to those classes. For example we could add "PRUint32 Get() { return mData; }" to nsSupportsPRUint32Impl and "const nsString& Get() { return mString; }" to nsSupportsStringImpl. We could then use these QIs and direct getters from Gecko to reduce code size. We should add some documentation to nsISupportsPrimitive.idl to make it clear that no-one should be implementing their own nsISupportsPrimitive implementations.

Comment 1

[Ian Bevan]

I'm taking a look at this as my first Mozilla bug to fix. With that in mind, please don't flame me if I'm way off base!

I'm still getting my head around the source code and how all this works. Looking at the class factory, I note that it is currently responsible for calling AddRef on a newly created instance. If we were to move to calling the ctor of nsSupports...Impl() directly, presumably you would want this added to the new ctor rather than placing it in the client (calling) code. I don't know to what extent other xpCOM classes have an AddRef in public constructors. Is this something you really want to do?

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No, we wouldn't want to AddRef in a constructor. That's a good point.

I think the best approach would be to have a static method in the Impl class, say Create(), that creates the object, addrefs it, and returns an already_AddRefed<...>.

Comment 3

[(no longer active)]

Attachment: Patch (v0)

The implementation of this is similar for all of the XPCOM primitives, so I thought I'd post my approach here to check if I'm not missing something, and then move on to the rest of the primitives, and change the callers accordingly.

Here is an implementation of the suggestions in comment 0 for nsSupportsIDImpl.  I have added a static Create method which returns an addrefed interface pointer, avoiding the SetData virtual method call.  I have also added an IID to nsSupportsIDImpl and made it support QI, plus the inline Get method.  I was not sure where the appropriate place is for defining the nsSupports*Impl IIDs.  At first I thought nsISupportsPrimitives.idl is the best place (because it's where the rest of the IIDs are defined) but now I'm thinking maybe it's OK to add them to nsSupportsPrimitives.h (since callers need to include that header file anyway if they're going to use the Get method.)

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  if (!object)
+  {
+    NS_IF_ADDREF(object);
+  }

This isn't right.

I think you should add NS_SUPPORTSID_IID to nsSupportsPrimitives.h.

Comment 5

[(no longer active)]

(In reply to comment #4)
> +  if (!object)
> +  {
> +    NS_IF_ADDREF(object);
> +  }
> 
> This isn't right.

Yes, sorry for the confusion on my part.

> I think you should add NS_SUPPORTSID_IID to nsSupportsPrimitives.h.

OK, will do.  I'll post a more complete patch when ready.

Comment 6

[(no longer active)]

Attachment: Patch (v1)

nsSupportsPrimitives changes, submitted for review.  I'll prepare a separate patch to change the callers to use the new API and will post here when ready.

Comment 7

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Do we have tests for nsISupportsPrimitives?

Should we enforce the "don't implement this" with a C++-only block that makes a private constructor and names some friends, or some other appropriate trick?

Comment 8

[Benjamin Smedberg]

I don't understand the original premise: if you get a nsISupportsSomePrimitive (from script?), how is it better to do:

nsCOMPtr<nsConcretePrimitive> concrete = do_QueryInterface(prim);
concrete.GetValueNonVirtually();

than to do concrete->GetValueVirtuallyWithoutQI() ?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #7)
> Do we have tests for nsISupportsPrimitives?

There are some tests for using them with XPConnect: http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/tests/js/xpctest_primitives.js

> Should we enforce the "don't implement this" with a C++-only block that makes
> a private constructor and names some friends, or some other appropriate trick?

That's a good idea, but would it actually block JS implementations?

(In reply to comment #8)
> I don't understand the original premise: if you get a nsISupportsSomePrimitive
> (from script?), how is it better to do:
> 
> nsCOMPtr<nsConcretePrimitive> concrete = do_QueryInterface(prim);
> concrete.GetValueNonVirtually();
> 
> than to do concrete->GetValueVirtuallyWithoutQI() ?

Because the getter has cleaner syntax. This is especially important for strings because it lets you avoid a copy. Also, in many cases you start with an nsISupports and have to QI that to something already, or do some similar dance. If you look at http://mxr.mozilla.org/mozilla-central/search?string=nsisupportspruint32 for example, you can see that pretty much all the nsISupportsPRUint32 values are already obtained via QI (or CreateInstance). Similarly for http://mxr.mozilla.org/mozilla-central/search?string=nsisupportsstring. Which makes sense, since the main reason to use these types in C++ code is to box values so you can use them in generic nsISupports slots.

Comment 10

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

(In reply to comment #9)
> > Should we enforce the "don't implement this" with a C++-only block that makes
> > a private constructor and names some friends, or some other appropriate trick?
> 
> That's a good idea, but would it actually block JS implementations?

No, it would only block the C++ ones.  We'd need a [noimpl] xpidl attribute and use of a scarce flag in the typelib to enforce at the xpconnect level, or we could just blacklist the IID in the xpconnect wrapping code.  I don't think it's as likely that people will try to implement it in JS, but I don't have a lot of deep thinking to support that position.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't think it's likely that anyone will actually reimplement these --- they're so simple, there's no reason to.

If we added a dummy [noscript] method, would that prevent JS implementation?

Comment 12

[Benjamin Smedberg]

no, it would only prevent JS implementation of that method (it would return NS_XPC_NOT_IMPLEMENTED or somesuch).

Comment 13

[Benjamin Smedberg]

Comment on attachment 321947 [details] [diff] [review]
Patch (v1)

I'm not convinced deCOM is worth it here... do we have profiles of how this might cause improvements, or dependent bugs that want to use this? If so, please re-request review with performance numbers. Otherwise, I'd like to WONTFIX this.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It's primarily for simpler client code, not for performance on particular testcases.

I think it's a good idea to require profile data for optimizations that *increase* complexity, but it doesn't make sense for changes that reduce complexity.

Comment 15

[(no longer active)]

Any status updates on the final decision here?

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I've got nothing more to add, it's up to bsmedberg to make a final decision.

Comment 17

[Benjamin Smedberg]

I don't think we want this. If we want to reduce complexity, it sounds better to me to actually implement an IDL discriminated variant union.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That would require changing interfaces in incompatible ways.

I still don't understand why this patch was rejected. Would it help if we changed all C++ users of nsISupportsPrimitives in this patch so that you can see how the code is simpler?

Comment 19

[(no longer active)]

Comment on attachment 321947 [details] [diff] [review]
Patch (v1)

OK, seems like nothing's going to happen here...