Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(546)

Issue 2834603006: ABANDONED [android] Create per-package configuration for multidex. (Closed)

Created:
3 years, 8 months ago by jbudorick
Modified:
3 years, 8 months ago
Reviewers:
agrieve
CC:
chromium-reviews, danakj+watch_chromium.org, agrieve+watch_chromium.org, vmpstr+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Abandoned: Superseded by https://codereview.chromium.org/2836723002/ [android] Create per-package configuration for multidex. This will allow separate multidex configurations for instrumentation test APKs and the corresponding APKs under test. BUG=712852

Patch Set 1 #

Patch Set 2 : tweaks #

Total comments: 11
Unified diffs Side-by-side diffs Delta from patch set Stats (+80 lines, -22 lines) Patch
M base/BUILD.gn View 2 chunks +2 lines, -0 lines 0 comments Download
A base/android/java/src/org/chromium/base/PackageBuildConfig.java View 1 chunk +35 lines, -0 lines 8 comments Download
M base/android/java/src/org/chromium/base/multidex/ChromiumMultiDexInstaller.java View 4 chunks +3 lines, -4 lines 0 comments Download
M base/android/java/templates/BuildConfig.template View 1 chunk +0 lines, -14 lines 0 comments Download
A base/android/java/templates/PackageConfig.template View 1 1 chunk +18 lines, -0 lines 0 comments Download
M build/android/main_dex_classes.flags View 1 1 chunk +4 lines, -0 lines 3 comments Download
M build/config/android/rules.gni View 1 3 chunks +18 lines, -4 lines 0 comments Download

Messages

Total messages: 9 (2 generated)
jbudorick
3 years, 8 months ago (2017-04-21 01:04:02 UTC) #2
jbudorick
On 2017/04/21 01:04:02, jbudorick wrote: Actually, hold off on this. I'm working through a few ...
3 years, 8 months ago (2017-04-21 01:50:48 UTC) #3
jbudorick
ok, ptal
3 years, 8 months ago (2017-04-21 14:41:03 UTC) #4
agrieve
https://codereview.chromium.org/2834603006/diff/20001/base/android/java/src/org/chromium/base/PackageBuildConfig.java File base/android/java/src/org/chromium/base/PackageBuildConfig.java (right): https://codereview.chromium.org/2834603006/diff/20001/base/android/java/src/org/chromium/base/PackageBuildConfig.java#newcode27 base/android/java/src/org/chromium/base/PackageBuildConfig.java:27: loader.loadClass(String.format("%s.PackageConfig", packageName)); Would this technique work with using the ...
3 years, 8 months ago (2017-04-21 15:43:31 UTC) #5
jbudorick
https://codereview.chromium.org/2834603006/diff/20001/base/android/java/src/org/chromium/base/PackageBuildConfig.java File base/android/java/src/org/chromium/base/PackageBuildConfig.java (right): https://codereview.chromium.org/2834603006/diff/20001/base/android/java/src/org/chromium/base/PackageBuildConfig.java#newcode27 base/android/java/src/org/chromium/base/PackageBuildConfig.java:27: loader.loadClass(String.format("%s.PackageConfig", packageName)); On 2017/04/21 15:43:31, agrieve wrote: > Would ...
3 years, 8 months ago (2017-04-21 16:03:57 UTC) #6
agrieve
https://codereview.chromium.org/2834603006/diff/20001/base/android/java/src/org/chromium/base/PackageBuildConfig.java File base/android/java/src/org/chromium/base/PackageBuildConfig.java (right): https://codereview.chromium.org/2834603006/diff/20001/base/android/java/src/org/chromium/base/PackageBuildConfig.java#newcode27 base/android/java/src/org/chromium/base/PackageBuildConfig.java:27: loader.loadClass(String.format("%s.PackageConfig", packageName)); On 2017/04/21 16:03:57, jbudorick wrote: > On ...
3 years, 8 months ago (2017-04-21 16:48:42 UTC) #7
jbudorick
3 years, 8 months ago (2017-04-21 19:03:04 UTC) #8
not yet updated

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

https://codereview.chromium.org/2834603006/diff/20001/base/android/java/src/o...
base/android/java/src/org/chromium/base/PackageBuildConfig.java:27:
loader.loadClass(String.format("%s.PackageConfig", packageName));
On 2017/04/21 16:48:42, agrieve wrote:
> On 2017/04/21 16:03:57, jbudorick wrote:
> > On 2017/04/21 15:43:31, agrieve wrote:
> > > Would this technique work with using the existing BuildConfig class?
> Wondering
> > > if the explicit getClassLoader() is enough without needing to also create
> > > classes with different names.
> > 
> > It could. My intention was to keep the two separate for conceptual reasons
--
> > BuildConfig handles build-scope constants, while PackageBuildConfig handles
> > package-scope constants. Perhaps that's unnecessary, though.
> 
> I think I'd prefer having just one BuildConfig then. It's already per-package
> since it has COMPRESSED_LOCALES as well.

I was wrong -- if we want to keep the Log message at all, this needs to stay out
of template form. Thoughts?

https://codereview.chromium.org/2834603006/diff/20001/base/android/java/src/o...
base/android/java/src/org/chromium/base/PackageBuildConfig.java:31: Log.e(TAG,
"Unable to determine whether multidex is enabled for %s", packageName, e);
On 2017/04/21 16:48:42, agrieve wrote:
> On 2017/04/21 16:03:57, jbudorick wrote:
> > On 2017/04/21 15:43:31, agrieve wrote:
> > > Should this be a fatal error (e.g. don't bother catching it, or rethrow as
> > > RuntimeException). Does it ever happen in practice?
> > 
> > It shouldn't happen except when making changes for multidex, so I hit it a
lot
> > :)
> > 
> > As to whether or not it should be fatal -- given the current implementation
of
> > PackageConfig, probably, though not being able to find PackageConfig is fine
> for
> > single-dex APKs. Unsurprisingly, for multidex APKs, hitting this is very bad
> and
> > is absolutely fatal slightly later in the startup process.
> 
> Given your explanation, maybe better to make it a Log.d() so that it's
stripped
> in release binaries?

Done.

Powered by Google App Engine
This is Rietveld 408576698