Reland #2 of Fix monochrome not booting issue

Reland #2 of Move language pak files to assets

Reverted in:
https://codereview.chromium.org/2406093002/
and https://codereview.chromium.org/2404043002

Reason for reland:
Now auto-detects whether locale paks are compressed or stored
(for chrome, but not webview)

TBR=michaelbai
BUG=653771, 644377, 529604
Committed: https://crrev.com/732f0d89ca9c435e2bddfcfcdb9b108fb9b7ac0d
Cr-Commit-Position: refs/heads/master@{#425402}

[agrieve, 2016-10-11 20:07:37]

Created Revert of Fix monochrome not booting issue

[agrieve, 2016-10-11 20:19:14]

Description was changed from

==========
Revert of Fix monochrome not booting issue (patchset #1 id:1 of
https://codereview.chromium.org/2406093002/ )

Reason for revert:
Will update when fixed.

Original issue's description:
> Fix monochrome not booting issue
> 
> Revert these two CLs to have monochrome working again on ToT.
> 
> Revert "Move PathUtils to use ContextUtils."
>  commit 7200e9f34aa2e9006d8ea5540035de744b13d27d.
> Revert "Reland of Move language pak files to assets."
>  commit 67101cd0338a409d80689b9c9c875c95ccde7957.
> 
> BUG=653771,644377, 529604
> 
>
TBR==agrieve@chromium.org,tedchoc@chromium.org,slan@chromium.org,xunjieli@chr...,
michaelbai@chomium.org, estevenson@chromium.org
> 
> Committed: https://crrev.com/5cb262f964b4a158be02b5497462e2e8d170a59b
> Cr-Commit-Position: refs/heads/master@{#424345}

TBR=tedchoc@chromium.org,slan@chromium.org,xunjieli@chromium.org,torne@chromi...
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=653771,644377, 529604
==========

to

==========
Reland #2 of Fix monochrome not booting issue

Reverted in:
https://codereview.chromium.org/2406093002/
and https://codereview.chromium.org/2404043002

Reason for reland:
Now adds ui::SetLocalePaksStoredInApk(true) to
chrome/browser/android/monochrome_entry_point.cc

BUG=653771,644377, 529604
==========

[agrieve, 2016-10-11 20:37:47]

On 2016/10/11 20:07:37, agrieve wrote:
> Created Revert of Fix monochrome not booting issue

Tested locally that this fixes monochrome not booting. PS1 contains differences.

[agrieve, 2016-10-11 20:38:20]

agrieve@chromium.org changed reviewers:
- estevenson@chromium.org, nyquist@chromium.org, sgurun@chromium.org,
slan@chromium.org, tedchoc@chromium.org, wnwen@chromium.org,
xunjieli@chromium.org

[agrieve, 2016-10-11 20:38:40]

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

[commit-bot: I haz the power, 2016-10-11 20:39:16]

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-10-11 20:46:54]

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

[commit-bot: I haz the power, 2016-10-11 20:46:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[michaelbai, 2016-10-11 21:27:43]

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

[michaelbai, 2016-10-11 21:27:44]

https://codereview.chromium.org/2404253003/diff/300001/chrome/browser/android...
File chrome/browser/android/monochrome_entry_point.cc (right):

https://codereview.chromium.org/2404253003/diff/300001/chrome/browser/android...
chrome/browser/android/monochrome_entry_point.cc:27:
ui::SetLocalePaksStoredInApk(true);
I still prefer not to do this, because I want to minimize the Monochrome special
code, normally if pak is uncompressed in APK, we have no reason not to read it
from APK, so

- For Chrome, if the pak is uncompressed, we just load it from APK, otherwise
from disk.
- For WebView, we only load pak from APK, otherwise fail.

We don't need special case for Monochrome here.

Also BuildConfig.java already has information about whether the PAK is
compressed or not, why we have another separated global flag
(g_locale_paks_in_apk) for almost same purpose? The not booting bug is
essentially caused by conflict of those two sources of same information, right?
What we really want is to enforce WebView only load pak from APK.

[agrieve, 2016-10-12 02:27:26]

https://codereview.chromium.org/2404253003/diff/300001/chrome/browser/android...
File chrome/browser/android/monochrome_entry_point.cc (right):

https://codereview.chromium.org/2404253003/diff/300001/chrome/browser/android...
chrome/browser/android/monochrome_entry_point.cc:27:
ui::SetLocalePaksStoredInApk(true);
On 2016/10/11 21:27:44, michaelbai wrote:
> I still prefer not to do this, because I want to minimize the Monochrome
special
> code, normally if pak is uncompressed in APK, we have no reason not to read it
> from APK, so
> 
> - For Chrome, if the pak is uncompressed, we just load it from APK, otherwise
> from disk.
Just to be clear - this is *not* the existing behaviour. Right now (mono)chrome
only checks disk.

> - For WebView, we only load pak from APK, otherwise fail.
> 
> We don't need special case for Monochrome here.
> 
> Also BuildConfig.java already has information about whether the PAK is
> compressed or not, why we have another separated global flag
> (g_locale_paks_in_apk) for almost same purpose? The not booting bug is
> essentially caused by conflict of those two sources of same information,
right?
> What we really want is to enforce WebView only load pak from APK.

This is what I had implemented in the previous patch set. It checked
BuildConfig.java
to see whether "en-US" was stored compressed, and then acted accordingly for the
requested locale.
torne pointed out that if we have a packaging bug, it would just fail gracefully
(but to our detriment), which I think is a valid concern (and applies to both
webview and monochrome)

[michaelbai, 2016-10-12 02:39:43]

On 2016/10/12 02:27:26, agrieve wrote:
>
https://codereview.chromium.org/2404253003/diff/300001/chrome/browser/android...
> File chrome/browser/android/monochrome_entry_point.cc (right):
> 
>
https://codereview.chromium.org/2404253003/diff/300001/chrome/browser/android...
> chrome/browser/android/monochrome_entry_point.cc:27:
> ui::SetLocalePaksStoredInApk(true);
> On 2016/10/11 21:27:44, michaelbai wrote:
> > I still prefer not to do this, because I want to minimize the Monochrome
> special
> > code, normally if pak is uncompressed in APK, we have no reason not to read
it
> > from APK, so
> > 
> > - For Chrome, if the pak is uncompressed, we just load it from APK,
otherwise
> > from disk.
> Just to be clear - this is *not* the existing behaviour. Right now
(mono)chrome
> only checks disk.
> 
> > - For WebView, we only load pak from APK, otherwise fail.
> > 
> > We don't need special case for Monochrome here.
> > 
> > Also BuildConfig.java already has information about whether the PAK is
> > compressed or not, why we have another separated global flag
> > (g_locale_paks_in_apk) for almost same purpose? The not booting bug is
> > essentially caused by conflict of those two sources of same information,
> right?
> > What we really want is to enforce WebView only load pak from APK.
> 
> This is what I had implemented in the previous patch set. It checked
> BuildConfig.java
> to see whether "en-US" was stored compressed, and then acted accordingly for
the
> requested locale.
> torne pointed out that if we have a packaging bug, it would just fail
gracefully
> (but to our detriment), which I think is a valid concern (and applies to both
> webview and monochrome)

Right, based on your previous patch, what I suggested is to replace
g_locale_paks_in_apk with g_always_load_pak_from_apk( its value is false by
default), and set it to true from WebView, if g_always_load_pak_from_apk is true
and pak is compressed,  loading pak just fails.

[agrieve, 2016-10-12 02:51:20]

On 2016/10/12 02:39:43, michaelbai wrote:
> On 2016/10/12 02:27:26, agrieve wrote:
> >
>
https://codereview.chromium.org/2404253003/diff/300001/chrome/browser/android...
> > File chrome/browser/android/monochrome_entry_point.cc (right):
> > 
> >
>
https://codereview.chromium.org/2404253003/diff/300001/chrome/browser/android...
> > chrome/browser/android/monochrome_entry_point.cc:27:
> > ui::SetLocalePaksStoredInApk(true);
> > On 2016/10/11 21:27:44, michaelbai wrote:
> > > I still prefer not to do this, because I want to minimize the Monochrome
> > special
> > > code, normally if pak is uncompressed in APK, we have no reason not to
read
> it
> > > from APK, so
> > > 
> > > - For Chrome, if the pak is uncompressed, we just load it from APK,
> otherwise
> > > from disk.
> > Just to be clear - this is *not* the existing behaviour. Right now
> (mono)chrome
> > only checks disk.
> > 
> > > - For WebView, we only load pak from APK, otherwise fail.
> > > 
> > > We don't need special case for Monochrome here.
> > > 
> > > Also BuildConfig.java already has information about whether the PAK is
> > > compressed or not, why we have another separated global flag
> > > (g_locale_paks_in_apk) for almost same purpose? The not booting bug is
> > > essentially caused by conflict of those two sources of same information,
> > right?
> > > What we really want is to enforce WebView only load pak from APK.
> > 
> > This is what I had implemented in the previous patch set. It checked
> > BuildConfig.java
> > to see whether "en-US" was stored compressed, and then acted accordingly for
> the
> > requested locale.
> > torne pointed out that if we have a packaging bug, it would just fail
> gracefully
> > (but to our detriment), which I think is a valid concern (and applies to
both
> > webview and monochrome)
> 
> Right, based on your previous patch, what I suggested is to replace
> g_locale_paks_in_apk with g_always_load_pak_from_apk( its value is false by
> default), and set it to true from WebView, if g_always_load_pak_from_apk is
true
> and pak is compressed,  loading pak just fails.

I could rename it, but that's already what the flag does. I'm just enabling it
for monochrome here.

[Torne, 2016-10-12 10:35:55]

LGTM

https://codereview.chromium.org/2404253003/diff/300001/chrome/browser/android...
File chrome/browser/android/monochrome_entry_point.cc (right):

https://codereview.chromium.org/2404253003/diff/300001/chrome/browser/android...
chrome/browser/android/monochrome_entry_point.cc:27:
ui::SetLocalePaksStoredInApk(true);
On 2016/10/12 02:27:25, agrieve wrote:
> On 2016/10/11 21:27:44, michaelbai wrote:
> > I still prefer not to do this, because I want to minimize the Monochrome
> special
> > code, normally if pak is uncompressed in APK, we have no reason not to read
it
> > from APK, so
> > 
> > - For Chrome, if the pak is uncompressed, we just load it from APK,
otherwise
> > from disk.
> Just to be clear - this is *not* the existing behaviour. Right now
(mono)chrome
> only checks disk.

Oh, so monochrome is currently extracting the locale paks when launched as
chrome, even though it could just map them directly from the APK? I didn't
realise this was already broken :/

> > - For WebView, we only load pak from APK, otherwise fail.
> > 
> > We don't need special case for Monochrome here.

Yes we do. Monochrome is packaged differently from regular chrome, on purpose,
and if that packaging doesn't work correctly we have a bug that we want to
notice as quickly as possible.

> > Also BuildConfig.java already has information about whether the PAK is
> > compressed or not, why we have another separated global flag
> > (g_locale_paks_in_apk) for almost same purpose? The not booting bug is
> > essentially caused by conflict of those two sources of same information,
> right?
> > What we really want is to enforce WebView only load pak from APK.

What we want to enforce is that APKs where the locales are expected to be stored
uncompressed (webview and monochrome) always load directly from the APK, so that
if we mess up the configuration it will immediately crash rather than trying to
fall back to extracting the files, which at best will waste disk space and at
worst will simply not work (in the webview case).

> 
> This is what I had implemented in the previous patch set. It checked
> BuildConfig.java
> to see whether "en-US" was stored compressed, and then acted accordingly for
the
> requested locale.
> torne pointed out that if we have a packaging bug, it would just fail
gracefully
> (but to our detriment), which I think is a valid concern (and applies to both
> webview and monochrome)

[michaelbai, 2016-10-12 16:39:49]

https://codereview.chromium.org/2404253003/diff/300001/chrome/browser/android...
File chrome/browser/android/monochrome_entry_point.cc (right):

https://codereview.chromium.org/2404253003/diff/300001/chrome/browser/android...
chrome/browser/android/monochrome_entry_point.cc:27:
ui::SetLocalePaksStoredInApk(true);
On 2016/10/12 02:27:25, agrieve wrote:
> On 2016/10/11 21:27:44, michaelbai wrote:
> > I still prefer not to do this, because I want to minimize the Monochrome
> special
> > code, normally if pak is uncompressed in APK, we have no reason not to read
it
> > from APK, so
> > 
> > - For Chrome, if the pak is uncompressed, we just load it from APK,
otherwise
> > from disk.
> Just to be clear - this is *not* the existing behaviour. Right now
(mono)chrome
> only checks disk.
> 
> > - For WebView, we only load pak from APK, otherwise fail.
> > 
> > We don't need special case for Monochrome here.
> > 
> > Also BuildConfig.java already has information about whether the PAK is
> > compressed or not, why we have another separated global flag
> > (g_locale_paks_in_apk) for almost same purpose? The not booting bug is
> > essentially caused by conflict of those two sources of same information,
> right?
> > What we really want is to enforce WebView only load pak from APK.
> 
> This is what I had implemented in the previous patch set. It checked
> BuildConfig.java
> to see whether "en-US" was stored compressed, and then acted accordingly for
the
> requested locale.
> torne pointed out that if we have a packaging bug, it would just fail
gracefully
> (but to our detriment), which I think is a valid concern (and applies to both
> webview and monochrome)

I understood you want both WebView and Chrome in monochrome are enforced to load
PAK from APK, my point is that if we have code to 
- load PAK from APK as long as PAK is not compressed( like what you did in
previous patch).
- set a flag to enforce only load PAK from APK from WebView (not here, but from
WebView) 

We can get the same goal, though Chrome in monochrome is not enforced, but if
PAK is compressed in monochrome APK, the issue is easy to be found from WebView
side. Monochrome doesn't need this code and this is not good place to put this
code, (actually monochrome has no good place to put this code now), Monochrome
should just a simple combination of WebView and Chrome at least from native
side.

[Torne, 2016-10-12 16:42:32]

On 2016/10/12 16:39:49, michaelbai wrote:
>
https://codereview.chromium.org/2404253003/diff/300001/chrome/browser/android...
> File chrome/browser/android/monochrome_entry_point.cc (right):
> 
>
https://codereview.chromium.org/2404253003/diff/300001/chrome/browser/android...
> chrome/browser/android/monochrome_entry_point.cc:27:
> ui::SetLocalePaksStoredInApk(true);
> On 2016/10/12 02:27:25, agrieve wrote:
> > On 2016/10/11 21:27:44, michaelbai wrote:
> > > I still prefer not to do this, because I want to minimize the Monochrome
> > special
> > > code, normally if pak is uncompressed in APK, we have no reason not to
read
> it
> > > from APK, so
> > > 
> > > - For Chrome, if the pak is uncompressed, we just load it from APK,
> otherwise
> > > from disk.
> > Just to be clear - this is *not* the existing behaviour. Right now
> (mono)chrome
> > only checks disk.
> > 
> > > - For WebView, we only load pak from APK, otherwise fail.
> > > 
> > > We don't need special case for Monochrome here.
> > > 
> > > Also BuildConfig.java already has information about whether the PAK is
> > > compressed or not, why we have another separated global flag
> > > (g_locale_paks_in_apk) for almost same purpose? The not booting bug is
> > > essentially caused by conflict of those two sources of same information,
> > right?
> > > What we really want is to enforce WebView only load pak from APK.
> > 
> > This is what I had implemented in the previous patch set. It checked
> > BuildConfig.java
> > to see whether "en-US" was stored compressed, and then acted accordingly for
> the
> > requested locale.
> > torne pointed out that if we have a packaging bug, it would just fail
> gracefully
> > (but to our detriment), which I think is a valid concern (and applies to
both
> > webview and monochrome)
> 
> I understood you want both WebView and Chrome in monochrome are enforced to
load
> PAK from APK, my point is that if we have code to 
> - load PAK from APK as long as PAK is not compressed( like what you did in
> previous patch).
> - set a flag to enforce only load PAK from APK from WebView (not here, but
from
> WebView) 
> 
> We can get the same goal, though Chrome in monochrome is not enforced, but if
> PAK is compressed in monochrome APK, the issue is easy to be found from
WebView
> side. Monochrome doesn't need this code and this is not good place to put this
> code, (actually monochrome has no good place to put this code now), Monochrome
> should just a simple combination of WebView and Chrome at least from native
> side.

I don't think that's true currently, nor should it be true. Monochrome can and
should behave differently to Chrome, because it's packaged differently due to
its higher minimum API level, and should be taking advantage of this. There's
multiple areas where this has come up already: JNI registration, library
loading; some of these are already different, and some of these *should* be made
different in future.

[michaelbai, 2016-10-12 16:50:53]

On 2016/10/12 16:42:32, Torne wrote:
> On 2016/10/12 16:39:49, michaelbai wrote:
> >
>
https://codereview.chromium.org/2404253003/diff/300001/chrome/browser/android...
> > File chrome/browser/android/monochrome_entry_point.cc (right):
> > 
> >
>
https://codereview.chromium.org/2404253003/diff/300001/chrome/browser/android...
> > chrome/browser/android/monochrome_entry_point.cc:27:
> > ui::SetLocalePaksStoredInApk(true);
> > On 2016/10/12 02:27:25, agrieve wrote:
> > > On 2016/10/11 21:27:44, michaelbai wrote:
> > > > I still prefer not to do this, because I want to minimize the Monochrome
> > > special
> > > > code, normally if pak is uncompressed in APK, we have no reason not to
> read
> > it
> > > > from APK, so
> > > > 
> > > > - For Chrome, if the pak is uncompressed, we just load it from APK,
> > otherwise
> > > > from disk.
> > > Just to be clear - this is *not* the existing behaviour. Right now
> > (mono)chrome
> > > only checks disk.
> > > 
> > > > - For WebView, we only load pak from APK, otherwise fail.
> > > > 
> > > > We don't need special case for Monochrome here.
> > > > 
> > > > Also BuildConfig.java already has information about whether the PAK is
> > > > compressed or not, why we have another separated global flag
> > > > (g_locale_paks_in_apk) for almost same purpose? The not booting bug is
> > > > essentially caused by conflict of those two sources of same information,
> > > right?
> > > > What we really want is to enforce WebView only load pak from APK.
> > > 
> > > This is what I had implemented in the previous patch set. It checked
> > > BuildConfig.java
> > > to see whether "en-US" was stored compressed, and then acted accordingly
for
> > the
> > > requested locale.
> > > torne pointed out that if we have a packaging bug, it would just fail
> > gracefully
> > > (but to our detriment), which I think is a valid concern (and applies to
> both
> > > webview and monochrome)
> > 
> > I understood you want both WebView and Chrome in monochrome are enforced to
> load
> > PAK from APK, my point is that if we have code to 
> > - load PAK from APK as long as PAK is not compressed( like what you did in
> > previous patch).
> > - set a flag to enforce only load PAK from APK from WebView (not here, but
> from
> > WebView) 
> > 
> > We can get the same goal, though Chrome in monochrome is not enforced, but
if
> > PAK is compressed in monochrome APK, the issue is easy to be found from
> WebView
> > side. Monochrome doesn't need this code and this is not good place to put
this
> > code, (actually monochrome has no good place to put this code now),
Monochrome
> > should just a simple combination of WebView and Chrome at least from native
> > side.
> 
> I don't think that's true currently, nor should it be true. Monochrome can and
> should behave differently to Chrome, because it's packaged differently due to
> its higher minimum API level, and should be taking advantage of this. There's
> multiple areas where this has come up already: JNI registration, library
> loading; some of these are already different, and some of these *should* be
made
> different in future.

The current difference(all are JNI stuff) is no way to work around, but this one
isn't necessary, even in the future, we should try to implement the feature in
either chrome or webview side, I don't want to see there is monochrome directory
along with chrome and android_webview, at least not the near future.

[agrieve, 2016-10-12 16:59:45]

On 2016/10/12 16:39:49, michaelbai wrote:
>
https://codereview.chromium.org/2404253003/diff/300001/chrome/browser/android...
> File chrome/browser/android/monochrome_entry_point.cc (right):
> 
>
https://codereview.chromium.org/2404253003/diff/300001/chrome/browser/android...
> chrome/browser/android/monochrome_entry_point.cc:27:
> ui::SetLocalePaksStoredInApk(true);
> On 2016/10/12 02:27:25, agrieve wrote:
> > On 2016/10/11 21:27:44, michaelbai wrote:
> > > I still prefer not to do this, because I want to minimize the Monochrome
> > special
> > > code, normally if pak is uncompressed in APK, we have no reason not to
read
> it
> > > from APK, so
> > > 
> > > - For Chrome, if the pak is uncompressed, we just load it from APK,
> otherwise
> > > from disk.
> > Just to be clear - this is *not* the existing behaviour. Right now
> (mono)chrome
> > only checks disk.
> > 
> > > - For WebView, we only load pak from APK, otherwise fail.
> > > 
> > > We don't need special case for Monochrome here.
> > > 
> > > Also BuildConfig.java already has information about whether the PAK is
> > > compressed or not, why we have another separated global flag
> > > (g_locale_paks_in_apk) for almost same purpose? The not booting bug is
> > > essentially caused by conflict of those two sources of same information,
> > right?
> > > What we really want is to enforce WebView only load pak from APK.
> > 
> > This is what I had implemented in the previous patch set. It checked
> > BuildConfig.java
> > to see whether "en-US" was stored compressed, and then acted accordingly for
> the
> > requested locale.
> > torne pointed out that if we have a packaging bug, it would just fail
> gracefully
> > (but to our detriment), which I think is a valid concern (and applies to
both
> > webview and monochrome)
> 
> I understood you want both WebView and Chrome in monochrome are enforced to
load
> PAK from APK, my point is that if we have code to 
> - load PAK from APK as long as PAK is not compressed( like what you did in
> previous patch).
> - set a flag to enforce only load PAK from APK from WebView (not here, but
from
> WebView) 
> 
> We can get the same goal, though Chrome in monochrome is not enforced, but if
> PAK is compressed in monochrome APK, the issue is easy to be found from
WebView
> side. Monochrome doesn't need this code and this is not good place to put this
> code, (actually monochrome has no good place to put this code now), Monochrome
> should just a simple combination of WebView and Chrome at least from native
> side.

I don't understand your objection here. Why should it just be a simple
combination? Shouldn't it contain tweaks for the things that make it different
(e.g. a different entry point, and different resource-related logic)?

I don't want to have both a "should I load from apk" flag as well as
auto-detection. Let's try and pick one or the other.

[Torne, 2016-10-12 17:01:08]

There is no good reason to treat Monochrome's Chrome and WebView entry points
differently in relation to pak loading. They are literally the exact same pak
files, by definition; it's the same APK. This CL makes them get treated the
same; that's what we want.

On 2016/10/12 16:50:53, michaelbai wrote:
> The current difference(all are JNI stuff) is no way to work around, but this
one
> isn't necessary, even in the future, we should try to implement the feature in
> either chrome or webview side, I don't want to see there is monochrome
directory
> along with chrome and android_webview, at least not the near future.

I don't think your position here is sustainable for the future. Monochrome
already has a unique method of loading the library that's not going away, and
the set of things for which Monochrome is not the same as Chrome is only going
to get larger. The entry point is already a separate implementation in a
different file.

If there's a better way to arrange this CL than putting it in the entry point,
then we could do something different. If that involves creating new source files
containing monochrome-specific code in the interests of having a clean, logical
structure, then I think that's fine, and if we end up with enough files that
having them in their own directory makes sense, then we *should* have a
monochrome directory alongside chrome and android_webview. I don't understand
your aversion to this idea.

[michaelbai, 2016-10-12 17:08:20]

On 2016/10/12 16:59:45, agrieve wrote:
> On 2016/10/12 16:39:49, michaelbai wrote:
> >
>
https://codereview.chromium.org/2404253003/diff/300001/chrome/browser/android...
> > File chrome/browser/android/monochrome_entry_point.cc (right):
> > 
> >
>
https://codereview.chromium.org/2404253003/diff/300001/chrome/browser/android...
> > chrome/browser/android/monochrome_entry_point.cc:27:
> > ui::SetLocalePaksStoredInApk(true);
> > On 2016/10/12 02:27:25, agrieve wrote:
> > > On 2016/10/11 21:27:44, michaelbai wrote:
> > > > I still prefer not to do this, because I want to minimize the Monochrome
> > > special
> > > > code, normally if pak is uncompressed in APK, we have no reason not to
> read
> > it
> > > > from APK, so
> > > > 
> > > > - For Chrome, if the pak is uncompressed, we just load it from APK,
> > otherwise
> > > > from disk.
> > > Just to be clear - this is *not* the existing behaviour. Right now
> > (mono)chrome
> > > only checks disk.
> > > 
> > > > - For WebView, we only load pak from APK, otherwise fail.
> > > > 
> > > > We don't need special case for Monochrome here.
> > > > 
> > > > Also BuildConfig.java already has information about whether the PAK is
> > > > compressed or not, why we have another separated global flag
> > > > (g_locale_paks_in_apk) for almost same purpose? The not booting bug is
> > > > essentially caused by conflict of those two sources of same information,
> > > right?
> > > > What we really want is to enforce WebView only load pak from APK.
> > > 
> > > This is what I had implemented in the previous patch set. It checked
> > > BuildConfig.java
> > > to see whether "en-US" was stored compressed, and then acted accordingly
for
> > the
> > > requested locale.
> > > torne pointed out that if we have a packaging bug, it would just fail
> > gracefully
> > > (but to our detriment), which I think is a valid concern (and applies to
> both
> > > webview and monochrome)
> > 
> > I understood you want both WebView and Chrome in monochrome are enforced to
> load
> > PAK from APK, my point is that if we have code to 
> > - load PAK from APK as long as PAK is not compressed( like what you did in
> > previous patch).
> > - set a flag to enforce only load PAK from APK from WebView (not here, but
> from
> > WebView) 
> > 
> > We can get the same goal, though Chrome in monochrome is not enforced, but
if
> > PAK is compressed in monochrome APK, the issue is easy to be found from
> WebView
> > side. Monochrome doesn't need this code and this is not good place to put
this
> > code, (actually monochrome has no good place to put this code now),
Monochrome
> > should just a simple combination of WebView and Chrome at least from native
> > side.
> 
> I don't understand your objection here. Why should it just be a simple
> combination? Shouldn't it contain tweaks for the things that make it different
> (e.g. a different entry point, and different resource-related logic)?
> 
> I don't want to have both a "should I load from apk" flag as well as
> auto-detection. Let's try and pick one or the other.

OK,

First of all, I think we all agree that the difference isn't  necessary .
Second, I don't like there are 2 places to do same thing either,  I suggested to
use auto-detection + enforce from WebView.
Finally, If this difference is must-have, JNI_OnLoad is not the right place to
add this code, we should have Monochrome specific content embedder, just like
current chrome and webview does, I think we don't want to make things
complicated now.

[Torne, 2016-10-12 17:13:32]

On 2016/10/12 17:08:20, michaelbai wrote:
> First of all, I think we all agree that the difference isn't  necessary .

I'm not sure what you mean; it sounds to me like we are in pretty significant
disagreement about what needs to be different and what doesn't.

> Second, I don't like there are 2 places to do same thing either,  I suggested
to
> use auto-detection + enforce from WebView.

Having multiple strategies has resulted in *numerous* demonstrable bugs in the
past. Enforcing everything that we possibly can and *not* supporting multiple
configurations unnecessarily is how we avoid this. We should *never* rely on the
environment to give us the right information if there's a way for us to know the
correct information intrinsically.

> Finally, If this difference is must-have, JNI_OnLoad is not the right place to
> add this code, we should have Monochrome specific content embedder, just like
> current chrome and webview does, I think we don't want to make things
> complicated now.

I'm not sure what you mean by a Monochrome-specific content embedder; this
doesn't seem to have anything to do with the content layer. If you want to
propose a better place to put this, then please do; you're the most familiar
with how Monochrome is structured. It doesn't have to be in the entry point, but
it *should* be somewhere in Monochrome to make sure we are doing the right
thing.

[michaelbai, 2016-10-12 18:36:50]

On 2016/10/12 17:13:32, Torne wrote:
> On 2016/10/12 17:08:20, michaelbai wrote:
> > First of all, I think we all agree that the difference isn't  necessary .
> 
> I'm not sure what you mean; it sounds to me like we are in pretty significant
> disagreement about what needs to be different and what doesn't.

In my previous comments, I suggested a new way that enforce WebView to only load
PAK from APK, and resolve the issue that there are 2 code to do the same thing,
and this way also avoid the specific code in Monochrome, since you didn’t say
any concern about it, I assumed you agreed with it, here I repeat my suggestion
in detail, please let me know your concern.

- In resource_bundle_android.cc, replace g_locale_paks_in_apk with
g_always_load_paks_from_apk, also change SetLocalePaksStoredInApk() to
SetAlwaysLoadPaksFromAPK()
- Replace SetLocalePaksStoredInApk with SetAlwaysLoadPaksFromAPK(true) only in
aw_browser_main_parts.cc, and remove other calls to SetLocalePaksStoredInApk()
- In resource_bundle_android.cc,  if g_always_load_paks_from_apk is set, alway
load PAK from APK, never fallback to local disk, otherwise if PAK file is
uncompressed in APK, read it from APK, otherwise from disk.


> 
> > Second, I don't like there are 2 places to do same thing either,  I
suggested
> to
> > use auto-detection + enforce from WebView.
> 
> Having multiple strategies has resulted in *numerous* demonstrable bugs in the
> past. Enforcing everything that we possibly can and *not* supporting multiple
> configurations unnecessarily is how we avoid this. We should *never* rely on
the
> environment to give us the right information if there's a way for us to know
the
> correct information intrinsically.
> 
> > Finally, If this difference is must-have, JNI_OnLoad is not the right place
to
> > add this code, we should have Monochrome specific content embedder, just
like
> > current chrome and webview does, I think we don't want to make things
> > complicated now.
> 
> I'm not sure what you mean by a Monochrome-specific content embedder; this
> doesn't seem to have anything to do with the content layer. If you want to
> propose a better place to put this, then please do; you're the most familiar
> with how Monochrome is structured. It doesn't have to be in the entry point,
but
> it *should* be somewhere in Monochrome to make sure we are doing the right
> thing.

As I said, there is no good place to put this code in monochrome now, if you
check where SetLocalePaksStoredInApk in currently called in Chrome and WebView,
the reasonable solution probably is having Monochrome to implement its own
content::BrowserMainParts, and put it there, or we might need something totally
new, fortunately, we don’t need to consider about it now since to call
SetLocalePaksStoredInApk is not necessary for Monochrome.

[Torne, 2016-10-13 14:44:39]

OK; if you feel it doesn't belong in the entry point we can leave it out for
now; as you say we're not that likely to fail to notice a packaging problem in
here because it's still enforced by the webview codepath in monochrome.

Is there already a bug filed to track the various general possible improvements
to monochrome we've talked about in this and other CLs/bugs? (like not doing
manual JNI registration, or doing stuff to make sure packaging is correct like
this CL) If there is, we should mention this on it as another thing that could
be improved in future when we have a chance to think about how to best allow
monochrome's behaviour to be different in some cases, and if not we should have
a bug about that I guess..

[michaelbai, 2016-10-13 16:07:53]

On 2016/10/13 14:44:39, Torne wrote:
> OK; if you feel it doesn't belong in the entry point we can leave it out for
> now; as you say we're not that likely to fail to notice a packaging problem in
> here because it's still enforced by the webview codepath in monochrome.
>

Yes, let's not add this call in JNI_OnLoad, and use other way to achieve same
goal. 
 
> Is there already a bug filed to track the various general possible
improvements
> to monochrome we've talked about in this and other CLs/bugs? (like not doing
> manual JNI registration, or doing stuff to make sure packaging is correct like
> this CL) If there is, we should mention this on it as another thing that could
> be improved in future when we have a chance to think about how to best allow
> monochrome's behaviour to be different in some cases, and if not we should
have
> a bug about that I guess..


We don't have a general bug, I will create one and link others to it, also file
a new bug to cover this one.

[agrieve, 2016-10-14 14:44:12]

On 2016/10/13 16:07:53, michaelbai wrote:
> On 2016/10/13 14:44:39, Torne wrote:
> > OK; if you feel it doesn't belong in the entry point we can leave it out for
> > now; as you say we're not that likely to fail to notice a packaging problem
in
> > here because it's still enforced by the webview codepath in monochrome.
> >
> 
> Yes, let's not add this call in JNI_OnLoad, and use other way to achieve same
> goal. 
>  
> > Is there already a bug filed to track the various general possible
> improvements
> > to monochrome we've talked about in this and other CLs/bugs? (like not doing
> > manual JNI registration, or doing stuff to make sure packaging is correct
like
> > this CL) If there is, we should mention this on it as another thing that
could
> > be improved in future when we have a chance to think about how to best allow
> > monochrome's behaviour to be different in some cases, and if not we should
> have
> > a bug about that I guess..
> 
> 
> We don't have a general bug, I will create one and link others to it, also
file
> a new bug to cover this one.

