hacker formerly known as seawood@netscape.com Reporter
	Description • 23 years ago

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>.

	Alec Flett Assignee
	Comment 1 • 23 years ago

I honestly think this dependency is not as important as the libjar <-> caps
codependency (bug 102943)

	Alec Flett 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.

	Doug Turner (:dougt)
	Comment 3 • 23 years ago

Don't append it onto a contract id.  Use the category manager instead.

	John Bandhauer
	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?

	Alec Flett 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..:)

	Alec Flett 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.

	Alec Flett 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.

	Alec Flett Assignee
	Comment 8 • 23 years ago

still waiting on reviews here....

	John Bandhauer
	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?

	Doug Turner (:dougt)
	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.  

	Alec Flett 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

	Alec Flett 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

	Alec Flett Assignee
	Comment 13 • 23 years ago

here's an updated patch, with extra javadoc comments for good measure.

	John Bandhauer
	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.

	John Bandhauer
	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.

	Alec Flett 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.

	Alec Flett Assignee
	Comment 17 • 23 years ago

true enough. I don't care either way. Which would you prefer?

	John Bandhauer
	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.

	John Bandhauer
	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.

	Alec Flett 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..

	John Bandhauer
	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.

	Alec Flett Assignee
	Comment 22 • 23 years ago

doh! trying that again :)

	John Bandhauer
	Comment 23 • 23 years ago

Comment on attachment 77511 [details] [diff] [review]
More issues addressed, with -N

sr=jband

	Doug Turner (:dougt)
	Comment 24 • 23 years ago

Comment on attachment 77511 [details] [diff] [review]
More issues addressed, with -N

r=dougt

	Asa Dotzler [:asa]
	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

	Alec Flett Assignee
	Comment 26 • 23 years ago

thanks all, fix is in

xpcom is no longer dependent on libjar!