Closed
Bug 379633
Opened 17 years ago
Closed 16 years ago
Scriptable zipwriter component
Categories
(Core :: General, defect)
Core
General
Tracking
()
RESOLVED
FIXED
mozilla1.9beta1
People
(Reporter: mossop, Assigned: mossop)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete)
Attachments
(3 files, 10 obsolete files)
154.05 KB,
patch
|
benjamin
:
review+
mossop
:
superreview+
bzbarsky
:
approval1.9+
|
Details | Diff | Splinter Review |
9.63 KB,
patch
|
benjamin
:
review+
benjamin
:
approval1.9+
|
Details | Diff | Splinter Review |
2.88 KB,
application/x-javascript
|
Details |
This is kind of a follow on from bug 338987 however the code I have has essentially been rewritten with what I think is a simpler API and providing more benefits. The code that I will attach soon implements a component that can load existing zip files and add/remove files to them, or just start with a blank zip file. It also implements a deflate stream converter which it uses internally to perform compression when requested for files.
Comment 1•17 years ago
|
||
Woohoo!
Assignee | ||
Comment 2•17 years ago
|
||
Here's the first version ready for review. This currently goes in mozilla/extensions/zipwriter though presumably somewhere more sensible might be found for it. This includes a bunch of testcases. I'm not sure it's worth me giving a full blow by blow description of the code here, but I can do if necessary.
Attachment #263779 -
Flags: review?
Assignee | ||
Comment 3•17 years ago
|
||
Oops, fix the comments in some places.
Attachment #263779 -
Attachment is obsolete: true
Attachment #263780 -
Flags: review?(benjamin)
Attachment #263779 -
Flags: review?
Assignee | ||
Comment 4•17 years ago
|
||
For those that don't know, you can build with this component by using the following in your mozconfig: ac_add_options --enable-extensions=default,zipwriter
Comment 5•17 years ago
|
||
First thoughts, not having taken more than a cursory glance at the patch: * Talked with Mossop over irc about undoing an action before committing (like add a file, remove the file, commit). He's indicated his zipwriter passes the testcase I suggested, and a future patch will include that testcase. * I wonder if there should be some allmakefiles.sh love. * nsIZipWriter (the interface) could be useful with other compression algorithms (tar, gz, bz2), while the component itself would handle only zip. I don't see this as a problem, but I would suggest the type of compression format be included in the component contract id somehow. (Of course, that would necessitate zipreader components for the other compression formats as well, if and when someone implements them.)
Comment 6•17 years ago
|
||
Things noted in a read-through (not super-intensive): - make the macros inline functions, for type-safety, not evaluating parameters multiple times (this happens numerous places in the patch, and compilation-unit boundaries make it possible not all can be caught by the compiler), not requiring extra parenthesization, etc. - variable declarations at the point of first use, not the C89 at-the-top-of-block style, when possible - crc_table might want to be PRUint32[] since it looks like you're relying on a particular width (if it's staying, sounds like it might not be) - crc_table's origins should at least be commented with a pointer to their source for doc purposes, if it stays - NS_ERROR_IN_PROGRESS is more informative than NS_ERROR_FAILURE if it's not abusing another module's nsresults too much - rearrange members of classes to move larger variables to the beginning; mixing PRUint32, PRUint16, PRUint8 is a good way to bloat classes with padding - use PRPackedBool for class members, not PRBool (or bitfields if you have many booleans, but I didn't remember such a place) - I see a bunch of uses of NS_Alloc/NS_Free for data used only within a single method; I'd prefer new/delete for this, in general, since that's less Mozilla-specific knowledge (stack-based would be even nicer, but it's possibly-large arrays, and only C99 has variable-size stack arrays, methinks) - addDirectoryEntry and addFile have out-of-date generated IDL comments in the implementation - need to null-check do_CreateInstance (and NS_Alloc, as later noted by Mook) - I'd prefer nsZipWriter::BeginProcessingNextItem's branches of execution as separate functions (inlined, perhaps, if the compiler is capable of doing so in response to an |inline|) - BeginProcessingNextItem is tail-recursive; GCC might get that right (maybe), but you can't rely on the compiler to get it right, so you risk stack overflow - nit: I prefer struct over class for POD types like nsZipQueueItem - implicit global variables in head_zipwriter.js aren't the best style, in my opinion (except for functions, Components.Constructor returns, and Cc/Ci/etc.) - consider using http://developer.mozilla.org/en/docs/Components.Constructor - for the |do_throw("shouldn't be able to add an existing entry");| test, the catch for it is empty -- test for a specific exception, even if not specified in the interface (probably should be), to ensure changes to the exception are intentional - many magic numbers in the tests should be consts with descriptive names
Assignee | ||
Comment 7•17 years ago
|
||
Comment on attachment 263780 [details] [diff] [review] implementation rev 1 Going to address a bunch of stuff from the earlier comments so cancelling review for now.
Attachment #263780 -
Flags: review?(benjamin)
Assignee | ||
Comment 8•17 years ago
|
||
This is an updated version that addresses the majority of the previous comments, adds some more testcases and fixes a problem that would have left certain zip files corrupt. The only comment that wasnt addressed was about the global variables in head_zipwriter.js. These are necessary in my opinion in order that any opened zip file can be correctly closed and deleted in tail_zipwriter.js
Attachment #263780 -
Attachment is obsolete: true
Assignee | ||
Updated•17 years ago
|
Attachment #263876 -
Flags: review?(benjamin)
Assignee | ||
Comment 9•17 years ago
|
||
Updated patch, most changes suggested by Mook in a review over IRC. Fixes some issues with ioflags on Open. Fixes an issue caused by assuming that nsIFile.lastSize was a PRTime. Performs better cleanup in the event of a failure in the middle of an operation (though the resultant zip file will end up pretty much corrupt). Removes some potential leaks. Gets rid of lots more magic numbers. Use helper functions for creating inputstreams etc. Adds more testcases for problems during asynchronous operation and directory adding.
Attachment #263876 -
Attachment is obsolete: true
Attachment #263876 -
Flags: review?(benjamin)
Comment 10•17 years ago
|
||
|delete foo| does the right thing if |foo| is null, so unconditionally deleting is fine if you null it out afterward. You also have at least a couple instances of |X* a = new X[N]; if (!a) { delete [] a; ... }| where the delete is just visual clutter (any decent compiler would have optimized it away). Style nit: add whitespace around binary operators for better readability, in other words |x += 3;| instead of |x+=3;|. You have the following pattern several places in the code: > if (FAILED(some operation)) > maybe do cleanup operation X and then return; > if (FAILED(some second operation)) > maybe do cleanup operation X and then return; The visual clutter can be reduced by the following transformation: > if (FAILED(some operation) || > FAILED(some second operation)) > maybe do cleanup operation X and then return; In nsZipWriter::OnFileEntryComplete you have quite a few error cases of |Cleanup(); return;|. If you structure the code right, you could simplify these substantially with well-placed gotos to a label, although I think you'd have to tweak local variable declarations so that you're not jumping over constructor execution (I think C++ forbids this, but I could be wrong). This might also be a place to apply the previous note, although for reader sanity I'm not sure you really want the ifs to be too long. The new function findError doesn't seem to be used anywhere; maybe a missed file?
Assignee | ||
Comment 11•17 years ago
|
||
(In reply to comment #10) > |delete foo| does the right thing if |foo| is null, so unconditionally deleting > is fine if you null it out afterward. I could only figure that you were referring to ~nsZipHeader here so I've corrected that. > You also have at least a couple > instances of |X* a = new X[N]; if (!a) { delete [] a; ... }| where the delete > is just visual clutter (any decent compiler would have optimized it away). I could only find one instance of this, but corrected that too. > Style nit: add whitespace around binary operators for better readability, in > other words |x += 3;| instead of |x+=3;|. Yeah bad habit from another code style I'm used to. > You have the following pattern several places in the code: > > > if (FAILED(some operation)) > > maybe do cleanup operation X and then return; > > if (FAILED(some second operation)) > > maybe do cleanup operation X and then return; > > The visual clutter can be reduced by the following transformation: > > > if (FAILED(some operation) || > > FAILED(some second operation)) > > maybe do cleanup operation X and then return; > > > In nsZipWriter::OnFileEntryComplete you have quite a few error cases of > |Cleanup(); return;|. If you structure the code right, you could simplify > these substantially with well-placed gotos to a label, although I think you'd > have to tweak local variable declarations so that you're not jumping over > constructor execution (I think C++ forbids this, but I could be wrong). This > might also be a place to apply the previous note, although for reader sanity > I'm not sure you really want the ifs to be too long. Not entirely sure about these points. Will have to sit down and look through on a case by case basis. > The new function findError doesn't seem to be used anywhere; maybe a missed > file? Thats a mistake, left in some debug code I was using. Will remove it. I won't bother to submit a new version of the patch with these changes right now.
Assignee | ||
Updated•17 years ago
|
Attachment #263912 -
Flags: review?(benjamin)
Assignee | ||
Comment 12•17 years ago
|
||
The previous version had some bad issues dealing with modification times across timezones so this fixes it. It also updates the testcases for moving the data into it's own subdirectory, and relies on working modification times coming from zipreader (so needs bug 379841 for them to pass). Also addresses some of the nits from the previous.
Attachment #263912 -
Attachment is obsolete: true
Attachment #264025 -
Flags: review?(benjamin)
Attachment #263912 -
Flags: review?(benjamin)
Comment 13•17 years ago
|
||
Comment on attachment 264025 [details] [diff] [review] implementation rev 4 General notes: please de-tab >Index: extensions/zipwriter/Makefile.in I'm tired of the extensions/ dumping-ground. Can we put this in modules/libjar/zipwriter? >Index: extensions/zipwriter/public/nsIZipWriter.idl >+ /** >+ * Returns a nsIZipEntry describing a specified zip entry or null if there >+ * is no such entry in the zip file >+ * >+ * @param aZipEntry the path of the entry >+ */ >+ nsIZipEntry getEntry(in AUTF8String aZipEntry); >+ >+ /** >+ * Checks whether the zipfile contains an entry specified by zipEntry. >+ * >+ * @param aZipEntry the path of the entry >+ */ >+ boolean hasEntry(in AUTF8String aZipEntry); These two methods are exact mirrors of nsIZipReader methods, right? Do you think there is any value in a common base interface shared by the two? >+ /** >+ * Adds a new directory entry to the zip file. This operation completes >+ * immediately. >+ * >+ * @param aZipEntry the path of the directory entry >+ * @param aModTime the modification time of the directory in microseconds >+ * >+ * @return the nsIZipEntry describing the stored directory. >+ * >+ * @throws NS_ERROR_NOT_INITIALIZED if no zip file has been opened >+ * @throws NS_ERROR_FILE_ALREADY_EXISTS if the path alread exists in the >+ * file >+ * @throws NS_ERROR_IN_PROGRESS if another operation is currently in progress >+ */ >+ nsIZipEntry addDirectoryEntry(in AUTF8String aZipEntry, in PRTime aModTime); There seems to be some unfortunate logic mismatch between the synchronous API and the async API. This method in particular takes a name + modtime, while the async API takes a nsIFile which happens to be a directory. Is there any reason you couldn't have a single API with a sync/async flag, e.g.: void addEntryDirectory(in AUTF8String aZipEntry, in PRTime aModTime, in boolean aQueue); void addEntryStream(in AUTF8String aZipEntry, in PRTime aModTime, in nsIChannel data, in boolean aQueue); void addEntryFile(in AUTF8String aZipEntry, in nsIFile data, in boolean aQueue); void removeEntry(in AUTF8String aZipEntry, in boolean aQueue) Note that this does switch from using an outputstream to a channel: if in sync mode it would call channel.open, and if in async it would call channel.asyncOpen. It also removes the nsIZipEntry return values because I'm having trouble seeing how they are valuable. But you could certainly keep them and return null in the async case. I'm not sure whether this is really hard to do, or whether this is a case of "perfect as the enemy of the good"... tell me what you think. >Index: extensions/zipwriter/src/Makefile.in >+LIBXUL_LIBRARY = 1 LIBXUL_LIBRARYies require changes to toolkit/library as well... >Index: extensions/zipwriter/src/nsDeflateConverter.cpp >+NS_IMETHODIMP nsDeflateConverter::OnDataAvailable(nsIRequest *aRequest, >+ nsISupports *aContext, >+ nsIInputStream *aInputStream, >+ PRUint32 aOffset, >+ PRUint32 aCount) >+{ >+ if (!mListener) >+ return NS_ERROR_NOT_INITIALIZED; >+ >+ char* buffer = new char[aCount]; Please use nsAutoArrayPtr to avoid the manual delete[] calls here and in nsZipReader::ReadCDSHeader nsZipOutputStream::OnDataAvailable >Index: extensions/zipwriter/test/Makefile.in >+check:: What's the empty check target for? I don't think this is necessary.
Attachment #264025 -
Flags: review?(benjamin) → review-
Assignee | ||
Comment 14•17 years ago
|
||
Next round. Addressed all the previous comments.
Attachment #264025 -
Attachment is obsolete: true
Comment 15•17 years ago
|
||
Comment on attachment 269739 [details] [diff] [review] implementation rev 5 More drive-by comments. (Note these comments actually sound harsher than I intended - sorry. This is in no way an official review, and you're free to ignore everything I've said here if you so choose.) I saw a couple instances of "till" in here. Please don't end up on bug 106386. :) >Index: modules/libjar/zipwriter/src/nsZipWriter.cpp >+nsresult nsZipWriter::ReadFile(nsIFile *aFile) >+{ ... >+ // Will never reach here in reality >+ return NS_ERROR_UNEXPECTED; Add a NS_NOTREACHED() perhaps? (Sure, the loop can't be broken out of... for now.) >+NS_IMETHODIMP nsZipWriter::AddEntryDirectory(const nsACString & aZipEntry, PRTime aModTime, PRBool aQueue) >+ if (aQueue) { ... >+ return NS_OK; >+ } >+ else { Else after return? http://beaufour.dk/jst-review/?patch=https%3A%2F%2Fbugzilla.mozilla.org%2Fattachment.cgi%3Fid%3D269739 You put an else after a return at least twice more. I'm surprised the jst-review simulacrum only caught one of them. >+NS_IMETHODIMP nsZipWriter::RemoveEntry(const nsACString & aZipEntry, PRBool aQueue) >+ while (count > 0) { >+ if (count < sizeof(buf)) >+ read = count; >+ else >+ read = sizeof(buf); Inconsistent spacing. >Index: modules/libjar/zipwriter/test/unit/tail_zipwriter.js >+try { >+ zipW.close(); >+} >+catch (e) { >+} I really dislike empty catch blocks. They don't tell me why you're catching the error. Even a |// do nothing| comment would be better. Besides, as I noted above, if ReadFile() returns NS_ERROR_UNEXPECTED (and it propagates all the way out, which I'm assuming it does), this will silently dump that on the floor. In test code in particular, I'd think you'd never want this to throw an exception after the test is complete. Please explain why you would want that, if I've misunderstood. >Index: modules/libjar/zipwriter/test/unit/test_asyncadd.js >+function run_test() >+{ >+ //do_throw(""); Why leave this commented out and in the patch? >Index: modules/libjar/zipwriter/test/unit/test_asyncbadadd.js >+const FILENAME = "missing.txt" Semicolon at the end of the line, please? (Even though it's not necessary.) Ditto for the following file: >Index: modules/libjar/zipwriter/test/unit/test_asyncbadremove.js >Index: modules/libjar/zipwriter/test/unit/test_editexisting.js >+ try { >+ zipW.removeEntry(BADENTRY, false); >+ do_throw("shouldn't be able to remove a non-existant"); >+ } Bug 106386 again ("non-existent"). Also, a non-existent what? >Index: modules/libjar/zipwriter/test/unit/test_storedata.js >+ var diff = Math.abs((entry.lastModifiedTime/PR_USEC_PER_MSEC) - time); Suggest a space before and after the slash here, for readability. >Index: modules/libjar/zipwriter/test/unit/test_undochange.js >+function run_test() >+{ >+ try { >+ var source = do_get_file(DATA_DIR + TESTS[0]); >+ zipW.addEntryFile(TESTS[0], Ci.nsIZipWriter.COMPRESSION_NONE, source, false); >+ do_throw("Should not be able to add the same file twice"); >+ } >+ catch (e) { >+ } You're not even checking the error type here. Previous tests have checked that.
Assignee | ||
Comment 16•17 years ago
|
||
Thanks for the comments, this addresses all those and I've fixed everything from the patch reviewer that I think are real issues.
Attachment #269739 -
Attachment is obsolete: true
Attachment #269848 -
Flags: review?(benjamin)
Comment 17•17 years ago
|
||
Comment on attachment 269848 [details] [diff] [review] implementation rev 6 >Index: modules/libjar/zipwriter/public/Makefile.in >+EXPORTS = $(NULL) Don't even bother... remove this line. >+XPIDLSRCS = nsIZipWriter.idl \ >+ $(NULL) In this and many places below, please use the following style: XPIDLSRCS = \ nsIZipWriter.idl \ $(NULL) (that's a two-space indent) >Index: modules/libjar/zipwriter/src/nsZipDataStream.cpp >+#define TBLS 1 >+#include "../../../zlib/src/zutil.h" >+#include "../../../zlib/src/crc32.h" This is not right. You want a makefile var (ZLIB_REQUIRES I think?) to make sure you pick up the correct version of the zlib headers and whatnot. I think this warrants a second-review... biesi if he's available, or maybe Neil.
Attachment #269848 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 18•17 years ago
|
||
Tidied up the makefiles. Removed the hack to get at the crc tables by just using zlib's crc32 implementation (duh!). Given that that's only a 2 line change I'll assume the reviews still good.
Attachment #269848 -
Attachment is obsolete: true
Attachment #269892 -
Flags: review+
Assignee | ||
Updated•17 years ago
|
Attachment #269892 -
Flags: superreview?(cbiesinger)
Comment 19•17 years ago
|
||
Comment on attachment 269892 [details] [diff] [review] implementation rev 7 Hmm, most code doesn't put the name of the original author on its own line modules/libjar/zipwriter/public/nsIZipWriter.idl Use forward declarations instead of #includes You should document whether it's necessary to call addEntryDirectory for adding entries in a subdir Documenting the format for entry names would be good too - do they have to start with a slash, do they use slashes or backslashes, etc Hm... I wonder if it would be a good idea to have an add function that returns an nsIOutputStream that the caller could write into modules/libjar/zipwriter/src/StreamFunctions.cpp + * Fully reads the required amount of data. Keeps reading untill all the until only has one l ZW_ReadData needs to handle the case when Read returns success and a 0 count (that indicates EOF) modules/libjar/zipwriter/src/StreamFunctions.h + val += ((PRUint16)buf[(*off)++] & 0xff) << 8; I'd use |= here, since you are really doing bit manipulation here, not addition as such The functions here should be marked NS_HIDDEN modules/libjar/zipwriter/src/nsDeflateConverter.cpp + // buffer is full, time to write it to disk! this doesn't necessarily write do disk, it just passes it on to the listener... modules/libjar/zipwriter/src/nsZipDataStream.cpp + nsIStreamConverter* converter = new nsDeflateConverter(aCompression); + NS_ENSURE_TRUE(converter, NS_ERROR_OUT_OF_MEMORY); + + rv = converter->AsyncConvertData("uncompressed", "deflate", mOutput, + nsnull); you shouldn't call functions on objects that have a zero refcount. I'd just store it in an nsCOMPtr. + } + else + mHeader->mMethod = ZIP_METHOD_STORE; hm, I sort of think it looks better to have braces around the else too, if the then-branch has them I'd have appreciated some comments about what the classes here are for, especially nsZipDataStream + if (NS_FAILED(rv)) { + mWriter->EntryCompleteCallback(mHeader, rv); + } + + rv = CompleteEntry(); + rv = mWriter->EntryCompleteCallback(mHeader, rv); so you're calling the callback twice in some cases? + mHeader->mCRC = crc32(mHeader->mCRC, (const unsigned char*)aBuffer, aCount); I'd prefer a C++ cast here modules/libjar/zipwriter/src/nsZipHeader.cpp +/* readonly attribute unsigned long size; */ +NS_IMETHODIMP nsZipHeader::GetSize(PRUint32 *aSize) +{ + NS_ENSURE_ARG_POINTER(aSize); nullchecking out params is a complete waste of codesize and cpu cycles IMO (not just in this function) + mComment = NS_LITERAL_CSTRING(""); mComment.Truncate(); (twice) It would be sort of nice if you could describe the overall structure of a zip file somewhere, so that people reading this code have an idea what a CDS header is, or a EOCDR header. Would be nice if BeginProcessingAddition didn't have to duplicate code from the individual Add* functions
Attachment #269892 -
Flags: superreview?(cbiesinger) → superreview+
Comment 20•17 years ago
|
||
(In reply to comment #19) > It would be sort of nice if you could describe the overall structure of a zip > file somewhere, so that people reading this code have an idea what a CDS header > is, or a EOCDR header. I'd assume most people working with this code will have stumbled upon <http://mxr.mozilla.org/mozilla/source/modules/libjar/appnote.txt>, but it probably wouldn't hurt to mention it in comments.
Assignee | ||
Comment 21•17 years ago
|
||
(In reply to comment #19) > (From update of attachment 269892 [details] [diff] [review]) > Hmm, most code doesn't put the name of the original author on its own line According to the boilerplate (http://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c), on a new line is the way to go. > Hm... I wonder if it would be a good idea to have an add function that returns > an nsIOutputStream that the caller could write into I don't disagree, in fact an earlier version of the patch had one, however it's reasonably easy to set this up with a pipe and I think right now it'd be good to get this landed before thinking about adding that. > Would be nice if BeginProcessingAddition didn't have to duplicate code from the > individual Add* functions I don't really see that there is much code duplicated at all between the functions, a few lines maybe, not worth doing this I think.
Assignee | ||
Comment 22•17 years ago
|
||
This fixes all the other issues mentioned by biesi and adds some additional comments to the idl and classes including one with a brief outline of the zip structure and a link to the full spec. This patch also adds a configure argument to enable/disable the component and marks it as default off.
Attachment #269892 -
Attachment is obsolete: true
Comment 23•17 years ago
|
||
Comment on attachment 274660 [details] [diff] [review] implementation rev 8 I don't think you need the AC_DEFINE... the AC_SUBST should be sufficient.
Attachment #274660 -
Flags: review+
Assignee | ||
Comment 24•17 years ago
|
||
Made that minor change and biesi gave his sr over IRC with a drop to the indent on the license blocks.
Attachment #274660 -
Attachment is obsolete: true
Attachment #274672 -
Flags: superreview+
Attachment #274672 -
Flags: review+
Comment 25•17 years ago
|
||
Are people aware of this comment in http://lxr.mozilla.org/seamonkey/source/LEGAL 34 C) Stac, Inc., and its licensing agent Hi/fn, own several patents which 35 disclose data compression methods implementing an LZS compression 36 algorithm, including U.S. Patent Nos. 4,701,745 and 5,016, 009 ("the 37 Stac Patents"). The Netscape Communicator code does not perform 38 compression. If you modify the Netscape source code to perform 39 compression, please take notice of the Stac Patents.
Assignee | ||
Comment 26•17 years ago
|
||
(In reply to comment #25) > Are people aware of this comment in > > http://lxr.mozilla.org/seamonkey/source/LEGAL My belief is that nothing in this code should have any effect on that statement. The compression code I call is the deflate code already in tree in zlib. What's more cairo, nss and libimg already call the same code. However I guess it might be worth getting some kind of legal input on it.
Assignee | ||
Comment 27•17 years ago
|
||
Comment on attachment 274672 [details] [diff] [review] ready for checkin Seeking approval to land this as is. It is not part of the default build so no risk to any of the current apps. I'm speaking to legal about the issue in comment 25 and won't actually land unless it's resolved.
Attachment #274672 -
Flags: approval1.9?
Updated•17 years ago
|
Attachment #274672 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Updated•17 years ago
|
Target Milestone: --- → mozilla1.9 M8
Comment 28•17 years ago
|
||
Relevant references: http://www.uspto.gov/main/faq/p120010.htm Patent 4,701,745 (utility) filed March 3, 1986, granted October 20, 1987 (I think). 17 years after the grant is October 20, 2007 (really close). 20 years after utility is March 3, 2006. In other words, if I'm reading this right, this one is about to go out of force. As for the other one, patent 5016009, filed January 13, 1989, granted May 14, 1991. Normally, I'd say "Uh-oh." But get this: http://www.uspto.gov/go/og/2003/week27/patexpi.htm It's listed as an expired patent. A petition has been filed for some reason - search on https://ramps.uspto.gov/eram/patentMaintFees.do. IANAL nor a paralegal (I'm an engineer), but this might be good information for the guys in legal to think about. It didn't take me very long to find.
Comment 29•17 years ago
|
||
Sorry, I forgot to include the patent search link: http://patft.uspto.gov/netahtml/PTO/srchnum.htm
Assignee | ||
Comment 30•16 years ago
|
||
This is updated for trunk. No code changes but required moving around for allmakefiles.sh. This also now enables zipwriter by default for all except the embedding profiles. Because of the change in behaviour re-requesting review and will re-request approval to land after the freeze.
Attachment #274672 -
Attachment is obsolete: true
Attachment #280789 -
Flags: superreview+
Attachment #280789 -
Flags: review?(benjamin)
Updated•16 years ago
|
Attachment #280789 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 31•16 years ago
|
||
Comment on attachment 280789 [details] [diff] [review] updated for trunk [checked in] Seeking approval to land this for M9. This is now enabled for all regular apps but as an unused component shouldn't impact any app or content however provides a much wanted feature for extension developers and application developers.
Attachment #280789 -
Flags: approval1.9?
![]() |
||
Comment 32•16 years ago
|
||
Are there any security concerns here? That is, is it possiblem for an extension to shoot itself in the foot by passing insufficiently-vetted args to this component?
Assignee | ||
Comment 33•16 years ago
|
||
(In reply to comment #32) > Are there any security concerns here? That is, is it possiblem for an > extension to shoot itself in the foot by passing insufficiently-vetted args to > this component? I can't think of anything that might be what I'd consider a security problem. About the only things you can pass to the component other than simple strings are channels, inputstreams and files. For all these all the component takes an interest in is the data it can pull out of those. So unless there is some issue with some sensitive data that an extension can already get hold of I can't think of a problem. Sure if an extension wanted to be malicious it could attempt to overwrite it's own, or Firefox's jar files with it, but they can do that already, and there are easier ways to override some of the app's chrome using a chrome override for instance. The component can also be used to create xpi files that could be installed as extensions (actually one of it's use cases), but again if someone wanted to be malicious there are easier ways to install a rogue extension into the browser.
![]() |
||
Comment 34•16 years ago
|
||
Comment on attachment 280789 [details] [diff] [review] updated for trunk [checked in] OK. Just making sure there are no situations where a zip file constructed from untrusted data (e.g. from a website) can cause crashes or whatnot. That is, that the internal structure of the data passed to this code is irrelevant and that it's automatically self-consistent. If that's the case, a=bzbarsky.
Attachment #280789 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 35•16 years ago
|
||
This was checked in however is showing test failures on windows and linux so I've disabled it by default for the time being. I have a good idea what's going on but need to double check it tomorrow
Assignee | ||
Comment 36•16 years ago
|
||
This is a follow up patch to correct 3 issues: Needed to add the interfaces to the packaging. The test failures are caused by nsIFile's stat cache, cloning the file gives an updated value which shows the tests passing. Spotted a potential problem. Since nsIFile is mutable we should clone a copy to retain for the zip file and any files added to the queue. This stops callers from altering the value we have.
Attachment #280789 -
Attachment is obsolete: true
Attachment #281180 -
Flags: review?(benjamin)
Updated•16 years ago
|
Attachment #281180 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 37•16 years ago
|
||
Comment on attachment 281180 [details] [diff] [review] follow-up patch Seeking approval, this fixes the unit tests, re-enables testing, packages the component correctly and corrects a potential issue in the component.
Attachment #281180 -
Flags: approval1.9?
Assignee | ||
Updated•16 years ago
|
Attachment #280789 -
Attachment description: updated for trunk → updated for trunk [checked in]
Attachment #280789 -
Attachment is obsolete: false
Updated•16 years ago
|
Attachment #281180 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 38•16 years ago
|
||
Final patch in and all tests passing now, looks like this is done :)
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 39•16 years ago
|
||
Is someone going to create a page on developer.mozilla.org documenting this?
Assignee | ||
Comment 40•16 years ago
|
||
yes there could do with being some docs put up
Keywords: dev-doc-needed
Assignee | ||
Updated•16 years ago
|
Target Milestone: mozilla1.9 M8 → mozilla1.9 M9
Updated•16 years ago
|
Flags: in-testsuite+
Comment 42•16 years ago
|
||
For some reason this code is not in my pulled copy of the trunk tree. I'm trying to figure out what's wrong. It's coming up very soon on my plate of things to document.
Comment 43•16 years ago
|
||
I would love to incorporate this component into a couple of my extensions (FEBE and CLEO) but I can't find any useful documentation anywhere. The code at http://mxr.mozilla.org/seamonkey/source/modules/libjar/zipwriter/public/nsIZipWriter.idl is too scant and cryptic to be of much help, and the nsIZipWriter Interface Reference at http://unstable.elemental.com/mozilla/build/latest/mozilla/modules/dox/interfacensIZipWriter.html times out when loading. A Google code search for "nsIZipWriter" turns up nothing. Do any docs, tutorials, examples, etc. exist yet?
Comment 44•16 years ago
|
||
AFAIK, no documentation is written yet, but you can use the tests as examples: http://mxr.mozilla.org/seamonkey/source/modules/libjar/zipwriter/test/unit/
Comment 45•16 years ago
|
||
Any word on the documentation yet? I'm making some progress using the above test examples but it's frustratingly slow going. The part that has me stumped now is how to incorporate the nsIObserver for the queue processing ... very little in the test examples that I can find.
Assignee | ||
Comment 46•16 years ago
|
||
nsIObserver is not used in this component. Possibly you are referring to nsIRequestObserver. As the comment says it is notified at the start and end of processing. Using the methods onStartRequest and onStopRequest
Comment 47•16 years ago
|
||
Comment 48•16 years ago
|
||
OK, I have been successful in creating an archive. All works as it should *except* adding files synchronously. Fx 3.0b2 generates a "Crash ! Bang! Boom!" error and shuts down. I don't know if it's my code or a bug in zipWriter. I've included my code as an attachment.
![]() |
||
Comment 49•16 years ago
|
||
Chuck, do you have a breakpad incident ID for that crash, by any chance?
Comment 50•16 years ago
|
||
I'm not sure what a "breakpad incident ID" is, but the crash was submitted through talkback each time. I can easily generate another crash if that would be helpful. Just tell me what to look for.
Comment 51•16 years ago
|
||
Breakpad is what's underneath the new crash reporter, we don't have talkback anymore. If you had a recent nightly build (or wait for b3), there's an about:crashes page that would let you find your reports.
![]() |
||
Comment 52•16 years ago
|
||
And in b2, you can follow the directions at http://kb.mozillazine.org/Breakpad#Location_of_crash_reports to find the submitted crash reports in question; those files will contain links to the website that has the stack and such.
Comment 53•16 years ago
|
||
Crash ID: bp-178b77ca-d1de-11dc-b2fd-001a4bd43ef6
Assignee | ||
Comment 54•16 years ago
|
||
(In reply to comment #53) > Crash ID: bp-178b77ca-d1de-11dc-b2fd-001a4bd43ef6 Could you please file a new bug for this.
Comment 55•16 years ago
|
||
Documented here: http://developer.mozilla.org/en/docs/nsIZipWriter
Keywords: dev-doc-needed → dev-doc-complete
Comment 56•16 years ago
|
||
Very nice! I noticed that there was no testArchive() method mentioned. Was this an oversight or is it not implemented?
Assignee | ||
Comment 57•16 years ago
|
||
(In reply to comment #56) > Very nice! I noticed that there was no testArchive() method mentioned. Was > this an oversight or is it not implemented? > There was never any testArchive method on this component. In fact I'm don't see anywhere that the suggestion was ever mentioned.
Comment 58•16 years ago
|
||
Ok. Considered it mentioned :)
Assignee | ||
Comment 59•16 years ago
|
||
nsIZipWriter does no extraction which is what testing would require. The nsIZipReader interface already has test functionality.
You need to log in
before you can comment on or make changes to this bug.
Description
•