David Teller [:Yoric] - still alive but not very active Reporter
	Description • 11 years ago

In bug 562674, Baku wrote
> ArchiveReader is disabled by pref, but we can make it chrome-only if we want/need.

Mossop also wrote
> It also doesn't look like ArchiveReader supports exposing the permissions of the files which is a hard requirement here.

If these are the only missing features of ArchiveReader, I would like to see them added. If they are not, let's see what's missing.

	Dave Townsend [:mossop]
	Comment 1 • 11 years ago

It's unclear to me whether ArchiveReader also allows listing the directory entries in the archive. nsIZipReader also creates synthetic directory entries when real ones don't exist, that would be a bonus but not strictly necessary.

As I said in bug 562674 I don't think loading the entire archive into memory is a viable approach, equally having to load each file to extract into memory would be a problem too.

	Andrea Marchesini [:baku]
	Comment 2 • 11 years ago

(In reply to Dave Townsend (:Mossop) from comment #1)
> It's unclear to me whether ArchiveReader also allows listing the directory
> entries in the archive. nsIZipReader also creates synthetic directory
> entries when real ones don't exist, that would be a bonus but not strictly
> necessary.

Yes it does. but just the content (the files) of the directories is shown.

	Dave Townsend [:mossop]
	Comment 3 • 11 years ago

(In reply to Andrea Marchesini (:baku) from comment #2)
> (In reply to Dave Townsend (:Mossop) from comment #1)
> > It's unclear to me whether ArchiveReader also allows listing the directory
> > entries in the archive. nsIZipReader also creates synthetic directory
> > entries when real ones don't exist, that would be a bonus but not strictly
> > necessary.
> 
> Yes it does. but just the content (the files) of the directories is shown.

I'm not sure what that means. For example if an archive contains an empty directory is that name included in the result of getFilenames?

	Andrea Marchesini [:baku]
	Comment 4 • 11 years ago

> I'm not sure what that means. For example if an archive contains an empty
> directory is that name included in the result of getFilenames?

No. ArchiveReader returns just DOM Files or filenames of files. An empty directory is not included in getFilenames.

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 5 • 11 years ago

(In reply to Dave Townsend (:Mossop) from comment #1)
> As I said in bug 562674 I don't think loading the entire archive into memory
> is a viable approach, equally having to load each file to extract into
> memory would be a problem too.

We can certainly get away without loading the entire archive into memory. This might not be completely without main thread I/O in a first version (if my understanding/recollection of the code used by ArchiveWorker is correct), but that's something that can be fixed at a later stage.

Do you have any hard facts to back the fact that extracting each file to RAM before writing it is problematic, or is it a gut feeling? (I hope I understand your above sentence correctly, if not, don't hesitate to correct me).

	Dave Townsend [:mossop]
	Comment 6 • 11 years ago

(In reply to David Rajchenbach Teller [:Yoric] from comment #5)
> (In reply to Dave Townsend (:Mossop) from comment #1)
> > As I said in bug 562674 I don't think loading the entire archive into memory
> > is a viable approach, equally having to load each file to extract into
> > memory would be a problem too.
> 
> We can certainly get away without loading the entire archive into memory.
> This might not be completely without main thread I/O in a first version (if
> my understanding/recollection of the code used by ArchiveWorker is correct),
> but that's something that can be fixed at a later stage.
> 
> Do you have any hard facts to back the fact that extracting each file to RAM
> before writing it is problematic, or is it a gut feeling? (I hope I
> understand your above sentence correctly, if not, don't hesitate to correct
> me).

Without actually trying it I can't say what the result would be, but mobile devices don't have a lot of memory and it's extremely wasteful to load tens of megabytes from disk in one block just to write it to disk again. Streaming it to disk in chunks as the current code does is far more efficient

	David Teller [:Yoric] - still alive but not very active Reporter
	Comment 7 • 11 years ago

baku, how hard would it be to extend ArchiveReader to add reading permissions?
Combined with bug 925838, this should be sufficient to get rid of all main thread I/O in xpi extraction.

	Andrea Marchesini [:baku]
	Comment 8 • 11 years ago

(In reply to David Rajchenbach Teller [:Yoric] <needinfo? me> from comment #7)
> baku, how hard would it be to extend ArchiveReader to add reading
> permissions?
> Combined with bug 925838, this should be sufficient to get rid of all main
> thread I/O in xpi extraction.

I don't think it's hard at all. Are we talking about the permissions of the files contained into the zip archive?
ArchiveReader is not actually used but I still want to know what Jonas thinks about this changes.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 9 • 10 years ago

There's various discussions around supporting "packages" as a native part of the web. Including allowing URLs directly into packages. I think we should see where that discussion heads before exposing any ArchiveReader APIs to the web.

In the meantime I don't have strong opinions about what, if anything, we expose through a chrome-only API.