Enable icu_use_data_file_flag on Android

Enable icu_use_data_file_flag on Android

0. Roll icu to r249466 to copy Android-specific icudtl.dat
1. Turn it in build/common.gypi by default except for android webview build. Move it inside L2 var dict and pull it up to the top level var dict to make 'Google Chrome' build on Android happy. 
2. Add an entry for icudtl.dat to chrome/chrome_android_paks.gypi so that
it's copied to assets directory for Chrome and related targets. 
3. Extract icudtl.dat from the asset and copy to DIR_ANDROID_APP_DATA and make it world-readable so that child processes (sandboxed with uid) can read it. 
4. Add icudtl.dat to MANDATORY_PAKS list in various Android build targets 

Google Chrome has #4 but in a separate CL ( https://chrome-internal-review.googlesource.com/#/c/155554/ ). That CL will land before this CL. 

This also depends on a Blink change https://codereview.chromium.org/183013006/ 

TBR=avi@chromium.org
BUG=72633
TEST=base_unittests:*Convers*, net_unittests:*IDN*, browser tests on all platforms.
TEST=On Android, 1. Layout tests 
2. build/android/test_runner.py gtest -s webkit_unit_tests 
--gtest_filter=WebFrameTest.SelectRange*

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

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

[Andrew Hayden (chromium.org), 2014-02-06 18:06:54]

LGTM
+miguelg +torne as FYI

[jungshik at Google, 2014-02-07 00:47:52]

On 2014/02/06 18:06:54, Andrew Hayden wrote:
> LGTM
> +miguelg +torne as FYI

Thanks, but it's not working yet. chromium shell and content shell crash
immeditately apparently because it cannot find icudtl.dat (InitializeICU
failure). icudtl.dat is put along side *pak files (in assets). Should I put
somewhere else. InitializeICU is look for icudtl.dat in DIR_ANDROID_APP_DATA
(where I believe locale pak files are installed). 

I've tried to use gdb, but didn't work because somehow it crashes before I get a
gdb prompt. Aha.. Using a debug build, I may have a better luck.

[jungshik at Google, 2014-02-07 01:34:25]

On 2014/02/07 00:47:52, Jungshik Shin wrote:
> On 2014/02/06 18:06:54, Andrew Hayden wrote:
> > LGTM
> > +miguelg +torne as FYI
> 
> Thanks, but it's not working yet. chromium shell and content shell crash
> immeditately apparently because it cannot find icudtl.dat (InitializeICU
> failure). icudtl.dat is put along side *pak files (in assets). Should I put
> somewhere else. InitializeICU is look for icudtl.dat in DIR_ANDROID_APP_DATA
> (where I believe locale pak files are installed). 

OTOH, various unittests all pass on trybots. 

> 
> I've tried to use gdb, but didn't work because somehow it crashes before I get
a
> gdb prompt. Aha.. Using a debug build, I may have a better luck.

I'll also use 'adb logcat' to capture the log from InitializeICU to figure out
where it's looking for icudtl.dat.

[jungshik at Google, 2014-02-07 19:58:36]

On 2014/02/07 01:34:25, Jungshik Shin wrote:
> On 2014/02/07 00:47:52, Jungshik Shin wrote:
> > On 2014/02/06 18:06:54, Andrew Hayden wrote:
> > > LGTM
> > > +miguelg +torne as FYI
> > 
> > Thanks, but it's not working yet. chromium shell and content shell crash
> > immeditately apparently because it cannot find icudtl.dat (InitializeICU
> > failure). icudtl.dat is put along side *pak files (in assets). Should I put
> > somewhere else. InitializeICU is look for icudtl.dat in DIR_ANDROID_APP_DATA
> > (where I believe locale pak files are installed). 
> 
> OTOH, various unittests all pass on trybots. 
> 
> > 
> > I've tried to use gdb, but didn't work because somehow it crashes before I
get
> a
> > gdb prompt. Aha.. Using a debug build, I may have a better luck.
> 
> I'll also use 'adb logcat' to capture the log from InitializeICU to figure out
> where it's looking for icudtl.dat.

E/chromium( 2616): [ERROR:memory_mapped_file.cc(23)] Couldn't open /data/data/or
g.chromium.chrome.testshell/app_chromiumtestshell/icudtl.dat
E/chromium( 2616): [ERROR:icu_util.cc(117)] Couldn't mmap
/data/data/org.chromium.chrome.testshell/app_chromiumtestshell/icudtl.dat
F/chromium( 2616): [FATAL:content_main_runner.cc(735)] Check failed:
base::i18n::InitializeICU(). 

