Include zlib.h properly

Include zlib.h properly

Linux links with the system zlib, so system headers should be used.

Add dependency on zlib to make sure -Ithird_party/zlib gets passed on other platforms.

Also see Gentoo Linux bug report.
https://bugs.gentoo.org/show_bug.cgi?id=364205

Contributed by Mike Gilbert <floppymaster@gmail.com>;.

BUG=none
TEST=Remove third_party/zlib/zlib.h and compile on linux

[Mike Gilbert, 2011-04-21 03:31:30]

Please have a look and land this if ok.

[brettw, 2011-04-21 03:37:19]

Do all Linux builds link with the system zlib? What about other platforms? Is
there a Chromium bug about this?

[Mike Gilbert, 2011-04-21 03:44:55]

On 2011/04/21 03:37:19, brettw wrote:
> Do all Linux builds link with the system zlib? What about other platforms? Is
> there a Chromium bug about this?

third_party/zlib/zlib.gyp shows that use_system_zlib=1 by default on linux and
freebsd. Other platforms default to use_system_zlib=0.

If you pass -Duse_system_zlib=0 to gyp_chromium, you can force it to use the
bundled zlib on linux, and I have also tested that.

I don't see a chromium bug report, which doesn't suprise me. Gentoo is the only
distro I know of that removes bundled headers before building, and it would be
hard to spot otherwise.

[dcheng, 2011-04-21 03:55:58]

Thanks for the catch.

http://codereview.chromium.org/6873148/diff/1/content/browser/renderer_host/c...
File content/browser/renderer_host/clipboard_message_filter.cc (right):

http://codereview.chromium.org/6873148/diff/1/content/browser/renderer_host/c...
content/browser/renderer_host/clipboard_message_filter.cc:16: #include <zlib.h>
Will this work on platforms without a system zlib? Looking around, it looks like
the canonical way to do this is
http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/net/base/gzip....

I'll double check with someone who's more familiar with this before landing it
tomorrow.

[Mike Gilbert, 2011-04-21 04:10:09]

On 2011/04/21 03:55:58, dcheng wrote:
> Thanks for the catch.
> 
>
http://codereview.chromium.org/6873148/diff/1/content/browser/renderer_host/c...
> File content/browser/renderer_host/clipboard_message_filter.cc (right):
> 
>
http://codereview.chromium.org/6873148/diff/1/content/browser/renderer_host/c...
> content/browser/renderer_host/clipboard_message_filter.cc:16: #include
<zlib.h>
> Will this work on platforms without a system zlib? Looking around, it looks
like
> the canonical way to do this is
>
http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/net/base/gzip....
> 
> I'll double check with someone who's more familiar with this before landing it
> tomorrow.

I think this should work everywhere, but I have no means to test it on non-linux
platforms.

third_party/zlib/zlib.gyp:zlib sets include_dirs = '.' in
direct_dependent_settings, so the #if USE_SYSTEM_ZLIB wrapper should be
unnecessary.

[brettw, 2011-04-21 04:40:16]

There should be a chromium bug to track this. You can file one yourself.

The zlib.gyp file explicitly doesn't use the system zlib for some platforms
since it says there are incompatibilities, so it seems this patch is wrong to
include it as <zlib.h>

[Mike Gilbert, 2011-04-21 05:25:40]

Including it as <zlib.h> does not mean it will always use the system zlib
header.

The third_party/zlib directory gets prepended to the header search path if
use_system_zlib=0 in zlib.gyp, which means it will look for zlib.h there first.
That's why I added the dependency in content_browser.gypi.

This patch puts full control over which header is used in the hands of zlib.gyp,
as it should be.

[Paweł Hajdan Jr., 2011-04-21 08:18:00]

Thanks for sending the patch.

http://codereview.chromium.org/6873148/diff/1/content/browser/renderer_host/c...
File content/browser/renderer_host/clipboard_message_filter.cc (right):

http://codereview.chromium.org/6873148/diff/1/content/browser/renderer_host/c...
content/browser/renderer_host/clipboard_message_filter.cc:16: #include <zlib.h>
On 2011/04/21 03:55:58, dcheng wrote:
> Will this work on platforms without a system zlib? Looking around, it looks
like
> the canonical way to do this is
>
http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/net/base/gzip....
> 
> I'll double check with someone who's more familiar with this before landing it
> tomorrow.

Yeah, I think wrapping it in #if defined(USE_SYSTEM_ZLIB) for consistency is
better than arguing for this approach (also, #include <> lines should go before
the #include "" lines, and this patch doesn't follow it).

[Mike Gilbert, 2011-04-21 14:54:40]

Ok, I have altered the patch to match the one Paweł applied downstream at
Gentoo. This uses the #if defined(USE_SYSTEM_ZLIB) approach.

Does that work for everyone?

[brettw, 2011-04-21 15:31:04]

I'll defer my review to Pawel.

[Mike Gilbert, 2011-04-26 00:22:18]

Ping.

Can someone land this or give advice on how to make it commit-worthy?

[dcheng, 2011-04-26 00:30:08]

On 2011/04/26 00:22:18, Mike Gilbert wrote:
> Ping.
> 
> Can someone land this or give advice on how to make it commit-worthy?

LGTM. You can just use the commit bot to land this patch I think.

[commit-bot: I haz the power, 2011-04-26 00:41:29]

Presubmit check for 6873148-4003 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/browser/renderer_host/clipboard_message_filter.cc,content/content_browser.gypi

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[commit-bot: I haz the power, 2011-04-26 00:42:15]

Presubmit check for 6873148-4003 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/browser/renderer_host/clipboard_message_filter.cc,content/content_browser.gypi

[commit-bot: I haz the power, 2011-04-26 00:45:32]

Presubmit check for 6873148-4003 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/browser/renderer_host/clipboard_message_filter.cc,content/content_browser.gypi

[Mike Gilbert, 2011-04-26 00:46:37]

On 2011/04/26 00:30:08, dcheng wrote:
> On 2011/04/26 00:22:18, Mike Gilbert wrote:
> > Ping.
> > 
> > Can someone land this or give advice on how to make it commit-worthy?
> 
> LGTM. You can just use the commit bot to land this patch I think.

Oh that's neat! Looks like I need a sign-off from brettw (or another OWNER).

[Paweł Hajdan Jr., 2011-04-26 14:54:30]

Landed in http://codereview.chromium.org/6893021 . I'm assuming landing it is
fine as Brett said "I'll defer my review to Pawel.". I can do a follow-up if
anything more is needed.

[brettw, 2011-04-26 16:02:26]

Yes, that's fine, thanks.