Load non-locale .pak files directly from the .apk on Android (rather than extracting on start-up).

Load non-locale .pak files directly from the .apk on Android (rather than extracting on start-up).

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

[agrieve, 2015-06-16 18:30:07]

agrieve@chromium.org changed reviewers:
+ cjhopman@chromium.org, gunsch@chromium.org, jochen@chromium.org,
sadrul@chromium.org, yfriedman@chromium.org

[agrieve, 2015-06-16 18:30:08]

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

sadrul@chromium.org: Please review changes in ui/

cjhopman@chromium.org: Please review changes in build/

yfriedman@chromium.org: Please review changes in */android

jochen@chromium.org: Please review changes in chrome/, content/

[gunsch, 2015-06-16 20:27:31]

chromecast/ lgtm, though it does feel like a lot of common content embedder
boilerplate :(

https://codereview.chromium.org/1181953002/diff/40001/ui/base/resource/resour...
File ui/base/resource/resource_bundle_android.h (right):

https://codereview.chromium.org/1181953002/diff/40001/ui/base/resource/resour...
ui/base/resource/resource_bundle_android.h:18: UI_BASE_EXPORT void
LoadMainAndroidPackFile();
This method doesn't appear to have a caller?

[agrieve, 2015-06-17 01:48:02]

https://codereview.chromium.org/1181953002/diff/40001/ui/base/resource/resour...
File ui/base/resource/resource_bundle_android.h (right):

https://codereview.chromium.org/1181953002/diff/40001/ui/base/resource/resour...
ui/base/resource/resource_bundle_android.h:18: UI_BASE_EXPORT void
LoadMainAndroidPackFile();
On 2015/06/16 20:27:31, gunsch wrote:
> This method doesn't appear to have a caller?

Yikes! Nice catch! Seems I had my branches crossed. It's properly used now in
chrome_browser_main.cc

[jochen (gone - plz use gerrit), 2015-06-17 08:47:54]

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

https://codereview.chromium.org/1181953002/diff/60001/content/public/browser/...
content/public/browser/content_browser_client.h:626: #if defined(OS_ANDROID)
please ifdef the entire method, not just part of its signature

[agrieve, 2015-06-17 13:43:43]

On 2015/06/16 20:27:31, gunsch wrote:
> chromecast/ lgtm, though it does feel like a lot of common content embedder
> boilerplate :(
> 
>
https://codereview.chromium.org/1181953002/diff/40001/ui/base/resource/resour...
> File ui/base/resource/resource_bundle_android.h (right):
> 
>
https://codereview.chromium.org/1181953002/diff/40001/ui/base/resource/resour...
> ui/base/resource/resource_bundle_android.h:18: UI_BASE_EXPORT void
> LoadMainAndroidPackFile();
> This method doesn't appear to have a caller?

Also - I do plan on removing the boiler plate call to ResourceExtractor once I
get the rest of the changes in to not need it in most cases.

[Yaron, 2015-06-17 14:12:29]

https://codereview.chromium.org/1181953002/diff/60001/chrome/app/chrome_main_...
File chrome/app/chrome_main_delegate.cc (left):

https://codereview.chromium.org/1181953002/diff/60001/chrome/app/chrome_main_...
chrome/app/chrome_main_delegate.cc:748: CHECK(locale_pak_fd != -1);
why are the CHECKs removed?

https://codereview.chromium.org/1181953002/diff/60001/components/test/android...
File
components/test/android/browsertests_apk/src/org/chromium/components_browsertests_apk/ComponentsBrowserTestsApplication.java
(left):

https://codereview.chromium.org/1181953002/diff/60001/components/test/android...
components/test/android/browsertests_apk/src/org/chromium/components_browsertests_apk/ComponentsBrowserTestsApplication.java:20:
private static final String PRIVATE_DATA_DIRECTORY_SUFFIX = "components_shell";
?

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

https://codereview.chromium.org/1181953002/diff/60001/content/public/browser/...
content/public/browser/content_browser_client.h:629: ) {};
remove ";"

https://codereview.chromium.org/1181953002/diff/60001/content/shell/app/shell...
File content/shell/app/shell_main_delegate.cc (right):

https://codereview.chromium.org/1181953002/diff/60001/content/shell/app/shell...
content/shell/app/shell_main_delegate.cc:314:
base::android::OpenApkAsset("assets/content_shell.pak", &pak_region);
Why does webview have to fallback to loading from path
(aw_browser_main_parts:84-89)?

https://codereview.chromium.org/1181953002/diff/60001/ui/base/resource/resour...
File ui/base/resource/resource_bundle_android.cc (left):

https://codereview.chromium.org/1181953002/diff/60001/ui/base/resource/resour...
ui/base/resource/resource_bundle_android.cc:24:
PathService::Get(ui::DIR_RESOURCE_PAKS_ANDROID, &path);
Will this constant only be removed when language paks are loaded from apk? Or
actually, language paks would only be loaded from apk for ChromeModern, right?

https://codereview.chromium.org/1181953002/diff/60001/ui/base/resource/resour...
File ui/base/resource/resource_bundle_android.cc (right):

https://codereview.chromium.org/1181953002/diff/60001/ui/base/resource/resour...
ui/base/resource/resource_bundle_android.cc:10: //#include
"base/files/file_util.h"
remove

https://codereview.chromium.org/1181953002/diff/60001/ui/base/resource/resour...
ui/base/resource/resource_bundle_android.cc:29: "assets/chrome_100_percent.pak",
&g_chrome_100_percent_region);
That's odd. I'm surprised that ui/ is dealing with "chrome" code and not having
it injected but it looks like that was already the case.