icudtl.dat is searched for in DIR_ANDROID_APP_DATA that is resolved to
/data/data/org.chromium.chrome.testshell/app_chromiumtestshell/icudtl.dat, but
icudtl.dat is not there.
The directory has paks subdirectory with the following files (a subset of files
I put in assets directory in chrome_android_paks.gypi:

chrome_100_percent.pak
en-US.pak
ko.pak
pak_timestamp-0-1391798692388
resources.pak

I guess ko.pak is there because my Android's UI language is set to Korean.

So, just putting icudtl.dat in assets during the build is not sufficient. We
need to change the Android installer to copy it to the above directory along
with a subset of pak files.

[jungshik at Google, 2014-02-08 01:33:14]

Ben, can you take a look at my attempt to extract icudtl.dat for Android? 

It may not be an optimal place to do so and I'm open to suggestion. 

Moreover, even with this change, base/i18n/icu_util.cc::InitializeICU fails when
it's called from a child process of ChromiumTestShell. The first time, it
succeeds, but the second time on, it fails. I don't know why.

[jungshik at Google, 2014-02-10 18:56:27]

On 2014/02/08 01:33:14, Jungshik Shin wrote:
> Ben, can you take a look at my attempt to extract icudtl.dat for Android? 
> 
> It may not be an optimal place to do so and I'm open to suggestion. 
> 
> Moreover, even with this change, base/i18n/icu_util.cc::InitializeICU fails
when
> it's called from a child process of ChromiumTestShell. The first time, it
> succeeds, but the second time on, it fails. I don't know why.

The reason a renderer process failed to read the data (even though the read is
requested before sandbox is explicitly initialized) os that on Android, the file
has '600' permission (readable only by a browser process). Hmm, should I make it
644 so that renderer processes can read it?  It should be harmless, but I'm not
a security person.

[jungshik at Google, 2014-02-10 20:04:02]

Mark : can you review icu_util.cc and gyp?
Maruel: can you review setup.py on Android? 
Ben : Can you look at ResourceExtractor.java? 

Thanks.

[Mark Mentovai, 2014-02-10 20:07:12]

LGTM

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

https://codereview.chromium.org/156333002/diff/280001/base/i18n/icu_util.cc#n...
base/i18n/icu_util.cc:107: DVLOG(0) << "Looking for the icudata at " <<
data_path.AsUTF8Unsafe();
Necessary, or just spammy? There are already DLOG(ERROR)s if the file can’t be
loaded, so I think that this isn’t very useful.

https://codereview.chromium.org/156333002/diff/280001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/156333002/diff/280001/build/common.gypi#newco...
build/common.gypi:1308: # Should we also disable this when use_system_icu==1 on
Linux?
No tabs.

[M-A Ruel, 2014-02-10 20:10:01]

build/android/pylib/gtest/setup.py rubberstamp lgtm but I'm not an owner in this
directory.

[jungshik at Google, 2014-02-10 20:41:11]

On 2014/02/10 20:07:12, Mark Mentovai wrote:
> LGTM
> 
> https://codereview.chromium.org/156333002/diff/280001/base/i18n/icu_util.cc
> File base/i18n/icu_util.cc (right):
> 
>
https://codereview.chromium.org/156333002/diff/280001/base/i18n/icu_util.cc#n...
> base/i18n/icu_util.cc:107: DVLOG(0) << "Looking for the icudata at " <<
> data_path.AsUTF8Unsafe();
> Necessary, or just spammy? There are already DLOG(ERROR)s if the file can’t be
> loaded, so I think that this isn’t very useful.

You're right. While I was debugging, it's assuring to get no more message after
the one I added (it looked for the file and no other msg means that it's
successful.). With that debug done, it's not so useful any more. Removed it. 

> 
> https://codereview.chromium.org/156333002/diff/280001/build/common.gypi
> File build/common.gypi (right):
> 
>
https://codereview.chromium.org/156333002/diff/280001/build/common.gypi#newco...
> build/common.gypi:1308: # Should we also disable this when use_system_icu==1
on
> Linux?
> No tabs.

Argh... I have to look into my editor conf as to why it keeps adding one without
my knowledge. Removed. Thanks.

[jungshik at Google, 2014-02-10 20:44:33]

@frankf, can you do owner-review of android/pylib/gtest/setup.py? Thanks. 
@maruel, thank you for the alert/lgtm.

[jungshik at Google, 2014-02-10 20:47:13]

bulach@chromium.org: Please review changes in 
ResourceExtractor.java (and gtest/setup.py if you like to).
Thanks.

[benm (inactive), 2014-02-10 21:30:44]

https://codereview.chromium.org/156333002/diff/380001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ResourceExtractor.java
(right):

https://codereview.chromium.org/156333002/diff/380001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ResourceExtractor.java:346:
* false for the WebView build.
i think that if an app using WebView had bundled a file called icudtl.dat that
we'd now extract this to it's data directory?

Also, I think that looking into the AssetManager might be slow (although I don't
have data to back that up), and this happens on the critical app start up path.
Would it be better to set a flag to say if we should extract ICU data, like we
set the mandatory paks? Maybe we just have MANDATORY_RESOURCES that combines the
needed PAKs and other files? we could still do the check on the filename when we
extract above to put the ICU file into the correct place.

[frankf, 2014-02-10 21:47:59]

//build/android/pylib lgtm

[jungshik at Google, 2014-02-10 22:10:30]

Thank you for taking a look, Ben. Below is my answer.

https://codereview.chromium.org/156333002/diff/380001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ResourceExtractor.java
(right):

https://codereview.chromium.org/156333002/diff/380001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ResourceExtractor.java:346:
* false for the WebView build.
On 2014/02/10 21:30:45, benm wrote:
> i think that if an app using WebView had bundled a file called icudtl.dat that
> we'd now extract this to it's data directory?

Although that's not likely, I agree that we'd better be on the safe side. 

> 
> Also, I think that looking into the AssetManager might be slow (although I
don't
> have data to back that up), and this happens on the critical app start up
path.
> Would it be better to set a flag to say if we should extract ICU data, like we
> set the mandatory paks? Maybe we just have MANDATORY_RESOURCES that combines
the
> needed PAKs and other files? we could still do the check on the filename when
we
> extract above to put the ICU file into the correct place.

I'm less worried about the performance than about your 1st concern (although I'm
just a novice here and I may as well completely wrong) given that line 116 uses
AssetManager to list all the files in APK. 

