Last Comment Bug 695843 - Nested omni.jar for Android
: Nested omni.jar for Android
Status: RESOLVED FIXED
[mobilestartupshrink][ts][inbound]
:
Product: Core
Classification: Components
Component: Widget: Android (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: mozilla11
Assigned To: Mike Hommey [:glandium]
:
:
Mentors:
Depends on: 712433 709250
Blocks: 697356
  Show dependency treegraph
 
Reported: 2011-10-19 13:39 PDT by Mike Hommey [:glandium]
Modified: 2011-12-20 14:01 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
[WIP] part 1 - Make Omnijar::GetReader return an nsIZipReader instead of nsZipArchive (19.79 KB, patch)
2011-10-24 06:19 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
part 2 - Avoid code duplication for components manifest registration (3.52 KB, patch)
2011-10-24 06:23 PDT, Mike Hommey [:glandium]
benjamin: review+
Details | Diff | Splinter Review
[WIP] part 3 - Get the component manager to use Omnijar::GetReader instead of Omnijar::GetPath (9.51 KB, patch)
2011-10-24 06:29 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
[WIP] part 4 - Allow nested omni.jar and build one for Android (5.57 KB, patch)
2011-10-24 06:35 PDT, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
part 1 - Add Refcounting on nsZipArchives (15.44 KB, patch)
2011-11-08 09:02 PST, Mike Hommey [:glandium]
mwu.code: review+
Details | Diff | Splinter Review
part 3 - Use mozilla::AutoFDClose instead of ad hoc AutoCloseFD classes (3.70 KB, patch)
2011-11-08 09:03 PST, Mike Hommey [:glandium]
benjamin: review+
Details | Diff | Splinter Review
part 4 - Add a FileLocation class to either store a pair (zip, path) or a file, and do some operations on these (13.46 KB, patch)
2011-11-08 09:05 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
part 5 - Allow nsZipCursor to always use the given buffer (2.20 KB, patch)
2011-11-08 09:06 PST, Mike Hommey [:glandium]
taras.mozilla: review-
Details | Diff | Splinter Review
part 6 - Don't separate file and JAR modules in component manager (7.83 KB, patch)
2011-11-08 09:07 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
part 7 - Use the URI string as the key for the various mozJSComponentLoader hashtables (9.21 KB, patch)
2011-11-08 09:09 PST, Mike Hommey [:glandium]
mrbkap: review+
Details | Diff | Splinter Review
part 8 - Properly handle jar-in-jars when importing modules (1.28 KB, patch)
2011-11-08 09:10 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
part 9 - Use FileLocations in the component manager (58.12 KB, patch)
2011-11-08 09:11 PST, Mike Hommey [:glandium]
benjamin: feedback+
Details | Diff | Splinter Review
part 10 - Allow nested omni.jar and build one for Android (4.57 KB, patch)
2011-11-08 09:14 PST, Mike Hommey [:glandium]
benjamin: review+
Details | Diff | Splinter Review
part 6 - Don't separate file and JAR modules in component manager (7.83 KB, patch)
2011-11-08 09:19 PST, Mike Hommey [:glandium]
benjamin: review+
Details | Diff | Splinter Review
part 8 - Properly handle jar-in-jars when importing modules (1.26 KB, patch)
2011-11-09 05:29 PST, Mike Hommey [:glandium]
mrbkap: review+
Details | Diff | Splinter Review
part 1 - Add Refcounting on nsZipArchives (15.48 KB, patch)
2011-11-09 05:30 PST, Mike Hommey [:glandium]
mh+mozilla: review+
Details | Diff | Splinter Review
part 1 - Add Refcounting on nsZipArchives. (16.17 KB, patch)
2011-12-07 00:57 PST, Mike Hommey [:glandium]
mh+mozilla: review+
Details | Diff | Splinter Review
part 3 - Use mozilla::AutoFDClose instead of ad hoc AutoCloseFD classes (3.75 KB, patch)
2011-12-07 01:00 PST, Mike Hommey [:glandium]
mh+mozilla: review+
Details | Diff | Splinter Review
part 4 - Allow nsZipCursor to always use the given buffer (2.63 KB, patch)
2011-12-07 01:05 PST, Mike Hommey [:glandium]
taras.mozilla: review+
Details | Diff | Splinter Review
part 5 - Add a FileLocation class to either store a pair (zip, path) or a file, and do some operations on these (16.34 KB, patch)
2011-12-07 01:11 PST, Mike Hommey [:glandium]
benjamin: review+
Details | Diff | Splinter Review
part 9 - Use FileLocations in the component manager (58.48 KB, patch)
2011-12-07 01:23 PST, Mike Hommey [:glandium]
benjamin: review+
Details | Diff | Splinter Review
part 10 - Allow nested omni.jar and build one for Android (4.59 KB, patch)
2011-12-07 01:26 PST, Mike Hommey [:glandium]
mh+mozilla: review+
Details | Diff | Splinter Review
extra - Use more canonical keys for component manager and JS component loader hash tables (14.34 KB, patch)
2011-12-07 01:33 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
part 5 - Add a FileLocation class to either store a pair (zip, path) or a file, and do some operations on these. (16.40 KB, patch)
2011-12-07 23:34 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
part 9 - Use FileLocations in the component manager. (58.54 KB, patch)
2011-12-07 23:51 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
part 5 - Add a FileLocation class to either store a pair (zip, path) or a file, and do some operations on these. (16.38 KB, patch)
2011-12-07 23:52 PST, Mike Hommey [:glandium]
mwu.code: review+
Details | Diff | Splinter Review
part 9 - Use FileLocations in the component manager. (58.49 KB, patch)
2011-12-07 23:58 PST, Mike Hommey [:glandium]
mh+mozilla: review+
Details | Diff | Splinter Review
extra - Use more canonical keys for component manager and JS component loader hash tables (14.34 KB, patch)
2011-12-07 23:59 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review

Description Mike Hommey [:glandium] 2011-10-19 13:39:38 PDT
Around 200ms are wasted on a nexus s at startup because Android does something with the apk zip entries and takes time for this because we have so many entries in the apk. Getting the number of entries down should allow us to avoid wasting these 200ms. We can do that by keeping chrome, components and resources compressed under a single (uncompressed) file in the apk.
Comment 1 Mike Hommey [:glandium] 2011-10-24 06:16:10 PDT
(In reply to Mike Hommey [:glandium] from comment #0)
> Around 200ms are wasted on a nexus s at startup.

Note that it's even more on multi-language builds.
Comment 2 Mike Hommey [:glandium] 2011-10-24 06:19:42 PDT
Created attachment 569041 [details] [diff] [review]
[WIP] part 1 - Make Omnijar::GetReader return an nsIZipReader instead of nsZipArchive

It still needs to be modified such that nsJAR::GetZipArchive is not required.
Comment 3 Mike Hommey [:glandium] 2011-10-24 06:23:08 PDT
Created attachment 569042 [details] [diff] [review]
part 2 - Avoid code duplication for components manifest registration
Comment 4 Mike Hommey [:glandium] 2011-10-24 06:29:53 PDT
Created attachment 569043 [details] [diff] [review]
[WIP] part 3 - Get the component manager to use Omnijar::GetReader instead of Omnijar::GetPath

I'm not sure we should keep nsIZipReaders around, here. The impact should definitely be checked.
Comment 5 Mike Hommey [:glandium] 2011-10-24 06:35:32 PDT
Created attachment 569044 [details] [diff] [review]
[WIP] part 4 - Allow nested omni.jar and build one for Android

This part definitely needs more work, to have something proper for the chrome registry and js component loader. The other part that's broken with this patch is that ManifestProcessingContext, both in the component manager and the chrome registry, are using nsIFiles for the base path to resolve from, and thus fail with nested omni.jar. They probably need to store either an nsIFile or an nsIZipReader depending on cases.

(I just wish we had fake nsIFiles that would allow to use Zips transparently, like directories, but so many things rely on nsIFiles to actually be nsILocalFiles...)
Comment 6 Mike Hommey [:glandium] 2011-11-08 09:02:25 PST
Created attachment 572834 [details] [diff] [review]
part 1 - Add Refcounting on nsZipArchives
Comment 7 Mike Hommey [:glandium] 2011-11-08 09:03:41 PST
Created attachment 572835 [details] [diff] [review]
part 3 - Use mozilla::AutoFDClose instead of ad hoc AutoCloseFD classes
Comment 8 Mike Hommey [:glandium] 2011-11-08 09:05:17 PST
Created attachment 572836 [details] [diff] [review]
part 4 - Add a FileLocation class to either store a pair (zip, path) or a file, and do some operations on these

I'm not settled on this one. I may switch to using nsZipHandles instead of nsZipArchives in FileLocation. The class also needs some documentation, and the API may require some tweaking.
Comment 9 Mike Hommey [:glandium] 2011-11-08 09:06:28 PST
Created attachment 572838 [details] [diff] [review]
part 5 - Allow nsZipCursor to always use the given buffer
Comment 10 Mike Hommey [:glandium] 2011-11-08 09:07:44 PST
Created attachment 572840 [details] [diff] [review]
part 6 - Don't separate file and JAR modules in component manager
Comment 11 Mike Hommey [:glandium] 2011-11-08 09:08:46 PST
(In reply to Mike Hommey [:glandium] from comment #10)
> Created attachment 572840 [details] [diff] [review] [diff] [details] [review]
> part 6 - Don't separate file and JAR modules in component manager

Note that the use of net_GetURLSpecFromActualFile goes away in a subsequent patch.
Comment 12 Mike Hommey [:glandium] 2011-11-08 09:09:57 PST
Created attachment 572841 [details] [diff] [review]
part 7 - Use the URI string as the key for the various mozJSComponentLoader hashtables
Comment 13 Mike Hommey [:glandium] 2011-11-08 09:10:37 PST
Created attachment 572842 [details] [diff] [review]
part 8 - Properly handle jar-in-jars when importing modules
Comment 14 Mike Hommey [:glandium] 2011-11-08 09:11:47 PST
Created attachment 572843 [details] [diff] [review]
part 9 - Use FileLocations in the component manager

This uses the API from part 4.
Comment 15 Mike Hommey [:glandium] 2011-11-08 09:14:53 PST
Created attachment 572844 [details] [diff] [review]
part 10 - Allow nested omni.jar and build one for Android
Comment 16 Mike Hommey [:glandium] 2011-11-08 09:19:54 PST
Created attachment 572848 [details] [diff] [review]
part 6 - Don't separate file and JAR modules in component manager
Comment 17 (dormant account) 2011-11-08 09:55:55 PST
Comment on attachment 572838 [details] [diff] [review]
part 5 - Allow nsZipCursor to always use the given buffer

Wouldn't it be cleaner to add a nsZipCursor::Copy?
Comment 18 Michael Wu [:mwu] 2011-11-08 12:05:40 PST
Comment on attachment 572838 [details] [diff] [review]
part 5 - Allow nsZipCursor to always use the given buffer

Taras seems to have more of an opinion on this one than I do.
Comment 19 Mike Hommey [:glandium] 2011-11-08 13:51:48 PST
Try result on this patch queue:
https://tbpl.mozilla.org/?noignore=1&tree=Try&rev=cc189b22e21d

Summary:
- 2 nsZipArchives are leaked
- Bootstrap extensions test seems to go in an infinite loop on windows.
Comment 20 Mike Hommey [:glandium] 2011-11-09 05:19:28 PST
(In reply to Mike Hommey [:glandium] from comment #19)
> - 2 nsZipArchives are leaked

Found the culprit. Will update the relevant patch later.

Another issue: jars are likely to be mapped twice in memory with the current patch queue. Bug 504217 should help there.
Comment 21 Mike Hommey [:glandium] 2011-11-09 05:29:10 PST
Created attachment 573171 [details] [diff] [review]
part 8 - Properly handle jar-in-jars when importing modules
Comment 22 Mike Hommey [:glandium] 2011-11-09 05:30:33 PST
Created attachment 573172 [details] [diff] [review]
part 1 - Add Refcounting on nsZipArchives

Same patch, which removes a "mZip.forget()" that was a leftover from the original use of nsAutoPtr.
Comment 23 Mike Hommey [:glandium] 2011-11-09 05:32:32 PST
Comment on attachment 573172 [details] [diff] [review]
part 1 - Add Refcounting on nsZipArchives

Carrying previous r+, considering the changes.
Comment 24 Mike Hommey [:glandium] 2011-11-09 09:03:42 PST
(In reply to Mike Hommey [:glandium] from comment #19)
> - Bootstrap extensions test seems to go in an infinite loop on windows.

The test goes on waiting for nothing because it doesn't handle failure gracefully. The failure itself is due to the xpi file still being open (iow, a nsZipArchive still existing) when it is removed (NS_ERROR_FILE_ACCESS_DENIED on nsIFile.remove) in test_bootstrap.js line 113, function manuallyUninstall. Obviously that kind of thing only fail on windows.
Comment 25 (dormant account) 2011-11-10 10:18:49 PST
Comment on attachment 572838 [details] [diff] [review]
part 5 - Allow nsZipCursor to always use the given buffer

We decided to add a nsZipCursor::Copy
Comment 26 Michael Wu [:mwu] 2011-11-17 12:01:43 PST
Comment on attachment 572836 [details] [diff] [review]
part 4 - Add a FileLocation class to either store a pair (zip, path) or a file, and do some operations on these

So, I think FileLocation makes a lot of sense for the component manager code but not as much for nsZipHandle. That being said, this is one of the easier ways for you to generate uris and it doesn't mess with nsZipHandle too much. I suspect that having nsZipHandle directly generate the information you want is a cleaner way to go here, and might be useful for some other code along the way. Up to you though - if moving things to nsZipHandle is too complicated for now, that's fine.
Comment 27 Benjamin Smedberg [:bsmedberg] 2011-12-01 07:54:39 PST
Comment on attachment 572848 [details] [diff] [review]
part 6 - Don't separate file and JAR modules in component manager

I'm not sure that hashing on the URI representation is correct or will work correctly: we've had issues in the past with short and long path names not comparing equal correctly. What is the actual behavior of net_GetURLSpecFromActualFile WRT case differences or things like that?
Comment 28 Mike Hommey [:glandium] 2011-12-01 08:19:51 PST
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #27)
> Comment on attachment 572848 [details] [diff] [review] [diff] [details] [review]
> part 6 - Don't separate file and JAR modules in component manager
> 
> I'm not sure that hashing on the URI representation is correct or will work
> correctly: we've had issues in the past with short and long path names not
> comparing equal correctly. What is the actual behavior of
> net_GetURLSpecFromActualFile WRT case differences or things like that?

net_GetURLSpecFromActualFile doesn't ensure a canonical path before turning the path into an url :(
The same concern would also apply on part 7. I guess we could canonicalize paths before putting them in the uri string used for keys.
Comment 29 Mike Hommey [:glandium] 2011-12-01 08:52:21 PST
Note that the nsIHashable part of nsLocalFile* doesn't handle case sensitivity on Mac, AFAICS, so we may already have the case sensitivity problem.
Comment 30 :Ms2ger (⌚ UTC+1/+2) 2011-12-06 04:06:40 PST
Comment on attachment 573172 [details] [diff] [review]
part 1 - Add Refcounting on nsZipArchives

>--- a/startupcache/StartupCache.cpp
>+++ b/startupcache/StartupCache.cpp
>+nsresult
>+GetBufferFromZipArchive(nsZipArchive *zip, bool doCRC, const char* id,
>+                        char** outbuf, PRUint32* length)
>+{
>+  if (zip) {
>+    nsZipItemPtr<char> zipItem(zip, id, doCRC);
>+    if (zipItem) {
>+      *outbuf = zipItem.Forget();
>+      *length = zipItem.Length();
>+      return NS_OK;
>+    }
>+  }
>+  return NS_ERROR_NOT_AVAILABLE;
>+}

Early returns, please.

>+  nsresult rv;
>+  rv = GetBufferFromZipArchive(mArchive, true, id, outbuf, length);

nsresult rv = GetBufferFromZipArchive(mArchive, true, id, outbuf, length);

>--- a/xpcom/build/Omnijar.cpp
>+++ b/xpcom/build/Omnijar.cpp
> Omnijar::InitOne(nsIFile *aPath, Type aType)
>+    nsRefPtr<nsZipArchive> zipReader = new nsZipArchive();
>+    if (!zipReader || NS_FAILED(zipReader->OpenArchive(file))) {

Remove the '!zipReader' check, 'new' can't fail.

>--- a/xpcom/build/Omnijar.h
>+++ b/xpcom/build/Omnijar.h
>+static inline already_AddRefed<nsZipArchive>
> GetReader(Type aType)
> {
>     NS_ABORT_IF_FALSE(IsInitialized(), "Omnijar not initialized");
>+    NS_IF_ADDREF(sReader[aType]);
>     return sReader[aType];

nsRefPtr<nsZipArchive> reader = sReader[aType];
return reader.forget();
Comment 31 :Ms2ger (⌚ UTC+1/+2) 2011-12-06 04:13:31 PST
Comment on attachment 572836 [details] [diff] [review]
part 4 - Add a FileLocation class to either store a pair (zip, path) or a file, and do some operations on these

>--- /dev/null
>+++ b/xpcom/build/FileLocation.cpp
>+namespace mozilla {
>+
>+nsresult FileLocation::GetData(char *buf, PRUint32 len)
>+{
>+    nsZipCursor cursor(item, mBaseZip, (PRUint8 *)buf, len, true);

static_cast

>+  mozilla::AutoFDClose fd;

No need for the 'mozilla::'.

>--- /dev/null
>+++ b/xpcom/build/FileLocation.h
>+  void Init(nsILocalFile *file)
>+  {
>+    mBaseFile = file;
>+    mPath = (char *)NULL;

mPath.Truncate()?

>+/* This needs to be included at the end because of the cross-references */
>+#include "nsZipArchive.h"

Uh, really? Why does this need to be included at all?
Comment 32 :Ms2ger (⌚ UTC+1/+2) 2011-12-06 04:17:56 PST
Comment on attachment 572844 [details] [diff] [review]
part 10 - Allow nested omni.jar and build one for Android

>--- a/xpcom/build/Omnijar.cpp
>+++ b/xpcom/build/Omnijar.cpp
>@@ -105,16 +106,24 @@ Omnijar::InitOne(nsIFile *aPath, Type aT
>+    nsRefPtr<nsZipHandle> handle;
>+    if (NS_SUCCEEDED(nsZipHandle::Init(zipReader, "omni.jar", getter_AddRefs(handle)))) {
>+        zipReader = new nsZipArchive();
>+        if (!zipReader...

And here
Comment 33 Benjamin Smedberg [:bsmedberg] 2011-12-06 09:49:26 PST
Comment on attachment 572848 [details] [diff] [review]
part 6 - Don't separate file and JAR modules in component manager

Ugh. I guess that since we're using manifests now it's unlikely that we'll have mixed-case issues; I guess we'll find out later!
Comment 34 Mike Hommey [:glandium] 2011-12-07 00:57:27 PST
Created attachment 579619 [details] [diff] [review]
part 1 - Add Refcounting on nsZipArchives.

Updated with Ms2ger comments.
Comment 35 Mike Hommey [:glandium] 2011-12-07 00:58:08 PST
Comment on attachment 579619 [details] [diff] [review]
part 1 - Add Refcounting on nsZipArchives.

Carrying over r+
Comment 36 Mike Hommey [:glandium] 2011-12-07 01:00:39 PST
Created attachment 579620 [details] [diff] [review]
part 3 - Use mozilla::AutoFDClose instead of ad hoc AutoCloseFD classes

Refreshed against current m-c
Comment 37 Mike Hommey [:glandium] 2011-12-07 01:02:27 PST
Comment on attachment 579620 [details] [diff] [review]
part 3 - Use mozilla::AutoFDClose instead of ad hoc AutoCloseFD classes

Carrying over r+
Comment 38 Mike Hommey [:glandium] 2011-12-07 01:05:41 PST
Created attachment 579621 [details] [diff] [review]
part 4 - Allow nsZipCursor to always use the given buffer
Comment 39 Mike Hommey [:glandium] 2011-12-07 01:11:27 PST
Created attachment 579624 [details] [diff] [review]
part 5 - Add a FileLocation class to either store a pair (zip, path) or a file, and do some operations on these
Comment 40 Mike Hommey [:glandium] 2011-12-07 01:23:37 PST
Created attachment 579626 [details] [diff] [review]
part 9 - Use FileLocations in the component manager
Comment 41 Mike Hommey [:glandium] 2011-12-07 01:26:53 PST
Created attachment 579627 [details] [diff] [review]
part 10 - Allow nested omni.jar and build one for Android

Basically the same as previous version, but uses OMNIJAR_NAME instead of hardcoding omni.jar.
Comment 42 Mike Hommey [:glandium] 2011-12-07 01:28:46 PST
Comment on attachment 579627 [details] [diff] [review]
part 10 - Allow nested omni.jar and build one for Android

Carrying over r+
Comment 43 Mike Hommey [:glandium] 2011-12-07 01:33:50 PST
Created attachment 579628 [details] [diff] [review]
extra - Use more canonical keys for component manager and JS component loader hash tables

In the case we end up having problems with short vs. long path names making uris not stable enough to be hash keys for the component manager and the JS component loader, here is a patch that makes the relevant hash tables use something more canonical. Attaching so that it can be found later if needed, since I already did the work (instead of being lost on my harddrive)
Comment 44 (dormant account) 2011-12-07 09:46:31 PST
Comment on attachment 579621 [details] [diff] [review]
part 4 - Allow nsZipCursor to always use the given buffer

thanks
Comment 45 Benjamin Smedberg [:bsmedberg] 2011-12-07 10:24:11 PST
Comment on attachment 579624 [details] [diff] [review]
part 5 - Add a FileLocation class to either store a pair (zip, path) or a file, and do some operations on these

Some of the single-line ifs in FileLocation::FileLocation aren't braced and I find this hard to read: please brace them all.

Please comment namespace-closing brace in FileLocation.cpp, e.g.:

} // namespace mozilla

The opening brace of "class FileLocation {" and FileLocation::Data should be on the next line

It's unusual to return an nsCString (in FileLocation::GetURIString and FileLocation::GetPath). Normally methods that return a string use: 

void ReturnAString(nsACString& result);

Since we have copy-on-write strings, perhaps this isn't a problem, but I'd prefer to be consistent with other parts of our code.

r=me with those changes
Comment 46 Mike Hommey [:glandium] 2011-12-07 23:34:40 PST
Created attachment 579972 [details] [diff] [review]
part 5 - Add a FileLocation class to either store a pair (zip, path) or a file, and do some operations on these.

With bsmedberg's comments addressed
Comment 47 Mike Hommey [:glandium] 2011-12-07 23:51:02 PST
Created attachment 579973 [details] [diff] [review]
part 9 - Use FileLocations in the component manager.

Refreshed with the new modified GetURIString signature from part 5.
Comment 48 Mike Hommey [:glandium] 2011-12-07 23:52:24 PST
Created attachment 579974 [details] [diff] [review]
part 5 - Add a FileLocation class to either store a pair (zip, path) or a file, and do some operations on these.

Removed a bit that is meant to be in the "extra" patch.
Comment 49 Mike Hommey [:glandium] 2011-12-07 23:58:00 PST
Created attachment 579975 [details] [diff] [review]
part 9 - Use FileLocations in the component manager.

Removed an unused variable
Comment 50 Mike Hommey [:glandium] 2011-12-07 23:59:26 PST
Created attachment 579976 [details] [diff] [review]
extra - Use more canonical keys for component manager and JS component loader hash tables

Refreshed against refreshed part 9.
Comment 51 Mike Hommey [:glandium] 2011-12-08 00:00:03 PST
Comment on attachment 579975 [details] [diff] [review]
part 9 - Use FileLocations in the component manager.

Carrying over r+.
Comment 52 Michael Wu [:mwu] 2011-12-08 01:13:42 PST
Comment on attachment 579974 [details] [diff] [review]
part 5 - Add a FileLocation class to either store a pair (zip, path) or a file, and do some operations on these.

Review of attachment 579974 [details] [diff] [review]:
-----------------------------------------------------------------

Looks ok overall though I haven't really studied how some of the callers use some of this API. Comments are just nits.

::: xpcom/build/FileLocation.cpp
@@ +106,5 @@
> +  if (IsZip() && mBaseZip) {
> +    nsRefPtr<nsZipHandle> handler = mBaseZip->GetFD();
> +    if (handler)
> +      return handler->mFile.GetBaseFile();
> +  } else {

I think this else could be removed after moving return NULL to the "if (IsZip() && mBaseZip)" block, or maybe invert this if/else. Having the return NULL only apply to one of the if/else blocks is a bit weird.

@@ +139,5 @@
> +
> +nsresult
> +FileLocation::GetData(Data &data)
> +{
> +  if (IsZip()) {

if (!IsZip())
  return mBaseFile->OpenNSPRFileDesc(PR_RDONLY, 0444, &data.mFd);

::: xpcom/build/FileLocation.h
@@ +132,5 @@
> +
> +  /**
> +   * Returns whether the "base file" (see GetBaseFile) is an archive
> +   */
> +  bool IsZip() const {

Curly brace style inconsistent here and in the next two functions.
Comment 55 Mark Finkle (:mfinkle) (use needinfo?) 2011-12-09 14:48:13 PST
Note: this seems to have regressed being able to access APK resources from Java using jar: URIs

Note You need to log in before you can comment on or make changes to this bug.