Move language pak files to assets.

Move language pak files to assets.

Language pak files were moved to res/raw when split apks was being implemented.
Moving them to assets with a java constant keeping track of pak file locations
will improve lookup time and save some space (no more xml files and
resources.arsc).

BUG=529604
Committed: https://crrev.com/34ae27889dd8c8ffd5f5ca8c7bc10c17e57c12e2
Cr-Commit-Position: refs/heads/master@{#420641}

[estevenson, 2016-09-15 23:12:46]

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

[commit-bot: I haz the power, 2016-09-15 23:13:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 23:32:23]

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

[commit-bot: I haz the power, 2016-09-15 23:32:24]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[estevenson, 2016-09-16 14:00:18]

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

[commit-bot: I haz the power, 2016-09-16 14:00:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 14:24:29]

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

[commit-bot: I haz the power, 2016-09-16 14:24:30]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[estevenson, 2016-09-16 19:29:07]

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

[commit-bot: I haz the power, 2016-09-16 19:29:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 20:48:53]

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

[commit-bot: I haz the power, 2016-09-16 20:48:54]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[estevenson, 2016-09-19 22:10:44]

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

[commit-bot: I haz the power, 2016-09-19 22:11:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-19 22:20:30]

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

[commit-bot: I haz the power, 2016-09-19 22:20:31]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[estevenson, 2016-09-20 15:16:04]

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

[commit-bot: I haz the power, 2016-09-20 15:16:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 16:15:08]

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

[commit-bot: I haz the power, 2016-09-20 16:15:09]

Dry run: This issue passed the CQ dry run.

[estevenson, 2016-09-20 23:23:39]

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

[commit-bot: I haz the power, 2016-09-20 23:24:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estevenson, 2016-09-20 23:43:37]

Description was changed from

==========
Move language pak files back to assets rather than res.

BUG=529604
==========

to

==========
Move language pak files to assets.

Language pak files were moved to res/raw when split apks was being implemented.
Moving them to assets with a java constant keeping track of pak file locations
will improve lookup time and save some space (no more xml files and
resources.arsc).






BUG=529604
==========

[estevenson, 2016-09-20 23:49:58]

Description was changed from

==========
Move language pak files to assets.

Language pak files were moved to res/raw when split apks was being implemented.
Moving them to assets with a java constant keeping track of pak file locations
will improve lookup time and save some space (no more xml files and
resources.arsc).






BUG=529604
==========

to

==========
Move language pak files to assets.

Language pak files were moved to res/raw when split apks was being implemented.
Moving them to assets with a java constant keeping track of pak file locations
will improve lookup time and save some space (no more xml files and
resources.arsc).


BUG=529604
==========

[estevenson, 2016-09-20 23:51:15]

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

[estevenson, 2016-09-20 23:51:16]

ptal Andrew!

[agrieve, 2016-09-21 00:36:56]

Description was changed from

==========
Move language pak files to assets.

Language pak files were moved to res/raw when split apks was being implemented.
Moving them to assets with a java constant keeping track of pak file locations
will improve lookup time and save some space (no more xml files and
resources.arsc).


BUG=529604
==========

to

==========
Move language pak files to assets.

Language pak files were moved to res/raw when split apks was being implemented.
Moving them to assets with a java constant keeping track of pak file locations
will improve lookup time and save some space (no more xml files and
resources.arsc).

BUG=529604
==========

[agrieve, 2016-09-21 01:06:42]

Sorry to ask for big changes after having already looked at it offline. Reading
through this on the code review site sparked a couple ideas that hadn't occurred
to me earlier in terms of simplifications.

https://codereview.chromium.org/2345143002/diff/100001/base/android/java/src/...
File base/android/java/src/org/chromium/base/ResourceExtractor.java (right):

https://codereview.chromium.org/2345143002/diff/100001/base/android/java/src/...
base/android/java/src/org/chromium/base/ResourceExtractor.java:46: public final
String pathWithinApk;
nit: with this unused now, we should probably just remove this class in favour
of using String directly.

https://codereview.chromium.org/2345143002/diff/100001/build/android/gyp/loca...
File build/android/gyp/locale_pak_assets.py (right):

https://codereview.chromium.org/2345143002/diff/100001/build/android/gyp/loca...
build/android/gyp/locale_pak_assets.py:10: enumerated and extracted as
necessary.
nit: can you mention the motivation is that AssetManger.list() is slow.

https://codereview.chromium.org/2345143002/diff/100001/build/android/gyp/writ...
File build/android/gyp/write_build_config.py (right):

https://codereview.chromium.org/2345143002/diff/100001/build/android/gyp/writ...
build/android/gyp/write_build_config.py:247:
parser.add_option('--is-locale-asset', action='store_true',
Idea: If we add the assets target directly to the android_apk() target, it's
likely that we could delete this locale-overriding logic altogether.
Additionally, if we did this, we could also use jar_excluded_patterns to drop in
a new version of the generated class for Monochrome vs Chrome

 wdyt?

https://codereview.chromium.org/2345143002/diff/100001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/base/ResourceBundle.java (right):

https://codereview.chromium.org/2345143002/diff/100001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/base/ResourceBundle.java:38: String[]
pakFiles =
localePakFiles.getPakFilesForLocale(Locale.getDefault().getLanguage());
I think this could be simplified quite a bit if we:
1. Change this method to take in a String[] of all localePaks that exist (make
the generated class just have a single static field of String[]
2. Have this method do the language mapping via LocaleUtils.java
3. Loop through all pak files and select those that .startsWith() the active
language.

I think changing from a HashMap to a linear scan will actually be more efficient
and simpler. It'll also be good to have the android->chrome locale mapping
appear only in java again (and not need to keep the python in sync).

[commit-bot: I haz the power, 2016-09-21 01:35:14]

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

[commit-bot: I haz the power, 2016-09-21 01:35:15]

Dry run: This issue passed the CQ dry run.

[estevenson, 2016-09-21 19:48:24]

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

[commit-bot: I haz the power, 2016-09-21 19:48:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estevenson, 2016-09-21 19:50:56]

ptal Andrew!

https://codereview.chromium.org/2345143002/diff/100001/base/android/java/src/...
File base/android/java/src/org/chromium/base/ResourceExtractor.java (right):

https://codereview.chromium.org/2345143002/diff/100001/base/android/java/src/...
base/android/java/src/org/chromium/base/ResourceExtractor.java:46: public final
String pathWithinApk;
On 2016/09/21 01:06:41, agrieve wrote:
> nit: with this unused now, we should probably just remove this class in favour
> of using String directly.

Done.

https://codereview.chromium.org/2345143002/diff/100001/build/android/gyp/loca...
File build/android/gyp/locale_pak_assets.py (right):

https://codereview.chromium.org/2345143002/diff/100001/build/android/gyp/loca...
build/android/gyp/locale_pak_assets.py:10: enumerated and extracted as
necessary.
On 2016/09/21 01:06:42, agrieve wrote:
> nit: can you mention the motivation is that AssetManger.list() is slow.

Done.

https://codereview.chromium.org/2345143002/diff/100001/build/android/gyp/writ...
File build/android/gyp/write_build_config.py (right):

https://codereview.chromium.org/2345143002/diff/100001/build/android/gyp/writ...
build/android/gyp/write_build_config.py:247:
parser.add_option('--is-locale-asset', action='store_true',
On 2016/09/21 01:06:42, agrieve wrote:
> Idea: If we add the assets target directly to the android_apk() target, it's
> likely that we could delete this locale-overriding logic altogether.
> Additionally, if we did this, we could also use jar_excluded_patterns to drop
in
> a new version of the generated class for Monochrome vs Chrome
> 
>  wdyt?

Done.

https://codereview.chromium.org/2345143002/diff/100001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/base/ResourceBundle.java (right):

https://codereview.chromium.org/2345143002/diff/100001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/base/ResourceBundle.java:38: String[]
pakFiles =
localePakFiles.getPakFilesForLocale(Locale.getDefault().getLanguage());
On 2016/09/21 01:06:42, agrieve wrote:
> I think this could be simplified quite a bit if we:
> 1. Change this method to take in a String[] of all localePaks that exist (make
> the generated class just have a single static field of String[]
> 2. Have this method do the language mapping via LocaleUtils.java
> 3. Loop through all pak files and select those that .startsWith() the active
> language.
> 
> I think changing from a HashMap to a linear scan will actually be more
efficient
> and simpler. It'll also be good to have the android->chrome locale mapping
> appear only in java again (and not need to keep the python in sync).

Done.

[commit-bot: I haz the power, 2016-09-21 20:12:43]

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

[commit-bot: I haz the power, 2016-09-21 20:12:44]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[agrieve, 2016-09-21 20:34:01]

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

[agrieve, 2016-09-21 20:34:02]

lgtm. 
+michaelbai to keep him in the loop about how locales in monochrome are
changing.

https://codereview.chromium.org/2345143002/diff/140001/android_webview/glue/B...
File android_webview/glue/BUILD.gn (right):

https://codereview.chromium.org/2345143002/diff/140001/android_webview/glue/B...
android_webview/glue/BUILD.gn:46: "//android_webview:locale_pak_srcjar",
Monochrome depends on this as well, so you'll need to add jar_excluded_patterns
here as well and add it for real in the webview apk target(s)

https://codereview.chromium.org/2345143002/diff/140001/build/android/gyp/loca...
File build/android/gyp/locale_pak_assets.py (right):

https://codereview.chromium.org/2345143002/diff/140001/build/android/gyp/loca...
build/android/gyp/locale_pak_assets.py:94:
build_utils.WriteDepfile(args.depfile, args.srcjar, sources)
nit: don't pass sources into WriteDepfile. GN already knows about these
dependency edges.

https://codereview.chromium.org/2345143002/diff/140001/build/config/android/r...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/2345143002/diff/140001/build/config/android/r...
build/config/android/rules.gni:1155: #   package_suffix: The package where the
generated Java file should live.
nit: this might be simpler as just "java_package" rather than having suffix. I'd
also make it mandatory.

https://codereview.chromium.org/2345143002/diff/140001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/base/ResourceBundle.java (right):

https://codereview.chromium.org/2345143002/diff/140001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/base/ResourceBundle.java:27: private static
ArrayList<String> sActiveLocaleResources;
nit: This never needs to grow/shrink after it's initialized. It'd be better to
have it be a String[] I think, and assign it using toArray()

[michaelbai, 2016-09-21 20:44:23]

Is there downstream patch corresponding to this?

[estevenson, 2016-09-21 20:46:10]

On 2016/09/21 20:44:23, michaelbai wrote:
> Is there downstream patch corresponding to this?

Yep, just added you as a reviewer

[michaelbai, 2016-09-21 22:14:33]

https://codereview.chromium.org/2345143002/diff/140001/android_webview/glue/B...
File android_webview/glue/BUILD.gn (right):

https://codereview.chromium.org/2345143002/diff/140001/android_webview/glue/B...
android_webview/glue/BUILD.gn:46: "//android_webview:locale_pak_srcjar",
On 2016/09/21 20:34:01, agrieve wrote:
> Monochrome depends on this as well, so you'll need to add
jar_excluded_patterns
> here as well and add it for real in the webview apk target(s)

The LocalePakFiles which generated for Monochrome has chrome package name, can't
be used for WebView, right?

Can we always use org.chromium.ui.LocalePakFile?

[estevenson, 2016-09-21 22:28:09]

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

[commit-bot: I haz the power, 2016-09-21 22:29:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estevenson, 2016-09-21 22:35:18]

https://codereview.chromium.org/2345143002/diff/140001/android_webview/glue/B...
File android_webview/glue/BUILD.gn (right):

https://codereview.chromium.org/2345143002/diff/140001/android_webview/glue/B...
android_webview/glue/BUILD.gn:46: "//android_webview:locale_pak_srcjar",
On 2016/09/21 22:14:33, michaelbai wrote:
> On 2016/09/21 20:34:01, agrieve wrote:
> > Monochrome depends on this as well, so you'll need to add
> jar_excluded_patterns
> > here as well and add it for real in the webview apk target(s)
> 
> The LocalePakFiles which generated for Monochrome has chrome package name,
can't
> be used for WebView, right?
> 
> Can we always use org.chromium.ui.LocalePakFile?

Yeah, and there's no need to use different packages for different targets now
since we're explicitly adding the correct srcjar to the apk. Changed
locale_pak_assets to always use org.chromium.ui.

https://codereview.chromium.org/2345143002/diff/140001/build/android/gyp/loca...
File build/android/gyp/locale_pak_assets.py (right):

https://codereview.chromium.org/2345143002/diff/140001/build/android/gyp/loca...
build/android/gyp/locale_pak_assets.py:94:
build_utils.WriteDepfile(args.depfile, args.srcjar, sources)
On 2016/09/21 20:34:02, agrieve wrote:
> nit: don't pass sources into WriteDepfile. GN already knows about these
> dependency edges.

Done.

https://codereview.chromium.org/2345143002/diff/140001/build/config/android/r...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/2345143002/diff/140001/build/config/android/r...
build/config/android/rules.gni:1155: #   package_suffix: The package where the
generated Java file should live.
On 2016/09/21 20:34:02, agrieve wrote:
> nit: this might be simpler as just "java_package" rather than having suffix.
I'd
> also make it mandatory.

Done.

https://codereview.chromium.org/2345143002/diff/140001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/base/ResourceBundle.java (right):

https://codereview.chromium.org/2345143002/diff/140001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/base/ResourceBundle.java:27: private static
ArrayList<String> sActiveLocaleResources;
On 2016/09/21 20:34:02, agrieve wrote:
> nit: This never needs to grow/shrink after it's initialized. It'd be better to
> have it be a String[] I think, and assign it using toArray()

Done.

[estevenson, 2016-09-21 22:47:04]

estevenson@chromium.org changed reviewers:
+ nyquist@chromium.org, thakis@chromium.org

[estevenson, 2016-09-21 22:47:05]

thakis@chromium.org: Please review changes in ResourceBundle.java

nyquist@chromium.org: Please review changes in ChromeApplication.java,
BlimpApplication.java

[commit-bot: I haz the power, 2016-09-21 23:18:04]

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

[commit-bot: I haz the power, 2016-09-21 23:18:05]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[michaelbai, 2016-09-21 23:51:57]

https://codereview.chromium.org/2345143002/diff/160001/android_webview/glue/B...
File android_webview/glue/BUILD.gn (right):

https://codereview.chromium.org/2345143002/diff/160001/android_webview/glue/B...
android_webview/glue/BUILD.gn:46: "//android_webview:locale_pak_srcjar",
Should this move to system_webview_apk_tmpl.gni?

[estevenson, 2016-09-22 15:31:20]

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

[commit-bot: I haz the power, 2016-09-22 15:31:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-22 16:14:57]

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

[commit-bot: I haz the power, 2016-09-22 16:14:58]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Nico, 2016-09-22 17:11:53]

On 2016/09/21 22:47:05, estevenson wrote:
> mailto:thakis@chromium.org: Please review changes in ResourceBundle.java

Please get ui/android/OWNERS – try to prefer more nested owners

[michaelbai, 2016-09-22 18:17:24]

lgtm

[estevenson, 2016-09-22 20:27:43]

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

[commit-bot: I haz the power, 2016-09-22 20:28:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estevenson, 2016-09-22 22:19:08]

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

[commit-bot: I haz the power, 2016-09-22 22:19:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-22 23:55:19]

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

[commit-bot: I haz the power, 2016-09-22 23:55:31]

Dry run: This issue passed the CQ dry run.

[estevenson, 2016-09-23 00:18:17]

estevenson@chromium.org changed reviewers:
+ tedchoc@chromium.org
- thakis@chromium.org

[estevenson, 2016-09-23 00:18:18]

tedchoc@chromium.org: Please review changes in ChromeApplication.java and
ResourceBundle.java

[Ted C, 2016-09-23 00:42:10]

On 2016/09/23 00:18:18, estevenson wrote:
> mailto:tedchoc@chromium.org: Please review changes in ChromeApplication.java
and
> ResourceBundle.java

lgtm

[nyquist, 2016-09-23 15:19:47]

blimp lgtm

[estevenson, 2016-09-23 17:13:18]

The CQ bit was checked by estevenson@chromium.org

[estevenson, 2016-09-23 17:13:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
agrieve@chromium.org, michaelbai@chromium.org
Link to the patchset: https://codereview.chromium.org/2345143002/#ps240001
(title: "Move resource initialization back to ChromeApplication")

[commit-bot: I haz the power, 2016-09-23 17:13:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-23 17:19:35]

Description was changed from

==========
Move language pak files to assets.

Language pak files were moved to res/raw when split apks was being implemented.
Moving them to assets with a java constant keeping track of pak file locations
will improve lookup time and save some space (no more xml files and
resources.arsc).

BUG=529604
==========

to

==========
Move language pak files to assets.

Language pak files were moved to res/raw when split apks was being implemented.
Moving them to assets with a java constant keeping track of pak file locations
will improve lookup time and save some space (no more xml files and
resources.arsc).

BUG=529604
==========

[commit-bot: I haz the power, 2016-09-23 17:19:37]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-09-23 17:21:45]

Description was changed from

==========
Move language pak files to assets.

Language pak files were moved to res/raw when split apks was being implemented.
Moving them to assets with a java constant keeping track of pak file locations
will improve lookup time and save some space (no more xml files and
resources.arsc).

BUG=529604
==========

to

==========
Move language pak files to assets.

Language pak files were moved to res/raw when split apks was being implemented.
Moving them to assets with a java constant keeping track of pak file locations
will improve lookup time and save some space (no more xml files and
resources.arsc).

BUG=529604

Committed: https://crrev.com/34ae27889dd8c8ffd5f5ca8c7bc10c17e57c12e2
Cr-Commit-Position: refs/heads/master@{#420641}
==========

[commit-bot: I haz the power, 2016-09-23 17:21:47]

Patchset 13 (id:??) landed as
https://crrev.com/34ae27889dd8c8ffd5f5ca8c7bc10c17e57c12e2
Cr-Commit-Position: refs/heads/master@{#420641}

[Khushal, 2016-09-23 23:52:46]

A revert of this CL (patchset #13 id:240001) has been created in
https://codereview.chromium.org/2369683002/ by khushalsagar@chromium.org.

The reason for reverting is: Broke the internal build,
https://uberchromegw.corp.google.com/i/internal.client.clank_tot/builders/ins....