Updated *.pak file format to support both UTF8 and UTF16

Updated *.pak file format to support both UTF8 and UTF16

Inserted a new field in the header that specifies which encoding is to be used for the text resources.

I also upped file format to version 4.

BUG=76281
TEST=unit_tests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=100973

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=101213

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=101846

[marcvs, 2011-08-29 19:46:57]

We should also decide on how to select which languages to encode in UTF16 and
which ones in UTF8. According to the results spreadsheet, there's not very many
of them, and we could hardcode a list, plus fallback to UTF8 for those not on
the list.

[tony, 2011-08-29 21:24:39]

http://codereview.chromium.org/7744017/diff/10001/chrome/browser/themes/brows...
File chrome/browser/themes/browser_theme_pack.cc (right):

http://codereview.chromium.org/7744017/diff/10001/chrome/browser/themes/brows...
chrome/browser/themes/browser_theme_pack.cc:32: const int kThemePackVersion =
17;
Please bump the kThemePackVersion to 18.

http://codereview.chromium.org/7744017/diff/10001/tools/data_pack/data_pack.py
File tools/data_pack/data_pack.py (right):

http://codereview.chromium.org/7744017/diff/10001/tools/data_pack/data_pack.p...
tools/data_pack/data_pack.py:14: # encoding of text resources)
Can we use something smaller than an uint32 for the encoding?  Maybe an int16 or
int8 (ie., unsigned char)?

http://codereview.chromium.org/7744017/diff/10001/tools/data_pack/data_pack.p...
tools/data_pack/data_pack.py:17: UTF16 = 2
BINARY, UTF8, UTF16 = range(3)

http://codereview.chromium.org/7744017/diff/10001/tools/data_pack/data_pack.p...
tools/data_pack/data_pack.py:23: """Reads a data pack file and returns a
dictionary."""
It now returns a tuple.  I would maybe make a small class that holds the
resources and encoding.

http://codereview.chromium.org/7744017/diff/10001/tools/data_pack/repack.py
File tools/data_pack/repack.py (right):

http://codereview.chromium.org/7744017/diff/10001/tools/data_pack/repack.py#n...
tools/data_pack/repack.py:30: if (encoding is None or encoding is
data_pack.BINARY):
Nit: no () around the condition and I would write it as:
if encoding in (None, data_pack.BINARY):

http://codereview.chromium.org/7744017/diff/10001/tools/data_pack/repack.py#n...
tools/data_pack/repack.py:33: if (new_encoding != data_pack.BINARY and encoding
!= new_encoding):
Nit: no () around the condition and use elif.

http://codereview.chromium.org/7744017/diff/10001/tools/data_pack/repack.py#n...
tools/data_pack/repack.py:40: if (encoding is None):
Nit: no () around the condition

http://codereview.chromium.org/7744017/diff/10001/ui/base/resource/data_pack.cc
File ui/base/resource/data_pack.cc (right):

http://codereview.chromium.org/7744017/diff/10001/ui/base/resource/data_pack....
ui/base/resource/data_pack.cc:191: << ", expected betwee " << BINARY << " and "
<< UTF16;
typo: between

http://codereview.chromium.org/7744017/diff/10001/ui/base/resource/data_pack.h
File ui/base/resource/data_pack.h (right):

http://codereview.chromium.org/7744017/diff/10001/ui/base/resource/data_pack....
ui/base/resource/data_pack.h:63: const uint32 textEncodingType);
nit: TextEncodingType instead of uint32.  Also, no const.

http://codereview.chromium.org/7744017/diff/10001/ui/base/resource/data_pack....
ui/base/resource/data_pack.h:66: uint32 GetTextEncodingType() const { return
text_encoding_type_; }
TextEncodingType as the return type.

http://codereview.chromium.org/7744017/diff/10001/ui/base/resource/data_pack....
ui/base/resource/data_pack.h:76: uint32 text_encoding_type_;
TextEncodingType as the type.

http://codereview.chromium.org/7744017/diff/10001/ui/base/resource/data_pack_...
File ui/base/resource/data_pack_unittest.cc (right):

http://codereview.chromium.org/7744017/diff/10001/ui/base/resource/data_pack_...
ui/base/resource/data_pack_unittest.cc:57: ASSERT_TRUE(DataPack::WritePack(file,
resources, DataPack::UTF16));
Can you have the test write/read the data as UTF16, UTF8 and binary?

http://codereview.chromium.org/7744017/diff/10001/ui/base/resource/resource_b...
File ui/base/resource/resource_bundle_posix.cc (right):

