Store and load icudtl.dat directly from the apk rather than extracting on start-up

Store and load icudtl.dat directly from the apk rather than extracting on start-up.

Also changes so that browser process caches a FD to the file rather than re-opening it for each child process.

We store icudtl.dat uncompressed in the apk now, increasing the apk size, but saving 6mb on-disk.

BUG=394502
Committed: https://crrev.com/f3930cfae7a93c5065588c9ba7be5c6927f46588
Cr-Commit-Position: refs/heads/master@{#335261}

[agrieve, 2015-06-01 17:13:19]

agrieve@chromium.org changed reviewers:
+ thestig@chromium.org, yfriedman@chromium.org

[agrieve, 2015-06-01 17:13:20]

yfriedman@chromium.org: Please review changes in 

java

thestig@chromium.org: Please review changes in 

base/i18n

[Yaron, 2015-06-01 22:24:52]

https://codereview.chromium.org/1147213004/diff/1/android_webview/test/shell/...
File
android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellApplication.java
(right):

https://codereview.chromium.org/1147213004/diff/1/android_webview/test/shell/...
android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellApplication.java:24:
private static final String[] MANDATORY_PAKS = {};
Please remove entirely.

https://codereview.chromium.org/1147213004/diff/1/chrome/browser/chrome_conte...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/1147213004/diff/1/chrome/browser/chrome_conte...
chrome/browser/chrome_content_browser_client.h:314: base::ScopedFD icudtl_fd_;
Please add a comment for these and mention this change in the CL description
(since we now cache these across loads)

[Yaron, 2015-06-02 14:28:01]

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

https://codereview.chromium.org/1147213004/diff/20001/base/i18n/icu_util.cc#n...
base/i18n/icu_util.cc:125: #elif (ICU_UTIL_DATA_IMPL == ICU_UTIL_DATA_FILE) &&
OS_ANDROID
defined(OS_ANDROID)

https://codereview.chromium.org/1147213004/diff/20001/base/i18n/icu_util.cc#n...
base/i18n/icu_util.cc:130: #if !defined(NDEBUG)
Yikes. This is pretty awkward (to be fair this function is a mess even before
you getting here).

If you look at the comment in icu_util.h, it mentions that android is already
supposed to call InitializeICUWithFileDescriptor. Then looking at
content_main_runner.cc, I see it trying to initialize using an fd. I guess that
part only works on the renderer process?

One option would be to codify this more meaningfully in content_main_runner.cc.
There you have access to |process_type| and can specify to do this for browser
process, but load from previously initialized fds for child processes.

However, I also see many other callers to InitializeICU. Have you tried running
native tests? It looks like they'll now try and load icu from the apk - is it
packaged for them? Same question for content_shell.

https://codereview.chromium.org/1147213004/diff/20001/base/i18n/icu_util.cc#n...
base/i18n/icu_util.cc:134: #elif (ICU_UTIL_DATA_IMPL == ICU_UTIL_DATA_FILE) &&
!OS_ANDROID
!defined(OS_ANDROID)

[agrieve, 2015-06-02 17:24:40]

https://codereview.chromium.org/1147213004/diff/1/android_webview/test/shell/...
File
android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellApplication.java
(right):

https://codereview.chromium.org/1147213004/diff/1/android_webview/test/shell/...
android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellApplication.java:24:
private static final String[] MANDATORY_PAKS = {};
On 2015/06/01 22:24:52, Yaron wrote:
> Please remove entirely.

Done.

https://codereview.chromium.org/1147213004/diff/1/chrome/browser/chrome_conte...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/1147213004/diff/1/chrome/browser/chrome_conte...
chrome/browser/chrome_content_browser_client.h:314: base::ScopedFD icudtl_fd_;
On 2015/06/01 22:24:52, Yaron wrote:
> Please add a comment for these and mention this change in the CL description
> (since we now cache these across loads)

Done.

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

https://codereview.chromium.org/1147213004/diff/20001/base/i18n/icu_util.cc#n...
base/i18n/icu_util.cc:125: #elif (ICU_UTIL_DATA_IMPL == ICU_UTIL_DATA_FILE) &&
OS_ANDROID
On 2015/06/02 14:28:01, Yaron wrote:
> defined(OS_ANDROID)

Done.

https://codereview.chromium.org/1147213004/diff/20001/base/i18n/icu_util.cc#n...
base/i18n/icu_util.cc:130: #if !defined(NDEBUG)
On 2015/06/02 14:28:01, Yaron wrote:
> Yikes. This is pretty awkward (to be fair this function is a mess even before
> you getting here).
> 
> If you look at the comment in icu_util.h, it mentions that android is already
> supposed to call InitializeICUWithFileDescriptor. Then looking at
> content_main_runner.cc, I see it trying to initialize using an fd. I guess
that
> part only works on the renderer process?
> 
> One option would be to codify this more meaningfully in
content_main_runner.cc.
> There you have access to |process_type| and can specify to do this for browser
> process, but load from previously initialized fds for child processes.
> 
> However, I also see many other callers to InitializeICU. Have you tried
running
> native tests? It looks like they'll now try and load icu from the apk - is it
> packaged for them? Same question for content_shell.

Agree it's awkward :S

InitializeICU is called from many places (tests, remoting, demo apps), so I
think it makes sense to continue to support it.

The native tests do also load right from the apk. I'm hoping to make all asset
loading to be consistent across targets (never extract and always mmap).

https://codereview.chromium.org/1147213004/diff/20001/base/i18n/icu_util.cc#n...
base/i18n/icu_util.cc:134: #elif (ICU_UTIL_DATA_IMPL == ICU_UTIL_DATA_FILE) &&
!OS_ANDROID
On 2015/06/02 14:28:01, Yaron wrote:
> !defined(OS_ANDROID)

Done.

[Lei Zhang, 2015-06-02 20:31:14]

thestig@chromium.org changed reviewers:
+ jshin@chromium.org

[Lei Zhang, 2015-06-02 20:31:16]

+jshin for base/i18n/

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

https://codereview.chromium.org/1147213004/diff/40001/base/i18n/icu_util.cc#n...
base/i18n/icu_util.cc:128: CHECK(fd != -1);
CHECK_GE(fd, 0);

https://codereview.chromium.org/1147213004/diff/40001/base/i18n/icu_util.cc#n...
base/i18n/icu_util.cc:131: g_called_once = false;
Can you explain what's going on here?

[Yaron, 2015-06-02 21:19:40]

lgtm for the java bits. I'm out for the rest of the week so you can sort out the
icu bits with other reviewers.

note also I don't have owners for a few of the application files so you'll need
a few more

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

https://codereview.chromium.org/1147213004/diff/40001/base/i18n/icu_util.cc#n...
base/i18n/icu_util.cc:131: g_called_once = false;
On 2015/06/02 20:31:15, Lei Zhang wrote:
> Can you explain what's going on here?

In comment or why this is happening? It was bugging me too (see other comments)
but it's needed to be unset because the next step is to call into
InitializeICUWithFileDescriptor which also checks the flag (to prevent
initializing twice).

The simplest fix might be to define helper *Internal versions of these functions
that are private. Then InitializeICU can call
InitializeICUWithFileDescriptorInternal and not have to unset the flag for
public API access.

[Yaron, 2015-06-02 21:24:00]

On 2015/06/02 21:19:40, Yaron wrote:
> lgtm for the java bits. I'm out for the rest of the week so you can sort out
the
> icu bits with other reviewers.
> 
> note also I don't have owners for a few of the application files so you'll
need
> a few more
> 
> https://codereview.chromium.org/1147213004/diff/40001/base/i18n/icu_util.cc
> File base/i18n/icu_util.cc (right):
> 
>
https://codereview.chromium.org/1147213004/diff/40001/base/i18n/icu_util.cc#n...
> base/i18n/icu_util.cc:131: g_called_once = false;
> On 2015/06/02 20:31:15, Lei Zhang wrote:
> > Can you explain what's going on here?
> 
> In comment or why this is happening? It was bugging me too (see other
comments)
> but it's needed to be unset because the next step is to call into
> InitializeICUWithFileDescriptor which also checks the flag (to prevent
> initializing twice).
> 
> The simplest fix might be to define helper *Internal versions of these
functions
> that are private. Then InitializeICU can call
> InitializeICUWithFileDescriptorInternal and not have to unset the flag for
> public API access.

Oh actually, can you also include what the size impact of this change is?
Presumably the apk is now larger although overall disk space is reduced since we
aren't extracting it out of the apk. Still would be good to quantify it

[Yaron, 2015-06-02 21:27:37]

On 2015/06/02 21:24:00, Yaron wrote:
> On 2015/06/02 21:19:40, Yaron wrote:
> > lgtm for the java bits. I'm out for the rest of the week so you can sort out
> the
> > icu bits with other reviewers.
> > 
> > note also I don't have owners for a few of the application files so you'll
> need
> > a few more
> > 
> > https://codereview.chromium.org/1147213004/diff/40001/base/i18n/icu_util.cc
> > File base/i18n/icu_util.cc (right):
> > 
> >
>
https://codereview.chromium.org/1147213004/diff/40001/base/i18n/icu_util.cc#n...
> > base/i18n/icu_util.cc:131: g_called_once = false;
> > On 2015/06/02 20:31:15, Lei Zhang wrote:
> > > Can you explain what's going on here?
> > 
> > In comment or why this is happening? It was bugging me too (see other
> comments)
> > but it's needed to be unset because the next step is to call into
> > InitializeICUWithFileDescriptor which also checks the flag (to prevent
> > initializing twice).
> > 
> > The simplest fix might be to define helper *Internal versions of these
> functions
> > that are private. Then InitializeICU can call
> > InitializeICUWithFileDescriptorInternal and not have to unset the flag for
> > public API access.
> 
> Oh actually, can you also include what the size impact of this change is?
> Presumably the apk is now larger although overall disk space is reduced since
we
> aren't extracting it out of the apk. Still would be good to quantify it

Looks like it's a 6mb on-disk savings although chrome will look bigger in play
store.

[agrieve, 2015-06-03 14:51:52]

agrieve@chromium.org changed reviewers:
+ jam@chromium.org

[agrieve, 2015-06-03 14:51:52]

jam@chromium.org: Please review changes in 

All of the *content_browser_client.* files

[agrieve, 2015-06-03 14:59:05]

On 2015/06/02 20:31:16, Lei Zhang wrote:
> +jshin for base/i18n/
> 
> https://codereview.chromium.org/1147213004/diff/40001/base/i18n/icu_util.cc
> File base/i18n/icu_util.cc (right):
> 
>
https://codereview.chromium.org/1147213004/diff/40001/base/i18n/icu_util.cc#n...
> base/i18n/icu_util.cc:128: CHECK(fd != -1);
> CHECK_GE(fd, 0);
Done
> 
>
https://codereview.chromium.org/1147213004/diff/40001/base/i18n/icu_util.cc#n...
> base/i18n/icu_util.cc:131: g_called_once = false;
> Can you explain what's going on here?

Took Yaron's suggestion and created a private helper function to not have to
unset g_called_once

[jam, 2015-06-04 03:13:30]

https://codereview.chromium.org/1147213004/diff/80001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1147213004/diff/80001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:2294:
regions->insert(std::make_pair(kAndroidICUDataDescriptor, icudtl_region_));
why is chrome layer doing this instead of content layer? content knows about ICU

https://codereview.chromium.org/1147213004/diff/80001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/1147213004/diff/80001/content/public/browser/...
content/public/browser/content_browser_client.h:616: std::map<int,
base::MemoryMappedFile::Region>* regions
nit: why not just combine these two lines together? it's not like starting a
line with a comma is standard either? i.e. seems starting with ", std::" is the
same

[agrieve, 2015-06-04 19:17:20]

https://codereview.chromium.org/1147213004/diff/80001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1147213004/diff/80001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:2294:
regions->insert(std::make_pair(kAndroidICUDataDescriptor, icudtl_region_));
On 2015/06/04 03:13:30, jam wrote:
> why is chrome layer doing this instead of content layer? content knows about
ICU

I'm new to this code, and have no idea. I do know content still is the one to
use the FD that's opened here. Maybe the authors wanted to keep all
renderer-related file opens in one spot?

https://codereview.chromium.org/1147213004/diff/80001/content/public/browser/...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/1147213004/diff/80001/content/public/browser/...
content/public/browser/content_browser_client.h:616: std::map<int,
base::MemoryMappedFile::Region>* regions
On 2015/06/04 03:13:30, jam wrote:
> nit: why not just combine these two lines together? it's not like starting a
> line with a comma is standard either? i.e. seems starting with ", std::" is
the
> same

I didn't know how to format it, so I let clang-format decide. I also think the
combining is nicer, so I've gone ahead and done that.

[Lei Zhang, 2015-06-05 02:52:44]

base and chrome lgtm with some nits

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

https://codereview.chromium.org/1147213004/diff/100001/base/i18n/icu_util.cc#...
base/i18n/icu_util.cc:57: namespace {
nit: add a blank line after, since you end the namespace with a blank line.

https://codereview.chromium.org/1147213004/diff/100001/base/i18n/icu_util.cc#...
base/i18n/icu_util.cc:94: MemoryMappedFile::Region data_region) {
BTW, why are regions passed by value in this file?

https://codereview.chromium.org/1147213004/diff/100001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1147213004/diff/100001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:2289: &icudtl_region_));
nit: funny indentation.

[agrieve, 2015-06-05 14:52:14]

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

https://codereview.chromium.org/1147213004/diff/100001/base/i18n/icu_util.cc#...
base/i18n/icu_util.cc:57: namespace {
On 2015/06/05 02:52:43, Lei Zhang wrote:
> nit: add a blank line after, since you end the namespace with a blank line.

Done.

https://codereview.chromium.org/1147213004/diff/100001/base/i18n/icu_util.cc#...
base/i18n/icu_util.cc:57: namespace {
On 2015/06/05 02:52:43, Lei Zhang wrote:
> nit: add a blank line after, since you end the namespace with a blank line.

Done.

https://codereview.chromium.org/1147213004/diff/100001/base/i18n/icu_util.cc#...
base/i18n/icu_util.cc:94: MemoryMappedFile::Region data_region) {
On 2015/06/05 02:52:43, Lei Zhang wrote:
> BTW, why are regions passed by value in this file?

My thinking was just that they are small, so it didn't matter. Given that it
raises an eyebrow for you though, going to take that as a suggestion to
pass-by-ref (changed).

https://codereview.chromium.org/1147213004/diff/100001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1147213004/diff/100001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:2289: &icudtl_region_));
On 2015/06/05 02:52:43, Lei Zhang wrote:
> nit: funny indentation.

Done.

[agrieve, 2015-06-05 14:54:04]

agrieve@chromium.org changed reviewers:
+ cjhopman@chromium.org

[agrieve, 2015-06-05 14:54:28]

On 2015/06/05 14:52:16, agrieve wrote:

Added cjhopman for the build/ changes

[agrieve, 2015-06-08 20:01:42]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[agrieve, 2015-06-08 20:01:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
yfriedman@chromium.org, thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1147213004/#ps140001
(title: "Add new param to
ash/shell/content_client/shell_content_browser_client.cc")

[commit-bot: I haz the power, 2015-06-08 20:02:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1147213004/140001

[commit-bot: I haz the power, 2015-06-08 20:36:17]

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

[commit-bot: I haz the power, 2015-06-08 20:36:18]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[agrieve, 2015-06-09 00:37:11]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[agrieve, 2015-06-09 00:37:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, yfriedman@chromium.org
Link to the patchset: https://codereview.chromium.org/1147213004/#ps160001
(title: "Fix compile of battery_monitor_integration_browsertest.cc")

[commit-bot: I haz the power, 2015-06-09 00:39:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1147213004/160001

[jam, 2015-06-09 01:12:46]

https://codereview.chromium.org/1147213004/diff/80001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1147213004/diff/80001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:2294:
regions->insert(std::make_pair(kAndroidICUDataDescriptor, icudtl_region_));
On 2015/06/04 19:17:20, agrieve wrote:
> On 2015/06/04 03:13:30, jam wrote:
> > why is chrome layer doing this instead of content layer? content knows about
> ICU
> 
> I'm new to this code, and have no idea. I do know content still is the one to
> use the FD that's opened here. Maybe the authors wanted to keep all
> renderer-related file opens in one spot?

Hi, please work with someone on the android side to figure out if this can be
done in content and not in chrome.

[Yaron, 2015-06-09 03:39:40]

https://codereview.chromium.org/1147213004/diff/80001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1147213004/diff/80001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:2294:
regions->insert(std::make_pair(kAndroidICUDataDescriptor, icudtl_region_));
On 2015/06/09 01:12:46, jam wrote:
> On 2015/06/04 19:17:20, agrieve wrote:
> > On 2015/06/04 03:13:30, jam wrote:
> > > why is chrome layer doing this instead of content layer? content knows
about
> > ICU
> > 
> > I'm new to this code, and have no idea. I do know content still is the one
to
> > use the FD that's opened here. Maybe the authors wanted to keep all
> > renderer-related file opens in one spot?
> 
> Hi, please work with someone on the android side to figure out if this can be
> done in content and not in chrome.

Hmm. I definitely touched the crash fd code and it had to be done in chrome
layer because content doesn't deal with breakpad. I'm pretty sure that predated
the ICU fd passing and they probably just mirrored crash. I can take a deeper
look when I'm in tomorrow.

[jungshik at Google, 2015-06-09 23:45:15]

Sorry for the dealy. base/i18n : LGTM

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

https://codereview.chromium.org/1147213004/diff/180001/base/i18n/icu_util.cc#...
base/i18n/icu_util.cc:185: #endif  // OS check
It's not yours (it's my fault), but could you change the comment to
|!defined(OS_MACOSX)|. Thanks

https://codereview.chromium.org/1147213004/diff/180001/chromecast/browser/cas...
File chromecast/browser/cast_content_browser_client.cc (right):

https://codereview.chromium.org/1147213004/diff/180001/chromecast/browser/cas...
chromecast/browser/cast_content_browser_client.cc:369:
base::ScopedFD(icudata_file.TakePlatformFile()));
I'm wondering why this file does not require the same change as
chrome_content_browser.cc.

[agrieve, 2015-06-11 15:00:32]

On 2015/06/09 01:12:46, jam wrote:
>
https://codereview.chromium.org/1147213004/diff/80001/chrome/browser/chrome_c...
> File chrome/browser/chrome_content_browser_client.cc (right):
> 
>
https://codereview.chromium.org/1147213004/diff/80001/chrome/browser/chrome_c...
> chrome/browser/chrome_content_browser_client.cc:2294:
> regions->insert(std::make_pair(kAndroidICUDataDescriptor, icudtl_region_));
> On 2015/06/04 19:17:20, agrieve wrote:
> > On 2015/06/04 03:13:30, jam wrote:
> > > why is chrome layer doing this instead of content layer? content knows
about
> > ICU
> > 
> > I'm new to this code, and have no idea. I do know content still is the one
to
> > use the FD that's opened here. Maybe the authors wanted to keep all
> > renderer-related file opens in one spot?
> 
> Hi, please work with someone on the android side to figure out if this can be
> done in content and not in chrome.

Thanks for pointing this out! Now have
https://codereview.chromium.org/1182443003/ out I think it does simplify things
quite a bit. Will rebase this ontop of it.

[agrieve, 2015-06-13 02:15:57]

On 2015/06/11 15:00:32, agrieve wrote:
> On 2015/06/09 01:12:46, jam wrote:
> >
>
https://codereview.chromium.org/1147213004/diff/80001/chrome/browser/chrome_c...
> > File chrome/browser/chrome_content_browser_client.cc (right):
> > 
> >
>
https://codereview.chromium.org/1147213004/diff/80001/chrome/browser/chrome_c...
> > chrome/browser/chrome_content_browser_client.cc:2294:
> > regions->insert(std::make_pair(kAndroidICUDataDescriptor, icudtl_region_));
> > On 2015/06/04 19:17:20, agrieve wrote:
> > > On 2015/06/04 03:13:30, jam wrote:
> > > > why is chrome layer doing this instead of content layer? content knows
> about
> > > ICU
> > > 
> > > I'm new to this code, and have no idea. I do know content still is the one
> to
> > > use the FD that's opened here. Maybe the authors wanted to keep all
> > > renderer-related file opens in one spot?
> > 
> > Hi, please work with someone on the android side to figure out if this can
be
> > done in content and not in chrome.
> 
> Thanks for pointing this out! Now have
> https://codereview.chromium.org/1182443003/ out I think it does simplify
things
> quite a bit. Will rebase this ontop of it.

This is now rebased and ready for review (and much smaller now :))

[jam, 2015-06-16 03:13:33]

On 2015/06/13 02:15:57, agrieve wrote:
> On 2015/06/11 15:00:32, agrieve wrote:
> > On 2015/06/09 01:12:46, jam wrote:
> > >
> >
>
https://codereview.chromium.org/1147213004/diff/80001/chrome/browser/chrome_c...
> > > File chrome/browser/chrome_content_browser_client.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1147213004/diff/80001/chrome/browser/chrome_c...
> > > chrome/browser/chrome_content_browser_client.cc:2294:
> > > regions->insert(std::make_pair(kAndroidICUDataDescriptor,
icudtl_region_));
> > > On 2015/06/04 19:17:20, agrieve wrote:
> > > > On 2015/06/04 03:13:30, jam wrote:
> > > > > why is chrome layer doing this instead of content layer? content knows
> > about
> > > > ICU
> > > > 
> > > > I'm new to this code, and have no idea. I do know content still is the
one
> > to
> > > > use the FD that's opened here. Maybe the authors wanted to keep all
> > > > renderer-related file opens in one spot?
> > > 
> > > Hi, please work with someone on the android side to figure out if this can
> be
> > > done in content and not in chrome.
> > 
> > Thanks for pointing this out! Now have
> > https://codereview.chromium.org/1182443003/ out I think it does simplify
> things
> > quite a bit. Will rebase this ontop of it.
> 
> This is now rebased and ready for review (and much smaller now :))

Great, thanks. This looks good to me. Do you need me for review anymore?

[Yaron, 2015-06-16 03:17:45]

still lgtm

[agrieve, 2015-06-16 03:18:57]

agrieve@chromium.org changed reviewers:
- jam@chromium.org

[agrieve, 2015-06-16 03:20:02]

agrieve@chromium.org changed reviewers:
+ gunsch@chromium.org, jaekyun@chromium.org, torne@chromium.org

[agrieve, 2015-06-16 03:20:03]

jaekyun@chromium.org: Please review changes in components/test/android

gunsch@chromium.org: Please review changes in chromecast/

torne@chromium.org: Please review changes in android_webview/

[agrieve, 2015-06-16 03:21:04]

On 2015/06/16 03:13:33, jam wrote:
> On 2015/06/13 02:15:57, agrieve wrote:
> > On 2015/06/11 15:00:32, agrieve wrote:
> > > On 2015/06/09 01:12:46, jam wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1147213004/diff/80001/chrome/browser/chrome_c...
> > > > File chrome/browser/chrome_content_browser_client.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1147213004/diff/80001/chrome/browser/chrome_c...
> > > > chrome/browser/chrome_content_browser_client.cc:2294:
> > > > regions->insert(std::make_pair(kAndroidICUDataDescriptor,
> icudtl_region_));
> > > > On 2015/06/04 19:17:20, agrieve wrote:
> > > > > On 2015/06/04 03:13:30, jam wrote:
> > > > > > why is chrome layer doing this instead of content layer? content
knows
> > > about
> > > > > ICU
> > > > > 
> > > > > I'm new to this code, and have no idea. I do know content still is the
> one
> > > to
> > > > > use the FD that's opened here. Maybe the authors wanted to keep all
> > > > > renderer-related file opens in one spot?
> > > > 
> > > > Hi, please work with someone on the android side to figure out if this
can
> > be
> > > > done in content and not in chrome.
> > > 
> > > Thanks for pointing this out! Now have
> > > https://codereview.chromium.org/1182443003/ out I think it does simplify
> > things
> > > quite a bit. Will rebase this ontop of it.
> > 
> > This is now rebased and ready for review (and much smaller now :))
> 
> Great, thanks. This looks good to me. Do you need me for review anymore?

Great! Probably I should add specific reviewers for the other directories to
relevant people are aware. I've updated reviewers and moved you to CC.

[Jaekyun Seok (inactive), 2015-06-16 03:35:58]

lgtm

[Torne, 2015-06-16 10:10:53]

android_webview LGTM

[cjhopman, 2015-06-17 21:22:12]

lgtm

[gunsch, 2015-06-18 01:55:30]

lgtm

[agrieve, 2015-06-19 12:18:11]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2015-06-19 12:18:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, jshin@chromium.org, torne@chromium.org,
gunsch@chromium.org, cjhopman@chromium.org, yfriedman@chromium.org,
jaekyun@chromium.org
Link to the patchset: https://codereview.chromium.org/1147213004/#ps280001
(title: "rebase")

[commit-bot: I haz the power, 2015-06-19 12:18:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1147213004/280001

[commit-bot: I haz the power, 2015-06-19 13:12:37]

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

[commit-bot: I haz the power, 2015-06-19 13:12:39]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[agrieve, 2015-06-19 13:31:20]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2015-06-19 13:31:21]

The patchset sent to the CQ was uploaded after l-g-t-m from torne@chromium.org,
thestig@chromium.org, jshin@chromium.org, gunsch@chromium.org,
cjhopman@chromium.org, yfriedman@chromium.org, jaekyun@chromium.org
Link to the patchset: https://codereview.chromium.org/1147213004/#ps300001
(title: "fix compile")

[commit-bot: I haz the power, 2015-06-19 13:31:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1147213004/300001

[commit-bot: I haz the power, 2015-06-19 15:35:58]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2015-06-19 15:36:45]

Patchset 16 (id:??) landed as
https://crrev.com/f3930cfae7a93c5065588c9ba7be5c6927f46588
Cr-Commit-Position: refs/heads/master@{#335261}