Closed Bug 349002 Opened 14 years ago Closed 13 years ago

Refactor xptcall into a frozen API


(Core :: XPCOM, defect, P2)






(Reporter: benjamin, Assigned: benjamin)




(7 files, 2 obsolete files)

Currently xptcall is exported as a bunch of nonfrozen C++ symbols. This bug will track the work needed to port xptcall over to use frozen C exports. I originally began this work on BSMEDBERG_20060330_XPTCALL_BRANCH.
Assignee: nobody → benjamin
Priority: -- → P2
Target Milestone: --- → mozilla1.9alpha
Attached patch NS_GetXPTCallStub love, rev. 1 (obsolete) — Splinter Review
Darin, I did a mostly-global s/XPTC_InvokeByIndex/NS_InvokeByIndex/, which caused a little bit of whitespace to be unaligned. I can fix that before checkin if you prefer.

I have fixed up the platform-specific xptcall bits for linux x86 and x86-64, windows x86, mac-ppc, and mac-x86. After reviews, I will issue a call for additional xptcall implementations before checkin.
Attachment #235073 - Flags: review?(darin)
bsmedberg, it looks like you did more than just rename XPTC_InvokeByIndex.  Can you please list / summarize the complete set of changes?
Yeah, the renaming was the least important part.


The XPTCall API should be available entirely through frozen structures and a C API. To this end, I have replaced the nsXPTCStubBase class (which is linked via C++) with a frozen accessor NS_GetXPTCallStub() (xptcall.h has the docs).

The remaining changes should be simply changing client code to use the new API. This replaces classes that inherited from nsXPTCStubBase with a mXPTCStub member and NS_Get/DestroyXPTCallStub pair in the constructor/initializer and destructor.
Comment on attachment 235073 [details] [diff] [review]
NS_GetXPTCallStub love, rev. 1

+  if (!result || result->mXPTCStub)

that's missing a ! in the second clause of the if, right?

have you considered writing a helper class that avoids having to call NS_DestroyXPTCallStub in each destructor? (and in the constructors?)

+  nsRefPtr<nsXTFWeakTearoff> result
+    = new nsXTFWeakTearoff(iid, obj);

isn't it more usual to put the = on the first line?

-  nsresult invokeResult = XPTC_InvokeByIndex(realObject, methodIndex,
+  nsresult invokeResult = NS_InvokeByIndex(realObject, methodIndex,
                                              paramCount, params);

the second line now has wrong indentation :)

+nsJavaXPTCStub::nsJavaXPTCStub(jobject aJavaObject, nsIInterfaceInfo *aIInfo,
+                               nsresult &rv)

might be nicer to pass rv as a pointer so that it's more obvious that it's an out parameter

+        nsISupports* sthis = this;
+        NS_ADDREF(sthis);
+        *aInstancePtr = NS_REINTERPRET_CAST(void*, sthis);

Comment on attachment 235073 [details] [diff] [review]
NS_GetXPTCallStub love, rev. 1

I'm sorry, I won't have time to give this a thorough review.  I suggest finding another reviewer.
Attachment #235073 - Flags: review?(darin)
Attachment #235073 - Attachment is obsolete: true
Attachment #242194 - Flags: review?(timeless)
Comment on attachment 242194 [details] [diff] [review]
NS_GetXPTCallStub love, rev. 2

please make this pattern not crash on oom:
new nsXPTCVariant[paramCount]
     fullPars[i].Init(params[i], paramInfo.GetType(), flags);

+    *aInstancePtr = stub->mXPTCStub;;

please watch for doubled ;'s


please assert that this doesn't fail.

+ * A helper class which initializes an xptcall helper at construction

'that' i think.

this link is bad, how about bonsai pinning it?
 The <b><i>invoke</i></b> functionality requires the implementation of the
 following on each platform (from <a href="">xptcall/public/xptcall.h</a>):
+NS_InvokeByIndex(nsISupports* that, PRUint32 methodIndex,

same goes for the next place in this file

+ * @note No objects are addrefed on return of this function.

/awk/. rewrite :)