http://codereview.chromium.org/7744017/diff/10001/ui/base/resource/resource_b...
ui/base/resource/resource_bundle_posix.cc:103: msg = UTF8ToUTF16(data);
This is fine for now.  We'll want to probably fix up l10n_util to get the raw
data and do the conversion in a follow up patch (to avoid unnecessary string
conversions).

[marcvs, 2011-08-30 07:58:54]

http://codereview.chromium.org/7744017/diff/10001/chrome/browser/themes/brows...
File chrome/browser/themes/browser_theme_pack.cc (right):

http://codereview.chromium.org/7744017/diff/10001/chrome/browser/themes/brows...
chrome/browser/themes/browser_theme_pack.cc:32: const int kThemePackVersion =
17;
On 2011/08/29 21:24:39, tony wrote:
> Please bump the kThemePackVersion to 18.

Done.

http://codereview.chromium.org/7744017/diff/10001/tools/data_pack/data_pack.py
File tools/data_pack/data_pack.py (right):

http://codereview.chromium.org/7744017/diff/10001/tools/data_pack/data_pack.p...
tools/data_pack/data_pack.py:14: # encoding of text resources)
On 2011/08/29 21:24:39, tony wrote:
> Can we use something smaller than an uint32 for the encoding?  Maybe an int16
or
> int8 (ie., unsigned char)?

Changed to uint8 (== unsigned char).

http://codereview.chromium.org/7744017/diff/10001/tools/data_pack/data_pack.p...
tools/data_pack/data_pack.py:17: UTF16 = 2
On 2011/08/29 21:24:39, tony wrote:
> BINARY, UTF8, UTF16 = range(3)

Done. (also in the other files)

http://codereview.chromium.org/7744017/diff/10001/tools/data_pack/data_pack.p...
tools/data_pack/data_pack.py:23: """Reads a data pack file and returns a
dictionary."""
On 2011/08/29 21:24:39, tony wrote:
> It now returns a tuple.  I would maybe make a small class that holds the
> resources and encoding.

Done.

http://codereview.chromium.org/7744017/diff/10001/tools/data_pack/repack.py
File tools/data_pack/repack.py (right):

http://codereview.chromium.org/7744017/diff/10001/tools/data_pack/repack.py#n...
tools/data_pack/repack.py:30: if (encoding is None or encoding is
data_pack.BINARY):
On 2011/08/29 21:24:39, tony wrote:
> Nit: no () around the condition and I would write it as:
> if encoding in (None, data_pack.BINARY):

Done.

http://codereview.chromium.org/7744017/diff/10001/tools/data_pack/repack.py#n...
tools/data_pack/repack.py:33: if (new_encoding != data_pack.BINARY and encoding
!= new_encoding):
On 2011/08/29 21:24:39, tony wrote:
> Nit: no () around the condition and use elif.

Done.

http://codereview.chromium.org/7744017/diff/10001/tools/data_pack/repack.py#n...
tools/data_pack/repack.py:40: if (encoding is None):
On 2011/08/29 21:24:39, tony wrote:
> Nit: no () around the condition

Done.

http://codereview.chromium.org/7744017/diff/10001/ui/base/resource/data_pack.cc
File ui/base/resource/data_pack.cc (right):

http://codereview.chromium.org/7744017/diff/10001/ui/base/resource/data_pack....
ui/base/resource/data_pack.cc:191: << ", expected betwee " << BINARY << " and "
<< UTF16;
On 2011/08/29 21:24:39, tony wrote:
> typo: between

Done.

http://codereview.chromium.org/7744017/diff/10001/ui/base/resource/data_pack.h
File ui/base/resource/data_pack.h (right):

http://codereview.chromium.org/7744017/diff/10001/ui/base/resource/data_pack....
ui/base/resource/data_pack.h:63: const uint32 textEncodingType);
On 2011/08/29 21:24:39, tony wrote:
> nit: TextEncodingType instead of uint32.  Also, no const.

Done.

http://codereview.chromium.org/7744017/diff/10001/ui/base/resource/data_pack....
ui/base/resource/data_pack.h:66: uint32 GetTextEncodingType() const { return
text_encoding_type_; }
On 2011/08/29 21:24:39, tony wrote:
> TextEncodingType as the return type.

Done.

http://codereview.chromium.org/7744017/diff/10001/ui/base/resource/data_pack....
ui/base/resource/data_pack.h:76: uint32 text_encoding_type_;
On 2011/08/29 21:24:39, tony wrote:
> TextEncodingType as the type.

Done.