There are 5 files specifying MANDATORY_PAKS ( http://goo.gl/KgHvhZ ). Are they
all non-webview stadalone targets? Then, I have to add icudtl.dat to all 5 of
them.

[jungshik at Google, 2014-02-10 22:26:10]

On 2014/02/10 22:10:30, Jungshik Shin wrote:
> Thank you for taking a look, Ben. Below is my answer.
> 
>
https://codereview.chromium.org/156333002/diff/380001/content/public/android/...
> File
>
content/public/android/java/src/org/chromium/content/browser/ResourceExtractor.java
> (right):
> 
>
https://codereview.chromium.org/156333002/diff/380001/content/public/android/...
>
content/public/android/java/src/org/chromium/content/browser/ResourceExtractor.java:346:
> * false for the WebView build.
> On 2014/02/10 21:30:45, benm wrote:
> > i think that if an app using WebView had bundled a file called icudtl.dat
that
> > we'd now extract this to it's data directory?
> 
> Although that's not likely, I agree that we'd better be on the safe side. 
> 
> > 
> > Also, I think that looking into the AssetManager might be slow (although I
> don't
> > have data to back that up), and this happens on the critical app start up
> path.
> > Would it be better to set a flag to say if we should extract ICU data, like
we
> > set the mandatory paks? Maybe we just have MANDATORY_RESOURCES that combines
> the
> > needed PAKs and other files? we could still do the check on the filename
when
> we
> > extract above to put the ICU file into the correct place.
> 
> I'm less worried about the performance than about your 1st concern (although
I'm
> just a novice here and I may as well completely wrong) given that line 116
uses
> AssetManager to list all the files in APK. 
> 
> There are 5 files specifying MANDATORY_PAKS ( http://goo.gl/KgHvhZ ). Are they
> all non-webview stadalone targets? Then, I have to add icudtl.dat to all 5 of
> them.

BTW, I guess I also have to add icudtl.dat to MANDATORY_PAKS list for Google
Chrome (I don't have an access to that tree. I'll ask for the privilege ). I
think I have to add icudtl.dat to MANDATORY_PAKS for Google Chrome before
landing this CL (because a missing file is ok).

[benm (inactive), 2014-02-10 22:28:32]

https://codereview.chromium.org/156333002/diff/380001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ResourceExtractor.java
(right):

https://codereview.chromium.org/156333002/diff/380001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ResourceExtractor.java:346:
* false for the WebView build.
On 2014/02/10 22:10:31, Jungshik Shin wrote:
> On 2014/02/10 21:30:45, benm wrote:
> > i think that if an app using WebView had bundled a file called icudtl.dat
that
> > we'd now extract this to it's data directory?
> 
> Although that's not likely, I agree that we'd better be on the safe side. 
> 
> > 
> > Also, I think that looking into the AssetManager might be slow (although I
> don't
> > have data to back that up), and this happens on the critical app start up
> path.
> > Would it be better to set a flag to say if we should extract ICU data, like
we
> > set the mandatory paks? Maybe we just have MANDATORY_RESOURCES that combines
> the
> > needed PAKs and other files? we could still do the check on the filename
when
> we
> > extract above to put the ICU file into the correct place.
> 
> I'm less worried about the performance than about your 1st concern (although
I'm
> just a novice here and I may as well completely wrong) given that line 116
uses
> AssetManager to list all the files in APK. 

I think that the other use will only happen in the case that PAK extraction is
actually required, which should be only the first time after a new install or
update. I think this new check will happen on every start up regardless?

> 
> There are 5 files specifying MANDATORY_PAKS ( http://goo.gl/KgHvhZ ). Are they
> all non-webview stadalone targets? Then, I have to add icudtl.dat to all 5 of
> them.

Yes, they are all non-WebView. They all look like test runners - do they all
require the ICU data file?

[jungshik at Google, 2014-02-10 23:41:46]

Thank you for the reply. Inlined is my reply.

https://codereview.chromium.org/156333002/diff/380001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ResourceExtractor.java
(right):

https://codereview.chromium.org/156333002/diff/380001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ResourceExtractor.java:346:
* false for the WebView build.
On 2014/02/10 22:28:32, benm wrote:
> On 2014/02/10 22:10:31, Jungshik Shin wrote:
> > On 2014/02/10 21:30:45, benm wrote:
> > > i think that if an app using WebView had bundled a file called icudtl.dat
> that
> > > we'd now extract this to it's data directory?
> > 
> > Although that's not likely, I agree that we'd better be on the safe side. 
> > 
> > > 
> > > Also, I think that looking into the AssetManager might be slow (although I
> > don't
> > > have data to back that up), and this happens on the critical app start up
> > path.
> > > Would it be better to set a flag to say if we should extract ICU data,
like
> we
> > > set the mandatory paks? Maybe we just have MANDATORY_RESOURCES that
combines
> > the
> > > needed PAKs and other files? we could still do the check on the filename
> when
> > we
> > > extract above to put the ICU file into the correct place.
> > 
> > I'm less worried about the performance than about your 1st concern (although
> I'm
> > just a novice here and I may as well completely wrong) given that line 116
> uses
> > AssetManager to list all the files in APK. 
> 
> I think that the other use will only happen in the case that PAK extraction is
> actually required, which should be only the first time after a new install or
> update. I think this new check will happen on every start up regardless?

