Replace Libarchive with MiniZip.

Replace Libarchive with MiniZip.

This CL rewrites all the components that depend on libarchive. Since we
plan to add the files related to build such as BUILD.gn in another CL,
GN doesn't compile the code in this CL yet.

BUG=607078
TEST=manually tested
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2807063002
Cr-Commit-Position: refs/heads/master@{#463567}
Committed: https://chromium.googlesource.com/chromium/src/+/cac7340db735b9e8228e8b5abf886b20719db262

[takise, 2017-04-10 04:36:06]

Description was changed from

==========
Replace Libarchive with MiniZip.

This CL rewrites all the components that depend on libarchive. Since we
plan to add the files related to build such as BUILD.gn in another CL,
GN doesn't compile the code in this CL yet.

BUG=607078
TEST=manually tested
==========

to

==========
Replace Libarchive with MiniZip.

This CL rewrites all the components that depend on libarchive. Since we
plan to add the files related to build such as BUILD.gn in another CL,
GN doesn't compile the code in this CL yet.

BUG=607078
TEST=manually tested
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[takise, 2017-04-10 04:39:40]

Description was changed from

==========
Replace Libarchive with MiniZip.

This CL rewrites all the components that depend on libarchive. Since we
plan to add the files related to build such as BUILD.gn in another CL,
GN doesn't compile the code in this CL yet.

BUG=607078
TEST=manually tested
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Replace Libarchive with MiniZip.

This CL rewrites all the components that depend on libarchive. Since we
plan to add the files related to build such as BUILD.gn in another CL,
GN doesn't compile the code in this CL yet.

BUG=607078
TEST=manually tested
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[takise, 2017-04-10 04:39:40]

takise@google.com changed reviewers:
+ mtomasz@chromium.org

[takise, 2017-04-10 04:43:26]

This is a big(about 1000 lines) change that replaces libarchive with minizip.
Please take a look and ask me if you have any questions. Thank you!

[mtomasz, 2017-04-10 06:16:28]

Could you update tests?

[mtomasz, 2017-04-10 07:15:10]

Thanks!

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
File ui/file_manager/zip_archiver/cpp/compressor.cc (right):

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor.cc:11: #include
"compressor_archive_libarchive.h"
Let's rename files in a separate CL.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
File ui/file_manager/zip_archiver/cpp/compressor_archive.h (right):

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_archive.h:24: // Releases all
resources obtained by libarchive.
nit: libarchive -> minizip

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
File ui/file_manager/zip_archiver/cpp/compressor_archive_libarchive.cc (right):

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_archive_libarchive.cc:14: // Called
when minizip tries to open a zip archive file. We do nothing here
nit: Let's add \n after the namespace line for better readability.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_archive_libarchive.cc:21: uLong
CustomArchiveRead(void* opaque, void* /*stream*/, void* buf, uLong size) {
nit: We usually comment out unused arguments. So let's either comment all
unused, or none.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_archive_libarchive.cc:28: uLong
CustomArchiveWrite(void* opaque,
Why is this argument called opaque? Can we rename it to compressor?

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_archive_libarchive.cc:50:
CompressorArchiveLibarchive* compressor_libarchive =
Can we rename libarchive from variable/class/method names since we don't use
libarchive anymore?

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_archive_libarchive.cc:84: // it is
deleted elsewhere, so we don't need to delete it here.
nit: elsewhere -> ...
Can we write a method name instead?

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_archive_libarchive.cc:124: zip_file_
= zipOpen2(NULL /* pathname */,
NULL -> nullptr here and in other places

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_archive_libarchive.cc:135: bool
CompressorArchiveLibarchive::AddToArchive(const std::string& _filename,
nit: We never prefix with _. Can we keep filename here, and later use eg.
normalized_filename, or filename_with_slash or something like that?

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_archive_libarchive.cc:142: filename
+= "/";
Does it work with Windows paths? If not, did it work before? Let's make sure we
don't regress.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
File ui/file_manager/zip_archiver/cpp/compressor_archive_libarchive.h (right):

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_archive_libarchive.h:19: const char
kAddtoArchiveError[] = "Failed to add entry to archive.";
nit: Addto -> AddTo ?

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_archive_libarchive.h:35: } //
compressor_archive_functions
nit: \s\s before //

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_archive_libarchive.h:37: class
CompressorArchiveLibarchive : public CompressorArchive {
nit: Libarchive -> MiniZip?

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
File ui/file_manager/zip_archiver/cpp/compressor_io_javascript_stream.cc
(right):

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_io_javascript_stream.cc:66: const
void* zip_buffer) {
nit: Do we need const void* here? Can we have const char* and avoid cast?

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_io_javascript_stream.cc:93: ) {
nit: Please check the indent.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_io_javascript_stream.cc:99: int64_t
copy_length = std::min(
nit: Please add some comments, as the code below is not obvious.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
File ui/file_manager/zip_archiver/cpp/compressor_io_javascript_stream.h (right):

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_io_javascript_stream.h:23: const
int64_t kMaximumDataChunkSize = 512 * 1024;
nit: AFAIK initializations should be in .cc file.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
File ui/file_manager/zip_archiver/cpp/volume.cc (right):

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume.cc:364: int return_value =
volume_archive_->GoToNextFile();
Please check performance of opening large files. I usually try linux hernel
here:

https://github.com/torvalds/linux/archive/master.zip

How does it compare with libarchive?

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
File ui/file_manager/zip_archiver/cpp/volume_archive.h (right):

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_archive.h:46: // Seeks to the |index|-th
header.
nit: Please update comment.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
File ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.cc (right):

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.cc:24: int64_t
DynamicCache(VolumeArchiveLibarchive* va, int64_t unz_size) {
nit: Can we chose a better name? What does this method do?

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.cc:24: int64_t
DynamicCache(VolumeArchiveLibarchive* va, int64_t unz_size) {
nit: What is unz in unz_size? Can we find a better argument name?

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.cc:65:
VolumeArchiveLibarchive* va = static_cast<VolumeArchiveLibarchive*>(opaque);
nit: Our style guide forbids variable name abbreviations. How about simply
archive?

nit: Librchive -> Minizip/MiniZip/minizip?

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.cc:70: // Relative
offset in the central directory.
This may only work for small archives. Please check.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.cc:80: char*
unz_buffer_pointer = static_cast<char*>(buffer);
nit: unz -> ...

While existing method naming eg. in minizip are unfortunate, for new code let's
avoid abbreviations.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.cc:170: // Set up
minizip obejct.
typo: object

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.cc:181: // Load
muximum static_cache_size_ bytes from the end of the archive to
typo: maximum

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.cc:239: //
OpenCurrentEntryInZip
Is the comment correct?

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.cc:309: char
raw_file_name_in_zip[volume_archive_constants::kZipMaxPath] = {};
nit: Is the max path size matching ZIP format specs?

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
File ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.h (right):

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.h:49: }  // namespace
volume_archive_constants
nit: \n missing before } for consistency with \n after namespace {

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.h:53: // A name space
with custom functions passed to minizip.
nit: name space -> namespace

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.h:58: long
CustomArchiveTell(void* opaque, void* /*stream*/);
nit: Can we have an empty line between functions? It's much easier to read.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.h:64: } //
compressor_archive_functions
nit: \s\s before //

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.h:107: void* opaque,
void* /*stream*/, void* buf, uLong size);
nit: No need for commenting out arguments in header files. We only do it in cc
files, when body of a function doesn't use them, as some linters warn about
unused variables.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.h:142: // in this
buffer is also used when the data in the central directory is
Is size of this cache enough for large ZIP files, like the mentioned earlier
linux kernel? Can we reload it once reaching the end of the cache? Then it's
also dynamic but going in the other direction, so maybe we could change the
naming.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
File ui/file_manager/zip_archiver/cpp/volume_reader_javascript_stream.h (right):

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_reader_javascript_stream.h:72: virtual
int64_t offset();
nit: Why is this moved to .cc?

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
File ui/file_manager/zip_archiver/js/app.js (right):

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/js/app.js:33: * Since .nmf file is not available in
.grd, we use .txt instead.
This sounds like a workaround. Is there a bug filed for it? If so, please add a
reference to the bug here.

[takise, 2017-04-10 07:57:24]

On 2017/04/10 06:16:28, mtomasz wrote:
> Could you update tests?

Since it was last week we made sure we can go with MiniZip and finished
implementation, I haven't been prepared tests yet.
I still have one week, so I guess it may not impossible to write tests from now.
The problem is that we need to use different test tools in the previous
repository and in this chromium source tree.
So, we need to rewrite most of the test code.
Another problem is that the test code need to be placed ui/file_manager.
This means that we need to ask someone outside this team for a huge code review,
which will take a long time.
Considering those reasons, we are planning to leave the tests for future work of
this project.

[mtomasz, 2017-04-10 08:13:07]

On 2017/04/10 07:57:24, takise wrote:
> On 2017/04/10 06:16:28, mtomasz wrote:
> > Could you update tests?
> 
> Since it was last week we made sure we can go with MiniZip and finished
> implementation, I haven't been prepared tests yet.
> I still have one week, so I guess it may not impossible to write tests from
now.
> The problem is that we need to use different test tools in the previous
> repository and in this chromium source tree.
> So, we need to rewrite most of the test code.
> Another problem is that the test code need to be placed ui/file_manager.
> This means that we need to ask someone outside this team for a huge code
review,
> which will take a long time.
> Considering those reasons, we are planning to leave the tests for future work
of
> this project.

C++ tests use gtest, so the same as Chromium. Updating them shouldn't be a lot
of work. However, JS tests may require more time.

I understand that there is no much time left. If you find time to migrate at
least basic tests, it would be great. I could dedicate entire day for doing fast
reviews for it.

We can land this without tests, but not sure if we can ship it. Please confirm
with @yawano that it's alright (unless confirmed already).

[takise, 2017-04-11 06:00:52]

Please take a look.
I explained about the background of our cache algorithms,
so please read them.

Also, this patch already has lots of changes, so I'm going to rename
libarchive -> minizip in another patch.

Thanks for the review!

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
File ui/file_manager/zip_archiver/cpp/compressor_archive.h (right):

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_archive.h:24: // Releases all
resources obtained by libarchive.
On 2017/04/10 07:15:08, mtomasz wrote:
> nit: libarchive -> minizip

Done.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
File ui/file_manager/zip_archiver/cpp/compressor_archive_libarchive.cc (right):

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_archive_libarchive.cc:14: // Called
when minizip tries to open a zip archive file. We do nothing here
On 2017/04/10 07:15:08, mtomasz wrote:
> nit: Let's add \n after the namespace line for better readability.

Done.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_archive_libarchive.cc:21: uLong
CustomArchiveRead(void* opaque, void* /*stream*/, void* buf, uLong size) {
On 2017/04/10 07:15:08, mtomasz wrote:
> nit: We usually comment out unused arguments. So let's either comment all
> unused, or none.

Done.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_archive_libarchive.cc:28: uLong
CustomArchiveWrite(void* opaque,
On 2017/04/10 07:15:08, mtomasz wrote:
> Why is this argument called opaque? Can we rename it to compressor?

Done.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_archive_libarchive.cc:84: // it is
deleted elsewhere, so we don't need to delete it here.
On 2017/04/10 07:15:08, mtomasz wrote:
> nit: elsewhere -> ...
> Can we write a method name instead?

Done.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_archive_libarchive.cc:124: zip_file_
= zipOpen2(NULL /* pathname */,
On 2017/04/10 07:15:08, mtomasz wrote:
> NULL -> nullptr here and in other places

Done.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_archive_libarchive.cc:135: bool
CompressorArchiveLibarchive::AddToArchive(const std::string& _filename,
On 2017/04/10 07:15:08, mtomasz wrote:
> nit: We never prefix with _. Can we keep filename here, and later use eg.
> normalized_filename, or filename_with_slash or something like that?

Done.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
File ui/file_manager/zip_archiver/cpp/compressor_archive_libarchive.h (right):

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_archive_libarchive.h:19: const char
kAddtoArchiveError[] = "Failed to add entry to archive.";
On 2017/04/10 07:15:08, mtomasz wrote:
> nit: Addto -> AddTo ?

Done.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_archive_libarchive.h:35: } //
compressor_archive_functions
On 2017/04/10 07:15:08, mtomasz wrote:
> nit: \s\s before //

Done.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
File ui/file_manager/zip_archiver/cpp/compressor_io_javascript_stream.cc
(right):

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_io_javascript_stream.cc:66: const
void* zip_buffer) {
On 2017/04/10 07:15:09, mtomasz wrote:
> nit: Do we need const void* here? Can we have const char* and avoid cast?

Done.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_io_javascript_stream.cc:93: ) {
On 2017/04/10 07:15:09, mtomasz wrote:
> nit: Please check the indent.

Done.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_io_javascript_stream.cc:99: int64_t
copy_length = std::min(
On 2017/04/10 07:15:08, mtomasz wrote:
> nit: Please add some comments, as the code below is not obvious.

Done.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
File ui/file_manager/zip_archiver/cpp/compressor_io_javascript_stream.h (right):

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_io_javascript_stream.h:23: const
int64_t kMaximumDataChunkSize = 512 * 1024;
On 2017/04/10 07:15:09, mtomasz wrote:
> nit: AFAIK initializations should be in .cc file.

I want to place this constant here because this value is used in several .cc
files.
You mean we should just declare this variable here and assign the value in .cc?

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
File ui/file_manager/zip_archiver/cpp/volume.cc (right):

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume.cc:364: int return_value =
volume_archive_->GoToNextFile();
On 2017/04/10 07:15:09, mtomasz wrote:
> Please check performance of opening large files. I usually try linux hernel
> here:
> 
> https://github.com/torvalds/linux/archive/master.zip
> 
> How does it compare with libarchive?

I compared the time to open
https://github.com/torvalds/linux/archive/master.zip. 
libarchive takes about 8~10sec, while minizip takes about 20 sec.

I'm not sure this regression is endurable.

I don't think we can make this operation faster with minizip anymore.
I investigated how minizip requests data carefully and optimize it as much as we
can so that it runs efficiently even for large zip files.
(Please look at my comment in volume_archive_libarchive.h to see how it works.)

wdyt?

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
File ui/file_manager/zip_archiver/cpp/volume_archive.h (right):

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_archive.h:46: // Seeks to the |index|-th
header.
On 2017/04/10 07:15:09, mtomasz wrote:
> nit: Please update comment.

Done.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
File ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.cc (right):

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.cc:24: int64_t
DynamicCache(VolumeArchiveLibarchive* va, int64_t unz_size) {
On 2017/04/10 07:15:09, mtomasz wrote:
> nit: Can we chose a better name? What does this method do?

As mentioned in volume_archive_libarchive.h, this buffer is generally used to
read data except for
reading the data around the end of the file(because we have another cache to
read around the end).
What do you think is a better name?

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.cc:65:
VolumeArchiveLibarchive* va = static_cast<VolumeArchiveLibarchive*>(opaque);
On 2017/04/10 07:15:09, mtomasz wrote:
> nit: Our style guide forbids variable name abbreviations. How about simply
> archive?
> 
> nit: Librchive -> Minizip/MiniZip/minizip?

Done.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.cc:70: // Relative
offset in the central directory.
On 2017/04/10 07:15:09, mtomasz wrote:
> This may only work for small archives. Please check.

Yes, this is only work for small archives, but the main reason to use
static_cache is not to read the central directory, but to read the end of
central directory as I explained in volume_archive_libarchive.h.
It's just a bonus if we can use this cache here.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.cc:80: char*
unz_buffer_pointer = static_cast<char*>(buffer);
On 2017/04/10 07:15:09, mtomasz wrote:
> nit: unz -> ...
> 
> While existing method naming eg. in minizip are unfortunate, for new code
let's
> avoid abbreviations.

Done.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.cc:170: // Set up
minizip obejct.
On 2017/04/10 07:15:09, mtomasz wrote:
> typo: object

Done.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.cc:181: // Load
muximum static_cache_size_ bytes from the end of the archive to
On 2017/04/10 07:15:09, mtomasz wrote:
> typo: maximum

Done.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.cc:239: //
OpenCurrentEntryInZip
On 2017/04/10 07:15:09, mtomasz wrote:
> Is the comment correct?

Done.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.cc:309: char
raw_file_name_in_zip[volume_archive_constants::kZipMaxPath] = {};
On 2017/04/10 07:15:09, mtomasz wrote:
> nit: Is the max path size matching ZIP format specs?

Yes, I checked the ZIP specification.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
File ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.h (right):

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.h:49: }  // namespace
volume_archive_constants
On 2017/04/10 07:15:09, mtomasz wrote:
> nit: \n missing before } for consistency with \n after namespace {

Done.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.h:53: // A name space
with custom functions passed to minizip.
On 2017/04/10 07:15:09, mtomasz wrote:
> nit: name space -> namespace

Done.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.h:58: long
CustomArchiveTell(void* opaque, void* /*stream*/);
On 2017/04/10 07:15:09, mtomasz wrote:
> nit: Can we have an empty line between functions? It's much easier to read.

Done.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.h:64: } //
compressor_archive_functions
On 2017/04/10 07:15:09, mtomasz wrote:
> nit: \s\s before //

Done.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.h:107: void* opaque,
void* /*stream*/, void* buf, uLong size);
On 2017/04/10 07:15:09, mtomasz wrote:
> nit: No need for commenting out arguments in header files. We only do it in cc
> files, when body of a function doesn't use them, as some linters warn about
> unused variables.

Done.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.h:142: // in this
buffer is also used when the data in the central directory is
On 2017/04/10 07:15:09, mtomasz wrote:
> Is size of this cache enough for large ZIP files, like the mentioned earlier
> linux kernel? Can we reload it once reaching the end of the cache? Then it's
> also dynamic but going in the other direction, so maybe we could change the
> naming.

Actually, this buffer doesn't have 'central directory', which contains the
matadata
of all the files, but have 'end of central directory header', which follows
'central directory'.
Minizip wants to read the metadata in central directory, but to read the central
directory,
minizip needs to know where central directory starts.
'end of central directory header' holds the first index of 'central directory'.
To find the first index of 'central directory', minizip first tries to look for
the first index of 'end of central directory header'.
The problem is that the size of 'end of central directory header' changes from
archive to archive
because 'end of central directory header' has an field called global comment,
which can be maximum 65536 bytes.
That's why minizip first requests data little by little (256bytes) backwards
from the end to find the first index
of 'end of central directory header'.
It is guaranteed that 'global comment' field has maximum 65536 bytes, so 128 *
1024 bytes are enough
to have whole 'end of central directory header' regardless of the size of the
archive.

Another problem is that minizip assumes zip files are in ZIP64 format and if it
fails,
then minizip considers archives to be in ZIP format. (although most of the zip
files are ZIP format. )
More specifically, Minizip tries to read data little by little backwards to find
the signature of end of central directory in ZIP64 format.
And if it fails, minizip read the same data again little by little to find the
signature in ZIP format.
This is the reason why we use static cache.

Please refer to the function LoadCentralDirectoryRecord() in minizip to see how
it works.
https://cs.chromium.org/chromium/src/third_party/zlib/contrib/minizip/zip.c?s...

We never reload this buffer again. That's why I call this buffer static.
But, I agree there is better name for this. end_of_archive_buffer or something?

I decided to implement the cache like this after I had a few discussions with
satoru-san and yawano-san.
I'm sorry not to sharing the context with you well. Please ask me if the
explanation above doesn't make sense to me.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
File ui/file_manager/zip_archiver/cpp/volume_reader_javascript_stream.h (right):

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_reader_javascript_stream.h:72: virtual
int64_t offset();
On 2017/04/10 07:15:09, mtomasz wrote:
> nit: Why is this moved to .cc?

Done.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
File ui/file_manager/zip_archiver/js/app.js (right):

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/js/app.js:33: * Since .nmf file is not available in
.grd, we use .txt instead.
On 2017/04/10 07:15:10, mtomasz wrote:
> This sounds like a workaround. Is there a bug filed for it? If so, please add
a
> reference to the bug here.

This is the way ImageLoader loads .pexe file. I read the code about this in
ImageLoader, but no bug is mentioned.

[mtomasz, 2017-04-11 06:36:45]

LGTM with nits

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
File ui/file_manager/zip_archiver/cpp/volume.cc (right):

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume.cc:364: int return_value =
volume_archive_->GoToNextFile();
On 2017/04/11 06:00:51, takise wrote:
> On 2017/04/10 07:15:09, mtomasz wrote:
> > Please check performance of opening large files. I usually try linux hernel
> > here:
> > 
> > https://github.com/torvalds/linux/archive/master.zip
> > 
> > How does it compare with libarchive?
> 
> I compared the time to open
> https://github.com/torvalds/linux/archive/master.zip. 
> libarchive takes about 8~10sec, while minizip takes about 20 sec.
> 
> I'm not sure this regression is endurable.
> 
> I don't think we can make this operation faster with minizip anymore.
> I investigated how minizip requests data carefully and optimize it as much as
we
> can so that it runs efficiently even for large zip files.
> (Please look at my comment in volume_archive_libarchive.h to see how it
works.)
> 
> wdyt?

Thanks for checking. 2 times slower on such a large archive is not terrible. I'm
curious why it's slower though. Maybe the entire central directory doesn't fit
in static cache? Is there any easy way to check it?


Note, that we don't have to fix it in this CL.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
File ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.h (right):

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/volume_archive_libarchive.h:142: // in this
buffer is also used when the data in the central directory is
On 2017/04/11 06:00:52, takise wrote:
> On 2017/04/10 07:15:09, mtomasz wrote:
> > Is size of this cache enough for large ZIP files, like the mentioned earlier
> > linux kernel? Can we reload it once reaching the end of the cache? Then it's
> > also dynamic but going in the other direction, so maybe we could change the
> > naming.
> 
> Actually, this buffer doesn't have 'central directory', which contains the
> matadata
> of all the files, but have 'end of central directory header', which follows
> 'central directory'.
> Minizip wants to read the metadata in central directory, but to read the
central
> directory,
> minizip needs to know where central directory starts.
> 'end of central directory header' holds the first index of 'central
directory'.
> To find the first index of 'central directory', minizip first tries to look
for
> the first index of 'end of central directory header'.
> The problem is that the size of 'end of central directory header' changes from
> archive to archive
> because 'end of central directory header' has an field called global comment,
> which can be maximum 65536 bytes.
> That's why minizip first requests data little by little (256bytes) backwards
> from the end to find the first index
> of 'end of central directory header'.
> It is guaranteed that 'global comment' field has maximum 65536 bytes, so 128 *
> 1024 bytes are enough
> to have whole 'end of central directory header' regardless of the size of the
> archive.
> 
> Another problem is that minizip assumes zip files are in ZIP64 format and if
it
> fails,
> then minizip considers archives to be in ZIP format. (although most of the zip
> files are ZIP format. )
> More specifically, Minizip tries to read data little by little backwards to
find
> the signature of end of central directory in ZIP64 format.
> And if it fails, minizip read the same data again little by little to find the
> signature in ZIP format.
> This is the reason why we use static cache.
> 
> Please refer to the function LoadCentralDirectoryRecord() in minizip to see
how
> it works.
>
https://cs.chromium.org/chromium/src/third_party/zlib/contrib/minizip/zip.c?s...
> 
> We never reload this buffer again. That's why I call this buffer static.
> But, I agree there is better name for this. end_of_archive_buffer or
something?
> 
> I decided to implement the cache like this after I had a few discussions with
> satoru-san and yawano-san.
> I'm sorry not to sharing the context with you well. Please ask me if the
> explanation above doesn't make sense to me.

Thanks for the explanation. Now I understand very well. Please add some short
comment around the buffer size constant, saying something like "The EOCD (End Of
Central Directory) header is guaranteed to be in the last 64KB of the file, so
cache last 64KB.

Or something like that.

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
File ui/file_manager/zip_archiver/js/app.js (right):

https://codereview.chromium.org/2807063002/diff/20001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/js/app.js:33: * Since .nmf file is not available in
.grd, we use .txt instead.
On 2017/04/11 06:00:52, takise wrote:
> On 2017/04/10 07:15:10, mtomasz wrote:
> > This sounds like a workaround. Is there a bug filed for it? If so, please
add
> a
> > reference to the bug here.
> 
> This is the way ImageLoader loads .pexe file. I read the code about this in
> ImageLoader, but no bug is mentioned.

Got it. If we already do that, then let's keep it as it is.

https://codereview.chromium.org/2807063002/diff/40001/ui/file_manager/zip_arc...
File ui/file_manager/zip_archiver/cpp/compressor_archive_libarchive.cc (right):

https://codereview.chromium.org/2807063002/diff/40001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_archive_libarchive.cc:24: void*
/*buffuer*/, uLong /*size*/) {
typo: buffer

https://codereview.chromium.org/2807063002/diff/40001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_archive_libarchive.cc:84: return -1
/* Error */;
nit: We could introduce an enum and avoid these comments all the time for each
return. No need to fix it in this CL.

https://codereview.chromium.org/2807063002/diff/40001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_archive_libarchive.cc:172: nullptr, 
       // extrafield_global
nit: Please fix indentation of comments.

https://codereview.chromium.org/2807063002/diff/40001/ui/file_manager/zip_arc...
File ui/file_manager/zip_archiver/cpp/compressor_io_javascript_stream.cc
(right):

https://codereview.chromium.org/2807063002/diff/40001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_io_javascript_stream.cc:99: Flush();
Do we need to check for the return value of Flush?

https://codereview.chromium.org/2807063002/diff/40001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_io_javascript_stream.cc:114: //
Update variables.
nit: This comment has no meaning, let's remove.

https://codereview.chromium.org/2807063002/diff/40001/ui/file_manager/zip_arc...
File ui/file_manager/zip_archiver/cpp/compressor_stream.h (right):

https://codereview.chromium.org/2807063002/diff/40001/ui/file_manager/zip_arc...
ui/file_manager/zip_archiver/cpp/compressor_stream.h:18: virtual int64_t Flush()
= 0;
Please add a comment for this method.

[takise, 2017-04-11 07:14:41]

The CQ bit was checked by takise@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 07:14:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 07:33:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 07:33:43]

Dry run: This issue passed the CQ dry run.

[takise, 2017-04-11 07:34:29]

The CQ bit was checked by takise@google.com

[takise, 2017-04-11 07:34:29]

The patchset sent to the CQ was uploaded after l-g-t-m from mtomasz@chromium.org
Link to the patchset: https://codereview.chromium.org/2807063002/#ps60001
(title: "Fix a few nits.")

[commit-bot: I haz the power, 2017-04-11 07:34:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 07:39:22]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491896069122550,
"parent_rev": "ce0624a942214db2dea8fcb25d2585a63e9d3da0", "commit_rev":
"cac7340db735b9e8228e8b5abf886b20719db262"}

[commit-bot: I haz the power, 2017-04-11 07:41:04]

Description was changed from

==========
Replace Libarchive with MiniZip.

This CL rewrites all the components that depend on libarchive. Since we
plan to add the files related to build such as BUILD.gn in another CL,
GN doesn't compile the code in this CL yet.

BUG=607078
TEST=manually tested
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Replace Libarchive with MiniZip.

This CL rewrites all the components that depend on libarchive. Since we
plan to add the files related to build such as BUILD.gn in another CL,
GN doesn't compile the code in this CL yet.

BUG=607078
TEST=manually tested
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2807063002
Cr-Commit-Position: refs/heads/master@{#463567}
Committed:
https://chromium.googlesource.com/chromium/src/+/cac7340db735b9e8228e8b5abf88...
==========

[commit-bot: I haz the power, 2017-04-11 07:41:05]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/cac7340db735b9e8228e8b5abf88...