[android_webview] Do not extract webviewchromium.pak

[android_webview] Do not extract webviewchromium.pak

- Change the android_webview_apk gyp files to include the en-US.pak
  file in the apk, which was erroneously missing.
- Add the do_not_compress gyp plumbing to store and zipalign paks in
  the apk (only for the android_webview_apk target).
- Stop decompressing pak files in the android webview shell apk.
- Change aw_browser_main_parts.cc to take advantage of the new AwAssets
  and DataPack/ResourceBundle *FromRegion methods (mmap from the apk).
- Add temporary workaround which loads always the en-US.pak locale, in
  lack of a proper refactoring of i10n_utils and ResourceBundle.

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

[Primiano Tucci (use gerrit), 2014-07-18 01:40:19]

Aaaand, this is the missing piece.

[mkosiba (inactive), 2014-07-18 05:54:22]

https://codereview.chromium.org/402603006/diff/1/android_webview/browser/aw_b...
File android_webview/browser/aw_browser_main_parts.cc (right):

https://codereview.chromium.org/402603006/diff/1/android_webview/browser/aw_b...
android_webview/browser/aw_browser_main_parts.cc:66: //
ui::ResourceBundle::GetSharedInstance().AddDataPackFromPath(
forgot to remove?

[benm (inactive), 2014-07-18 15:04:36]

So once we land this, we will always be trying to load webviewchromium.pak from
the APK, right? So we need to make sure that a WebView APK on the device has it
in place correctly before this merges downstream?

Maybe we should fallback to the old filename based approach for now so that we
can land this piece independently of a WebView APK being in place?

[Torne, 2014-07-18 16:13:30]

Yeah, we will need this to fall back for now otherwise android_aosp will
definitely break.

[Primiano Tucci (use gerrit), 2014-07-18 19:01:04]

I added the fallback + the gyp plumbing to skip pak compression only on our
webview shell apk

https://codereview.chromium.org/402603006/diff/1/android_webview/browser/aw_b...
File android_webview/browser/aw_browser_main_parts.cc (right):

https://codereview.chromium.org/402603006/diff/1/android_webview/browser/aw_b...
android_webview/browser/aw_browser_main_parts.cc:66: //
ui::ResourceBundle::GetSharedInstance().AddDataPackFromPath(
On 2014/07/18 05:54:22, mkosiba wrote:
> forgot to remove?
Oh, yeah.

[benm (inactive), 2014-07-18 21:10:22]

https://codereview.chromium.org/402603006/diff/60001/android_webview/test/she...
File
android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellApplication.java
(right):

https://codereview.chromium.org/402603006/diff/60001/android_webview/test/she...
android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellApplication.java:25:
"en-US.pak", "icudtl.dat"
As discussed offline; I think that if we update resource_bundle.cc to be able to
load the locale from a Region, we can get rid of this locale pak file too

[benm (inactive), 2014-07-20 16:32:35]

On 2014/07/18 21:10:22, benm wrote:
>
https://codereview.chromium.org/402603006/diff/60001/android_webview/test/she...
> File
>
android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellApplication.java
> (right):
> 
>
https://codereview.chromium.org/402603006/diff/60001/android_webview/test/she...
>
android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellApplication.java:25:
> "en-US.pak", "icudtl.dat"
> As discussed offline; I think that if we update resource_bundle.cc to be able
to
> load the locale from a Region, we can get rid of this locale pak file too

(For WebView, the locale paks are bundled into e.g
/system/framework/webview/paks/en-US.pak. They live as
webview_chromium_webkit_strings_en_US.pak during the build, but are renamed to
just the locale when bundled into the system image).

So, I actually think that the code in this patch to load the locale pak you have
right now should be sufficient, so should be able to just go ahead and remove
en-US.pak from MANDATORY_PAKS and check it all still works :-)

[mkosiba (inactive), 2014-07-21 14:20:59]

lgtm

https://codereview.chromium.org/402603006/diff/80001/android_webview/browser/...
File android_webview/browser/aw_browser_main_parts.cc (right):

https://codereview.chromium.org/402603006/diff/80001/android_webview/browser/...
android_webview/browser/aw_browser_main_parts.cc:59: VLOG(0) << "Loading " <<
locale_pak_file << " from FD: " << pak_fd << " OFF: " << pak_off << " LEN: " <<
pak_len;
aren't these lines a bit tooo loooonnnggg?

https://codereview.chromium.org/402603006/diff/80001/android_webview/browser/...
android_webview/browser/aw_browser_main_parts.cc:63:
/*should_load_common_resources=*/false);
nit: I _think_  it should be like: /*should_load_common_resources*/ false