http://codereview.chromium.org/7744017/diff/10001/ui/base/resource/data_pack_...
File ui/base/resource/data_pack_unittest.cc (right):

http://codereview.chromium.org/7744017/diff/10001/ui/base/resource/data_pack_...
ui/base/resource/data_pack_unittest.cc:57: ASSERT_TRUE(DataPack::WritePack(file,
resources, DataPack::UTF16));
On 2011/08/29 21:24:39, tony wrote:
> Can you have the test write/read the data as UTF16, UTF8 and binary?

I created parameterized tests with TEST_P and INSTANTIATE_TEST_CASE_P for all
three encodings.

http://codereview.chromium.org/7744017/diff/10001/ui/base/resource/resource_b...
File ui/base/resource/resource_bundle_posix.cc (right):

http://codereview.chromium.org/7744017/diff/10001/ui/base/resource/resource_b...
ui/base/resource/resource_bundle_posix.cc:103: msg = UTF8ToUTF16(data);
On 2011/08/29 21:24:39, tony wrote:
> This is fine for now.  We'll want to probably fix up l10n_util to get the raw
> data and do the conversion in a follow up patch (to avoid unnecessary string
> conversions).

Yes, since GTK uses UTF8 I was saving this for later in an effort to replace
some of those calls, too so that they don't waste cycles doing UTF8->UTF16->UTF8
if the file already provides the adequate encoding.

[tony, 2011-08-30 21:19:57]

Almost done! Just a couple small nits left, then I think we can land this patch.

http://codereview.chromium.org/7744017/diff/15003/tools/data_pack/repack.py
File tools/data_pack/repack.py (right):

http://codereview.chromium.org/7744017/diff/15003/tools/data_pack/repack.py#n...
tools/data_pack/repack.py:23: #new_resources, new_encoding =
data_pack.ReadDataPack(filename)
Nit: You probably meant to remove this line.

http://codereview.chromium.org/7744017/diff/15003/tools/grit/grit/format/data...
File tools/grit/grit/format/data_pack_unittest.py (right):

http://codereview.chromium.org/7744017/diff/15003/tools/grit/grit/format/data...
tools/grit/grit/format/data_pack_unittest.py:22: '\x01\x00\x2a\x00\x00\x00'     
    # index entry 1
Is this still correct?  I would expect these to be 27 now (increment by 1 rather
than 4).

http://codereview.chromium.org/7744017/diff/15003/ui/base/resource/data_pack.cc
File ui/base/resource/data_pack.cc (right):

http://codereview.chromium.org/7744017/diff/15003/ui/base/resource/data_pack....
ui/base/resource/data_pack.cc:78: // Parse the header of the file.
We should probably make sure that kHeaderLength <= mmap_->length() here (a bug
in the existing code).

http://codereview.chromium.org/7744017/diff/15003/ui/base/resource/data_pack....
ui/base/resource/data_pack.cc:94: text_encoding_type_ =
(TextEncodingType)(*ptr_encoding);
Nit: static_cast<TextEncodingType>

http://codereview.chromium.org/7744017/diff/15003/ui/base/resource/data_pack_...
File ui/base/resource/data_pack_unittest.cc (right):

http://codereview.chromium.org/7744017/diff/15003/ui/base/resource/data_pack_...
ui/base/resource/data_pack_unittest.cc:17: public:
Nit: 1 space indent

http://codereview.chromium.org/7744017/diff/15003/ui/base/resource/data_pack_...
ui/base/resource/data_pack_unittest.cc:18: DataPackTest() {}
Nit: 2 space indent

http://codereview.chromium.org/7744017/diff/15003/ui/base/resource/data_pack_...
ui/base/resource/data_pack_unittest.cc:51: DataPack::UTF16));
Nice, I didn't know about these gtest macros.

[marcvs, 2011-08-30 22:17:22]

http://codereview.chromium.org/7744017/diff/15003/tools/data_pack/repack.py
File tools/data_pack/repack.py (right):

http://codereview.chromium.org/7744017/diff/15003/tools/data_pack/repack.py#n...
tools/data_pack/repack.py:23: #new_resources, new_encoding =
data_pack.ReadDataPack(filename)
On 2011/08/30 21:19:58, tony wrote:
> Nit: You probably meant to remove this line.

Done.

http://codereview.chromium.org/7744017/diff/15003/tools/grit/grit/format/data...
File tools/grit/grit/format/data_pack_unittest.py (right):

