If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

deCOMtaminate parts of nsISupportsPrimitives

RESOLVED WONTFIX

Status

()

Core
XPCOM
RESOLVED WONTFIX
10 years ago
9 years ago

People

(Reporter: roc, Assigned: Ehsan)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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

10 years ago
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?

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<...>.
Created attachment 320391 [details] [diff] [review]
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.)
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
+  if (!object)
+  {
+    NS_IF_ADDREF(object);
+  }

This isn't right.

I think you should add NS_SUPPORTSID_IID to nsSupportsPrimitives.h.
(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.
Created attachment 321947 [details] [diff] [review]
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.
Attachment #320391 - Attachment is obsolete: true
Attachment #321947 - Flags: superreview?(shaver)
Attachment #321947 - Flags: review?(benjamin)
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

9 years ago
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() ?
(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.
(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.
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

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

Comment 13

9 years ago
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.
Attachment #321947 - Flags: review?(benjamin) → review-
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.
Any status updates on the final decision here?
I've got nothing more to add, it's up to bsmedberg to make a final decision.

Comment 17

9 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → WONTFIX
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 on attachment 321947 [details] [diff] [review]
Patch (v1)

OK, seems like nothing's going to happen here...
Attachment #321947 - Flags: superreview?(shaver)
You need to log in before you can comment on or make changes to this bug.