Use icudat*.dat file on Mac

Use icudat*.dat file on Mac

1. Set icu_use_data_file_flag to 1 in common.gypi
2. Add icudatl.dat to the resournce bundle list in
chrome_dll_bundle.gypi and content_shell.gypi
3. Move ICU_UTIL_DATA_IMPL to base.gyp

This CL has to be landed after
https://codereview.chromium.org/111723007/ is landed and rolled in. 
(done in https://codereview.chromium.org/118313004/ )

Note to perf-sheriff: This CL adds ~10MB to the resource bundle while cutting down the same amount from the Chrome executable/binary.  

BUG=72633
TEST=All the Mac builds (static, shared) go through and tests (e.g.
layout tests, base_unittests --gtest_filter=*String*, net_unittests
--gtest_filter=*IDN*) loading the ICU data pass.

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

[jungshik at Google, 2013-12-18 17:53:28]

Hi Mark, 

It's still WIP and didn't work if icu_use_data_file is NOT set to 1 (current
setting). 

Adding 'conditions' with icu_use_data_file_flag in two gypis doesn't results in
what I want (only bundle icudtl.dat when that flag is 1 and do not bundle it
otherwise). Using 'target_conditions' in two gypi files leads to an undefined
variable error.

[jungshik at Google, 2013-12-18 17:57:56]

On 2013/12/18 17:53:28, Jungshik Shin wrote:
> Hi Mark, 
> 
> It's still WIP and didn't work if icu_use_data_file is NOT set to 1 (current
> setting). 

patch set 1 works with icu_use_data_file_flag=1 but does not work with
icu_use_data_file_flag=0 because icudtl.dat is bundled in 
both cases as opposed to only in the former case. 

To resolve that issue, I added 'conditions' to check icu_use_data_file in two
gypi files (patch set 2), but it's the opposite effect. icudtl.dat is NOT
bundled in even whtn icu_use_data_file_flag=1. 

Trying target_conditions didn't work either because gyp complains about an
undefined variable (icu_use_data_file_flag). (I thought they'd get from
common.gypi, but that does not seem to be the case). 

Could you help me with getting out of this? Thanks

> 
> Adding 'conditions' with icu_use_data_file_flag in two gypis doesn't results
in
> what I want (only bundle icudtl.dat when that flag is 1 and do not bundle it
> otherwise). Using 'target_conditions' in two gypi files leads to an undefined
> variable error.

[Mark Mentovai, 2013-12-18 19:04:04]

When you’re trying to change the value of this variable, how are you doing it?
-D on the command line or the GYP_DEFINES environment variable?
~/.gyp/include.gypi file? Changing the values directly in common.gypi?

https://codereview.chromium.org/109013004/diff/20001/base/i18n/icu_util.cc
File base/i18n/icu_util.cc (right):

https://codereview.chromium.org/109013004/diff/20001/base/i18n/icu_util.cc#ne...
base/i18n/icu_util.cc:36: #define ICU_UTIL_DATA_FILE_NAME "icudtl.dat"
For my own edification: what’s the l stand for?

https://codereview.chromium.org/109013004/diff/20001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/109013004/diff/20001/build/common.gypi#newcode12
build/common.gypi:12: # Putting a variables dict inside another variables dict
looks kind of
At first, I thought that this comment might be what your problem was, but now I
don’t think so. For reference, I was going to suggest doing roughly

  'variables': {
    'variables': {
      'icu_use_data_file_flag%': 0,
    },
    'icu_use_data_file_flag%': '<(icu_use_data_file_flag)',
    'conditions': [
      ['OS=="mac"', {
        'icu_use_data_file_flag%': 1,
      }],
    ],
  },

but I don’t think that would make a difference, because you don’t use
icu_use_data_file_flag in a conditions block in a context where it might be a
problem.

https://codereview.chromium.org/109013004/diff/20001/chrome/chrome_dll_bundle...
File chrome/chrome_dll_bundle.gypi (right):

https://codereview.chromium.org/109013004/diff/20001/chrome/chrome_dll_bundle...
chrome/chrome_dll_bundle.gypi:72: ['icu_use_data_file_flag', {
It’s more usual to write

  icu_use_data_file_flag!=0

https://codereview.chromium.org/109013004/diff/20001/chrome/chrome_dll_bundle...
chrome/chrome_dll_bundle.gypi:74:
'<(DEPTH)/third_party/icu/source/data/in/icudtl.dat',
Did you have something else copy the data file to <(PRODUCT_DIR) already? That
might be a more convenient place to pick it up.

Same in content_shell.gypi.

[Mark Mentovai, 2013-12-18 19:08:13]

And are you getting the correct value for ICU_UTIL_DATA_IMPL?

[jungshik at Google, 2013-12-18 19:53:19]

Thanks for looking into it. I'll keep trying. :-)

https://codereview.chromium.org/109013004/diff/20001/base/i18n/icu_util.cc
File base/i18n/icu_util.cc (right):

https://codereview.chromium.org/109013004/diff/20001/base/i18n/icu_util.cc#ne...
base/i18n/icu_util.cc:36: #define ICU_UTIL_DATA_FILE_NAME "icudtl.dat"
On 2013/12/18 19:04:04, Mark Mentovai wrote:
> For my own edification: what’s the l stand for?
'l' is for Little Endian.  
I got this part out of this CL and make a separate CL with icu roll (which now
has icudtl.dat instead of icudt46l.dat) [1]. After that CL (icu roll and data
file name change) is landed, we can land mac, linux and windows CL one by one. 

[1] https://codereview.chromium.org/118313004/

https://codereview.chromium.org/109013004/diff/20001/chrome/chrome_dll_bundle...
File chrome/chrome_dll_bundle.gypi (right):

https://codereview.chromium.org/109013004/diff/20001/chrome/chrome_dll_bundle...
chrome/chrome_dll_bundle.gypi:72: ['icu_use_data_file_flag', {
On 2013/12/18 19:04:04, Mark Mentovai wrote:
> It’s more usual to write
> 
>   icu_use_data_file_flag!=0

will do

https://codereview.chromium.org/109013004/diff/20001/chrome/chrome_dll_bundle...
chrome/chrome_dll_bundle.gypi:74:
'<(DEPTH)/third_party/icu/source/data/in/icudtl.dat',
On 2013/12/18 19:04:04, Mark Mentovai wrote:
> Did you have something else copy the data file to <(PRODUCT_DIR) already? That
> might be a more convenient place to pick it up.
> 
> Same in content_shell.gypi.

There's a mac_bundle_resources {} right above my addition that bundles a bunch
of files. In patch set 1, I just added icudtl.dat to that block, which obviously
has a problem of copying icudtl.dat even when icu_use_data_file_flag == 0. 

To avoid that, I tried 'conditions' and 'target_conditions', but the former
didn't work (icu*dat is not copied) and the latter led to an undefined variable
error.

[jungshik at Google, 2013-12-18 19:56:50]

On 2013/12/18 19:08:13, Mark Mentovai wrote:
> And are you getting the correct value for ICU_UTIL_DATA_IMPL?

Yes, I'm. When icu_use_data_file_flag is set to 1, I get the correct value and
icu_initialize() looks for icu*.dat. 

> When you’re trying to change the value of this variable, how are you doing it?
> -D on the command line or the GYP_DEFINES environment variable?
> ~/.gyp/include.gypi file? Changing the values directly in common.gypi?

I changed the value directly in common.gypi as you can see in this CL. I haven't
tried '-D' or GYP_DEFINES.

[Mark Mentovai, 2013-12-18 20:02:57]

I took a second look and spotted the problem.

https://codereview.chromium.org/109013004/diff/20001/chrome/chrome_dll_bundle...
File chrome/chrome_dll_bundle.gypi (right):

https://codereview.chromium.org/109013004/diff/20001/chrome/chrome_dll_bundle...
chrome/chrome_dll_bundle.gypi:228: 'conditions': [
This overwrites the “conditions” block on line 71.

https://codereview.chromium.org/109013004/diff/20001/content/content_shell.gypi
File content/content_shell.gypi (right):

https://codereview.chromium.org/109013004/diff/20001/content/content_shell.gy...
content/content_shell.gypi:433: 'conditions': [
This overwrites the “conditions” block on line 407.

[Mark Mentovai, 2013-12-18 20:16:07]

https://codereview.chromium.org/109013004/diff/20001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/109013004/diff/20001/build/common.gypi#newcod...
build/common.gypi:2210: 'defines': ['ICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_FILE'],
Thinking about this some more, it really only needs to be defined when building
base/i18n/icu_util.cc. You can move this block into base/base.gyp for the
base_i18n target only. There’s no reason to set it for the rest of the Chrome
build, we’re just promoting more unnecessary widespread macro bloat.

[jungshik at Google, 2013-12-18 21:39:22]

On 2013/12/18 20:02:57, Mark Mentovai wrote:
> I took a second look and spotted the problem.
> 
>
https://codereview.chromium.org/109013004/diff/20001/chrome/chrome_dll_bundle...
> File chrome/chrome_dll_bundle.gypi (right):
> 
>
https://codereview.chromium.org/109013004/diff/20001/chrome/chrome_dll_bundle...
> chrome/chrome_dll_bundle.gypi:228: 'conditions': [
> This overwrites the “conditions” block on line 71.
> 
>
https://codereview.chromium.org/109013004/diff/20001/content/content_shell.gypi
> File content/content_shell.gypi (right):
> 
>
https://codereview.chromium.org/109013004/diff/20001/content/content_shell.gy...
> content/content_shell.gypi:433: 'conditions': [
> This overwrites the “conditions” block on line 407.

Great ! Thanks a lot. Now it works !

[jungshik at Google, 2013-12-18 21:39:45]

On 2013/12/18 20:16:07, Mark Mentovai wrote:
> https://codereview.chromium.org/109013004/diff/20001/build/common.gypi
> File build/common.gypi (right):
> 
>
https://codereview.chromium.org/109013004/diff/20001/build/common.gypi#newcod...
> build/common.gypi:2210: 'defines': ['ICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_FILE'],
> Thinking about this some more, it really only needs to be defined when
building
> base/i18n/icu_util.cc. You can move this block into base/base.gyp for the
> base_i18n target only. There’s no reason to set it for the rest of the Chrome
> build, we’re just promoting more unnecessary widespread macro bloat.

Ok. I'll move it. Thanks.

[jungshik at Google, 2013-12-19 00:41:46]

On 2013/12/18 21:39:45, Jungshik Shin wrote:
> On 2013/12/18 20:16:07, Mark Mentovai wrote:
> > https://codereview.chromium.org/109013004/diff/20001/build/common.gypi
> > File build/common.gypi (right):
> > 
> >
>
https://codereview.chromium.org/109013004/diff/20001/build/common.gypi#newcod...
> > build/common.gypi:2210: 'defines':
['ICU_UTIL_DATA_IMPL=ICU_UTIL_DATA_FILE'],
> > Thinking about this some more, it really only needs to be defined when
> building
> > base/i18n/icu_util.cc. You can move this block into base/base.gyp for the
> > base_i18n target only. There’s no reason to set it for the rest of the
Chrome
> > build, we’re just promoting more unnecessary widespread macro bloat.
> 
> Ok. I'll move it. Thanks.

Moved it. Can you take another look? Thanks

[jungshik at Google, 2013-12-19 00:42:37]

avi@chromium.org: Please review/owner-approve changes in content_shell.gypi.
Thanks

[Avi (use Gerrit), 2013-12-19 07:14:28]

On 2013/12/19 00:42:37, Jungshik Shin wrote:
> avi@chromium.org: Please review/owner-approve changes in content_shell.gypi.
> Thanks

Once Mark LGs I will stamp.

[Mark Mentovai, 2013-12-19 14:51:17]

https://codereview.chromium.org/109013004/diff/50001/chrome/chrome_dll_bundle...
File chrome/chrome_dll_bundle.gypi (right):

https://codereview.chromium.org/109013004/diff/50001/chrome/chrome_dll_bundle...
chrome/chrome_dll_bundle.gypi:228: 'conditions': [
This “conditions” block still overwrites the one on line 71.

https://codereview.chromium.org/109013004/diff/50001/content/content_shell.gypi
File content/content_shell.gypi (right):

https://codereview.chromium.org/109013004/diff/50001/content/content_shell.gy...
content/content_shell.gypi:436: 'conditions': [
And this, the one on line 410.

[jungshik at Google, 2013-12-20 11:45:49]

On 2013/12/19 14:51:17, Mark Mentovai wrote:
>
https://codereview.chromium.org/109013004/diff/50001/chrome/chrome_dll_bundle...
> File chrome/chrome_dll_bundle.gypi (right):
> 
>
https://codereview.chromium.org/109013004/diff/50001/chrome/chrome_dll_bundle...
> chrome/chrome_dll_bundle.gypi:228: 'conditions': [
> This “conditions” block still overwrites the one on line 71.
> 
>
https://codereview.chromium.org/109013004/diff/50001/content/content_shell.gypi
> File content/content_shell.gypi (right):
> 
>
https://codereview.chromium.org/109013004/diff/50001/content/content_shell.gy...
> content/content_shell.gypi:436: 'conditions': [
> And this, the one on line 410.

Oops.  Shuffling multiple branches across machines, I  must have uploaded from a
wrong branch (because I did test with the issue resolved). Anyway, I've just
updated the CL.

[Mark Mentovai, 2013-12-20 14:27:34]

LGTM. I think you can use <(PRODUCT_DIR) now that you fixed the gyp files in
ICU. But if you still can’t for some reason, use .. instead of <(DEPTH).

https://codereview.chromium.org/109013004/diff/90001/chrome/chrome_dll_bundle...
File chrome/chrome_dll_bundle.gypi (right):

https://codereview.chromium.org/109013004/diff/90001/chrome/chrome_dll_bundle...
chrome/chrome_dll_bundle.gypi:313:
'<(DEPTH)/third_party/icu/source/data/in/icudtl.dat',
You should specify this path using ../third_party/icu/source/data/in/icudtl.dat
instead of <(DEPTH). But haven’t you copied it to <(PRODUCT_DIR) in the patch
that updated ICU? If so, you can just get it from there:
<(PRODUCT_DIR)/icudtl.dat.

https://codereview.chromium.org/109013004/diff/90001/content/content_shell.gypi
File content/content_shell.gypi (right):

https://codereview.chromium.org/109013004/diff/90001/content/content_shell.gy...
content/content_shell.gypi:479:
'<(DEPTH)/third_party/icu/source/data/in/icudtl.dat',
Same.

[Avi (use Gerrit), 2013-12-20 16:27:10]

LGTM stamp now that Mark is happy.

[jungshik at Google, 2014-01-03 21:00:40]

On 2013/12/20 16:27:10, Avi wrote:
> LGTM stamp now that Mark is happy.

Thank you, Mark and Avi. I'll land this CL (updated per Mark's last review
comment about DEPTH vs PRODUCT_DIR) after
https://codereview.chromium.org/124143002/ (a pre-requisite to this CL) is
committed.

[commit-bot: I haz the power, 2014-01-21 23:48:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jshin@chromium.org/109013004/150001

[commit-bot: I haz the power, 2014-01-22 02:53:39]

Retried try job too often on mac_rel for step(s) app_list_unittests,
cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git,
chromedriver_unittests, components_unittests, content_browsertests,
content_unittests, crypto_unittests, google_apis_unittests, gpu_unittests,
ipc_tests, jingle_unittests, media_unittests, message_center_unittests,
nacl_integration, ppapi_unittests, printing_unittests, remoting_unittests,
sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[jungshik at Google, 2014-01-22 19:23:18]

On 2014/01/22 02:53:39, I haz the power (commit-bot) wrote:
> Retried try job too often on mac_rel for step(s) app_list_unittests,
> cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git,
> chromedriver_unittests, components_unittests, content_browsertests,
> content_unittests, crypto_unittests, google_apis_unittests, gpu_unittests,
> ipc_tests, jingle_unittests, media_unittests, message_center_unittests,
> nacl_integration, ppapi_unittests, printing_unittests, remoting_unittests,
> sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_unittests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

content_browsertests fails because it looks for icudtl.dat in "Content
Shell.app/Contents/Frameworks/Content Shell Framework.framework" while
icudtl.dat is copied to Content\ Shell.app/Contents/Resources.  Content Shell
also looks for icudtl.dat in the same localion and fails, too. 

OTOH, chromium is fine. icudtl.dat is copied where it looks for :
Chromium.app/Contents/Versions/34.0.1799.0/Chromium
Framework.framework/Resources/

Apparently, I need to change content_shell.gypi to copy icudtl.dat to the
correct location.  I missed this because I have tried running chrome but not
content_shell. (I thought I had done that, but apparently I just checked
icudtl.dat is coped soemwhere under Content Shell.app).

[jungshik at Google, 2014-01-22 19:38:41]

https://codereview.chromium.org/109013004/diff/150001/content/content_shell.gypi
File content/content_shell.gypi (right):

https://codereview.chromium.org/109013004/diff/150001/content/content_shell.g...
content/content_shell.gypi:481: }],
I'll move this under content_shell_framework target

[jungshik at Google, 2014-01-22 20:24:30]

On 2014/01/22 19:38:41, Jungshik Shin wrote:
>
https://codereview.chromium.org/109013004/diff/150001/content/content_shell.gypi
> File content/content_shell.gypi (right):
> 
>
https://codereview.chromium.org/109013004/diff/150001/content/content_shell.g...
> content/content_shell.gypi:481: }],
> I'll move this under content_shell_framework target

Done the above. I'll add the CL to CQ once mac_rel:content_browsertests succeeds
locally and on a trybot.

[jungshik at Google, 2014-01-22 20:48:18]

On 2014/01/22 20:24:30, Jungshik Shin wrote:
> On 2014/01/22 19:38:41, Jungshik Shin wrote:
> >
>
https://codereview.chromium.org/109013004/diff/150001/content/content_shell.gypi
> > File content/content_shell.gypi (right):
> > 
> >
>
https://codereview.chromium.org/109013004/diff/150001/content/content_shell.g...
> > content/content_shell.gypi:481: }],
> > I'll move this under content_shell_framework target
> 
> Done the above. I'll add the CL to CQ once mac_rel:content_browsertests
succeeds
> locally and on a trybot.

Mark, do I have to do anything for mac installer (in chrome/installer/mac) for a
new file (icudtl.dat)? I guess not because it's a part of the resource bundle.

[commit-bot: I haz the power, 2014-01-27 23:59:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jshin@chromium.org/109013004/440001

[commit-bot: I haz the power, 2014-01-28 06:00:06]

Change committed as 247404