+ * Destroys an XPTCall stub previous created with NS_GetXPTCallStub.


+NS_GetXPTCallStub(REFNSIID aIID, nsIXPTCProxy* aOuter,

someone should either ASSERT or ENSURE aOuter.

bsmedberg of course promises to try to fix the indentation even in the cases where they weren't his fault :)
Attachment #242194 - Flags: review?(timeless) → review+
This is the patch for checkin. I'm going to post to the newsgroups asking for volunteers to fixup the xptcall ports, and try to commit next Monday.
Benjamin, you reference nsXPTCUtils.h in your patch several times, but it doesn't seem to be included. Does that come in from work in another bug/patch or is it missing from this one (or did I mess up my tree again)?
These are the changes to the relevant files for OS/2. It builds xpcom\reflect\xptcall\src\md\os2\ fine with this patch and the one from attachment 244923 [details] [diff] [review], but without the missing header file I cannot build a final product to test if it still works. (Btw, thanks for the heads-up in the newsgroup that pointed to this bug!)
Checked this in, had issues with xpcshell not linking on linux. I can't reproduce. I'm a little frustrated.
Fixed on trunk. Turns out the Linux x86 error was a remaining reference to XPTC_InvokeByIndex in the arch-specific assembly file.

Port owners, please file new bugs to get port-specific fixes in as a result of this change. I am happy to review.
Closed: 13 years ago
Resolution: --- → FIXED
Blocks: 361470
There's a patch attached to bug 361415 which should update PPC32 for this too, as well as implementing it all for PPC64.
Blocks: 361533
maybe bug 361533 would be reasonable reason to reopen this bug ?
Attached patch xptcall ARM fix (obsolete) — Splinter Review
Attachment #246459 - Flags: review?(timeless)
romaxa, submit your patch to bug 361533 also ;)
maybe arm patch misses something ... it builds, but crashes on launching
Attachment #246459 - Attachment is obsolete: true
Attachment #247274 - Flags: review?(timeless)
Attachment #246459 - Flags: review?(timeless)
Attachment #247274 - Flags: review?(benjamin)
Attachment #247274 - Flags: review?(benjamin) → review+
This patch fixes compilation for Alpha Linux. Note that I have no clue as to what I'm doing, I just copied the changes from the Arm patch and tested them.
Blocks: 368482
Patch for Linux PPC.  I'm a Mozilla internals neophyte and have done this patch with the guidance of David Bolter, Christian Biesinger, Aaron Leventhal, and Ginn Chen (thanks guys!).  I have no clue if what I've done here is correct, but I'm submitting this patch using Firefox built from CVS HEAD on my Mac PowerBook G4.
Attachment #256782 - Flags: superreview?(benjamin)
Attachment #256782 - Flags: review?(benjamin)
Attachment #256782 - Flags: superreview?(benjamin)
Attachment #256782 - Flags: superreview+
Attachment #256782 - Flags: review?(benjamin)
Attachment #256782 - Flags: review+
Attachment #256782 - Attachment description: Patch for Linux PPC → Patch for Linux PPC [checked in]
Is there a particular reason for a bunch of architectures not to have been updated ?
I did not update architectures which didn't have tinderboxes and which I didn't have build environments for. I did post to the newsgroups notifying the port owners that a change was needed.
Blocks: 443234
Hi Benjamin,

I've been trying to restore WSDL support for Jaxer server (based on Firefox 3 sources) and came with the following patch. Existing WSDL tests are working fine. So I would like to contribute it to Mozilla and get some feedback/review.

All changes are around an additional call NS_GetXPTCallStubII() which creates nsXPTCStubBase from an nsIInterfaceInfo*. 

Thanks for your attention,
Max, kindly file a new bug indicating what the bug is you're trying to fix and attach your patch to the new bug with the appropriate review flags.
Attachment #247274 - Flags: review?(timeless)
You need to log in before you can comment on or make changes to this bug.