That's what I expect us to do, but my reading of the current code (lines 106 ~
125) is that every time it starts, AssetManager is called to get the list of all
the files in assets/, match the list against 'pattern' and check their existence
in the target directory (if exist / already copied/extreacted, do not
copy/extract). 
Am I misreading it? 


> > 
> > There are 5 files specifying MANDATORY_PAKS ( http://goo.gl/KgHvhZ ). Are
they
> > all non-webview stadalone targets? Then, I have to add icudtl.dat to all 5
of
> > them.
> 
> Yes, they are all non-WebView. They all look like test runners - do they all
> require the ICU data file? 


Anything that renders web pages need the icudata unless we use the 'system icu'
(ICU from Android), which is the case of WebView. ICU is used also in the
UI/browser process. 

BTW, is this also non-webview?  

android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellApplication.java

[jungshik at Google, 2014-02-11 19:55:06]

frankf, bulach, cjhopman: do you have any opinion on what I wrote below?

https://codereview.chromium.org/156333002/diff/610001/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/156333002/diff/610001/content/content_tests.g...
content/content_tests.gypi:1450: 'includes': [ '../build/java_apk.gypi' ],
With this PS, content_shell now works. However, I'm not so happy about having to

duplicate the above in 3 different places (twice in this file and once in
content_shell.gypi. Moreover, there are other targets that include java_apk.gypi
that may need the same treatment. 

So, I'm thinking of adding a new variable to java_apk.gypi that controls whether
or not to copy and add icudtl.dat to assets in apk. The default value perhaps
should be 1.

[cjhopman, 2014-02-11 20:54:59]

https://codereview.chromium.org/156333002/diff/610001/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/156333002/diff/610001/content/content_tests.g...
content/content_tests.gypi:1450: 'includes': [ '../build/java_apk.gypi' ],
On 2014/02/11 19:55:06, Jungshik Shin wrote:
> With this PS, content_shell now works. However, I'm not so happy about having
to
> 
> duplicate the above in 3 different places (twice in this file and once in
> content_shell.gypi. Moreover, there are other targets that include
java_apk.gypi
> that may need the same treatment. 
> 
> So, I'm thinking of adding a new variable to java_apk.gypi that controls
whether
> or not to copy and add icudtl.dat to assets in apk. The default value perhaps
> should be 1. 

Hm. I think adding something to java_apk.gypi to make this simpler would be
good, but I would prefer something more general. For example, you could add an
additional_asset_paths variable (or additional_apk_asset_paths or something) to
java_apk.gypi and then have that deal with the copies step. Then you would only
need the conditions block in the variables section. Also, if there is some
native library part that corresponds to this .dat file (i.e. any apk with that
native library would want that .dat) you could use an all_dependent_settings
block there and you wouldn't even need to change the apk targets.

[bulach, 2014-02-12 10:53:19]

lgtm % chris being happy :)

there's a few crashes in the android rel try-bot:
http://build.chromium.org/p/tryserver.chromium/builders/android_rel_triggered...

it looks like it's hitting this assert:
CHECK(base::i18n::InitializeICU());

in content::ContentMainRunnerImpl::Initialize...

please make sure the bots are happy too!

https://codereview.chromium.org/156333002/diff/610001/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/156333002/diff/610001/content/content_tests.g...
content/content_tests.gypi:1450: 'includes': [ '../build/java_apk.gypi' ],
On 2014/02/11 20:55:00, cjhopman wrote:
> On 2014/02/11 19:55:06, Jungshik Shin wrote:
> > With this PS, content_shell now works. However, I'm not so happy about
having
> to
> > 
> > duplicate the above in 3 different places (twice in this file and once in
> > content_shell.gypi. Moreover, there are other targets that include
> java_apk.gypi
> > that may need the same treatment. 
> > 
> > So, I'm thinking of adding a new variable to java_apk.gypi that controls
> whether
> > or not to copy and add icudtl.dat to assets in apk. The default value
perhaps
> > should be 1. 
> 
> Hm. I think adding something to java_apk.gypi to make this simpler would be
> good, but I would prefer something more general. For example, you could add an
> additional_asset_paths variable (or additional_apk_asset_paths or something)
to
> java_apk.gypi and then have that deal with the copies step. Then you would
only
> need the conditions block in the variables section. Also, if there is some
> native library part that corresponds to this .dat file (i.e. any apk with that
> native library would want that .dat) you could use an all_dependent_settings
> block there and you wouldn't even need to change the apk targets.

+1 for encapsulating within java_apk.gypi and +1 for cjhopman's suggestion on
making it generic. :)

[jungshik at Google, 2014-02-13 20:02:03]

On 2014/02/12 10:53:19, bulach wrote:
> lgtm % chris being happy :)

thanks a lot for the review and suggestion, chris and bulach. 

> there's a few crashes in the android rel try-bot:
>
http://build.chromium.org/p/tryserver.chromium/builders/android_rel_triggered...
> 
> it looks like it's hitting this assert:
> CHECK(base::i18n::InitializeICU());
> 
> in content::ContentMainRunnerImpl::Initialize...

Yes, it's a bit mysterious because it happens only in a subset of cases (if it
happens, it has to happen everytime content_shell test is run). Moreover, I
can't reproduce it locally (no crash from InitializeICU which looks for
icudtl.dat file and mmap it). 