[agrieve, 2015-06-17 14:25:31]

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

https://codereview.chromium.org/1181953002/diff/60001/content/public/browser/...
content/public/browser/content_browser_client.h:626: #if defined(OS_ANDROID)
On 2015/06/17 08:47:54, jochen wrote:
> please ifdef the entire method, not just part of its signature

Done.

[agrieve, 2015-06-17 14:35:40]

https://codereview.chromium.org/1181953002/diff/60001/chrome/app/chrome_main_...
File chrome/app/chrome_main_delegate.cc (left):

https://codereview.chromium.org/1181953002/diff/60001/chrome/app/chrome_main_...
chrome/app/chrome_main_delegate.cc:748: CHECK(locale_pak_fd != -1);
On 2015/06/17 14:12:28, Yaron wrote:
> why are the CHECKs removed?

I switched it to use global_descriptors->Get instead of
global_descriptors->MaybeGet. The only diff between these is that Get CHECKs.

https://codereview.chromium.org/1181953002/diff/60001/components/test/android...
File
components/test/android/browsertests_apk/src/org/chromium/components_browsertests_apk/ComponentsBrowserTestsApplication.java
(left):

https://codereview.chromium.org/1181953002/diff/60001/components/test/android...
components/test/android/browsertests_apk/src/org/chromium/components_browsertests_apk/ComponentsBrowserTestsApplication.java:20:
private static final String PRIVATE_DATA_DIRECTORY_SUFFIX = "components_shell";
On 2015/06/17 14:12:28, Yaron wrote:
> ?

Done.

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

https://codereview.chromium.org/1181953002/diff/60001/content/public/browser/...
content/public/browser/content_browser_client.h:629: ) {};
On 2015/06/17 14:12:28, Yaron wrote:
> remove ";"

Done.

https://codereview.chromium.org/1181953002/diff/60001/content/shell/app/shell...
File content/shell/app/shell_main_delegate.cc (right):

https://codereview.chromium.org/1181953002/diff/60001/content/shell/app/shell...
content/shell/app/shell_main_delegate.cc:314:
base::android::OpenApkAsset("assets/content_shell.pak", &pak_region);
On 2015/06/17 14:12:28, Yaron wrote:
> Why does webview have to fallback to loading from path
> (aw_browser_main_parts:84-89)?

Likely for tests (where files are put there by isolate logic). Not positive
though.

https://codereview.chromium.org/1181953002/diff/60001/ui/base/resource/resour...
File ui/base/resource/resource_bundle_android.cc (left):

https://codereview.chromium.org/1181953002/diff/60001/ui/base/resource/resour...
ui/base/resource/resource_bundle_android.cc:24:
PathService::Get(ui::DIR_RESOURCE_PAKS_ANDROID, &path);
On 2015/06/17 14:12:28, Yaron wrote:
> Will this constant only be removed when language paks are loaded from apk? Or
> actually, language paks would only be loaded from apk for ChromeModern, right?

