Closed
Bug 107289
Opened 23 years ago
Closed 23 years ago
xpcom has dependency upon libjar
Categories
(Core :: XPCOM, defect, P2)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla1.0
People
(Reporter: netscape, Assigned: alecf)
References
Details
(Keywords: embed, topembed-)
Attachments
(1 file, 3 obsolete files)
29.05 KB,
patch
|
dougt
:
review+
jband_mozilla
:
superreview+
asa
:
approval+
|
Details | Diff | Splinter Review |
We have a circular module dependency between libjar & xpcom. nsIZipReader is needed by xptinfo. I made a cursory attempt at replacing the #include with forward class declarations but that doesn't work when using a nsCOMPtr<nsIZipReaderCache>.
Assignee | ||
Comment 1•23 years ago
|
||
I honestly think this dependency is not as important as the libjar <-> caps codependency (bug 102943)
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla0.9.8
Assignee | ||
Updated•23 years ago
|
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Assignee | ||
Comment 2•23 years ago
|
||
ok, my proposal for this (and I'm looking for comments) is to take the file extension of the file, append it onto a contract id, and then try to instantiate an object which can decode that file extension and provide a typelib or set of typelibs out of it. If successful, we'll call some interface on that object which gives us back a typelib. jband, how does this sound? I haven't decided where I'd move the code which calls libjar just yet, but this would get it out of xpcom.
Comment 3•23 years ago
|
||
Don't append it onto a contract id. Use the category manager instead.
Comment 4•23 years ago
|
||
huh? The typelib loader (xpti) needs to do its work very early in xpcom startup. It does not want something else to take a file and return a typelib. It just wants to get files out of jars. xpcom does not link to libjar. xpti simply uses a couple of interface declarations exported by the libjar module. So, if you just can't allow that, then why not abstract out what it uses into some interfaces that are declared in xpcom and have the libjar interfaces inherit from (or objects just implement) those interfaces. I don't see any reason for anything but superficial code changes here. Am I missing something?
Assignee | ||
Comment 5•23 years ago
|
||
ok, that's fine as well... I figured while I was moving the jar-handling out of xpcom, I might as well abstract it slightly to allow for other storage mechanisms.. but I'm not picky..:)
Assignee | ||
Updated•23 years ago
|
Target Milestone: mozilla0.9.9 → mozilla1.1
Updated•23 years ago
|
Keywords: mozilla1.0+
Updated•23 years ago
|
Assignee | ||
Comment 6•23 years ago
|
||
turns out this wasn't too hard, I just did the work. Moving back to 1.0 and looking for reviews.
Target Milestone: mozilla1.1alpha → mozilla1.0
Assignee | ||
Comment 7•23 years ago
|
||
Here's the patch, hoping to get some reviews from jband.. Here's what I've done: 1) created a new interface, nsIXPTLoader, which basically says that the implementation can load XPT files. There are two methods to load XPTs: - by name, with loadEntry() - by enumeration, with enumerateEntries, which takes a nsIXPTLoaderSink The interface is generic, and doesn't necessary have to be Zip. All access to the .xpt file data are done through nsIInputStream. The contractID for the implementation is parameterized to indicate the type of file. In this patch, the only type we use is "zip" 2) adapted the old xptiZipLoader to use this new mechanism, which involved - abstracting out the ability to read an XPT from an nsIInputStream (most of the implementation came from the xptiZipReader class, which was using nsIInputStream internally anyway) - creating a seperate xptiZipLoaderSink class, to be a container for the "workingSet" closure.. xptiInterfaceInfoManager is no longer a sink for the loader, and this helper class allows us to keep special knowledge of xptiWorkingSet objects out of nsIXPTLoader. This sink implementation does need to call back into xptiInterfaceInfoManager in order to call FoundZipEntry() - using do_GetService() to get to the XPTLoader for zip files, and safely handling the lack of the service. 2) implementing a little stub XPT loader in libjar, which basically just passes off the inputstream from libjar to the caller. this patch includes everything except the mac build changes because I'm posting this from my PC, but its basically just adding the nsIXPTLoader.idl to the xpcom IDL project, and adding nsXPTZipLoader.cpp to the libjar project. also, I have no idea if I got the license right on the 3 new files...if there any errors, let me know.
Assignee | ||
Comment 8•23 years ago
|
||
still waiting on reviews here....
Comment 9•23 years ago
|
||
Alec: I'm reading the code. I'll have review in a bit. A couple of questions... Have you tested this with zipped xpt files yet? I'm not very happy about moving this xpti specific code into libjar. Is this *really* worth avoiding #include level dependencies?
Comment 10•23 years ago
|
||
a couple general comments: In nsIXPTLoader.idl, how about reordering the two interfaces so that you can avoid "forward declare". Do you really need the #define NS_XPTLOADER_CONTRACTID_PREFIX in the IDL? In nsXPTZipLoader::EnumerateEntries I would have done a while loop: PRBool hasMore; while (NS_SUCCEEDED(entries->HasMoreElements(&hasMore)) && hasMore) {} I think that the above is a more common pattern. More in a bit.
Assignee | ||
Comment 11•23 years ago
|
||
thanks for the comments..yep, I've tested it with some .zip files, it works great! regarding xpt-specificness, you'll see its not very XPT-specific.. I mean the interface is nsIXPTLoader, but you'll see that mostly all it deals with are nsIInputStream/etc. I wasn't sure where else to put it besides libjar. its only a tiny piece of code and yes, it is important for modular builds, which is important for embedding. I've been fighting this battle for months, I'd rather not get into a debate about it here.. :) regarding the loop, I'll fix it, but I copied and pasted it from the loop that CVS Blame says you wrote :) frankly, I prefer the pattern you just wrote above too anyway, I was just being lazy
Assignee | ||
Comment 12•23 years ago
|
||
oh, and yeah I prefer the contractID prefix in the IDL because then there's just one header to include by the implementor, and the ContractID itself isn't specified there, just the prefix
Assignee | ||
Comment 13•23 years ago
|
||
here's an updated patch, with extra javadoc comments for good measure.
Attachment #76462 -
Attachment is obsolete: true
Comment 14•23 years ago
|
||
Hey! That was dougt pulling your chain not me. You ended up with a 'while' on both ends of the block... + while (NS_SUCCEEDED(entries->HasMoreElements(&hasMore)) && hasMore) { ... + } while (1); That can't possibly work right.
Comment 15•23 years ago
|
||
And for that block I was originally making the distinction between one case where we fail and another where we just detect that we are done. I don't know if that matters much. But this is a change. It could have been a 'while(1){}' rather than a "do{}while(1)'. But equating an enumerator failure with the done flag *is* as logic change here.
Assignee | ||
Comment 16•23 years ago
|
||
doh! wow that actually compiled on windows though. who'da thunk. change that last "} while (1);" to just "}" - I have in my patch.
Assignee | ||
Comment 17•23 years ago
|
||
true enough. I don't care either way. Which would you prefer?
Comment 18•23 years ago
|
||
I don't care if the 'do' is there or not. But, when it comes down to it, I'd rather see it fail if the iterator pukes than have it silently act like nothing was wrong. So, I'd say put it back like it was and mode the 'while(1)' (or use for(;;), whatever) if that makes anyone happier.
Comment 19•23 years ago
|
||
+ } else { + NS_WARNING("Could not load XPT Zip loader"); + } You need to set 'header = nsnull;' in both instances of this block. + rv = loader->EnumerateEntries(file, NS_STATIC_CAST(nsIXPTLoaderSink*, sink)); Why the cast? sink is already of nsCOMPtr of that type. no? + printf("xptiZipLoaderSink::FoundEntry(%s, %d, ..)\n", entryName, index); + XPTHeader *header = + xptiZipLoader::ReadXPTFileFromInputStream(aStream, mWorkingSet); + printf("\tGot header %p\n", header); huh? I'm sure you didn't mean to leave the printfs in. Otherwise I'm OK with this. The xpti specific stuff in libjar that bothered me is stuff that we might want to tune later like the gCacheSize and the decisions the loader makes about where in the zip file it expects to find .xpt files (only in the root of the zip) and the fact that it is actually looking for '*.xpt' files at all. Still, I don't think it is worth adding more layers just to give xpti more direct control. We can live with this. I probably would have not made this a service - and instead had xpti create and explicitly init the one instance it would make. But, I'm fine with the choice you made and I'm not suggesting that you redo it. One more patch to address the little issues I mentioned at the top and I'm happy. BTW, thanks for attacking this Alec (despite the hard time I've given you about the whole thing). This is good stuff.
Assignee | ||
Comment 20•23 years ago
|
||
I think that static_cast was just left over cruft from an earlier iteration. good catch on that header=nsnull thing. I've fixed everything you pointed out (including removing the printfs! doh!) thanks for the review..
Attachment #77306 -
Attachment is obsolete: true
Comment 21•23 years ago
|
||
alecf: Looks like you forgot the -N option for cvs diff to get the new files in to the patch this time. I'm *so* ready to sr= this.
Assignee | ||
Comment 22•23 years ago
|
||
doh! trying that again :)
Attachment #77499 -
Attachment is obsolete: true
Comment 23•23 years ago
|
||
Comment on attachment 77511 [details] [diff] [review] More issues addressed, with -N sr=jband
Attachment #77511 -
Flags: superreview+
Comment 24•23 years ago
|
||
Comment on attachment 77511 [details] [diff] [review] More issues addressed, with -N r=dougt
Attachment #77511 -
Flags: review+
Comment 25•23 years ago
|
||
Comment on attachment 77511 [details] [diff] [review] More issues addressed, with -N a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #77511 -
Flags: approval+
Assignee | ||
Comment 26•23 years ago
|
||
thanks all, fix is in xpcom is no longer dependent on libjar!
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•