(class nsIXPConnectWrappedJS; should be removed from elm.h) I could just call ->GetJSObject() since that does unmarking, but I want to be explicit in these cases.

Comment on attachment 590896 [details] [diff] [review] patch Review of attachment 590896 [details] [diff] [review]: ----------------------------------------------------------------- A concern here is that it seems like overkill to drag in all of XPConnect, and xpcprivate, for what seems like a single thing you need, namely marking a held object black. I think you don't even need to do that. The idea is just to mark whatever object is held by the wrapped JS black, right? I think this is sufficient to do this: JSObject *o; wjs->GetJSObject(&o); You don't even need to explicitly unmarkGray the JSObject because for CC safety that is done any time you hand out an object. Those two lines would replace all of this: nsXPCWrappedJS* w = static_cast<nsXPCWrappedJS*>(wjs.get()); if (w) { JSObject* o = w->GetJSObjectPreserveColor(); if (o) { xpc_UnmarkGrayObject(o); } } Then I think you can use xpcpublic.h instead of xpcprivate.h. I don't know how that affects what you need to put in the Makefile, if at all. It looks to me like that can be done in both places. Does that make sense to you? I'll try to understand the actual listener stuff now. :)

You may need to do an xpc_UnmarkGrayObject after all, because XPCJSObjectHolder::GetJSObject doesn't ungray, but in that case your old code would have just crashed I think, so maybe it isn't relevant.

(In reply to Andrew McCreight [:mccr8] from comment #1) > A concern here is that it seems like overkill to drag in all of XPConnect, > and xpcprivate I'm going to drag xpcprivate to everywhere :) Well, in fact it is already used in many place where I need it. (In reply to Andrew McCreight [:mccr8] from comment #2) > You may need to do an xpc_UnmarkGrayObject after all, because > XPCJSObjectHolder::GetJSObject doesn't ungray, but in that case your old > code would have just crashed I think, so maybe it isn't relevant. What would have crashed and where? And this is about nsXPCWrappedJS, not XPCJSObjectHolder. nsXPCWrappedJS::GetJSObject does clear the gray bit. I just don't think calling GetJSObject should be the way to unmark gray object, when unmarking is the only thing to do to an object. It is a side effect of GetJSObject that is unmarks. (and I can mumble about virtual call)

Also, I need the xpc_UnmarkGrayObject for nsJSEventListener stuff anyway

Comment on attachment 590896 [details] [diff] [review] patch Asking re-review. Btw, nsDOMEventTargetHelper stuff will go away reasonable soon.

(In reply to Olli Pettay [:smaug] from comment #3) > (In reply to Andrew McCreight [:mccr8] from comment #1) > > A concern here is that it seems like overkill to drag in all of XPConnect, > > and xpcprivate > I'm going to drag xpcprivate to everywhere :) Okay, but what exactly do you need from there? > What would have crashed and where? Never mind, you are QIing to the interface version of XPCWrappedJS. > And this is about nsXPCWrappedJS, not XPCJSObjectHolder. > nsXPCWrappedJS::GetJSObject does clear the gray bit. > I just don't think calling GetJSObject should be the way to unmark gray > object, > when unmarking is the only thing to do to an object. > It is a side effect of GetJSObject that is unmarks. > (and I can mumble about virtual call) Well, you can use GetJSObject, then call xpc_UnmarkGrayObject on it, if you prefer, or add an assertion that the object isn't gray after you get it. The actual overhead of the getting part is not very high. Or if you are really concerned about the virtual call overhead, you could add a method like UnmarkGrayJSObject to nsIXPConnectWrappedJS and then call that. (In reply to Olli Pettay [:smaug] from comment #4) > Also, I need the xpc_UnmarkGrayObject for nsJSEventListener stuff anyway That's in xpcpublic.h, not xpcprivate.h.

Also, xpc_UnmarkGrayObject does a NULL check, so you don't need to guard calls to it with your own NULL checks.

(In reply to Andrew McCreight [:mccr8] from comment #7) > Or if you are > really concerned about the virtual call overhead, you could add a method > like UnmarkGrayJSObject to nsIXPConnectWrappedJS and then call that. Though I guess all this IDL stuff gets turned into virtual calls so never mind, that probably won't help. > That's in xpcpublic.h, not xpcprivate.h. To follow up, when I made the change I suggested above to this patch, it compiled with xpcpublic. I didn't test it otherwise so maybe it doesn't work. ;)

Oh, ok, it is in public after all. I is the nsXPCWrappedJS which is in private. If you really want me to use only xpcpublic, I can, but I don't quite see the reason. Null check can be removed though.

(In reply to Olli Pettay [:smaug] from comment #10) > I can, but I don't quite see > the reason. > ...since if we can call non-virtual method easily, we should, IMHO.

It just seems like a lot of stuff to drag in just for this one thing. I guess you could add a function that wraps that call to put in xpcpublic, so you would have only direct calls. But you'd have an additional indirection. Maybe I'm being too picky. I guess I'm just not comfortable with making the judgement about whether it is appropriate to include xpcprivate myself. If an XPConnect peer says it is okay to include xpcprivate in these files (and whatever Makefile stuff is needed for that, as seen in this patch) then I'm okay with it. Here are the files where it is added (from this patch and some others up for review now): content/base/src/nsEventSource.cpp content/base/src/nsWebSocket.cpp content/base/src/nsXMLHttpRequest.cpp content/events/src/nsEventListenerManager.cpp content/base/src/nsFrameMessageManager.cpp jst, do you have any thoughts on this?

Err, CC didn't go through, so I'll have to add jst again.

Boo, XPConnect peers don't want xpcprivate to be used outside XPConnect. I guess this takes couple of hours to duplicate some code to xpcpublic

Comment on attachment 591108 [details] [diff] [review] using the xpc stuff Review of attachment 591108 [details] [diff] [review]: ----------------------------------------------------------------- Just so I understand what is going on here, the point of mWrappedJS is to avoid having to do that QI every time you UnmarkGrayJSListeners unless you need to? A listener is never somehow replaced so that you might have to update the mWrappedJS flag later? I looked over the ELM file and didn't see anything like that. Looks good.

(In reply to Andrew McCreight [:mccr8] from comment #17) > Just so I understand what is going on here, the point of mWrappedJS is to > avoid having to do that QI every time you UnmarkGrayJSListeners unless you > need to? Yup. QIing is virtual-heavy stuff. Better to avoid it, especially since we have extra bits in the listener struct. > A listener is never somehow replaced so that you might have to update the > mWrappedJS flag later? No. Event listeners can be added or removed, not updated.