Moving components/zip ?

[Alec Flett]

I'd like to introduce a dependency on 'zip' in this CL:

https://codereview.chromium.org/13949013/

For background, this is a tool (chrome://indexeddb-internals) which can download an IDB directory (potentially containing hundreds/thousands of files) from your profile for debugging as a single ZIP file. 

John you appropriately pointed out that content/ shouldn't depend on components/

this seems quite reasonable except that 'zip' seems unique among components/* in that it has no complex dependencies - just zlib and base, both of which are already dependencies of content/

dgrogan found an earlier CL where zip was moved out of chrome/common, and there was some discussion if zip should be in base/ itself:

https://codereview.chromium.org/13257004

but Brett, you objected at the time.

Any thoughts here?

(as an aside: The compression aspect of ZIP is of moderate importance  because one of the main use cases is to download someone's (potentially large) offline google drive database on chromeos, which can be disk-constrained. We could live without the compression if we had to, but I'd like to keep it - but the fact that we need zlib anyway in content/ makes this sort of moot)

Alec

[Jói Sigurðsson]

If you need to use zip from //content then certainly it needs to move
out of //components. Making it an optional library on top of //base
(similar to base_i18n and base_prefs which live in similarly-named
subdirectories //base/i18n and //base/prefs) seems reasonable, but
Brett will need to chime in.

Cheers,
Jói

> --
> --
> Chromium Developers mailing list: chromi...@chromium.org
> View archives, change email options, or unsubscribe:
> http://groups.google.com/a/chromium.org/group/chromium-dev
>
>
>

[Thiago Farina]

On Wed, Apr 17, 2013 at 1:16 PM, Alec Flett <alec...@chromium.org> wrote:
> John you appropriately pointed out that content/ shouldn't depend on
> components/
>

Yup, because //components already depends on //content, and making
content depend on components would create a circular dependency.

Can it be moved to a top-level directory, like sql, dbus, net, etc?

--
Thiago

[John Abd-El-Malek]

(in the future, please avoid asking questions in both a code review and an email thread, I noticed the code review reply first and it's best not to have two threads about a topic)

in other cases, afaik we have put the wrapper code around a third party library in the third_party/foo directory. perhaps that's another option here.

[Paweł Hajdan, Jr.]

I really don't like putting Chromium code in third_party, because that makes it harder to distinguish between bundled libraries that can be unbundled and Chromium code that needs to be preserved.

If the code is needed in content, why not move it to content? John, do you have objections against that?

Alternatively, I think moving to a library on top of base but physically in src/base (like base/i18n or base/prefs) would also be a good option, provided Brett is OK with that.

I'd rather avoid creating another top-level directory for a very small piece of code - and we already have many top-level directories.

Paweł

[John Abd-El-Malek]

On Wed, Apr 17, 2013 at 10:15 AM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

I really don't like putting Chromium code in third_party, because that makes it harder to distinguish between bundled libraries that can be unbundled and Chromium code that needs to be preserved.

If the code is needed in content, why not move it to content? John, do you have objections against that?

src/chromeos uses this as well, and that can't depend on content

even ignoring the above, src/chrome uses it. so if we put it in content, we'd have to wrap these wrappers in the content api. it seems that this stuff doesn't belong in the public content api

[David Grogan]

On Wed, Apr 17, 2013 at 9:16 AM, Alec Flett <alec...@chromium.org> wrote:

I'd like to introduce a dependency on 'zip' in this CL:

https://codereview.chromium.org/13949013/

For background, this is a tool (chrome://indexeddb-internals) which can download an IDB directory (potentially containing hundreds/thousands of files) from your profile for debugging as a single ZIP file. 

John you appropriately pointed out that content/ shouldn't depend on components/

this seems quite reasonable except that 'zip' seems unique among components/* in that it has no complex dependencies - just zlib and base, both of which are already dependencies of content/

dgrogan found an earlier CL where zip was moved out of chrome/common, and there was some discussion if zip should be in base/ itself:

https://codereview.chromium.org/13257004

but Brett, you objected at the time.

base depending on zlib was discussed two years ago[1]. The consensus seemed to be that it was ok. What became of that?

1. https://groups.google.com/a/chromium.org/forum/?fromgroups=#!topic/chromium-dev/KixJAkXnRIA

 

Any thoughts here?

(as an aside: The compression aspect of ZIP is of moderate importance  because one of the main use cases is to download someone's (potentially large) offline google drive database on chromeos, which can be disk-constrained. We could live without the compression if we had to, but I'd like to keep it - but the fact that we need zlib anyway in content/ makes this sort of moot)

Alec

[Alec Flett]

ok so it sounds like we have a few options:

1) Move to a top-level directory - //zip

2) Move to base as an optional module - //base/zip

3) Move to third_party //third_party/zip

More information: 

• components/zip is actually a wrapper around minizip, of which there is the USE_SYSTEM_MINIZIP and third_party/zlib/contrib/minizip. 
• base/ does not directly depend on zlib but generally people seemed ok with that some years ago (thanks to dgrogan for the pointer) https://groups.google.com/a/chromium.org/forum/?fromgroups=#!topic/chromium-dev/KixJAkXnRIA
  
• base/ has a number of other third_party libraries (e.g. ICU, JSON) that seem to be at a similar level of system-library-ness to me

so which option do folks prefer? 

Personally I'd go with #1, but honestly I don't feel that strongly and I'm willing to do the work to make any of them happen.

Alec

On Wed, Apr 17, 2013 at 10:26 AM, Brett Wilson <bre...@google.com> wrote:

If it was just one thing, making a separate base subproject would
probably be fine, but this has come up with some other third party
libraries where people write wrappers around them, and I suspect it
will come up more as people slice and dice the codebase to be more
componentized.

This is where the recommendation to put the code in the third_party
directory came from, since when you depend on the third_party thingy,
you automatically get the nice wrapper code.

But this isn't ideal for the reasons Paweł points out. I feel like we
should have a better system for doing this, something like
standardizing on making directories like "third_party/.../google" for
the wrapper code.

Brett

[Brett Wilson]

I still have my previous concern that this is a common problem and
adding separate base directories won't scale well. The same goes for
toplevel directories. We could add another directory of third_party
wrappers for this kind of thing, but that doesn't seem noticeably
better than having a good scheme for where google code lives in
third_party.

Brett

[John Abd-El-Malek]

I agree that this seems like the best of the solutions. Regarding the earlier messages comparing this to third_party\prefs, I think that's different as prefs doesn't wrap something in third_party.

btw I can't remember, why was prefs put into src\base\prefs instead of src\components?

[Jói Sigurðsson]

> btw I can't remember, why was prefs put into src\base\prefs instead of
> src\components?

One reason is that //components didn't exist at the time, but a more
relevant reason is that it has no dependencies on anything but //base
and so was considered a good candidate to be an optional add-on to
//base similar to base_i18n.

Cheers,
Jói

[John Abd-El-Malek]

I see. It seems base_i18n is different from prefs, as the former is used by other top level directories (quick search shows ui/net/ash/sync/printing/webkit/ppapi all of which seem below the src\components layer).

With the refactorings that will happen to support iOS upstreaming, more code will move to src\components that doesn't depend on content. For consistency, would it make sense to move prefs to src\components now that we have that directory? 

[Jói Sigurðsson]

//components was only supposed to be for stuff that depends on
//content and down. Unfortunately there are a couple of exceptions to
this at the moment, due to there not being a better place for them.
If we want to be able to consider //components a fairly well-defined
layer, then we should stick to the originally intended rule - that way
it's a layer on top of //content.

We could revisit the idea that came up when we initially were
discussing //components, which was to have one components directory
for each architectural layer, where components live that depend on
that layer and below, e.g. //base_components, //net_components,
//content_components. In such a scenario, prefs and zip and maybe
i18n would move to //base_components.

Cheers,
Jói

[John Abd-El-Malek]

On Thu, Apr 18, 2013 at 9:05 AM, Jói Sigurðsson <j...@chromium.org> wrote:

//components was only supposed to be for stuff that depends on
//content and down. Unfortunately there are a couple of exceptions to
this at the moment, due to there not being a better place for them.
If we want to be able to consider //components a fairly well-defined
layer, then we should stick to the originally intended rule - that way
it's a layer on top of //content.

What about reconsidering that stuff in components is only stuff that depends on content?

i.e. what if src\components is the home for all features that can be reused? It seems that if some of them use or don't use content it doesn't take away from the fact that they're reusable, just like some of them may or may not use src/ui or src/net..

We could revisit the idea that came up when we initially were
discussing //components, which was to have one components directory
for each architectural layer, where components live that depend on
that layer and below, e.g. //base_components, //net_components,
//content_components.  In such a scenario, prefs and zip and maybe
i18n would move to //base_components.

I've always preferred less top level directories, so that's why I favor one components directory. I realize this is just personal style though :)

[Jói Sigurðsson]

> What about reconsidering that stuff in components is only stuff that depends
> on content?

We could, but it makes it harder to write simple DEPS rules that keep
us from introducing unwanted dependencies between components designed
to depend on different layers.

Consider an imaginary //components/foo that is designed to depend only
on //base. If that component wants to depend on any other component
from //components, it needs to be diligent about checking what layers
that component depends on.

If on the other hand we had a //base_components, then anything in
//base_components can depend freely on anything else in
//base_components (avoiding circular dependencies, though) since it is
a given that anything in //base_components depends only on //base, and
DEPS rules become simpler.

I'm not sure if we need all these new top-level directories though.
From my perspective, //base/i18n and //base/prefs are already examples
of things that would live in //base_components if we had it, but might
as well live where they are. Also, we so far have no examples that
I'm aware of of components that depend on //net and below, or some
other architectural layer and below. So in my opinion, the simplest
solution is to leave //base/i18n and //base/prefs where they are, and
add //base/zip.

Cheers,
Jói

[Brett Wilson]

I agree that there are some disadvantages to having some components
depend on content and some not, but this seems better than the
alternatives to me. Having two different directories I think would get
confusing, and it's easy to write DEPS rules for this where you can
see if something needs content or not. It also allows us to add or
remove content dependencies if needed without moving everything.

Brett

[Ben Goodger (Google)]

Agreed. DEPS is what you use to express dependencies and we should all become accustomed with looking at it.

I think having dep-specific component dirs will just make refactoring to add/remove dependencies more challenging, since you'll have to move components around when their DEPS change.

[Jói Sigurðsson]

OK, I'm convinced, //components it is. In this case, //components/zip
should be fine, and //content can depend on //components/zip.

I think it would be worth doing the following, and will shepherd this
work to make sure it happens:

a) Separate components under //components into a few .gyp files; these
can be by layer depended on; thus, //content could depend on targets
in //components/base_components.gyp, but not on targets in
//components/content_components.gyp

b) Improve tooling to make it easier to write simple DEPS rules for
components. One idea I think might be worth exploring is making it
possible for directories to express rules about which other
directories can depend on them (whereas today directories express
rules about which directories they may depend on). I had thought of
this when we were talking about how to make sure layers of Chromium
don't start depending incorrectly on blink (e.g. depending on
internals rather than API) and I think it could help there and in
content as well as in components.

Cheers,
Jói

[John Abd-El-Malek]

On Thu, Apr 18, 2013 at 11:14 AM, Jói Sigurðsson <j...@chromium.org> wrote:

OK, I'm convinced, //components it is.  In this case, //components/zip
should be fine,

 

and //content can depend on //components/zip.

Does this necessarily follow? I'd prefer if src\content doesn't depend on src\components since I think it's important to keep content's top-level dependencies as small as possible. i.e. why can't we put the zip wrapper code in third_party\zlib? Then also the work below wouldn't be needed.

[Alec Flett]

On Thu, Apr 18, 2013 at 10:11 AM, Jói Sigurðsson <j...@chromium.org> wrote:

Consider an imaginary //components/foo that is designed to depend only
on //base.  If that component wants to depend on any other component
from //components, it needs to be diligent about checking what layers
that component depends on.

So I just stumbled upon this in trying to move components/zip into base/zip - it seems that components/zip does depend on net/base/file_* to open files, synchronously... but I think it picked up that dependency indirectly because its a static library - it happily built from base/ but failed to link base_unittest (but literally every other target built just fine)

conveniently, everyone who depends on zip also happens to depend on net/ so this could only have been caught by IWYU :(

This also means at least in its current form, components/zip can't move to base/ - with the net/base dependency, it probably can't move to third_party either?

And now I see Jói and John's recent messages. 

sounds like we're down to toplevel directory vs breaking up components?

it sounds like the only difference between any of these is how many toplevel directories we end up with.

Seems like the issue here is that components/ is a mishmash of high and low level 'components' 

I'm starting to lean towards the toplevel directory, even though I'm with John about limiting the number of those.

Alec

[Jói Sigurðsson]

Yes, I think it necessarily follows, even though you could dodge the
need to face that fact by putting the zip wrapper code where you
suggested. Otherwise, //components would be the directory from which
everybody except //content can reuse features, and that's not exactly
a transparent rule (and why wouldn't other layers say that their layer
shouldn't depend back on //components then?).

I think we need to pick one or the other of these rules:

a) Reusable components are grouped into //base_components,
//content_components and so forth, based on what is the topmost layer
they depend on. In this scenario, //content could depend on e.g.
//base_components and //net_components but not //content_components.

a) Any reusable component can go into //components, independent of
what the topmost architectural layer it depends on is. Components are
grouped into gyp files by layer. From this, it follows that //content
can depend on those components in //components that are in gyp files
named for layers below content.

Cheers,
Jói

[Thiago Farina]

On Thu, Apr 18, 2013 at 3:28 PM, Jói Sigurðsson <j...@chromium.org> wrote:
> Yes, I think it necessarily follows, even though you could dodge the
> need to face that fact by putting the zip wrapper code where you
> suggested. Otherwise, //components would be the directory from which
> everybody except //content can reuse features, and that's not exactly
> a transparent rule (and why wouldn't other layers say that their layer
> shouldn't depend back on //components then?).
>
> I think we need to pick one or the other of these rules:
>
> a) Reusable components are grouped into //base_components,
> //content_components and so forth, based on what is the topmost layer
> they depend on. In this scenario, //content could depend on e.g.
> //base_components and //net_components but not //content_components.
>
> a) Any reusable component can go into //components, independent of
> what the topmost architectural layer it depends on is. Components are
> grouped into gyp files by layer. From this, it follows that //content
> can depend on those components in //components that are in gyp files
> named for layers below content.
>

Are deps among libraries/directories better enforced in google3 (BUILD
files) than in chromium?


--
Thiago

[John Abd-El-Malek]

On Thu, Apr 18, 2013 at 11:28 AM, Jói Sigurðsson <j...@chromium.org> wrote:

Yes, I think it necessarily follows, even though you could dodge the
need to face that fact by putting the zip wrapper code where you
suggested.  Otherwise, //components would be the directory from which
everybody except //content can reuse features, and that's not exactly
a transparent rule (and why wouldn't other layers say that their layer
shouldn't depend back on //components then?).

How I think about it is that src/components is the the place for getting extra functionality to add to your browser. So it's not just content that can't include it, but also other low-level directories like src/net, src/ui etc.. The only places that should include stuff from components would be browser apps, i.e. src/chrome, src/android_webview, and src/wherever_we_upstream_ios_code.

I worry that if we make src/components another place to put shared code between top level directories (instead of just shared code used by browser apps) then it would confusingly be another place where "base-type" code goes. Traditionally, src/base has been the place that code shared by multiple top level directories goes.

[John Abd-El-Malek]

On Thu, Apr 18, 2013 at 11:26 AM, Alec Flett <alec...@chromium.org> wrote:

On Thu, Apr 18, 2013 at 10:11 AM, Jói Sigurðsson <j...@chromium.org> wrote:

Consider an imaginary //components/foo that is designed to depend only
on //base.  If that component wants to depend on any other component
from //components, it needs to be diligent about checking what layers
that component depends on.

So I just stumbled upon this in trying to move components/zip into base/zip - it seems that components/zip does depend on net/base/file_* to open files, synchronously... but I think it picked up that dependency indirectly because its a static library - it happily built from base/ but failed to link base_unittest (but literally every other target built just fine)

conveniently, everyone who depends on zip also happens to depend on net/ so this could only have been caught by IWYU :(

This also means at least in its current form, components/zip can't move to base/ - with the net/base dependency, it probably can't move to third_party either?

And now I see Jói and John's recent messages. 

sounds like we're down to toplevel directory vs breaking up components?

it sounds like the only difference between any of these is how many toplevel directories we end up with.

Seems like the issue here is that components/ is a mishmash of high and low level 'components'

Personally I don't think it should matter which layer src/components/foo depends on. I think about it as someone starts with a browser based on content (ignoring ios for now), and then picks and chooses whatever extra features they want for their app from src/components.

 

 

I'm starting to lean towards the toplevel directory, even though I'm with John about limiting the number of those.

I really don't think we should create a new top level directory for the 8 source files for the zip wrappers.

[Alec Flett]

On Thu, Apr 18, 2013 at 1:40 PM, John Abd-El-Malek <j...@chromium.org> wrote:

On Thu, Apr 18, 2013 at 11:28 AM, Jói Sigurðsson <j...@chromium.org> wrote:

Yes, I think it necessarily follows, even though you could dodge the
need to face that fact by putting the zip wrapper code where you
suggested.  Otherwise, //components would be the directory from which
everybody except //content can reuse features, and that's not exactly
a transparent rule (and why wouldn't other layers say that their layer
shouldn't depend back on //components then?).

How I think about it is that src/components is the the place for getting extra functionality to add to your browser. So it's not just content that can't include it, but also other low-level directories like src/net, src/ui etc.. The only places that should include stuff from components would be browser apps, i.e. src/chrome, src/android_webview, and src/wherever_we_upstream_ios_code.

ok this makes sense to me. 

but that leaves open the question of zip - because I don't think it falls into this category (which sounds like "content+" to me?) - it depends on base and net, but not content.

Alec

[Jói Sigurðsson]

> How I think about it is that src/components is the the place for getting
> extra functionality to add to your browser. So it's not just content that
> can't include it, but also other low-level directories like src/net, src/ui
> etc.. The only places that should include stuff from components would be
> browser apps, i.e. src/chrome, src/android_webview, and
> src/wherever_we_upstream_ios_code.

Thanks John, that definition for //components would also make sense
and is perhaps more intuitive. As Alec has said, that leaves the
question of where to put things that are not "features for your
browser" so to speak. I guess we could doge that question now by
putting the zip wrapper code in third_party/zip as has been suggested.

Cheers,
Jói

[John Abd-El-Malek]

On Thu, Apr 18, 2013 at 4:15 PM, Jói Sigurðsson <j...@chromium.org> wrote:

> How I think about it is that src/components is the the place for getting
> extra functionality to add to your browser. So it's not just content that
> can't include it, but also other low-level directories like src/net, src/ui
> etc.. The only places that should include stuff from components would be
> browser apps, i.e. src/chrome, src/android_webview, and
> src/wherever_we_upstream_ios_code.

Thanks John, that definition for //components would also make sense
and is perhaps more intuitive.  As Alec has said, that leaves the
question of where to put things that are not "features for your
browser" so to speak.  I guess we could doge that question now by
putting the zip wrapper code in third_party/zip as has been suggested.

Thanks, that sounds best to me. I suspect that the zip example is a one-off, and it would be very rare that we need to share code between content and chrome that can't go into base.

>> >> b) Improve tooling to make it easier to write simple DEPS rules for
>> >> components.  One idea I think might be worth exploring is making it
>> >> possible for directories to express rules about which other
>> >> directories can depend on them (whereas today directories express
>> >> rules about which directories they may depend on).  I had thought of
>> >> this when we were talking about how to make sure layers of Chromium
>> >> don't start depending incorrectly on blink (e.g. depending on
>> >> internals rather than API) and I think it could help there and in
>> >> content as well as in components.

I forgot to reply to this. I've wished for something similar to this as well. There are two cases I've seen which can be problematic:

-new top level directories, or even subdirectories of existing top level directories, start including content and we don't want these directories to include it because it's a layering violation. an example that happened before is src/device, but luckily we found that early and fixed the dependencies.

-existing directories that include content/public poking at the internals by adding DEPs entries for content/browser

So if we had a reverse-deps for what can include content, and that file was maintained inside content, it would solve that issue. But I wonder if that's too much effort, and maybe there's something simpler. i.e. maybe we can add presubmit rules that adding a +"foo" line to a DEPS file requires owners approval from src/foo/OWNERS?

[Satoru Takabayashi]

On Fri, Apr 19, 2013 at 8:40 AM, John Abd-El-Malek <j...@chromium.org> wrote:

On Thu, Apr 18, 2013 at 4:15 PM, Jói Sigurðsson <j...@chromium.org> wrote:

> How I think about it is that src/components is the the place for getting
> extra functionality to add to your browser. So it's not just content that
> can't include it, but also other low-level directories like src/net, src/ui
> etc.. The only places that should include stuff from components would be
> browser apps, i.e. src/chrome, src/android_webview, and
> src/wherever_we_upstream_ios_code.

Thanks John, that definition for //components would also make sense
and is perhaps more intuitive.  As Alec has said, that leaves the
question of where to put things that are not "features for your
browser" so to speak.  I guess we could doge that question now by
putting the zip wrapper code in third_party/zip as has been suggested.

Thanks, that sounds best to me. I suspect that the zip example is a one-off, and it would be very rare that we need to share code between content and chrome that can't go into base.

Sorry for coming late, but I found it rather awkward to have this in third_party/zip. Here's why:

1) "components/zip" is built on top of 'zlib' but also uses 'base' and 'net' (for FileStream). Somewhat awkward to use 'base' and 'net' from third_party.

2) The code consists of 1752 lines of C++ code including unit tests. This is not a trivial wrapper.

3) If we are to move this to third_party, are we going to introduce 'third_party_unittests'? This looks somewhat awkward.

I don't have a strong feeling on this, but wanted to raise a concern before it's final.

Satoru 

[John Abd-El-Malek]

On Thu, Apr 18, 2013 at 4:48 PM, Satoru Takabayashi <sat...@chromium.org> wrote:

On Fri, Apr 19, 2013 at 8:40 AM, John Abd-El-Malek <j...@chromium.org> wrote:

On Thu, Apr 18, 2013 at 4:15 PM, Jói Sigurðsson <j...@chromium.org> wrote:

> How I think about it is that src/components is the the place for getting
> extra functionality to add to your browser. So it's not just content that
> can't include it, but also other low-level directories like src/net, src/ui
> etc.. The only places that should include stuff from components would be
> browser apps, i.e. src/chrome, src/android_webview, and
> src/wherever_we_upstream_ios_code.

Thanks John, that definition for //components would also make sense
and is perhaps more intuitive.  As Alec has said, that leaves the
question of where to put things that are not "features for your
browser" so to speak.  I guess we could doge that question now by
putting the zip wrapper code in third_party/zip as has been suggested.

Thanks, that sounds best to me. I suspect that the zip example is a one-off, and it would be very rare that we need to share code between content and chrome that can't go into base.

Sorry for coming late, but I found it rather awkward to have this in third_party/zip. Here's why:

1) "components/zip" is built on top of 'zlib' but also uses 'base' and 'net' (for FileStream). Somewhat awkward to use 'base' and 'net' from third_party.

searching cs for base includes in third_party shows 300 hits. net has 2 hits.

 

2) The code consists of 1752 lines of C++ code including unit tests. This is not a trivial wrapper. 

I'm not sure why the size of the wrapper affects where it should go? 

3) If we are to move this to third_party, are we going to introduce 'third_party_unittests'? This looks somewhat awkward.

We can include that file from an existing unittest target.

[Satoru Takabayashi]

On Fri, Apr 19, 2013 at 9:18 AM, John Abd-El-Malek <j...@chromium.org> wrote:

On Thu, Apr 18, 2013 at 4:48 PM, Satoru Takabayashi <sat...@chromium.org> wrote:

On Fri, Apr 19, 2013 at 8:40 AM, John Abd-El-Malek <j...@chromium.org> wrote:

On Thu, Apr 18, 2013 at 4:15 PM, Jói Sigurðsson <j...@chromium.org> wrote:

> How I think about it is that src/components is the the place for getting
> extra functionality to add to your browser. So it's not just content that
> can't include it, but also other low-level directories like src/net, src/ui
> etc.. The only places that should include stuff from components would be
> browser apps, i.e. src/chrome, src/android_webview, and
> src/wherever_we_upstream_ios_code.

Thanks John, that definition for //components would also make sense
and is perhaps more intuitive.  As Alec has said, that leaves the
question of where to put things that are not "features for your
browser" so to speak.  I guess we could doge that question now by
putting the zip wrapper code in third_party/zip as has been suggested.

Thanks, that sounds best to me. I suspect that the zip example is a one-off, and it would be very rare that we need to share code between content and chrome that can't go into base.

Sorry for coming late, but I found it rather awkward to have this in third_party/zip. Here's why:

1) "components/zip" is built on top of 'zlib' but also uses 'base' and 'net' (for FileStream). Somewhat awkward to use 'base' and 'net' from third_party.

searching cs for base includes in third_party shows 300 hits. net has 2 hits.

 

2) The code consists of 1752 lines of C++ code including unit tests. This is not a trivial wrapper. 

I'm not sure why the size of the wrapper affects where it should go? 

My feeling is that if it's non trivial, it's more like a first class library of Chromium project hence not in third_party, but I don't feel strongly about it.

If we are to put this in third_party/zip, what about 'sql' and 'dbus' in the top level directory? These can also be considered wrappers for libsqlite and libdbus. For consistency, should they also be moved to third_party? I think it'd make sense to have 'sql', 'dbus', and 'zip' in the same directory.

Satoru

[Alec Flett]

Also to clarify:

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

- third_party/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.

- 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.

I don't actually think that zip is particularly complex, but there is definitely enough chromium-specific glue there that I wouldn't write it off as a trivial wrapper.

Alec

On Thu, Apr 18, 2013 at 4:48 PM, Satoru Takabayashi <sat...@chromium.org> wrote:

[Alec Flett]

On Thu, Apr 18, 2013 at 5:54 PM, Satoru Takabayashi <sat...@chromium.org> wrote:

My feeling is that if it's non trivial, it's more like a first class library of Chromium project hence not in third_party, but I don't feel strongly about it.

If we are to put this in third_party/zip, what about 'sql' and 'dbus' in the top level directory? These can also be considered wrappers for libsqlite and libdbus. For consistency, should they also be moved to third_party? I think it'd make sense to have 'sql', 'dbus', and 'zip' in the same directory.

third_party is beginning to sound strange to me. Other suggestions:

• lib/
• wrappers/
• support/

Other toplevel candidates for this directory (I don't even know what all of these are, I'm mainly looking at libraries that have very simple dependencies, that appear to be standalone or "simple" wrappers around other libraries)

• rlz/ (standalone, so obviously not for 'wrappers')
• jingle/ (wrapper around libjingle?)
• googleurl/ (depends on base and third_party/icu)
• url/ (not even sure how this relates to googleurl or not?)
  

Seems like the net effect of this could be the removal of 5+ toplevel directories... personally my vote is for 'lib' or 'support' and for making each subdirectory be independent of its peers (i.e. no real reason for a lib/lib.gyp or what have you)

but ultimately I'm still blocked trying to find a good home for zip/ - I'd like to reach some kind of consensus.. 

Alec

[Paweł Hajdan, Jr.]

Jói, with the concern raised about too many toplevel directories, why not a slightly different structure with the same goal?

base_components -> components/base

net_components -> components/net

content_components -> components/content

WDYT?

I can do the move if there is a consensus about this. I really think that would be better than a proliferation of toplevel directories or hacks like putting Chromium code in third_party.

Paweł

[Scott Hess]

I own sql/, which also is odd to have at root.

I think there are two reasons why a project generally wants to isolate things:

 - prevent untoward dependencies from creeping in.

 - don't compile in things you won't need.

I think we should treat the latter case as a tooling issue.  The linker should drop unused stuff, and if it isn't then contorting our tree layout to allow dropping trivial wrapper libraries is perhaps a cure worse than the disease.

The former issue is more one of correctness.  If module foo/ depends on sql/, and someone changes sql/ to depend on some bar/ which foo/ should not depend on, that's a problem. base_components/ might have potential, but zip/ already depends on net/ (but AFAICT the dependency is in the implementation, so it could perhaps be refactored away).  Others in your list also depend on net/, so net_components/?

[I'd be fine with base_components/ allowing dependencies on net/, myself, as some areas of base/ are really grab-bag already.]

-scott

[John Abd-El-Malek]

On Fri, Apr 19, 2013 at 10:37 AM, Alec Flett <alec...@chromium.org> wrote:

On Thu, Apr 18, 2013 at 5:54 PM, Satoru Takabayashi <sat...@chromium.org> wrote:

My feeling is that if it's non trivial, it's more like a first class library of Chromium project hence not in third_party, but I don't feel strongly about it.

If we are to put this in third_party/zip, what about 'sql' and 'dbus' in the top level directory? These can also be considered wrappers for libsqlite and libdbus. For consistency, should they also be moved to third_party? I think it'd make sense to have 'sql', 'dbus', and 'zip' in the same directory.

third_party is beginning to sound strange to me. Other suggestions:

• lib/
• wrappers/
• support/

My opposition for another generic top level directory is that it's very similar to base. Now we'll basically have two base directories with different names.

[John Abd-El-Malek]

From the previous messages, unless I'm misunderstanding, Joi is now agreeing that there would be one components directory. See previous emails about why having multiple (or subdirectories of one, which is really equivalent) is problematic.

[Paweł Hajdan, Jr.]

John, unless I have missed something, the problems mentioned are:

1. Difficulty of writing DEPS files. I believe this is not the case with my proposal. Existing components/DEPS would move to components/content/DEPS . components/base and components/net would have their own DEPS files, equally easy to verify as current components/DEPS. Does anyone disagree about that?

2. components becoming the new base, i.e. other toplevel directories starting to depend on components. I agree that would be bad. I think most cases, if they arise, could be solved by actually putting said toplevel directories inside components (at the right level, which could well be components/base).

Please correct me if I missed some other concern about components/{base,net,content} subdirectories. I still think this would be the best solution.

Paweł

[John Abd-El-Malek]

On Fri, Apr 19, 2013 at 12:12 PM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

John, unless I have missed something, the problems mentioned are:

see brett and ben's emails

 

1. Difficulty of writing DEPS files. I believe this is not the case with my proposal. Existing components/DEPS would move to components/content/DEPS . components/base and components/net would have their own DEPS files, equally easy to verify as current components/DEPS. Does anyone disagree about that?

ben's email

 

2. components becoming the new base, i.e. other toplevel directories starting to depend on components. I agree that would be bad. I think most cases, if they arise, could be solved by actually putting said toplevel directories inside components (at the right level, which could well be components/base).

this won't always work. and again, this also doesn't solve having two bases.

[John Abd-El-Malek]

On Thu, Apr 18, 2013 at 4:15 PM, Jói Sigurðsson <j...@chromium.org> wrote:

> How I think about it is that src/components is the the place for getting
> extra functionality to add to your browser. So it's not just content that
> can't include it, but also other low-level directories like src/net, src/ui
> etc.. The only places that should include stuff from components would be
> browser apps, i.e. src/chrome, src/android_webview, and
> src/wherever_we_upstream_ios_code.

Thanks John, that definition for //components would also make sense
and is perhaps more intuitive.  As Alec has said, that leaves the
question of where to put things that are not "features for your
browser" so to speak.  I guess we could doge that question now by
putting the zip wrapper code in third_party/zip as has been suggested.

btw this is what I favor now in this case as well. As mentioned in my other email, there are 300 includes of base from third_party, so there's lot of redundant there. 

[Thiago Farina]

On Fri, Apr 19, 2013 at 2:37 PM, Alec Flett <alec...@chromium.org> wrote:
> third_party is beginning to sound strange to me. Other suggestions:
>
> lib/
> wrappers/
> support/
>
> Other toplevel candidates for this directory (I don't even know what all of
> these are, I'm mainly looking at libraries that have very simple
> dependencies, that appear to be standalone or "simple" wrappers around other
> libraries)
>

> googleurl/ (depends on base and third_party/icu)
> url/ (not even sure how this relates to googleurl or not?)
>

googleurl/ is going away soon. I'm working on it with Brett.
url/ will replace it.

See Brett's recent email to this mailing list.

(Please, cut off parts irrelevants when replying!).

--
Thiago

[Thiago Farina]

On Fri, Apr 19, 2013 at 2:56 PM, Paweł Hajdan, Jr.
<phajd...@chromium.org> wrote:
> Jói, with the concern raised about too many toplevel directories, why not a
> slightly different structure with the same goal?
>
> base_components -> components/base
> net_components -> components/net
> content_components -> components/content
>

From the beginning I always thought //components as a strange place
and confused named.

What is a component? Something that is reusable by Browser but extract
from chrome/browser and only depends on content/? Why we have to put
our libraries there to start with?

Note that I don't see any problems with many toplevel directories.
Could someone underline/summarize the problems with have them?

Thanks,

--
Thiago

[Jói Sigurðsson]

> From the previous messages, unless I'm misunderstanding, Joi is now agreeing
> that there would be one components directory. See previous emails about why
> having multiple (or subdirectories of one, which is really equivalent) is
> problematic.

Yes, confirmed, I agree we can go with one components directory where
the definition of the directory is "features those embedding //content
might want to use in addition". This implies it shouldn't contain
stuff that //content depends on.

This doesn't solve the question of where to put stuff like the zip
wrapper, but the suggestion of placing it in third_party/zip has come
up. I don't have a strong opinion on that. From my perspective
though, things like //base/prefs and //base/i18n should stay where
they are.

Cheers,
Jói

[Alec Flett]

ok, I'm going to go ahead and move zip into third_party/zip - a peer of zlib and minizip since it depends on both of them. 

Alec

[Satoru Takabayashi]

I recalled one thing. I think the purpose of third_party directory is to make a clear boundary between chromium code and third_party code. Moving code like 'zip' to third_party may make the boundary unclear.

As described in https://sites.google.com/a/chromium.org/dev/developers/adding-3rd-party-libraries it'd be nice to contact open-source-thi...@google.com on this change.

Satoru

[John Abd-El-Malek]

On Mon, Apr 22, 2013 at 4:57 PM, Satoru Takabayashi <sat...@chromium.org> wrote:

I recalled one thing. I think the purpose of third_party directory is to make a clear boundary between chromium code and third_party code. Moving code like 'zip' to third_party may make the boundary unclear.

As described in https://sites.google.com/a/chromium.org/dev/developers/adding-3rd-party-libraries it'd be nice to contact open-source-thi...@google.com on this change.

Note: those instructions are when adding third party code to this directory.

Also, note that there are 300 hits for includes from base/ in src/third_party: https://code.google.com/p/chromium/codesearch#search/&q=%22%5C%22base/%22%20file:src/third_party&sq=package:chromium&type=cs 

[Satoru Takabayashi]

On Tue, Apr 23, 2013 at 9:31 AM, John Abd-El-Malek <j...@chromium.org> wrote:

On Mon, Apr 22, 2013 at 4:57 PM, Satoru Takabayashi <sat...@chromium.org> wrote:

I recalled one thing. I think the purpose of third_party directory is to make a clear boundary between chromium code and third_party code. Moving code like 'zip' to third_party may make the boundary unclear.

As described in https://sites.google.com/a/chromium.org/dev/developers/adding-3rd-party-libraries it'd be nice to contact open-source-thi...@google.com on this change.

Note: those instructions are when adding third party code to this directory.

Yes, and we are about to add non-third party code to that directory. :) I think it would make sense to contact the open source experts just in case they have some opinion on this.

Satoru

[Paweł Hajdan, Jr.]

On Mon, Apr 22, 2013 at 5:31 PM, John Abd-El-Malek <j...@chromium.org> wrote:

On Mon, Apr 22, 2013 at 4:57 PM, Satoru Takabayashi <sat...@chromium.org> wrote:

I recalled one thing. I think the purpose of third_party directory is to make a clear boundary between chromium code and third_party code. Moving code like 'zip' to third_party may make the boundary unclear.

As described in https://sites.google.com/a/chromium.org/dev/developers/adding-3rd-party-libraries it'd be nice to contact open-source-thi...@google.com on this change.

Note: those instructions are when adding third party code to this directory.

I think Satoru has a good point (in fact I share the same concern). When third_party becomes a mix of real third party and Chromium code, it starts to diminish the value of a separate third_party directory.

 

Also, note that there are 300 hits for includes from base/ in src/third_party: https://code.google.com/p/chromium/codesearch#search/&q=%22%5C%22base/%22%20file:src/third_party&sq=package:chromium&type=cs 

The above is not exactly accurate. cld seems to have a directory called "base" right in it. Then multiple projects have been patched to use src/base, but are otherwise not Chromium code.

[John Abd-El-Malek]

On Tue, Apr 23, 2013 at 10:26 AM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

On Mon, Apr 22, 2013 at 5:31 PM, John Abd-El-Malek <j...@chromium.org> wrote:

On Mon, Apr 22, 2013 at 4:57 PM, Satoru Takabayashi <sat...@chromium.org> wrote:

I recalled one thing. I think the purpose of third_party directory is to make a clear boundary between chromium code and third_party code. Moving code like 'zip' to third_party may make the boundary unclear.

As described in https://sites.google.com/a/chromium.org/dev/developers/adding-3rd-party-libraries it'd be nice to contact open-source-thi...@google.com on this change.

Note: those instructions are when adding third party code to this directory.

I think Satoru has a good point (in fact I share the same concern). When third_party becomes a mix of real third party and Chromium code, it starts to diminish the value of a separate third_party directory.

The value of third_party isn't that it's only third_party _code_. It's that third_party code must go there. There's nothing to say that we can't put our own code there. A bunch of the projects there is by chromium devs anyways, just pulled in from google code etc.

 

Also, note that there are 300 hits for includes from base/ in src/third_party: https://code.google.com/p/chromium/codesearch#search/&q=%22%5C%22base/%22%20file:src/third_party&sq=package:chromium&type=cs 

The above is not exactly accurate. cld seems to have a directory called "base" right in it. Then multiple projects have been patched to use src/base, but are otherwise not Chromium code.

excluding third_party/cld, there are 272 

hits: https://code.google.com/p/chromium/codesearch#search/&q=%22%5C%22base/%22%20file:src/third_party%20-file:third_party/cld&sq=package:chromium&type=cs

my point is there's a lot of precedent

[Nico Weber]

In the first few pages, this is mostly tcmalloc, which has its own base/ directory. Excluding that, leveldb is the next big block, which has chromium code in its upstream repo that we pull in unmodified. I don't think there's that much precedent.

[John Abd-El-Malek]

On Tue, Apr 23, 2013 at 12:21 PM, Nico Weber <tha...@chromium.org> wrote:

On Tue, Apr 23, 2013 at 10:32 AM, John Abd-El-Malek <j...@chromium.org> wrote:

On Tue, Apr 23, 2013 at 10:26 AM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

On Mon, Apr 22, 2013 at 5:31 PM, John Abd-El-Malek <j...@chromium.org> wrote:

On Mon, Apr 22, 2013 at 4:57 PM, Satoru Takabayashi <sat...@chromium.org> wrote:

I recalled one thing. I think the purpose of third_party directory is to make a clear boundary between chromium code and third_party code. Moving code like 'zip' to third_party may make the boundary unclear.

As described in https://sites.google.com/a/chromium.org/dev/developers/adding-3rd-party-libraries it'd be nice to contact open-source-thi...@google.com on this change.

Note: those instructions are when adding third party code to this directory.

I think Satoru has a good point (in fact I share the same concern). When third_party becomes a mix of real third party and Chromium code, it starts to diminish the value of a separate third_party directory.

The value of third_party isn't that it's only third_party _code_. It's that third_party code must go there. There's nothing to say that we can't put our own code there. A bunch of the projects there is by chromium devs anyways, just pulled in from google code etc.

 

Also, note that there are 300 hits for includes from base/ in src/third_party: https://code.google.com/p/chromium/codesearch#search/&q=%22%5C%22base/%22%20file:src/third_party&sq=package:chromium&type=cs 

The above is not exactly accurate. cld seems to have a directory called "base" right in it. Then multiple projects have been patched to use src/base, but are otherwise not Chromium code.

excluding third_party/cld, there are 272 

hits: https://code.google.com/p/chromium/codesearch#search/&q=%22%5C%22base/%22%20file:src/third_party%20-file:third_party/cld&sq=package:chromium&type=cs

my point is there's a lot of precedent

In the first few pages, this is mostly tcmalloc, which has its own base/ directory. Excluding that, leveldb is the next big block, which has chromium code in its upstream repo

that is code in our repo, not upstream. there's also npapi

i see now that some other directories are using base and pulled through deps. that's wrong.

[Nico Weber]

Unrelatedly, I checked with open-source-thi...@google.com, they say it's fine to put first-party code below third_party.

It sounds like some folks think putting the code in third_party is weird because it's not third-party code (but there's already some amounts of first-party code there), and others think putting the code in a top-level directory is weird because they don't like having toplevel directories (but there already are some toplevel directories for 3rd-party wrappers). Neither position sounds obviously more convincing, so either we leave the decision to alecflett who wants to make the change (and if it turns down the road out that his decision isn't the best one, we can move it), or we need to escalate to one of our two toplevel OWNERS.

[Alec Flett]

So I'm going to put it in third_party/zip (have it in my tree, just need a little extra gyp-love) and leave it up to reviewers to decide. I'll post to this thread when the CL is ready.

Alec

[Alec Flett]

Ok, took longer than I thought but here's the CL:

https://codereview.chromium.org/14021015/

As I've mentioned to people before: consider me the messenger here - I don't care where this ends up ultimately. reluctant consensus seemed to be in third_party/zip

I welcome all feedback on correctness of the patch but please see my first comment in the CL about open issues.

Alec