> 
> please make sure the bots are happy too!
> 
>
https://codereview.chromium.org/156333002/diff/610001/content/content_tests.gypi
> File content/content_tests.gypi (right):
> 
>
https://codereview.chromium.org/156333002/diff/610001/content/content_tests.g...
> content/content_tests.gypi:1450: 'includes': [ '../build/java_apk.gypi' ],
> On 2014/02/11 20:55:00, cjhopman wrote:
> > On 2014/02/11 19:55:06, Jungshik Shin wrote:
> > > With this PS, content_shell now works. However, I'm not so happy about
> having
> > to
> > > 
> > > duplicate the above in 3 different places (twice in this file and once in
> > > content_shell.gypi. Moreover, there are other targets that include
> > java_apk.gypi
> > > that may need the same treatment. 
> > > 
> > > So, I'm thinking of adding a new variable to java_apk.gypi that controls
> > whether
> > > or not to copy and add icudtl.dat to assets in apk. The default value
> perhaps
> > > should be 1. 
> > 
> > Hm. I think adding something to java_apk.gypi to make this simpler would be
> > good, but I would prefer something more general. For example, you could add
an
> > additional_asset_paths variable (or additional_apk_asset_paths or something)
> to
> > java_apk.gypi and then have that deal with the copies step. Then you would
> only
> > need the conditions block in the variables section. Also, if there is some
> > native library part that corresponds to this .dat file (i.e. any apk with
that
> > native library would want that .dat) you could use an all_dependent_settings
> > block there and you wouldn't even need to change the apk targets.
> 
> +1 for encapsulating within java_apk.gypi and +1 for cjhopman's suggestion on
> making it generic. :)

I like the idea, but there's a caveat. 

3 targets want to copy icudtl.dat in content_shell/assets. In patch set 17, we
set 'additional_asset_path' in those 3 targets and include  java_apk.gypi that
does the actual copying. The end result is like patch set 13 (which copies
icudtl.dat to the same location in 3 different targets) leading ninja to
complain about 'multiple rules generate content_shell/assets/icudtl.dat.  I
should have mentioned that PS 13 had that warnings emitted by ninja. I think
it's harmless, but want to avoid it if possible.
I'm afraid all_dependent_settings added to a native library part (that also
requires .dat) would have the same problem. (moreover, I'm not sure if it's
possible to 
refer to <asset_location> in all_dependent_settings section of that native
library part). 

An alternative is patchset 16. I added a new target 'content_icudata' to
content.gyp and made 3 targets depend on it. It's not generic at all and checks
icu_use_data_file_flag before  copying. OTOH, it does the job without ninja
complaining. 

So, I'm inclined to go with PS #16. Would you take a look again at PS 16?
Thanks.

[jungshik at Google, 2014-02-14 08:03:32]