https://codereview.chromium.org/402603006/diff/80001/android_webview/browser/...
android_webview/browser/aw_browser_main_parts.cc:64: } else {
does this make sense? If it's not there in the apk, the odds of us finding the
right file in the app's data dir are pretty slim. Maybe we should just crash
instead.

[benm (inactive), 2014-07-21 14:38:55]

https://codereview.chromium.org/402603006/diff/80001/android_webview/browser/...
File android_webview/browser/aw_browser_main_parts.cc (right):

https://codereview.chromium.org/402603006/diff/80001/android_webview/browser/...
android_webview/browser/aw_browser_main_parts.cc:64: } else {
Long term, yes we should remove this. But for now I think that we need the
fallback to /system for the WebView.

On 2014/07/21 14:20:58, mkosiba wrote:
> does this make sense? If it's not there in the apk, the odds of us finding the
> right file in the app's data dir are pretty slim. Maybe we should just crash
> instead.

[Primiano Tucci (use gerrit), 2014-07-21 15:21:33]

On 2014/07/20 16:32:35, benm wrote:
> On 2014/07/18 21:10:22, benm wrote:
> >
>
https://codereview.chromium.org/402603006/diff/60001/android_webview/test/she...
> > File
> >
>
android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellApplication.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/402603006/diff/60001/android_webview/test/she...
> >
>
android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellApplication.java:25:
> > "en-US.pak", "icudtl.dat"
> > As discussed offline; I think that if we update resource_bundle.cc to be
able
> to
> > load the locale from a Region, we can get rid of this locale pak file too
> 
> (For WebView, the locale paks are bundled into e.g
> /system/framework/webview/paks/en-US.pak. They live as
> webview_chromium_webkit_strings_en_US.pak during the build, but are renamed to
> just the locale when bundled into the system image).
> 
> So, I actually think that the code in this patch to load the locale pak you
have
> right now should be sufficient, so should be able to just go ahead and remove
> en-US.pak from MANDATORY_PAKS and check it all still works :-)

Actually, turns out I need to rework this patch a bit. In order to get the
proper locale, GetApplicationLocale in l10n_util builds up a list of candidates
and then selects the first among them looking at the existance of the pak files.
Hence, I've to rework that logic, because what I'm trying to do is deciding
which locale pak file I have to load from the apk upfront.
In other words, GetApplicationLocale depends on the pak files to determine the
final language. I want to know the locale in order to decide which pak file load
from the apk. There is a logic cyclic dependency that I have to break here to
support the locales.

[Primiano Tucci (use gerrit), 2014-07-21 17:20:03]

I reworked the CL to deal with the locale.
More details in the new commit message / aw_browser_main_parts.cc

https://codereview.chromium.org/402603006/diff/80001/android_webview/browser/...
File android_webview/browser/aw_browser_main_parts.cc (right):

https://codereview.chromium.org/402603006/diff/80001/android_webview/browser/...
android_webview/browser/aw_browser_main_parts.cc:59: VLOG(0) << "Loading " <<
locale_pak_file << " from FD: " << pak_fd << " OFF: " << pak_off << " LEN: " <<
pak_len;
On 2014/07/21 14:20:58, mkosiba wrote:
> aren't these lines a bit tooo loooonnnggg?
Yeah, that's why the patchset was entitled "WIP" ;-)