Only for ChromeModern, and we can't remove it unless we change all the tests to
load from apk rather than use isolate logic (which would be a lot of build logic
work - maybe wait for GN if we're going to do this...)

https://codereview.chromium.org/1181953002/diff/60001/ui/base/resource/resour...
File ui/base/resource/resource_bundle_android.cc (right):

https://codereview.chromium.org/1181953002/diff/60001/ui/base/resource/resour...
ui/base/resource/resource_bundle_android.cc:10: //#include
"base/files/file_util.h"
On 2015/06/17 14:12:28, Yaron wrote:
> remove

Done.

https://codereview.chromium.org/1181953002/diff/60001/ui/base/resource/resour...
ui/base/resource/resource_bundle_android.cc:29: "assets/chrome_100_percent.pak",
&g_chrome_100_percent_region);
On 2015/06/17 14:12:28, Yaron wrote:
> That's odd. I'm surprised that ui/ is dealing with "chrome" code and not
having
> it injected but it looks like that was already the case.

That caught me too. I think the original intention was that there was no
dependency, but then LoadCommonResources is defined by each _platform.cc file.
Maybe that was added later on?

Considered following the same pattern for LoadMainPackFile(), but didn't want to
further the layering violation if infact it is one.

[cjhopman, 2015-06-17 21:47:50]

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

https://codereview.chromium.org/1181953002/diff/100001/build/android/gyp/loca...
build/android/gyp/locale_pak_resources.py:36: return name.replace('-',
'_').replace('.pak', '.lpak').lower()
What do you think of changing it so that we just use a special extension for
things (these pak files, icudtl, maybe the v8 snapshots) that we want
uncompressed (like .uncompressed, or something shorter)?

Could maybe just handle it in the thing that opens apk assets, too.

https://codereview.chromium.org/1181953002/diff/100001/ui/base/resource/resou...
File ui/base/resource/resource_bundle_android.h (right):

https://codereview.chromium.org/1181953002/diff/100001/ui/base/resource/resou...
ui/base/resource/resource_bundle_android.h:18: UI_BASE_EXPORT void
LoadMainAndroidPackFile();
It seems that PakFile/PakFd is more commonly used than PackFile/PackFd.

[agrieve, 2015-06-18 15:50:41]

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

https://codereview.chromium.org/1181953002/diff/100001/build/android/gyp/loca...
build/android/gyp/locale_pak_resources.py:36: return name.replace('-',
'_').replace('.pak', '.lpak').lower()
On 2015/06/17 21:47:50, cjhopman wrote:
> What do you think of changing it so that we just use a special extension for
> things (these pak files, icudtl, maybe the v8 snapshots) that we want
> uncompressed (like .uncompressed, or something shorter)?
> 
> Could maybe just handle it in the thing that opens apk assets, too.
> 

I went down this path for a little while, but abandoned it:
- The build rules for assets are quite copy & pasted, and to rename the assets
for every target would be a lot of work.
- For locale paks, we want to store them uncompressed in splits, but compressed
in non-splits. Would complicate the code if they had different names.
- The aapt command for marking files as do-not-store actually does suffix
matches instead of just extension matches (so we do have the ability to be more
specific about which files to not compress if we wanted to)

https://codereview.chromium.org/1181953002/diff/100001/ui/base/resource/resou...
File ui/base/resource/resource_bundle_android.h (right):

https://codereview.chromium.org/1181953002/diff/100001/ui/base/resource/resou...
ui/base/resource/resource_bundle_android.h:18: UI_BASE_EXPORT void
LoadMainAndroidPackFile();
On 2015/06/17 21:47:50, cjhopman wrote:
> It seems that PakFile/PakFd is more commonly used than PackFile/PackFd.

Yeah, struggled with this since resource_bundle.h has both, and Pack seems more
prominent there. Ask again and I'll change it though :)

[Yaron, 2015-06-19 17:33:00]

lgtm

[cjhopman, 2015-06-19 22:58:41]

lgtm

[jochen (gone - plz use gerrit), 2015-06-22 08:37:30]

lgtm

[sadrul, 2015-06-22 20:35:04]

https://codereview.chromium.org/1181953002/diff/100001/ui/base/resource/resou...
File ui/base/resource/resource_bundle_android.cc (right):