On 2014/02/13 20:02:03, Jungshik Shin wrote:
> On 2014/02/12 10:53:19, bulach wrote:
> > lgtm % chris being happy :)
> 
> thanks a lot for the review and suggestion, chris and bulach. 
> 
> > there's a few crashes in the android rel try-bot:
> >
>
http://build.chromium.org/p/tryserver.chromium/builders/android_rel_triggered...
> > 
> > it looks like it's hitting this assert:
> > CHECK(base::i18n::InitializeICU());
> > 
> > in content::ContentMainRunnerImpl::Initialize...
> 
> Yes, it's a bit mysterious because it happens only in a subset of cases (if it
> happens, it has to happen everytime content_shell test is run). Moreover, I
> can't reproduce it locally (no crash from InitializeICU which looks for
> icudtl.dat file and mmap it). 
> 
> > 
> > please make sure the bots are happy too!
> > 
> >
>
https://codereview.chromium.org/156333002/diff/610001/content/content_tests.gypi
> > File content/content_tests.gypi (right):
> > 
> >
>
https://codereview.chromium.org/156333002/diff/610001/content/content_tests.g...
> > content/content_tests.gypi:1450: 'includes': [ '../build/java_apk.gypi' ],
> > On 2014/02/11 20:55:00, cjhopman wrote:
> > > On 2014/02/11 19:55:06, Jungshik Shin wrote:
> > > > With this PS, content_shell now works. However, I'm not so happy about
> > having
> > > to
> > > > 
> > > > duplicate the above in 3 different places (twice in this file and once
in
> > > > content_shell.gypi. Moreover, there are other targets that include
> > > java_apk.gypi
> > > > that may need the same treatment. 
> > > > 
> > > > So, I'm thinking of adding a new variable to java_apk.gypi that controls
> > > whether
> > > > or not to copy and add icudtl.dat to assets in apk. The default value
> > perhaps
> > > > should be 1. 
> > > 
> > > Hm. I think adding something to java_apk.gypi to make this simpler would
be
> > > good, but I would prefer something more general. For example, you could
add
> an
> > > additional_asset_paths variable (or additional_apk_asset_paths or
something)
> > to
> > > java_apk.gypi and then have that deal with the copies step. Then you would
> > only
> > > need the conditions block in the variables section. Also, if there is some
> > > native library part that corresponds to this .dat file (i.e. any apk with
> that
> > > native library would want that .dat) you could use an
all_dependent_settings
> > > block there and you wouldn't even need to change the apk targets.
> > 
> > +1 for encapsulating within java_apk.gypi and +1 for cjhopman's suggestion
on
> > making it generic. :)
> 
> I like the idea, but there's a caveat. 
> 
> 3 targets want to copy icudtl.dat in content_shell/assets. In patch set 17, we
> set 'additional_asset_path' in those 3 targets and include  java_apk.gypi that
> does the actual copying. The end result is like patch set 13 (which copies
> icudtl.dat to the same location in 3 different targets) leading ninja to
> complain about 'multiple rules generate content_shell/assets/icudtl.dat.  I
> should have mentioned that PS 13 had that warnings emitted by ninja. I think
> it's harmless, but want to avoid it if possible.
> I'm afraid all_dependent_settings added to a native library part (that also
> requires .dat) would have the same problem. (moreover, I'm not sure if it's
> possible to 
> refer to <asset_location> in all_dependent_settings section of that native
> library part). 
> 
> An alternative is patchset 16. I added a new target 'content_icudata' to
> content.gyp and made 3 targets depend on it. It's not generic at all and
checks
> icu_use_data_file_flag before  copying. OTOH, it does the job without ninja
> complaining. 
> 
> So, I'm inclined to go with PS #16. Would you take a look again at PS 16?
> Thanks.

PS 18 (the latest) is the same as PS 16. 
Can you take another look? Thanks

[Andrew Hayden (chromium.org), 2014-02-14 14:49:03]

+aurimas, who has been playing around with PAK files.

[cjhopman, 2014-02-14 18:38:33]

On 2014/02/14 14:49:03, Andrew Hayden wrote:
> +aurimas, who has been playing around with PAK files.

*.gyp/gypi lgtm

[aurimas (slooooooooow), 2014-02-18 18:13:01]

On 2014/02/14 18:38:33, cjhopman wrote:
> On 2014/02/14 14:49:03, Andrew Hayden wrote:
> > +aurimas, who has been playing around with PAK files.
> 
> *.gyp/gypi lgtm

lgtm

[jungshik at Google, 2014-02-20 00:02:48]

Thank you for the review, bulach, aurimas, cjhopman !

@Mark, can you take another look at build/common.gypi? 

Initially, I just declared and set the default value at L2 var dict and hoisted
out to L1 var dict, after which 'conditions' block is used to set it to 1
conditionally. It didn't work for clank.gyp because icu_use_data_file_flag is
conditionally set at the same level (L1) as it's used in another conditions
block (chrome_android_paks.gypi). 

In the latest PS, I set it to 1 at L2 (if android_webview_build == 0 ; note that
android_webview==1 is the only case where we don't want to use icudtl.dat) and
pull it up to L1. 

(tested on Android. Not yet tested on Linux/Win/Mac/Chrome OS, but should be
fine there). 
Thanks.

[jungshik at Google, 2014-02-20 09:37:59]

yfriedman@chromium.org: Please review changes in content/{shell,public}/android.

[jungshik at Google, 2014-02-20 09:42:03]

avi@chromium.org: Please review / owner-approve changes  in contents/. 

benm@ : can you review android_webview changes as well as ResourceExtractor.java
(I did what you proposed)?  Thank you.

[benm (inactive), 2014-02-20 14:13:06]

android_webview lgtm % nit

thanks!

https://codereview.chromium.org/156333002/diff/1420001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ResourceExtractor.java
(right):

https://codereview.chromium.org/156333002/diff/1420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ResourceExtractor.java:341:
* this process if they call setMandatoryPaksToExtract with a single empty String
*/
nit: add back the period, and */ onto the next line. Undo this diff, basically
:)

[Mark Mentovai, 2014-02-20 14:15:53]

LGTM. For this round, I only looked at the latest changes to build/common.gypi.

https://codereview.chromium.org/156333002/diff/1420001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/156333002/diff/1420001/build/common.gypi#newc...
build/common.gypi:157: 
Extra blank line.

[Yaron, 2014-02-20 18:15:00]

lgtm

https://codereview.chromium.org/156333002/diff/1420001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ResourceExtractor.java
(right):

https://codereview.chromium.org/156333002/diff/1420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ResourceExtractor.java:318:
}
nit: add ws line after

[Andrew Hayden (chromium.org), 2014-02-21 08:49:39]