https://codereview.chromium.org/402603006/diff/80001/android_webview/browser/...
android_webview/browser/aw_browser_main_parts.cc:63:
/*should_load_common_resources=*/false);
On 2014/07/21 14:20:58, mkosiba wrote:
> nit: I _think_  it should be like: /*should_load_common_resources*/ false
A search in code search seems to suggest the contrary
(\/\*\w+\=\*\/ vs \/\*\w+\*\/)

https://codereview.chromium.org/402603006/diff/80001/android_webview/browser/...
android_webview/browser/aw_browser_main_parts.cc:64: } else {
Quoting Torne's previous comment in this CL:
"Yeah, we will need this to fall back for now otherwise android_aosp will
definitely break."

[benm (inactive), 2014-07-21 17:55:52]

seems like a reasonable compromise for now.

l-gtm when it compiles ;)

https://codereview.chromium.org/402603006/diff/100001/ui/base/resource/resour...
File ui/base/resource/resource_bundle.h (right):

https://codereview.chromium.org/402603006/diff/100001/ui/base/resource/resour...
ui/base/resource/resource_bundle.h:144: const base::File::Region& region,
needs a fwd decl for Region.

[Primiano Tucci (use gerrit), 2014-07-21 22:29:55]

>
https://codereview.chromium.org/402603006/diff/100001/ui/base/resource/resour...
> ui/base/resource/resource_bundle.h:144: const base::File::Region& region,
> needs a fwd decl for Region.

I was wondering why the heck it didn't happen locally, it took me a while.
Clearly this is based on top of crrev.com/394313002  :)

[Primiano Tucci (use gerrit), 2014-07-21 22:49:13]

> I was wondering why the heck it didn't happen locally, it took me a while.
> Clearly this is based on top of crrev.com/394313002  :)

FTR this is the squashed patch https://codereview.chromium.org/406113002/
currently running on tryjobs.

[benm (inactive), 2014-07-28 18:18:28]

+tony for ui/base/resource

Hi tony, we are trying to move very fast with this change, and it still needs a
little bit of refactoring in the android_webview layer. However the
ui/base/resource changes shouldn't change so if we could have your review there
we can continue in London tomorrow.

Many thanks!

[darin (slow to review), 2014-07-28 22:01:58]

tony@ -> sky@

[sky, 2014-07-28 22:09:57]

https://codereview.chromium.org/402603006/diff/120001/ui/base/resource/resour...
File ui/base/resource/resource_bundle.h (right):

https://codereview.chromium.org/402603006/diff/120001/ui/base/resource/resour...
ui/base/resource/resource_bundle.h:138: static void
InitSharedInstanceWithPakFile(base::File file,
InitSharedInstanceWithPakFile is only called in a couple of places. I would
rather you update this to take the region and update call sites.

[Primiano Tucci (use gerrit), 2014-07-29 11:07:36]

https://codereview.chromium.org/402603006/diff/120001/ui/base/resource/resour...
File ui/base/resource/resource_bundle.h (right):

https://codereview.chromium.org/402603006/diff/120001/ui/base/resource/resour...
ui/base/resource/resource_bundle.h:138: static void
InitSharedInstanceWithPakFile(base::File file,
On 2014/07/28 22:09:57, sky wrote:
> InitSharedInstanceWithPakFile is only called in a couple of places. I would
> rather you update this to take the region and update call sites.

Done.

[Torne, 2014-07-29 12:05:41]

lgtm

[Ben Goodger (Google), 2014-07-29 16:15:56]

lgtm

[tony, 2014-07-29 16:22:12]

Sorry, I was out of the office yesterday.  ui/base/resource LGTM.

[Primiano Tucci (use gerrit), 2014-07-29 16:52:15]

The CQ bit was checked by primiano@chromium.org

[commit-bot: I haz the power, 2014-07-29 16:53:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/primiano@chromium.org/402603006/160001

[sky, 2014-07-29 17:53:55]

LGTM

[commit-bot: I haz the power, 2014-07-29 18:11:36]

Change committed as 286258