Alright, thanks for working that out! :)

I've updated the change so the default is to look only in the apk for locale pak
files, and chrome main now has auto-detect logic.

[Torne, 2016-10-14 14:59:52]

LGTM, sorry for the delay on this one Andrew :)

[michaelbai, 2016-10-14 15:48:49]

thanks for update patch, LGTM with nit, the description needs update,

[michaelbai, 2016-10-14 16:04:20]

Probably a potential issue, since the default value of flag is changed,
g_locale_paks_in_apk = !g_locale_paks_on_disk, I don't know if there are APKs
rely on  g_locale_paks_in_apk = false.

[agrieve, 2016-10-14 16:07:41]

Description was changed from

==========
Reland #2 of Fix monochrome not booting issue

Reverted in:
https://codereview.chromium.org/2406093002/
and https://codereview.chromium.org/2404043002

Reason for reland:
Now adds ui::SetLocalePaksStoredInApk(true) to
chrome/browser/android/monochrome_entry_point.cc

BUG=653771,644377, 529604
==========

to

==========
Reland #2 of Move language pak files to assets

Reverted in:
https://codereview.chromium.org/2406093002/
and https://codereview.chromium.org/2404043002

Reason for reland:
Now auto-detects whether locale paks are compressed or stored
(for chrome, but not webview)

