Move the logic for getting icu data out of icu_util

Move the logic for getting icu data out of icu_util

Currently, how to build/link the icu data is split between
common.gypi and icu_util.cc

Move it out of icu_util.cc and put it in common.gypi

Also, remove icudata from the dependency list in url.gyp. icuuc depends
on icudata and specifying icuuc alone is sufficient. Otherwise, ninja
complains about multiple rules specified for icudata and a circular
dependency when icu_use_data_file_flag is set to 1 on Linux.

A similar change has to be made in
third_party/WebKit/Source/web/web.gyp. 
(see https://codereview.chromium.org/93053003/ )

This CL does not change the actual build process, yet. It's just to prepare
to switch to "icu*.dat" on Mac and Linux.

BUG=72633
TEST=All the configuration/builds go fine on all platforms.

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

[jungshik at Google, 2013-11-27 23:50:59]

This is just a prep for an actual fix. 

Setting icu_use_data_file_flag to 1 on Linux still does not work (this CL leave
it to be 0) due to what Ninja claims to be a circular dependency (for which I
have yet to figure out the reason). 

In the meantime, could you take a look at this? Thanks

[jungshik at Google, 2013-11-28 01:33:12]

On 2013/11/27 23:50:59, Jungshik Shin wrote:
> This is just a prep for an actual fix. 
> 
> Setting icu_use_data_file_flag to 1 on Linux still does not work (this CL
leave
> it to be 0) due to what Ninja claims to be a circular dependency (for which I
> have yet to figure out the reason). 
> 
> In the meantime, could you take a look at this? Thanks

With the PS 6, setting icu_use_data_file_flag to 1 on Linux works fine. (alogn
with a webkit change).

[TVL, 2013-12-02 14:15:55]

Handing off to Mark.  I've been away from this too long to safely speak on
changes for all platforms.

[Mark Mentovai, 2013-12-02 17:24:53]

LGTM otherwise

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

https://codereview.chromium.org/89863002/diff/100001/base/i18n/icu_util.cc#ne...
base/i18n/icu_util.cc:102: if (data_path.empty()) {
This check can stay inside the OS_MACOSX #ifdef. It’s the equivalent of the
DCHECK(path_ok) in the non-OS_MACOSX case. It can’t be empty at this point for
non-OS_MACOSX.

https://codereview.chromium.org/89863002/diff/100001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/89863002/diff/100001/build/common.gypi#newcod...
build/common.gypi:1222: 'icu_use_data_file_flag%': 0,
_flag is kind of obvious, I’d just call it icu_use_data_file.

https://codereview.chromium.org/89863002/diff/100001/build/common.gypi#newcod...
build/common.gypi:2165: 'defines': ['ICU_UTIL_DATA_IMPL=0'], # use 'dat' file
You can say “=ICU_UTIL_DATA_FILE” instead of “=0” here. Similar on lines 2169
and 2171. That’s self-documenting and eliminates the need for the comment.

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

Thank you, Mark, for the review. I took care of your comments except for
'_flag'. 

Are you ok with "_flag" which seems necessary to avoid a relative path munging
(see the old CL description copied below)?  I tried to come up with something
that does not end with 'file', but couldn't.

https://codereview.chromium.org/89863002/diff/100001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/89863002/diff/100001/build/common.gypi#newcod...
build/common.gypi:1222: 'icu_use_data_file_flag%': 0,
On 2013/12/02 17:24:53, Mark Mentovai wrote:
> _flag is kind of obvious, I’d just call it icu_use_data_file.

I also wondered why icu.gyp has that and found that it's added in
https://codereview.chromium.org/6474038 by Thomas for the following reason:

"Rename the variable to not end in "_file" since that triggers gyp relative
path munging when you try to set it in common.gypi."

I guess it's still the case.  (I haven't tried without _flag, yet)

[Mark Mentovai, 2013-12-02 18:48:38]

Oh, that’s right. OK, LGTM as-is, then.

[jungshik at Google, 2013-12-02 18:58:25]

Thanks, Mark. 

Adding abarth for the owner's review of url.gyp

[abarth-chromium, 2013-12-02 20:19:09]

url.gyp <--- LGTM

[commit-bot: I haz the power, 2013-12-03 19:25:23]

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

[commit-bot: I haz the power, 2013-12-03 23:43:02]

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

[commit-bot: I haz the power, 2013-12-04 04:37:08]

Change committed as 238567

[Jiang Jiang, 2013-12-04 10:43:35]

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

https://codereview.chromium.org/89863002/diff/140001/base/i18n/icu_util.cc#ne...
base/i18n/icu_util.cc:89: // Assume it is in the framework bundle's Resources
directory.
This comment should be moved down to close to that
base::mac::PathForFrameworkBundleResource() call.

[Jiang Jiang, 2013-12-04 10:45:41]

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

https://codereview.chromium.org/89863002/diff/140001/base/i18n/icu_util.cc#ne...
base/i18n/icu_util.cc:100:
base::mac::PathForFrameworkBundleResource(CFSTR(ICU_UTIL_DATA_FILE_NAME));
Wrong indent by the way.

I also wonder how it can work out for non-bundled executables such as test
cases. Do Chromium always set the override framework bundle path for test cases
such as base_unittests?

[jungshik at Google, 2013-12-09 05:54:46]

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

https://codereview.chromium.org/89863002/diff/140001/base/i18n/icu_util.cc#ne...
base/i18n/icu_util.cc:89: // Assume it is in the framework bundle's Resources
directory.
On 2013/12/04 10:43:35, Jiang wrote:
> This comment should be moved down to close to that
> base::mac::PathForFrameworkBundleResource() call.

Good catch. thanks.

https://codereview.chromium.org/89863002/diff/140001/base/i18n/icu_util.cc#ne...
base/i18n/icu_util.cc:100:
base::mac::PathForFrameworkBundleResource(CFSTR(ICU_UTIL_DATA_FILE_NAME));
On 2013/12/04 10:45:42, Jiang wrote:
> Wrong indent by the way.
> 
> I also wonder how it can work out for non-bundled executables such as test
> cases. Do Chromium always set the override framework bundle path for test
cases
> such as base_unittests?

Thank you for the comment. 
I don't know the answer. I'm not familiar with the way Mac executables are
built. Let's figure out a way to work in all cases in bug 72633.