https://codereview.chromium.org/156333002/diff/1560001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ResourceExtractor.java
(right):

https://codereview.chromium.org/156333002/diff/1560001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ResourceExtractor.java:327:
Log.w(LOGTAG, "Unable to remove the icudata " + icudata.getName());
This is worse than a warning. If we fail to update ICU, we're may have a very
broken browsing experience, depending upon what we've changed. Correct me if I
am wrong, but doesn't the data file have to be generated by EXACTLY the same
version of the ICU libraries that your code is linked against? What happens if
we upgrade Chrome and this part fails, will all the calls to ICU crash?

There's not really a way around this that I can see at present... but it likely
needs to be an error, not a warning, and depending on the way in which it fails
we might even have to reinstall Chrome to fix it, right?

[benm (inactive), 2014-02-21 13:55:41]

https://codereview.chromium.org/156333002/diff/1560001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ResourceExtractor.java
(right):

https://codereview.chromium.org/156333002/diff/1560001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ResourceExtractor.java:327:
Log.w(LOGTAG, "Unable to remove the icudata " + icudata.getName());
It's a fair point, but I don't think it's any different from failing to remove
the existing PAK files (below, line 333). We could make it a Log.e in both
cases.

Quickly looking over the extraction code, I think we could make this a little
more robust if we also removed the timestamp file when we failed to delete the
resources, that will cause us to try to extract again on next startup (to save
the user uninstalling and re-installing) but I think that improvement is out of
scope for this CL. If that deletion of the timestamp fails ... :-/

[jungshik at Google, 2014-02-21 21:50:48]

Log.e is put in place of Log.w in ResourceExtractor.java. 

Andrew and Ben, take another look. Thanks.

https://codereview.chromium.org/156333002/diff/1560001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ResourceExtractor.java
(right):

https://codereview.chromium.org/156333002/diff/1560001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ResourceExtractor.java:327:
Log.w(LOGTAG, "Unable to remove the icudata " + icudata.getName());
On 2014/02/21 13:55:42, benm wrote:
> It's a fair point, but I don't think it's any different from failing to remove
> the existing PAK files (below, line 333). We could make it a Log.e in both
> cases.

What Andrew worries about would not happen often (icudata file is mostly
compatible between versions), but still can happen. 

I'll change both to Log.e. 

> 
> Quickly looking over the extraction code, I think we could make this a little
> more robust if we also removed the timestamp file when we failed to delete the
> resources, that will cause us to try to extract again on next startup (to save
> the user uninstalling and re-installing) but I think that improvement is out
of
> scope for this CL. If that deletion of the timestamp fails ... :-/

I agree that it's out of scope for this CL. And, I might try in another CL, but
I may not be the best person to know all the implications...

[Andrew Hayden (chromium.org), 2014-02-25 13:52:55]

LGTM! Can't wait for this to land!

https://codereview.chromium.org/156333002/diff/1610001/chrome/android/testshe...
File
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellApplication.java
(right):

https://codereview.chromium.org/156333002/diff/1610001/chrome/android/testshe...
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellApplication.java:29:
"icudtl.dat",
It would be a good idea to add a comment here stating what this is. As a newbie
to ICU, I had absolutely no idea what icudtl.dat was. Someone had to tell me it
was the ICU data file, which is totally NOT obvious. A link to the ICU homepage
or something would be appropriate.

https://codereview.chromium.org/156333002/diff/1610001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ResourceExtractor.java
(right):

https://codereview.chromium.org/156333002/diff/1610001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ResourceExtractor.java:327:
Log.e(LOGTAG, "Unable to remove the icudata " + icudata.getName());
Thanks for adding this. Can we add a comment in here that this could break the
install of Chrome, but that there's no graceful way to recover if it does? That
way subsequent maintainers understand the severity and know you've thought about
it and that the risk is acceptable (I agree that it is acceptable and similar to
the PAK case below).

The comment can go at the top of the method since it applies to it all...

https://codereview.chromium.org/156333002/diff/1610001/content/shell/android/...
File
content/shell/android/linker_test_apk/src/org/chromium/chromium_linker_test_apk/ChromiumLinkerTestApplication.java
(right):

https://codereview.chromium.org/156333002/diff/1610001/content/shell/android/...
content/shell/android/linker_test_apk/src/org/chromium/chromium_linker_test_apk/ChromiumLinkerTestApplication.java:19:
"icudtl.dat"
Again would be a good idea to have a quick comment here explaining what ICU is.
Even better than linking to the ICU website, link to the readme.chromium in the
third_party lib, e.g.:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/icu/RE...

Here we can explain how we actually ingest the ICU data. This document should
probably also be augmented with a short description of what we do (the gyp flag
to set, and a brief statement of what it does would be food for me)

https://codereview.chromium.org/156333002/diff/1610001/content/shell/android/...
File
content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ContentShellApplication.java
(right):

https://codereview.chromium.org/156333002/diff/1610001/content/shell/android/...
content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ContentShellApplication.java:19:
"icudtl.dat"
Same here, should link to docs.

[jungshik at Google, 2014-02-27 00:56:44]

Thank you for the review comments. I've added comments as you suggested.

I'll land this CL after
https://chrome-internal-review.googlesource.com/#/c/155554/ is landed. 

