Aura on Android: fix mangling of definitions in jni.h by Byte definition in zlib.

Aura on Android: fix mangling of definitions in jni.h by Byte definition in zlib.

In image_writer_private_api.cc the operation_manager.h -> operation.h -> zip_reader.h -> minizip/unzip.h -> zlib.h -> zconf.h -> mozconff.h include path defines "Byte" as "MOZ_Z_Byte", which mangles some of the definitions in jni.h which is indirectly included by profile.h. This causes some compile errors.

This patch tries to solve this problem by forward declaring zip::ZipReader in operation.h and including zip_reader.h in operation.cc instead of operation.h.

BUG=554199
Committed: https://crrev.com/83deb42017ace10596869a7f9200cd80943492a4
Cr-Commit-Position: refs/heads/master@{#360076}

[Hadi, 2015-11-11 19:40:25]

moshayedi@chromium.org changed reviewers:
+ mfomitchev@chromium.org, sievers@chromium.org

[Hadi, 2015-11-11 19:40:25]

PTAL. This fixes the issue we had in "image_writer_private_api.cc" for which we
needed to include "jni.h" as the first include.

[mfomitchev, 2015-11-11 21:50:22]

Can you please add (one of the) owners of zlib to get their take on this?
Looks fine to me though.

[no sievers, 2015-11-11 22:02:22]

> indirectly includes zlib/google/zip_reader.h which defines "Byte" as
"MOZ_Z_Byte",

It's not zip_reader.h but the following include from zip_reader.h that defines
it, I assume?

#if defined(USE_SYSTEM_MINIZIP)
#include <minizip/unzip.h>
#else
#include "third_party/zlib/contrib/minizip/unzip.h"
#endif

Is it only for |zip_reader_|? Because then we could change it to scoped_ptr and
move the include to the .cc file.

[mfomitchev, 2015-11-11 22:56:46]

On 2015/11/11 22:02:22, sievers wrote:
> > indirectly includes zlib/google/zip_reader.h which defines "Byte" as
> "MOZ_Z_Byte",
> 
> It's not zip_reader.h but the following include from zip_reader.h that defines
> it, I assume?
> 
> #if defined(USE_SYSTEM_MINIZIP)
> #include <minizip/unzip.h>
> #else
> #include "third_party/zlib/contrib/minizip/unzip.h"
> #endif
> 
> Is it only for |zip_reader_|? Because then we could change it to scoped_ptr
and
> move the include to the .cc file.

Getting rid of the import would be cool. Are you suggesting we change the type
of zip_file_ in zip_reader.h to scoped_ptr<unzFile>? We should probably ask the
owners first, to see what they think of this approach.

[no sievers, 2015-11-11 23:08:25]

On 2015/11/11 22:56:46, mfomitchev wrote:
> On 2015/11/11 22:02:22, sievers wrote:
> > > indirectly includes zlib/google/zip_reader.h which defines "Byte" as
> > "MOZ_Z_Byte",
> > 
> > It's not zip_reader.h but the following include from zip_reader.h that
defines
> > it, I assume?
> > 
> > #if defined(USE_SYSTEM_MINIZIP)
> > #include <minizip/unzip.h>
> > #else
> > #include "third_party/zlib/contrib/minizip/unzip.h"
> > #endif
> > 
> > Is it only for |zip_reader_|? Because then we could change it to scoped_ptr
> and
> > move the include to the .cc file.
> 
> Getting rid of the import would be cool. Are you suggesting we change the type
> of zip_file_ in zip_reader.h

in operation.h

[no sievers, 2015-11-11 23:11:48]

On 2015/11/11 23:08:25, sievers wrote:
> On 2015/11/11 22:56:46, mfomitchev wrote:
> > On 2015/11/11 22:02:22, sievers wrote:
> > > > indirectly includes zlib/google/zip_reader.h which defines "Byte" as
> > > "MOZ_Z_Byte",
> > > 
> > > It's not zip_reader.h but the following include from zip_reader.h that
> defines
> > > it, I assume?
> > > 
> > > #if defined(USE_SYSTEM_MINIZIP)
> > > #include <minizip/unzip.h>
> > > #else
> > > #include "third_party/zlib/contrib/minizip/unzip.h"
> > > #endif
> > > 
> > > Is it only for |zip_reader_|? Because then we could change it to
scoped_ptr
> > and
> > > move the include to the .cc file.
> > 
> > Getting rid of the import would be cool. Are you suggesting we change the
type
> > of zip_file_ in zip_reader.h
> 
> in operation.h

operation.h:
-#include "third_party/zlib/google/zip_reader.h"
...
+namespace zip {
+ class ZipReader;
+}

...
-zip::ZipReader zip_reader_;
+scoped_ptr<zip::zip_reader>;

operation.cc:
+include "third_party/zlib/google/zip_reader.h"
...
+zip_reader_(new zip::ZipReader);
etc.

[Hadi, 2015-11-12 15:36:09]

Description was changed from

==========
Aura on Android: fix mangling of defs in jni.h by defs in zlib.

In image_writer_private_api.cc operation_manager.h indirectly includes
zlib/google/zip_reader.h which defines "Byte" as "MOZ_Z_Byte",
which mangles some of the definitions in jni.h which is indirectly included by
profile.h. This causes some compile errors.

This patch tries to solve this problem by undeffing "Byte" after zip_reader.h is
included in operation.h.

Since operation.h is the only header file which includes zip_reader.h, I don't
immediately see another header file which may cause a
similar problem.

BUG=554199
==========

to

==========
Aura on Android: fix mangling of definitions in jni.h by Byte definition in
zlib.

In image_writer_private_api.cc the operation_manager.h -> operation.h ->
zip_reader.h -> minizip/unzip.h -> zlib.h -> zconf.h -> mozconff.h include path
defines "Byte" as "MOZ_Z_Byte", which mangles some of the definitions in jni.h
which is indirectly included by profile.h. This causes some compile errors.

This patch tries to solve this problem by forward declaring zip::ZipReader in
operation.h and including zip_reader.h in operation.cc instead of operation.h.

BUG=554199
==========

[Hadi, 2015-11-12 15:45:36]

On 2015/11/11 23:11:48, sievers wrote:
> operation.h:
> -#include "third_party/zlib/google/zip_reader.h"
> ...
> +namespace zip {
> + class ZipReader;
> +}
> 
> ...
> -zip::ZipReader zip_reader_;
> +scoped_ptr<zip::zip_reader>;
> 
> operation.cc:
> +include "third_party/zlib/google/zip_reader.h"
> ...
> +zip_reader_(new zip::ZipReader);
> etc.

Yeah, I think this makes the change much cleaner and less surprising for the
future readers of the code. I applied this change.

[mfomitchev, 2015-11-12 15:47:17]

Awesome! LGTM

[Hadi, 2015-11-12 15:47:29]

moshayedi@chromium.org changed reviewers:
+ agl@chromium.org, haven@chromium.org

[Hadi, 2015-11-12 15:47:30]

+agl@ as zlib OWNER
+haven@ as OWNER of image_writer_private

[agl, 2015-11-12 17:28:03]

lgtm

[Drew Haven, 2015-11-16 21:19:00]

lgtm

https://codereview.chromium.org/1432293003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/image_writer_private/operation.h (left):

https://codereview.chromium.org/1432293003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/operation.h:211:
zip::ZipReader zip_reader_;
Can you add a quick comment on why this is a pointer and not a field? Just so
that no one gets confused in the future as to why this needs to be a pointer and
doesn't try to clean up the code.

[Hadi, 2015-11-16 21:33:03]

https://codereview.chromium.org/1432293003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/image_writer_private/operation.h (left):

https://codereview.chromium.org/1432293003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/image_writer_private/operation.h:211:
zip::ZipReader zip_reader_;
On 2015/11/16 21:19:00, Drew Haven wrote:
> Can you add a quick comment on why this is a pointer and not a field? Just so
> that no one gets confused in the future as to why this needs to be a pointer
and
> doesn't try to clean up the code.

Done.

[Hadi, 2015-11-17 14:59:54]

The CQ bit was checked by moshayedi@chromium.org

[Hadi, 2015-11-17 14:59:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
mfomitchev@chromium.org, agl@chromium.org, haven@chromium.org
Link to the patchset: https://codereview.chromium.org/1432293003/#ps60001
(title: "Addressed feedback from patch set 3.")

[commit-bot: I haz the power, 2015-11-17 15:00:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1432293003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1432293003/60001

[commit-bot: I haz the power, 2015-11-17 15:06:08]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-17 15:07:27]

Patchset 4 (id:??) landed as
https://crrev.com/83deb42017ace10596869a7f9200cd80943492a4
Cr-Commit-Position: refs/heads/master@{#360076}