https://codereview.chromium.org/1181953002/diff/100001/ui/base/resource/resou...
ui/base/resource/resource_bundle_android.cc:20: base::MemoryMappedFile::Region
g_resources_pack_region;
Does this add a couple of new static initializers?

[agrieve, 2015-06-22 23:53:33]

https://codereview.chromium.org/1181953002/diff/100001/ui/base/resource/resou...
File ui/base/resource/resource_bundle_android.cc (right):

https://codereview.chromium.org/1181953002/diff/100001/ui/base/resource/resou...
ui/base/resource/resource_bundle_android.cc:20: base::MemoryMappedFile::Region
g_resources_pack_region;
On 2015/06/22 20:35:03, sadrul (OOO until Jun.22) wrote:
> Does this add a couple of new static initializers?

It would have a couple of weeks ago, but after getting burned by this I changed
Region to be a POD, so it doesn't cause a static initializer.

[sadrul, 2015-06-23 08:20:26]

On 2015/06/22 23:53:33, agrieve wrote:
>
https://codereview.chromium.org/1181953002/diff/100001/ui/base/resource/resou...
> File ui/base/resource/resource_bundle_android.cc (right):
> 
>
https://codereview.chromium.org/1181953002/diff/100001/ui/base/resource/resou...
> ui/base/resource/resource_bundle_android.cc:20: base::MemoryMappedFile::Region
> g_resources_pack_region;
> On 2015/06/22 20:35:03, sadrul (OOO until Jun.22) wrote:
> > Does this add a couple of new static initializers?
> 
> It would have a couple of weeks ago, but after getting burned by this I
changed
> Region to be a POD, so it doesn't cause a static initializer.

lgtm

[agrieve, 2015-06-23 13:54:24]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2015-06-23 13:54:24]

The patchset sent to the CQ was uploaded after l-g-t-m from gunsch@chromium.org
Link to the patchset: https://codereview.chromium.org/1181953002/#ps100001
(title: "yfriedman review comments")

[commit-bot: I haz the power, 2015-06-23 13:55:00]

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

[commit-bot: I haz the power, 2015-06-23 13:57:43]

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

[commit-bot: I haz the power, 2015-06-23 13:57:45]

Exceeded global retry quota

[agrieve, 2015-06-23 15:20:11]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2015-06-23 15:20:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
cjhopman@chromium.org, yfriedman@chromium.org, jochen@chromium.org,
sadrul@chromium.org, gunsch@chromium.org
Link to the patchset: https://codereview.chromium.org/1181953002/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2015-06-23 15:20:53]

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

[commit-bot: I haz the power, 2015-06-23 15:46:47]

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

[commit-bot: I haz the power, 2015-06-23 15:46:49]

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

[agrieve, 2015-06-23 23:55:28]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2015-06-23 23:55:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
cjhopman@chromium.org, yfriedman@chromium.org, jochen@chromium.org,
sadrul@chromium.org, gunsch@chromium.org
Link to the patchset: https://codereview.chromium.org/1181953002/#ps180001
(title: "rebase2")

[commit-bot: I haz the power, 2015-06-23 23:57:03]

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

[commit-bot: I haz the power, 2015-06-24 02:40:53]

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

[commit-bot: I haz the power, 2015-06-24 02:40:55]

Try jobs failed on following builders:
  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-24 13:43:29]

The CQ bit was checked by agrieve@chromium.org

[commit-bot: I haz the power, 2015-06-24 13:44:16]

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

[commit-bot: I haz the power, 2015-06-24 16:21:48]

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

[commit-bot: I haz the power, 2015-06-24 16:21:50]

Try jobs failed on following builders:
  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-24 17:52:43]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2015-06-24 17:52:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
cjhopman@chromium.org, yfriedman@chromium.org, jochen@chromium.org,
sadrul@chromium.org, gunsch@chromium.org
Link to the patchset: https://codereview.chromium.org/1181953002/#ps200001
(title: "Fix content_browsertests")

[commit-bot: I haz the power, 2015-06-24 17:53:13]

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

[commit-bot: I haz the power, 2015-06-24 18:59:10]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-06-24 19:00:38]

Patchset 11 (id:??) landed as
https://crrev.com/05398f5f735ac47b070f7c6897b4d25a1aae8ca9
Cr-Commit-Position: refs/heads/master@{#335964}