BUG=653771,644377, 529604
==========

[Ted C, 2016-10-14 16:22:13]

On 2016/10/14 16:04:20, michaelbai wrote:
> Probably a potential issue, since the default value of flag is changed,
> g_locale_paks_in_apk = !g_locale_paks_on_disk, I don't know if there are APKs
> rely on  g_locale_paks_in_apk = false.

rs lgtm

[agrieve, 2016-10-14 17:44:09]

On 2016/10/14 16:04:20, michaelbai wrote:
> Probably a potential issue, since the default value of flag is changed,
> g_locale_paks_in_apk = !g_locale_paks_on_disk, I don't know if there are APKs
> rely on  g_locale_paks_in_apk = false.

Good point. I think it's actually just tests that this affects, but it's just as
easy to put the default back to what it was and have webview explicitly set it
to true for now. I've changed back the default.

[agrieve, 2016-10-14 17:45:29]

Description was changed from

==========
Reland #2 of Move language pak files to assets

Reverted in:
https://codereview.chromium.org/2406093002/
and https://codereview.chromium.org/2404043002

Reason for reland:
Now auto-detects whether locale paks are compressed or stored
(for chrome, but not webview)

BUG=653771,644377, 529604
==========

to

==========
Reland #2 of Move language pak files to assets

Reverted in:
https://codereview.chromium.org/2406093002/
and https://codereview.chromium.org/2404043002

