[Android] Enable multidex for debug builds of ChromePublic.

[Android] Enable multidex for debug builds of ChromePublic.

This is a non-incremental implementation. It adds @MainDex to classes used by
the renderer to keep them in the main dex, as it can't load secondary dexes
when using the multidex support library (i.e., on K and below).

BUG=272790
TBR=newt@chromium.org,sky@chromium.org

Committed: https://crrev.com/f667d3b3a1b953800bb5325c7b8d62775a1c5aeb
Cr-Commit-Position: refs/heads/master@{#359453}

[jbudorick, 2015-10-30 00:55:39]

jbudorick@chromium.org changed reviewers:
+ agrieve@chromium.org, yfriedman@chromium.org

[jbudorick, 2015-10-30 00:55:40]

Note that this is still gyp-only.

With gyp_managed_install=1, I got the following after making whitespace
modifications to ChromeActivity:

without CL:

$ time ninja -C out/Debug -j500 -l15 chrome_public_apk
ninja: Entering directory `out/Debug'
[19/19] STAMP obj/chrome/android/chrome_public_apk.actions_rules_copies.stamp

real	0m54.190s
user	2m43.086s
sys	0m10.268s


with CL:

$ time ninja -C out/Debug -j500 -l15 chrome_public_apk
ninja: Entering directory `out/Debug'
[18/18] STAMP obj/chrome/android/chrome_public_apk.actions_rules_copies.stamp

real	0m55.388s
user	3m57.737s
sys	0m11.765s


ptal

[agrieve, 2015-10-30 01:20:11]

What went wrong with the incremental approach?

On 2015/10/30 00:55:40, jbudorick wrote:
> Note that this is still gyp-only.
> 
> With gyp_managed_install=1, I got the following after making whitespace
> modifications to ChromeActivity:
> 
> without CL:
> 
> $ time ninja -C out/Debug -j500 -l15 chrome_public_apk
> ninja: Entering directory `out/Debug'
> [19/19] STAMP obj/chrome/android/chrome_public_apk.actions_rules_copies.stamp
> 
> real	0m54.190s
> user	2m43.086s
> sys	0m10.268s
> 
> 
> with CL:
> 
> $ time ninja -C out/Debug -j500 -l15 chrome_public_apk
> ninja: Entering directory `out/Debug'
> [18/18] STAMP obj/chrome/android/chrome_public_apk.actions_rules_copies.stamp
> 
> real	0m55.388s
> user	3m57.737s
> sys	0m11.765s
> 
> 
> ptal

[agrieve, 2015-10-30 01:20:27]

https://codereview.chromium.org/1408163009/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/BaseChromiumApplication.java
(right):

https://codereview.chromium.org/1408163009/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/BaseChromiumApplication.java:34:
ChromiumMultiDex.install(this);
Was there a thing about this causing a start-up regression even in the no-op
case? Or am I crazy?

[jbudorick, 2015-10-30 01:25:46]

The problem is with main dex class list generation. When you do it on a
per-target basis, each target can specify what classes within that target match
the main dex rules, but it can't pull up classes from other targets that are
used by classes in the target. (Otherwise, you're essentially doing
non-incremental on every target, rather than once at the end.)

I think that we could alternatively use incremental main dex list generation
_if_ every class or method that's used by a dependent target but isn't otherwise
in the main dex list gets annotated with @MainDex. I have a git branch locally
that partially did this.

https://codereview.chromium.org/1408163009/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/BaseChromiumApplication.java
(right):

https://codereview.chromium.org/1408163009/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/BaseChromiumApplication.java:34:
ChromiumMultiDex.install(this);
On 2015/10/30 01:20:27, agrieve wrote:
> Was there a thing about this causing a start-up regression even in the no-op
> case? Or am I crazy?

Oh right, there was, even in the no-op case. This needs to not be called if
we're not building a multidex build.

[agrieve, 2015-10-30 01:50:18]

Aaah, tricky. 

Well, timing certainly looks good, and it should be able to happen at the same
time as per-library dexing is going on (guessing that's why it didn't slow down
the real time much)


On 2015/10/30 01:25:46, jbudorick wrote:
> The problem is with main dex class list generation. When you do it on a
> per-target basis, each target can specify what classes within that target
match
> the main dex rules, but it can't pull up classes from other targets that are
> used by classes in the target. (Otherwise, you're essentially doing
> non-incremental on every target, rather than once at the end.)
> 
> I think that we could alternatively use incremental main dex list generation
> _if_ every class or method that's used by a dependent target but isn't
otherwise
> in the main dex list gets annotated with @MainDex. I have a git branch locally
> that partially did this.
> 
>
https://codereview.chromium.org/1408163009/diff/1/base/android/java/src/org/c...
> File base/android/java/src/org/chromium/base/BaseChromiumApplication.java
> (right):
> 
>
https://codereview.chromium.org/1408163009/diff/1/base/android/java/src/org/c...
> base/android/java/src/org/chromium/base/BaseChromiumApplication.java:34:
> ChromiumMultiDex.install(this);
> On 2015/10/30 01:20:27, agrieve wrote:
> > Was there a thing about this causing a start-up regression even in the no-op
> > case? Or am I crazy?
> 
> Oh right, there was, even in the no-op case. This needs to not be called if
> we're not building a multidex build.

[Yaron, 2015-10-30 17:26:45]

https://codereview.chromium.org/1408163009/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/BaseChromiumApplication.java
(right):

https://codereview.chromium.org/1408163009/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/BaseChromiumApplication.java:34:
ChromiumMultiDex.install(this);
On 2015/10/30 01:25:46, jbudorick wrote:
> On 2015/10/30 01:20:27, agrieve wrote:
> > Was there a thing about this causing a start-up regression even in the no-op
> > case? Or am I crazy?
> 
> Oh right, there was, even in the no-op case. This needs to not be called if
> we're not building a multidex build.

+1

https://codereview.chromium.org/1408163009/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/SystemMessageHandler.java (right):

https://codereview.chromium.org/1408163009/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/SystemMessageHandler.java:18: @MainDex
Please refresh my memory - what happens if you omit this? Runtime ClassNotFound?
Can you also add a brief mention of the need for this to CL description. When
you land this, worth a note to the team too.

[jbudorick, 2015-11-04 23:43:47]

This is now in the running for the ugliest patches I've ever submitted to
chromium. I'm optimistic that the GN equivalent will be cleaner.

https://codereview.chromium.org/1408163009/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/SystemMessageHandler.java (right):

https://codereview.chromium.org/1408163009/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/SystemMessageHandler.java:18: @MainDex
On 2015/10/30 17:26:45, Yaron wrote:
> Please refresh my memory - what happens if you omit this? Runtime
ClassNotFound?
> Can you also add a brief mention of the need for this to CL description. When
> you land this, worth a note to the team too.

Yeah. In this case, you get one of these:

I/cr_LibraryLoader(32751): Loading chrome_public
W/System.err(32751): java.lang.NoClassDefFoundError:
org/chromium/base/SystemMessageHandler
E/chromium(32724): [ERROR:layer_tree_host_impl.cc(2174)] Forcing zero-copy tile
initialization as worker context is missing
W/System.err(32751): 	at
org.chromium.base.library_loader.LegacyLinker.nativeLoadLibrary(Native Method)
W/System.err(32751): 	at
org.chromium.base.library_loader.LegacyLinker.loadLibraryImpl(LegacyLinker.java:504)
W/System.err(32751): 	at
org.chromium.base.library_loader.Linker.loadLibrary(Linker.java:570)
W/System.err(32751): 	at
org.chromium.base.library_loader.LibraryLoader.loadLibrary(LibraryLoader.java:205)
W/System.err(32751): 	at
org.chromium.base.library_loader.LibraryLoader.loadAlreadyLocked(LibraryLoader.java:250)
W/System.err(32751): 	at
org.chromium.base.library_loader.LibraryLoader.loadNow(LibraryLoader.java:148)
W/System.err(32751): 	at
org.chromium.content.app.ChildProcessService$2.run(ChildProcessService.java:178)
W/System.err(32751): 	at java.lang.Thread.run(Thread.java:841)
W/System.err(32751): Caused by: java.lang.ClassNotFoundException: Didn't find
class "org.chromium.base.SystemMessageHandler" on path: DexPathList[[zip file
"/data/app/org.chromium.chrome-1.apk"],nativeLibraryDirectories=[/data/app-lib/org.chromium.chrome-1,
/vendor/lib, /system/lib]]
W/System.err(32751): 	at
dalvik.system.BaseDexClassLoader.findClass(BaseDexClassLoader.java:56)
I/cr_ChromeTabbedActivity(32724): begin
FirstRunFlowSequencer.checkIfFirstRunIsNecessary
W/System.err(32751): 	at java.lang.ClassLoader.loadClass(ClassLoader.java:497)
W/System.err(32751): 	at java.lang.ClassLoader.loadClass(ClassLoader.java:457)
W/System.err(32751): 	... 8 more

[Yaron, 2015-11-10 03:30:21]

On 2015/11/04 23:43:47, jbudorick wrote:
> This is now in the running for the ugliest patches I've ever submitted to
> chromium. I'm optimistic that the GN equivalent will be cleaner.
> 
>
https://codereview.chromium.org/1408163009/diff/1/base/android/java/src/org/c...
> File base/android/java/src/org/chromium/base/SystemMessageHandler.java
(right):
> 
>
https://codereview.chromium.org/1408163009/diff/1/base/android/java/src/org/c...
> base/android/java/src/org/chromium/base/SystemMessageHandler.java:18: @MainDex
> On 2015/10/30 17:26:45, Yaron wrote:
> > Please refresh my memory - what happens if you omit this? Runtime
> ClassNotFound?
> > Can you also add a brief mention of the need for this to CL description.
When
> > you land this, worth a note to the team too.
> 
> Yeah. In this case, you get one of these:
> 
> I/cr_LibraryLoader(32751): Loading chrome_public
> W/System.err(32751): java.lang.NoClassDefFoundError:
> org/chromium/base/SystemMessageHandler
> E/chromium(32724): [ERROR:layer_tree_host_impl.cc(2174)] Forcing zero-copy
tile
> initialization as worker context is missing
> W/System.err(32751): 	at
> org.chromium.base.library_loader.LegacyLinker.nativeLoadLibrary(Native Method)
> W/System.err(32751): 	at
>
org.chromium.base.library_loader.LegacyLinker.loadLibraryImpl(LegacyLinker.java:504)
> W/System.err(32751): 	at
> org.chromium.base.library_loader.Linker.loadLibrary(Linker.java:570)
> W/System.err(32751): 	at
>
org.chromium.base.library_loader.LibraryLoader.loadLibrary(LibraryLoader.java:205)
> W/System.err(32751): 	at
>
org.chromium.base.library_loader.LibraryLoader.loadAlreadyLocked(LibraryLoader.java:250)
> W/System.err(32751): 	at
> org.chromium.base.library_loader.LibraryLoader.loadNow(LibraryLoader.java:148)
> W/System.err(32751): 	at
>
org.chromium.content.app.ChildProcessService$2.run(ChildProcessService.java:178)
> W/System.err(32751): 	at java.lang.Thread.run(Thread.java:841)
> W/System.err(32751): Caused by: java.lang.ClassNotFoundException: Didn't find
> class "org.chromium.base.SystemMessageHandler" on path: DexPathList[[zip file
>
"/data/app/org.chromium.chrome-1.apk"],nativeLibraryDirectories=[/data/app-lib/org.chromium.chrome-1,
> /vendor/lib, /system/lib]]
> W/System.err(32751): 	at
> dalvik.system.BaseDexClassLoader.findClass(BaseDexClassLoader.java:56)
> I/cr_ChromeTabbedActivity(32724): begin
> FirstRunFlowSequencer.checkIfFirstRunIsNecessary
> W/System.err(32751): 	at java.lang.ClassLoader.loadClass(ClassLoader.java:497)
> W/System.err(32751): 	at java.lang.ClassLoader.loadClass(ClassLoader.java:457)
> W/System.err(32751): 	... 8 more

I plan to look at your masterpiece again tomorrow, sorry for delay :/

[jbudorick, 2015-11-10 03:39:17]

On 2015/11/10 03:30:21, Yaron wrote:
> On 2015/11/04 23:43:47, jbudorick wrote:
> > This is now in the running for the ugliest patches I've ever submitted to
> > chromium. I'm optimistic that the GN equivalent will be cleaner.
> > 
> >
>
https://codereview.chromium.org/1408163009/diff/1/base/android/java/src/org/c...
> > File base/android/java/src/org/chromium/base/SystemMessageHandler.java
> (right):
> > 
> >
>
https://codereview.chromium.org/1408163009/diff/1/base/android/java/src/org/c...
> > base/android/java/src/org/chromium/base/SystemMessageHandler.java:18:
@MainDex
> > On 2015/10/30 17:26:45, Yaron wrote:
> > > Please refresh my memory - what happens if you omit this? Runtime
> > ClassNotFound?
> > > Can you also add a brief mention of the need for this to CL description.
> When
> > > you land this, worth a note to the team too.
> > 
> > Yeah. In this case, you get one of these:
> > 
> > I/cr_LibraryLoader(32751): Loading chrome_public
> > W/System.err(32751): java.lang.NoClassDefFoundError:
> > org/chromium/base/SystemMessageHandler
> > E/chromium(32724): [ERROR:layer_tree_host_impl.cc(2174)] Forcing zero-copy
> tile
> > initialization as worker context is missing
> > W/System.err(32751): 	at
> > org.chromium.base.library_loader.LegacyLinker.nativeLoadLibrary(Native
Method)
> > W/System.err(32751): 	at
> >
>
org.chromium.base.library_loader.LegacyLinker.loadLibraryImpl(LegacyLinker.java:504)
> > W/System.err(32751): 	at
> > org.chromium.base.library_loader.Linker.loadLibrary(Linker.java:570)
> > W/System.err(32751): 	at
> >
>
org.chromium.base.library_loader.LibraryLoader.loadLibrary(LibraryLoader.java:205)
> > W/System.err(32751): 	at
> >
>
org.chromium.base.library_loader.LibraryLoader.loadAlreadyLocked(LibraryLoader.java:250)
> > W/System.err(32751): 	at
> >
org.chromium.base.library_loader.LibraryLoader.loadNow(LibraryLoader.java:148)
> > W/System.err(32751): 	at
> >
>
org.chromium.content.app.ChildProcessService$2.run(ChildProcessService.java:178)
> > W/System.err(32751): 	at java.lang.Thread.run(Thread.java:841)
> > W/System.err(32751): Caused by: java.lang.ClassNotFoundException: Didn't
find
> > class "org.chromium.base.SystemMessageHandler" on path: DexPathList[[zip
file
> >
>
"/data/app/org.chromium.chrome-1.apk"],nativeLibraryDirectories=[/data/app-lib/org.chromium.chrome-1,
> > /vendor/lib, /system/lib]]
> > W/System.err(32751): 	at
> > dalvik.system.BaseDexClassLoader.findClass(BaseDexClassLoader.java:56)
> > I/cr_ChromeTabbedActivity(32724): begin
> > FirstRunFlowSequencer.checkIfFirstRunIsNecessary
> > W/System.err(32751): 	at
java.lang.ClassLoader.loadClass(ClassLoader.java:497)
> > W/System.err(32751): 	at
java.lang.ClassLoader.loadClass(ClassLoader.java:457)
> > W/System.err(32751): 	... 8 more
> 
> I plan to look at your masterpiece again tomorrow, sorry for delay :/

No worries, it seems that the current patchset has some issues.

[Yaron, 2015-11-11 02:06:38]

https://codereview.chromium.org/1408163009/diff/20001/base/android/multidex/j...
File
base/android/multidex/java_disabled/src/org/chromium/base/multidex/ChromiumMultiDex.java
(right):

https://codereview.chromium.org/1408163009/diff/20001/base/android/multidex/j...
base/android/multidex/java_disabled/src/org/chromium/base/multidex/ChromiumMultiDex.java:19:
public static void install(Context context) {
wow - that's pretty awesome! What do you think about instead having a
ChromiumMultiDex.java.template and use jinja to conditionally elide the body?

I think the simplified build might be worth it?

https://codereview.chromium.org/1408163009/diff/20001/build/android/gyp/apk_o...
File build/android/gyp/apk_obfuscate.py (right):

https://codereview.chromium.org/1408163009/diff/20001/build/android/gyp/apk_o...
build/android/gyp/apk_obfuscate.py:129: if options.main_dex_list_path:
nit: extract fn

https://codereview.chromium.org/1408163009/diff/20001/build/android/gyp/confi...
File build/android/gyp/configure_multidex.py (right):

https://codereview.chromium.org/1408163009/diff/20001/build/android/gyp/confi...
build/android/gyp/configure_multidex.py:29: help='The path to a preexisting ')
truncated

https://codereview.chromium.org/1408163009/diff/20001/build/android/gyp/dex.py
File build/android/gyp/dex.py (right):

https://codereview.chromium.org/1408163009/diff/20001/build/android/gyp/dex.p...
build/android/gyp/dex.py:76: options.multi_dex = False
Redundant?

[agrieve, 2015-11-11 14:28:20]

https://codereview.chromium.org/1408163009/diff/20001/base/android/multidex/j...
File
base/android/multidex/java_disabled/src/org/chromium/base/multidex/ChromiumMultiDex.java
(right):

https://codereview.chromium.org/1408163009/diff/20001/base/android/multidex/j...
base/android/multidex/java_disabled/src/org/chromium/base/multidex/ChromiumMultiDex.java:19:
public static void install(Context context) {
On 2015/11/11 02:06:38, Yaron wrote:
> wow - that's pretty awesome! What do you think about instead having a
> ChromiumMultiDex.java.template and use jinja to conditionally elide the body?
> 
> I think the simplified build might be worth it?

Keep in mind that doing things this way is super easy with GN:

if (enable_multidex) {
  java_files += [...]
} else {
  java_files += [...]
}

[jbudorick, 2015-11-11 14:29:43]

On 2015/11/11 14:28:20, agrieve wrote:
>
https://codereview.chromium.org/1408163009/diff/20001/base/android/multidex/j...
> File
>
base/android/multidex/java_disabled/src/org/chromium/base/multidex/ChromiumMultiDex.java
> (right):
> 
>
https://codereview.chromium.org/1408163009/diff/20001/base/android/multidex/j...
>
base/android/multidex/java_disabled/src/org/chromium/base/multidex/ChromiumMultiDex.java:19:
> public static void install(Context context) {
> On 2015/11/11 02:06:38, Yaron wrote:
> > wow - that's pretty awesome! What do you think about instead having a
> > ChromiumMultiDex.java.template and use jinja to conditionally elide the
body?
> > 
> > I think the simplified build might be worth it?
> 
> Keep in mind that doing things this way is super easy with GN:
> 
> if (enable_multidex) {
>   java_files += [...]
> } else {
>   java_files += [...]
> }

I think gn also lets me switch on debug/release at generation time, unlike gyp.

[agrieve, 2015-11-11 14:43:01]

https://codereview.chromium.org/1408163009/diff/20001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/1408163009/diff/20001/base/base.gyp#newcode1475
base/base.gyp:1475: 'target_name': 'base_multidex_enabled_java',
Curious why you made these into separate targets instead of using conditions +
additional_src_dirs?

https://codereview.chromium.org/1408163009/diff/20001/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/1408163009/diff/20001/build/java_apk.gypi#new...
build/java_apk.gypi:833: '--multidex-enabled-libs',
'>(multidex_enabled_jars_paths)',
nit: libs usually refers to native libs, can you call this
--multidex-enabled-jar-paths

[jbudorick, 2015-11-12 20:33:40]

Switching to the preprocessor cleaned this up considerably. ptal

https://codereview.chromium.org/1408163009/diff/20001/base/android/multidex/j...
File
base/android/multidex/java_disabled/src/org/chromium/base/multidex/ChromiumMultiDex.java
(right):

https://codereview.chromium.org/1408163009/diff/20001/base/android/multidex/j...
base/android/multidex/java_disabled/src/org/chromium/base/multidex/ChromiumMultiDex.java:19:
public static void install(Context context) {
On 2015/11/11 02:06:38, Yaron wrote:
> wow - that's pretty awesome! What do you think about instead having a
> ChromiumMultiDex.java.template and use jinja to conditionally elide the body?
> 
> I think the simplified build might be worth it?

Definitely worth it.

https://codereview.chromium.org/1408163009/diff/20001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/1408163009/diff/20001/base/base.gyp#newcode1475
base/base.gyp:1475: 'target_name': 'base_multidex_enabled_java',
On 2015/11/11 14:43:01, agrieve wrote:
> Curious why you made these into separate targets instead of using conditions +
> additional_src_dirs?

these two targets are gone after implementing Yaron's suggestion.

https://codereview.chromium.org/1408163009/diff/20001/build/android/gyp/apk_o...
File build/android/gyp/apk_obfuscate.py (right):

https://codereview.chromium.org/1408163009/diff/20001/build/android/gyp/apk_o...
build/android/gyp/apk_obfuscate.py:129: if options.main_dex_list_path:
On 2015/11/11 02:06:38, Yaron wrote:
> nit: extract fn

Done.

https://codereview.chromium.org/1408163009/diff/20001/build/android/gyp/confi...
File build/android/gyp/configure_multidex.py (right):

https://codereview.chromium.org/1408163009/diff/20001/build/android/gyp/confi...
build/android/gyp/configure_multidex.py:29: help='The path to a preexisting ')
On 2015/11/11 02:06:38, Yaron wrote:
> truncated

This parameter wasn't necessary after implementing your cpp suggestion.

https://codereview.chromium.org/1408163009/diff/20001/build/android/gyp/dex.py
File build/android/gyp/dex.py (right):

https://codereview.chromium.org/1408163009/diff/20001/build/android/gyp/dex.p...
build/android/gyp/dex.py:76: options.multi_dex = False
On 2015/11/11 02:06:38, Yaron wrote:
> Redundant?

It was. This logic got shuffled around once libs was no longer necessary.

https://codereview.chromium.org/1408163009/diff/20001/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/1408163009/diff/20001/build/java_apk.gypi#new...
build/java_apk.gypi:833: '--multidex-enabled-libs',
'>(multidex_enabled_jars_paths)',
On 2015/11/11 14:43:01, agrieve wrote:
> nit: libs usually refers to native libs, can you call this
> --multidex-enabled-jar-paths

Entirely gone after Yaron's cpp suggestion.

[Yaron, 2015-11-12 22:15:20]

lgtm (much cleaner!)

https://codereview.chromium.org/1408163009/diff/70032/base/android/java/templ...
File base/android/java/templates/ChromiumMultiDex.template (right):

https://codereview.chromium.org/1408163009/diff/70032/base/android/java/templ...
base/android/java/templates/ChromiumMultiDex.template:33: #if
defined(CONFIGURATION_NAME_Debug)
nit: why not just put this in the method body and avoid repeating the signature?
you want to encapsulate the full fn?

https://codereview.chromium.org/1408163009/diff/70032/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/1408163009/diff/70032/base/base.gyp#newcode1471
base/base.gyp:1471: 'additional_gcc_preprocess_options': [
i thought you could use jinja but this works too.

[jbudorick, 2015-11-12 22:20:49]

https://codereview.chromium.org/1408163009/diff/70032/base/android/java/templ...
File base/android/java/templates/ChromiumMultiDex.template (right):

https://codereview.chromium.org/1408163009/diff/70032/base/android/java/templ...
base/android/java/templates/ChromiumMultiDex.template:33: #if
defined(CONFIGURATION_NAME_Debug)
On 2015/11/12 22:15:20, Yaron wrote:
> nit: why not just put this in the method body and avoid repeating the
signature?
> you want to encapsulate the full fn?

I wanted to also encapsulate processIsIsolated() below.

https://codereview.chromium.org/1408163009/diff/70032/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/1408163009/diff/70032/base/base.gyp#newcode1471
base/base.gyp:1471: 'additional_gcc_preprocess_options': [
On 2015/11/12 22:15:20, Yaron wrote:
> i thought you could use jinja but this works too.

I imagine I could, but I'm more familiar with the c preprocessor & our existing
generated java code uses this instead of jinja.

[Yaron, 2015-11-12 22:27:45]

https://codereview.chromium.org/1408163009/diff/70032/base/android/java/templ...
File base/android/java/templates/ChromiumMultiDex.template (right):

https://codereview.chromium.org/1408163009/diff/70032/base/android/java/templ...
base/android/java/templates/ChromiumMultiDex.template:33: #if
defined(CONFIGURATION_NAME_Debug)
On 2015/11/12 22:20:49, jbudorick wrote:
> On 2015/11/12 22:15:20, Yaron wrote:
> > nit: why not just put this in the method body and avoid repeating the
> signature?
> > you want to encapsulate the full fn?
> 
> I wanted to also encapsulate processIsIsolated() below.

Acknowledged.

[jbudorick, 2015-11-12 22:29:27]

The CQ bit was checked by jbudorick@chromium.org

[jbudorick, 2015-11-12 22:32:04]

The CQ bit was unchecked by jbudorick@chromium.org

[commit-bot: I haz the power, 2015-11-12 22:38:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1408163009/70032
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1408163009/70032

[jbudorick, 2015-11-12 23:20:39]

jbudorick@chromium.org changed reviewers:
+ newt@chromium.org, sky@chromium.org, thakis@chromium.org

[jbudorick, 2015-11-12 23:20:39]

thakis: ptal at base/{base.gyp,BUILD.gn}

sky: TBR for mechanical changes in mojo/
newt: TBR for mechanical changes in ui/android/

[Nico, 2015-11-12 23:24:56]

my 2 files lgtm

[newt (away), 2015-11-12 23:41:54]

ui/android lgtm

[newt (away), 2015-11-12 23:45:32]

What's the logic for determining where to add @MainDex?

[jbudorick, 2015-11-12 23:54:57]

On 2015/11/12 23:45:32, newt wrote:
> What's the logic for determining where to add @MainDex?

when the renderer process dies with a ClassNotFoundException on start. I'll be
sending out a PSA with more details after this lands.

[jbudorick, 2015-11-13 00:01:15]

The CQ bit was checked by jbudorick@chromium.org

[jbudorick, 2015-11-13 00:01:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
yfriedman@chromium.org
Link to the patchset: https://codereview.chromium.org/1408163009/#ps110001
(title: "fix gn build")

[commit-bot: I haz the power, 2015-11-13 00:08:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1408163009/110001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1408163009/110001

[newt (away), 2015-11-13 00:10:47]

On 2015/11/12 23:54:57, jbudorick wrote:
> On 2015/11/12 23:45:32, newt wrote:
> > What's the logic for determining where to add @MainDex?
> 
> when the renderer process dies with a ClassNotFoundException on start. I'll be
> sending out a PSA with more details after this lands.

So, trial and error ;)

As long as it's consistent, I supposed that'll work.

[commit-bot: I haz the power, 2015-11-13 00:45:06]

Committed patchset #7 (id:110001)

[commit-bot: I haz the power, 2015-11-13 00:45:52]

Patchset 7 (id:??) landed as
https://crrev.com/f667d3b3a1b953800bb5325c7b8d62775a1c5aeb
Cr-Commit-Position: refs/heads/master@{#359453}