Closed Bug 107289 Opened 20 years ago Closed 20 years ago

xpcom has dependency upon libjar

Categories

(Core :: XPCOM, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla1.0

People

(Reporter: netscape, Assigned: alecf)

References

Details

(Keywords: embed, topembed-)

Attachments

(1 file, 3 obsolete files)

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>.
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
Target Milestone: mozilla0.9.8 → mozilla0.9.9
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.
Don't append it onto a contract id.  Use the category manager instead.
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?
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..:)
Target Milestone: mozilla0.9.9 → mozilla1.1
Keywords: topembed
Keywords: mozilla1.0+
Keywords: topembedembed, topembed-
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
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.
still waiting on reviews here....
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?
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.  

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
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
Attached patch updated per jband's comments (obsolete) — Splinter Review
here's an updated patch, with extra javadoc comments for good measure.
Attachment #76462 - Attachment is obsolete: true
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.
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.
doh! wow that actually compiled on windows though. who'da thunk.
change that last "} while (1);" to just "}" - I have in my patch.
true enough. I don't care either way. Which would you prefer?
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.
+        } 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.
Attached patch more issues addressed (obsolete) — Splinter Review
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
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.
doh! trying that again :)
Attachment #77499 - Attachment is obsolete: true
Comment on attachment 77511 [details] [diff] [review]
More issues addressed, with -N

sr=jband
Attachment #77511 - Flags: superreview+
Comment on attachment 77511 [details] [diff] [review]
More issues addressed, with -N

r=dougt
Attachment #77511 - Flags: review+
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+
thanks all, fix is in

xpcom is no longer dependent on libjar!
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.