Move components/zip to third_party/zip

Move components/zip to third_party/zip

Move components to zip as per the discussion here:
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/MgbMTQCNzR0/discussion

BUG=
R=agl@chromium.org, gavinp@chromium.org, jam@chromium.org, joi@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=197964

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198222

[alecflett, 2013-04-26 22:07:54]

And here is the patch. it has a few open issues, so I wanted to get feedback
before going any further

1) within third_party/zip, should I be #including through third_party/zip? If
this were a 3rd party library I think we'd be saying "no" but since this is kind
of half-third-party and half-not, I wasn't sure

2) DEPS - I intentionally did not change chrome/DEPS and instead changed one
subdirectory below instead. Not sure if that's the right thing or not.

3) unit tests are not in a .gyp yet, I wasn't sure if I should be making a
separate zip_unittests.gyp or if it was fine to put it in zip.gyp

[Jói, 2013-04-26 22:57:27]

LGTM with nits

For #1, I would say includes should be third_party/zip/...

#2 the way you did DEPS under //chrome looks good.

#3 seems fine to put the unit tests in zyp.gyp, but I'm not sure if we want a
separate unit test binary for third_party or what... for now you might want to
compile the unit tests as part of some existing unit test binary.

Cheers,
Jói

https://codereview.chromium.org/14021015/diff/1/third_party/zip/README.chromium
File third_party/zip/README.chromium (right):

https://codereview.chromium.org/14021015/diff/1/third_party/zip/README.chromi...
third_party/zip/README.chromium:1: Name: zip library for chromium
Should there be a file like this, given that it's not third party code?  Perhaps
README.chromium should just say "Not 3rd party code, but a wrapper around
'minizip' using base/ and net/ to read and write files and streams."

[Jói, 2013-04-26 22:58:51]

Forgot to say: I'm going to be away on vacation and local holidays for
the next 5 days, returning to the office Thursday next week.

Cheers,
Jói


On Fri, Apr 26, 2013 at 10:57 PM,  <joi@chromium.org> wrote:
> LGTM with nits
>
> For #1, I would say includes should be third_party/zip/...
>
> #2 the way you did DEPS under //chrome looks good.
>
> #3 seems fine to put the unit tests in zyp.gyp, but I'm not sure if we want
> a
> separate unit test binary for third_party or what... for now you might want
> to
> compile the unit tests as part of some existing unit test binary.
>
> Cheers,
> Jói
>
>
>
>
https://codereview.chromium.org/14021015/diff/1/third_party/zip/README.chromium
> File third_party/zip/README.chromium (right):
>
>
https://codereview.chromium.org/14021015/diff/1/third_party/zip/README.chromi...
> third_party/zip/README.chromium:1: Name: zip library for chromium
> Should there be a file like this, given that it's not third party code?
> Perhaps README.chromium should just say "Not 3rd party code, but a
> wrapper around 'minizip' using base/ and net/ to read and write files
> and streams."
>
> https://codereview.chromium.org/14021015/

[alecflett, 2013-04-26 23:31:31]

The resubmit checks gave specific instructions, not sure if this format is
simply required for README.chromium

[jam, 2013-04-29 16:22:51]

for the unittest, just make chrome's unit_tests target include the file from
third_party

https://codereview.chromium.org/14021015/diff/1/chrome/common/DEPS
File chrome/common/DEPS (right):

https://codereview.chromium.org/14021015/diff/1/chrome/common/DEPS#newcode34
chrome/common/DEPS:34: "+third_party/zip",
since you need this in chrome/browser, chrome/common, chrome/test and
chrome/utility, just put it in src/chrome/DEPS

https://codereview.chromium.org/14021015/diff/1/third_party/zip/DEPS
File third_party/zip/DEPS (right):

https://codereview.chromium.org/14021015/diff/1/third_party/zip/DEPS#newcode3
third_party/zip/DEPS:3: "-content",
nit: this line isn't needed, since by default a directory doesn't have +content

https://codereview.chromium.org/14021015/diff/1/third_party/zip/zip.cc
File third_party/zip/zip.cc (right):

https://codereview.chromium.org/14021015/diff/1/third_party/zip/zip.cc#newcode13
third_party/zip/zip.cc:13: #include "zip_internal.h"
nit: agree with Joi, these need to be third_party/zip-zip_internal.h

https://codereview.chromium.org/14021015/diff/1/third_party/zip/zip.h
File third_party/zip/zip.h (right):

https://codereview.chromium.org/14021015/diff/1/third_party/zip/zip.h#newcode5
third_party/zip/zip.h:5: #ifndef COMPONENTS_ZIP_ZIP_H_
nit: update

https://codereview.chromium.org/14021015/diff/1/third_party/zip/zip_internal.h
File third_party/zip/zip_internal.h (right):

https://codereview.chromium.org/14021015/diff/1/third_party/zip/zip_internal....
third_party/zip/zip_internal.h:5: #ifndef COMPONENTS_ZIP_ZIP_INTERNAL_H_
ditto

