337561 - Merge nsJARDirectoryInputStream into nsJARInputStream

Status: VERIFIED FIXED

(Core :: Networking: File, defect)

Description

[Alfred Kayser]

To reduce code, and to increase flexibility towards retrieving an nsIInputStream from a nsIURI (nsIJARURI), I propose to merge nsJARDirectoryInputStream into nsJARInputStream.
This will also make fixing the jarCache issue (bug 333202) easier.
Also, use nsZipReader directly from nsJARInputStream (for both files and directories) to prevent the overhead of nsJAR and nsIZipEntry (allowing to later the remove nsZipEntry easier).

Comment 1

[Alfred Kayser]

Attachment: V1: Merge DirectoryInputStream into nsJARInputStream

Note, this depends on bug 333129, so to get a compiling and running patch, this patch includes that patch as well (which only touches nsZipArchive.*, while this merge doesn't touch nsZipArchive.*).

Comment 2

[Alfred Kayser]

Attachment: Patch without the bug 333129 patch

Comment 3

[Alfred Kayser]

Removed dependency to 333129 which is no longer a prerequisite.

Comment 4

[Alfred Kayser]

Comment on attachment 221682 [details] [diff] [review]
Patch without the bug 333129 patch

Darin, can you review this? 
And also give some tips/thoughts on the AUTF8String/string usage as params for the IDL?

Comment 5

[Darin Fisher]

Comment on attachment 221682 [details] [diff] [review]
Patch without the bug 333129 patch

changes to interfaces require the interface UUID to be changed.

this patch should probably include the removal of nsJARDirectoryInputStream.* so that when someone goes to check this in, they know to remove those files.

r=darin with those fixes

Comment 6

[Alfred Kayser]

Attachment: V3: New UUID and included the cvs removal of nsJARDir...

Bzbarsky, can you review this?
I want to have this out of the way, so that I can work on the other bugs for libjar: bug 333129, bug 337120, bug 203627, bug 247458

Comment 7

[Boris Zbarsky [:bzbarsky]]

I'm not going to be able to get to this for at least a week; more likely 2-3 weeks.  Please find another sr if this is blocking you in any way.

Comment 8

[Alfred Kayser]

Comment on attachment 230734 [details] [diff] [review]
V3: New UUID and included the cvs removal of nsJARDir...

Darin, can you do the SR, or do you known someelse who can/must do this?

Comment 9

[Darin Fisher]

Comment on attachment 230734 [details] [diff] [review]
V3: New UUID and included the cvs removal of nsJARDir...

>+++ ./nsIZipReader.idl	2006-07-26 14:19:51.570980800 +0200

>     /**
>      * Returns an input stream containing the contents of the specified zip
>      * entry.
>      */
>     nsIInputStream getInputStream(in string zipEntry);
>+    nsIInputStream getInputStreamSpec(in AUTF8String aJarSpec, in string zipEntry);
> };

I think you should document this new function.  It's not clear from the
information here what aJarSpec means.  Also, what do you think about
naming it "getInputStreamWithSpec" or "getInputStreamForSpec"?

Comment 10

[Darin Fisher]

Comment on attachment 230734 [details] [diff] [review]
V3: New UUID and included the cvs removal of nsJARDir...

minusing to get attention.  please see previous comment.

Comment 11

[Alfred Kayser]

Attachment: V4: Updated to comments of Darin

Added comments to the new function in the IDL, and updated to current tree.

Comment 12

[Alfred Kayser]

Who can do the checkin for me?

Comment 13

[:Gavin Sharp [email: [e-mail]]]

mozilla/modules/libjar/nsJAR.cpp 	1.123
mozilla/modules/libjar/nsIZipReader.idl 	1.21
mozilla/modules/libjar/objs.mk 	1.9
mozilla/modules/libjar/nsJAR.h 	1.50
mozilla/modules/libjar/nsJARChannel.cpp 	1.122
mozilla/modules/libjar/nsJARDirectoryInputStream.cpp 	1.3
mozilla/modules/libjar/nsJARInputStream.cpp 	1.29
mozilla/modules/libjar/nsJARInputStream.h 	1.16

Comment 14

[:Gavin Sharp [email: [e-mail]]]

I backed out this patch because it caused bustage (ReadDirectory has no aBuffer or aBytesRead), and I'd rather not have to try and deal with the bustage of something that obviously hasn't been compiled.

Comment 15

[Alfred Kayser]

Attachment: V5: Shame on me! attribute names don't match the NS_ASSERTION's

Fixed the attribute names of ReadDirectory to be more inline with the rest of the code, so that the NS_ASSERTION's won't fail to compile in debug mode

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 239981 [details] [diff] [review]
V5: Shame on me! attribute names don't match the NS_ASSERTION's

Assuming this differs from the last patch only because of renamed arguments, and that this compiles with DEBUG both enabled and disabled, this should be fine to check in.

Comment 17

[Alfred Kayser]

Attachment: diff between v4 and v5 showing that only attribute names are changed

Comment 18

[Alfred Kayser]

Who wants to do the checkin for me?
Note, the original patch worked. It only failed to compile in debugmode.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

mozilla/modules/libjar/nsJARDirectoryInputStream.cpp 	1.5
mozilla/modules/libjar/nsJARChannel.cpp 	1.124
mozilla/modules/libjar/nsJAR.h 	1.52
mozilla/modules/libjar/nsJAR.cpp 	1.125
mozilla/modules/libjar/nsIZipReader.idl 	1.23
mozilla/modules/libjar/nsJARInputStream.cpp 	1.31
mozilla/modules/libjar/nsJARInputStream.h 	1.18
mozilla/modules/libjar/objs.mk 	1.11

Comment 20

[Alfred Kayser]

Thanks, this time is worked.

  libjar50.so
  	Total:	      -1108 (+2638/-3746)
  	Code:	      -1040 (+2634/-3678)
  	Data:	        -68 (+4/-68)

Comment 21

[Boris Zbarsky [:bzbarsky]]

So... this patch caused a significant (and bad) behavior change.  See bug 367459.

Was this the intended effect?