http://codereview.chromium.org/7744017/diff/15003/tools/grit/grit/format/data...
tools/grit/grit/format/data_pack_unittest.py:22: '\x01\x00\x2a\x00\x00\x00'     
    # index entry 1
On 2011/08/30 21:19:58, tony wrote:
> Is this still correct?  I would expect these to be 27 now (increment by 1
rather
> than 4).

Done.

http://codereview.chromium.org/7744017/diff/15003/ui/base/resource/data_pack.cc
File ui/base/resource/data_pack.cc (right):

http://codereview.chromium.org/7744017/diff/15003/ui/base/resource/data_pack....
ui/base/resource/data_pack.cc:78: // Parse the header of the file.
On 2011/08/30 21:19:58, tony wrote:
> We should probably make sure that kHeaderLength <= mmap_->length() here (a bug
> in the existing code).

Done.

http://codereview.chromium.org/7744017/diff/15003/ui/base/resource/data_pack....
ui/base/resource/data_pack.cc:94: text_encoding_type_ =
(TextEncodingType)(*ptr_encoding);
On 2011/08/30 21:19:58, tony wrote:
> Nit: static_cast<TextEncodingType>

Done.

http://codereview.chromium.org/7744017/diff/15003/ui/base/resource/data_pack_...
File ui/base/resource/data_pack_unittest.cc (right):

http://codereview.chromium.org/7744017/diff/15003/ui/base/resource/data_pack_...
ui/base/resource/data_pack_unittest.cc:17: public:
On 2011/08/30 21:19:58, tony wrote:
> Nit: 1 space indent

Done.

http://codereview.chromium.org/7744017/diff/15003/ui/base/resource/data_pack_...
ui/base/resource/data_pack_unittest.cc:18: DataPackTest() {}
On 2011/08/30 21:19:58, tony wrote:
> Nit: 2 space indent

Done.

[tony, 2011-08-30 22:23:00]

LGTM!  I will try running this through the try bots and landing it for you if it
passes.

[tony, 2011-09-08 22:41:42]

The last patch to remove string resources from chrome.dll got landed so we can
probably try to land this patch again.

[adriansc, 2011-09-08 22:54:04]

On Thu, Sep 8, 2011 at 3:41 PM, <tony@chromium.org> wrote:

> The last patch to remove string resources from chrome.dll got landed so we
> can
> probably try to land this patch again.
>
>
http://codereview.chromium.**org/7744017/<http://codereview.chromium.org/7744...
>

Thank you. I ran into some difficulties with a rietveld bug which delayed me
a little, but as soon as I can get the merging in, I'll rebase again and try
landing it.

[commit-bot: I haz the power, 2011-09-13 21:45:19]

Change committed as 100973

[commit-bot: I haz the power, 2011-09-15 01:15:46]

Change committed as 101213

[levin, 2011-09-15 02:09:22]

On 2011/09/15 01:15:46, I haz the power (commit-bot) wrote:
> Change committed as 101213

This change broke lots of WebKit layout tests on OSX.

You can see the results here:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpec...

I'm rolling this out. Sorry.

Please consider running this though the mac_layout test bot before relanding :).

[levin, 2011-09-15 02:23:02]

I think the link I put in above was broken.

Here's a comma separated list of tests that broke with this change. It is a long
list.

fast/dom/HTMLTextAreaElement/reset-textarea.html,fast/dom/isindex-001.html,fast/dom/isindex-002.html,fast/forms/blankbuttons.html,fast/forms/box-shadow-override.html,fast/forms/button-default-title.html,fast/forms/file-input-direction.html,fast/forms/hidden-input-file.html,fast/forms/input-appearance-height.html,fast/forms/input-file-re-render.html,fast/forms/input-first-letter.html,fast/forms/isindex-placeholder.html,fast/forms/text-style-color.html,fast/forms/validation-message-appearance.html,fast/html/details-no-summary1.html,fast/html/details-no-summary2.html,fast/html/details-no-summary3.html,fast/html/details-no-summary4.html,fast/html/details-remove-summary-1-and-click.html,fast/html/details-remove-summary-1.html,fast/html/details-remove-summary-4-and-click.html,fast/html/details-remove-summary-4.html,fast/html/keygen.html,tables/mozilla/bugs/bug39209.html,tables/mozill

a/bugs/bug4429.html,fast/forms/005.html,fast/forms/input-file-label.html,fast/invalid/residual-style.html,fast/parser/fragment-parser.html,fast/replaced/table-percent-height-text-controls.html,html5lib/runner.html,http/tests/misc/isindex-with-no-form-base-href.html,http/tests/misc/isindex-with-no-form.html