Zip utf-8 file names with encoding bit set.

Zip utf-8 file names with encoding bit set.

This commit sets the 'locale' bit in global directory of a zip which
means that the file name was written in UTF-8. Chromium always writes
and reads zips as UTF-8 so it is not a problem per se but other, more
zip compliant, programs see packed files with wrong paths if this bit
is not set.

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

[hshi1, 2013-12-04 18:30:40]

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

https://codereview.chromium.org/94093003/diff/1/third_party/zlib/google/zip.c...
third_party/zlib/google/zip.cc:63: const unsigned long LANGUAGE_ENCODING_FLAG =
0x1 << 11;
This flag definition seems to come out of nowhere and I couldn't find it in any
existing header files. Is this defined in a spec document?

[João Eiras, 2013-12-04 18:46:12]

On 2013/12/04 18:30:40, hshi1 wrote:
> https://codereview.chromium.org/94093003/diff/1/third_party/zlib/google/zip.cc
> File third_party/zlib/google/zip.cc (right):
> 
>
https://codereview.chromium.org/94093003/diff/1/third_party/zlib/google/zip.c...
> third_party/zlib/google/zip.cc:63: const unsigned long LANGUAGE_ENCODING_FLAG
=
> 0x1 << 11;
> This flag definition seems to come out of nowhere and I couldn't find it in
any
> existing header files. Is this defined in a spec document?

Right, http://www.pkware.com/documents/casestudies/APPNOTE.TXT section 4.4.4,
bit 11
I'll leave a comment in the code.

[hshi1, 2013-12-04 20:11:19]

lgtm

[commit-bot: I haz the power, 2013-12-05 01:48:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaoe@opera.com/94093003/20001

[satorux1, 2013-12-05 03:58:03]

could you write a unit test for this? utf-8 file names were garbled before this
change, right?

[João Eiras, 2013-12-05 09:28:40]

On 2013/12/05 03:58:03, satorux1 wrote:
> could you write a unit test for this? utf-8 file names were garbled before
this
> change, right?

Chromium always interprets the file names as utf-8, so the issue is not
detectable in chromium. Setting that bit is something for other programs not to
mangle the file name when reading the file. How would you like to test this ?

[satorux1, 2013-12-05 09:35:23]

I see. If it's not testable, then it's fine not to have a test. LGTM with a
request:

https://codereview.chromium.org/94093003/diff/20001/third_party/zlib/google/z...
File third_party/zlib/google/zip.cc (right):

https://codereview.chromium.org/94093003/diff/20001/third_party/zlib/google/z...
third_party/zlib/google/zip.cc:74: NULL, 0, 0, LANGUAGE_ENCODING_FLAG)) {
NULL, 0, 0 look cryptic. could you add some comment? While you are at, could you
also rearrange the existing parameters? It'd be easier to read if parameters are
arranged vertically with inline comments:

zipOpenNewFileInZip4(
  zip_file,
  str_path.c_str(),
  NULL,  // file_info.
  NULL,  // extrafield local,
  ...

[João Eiras, 2013-12-05 12:45:44]

> NULL, 0, 0 look cryptic. could you add some comment? While you are at, could
you
> also rearrange the existing parameters? It'd be easier to read if parameters
are
> arranged vertically with inline comments:
> 
> zipOpenNewFileInZip4(
>   zip_file,
>   str_path.c_str(),
>   NULL,  // file_info.
>   NULL,  // extrafield local,
>   ...

Looks good ?

[satorux1, 2013-12-05 22:28:09]

lgtm. thanks!

[commit-bot: I haz the power, 2013-12-06 00:50:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaoe@opera.com/94093003/40001

[commit-bot: I haz the power, 2013-12-06 04:56:06]

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

[commit-bot: I haz the power, 2013-12-06 13:10:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaoe@opera.com/94093003/40001

[commit-bot: I haz the power, 2013-12-06 14:41:48]

Change committed as 239216