https://codereview.chromium.org/14021015/diff/1/third_party/zip/zip_reader.h
File third_party/zip/zip_reader.h (right):

https://codereview.chromium.org/14021015/diff/1/third_party/zip/zip_reader.h#...
third_party/zip/zip_reader.h:5: #ifndef COMPONENTS_ZIP_ZIP_READER_H_
ditto

[alecflett, 2013-04-29 17:13:19]

All comments addressed, ready to go.

https://chromiumcodereview.appspot.com/14021015/diff/1/chrome/common/DEPS
File chrome/common/DEPS (right):

https://chromiumcodereview.appspot.com/14021015/diff/1/chrome/common/DEPS#new...
chrome/common/DEPS:34: "+third_party/zip",
On 2013/04/29 16:22:51, jam wrote:
> since you need this in chrome/browser, chrome/common, chrome/test and
> chrome/utility, just put it in src/chrome/DEPS

Done.

https://chromiumcodereview.appspot.com/14021015/diff/1/third_party/zip/DEPS
File third_party/zip/DEPS (right):

https://chromiumcodereview.appspot.com/14021015/diff/1/third_party/zip/DEPS#n...
third_party/zip/DEPS:3: "-content",
On 2013/04/29 16:22:51, jam wrote:
> nit: this line isn't needed, since by default a directory doesn't have
+content

Done.

https://chromiumcodereview.appspot.com/14021015/diff/1/third_party/zip/README...
File third_party/zip/README.chromium (right):

https://chromiumcodereview.appspot.com/14021015/diff/1/third_party/zip/README...
third_party/zip/README.chromium:1: Name: zip library for chromium
On 2013/04/26 22:57:27, Jói wrote:
> Should there be a file like this, given that it's not third party code? 
Perhaps
> README.chromium should just say "Not 3rd party code, but a wrapper around
> 'minizip' using base/ and net/ to read and write files and streams."

This is required for presubmit checks but I've clarified the description a bit.

https://chromiumcodereview.appspot.com/14021015/diff/1/third_party/zip/zip.cc
File third_party/zip/zip.cc (right):

https://chromiumcodereview.appspot.com/14021015/diff/1/third_party/zip/zip.cc...
third_party/zip/zip.cc:13: #include "zip_internal.h"
On 2013/04/29 16:22:51, jam wrote:
> nit: agree with Joi, these need to be third_party/zip-zip_internal.h

Done.

https://chromiumcodereview.appspot.com/14021015/diff/1/third_party/zip/zip.h
File third_party/zip/zip.h (right):

https://chromiumcodereview.appspot.com/14021015/diff/1/third_party/zip/zip.h#...
third_party/zip/zip.h:5: #ifndef COMPONENTS_ZIP_ZIP_H_
On 2013/04/29 16:22:51, jam wrote:
> nit: update

Done.

https://chromiumcodereview.appspot.com/14021015/diff/1/third_party/zip/zip_in...
File third_party/zip/zip_internal.h (right):

https://chromiumcodereview.appspot.com/14021015/diff/1/third_party/zip/zip_in...
third_party/zip/zip_internal.h:5: #ifndef COMPONENTS_ZIP_ZIP_INTERNAL_H_
On 2013/04/29 16:22:51, jam wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/14021015/diff/1/third_party/zip/zip_re...
File third_party/zip/zip_reader.h (right):

https://chromiumcodereview.appspot.com/14021015/diff/1/third_party/zip/zip_re...
third_party/zip/zip_reader.h:5: #ifndef COMPONENTS_ZIP_ZIP_READER_H_
On 2013/04/29 16:22:51, jam wrote:
> ditto

Done.

[jam, 2013-04-29 19:43:00]

I don't see unit_test target changes?

[alecflett, 2013-04-29 22:15:55]

On 2013/04/29 19:43:00, jam wrote:
> I don't see unit_test target changes?

Sorry, I completely flaked on the unit_tests stuff! Here they are, along with
test data.

[jam, 2013-04-29 23:27:47]

sorry I just noticed that this is a new directory in third_party. i had assumed
it would be in the same directory as zlib, with perhaps subdirs for google and
for zlib code. it seems strange to add a third_party directory that's just
completely our code.

[alecflett, 2013-04-29 23:46:56]

On 2013/04/29 23:27:47, jam wrote:
> sorry I just noticed that this is a new directory in third_party. i had
assumed
> it would be in the same directory as zlib, with perhaps subdirs for google and
> for zlib code. it seems strange to add a third_party directory that's just
> completely our code.

I'll reiterate what I said on the mailing list:

- third_party/zlib is the *compression* library  - it is just for compressing
streams, a la gzip

- third_party/zlib/contrib/minizip is the *zipping* library - i.e. put one or
more in a container with a directory listing, optionally compressing them (like
tar) - it uses zlib to compress individual files. (And technically minizip is
not part of zlib, they are separate open source projects!)

- zip is the chromium library that depends on 'base' and 'net' and uses them to
create actual files on disk - i.e. the *glue* between chromium and minizip to do
I/O in a chrome-safe way.