Reason for reland:
Now auto-detects whether locale paks are compressed or stored
(for chrome, but not webview)

TBR=michaelbai
BUG=653771,644377, 529604
==========

[agrieve, 2016-10-14 17:46:06]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2016-10-14 17:46:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelbai@chromium.org, tedchoc@chromium.org, torne@chromium.org
Link to the patchset: https://codereview.chromium.org/2404253003/#ps360001
(title: "Change default back to loading locales from disk")

[commit-bot: I haz the power, 2016-10-14 17:46:32]

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-10-14 18:49:39]

Description was changed from

==========
Reland #2 of Move language pak files to assets

Reverted in:
https://codereview.chromium.org/2406093002/
and https://codereview.chromium.org/2404043002

Reason for reland:
Now auto-detects whether locale paks are compressed or stored
(for chrome, but not webview)

TBR=michaelbai
BUG=653771,644377, 529604
==========

to

==========
Reland #2 of Move language pak files to assets

Reverted in:
https://codereview.chromium.org/2406093002/
and https://codereview.chromium.org/2404043002

Reason for reland:
Now auto-detects whether locale paks are compressed or stored
(for chrome, but not webview)

TBR=michaelbai
BUG=653771,644377, 529604
==========

[commit-bot: I haz the power, 2016-10-14 18:49:42]

Committed patchset #5 (id:360001)

[commit-bot: I haz the power, 2016-10-14 18:55:06]

Description was changed from

==========
Reland #2 of Move language pak files to assets

Reverted in:
https://codereview.chromium.org/2406093002/
and https://codereview.chromium.org/2404043002

Reason for reland:
Now auto-detects whether locale paks are compressed or stored
(for chrome, but not webview)

TBR=michaelbai
BUG=653771,644377, 529604
==========

to

==========
Reland #2 of Move language pak files to assets

Reverted in:
https://codereview.chromium.org/2406093002/
and https://codereview.chromium.org/2404043002

Reason for reland:
Now auto-detects whether locale paks are compressed or stored
(for chrome, but not webview)

TBR=michaelbai
BUG=653771,644377, 529604

Committed: https://crrev.com/732f0d89ca9c435e2bddfcfcdb9b108fb9b7ac0d
Cr-Commit-Position: refs/heads/master@{#425402}
==========

[commit-bot: I haz the power, 2016-10-14 18:55:09]

Patchset 5 (id:??) landed as
https://crrev.com/732f0d89ca9c435e2bddfcfcdb9b108fb9b7ac0d
Cr-Commit-Position: refs/heads/master@{#425402}