On 2014/02/25 13:52:55, Andrew Hayden wrote:
> LGTM! Can't wait for this to land!
> 
>
https://codereview.chromium.org/156333002/diff/1610001/chrome/android/testshe...
> File
>
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellApplication.java
> (right):
> 
>
https://codereview.chromium.org/156333002/diff/1610001/chrome/android/testshe...
>
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellApplication.java:29:
> "icudtl.dat",
> It would be a good idea to add a comment here stating what this is. As a
newbie
> to ICU, I had absolutely no idea what icudtl.dat was. Someone had to tell me
it
> was the ICU data file, which is totally NOT obvious. A link to the ICU
homepage
> or something would be appropriate.
> 
>
https://codereview.chromium.org/156333002/diff/1610001/content/public/android...
> File
>
content/public/android/java/src/org/chromium/content/browser/ResourceExtractor.java
> (right):
> 
>
https://codereview.chromium.org/156333002/diff/1610001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/ResourceExtractor.java:327:
> Log.e(LOGTAG, "Unable to remove the icudata " + icudata.getName());
> Thanks for adding this. Can we add a comment in here that this could break the
> install of Chrome, but that there's no graceful way to recover if it does?
That
> way subsequent maintainers understand the severity and know you've thought
about
> it and that the risk is acceptable (I agree that it is acceptable and similar
to
> the PAK case below).
> 
> The comment can go at the top of the method since it applies to it all...
> 
>
https://codereview.chromium.org/156333002/diff/1610001/content/shell/android/...
> File
>
content/shell/android/linker_test_apk/src/org/chromium/chromium_linker_test_apk/ChromiumLinkerTestApplication.java
> (right):
> 
>
https://codereview.chromium.org/156333002/diff/1610001/content/shell/android/...
>
content/shell/android/linker_test_apk/src/org/chromium/chromium_linker_test_apk/ChromiumLinkerTestApplication.java:19:
> "icudtl.dat"
> Again would be a good idea to have a quick comment here explaining what ICU
is.
> Even better than linking to the ICU website, link to the readme.chromium in
the
> third_party lib, e.g.:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/icu/RE...
> 
> Here we can explain how we actually ingest the ICU data. This document should
> probably also be augmented with a short description of what we do (the gyp
flag
> to set, and a brief statement of what it does would be food for me)
> 
>
https://codereview.chromium.org/156333002/diff/1610001/content/shell/android/...
> File
>
content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ContentShellApplication.java
> (right):
> 
>
https://codereview.chromium.org/156333002/diff/1610001/content/shell/android/...
>
content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ContentShellApplication.java:19:
> "icudtl.dat"
> Same here, should link to docs.

[jungshik at Google, 2014-02-27 18:50:00]

The CQ bit was checked by jshin@chromium.org

[commit-bot: I haz the power, 2014-02-27 18:50:43]

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

[commit-bot: I haz the power, 2014-02-27 19:10:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-27 19:10:14]

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

[jungshik at Google, 2014-02-27 19:30:57]

The CQ bit was checked by jshin@chromium.org

[commit-bot: I haz the power, 2014-02-27 19:33:56]

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

[commit-bot: I haz the power, 2014-02-27 22:09:06]

Change committed as 253938

[jungshik at Google, 2014-03-05 19:49:17]

On 2014/02/27 22:09:06, I haz the power (commit-bot) wrote:
> Change committed as 253938

This was reverted because WebKitUnitTest.apk does not have icudtl.dat. I changed
WebKitUnitTest.isolate to copy
icudtl.dat to a device ( https://codereview.chromium.org/183013006/ ). It's
landed in Blink. Once it's rolled in, I'll re-land this CL.

[jungshik at Google, 2014-03-06 18:12:19]

On 2014/03/05 19:49:17, Jungshik Shin wrote:
> On 2014/02/27 22:09:06, I haz the power (commit-bot) wrote:
> > Change committed as 253938
> 
> This was reverted because WebKitUnitTest.apk does not have icudtl.dat. I
changed
> WebKitUnitTest.isolate to copy
> icudtl.dat to a device ( https://codereview.chromium.org/183013006/ ). It's
> landed in Blink. Once it's rolled in, I'll re-land this CL.

Blink got rolled beyond 168506 (https://codereview.chromium.org/183013006)
yesterday so that I'm committing this again.

[jungshik at Google, 2014-03-06 18:16:34]

The CQ bit was checked by jshin@chromium.org

[jungshik at Google, 2014-03-06 18:16:56]

The CQ bit was unchecked by jshin@chromium.org

[jungshik at Google, 2014-03-07 17:53:40]

The CQ bit was checked by jshin@chromium.org

[commit-bot: I haz the power, 2014-03-07 17:55:58]

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

[commit-bot: I haz the power, 2014-03-07 19:53:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-07 19:53:46]

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

[jungshik at Google, 2014-03-07 22:21:10]

The CQ bit was checked by jshin@chromium.org

[commit-bot: I haz the power, 2014-03-07 22:22:48]

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

[commit-bot: I haz the power, 2014-03-08 11:06:32]

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

[commit-bot: I haz the power, 2014-03-08 12:02:33]

Change committed as 255762