Some guidance here. I don't care where this lives except it takes me an 1-2
hours every time I move it (to get gyp files aligned, unit tests working, etc)

Just tell me where it goes.

[jam, 2013-04-30 15:08:19]

apologies for the back and forth, I realize this has taken a while.

ok here are two options that I can see:

the files that you're moving go to third_party\zlib\google. it seems that
there's already precedent for this, i.e. a number of directories in third_party
have a "google" subdir.


third_party\zip:
-directory contains the files you're moving
-minizip subdir contains what's now in third_party/zlib/contrib/minizip
-zlib subidr contains what's now in third_party/zlib/

[alecflett, 2013-04-30 15:11:36]

I'll go with zlib/google/

[alecflett, 2013-05-01 19:53:40]

ok, now it is in third_party/zlib/google

I also removed README.chromium, now that we're already in a 3rd party directory
with an existing README.chromium

[jam, 2013-05-01 22:01:08]

lgtm

https://codereview.chromium.org/14021015/diff/29001/third_party/zlib/google/DEPS
File third_party/zlib/google/DEPS (right):

https://codereview.chromium.org/14021015/diff/29001/third_party/zlib/google/D...
third_party/zlib/google/DEPS:3: "+third_party/zlib/contrib/minizip",
nit: are you sure you need this? i thought it would automatically be allowed to
include anything in its parent directory

https://codereview.chromium.org/14021015/diff/29001/third_party/zlib/google/z...
File third_party/zlib/google/zip.h (right):

https://codereview.chromium.org/14021015/diff/29001/third_party/zlib/google/z...
third_party/zlib/google/zip.h:5: #ifndef THIRD_PARTY_ZIP_ZIP_H_
nit: update, also all the other headers

[commit-bot: I haz the power, 2013-05-01 22:06:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/14021015/35001

[commit-bot: I haz the power, 2013-05-01 22:06:47]

Can't process patch for file
third_party/zlib/google/test/data/test_nocompress.zip.
Binary file support is temporarilly disabled due to a bug. Please commit blindly
the binary files first then commit the source change as a separate CL. Sorry for
the annoyance.

[alecflett, 2013-05-01 23:36:56]

oops, need an owners review for zlib. gavinp or agl?

[gavinp, 2013-05-02 07:57:10]

lgtm

[commit-bot: I haz the power, 2013-05-02 08:00:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/14021015/35001

[commit-bot: I haz the power, 2013-05-02 08:00:57]

Can't process patch for file
third_party/zlib/google/test/data/test_nocompress.zip.
Binary file support is temporarilly disabled due to a bug. Please commit blindly
the binary files first then commit the source change as a separate CL. Sorry for
the annoyance.

[agl, 2013-05-02 14:46:11]

not LGTM.

What's a third_party/zlib/google directory? Is that like a double negative? Is
the code third_party or not? This seems like the wrong place.

[agl, 2013-05-02 15:03:55]

gavinp points out jam's comment on this matter above. Still seems odd, but LGTM.

[alecflett, 2013-05-02 20:15:07]

Committed patchset #7 manually as r197964 (presubmit successful).

[commit-bot: I haz the power, 2013-05-02 22:17:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/14021015/44001

[commit-bot: I haz the power, 2013-05-02 22:47:25]

Retried try job too often on ios_dbg_simulator for step(s) base_unittests,
compile, content_unittests, crypto_unittests, googleurl_unittests,
media_unittests, net_unittests, sql_unittests, ui_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-05-02 23:33:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/14021015/44001

[commit-bot: I haz the power, 2013-05-03 00:01:54]

Retried try job too often on ios_rel_device for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[commit-bot: I haz the power, 2013-05-03 00:43:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/14021015/44001

[commit-bot: I haz the power, 2013-05-03 01:13:44]

Retried try job too often on ios_rel_device for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[commit-bot: I haz the power, 2013-05-03 03:57:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/14021015/44001

[commit-bot: I haz the power, 2013-05-03 04:11:11]

Retried try job too often on ios_rel_device for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[commit-bot: I haz the power, 2013-05-03 05:44:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/14021015/44001

[commit-bot: I haz the power, 2013-05-03 06:14:43]

Retried try job too often on linux_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-05-03 16:57:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/14021015/44001

[commit-bot: I haz the power, 2013-05-03 17:12:13]

Retried try job too often on ios_rel_device for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[commit-bot: I haz the power, 2013-05-03 17:37:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/14021015/93002

[commit-bot: I haz the power, 2013-05-03 18:15:38]

Retried try job too often on ios_dbg_simulator for step(s) base_unittests,
compile, content_unittests, crypto_unittests, googleurl_unittests,
media_unittests, net_unittests, sql_unittests, ui_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-05-03 21:31:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alecflett@chromium.org/14021015/93002

[commit-bot: I haz the power, 2013-05-03 21:44:44]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[alecflett, 2013-05-03 23:03:17]

Committed patchset #9 manually as r198222 (presubmit successful).