Android: Store language .pak files in res/raw rather than assets

Android: Store language .pak files in res/raw rather than assets

This is a prerequisite for having them live within APK splits.

Although pak files need to be renamed to be stored under res/raw,
ResourceExtractor restores their original name during extraction (so no
change to native code).

All sub-locales are stored and extracted together, just as before.

BUG=371610
Committed: https://crrev.com/f30e04470453ccd7e6a9abf586185b991d174902
Cr-Commit-Position: refs/heads/master@{#335350}

[agrieve, 2015-06-05 14:46:58]

agrieve@chromium.org changed reviewers:
+ cjhopman@chromium.org, torne@chromium.org, yfriedman@chromium.org

[Torne, 2015-06-05 14:51:28]

How does this work in WebView, where we don't extract resources and load them
directly from the APK already? I don't see any changes to the code that actually
maps the pak files to allow for their new location..

[Torne, 2015-06-05 14:51:30]

How does this work in WebView, where we don't extract resources and load them
directly from the APK already? I don't see any changes to the code that actually
maps the pak files to allow for their new location..

[agrieve, 2015-06-05 15:16:47]

On 2015/06/05 14:51:30, Torne wrote:
> How does this work in WebView, where we don't extract resources and load them
> directly from the APK already? I don't see any changes to the code that
actually
> maps the pak files to allow for their new location..

For webview, nothing changes (that's my intention at least). Webview has custom
locale .pak build rules, and doesn't (I don't think) depend on chrome_java, so
won't pick up the added resources. I think it will now try and create a
ResourceExtractor timestamp file... not sure if that's an issue or not.

The code changes that adjust for the new location is in ResourceExtractor.java.
The next step after this is to enable direct-from-apk loading of locale .paks
for ChromeModern (like webview does)

[Torne, 2015-06-05 15:24:03]

On 2015/06/05 15:16:47, agrieve wrote:
> On 2015/06/05 14:51:30, Torne wrote:
> > How does this work in WebView, where we don't extract resources and load
them
> > directly from the APK already? I don't see any changes to the code that
> actually
> > maps the pak files to allow for their new location..
> 
> For webview, nothing changes (that's my intention at least). Webview has
custom
> locale .pak build rules, and doesn't (I don't think) depend on chrome_java, so
> won't pick up the added resources. I think it will now try and create a
> ResourceExtractor timestamp file... not sure if that's an issue or not.

We don't really want ResourceExtractor to ever do anything at all for webview,
because the data dir is whichever app we happen to be in, and we never want to
be in a position where we have to extract files to the app's data dir just to
operate. Creating a timestamp file isn't the end of the world but it would
probably be good to just have a way to completely neuter the extractor for
webview such that it's guaranteed to do nothing whatsoever, ever.

Are you intending to change webview to use res/ for paks at some point as well?
I don't see why we wouldn't also want this for webview.

> The code changes that adjust for the new location is in
ResourceExtractor.java.
> The next step after this is to enable direct-from-apk loading of locale .paks
> for ChromeModern (like webview does)

[agrieve, 2015-06-09 01:15:42]

On 2015/06/05 15:24:03, Torne wrote:
> On 2015/06/05 15:16:47, agrieve wrote:
> > On 2015/06/05 14:51:30, Torne wrote:
> > > How does this work in WebView, where we don't extract resources and load
> them
> > > directly from the APK already? I don't see any changes to the code that
> > actually
> > > maps the pak files to allow for their new location..
> > 
> > For webview, nothing changes (that's my intention at least). Webview has
> custom
> > locale .pak build rules, and doesn't (I don't think) depend on chrome_java,
so
> > won't pick up the added resources. I think it will now try and create a
> > ResourceExtractor timestamp file... not sure if that's an issue or not.
> 
> We don't really want ResourceExtractor to ever do anything at all for webview,
> because the data dir is whichever app we happen to be in, and we never want to
> be in a position where we have to extract files to the app's data dir just to
> operate. Creating a timestamp file isn't the end of the world but it would
> probably be good to just have a way to completely neuter the extractor for
> webview such that it's guaranteed to do nothing whatsoever, ever.

Gotcha. Reverted removal of shouldSkipPakExtraction(). Should be same as before
now for webview.

> 
> Are you intending to change webview to use res/ for paks at some point as
well?
> I don't see why we wouldn't also want this for webview.

Probably. If for no other reason than consistency (e.g. don't want to add
another condition to ResourceBundle::LocaleDataPakExists()).

The full plan:
- Have ChromeModern (L+ apk) contain locale-paks in apk splits, and have them
stored uncompressed and mmap'ed in.
- Have regular Chrome continue to store locale-paks compressed, and extract the
necessary one with Resource Extractor
- Have all other .pak files, icudtl.dat, and v8 snapshot .bin files stored
uncompressed & mmap'ed in all apks.

I was originally thinking of moving all of these files to res/raw, but I've now
learned that doing so is really annoying (restricts file name, creates useless
resource ID for each). So, only moving the locale paks to res/raw.




> 
> > The code changes that adjust for the new location is in
> ResourceExtractor.java.
> > The next step after this is to enable direct-from-apk loading of locale
.paks
> > for ChromeModern (like webview does)

[Torne, 2015-06-09 12:29:37]

OK, LGTM from webview perspective. Thanks for explaining in detail.

[cjhopman, 2015-06-09 22:35:13]

https://codereview.chromium.org/1158053005/diff/20001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/ResourceExtractor.java (right):

https://codereview.chromium.org/1158053005/diff/20001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ResourceExtractor.java:87: is.close();
if is.close() throws an exception, os is never closed.

https://codereview.chromium.org/1158053005/diff/20001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ResourceExtractor.java:292: public
static void setMandatoryPaksToExtract(int localPaksResId, String... paths) {
s/localP/localeP

https://codereview.chromium.org/1158053005/diff/20001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ResourceExtractor.java:368: //
deleteFiles() will not run and some files will be left. Oh well.
Hm, I really think we should clean up the old files in that case.

https://codereview.chromium.org/1158053005/diff/20001/chrome/android/BUILD.gn
File chrome/android/BUILD.gn (right):

https://codereview.chromium.org/1158053005/diff/20001/chrome/android/BUILD.gn...
chrome/android/BUILD.gn:29: [ rebase_path("//chrome/chrome_android_paks.gypi")
],
Can this just add an arg like:

"--replace=<(PRODUCT_DIR)=$root_out_dir" (this might not be quite the correct
replacement)

instead of having to do the double rebase whenever the lists are used?

[agrieve, 2015-06-11 15:34:38]

https://codereview.chromium.org/1158053005/diff/20001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/ResourceExtractor.java (right):

https://codereview.chromium.org/1158053005/diff/20001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ResourceExtractor.java:87: is.close();
On 2015/06/09 22:35:13, cjhopman wrote:
> if is.close() throws an exception, os is never closed.

Done.

https://codereview.chromium.org/1158053005/diff/20001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ResourceExtractor.java:292: public
static void setMandatoryPaksToExtract(int localPaksResId, String... paths) {
On 2015/06/09 22:35:13, cjhopman wrote:
> s/localP/localeP

Done.

https://codereview.chromium.org/1158053005/diff/20001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ResourceExtractor.java:368: //
deleteFiles() will not run and some files will be left. Oh well.
On 2015/06/09 22:35:13, cjhopman wrote:
> Hm, I really think we should clean up the old files in that case.

Expanded comment to say it doesn't affect Chrome for now. If we ship a version
of ChromeModern before all resources are loaded from the apk though, this will
be a real issue. I'm hoping I can get all resources loaded directly before we
ship ChromeModern though.

https://codereview.chromium.org/1158053005/diff/20001/chrome/android/BUILD.gn
File chrome/android/BUILD.gn (right):

https://codereview.chromium.org/1158053005/diff/20001/chrome/android/BUILD.gn...
chrome/android/BUILD.gn:29: [ rebase_path("//chrome/chrome_android_paks.gypi")
],
On 2015/06/09 22:35:13, cjhopman wrote:
> Can this just add an arg like:
> 
> "--replace=<(PRODUCT_DIR)=$root_out_dir" (this might not be quite the correct
> replacement)
> 
> instead of having to do the double rebase whenever the lists are used?

Much nicer!

[Yaron, 2015-06-11 20:34:12]

https://codereview.chromium.org/1158053005/diff/20001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/ResourceExtractor.java (right):

https://codereview.chromium.org/1158053005/diff/20001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ResourceExtractor.java:368: //
deleteFiles() will not run and some files will be left. Oh well.
On 2015/06/11 15:34:38, agrieve wrote:
> On 2015/06/09 22:35:13, cjhopman wrote:
> > Hm, I really think we should clean up the old files in that case.
> 
> Expanded comment to say it doesn't affect Chrome for now. If we ship a version
> of ChromeModern before all resources are loaded from the apk though, this will
> be a real issue. I'm hoping I can get all resources loaded directly before we
> ship ChromeModern though.

ChromeModern is shipping with 44.

[agrieve, 2015-06-13 00:24:04]

On 2015/06/11 20:34:12, Yaron wrote:
>
https://codereview.chromium.org/1158053005/diff/20001/base/android/java/src/o...
> File base/android/java/src/org/chromium/base/ResourceExtractor.java (right):
> 
>
https://codereview.chromium.org/1158053005/diff/20001/base/android/java/src/o...
> base/android/java/src/org/chromium/base/ResourceExtractor.java:368: //
> deleteFiles() will not run and some files will be left. Oh well.
> On 2015/06/11 15:34:38, agrieve wrote:
> > On 2015/06/09 22:35:13, cjhopman wrote:
> > > Hm, I really think we should clean up the old files in that case.
> > 
> > Expanded comment to say it doesn't affect Chrome for now. If we ship a
version
> > of ChromeModern before all resources are loaded from the apk though, this
will
> > be a real issue. I'm hoping I can get all resources loaded directly before
we
> > ship ChromeModern though.
> 
> ChromeModern is shipping with 44.

Understood. I'll leave the comment as-is for this change since it describes the
state of things well. But I'll need to figure out how to run the deletion code
once I get locale paks loading without being extracted.

[agrieve, 2015-06-16 00:32:53]

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

[agrieve, 2015-06-16 00:32:53]

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

jhawkins@chromium.org: Please review changes in chrome/

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

[Jaekyun Seok (inactive), 2015-06-16 00:39:02]

lgtm

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

https://codereview.chromium.org/1158053005/diff/60001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/ResourceExtractor.java (right):

https://codereview.chromium.org/1158053005/diff/60001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ResourceExtractor.java:71: if (buffer ==
null) {
This is unnecessary - you've changed the declaration to construct it.

https://codereview.chromium.org/1158053005/diff/60001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ResourceExtractor.java:147: if
(output.exists()) {
Hmm. Just confirming this doesn't break the case of pushing a new version and
there were old resources. deleteFiles would still delete your previously
extracted pak files because the destination is unchanged?

https://codereview.chromium.org/1158053005/diff/60001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ResourceExtractor.java:291: * @param
loaclePaksResId Resource ID for the locale_paks string array. Pass
localePaksResId

https://codereview.chromium.org/1158053005/diff/60001/build/android/gyp/local...
File build/android/gyp/locale_pak_resources.py (right):

https://codereview.chromium.org/1158053005/diff/60001/build/android/gyp/local...
build/android/gyp/locale_pak_resources.py:74: # Chrome's uses different region
mapping logic from Android, so include
s/Chrome's/Chrome/

https://codereview.chromium.org/1158053005/diff/60001/chromecast/browser/andr...
File
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastApplication.java
(right):

https://codereview.chromium.org/1158053005/diff/60001/chromecast/browser/andr...
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastApplication.java:40:
ResourceExtractor.setMandatoryPaksToExtract(0, MANDATORY_PAK_FILES);
The passing 0 for most of these is kinda ugly but I guess can be cleaned up
along with the TODO from https://codereview.chromium.org/1156873002/

[agrieve, 2015-06-16 15:47:42]

https://codereview.chromium.org/1158053005/diff/60001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/ResourceExtractor.java (right):

https://codereview.chromium.org/1158053005/diff/60001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ResourceExtractor.java:71: if (buffer ==
null) {
On 2015/06/16 03:50:45, Yaron wrote:
> This is unnecessary - you've changed the declaration to construct it.

Done.

https://codereview.chromium.org/1158053005/diff/60001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ResourceExtractor.java:147: if
(output.exists()) {
On 2015/06/16 03:50:45, Yaron wrote:
> Hmm. Just confirming this doesn't break the case of pushing a new version and
> there were old resources. deleteFiles would still delete your previously
> extracted pak files because the destination is unchanged?

Right - the deletion code (line 113) is unchanged and based on a timestamp file,
so isn't affected by this change.

https://codereview.chromium.org/1158053005/diff/60001/base/android/java/src/o...
base/android/java/src/org/chromium/base/ResourceExtractor.java:291: * @param
loaclePaksResId Resource ID for the locale_paks string array. Pass
On 2015/06/16 03:50:45, Yaron wrote:
> localePaksResId

Done.

https://codereview.chromium.org/1158053005/diff/60001/build/android/gyp/local...
File build/android/gyp/locale_pak_resources.py (right):

https://codereview.chromium.org/1158053005/diff/60001/build/android/gyp/local...
build/android/gyp/locale_pak_resources.py:74: # Chrome's uses different region
mapping logic from Android, so include
On 2015/06/16 03:50:45, Yaron wrote:
> s/Chrome's/Chrome/

Done.

https://codereview.chromium.org/1158053005/diff/60001/chromecast/browser/andr...
File
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastApplication.java
(right):

https://codereview.chromium.org/1158053005/diff/60001/chromecast/browser/andr...
chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastApplication.java:40:
ResourceExtractor.setMandatoryPaksToExtract(0, MANDATORY_PAK_FILES);
On 2015/06/16 03:50:45, Yaron wrote:
> The passing 0 for most of these is kinda ugly but I guess can be cleaned up
> along with the TODO from https://codereview.chromium.org/1156873002/

Yeah, no worse than passing "" IMO. Do plan to make it unnecessary though (as
per the TODO).

[Yaron, 2015-06-16 22:42:19]

yfriedman@chromium.org changed reviewers:
+ newt@chromium.org

[Yaron, 2015-06-16 22:42:20]

+newt who's our android localization expert

https://codereview.chromium.org/1158053005/diff/80001/build/android/gyp/local...
File build/android/gyp/locale_pak_resources.py (right):

https://codereview.chromium.org/1158053005/diff/80001/build/android/gyp/local...
build/android/gyp/locale_pak_resources.py:77: lang = android_locale[0:2]
Looking at a sample strings grd (e.g.
https://codereview.chromium.org/1158053005/), it looks like you can do more
specific region matching with something like (values-en-rGB)

[agrieve, 2015-06-17 01:25:41]

https://codereview.chromium.org/1158053005/diff/80001/build/android/gyp/local...
File build/android/gyp/locale_pak_resources.py (right):

https://codereview.chromium.org/1158053005/diff/80001/build/android/gyp/local...
build/android/gyp/locale_pak_resources.py:77: lang = android_locale[0:2]
On 2015/06/16 22:42:20, Yaron wrote:
> Looking at a sample strings grd (e.g.
> https://codereview.chromium.org/1158053005/), it looks like you can do more
> specific region matching with something like (values-en-rGB)

I did have this at first, but upon discovering the complicated logic in
ui/base/l10n/l10n_util.cc for choosing a region, it's clear that just keying off
language is the way to go. This (I suspect) is also why ResourceExtractor
extracts all regions for the active language (through its RegExp filtering -
although that's refactored in this CL to just use a build-time list)

[Yaron, 2015-06-17 01:50:42]

ok this lgtm but should still wait for newt

[newt (away), 2015-06-17 02:25:09]

On 2015/06/17 01:50:42, Yaron wrote:
> ok this lgtm but should still wait for newt

Should I take a look at the whole CL, or just the comment in
locale_pak_resources.py? (The code in locale_pak_resources.py looks good to me,
btw)

[Yaron, 2015-06-17 02:34:33]

On 2015/06/17 02:25:09, newt wrote:
> On 2015/06/17 01:50:42, Yaron wrote:
> > ok this lgtm but should still wait for newt
> 
> Should I take a look at the whole CL, or just the comment in
> locale_pak_resources.py? (The code in locale_pak_resources.py looks good to
me,
> btw)

that file is sufficient

[James Hawkins, 2015-06-17 05:29:31]

lgtm

[gunsch, 2015-06-17 16:41:05]

chromecast rs lgtm

[cjhopman, 2015-06-17 22:17:07]

lgtm

[agrieve, 2015-06-19 17:28:57]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2015-06-19 17:28:58]

The patchset sent to the CQ was uploaded after l-g-t-m from torne@chromium.org,
jaekyun@chromium.org, gunsch@chromium.org, cjhopman@chromium.org,
yfriedman@chromium.org, jhawkins@chromium.org
Link to the patchset: https://codereview.chromium.org/1158053005/#ps100001
(title: "rebased")

[commit-bot: I haz the power, 2015-06-19 17:29:39]

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

[commit-bot: I haz the power, 2015-06-19 17:58:00]

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

[commit-bot: I haz the power, 2015-06-19 17:58:02]

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

[agrieve, 2015-06-19 20:10:41]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2015-06-19 20:10:42]

The patchset sent to the CQ was uploaded after l-g-t-m from torne@chromium.org,
gunsch@chromium.org, cjhopman@chromium.org, yfriedman@chromium.org,
jaekyun@chromium.org, jhawkins@chromium.org
Link to the patchset: https://codereview.chromium.org/1158053005/#ps120001
(title: "fix chrome shell compile on GN")

[commit-bot: I haz the power, 2015-06-19 20:11:22]

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

[commit-bot: I haz the power, 2015-06-19 21:20:11]

Committed patchset #7 (id:120001)

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

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

[qinmin, 2015-06-20 02:53:12]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1193113004/ by qinmin@chromium.org.

The reason for reverting is: this change breaks android TOT bot.