[Android] Add gyp support for multidex.

[Android] Add gyp support for multidex.

Note that this does not enable multidex builds yet.

BUG=272790
Committed: https://crrev.com/e40294eca50d57360a2bb0718adcd5cde7810fbd
Cr-Commit-Position: refs/heads/master@{#345357}

[jbudorick, 2015-08-05 20:08:48]

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

[jbudorick, 2015-08-05 20:08:48]

Working on the GN side of this. I'll probably wind up doing that as a separate
patch.

[Yaron, 2015-08-05 21:56:41]

https://codereview.chromium.org/1278573002/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/annotations/MainDex.java (right):

https://codereview.chromium.org/1278573002/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/annotations/MainDex.java:13: * An
annotation that signals that a class should be kept in the main dex file.
So all classes that need to be kept need to have this annotation?

https://codereview.chromium.org/1278573002/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/multidex/ChromiumMultiDex.java
(right):

https://codereview.chromium.org/1278573002/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/multidex/ChromiumMultiDex.java:42:
Log.e(TAG, "Failed multidex installation", e);
Won't be explode in this case? We're expecting multidex - if we can't load all
functions Bad Things will happen. Log.wtf or just fail harder?

https://codereview.chromium.org/1278573002/diff/1/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/1278573002/diff/1/build/java_apk.gypi#newcode...
build/java_apk.gypi:1013: 'main_dex_rules_file':
'<(DEPTH)/build/android/main_dex_classes.rules',
Is this supposed to be added? It's not generated, is it?

[jbudorick, 2015-08-05 22:55:23]

https://codereview.chromium.org/1278573002/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/annotations/MainDex.java (right):

https://codereview.chromium.org/1278573002/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/annotations/MainDex.java:13: * An
annotation that signals that a class should be kept in the main dex file.
On 2015/08/05 at 21:56:41, Yaron wrote:
> So all classes that need to be kept need to have this annotation?

That or they need to be specified in some other way in main_dex_classes.flags
(or the base mainDexClasses.rules in the sdk).

https://codereview.chromium.org/1278573002/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/multidex/ChromiumMultiDex.java
(right):

https://codereview.chromium.org/1278573002/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/multidex/ChromiumMultiDex.java:42:
Log.e(TAG, "Failed multidex installation", e);
On 2015/08/05 at 21:56:41, Yaron wrote:
> Won't be explode in this case? We're expecting multidex - if we can't load all
functions Bad Things will happen. Log.wtf or just fail harder?

It depends. The only way we hit these exception handlers is if the private API
call below fails in some way -- MultiDex.install doesn't throw these exceptions.
If we're in a renderer, we should be ok, as all of the renderer classes should
be in the main dex. If not, then we're dead.

https://codereview.chromium.org/1278573002/diff/1/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/1278573002/diff/1/build/java_apk.gypi#newcode...
build/java_apk.gypi:1013: 'main_dex_rules_file':
'<(DEPTH)/build/android/main_dex_classes.rules',
On 2015/08/05 at 21:56:41, Yaron wrote:
> Is this supposed to be added? It's not generated, is it?

Missed it because *.rules is in the .gitignore. Renamed to
main_dex_classes.flags and added.

[jbudorick, 2015-08-10 18:40:38]

ping?

[Yaron, 2015-08-10 20:15:40]

https://codereview.chromium.org/1278573002/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/multidex/ChromiumMultiDex.java
(right):

https://codereview.chromium.org/1278573002/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/multidex/ChromiumMultiDex.java:42:
Log.e(TAG, "Failed multidex installation", e);
On 2015/08/05 22:55:22, jbudorick wrote:
> On 2015/08/05 at 21:56:41, Yaron wrote:
> > Won't be explode in this case? We're expecting multidex - if we can't load
all
> functions Bad Things will happen. Log.wtf or just fail harder?
> 
> It depends. The only way we hit these exception handlers is if the private API
> call below fails in some way -- MultiDex.install doesn't throw these
exceptions.
> If we're in a renderer, we should be ok, as all of the renderer classes should
> be in the main dex. If not, then we're dead.

I think if the private API call fails, we should fail hard. It means our
expectations aren't satisfied and the next call will fail somehow but that could
be harder to see.

https://codereview.chromium.org/1278573002/diff/60001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/multidex/ChromiumMultiDex.java
(right):

https://codereview.chromium.org/1278573002/diff/60001/base/android/java/src/o...
base/android/java/src/org/chromium/base/multidex/ChromiumMultiDex.java:33: //
TODO(jbudorick): Back out this version check once support for K & below works.
are we planning to ship before this gets sorted out?

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

https://codereview.chromium.org/1278573002/diff/60001/build/android/gyp/dex.p...
build/android/gyp/dex.py:30: proguard_cmd = [
I hate running proguard in debug mode. :(
What's the performance like for this call?

[jbudorick, 2015-08-10 22:00:49]

https://codereview.chromium.org/1278573002/diff/60001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/multidex/ChromiumMultiDex.java
(right):

https://codereview.chromium.org/1278573002/diff/60001/base/android/java/src/o...
base/android/java/src/org/chromium/base/multidex/ChromiumMultiDex.java:33: //
TODO(jbudorick): Back out this version check once support for K & below works.
On 2015/08/10 at 20:15:40, Yaron wrote:
> are we planning to ship before this gets sorted out?

No, I'm working on this part in a dependent git branch locally. I won't enable
multidex for any target before that lands.

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

https://codereview.chromium.org/1278573002/diff/60001/build/android/gyp/dex.p...
build/android/gyp/dex.py:30: proguard_cmd = [
On 2015/08/10 at 20:15:40, Yaron wrote:
> I hate running proguard in debug mode. :(
> What's the performance like for this call?

I tried nuking *.dex* in the chrome_public_apk intermediate directory both with
& witout multidex enabled.

multidex not enabled:

jbudorick@jbudorick:/usr/local/google/code/clankium-split/2/src$ time ninja -C
out/Debug -j500 -l15 chrome_public_apk
ninja: Entering directory `out/Debug'
[2/4] ACTION Packaging ChromePublic
[echo] multidex disabled

[4/4] STAMP obj/chrome/android/chrome_public_apk.actions_rules_copies.stamp

real	0m29.538s
user	0m34.090s
sys	0m1.706s


multidex enabled:

jbudorick@jbudorick:/usr/local/google/code/clankium-split/2/src$ time ninja -C
out/Debug -j500 -l15 chrome_public_apk
ninja: Entering directory `out/Debug'
[1/4] ACTION Creating dex file: chrome_public_apk/classes.dex.zip
      You should check if you need to specify additional program jars.
... //// Warnings omitted
[2/4] ACTION Packaging ChromePublic
[echo] multidex enabled

[4/4] STAMP obj/chrome/android/chrome_public_apk.actions_rules_copies.stamp

real	0m42.297s
user	1m3.343s
sys	0m3.408s


I suppose we could instead list the classes explicitly, but that seems much,
much more brittle to me.

[jbudorick, 2015-08-11 00:26:58]

https://codereview.chromium.org/1278573002/diff/60001/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/1278573002/diff/60001/build/java_apk.gypi#new...
build/java_apk.gypi:1045: '--excluded-paths',
'@FileArg(>(tested_apk_dex_path).inputs)'
This doesn't quite work as-is, so hold off on reviewing further until I upload
another patchset.

[Yaron, 2015-08-11 13:38:36]

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

https://codereview.chromium.org/1278573002/diff/60001/build/android/gyp/dex.p...
build/android/gyp/dex.py:30: proguard_cmd = [
On 2015/08/10 22:00:49, jbudorick wrote:
> On 2015/08/10 at 20:15:40, Yaron wrote:
> > I hate running proguard in debug mode. :(
> > What's the performance like for this call?
> 
> I tried nuking *.dex* in the chrome_public_apk intermediate directory both
with
> & witout multidex enabled.
> 
> multidex not enabled:
> 
> jbudorick@jbudorick:/usr/local/google/code/clankium-split/2/src$ time ninja -C
> out/Debug -j500 -l15 chrome_public_apk
> ninja: Entering directory `out/Debug'
> [2/4] ACTION Packaging ChromePublic
> [echo] multidex disabled
> 
> [4/4] STAMP obj/chrome/android/chrome_public_apk.actions_rules_copies.stamp
> 
> real	0m29.538s
> user	0m34.090s
> sys	0m1.706s
> 
> 
> multidex enabled:
> 
> jbudorick@jbudorick:/usr/local/google/code/clankium-split/2/src$ time ninja -C
> out/Debug -j500 -l15 chrome_public_apk
> ninja: Entering directory `out/Debug'
> [1/4] ACTION Creating dex file: chrome_public_apk/classes.dex.zip
>       You should check if you need to specify additional program jars.
> ... //// Warnings omitted
> [2/4] ACTION Packaging ChromePublic
> [echo] multidex enabled
> 
> [4/4] STAMP obj/chrome/android/chrome_public_apk.actions_rules_copies.stamp
> 
> real	0m42.297s
> user	1m3.343s
> sys	0m3.408s
> 
> 
> I suppose we could instead list the classes explicitly, but that seems much,
> much more brittle to me.

So.. many changes which touch java files are loads slower. :(
Can't say I'm thrilled unless we could do a managed-install type thing for java
files. agrieve was interested in building that on top of multidex

[jbudorick, 2015-08-11 13:42:22]

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

https://codereview.chromium.org/1278573002/diff/60001/build/android/gyp/dex.p...
build/android/gyp/dex.py:30: proguard_cmd = [
On 2015/08/11 at 13:38:36, Yaron wrote:
> On 2015/08/10 22:00:49, jbudorick wrote:
> > On 2015/08/10 at 20:15:40, Yaron wrote:
> > > I hate running proguard in debug mode. :(
> > > What's the performance like for this call?
> > 
> > I tried nuking *.dex* in the chrome_public_apk intermediate directory both
with
> > & witout multidex enabled.
> > 
> > multidex not enabled:
> > 
> > jbudorick@jbudorick:/usr/local/google/code/clankium-split/2/src$ time ninja
-C
> > out/Debug -j500 -l15 chrome_public_apk
> > ninja: Entering directory `out/Debug'
> > [2/4] ACTION Packaging ChromePublic
> > [echo] multidex disabled
> > 
> > [4/4] STAMP obj/chrome/android/chrome_public_apk.actions_rules_copies.stamp
> > 
> > real	0m29.538s
> > user	0m34.090s
> > sys	0m1.706s
> > 
> > 
> > multidex enabled:
> > 
> > jbudorick@jbudorick:/usr/local/google/code/clankium-split/2/src$ time ninja
-C
> > out/Debug -j500 -l15 chrome_public_apk
> > ninja: Entering directory `out/Debug'
> > [1/4] ACTION Creating dex file: chrome_public_apk/classes.dex.zip
> >       You should check if you need to specify additional program jars.
> > ... //// Warnings omitted
> > [2/4] ACTION Packaging ChromePublic
> > [echo] multidex enabled
> > 
> > [4/4] STAMP obj/chrome/android/chrome_public_apk.actions_rules_copies.stamp
> > 
> > real	0m42.297s
> > user	1m3.343s
> > sys	0m3.408s
> > 
> > 
> > I suppose we could instead list the classes explicitly, but that seems much,
> > much more brittle to me.
> 
> So.. many changes which touch java files are loads slower. :(
> Can't say I'm thrilled unless we could do a managed-install type thing for
java files. agrieve was interested in building that on top of multidex

A what? I'm not sure I follow...

I'm also not thrilled about raising the build time.

[Yaron, 2015-08-11 13:49:39]

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

[Yaron, 2015-08-11 13:49:40]

+agrieve

The idea was that if we already have multidex set up, we can make the java build
smarter and mirror managed install for native code. Rather than having to
repackage everything into chrome.apk, you could just update the dex file that
changed and reload chrome. Then we could turn multidex into a win for
productivity.

[agrieve, 2015-08-11 14:01:32]

On 2015/08/11 13:49:40, Yaron wrote:
> +agrieve
> 
> The idea was that if we already have multidex set up, we can make the java
build
> smarter and mirror managed install for native code. Rather than having to
> repackage everything into chrome.apk, you could just update the dex file that
> changed and reload chrome. Then we could turn multidex into a win for
> productivity.

How about we hard-code the classes needed for start-up and then add a test that
asserts the list matches what proguard says?

[cjhopman, 2015-08-11 21:59:34]

On 2015/08/11 14:01:32, agrieve wrote:
> On 2015/08/11 13:49:40, Yaron wrote:
> > +agrieve
> > 
> > The idea was that if we already have multidex set up, we can make the java
> build
> > smarter and mirror managed install for native code. Rather than having to
> > repackage everything into chrome.apk, you could just update the dex file
that
> > changed and reload chrome. Then we could turn multidex into a win for
> > productivity.
> 
> How about we hard-code the classes needed for start-up and then add a test
that
> asserts the list matches what proguard says?

I had prototyped managed install for resources+java(+native in a more clean way)
here: https://codereview.chromium.org/949803002/


We may be able to skip running proguard in almost all cases... I think we only
need to run it if one of the following happens:

1. Class adds/removes @MainDex
2. Class adds/removes base class Application/BroadcastReceiver/... (see
sdk/build-tools/22.0.0/mainDexClasses.rules)
3. One of the files included in the previous main dex changes

It's possible that we could detect that very quickly (1 and 2 could be done at
the library level instead of apk level) and skip the resulting slow steps.

[jbudorick, 2015-08-11 22:46:06]

On 2015/08/11 at 21:59:34, cjhopman wrote:
> On 2015/08/11 14:01:32, agrieve wrote:
> > On 2015/08/11 13:49:40, Yaron wrote:
> > > +agrieve
> > > 
> > > The idea was that if we already have multidex set up, we can make the java
> > build
> > > smarter and mirror managed install for native code. Rather than having to
> > > repackage everything into chrome.apk, you could just update the dex file
that
> > > changed and reload chrome. Then we could turn multidex into a win for
> > > productivity.
> > 
> > How about we hard-code the classes needed for start-up and then add a test
that
> > asserts the list matches what proguard says?
> 
> I had prototyped managed install for resources+java(+native in a more clean
way) here: https://codereview.chromium.org/949803002/
> 
> 
> We may be able to skip running proguard in almost all cases... I think we only
need to run it if one of the following happens:
> 
> 1. Class adds/removes @MainDex
> 2. Class adds/removes base class Application/BroadcastReceiver/... (see
sdk/build-tools/22.0.0/mainDexClasses.rules)
> 3. One of the files included in the previous main dex changes
> 
> It's possible that we could detect that very quickly (1 and 2 could be done at
the library level instead of apk level) and skip the resulting slow steps.

To do that, generating the main dex would have to be its own build target that
depends on 1-3 rather than on the APK's input jars, right?

[Yaron, 2015-08-12 19:21:45]

On 2015/08/11 22:46:06, jbudorick wrote:
> On 2015/08/11 at 21:59:34, cjhopman wrote:
> > On 2015/08/11 14:01:32, agrieve wrote:
> > > On 2015/08/11 13:49:40, Yaron wrote:
> > > > +agrieve
> > > > 
> > > > The idea was that if we already have multidex set up, we can make the
java
> > > build
> > > > smarter and mirror managed install for native code. Rather than having
to
> > > > repackage everything into chrome.apk, you could just update the dex file
> that
> > > > changed and reload chrome. Then we could turn multidex into a win for
> > > > productivity.
> > > 
> > > How about we hard-code the classes needed for start-up and then add a test
> that
> > > asserts the list matches what proguard says?
> > 
> > I had prototyped managed install for resources+java(+native in a more clean
> way) here: https://codereview.chromium.org/949803002/
> > 
> > 
> > We may be able to skip running proguard in almost all cases... I think we
only
> need to run it if one of the following happens:
> > 
> > 1. Class adds/removes @MainDex
> > 2. Class adds/removes base class Application/BroadcastReceiver/... (see
> sdk/build-tools/22.0.0/mainDexClasses.rules)
> > 3. One of the files included in the previous main dex changes
> > 
> > It's possible that we could detect that very quickly (1 and 2 could be done
at
> the library level instead of apk level) and skip the resulting slow steps.
> 
> To do that, generating the main dex would have to be its own build target that
> depends on 1-3 rather than on the APK's input jars, right?

Any idea what about proguard makes it slow? Could using a java annotation
processor help?
We could also absorb the hit for release builds only if that unblocks this. It
means we could recoup the memory savings for renderers but I'd still like to
minimize overhead before we tried enabling for debug.

[agrieve, 2015-08-13 00:28:51]

On 2015/08/12 19:21:45, Yaron wrote:
> On 2015/08/11 22:46:06, jbudorick wrote:
> > On 2015/08/11 at 21:59:34, cjhopman wrote:
> > > On 2015/08/11 14:01:32, agrieve wrote:
> > > > On 2015/08/11 13:49:40, Yaron wrote:
> > > > > +agrieve
> > > > > 
> > > > > The idea was that if we already have multidex set up, we can make the
> java
> > > > build
> > > > > smarter and mirror managed install for native code. Rather than having
> to
> > > > > repackage everything into chrome.apk, you could just update the dex
file
> > that
> > > > > changed and reload chrome. Then we could turn multidex into a win for
> > > > > productivity.
> > > > 
> > > > How about we hard-code the classes needed for start-up and then add a
test
> > that
> > > > asserts the list matches what proguard says?
> > > 
> > > I had prototyped managed install for resources+java(+native in a more
clean
> > way) here: https://codereview.chromium.org/949803002/
> > > 
> > > 
> > > We may be able to skip running proguard in almost all cases... I think we
> only
> > need to run it if one of the following happens:
> > > 
> > > 1. Class adds/removes @MainDex
> > > 2. Class adds/removes base class Application/BroadcastReceiver/... (see
> > sdk/build-tools/22.0.0/mainDexClasses.rules)
> > > 3. One of the files included in the previous main dex changes
> > > 
> > > It's possible that we could detect that very quickly (1 and 2 could be
done
> at
> > the library level instead of apk level) and skip the resulting slow steps.
> > 
> > To do that, generating the main dex would have to be its own build target
that
> > depends on 1-3 rather than on the APK's input jars, right?
> 
> Any idea what about proguard makes it slow? Could using a java annotation
> processor help?
> We could also absorb the hit for release builds only if that unblocks this. It
> means we could recoup the memory savings for renderers but I'd still like to
> minimize overhead before we tried enabling for debug.

I think the simplest path to getting this going would be to manually run
main_dex_list.py, then check in its output. If someone adds a class that will
need to be in the main jar, their app will likely crash on install / start-up
and they should know to run the script again to update the file. If this gets
too annoying, then we could look at pursuing an more automated system.

[jbudorick, 2015-08-13 00:34:46]

On 2015/08/13 at 00:28:51, agrieve wrote:
> On 2015/08/12 19:21:45, Yaron wrote:
> > On 2015/08/11 22:46:06, jbudorick wrote:
> > > On 2015/08/11 at 21:59:34, cjhopman wrote:
> > > > On 2015/08/11 14:01:32, agrieve wrote:
> > > > > On 2015/08/11 13:49:40, Yaron wrote:
> > > > > > +agrieve
> > > > > > 
> > > > > > The idea was that if we already have multidex set up, we can make
the
> > java
> > > > > build
> > > > > > smarter and mirror managed install for native code. Rather than
having
> > to
> > > > > > repackage everything into chrome.apk, you could just update the dex
file
> > > that
> > > > > > changed and reload chrome. Then we could turn multidex into a win
for
> > > > > > productivity.
> > > > > 
> > > > > How about we hard-code the classes needed for start-up and then add a
test
> > > that
> > > > > asserts the list matches what proguard says?
> > > > 
> > > > I had prototyped managed install for resources+java(+native in a more
clean
> > > way) here: https://codereview.chromium.org/949803002/
> > > > 
> > > > 
> > > > We may be able to skip running proguard in almost all cases... I think
we
> > only
> > > need to run it if one of the following happens:
> > > > 
> > > > 1. Class adds/removes @MainDex
> > > > 2. Class adds/removes base class Application/BroadcastReceiver/... (see
> > > sdk/build-tools/22.0.0/mainDexClasses.rules)
> > > > 3. One of the files included in the previous main dex changes
> > > > 
> > > > It's possible that we could detect that very quickly (1 and 2 could be
done
> > at
> > > the library level instead of apk level) and skip the resulting slow steps.
> > > 
> > > To do that, generating the main dex would have to be its own build target
that
> > > depends on 1-3 rather than on the APK's input jars, right?
> > 
> > Any idea what about proguard makes it slow? Could using a java annotation
> > processor help?
> > We could also absorb the hit for release builds only if that unblocks this.
It
> > means we could recoup the memory savings for renderers but I'd still like to
> > minimize overhead before we tried enabling for debug.
> 
> I think the simplest path to getting this going would be to manually run
main_dex_list.py, then check in its output. If someone adds a class that will
need to be in the main jar, their app will likely crash on install / start-up
and they should know to run the script again to update the file. If this gets
too annoying, then we could look at pursuing an more automated system.

patchset 5 is somewhat like that -- it instead builds the main dex list once,
then only rebuilds if the rules change or if the main_dex_list.py script
changes.

I'd like to do something along the lines of what Chris suggested. I'm still
investigating how to do that, including using the proguard flags, without using
proguard, or if I could generate the main dex list on a library level then
combine them at APK dex time.

[jbudorick, 2015-08-13 14:26:12]

Switched to generating a main dex list per library target, then merging them all
at APK dex time.

Incremental build time without this CL:

jbudorick@jbudorick:/usr/local/google/code/clankium-split/2/src$ touch
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java
jbudorick@jbudorick:/usr/local/google/code/clankium-split/2/src$ time ninja -C
out/Debug -j500 -l15 chrome_apk
ninja: Entering directory `out/Debug'
[23/23] STAMP obj/clank/native/chrome_apk.actions_rules_copies.stamp

real	1m7.657s
user	2m47.040s
sys	0m9.286s


Incremental build time with this CL:

jbudorick@jbudorick:/usr/local/google/code/clankium-split/2/src$ touch
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java
jbudorick@jbudorick:/usr/local/google/code/clankium-split/2/src$ time ninja -C
out/Debug -j500 -l15 chrome_apk
ninja: Entering directory `out/Debug'
[16/26] ACTION clank_java:
main_dex_list_for_clank_java_bb4aa2230c2287407635aba0b94550f6
      You should check if you need to specify additional program jars.
[24/26] ACTION Packaging Chrome
[echo] multidex enabled

[26/26] STAMP obj/clank/native/chrome_apk.actions_rules_copies.stamp

real	1m6.802s
user	3m2.852s
sys	0m10.120s

[agrieve, 2015-08-13 15:11:05]

On 2015/08/13 14:26:12, jbudorick wrote:
> Switched to generating a main dex list per library target, then merging them
all
> at APK dex time.
> 
> Incremental build time without this CL:
> 
> jbudorick@jbudorick:/usr/local/google/code/clankium-split/2/src$ touch
> chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java
> jbudorick@jbudorick:/usr/local/google/code/clankium-split/2/src$ time ninja -C
> out/Debug -j500 -l15 chrome_apk
> ninja: Entering directory `out/Debug'
> [23/23] STAMP obj/clank/native/chrome_apk.actions_rules_copies.stamp
> 
> real	1m7.657s
> user	2m47.040s
> sys	0m9.286s
> 
> 
> Incremental build time with this CL:
> 
> jbudorick@jbudorick:/usr/local/google/code/clankium-split/2/src$ touch
> chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java
> jbudorick@jbudorick:/usr/local/google/code/clankium-split/2/src$ time ninja -C
> out/Debug -j500 -l15 chrome_apk
> ninja: Entering directory `out/Debug'
> [16/26] ACTION clank_java:
> main_dex_list_for_clank_java_bb4aa2230c2287407635aba0b94550f6
>       You should check if you need to specify additional program jars.
> [24/26] ACTION Packaging Chrome
> [echo] multidex enabled
> 
> [26/26] STAMP obj/clank/native/chrome_apk.actions_rules_copies.stamp
> 
> real	1m6.802s
> user	3m2.852s
> sys	0m10.120s

Looks great!

[Yaron, 2015-08-13 20:12:11]

just a few minor things but lgtm with those fixed. would be great if Chris can
take a look too :)

https://codereview.chromium.org/1278573002/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/multidex/ChromiumMultiDex.java
(right):

https://codereview.chromium.org/1278573002/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/multidex/ChromiumMultiDex.java:42:
Log.e(TAG, "Failed multidex installation", e);
On 2015/08/10 20:15:40, Yaron wrote:
> On 2015/08/05 22:55:22, jbudorick wrote:
> > On 2015/08/05 at 21:56:41, Yaron wrote:
> > > Won't be explode in this case? We're expecting multidex - if we can't load
> all
> > functions Bad Things will happen. Log.wtf or just fail harder?
> > 
> > It depends. The only way we hit these exception handlers is if the private
API
> > call below fails in some way -- MultiDex.install doesn't throw these
> exceptions.
> > If we're in a renderer, we should be ok, as all of the renderer classes
should
> > be in the main dex. If not, then we're dead.
> 
> I think if the private API call fails, we should fail hard. It means our
> expectations aren't satisfied and the next call will fail somehow but that
could
> be harder to see.

bump

https://codereview.chromium.org/1278573002/diff/120001/build/android/ant/apk-...
File build/android/ant/apk-package.xml (right):

https://codereview.chromium.org/1278573002/diff/120001/build/android/ant/apk-...
build/android/ant/apk-package.xml:116: <echo message="multidex enabled" />
does this show actually show up? If so, please remove before landing

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

https://codereview.chromium.org/1278573002/diff/120001/build/android/gyp/dex....
build/android/gyp/dex.py:19: if options.multi_dex and
options.main_dex_list_paths:
Nit: I think it's cleaner to move this all to the caller. Then you'll have DoDex
and DoMutliDex which also uses DoDex

https://codereview.chromium.org/1278573002/diff/120001/build/android/gyp/dex....
build/android/gyp/dex.py:34: for l in main_dex_list_file:
use a list comprehension?

https://codereview.chromium.org/1278573002/diff/120001/build/android/gyp/main...
File build/android/gyp/main_dex_list.py (right):

https://codereview.chromium.org/1278573002/diff/120001/build/android/gyp/main...
build/android/gyp/main_dex_list.py:35: os.path.join(args.android_sdk_tools,
os.pardir, os.pardir, 'tools',
we're about to introduce a new proguard path:
https://codereview.chromium.org/1288393002/

Can you move it to constants.py?

[Yaron, 2015-08-13 20:18:28]

https://codereview.chromium.org/1278573002/diff/120001/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/1278573002/diff/120001/build/java_apk.gypi#ne...
build/java_apk.gypi:1048: '>@(main_dex_list_paths)',
Actually, does this include the file created from java files in this apk target?

[jbudorick, 2015-08-14 21:01:21]

https://codereview.chromium.org/1278573002/diff/1/base/android/java/src/org/c...
File base/android/java/src/org/chromium/base/multidex/ChromiumMultiDex.java
(right):

https://codereview.chromium.org/1278573002/diff/1/base/android/java/src/org/c...
base/android/java/src/org/chromium/base/multidex/ChromiumMultiDex.java:42:
Log.e(TAG, "Failed multidex installation", e);
On 2015/08/13 at 20:12:10, Yaron wrote:
> On 2015/08/10 20:15:40, Yaron wrote:
> > On 2015/08/05 22:55:22, jbudorick wrote:
> > > On 2015/08/05 at 21:56:41, Yaron wrote:
> > > > Won't be explode in this case? We're expecting multidex - if we can't
load
> > all
> > > functions Bad Things will happen. Log.wtf or just fail harder?
> > > 
> > > It depends. The only way we hit these exception handlers is if the private
API
> > > call below fails in some way -- MultiDex.install doesn't throw these
> > exceptions.
> > > If we're in a renderer, we should be ok, as all of the renderer classes
should
> > > be in the main dex. If not, then we're dead.
> > 
> > I think if the private API call fails, we should fail hard. It means our
> > expectations aren't satisfied and the next call will fail somehow but that
could
> > be harder to see.
> 
> bump

oops, lost this one.

Log.e -> Log.wtf

https://codereview.chromium.org/1278573002/diff/120001/build/android/ant/apk-...
File build/android/ant/apk-package.xml (right):

https://codereview.chromium.org/1278573002/diff/120001/build/android/ant/apk-...
build/android/ant/apk-package.xml:116: <echo message="multidex enabled" />
On 2015/08/13 at 20:12:10, Yaron wrote:
> does this show actually show up? If so, please remove before landing

It does, e.g.:

[24/26] ACTION Packaging Chrome
[echo] multidex enabled

Removed.

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

https://codereview.chromium.org/1278573002/diff/120001/build/android/gyp/dex....
build/android/gyp/dex.py:19: if options.multi_dex and
options.main_dex_list_paths:
On 2015/08/13 at 20:12:10, Yaron wrote:
> Nit: I think it's cleaner to move this all to the caller. Then you'll have
DoDex and DoMutliDex which also uses DoDex

done

https://codereview.chromium.org/1278573002/diff/120001/build/android/gyp/dex....
build/android/gyp/dex.py:34: for l in main_dex_list_file:
On 2015/08/13 at 20:12:10, Yaron wrote:
> use a list comprehension?

derp. done.

https://codereview.chromium.org/1278573002/diff/120001/build/android/gyp/main...
File build/android/gyp/main_dex_list.py (right):

https://codereview.chromium.org/1278573002/diff/120001/build/android/gyp/main...
build/android/gyp/main_dex_list.py:35: os.path.join(args.android_sdk_tools,
os.pardir, os.pardir, 'tools',
On 2015/08/13 at 20:12:10, Yaron wrote:
> we're about to introduce a new proguard path:
https://codereview.chromium.org/1288393002/
> 
> Can you move it to constants.py?

done

https://codereview.chromium.org/1278573002/diff/120001/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/1278573002/diff/120001/build/java_apk.gypi#ne...
build/java_apk.gypi:1048: '>@(main_dex_list_paths)',
On 2015/08/13 at 20:18:27, Yaron wrote:
> Actually, does this include the file created from java files in this apk
target?

Good catch. It does now.

[jbudorick, 2015-08-14 21:08:04]

https://codereview.chromium.org/1278573002/diff/140001/build/java_apk.gypi
File build/java_apk.gypi (right):

https://codereview.chromium.org/1278573002/diff/140001/build/java_apk.gypi#ne...
build/java_apk.gypi:886: # TODO(jbudorick): main_dex_action.gypi?
er... new patchset coming with this TODO removed and possibly implemented.

[Yaron, 2015-08-15 02:18:31]

lgtm

[jbudorick, 2015-08-16 14:44:55]

jbudorick@chromium.org changed reviewers:
+ thakis@chromium.org

[jbudorick, 2015-08-16 14:44:55]

+thakis for base/base.gyp OWNERS

[Nico, 2015-08-17 17:02:52]

base.gyp lgtm (feel free to tbr changes like this)

[jbudorick, 2015-08-17 17:05:17]

The CQ bit was checked by jbudorick@chromium.org

[jbudorick, 2015-08-17 17:05:18]

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/1278573002/#ps160001
(title: "main_dex_action.gypi")

[commit-bot: I haz the power, 2015-08-17 17:05:29]

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

[jbudorick, 2015-08-17 17:07:33]

On 2015/08/17 at 17:02:52, thakis wrote:
> base.gyp lgtm (feel free to tbr changes like this)

sgtm, will do in the future

[commit-bot: I haz the power, 2015-08-17 17:12:35]

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

[commit-bot: I haz the power, 2015-08-17 17:12:36]

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

[jbudorick, 2015-08-17 20:36:31]

The CQ bit was checked by jbudorick@chromium.org

[jbudorick, 2015-08-17 20:36:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
yfriedman@chromium.org, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1278573002/#ps180001
(title: "Remove changes to --android-sdk-tools & fix GN.")

[commit-bot: I haz the power, 2015-08-17 20:37:19]

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

[commit-bot: I haz the power, 2015-08-17 21:14:10]

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

[commit-bot: I haz the power, 2015-08-17 21:14:11]

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

[jbudorick, 2015-08-18 21:28:20]

jbudorick@chromium.org changed reviewers:
+ mmenke@chromium.org

[jbudorick, 2015-08-18 21:28:21]

+mmenke for net/ OWNERS

yfriedman: ptal again at the revised changes to base/test/android. I had to add
a shadow for MultiDex for the junit tests that use BaseChromiumApplication as
their application.

[mmenke, 2015-08-18 21:36:09]

On 2015/08/18 21:28:21, jbudorick wrote:
> +mmenke for net/ OWNERS
> 
> yfriedman: ptal again at the revised changes to base/test/android. I had to
add
> a shadow for MultiDex for the junit tests that use BaseChromiumApplication as
> their application.

LGTM, rubberstamp.

[Yaron, 2015-08-24 15:28:44]

https://codereview.chromium.org/1278573002/diff/200001/build/config/android/r...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/1278573002/diff/200001/build/config/android/r...
build/config/android/rules.gni:1088: #   supports_android: If true, Android
targets (android_library, android_apk)
Am I missing something? I don't see where this is enforced. Also, wouldn't this
be the default? We have few targets which don't work on android.

https://codereview.chromium.org/1278573002/diff/200001/chrome/android/BUILD.gn
File chrome/android/BUILD.gn (right):

https://codereview.chromium.org/1278573002/diff/200001/chrome/android/BUILD.g...
chrome/android/BUILD.gn:518: "//base:base_junit_test_support",
what about content? It also depends on BaseChromiumApplication

[jbudorick, 2015-08-24 21:13:51]

https://codereview.chromium.org/1278573002/diff/200001/build/config/android/r...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/1278573002/diff/200001/build/config/android/r...
build/config/android/rules.gni:1088: #   supports_android: If true, Android
targets (android_library, android_apk)
On 2015/08/24 at 15:28:44, Yaron wrote:
> Am I missing something? I don't see where this is enforced. Also, wouldn't
this be the default? We have few targets which don't work on android.

requires_android and supports_android are enforced here:
https://code.google.com/p/chromium/codesearch#chromium/src/build/android/gyp/...

Neither is enabled by default for java_X, but they are for android_X.

https://codereview.chromium.org/1278573002/diff/200001/chrome/android/BUILD.gn
File chrome/android/BUILD.gn (right):

https://codereview.chromium.org/1278573002/diff/200001/chrome/android/BUILD.g...
chrome/android/BUILD.gn:518: "//base:base_junit_test_support",
On 2015/08/24 at 15:28:44, Yaron wrote:
> what about content? It also depends on BaseChromiumApplication

The test case in content_junit_tests doesn't currently depend on
BaseChromiumApplication. I can add it if you'd prefer, though.

[Yaron, 2015-08-25 15:49:20]

On 2015/08/24 21:13:51, jbudorick wrote:
>
https://codereview.chromium.org/1278573002/diff/200001/build/config/android/r...
> File build/config/android/rules.gni (right):
> 
>
https://codereview.chromium.org/1278573002/diff/200001/build/config/android/r...
> build/config/android/rules.gni:1088: #   supports_android: If true, Android
> targets (android_library, android_apk)
> On 2015/08/24 at 15:28:44, Yaron wrote:
> > Am I missing something? I don't see where this is enforced. Also, wouldn't
> this be the default? We have few targets which don't work on android.
> 
> requires_android and supports_android are enforced here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/android/gyp/...
> 
> Neither is enabled by default for java_X, but they are for android_X.
> 
> https://codereview.chromium.org/1278573002/diff/200001/chrome/android/BUILD.gn
> File chrome/android/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1278573002/diff/200001/chrome/android/BUILD.g...
> chrome/android/BUILD.gn:518: "//base:base_junit_test_support",
> On 2015/08/24 at 15:28:44, Yaron wrote:
> > what about content? It also depends on BaseChromiumApplication
> 
> The test case in content_junit_tests doesn't currently depend on
> BaseChromiumApplication. I can add it if you'd prefer, though.

Depends on how obvious the failure would be?

lgtm

[jbudorick, 2015-08-25 15:51:53]

On 2015/08/25 at 15:49:20, yfriedman wrote:
> On 2015/08/24 21:13:51, jbudorick wrote:
> >
https://codereview.chromium.org/1278573002/diff/200001/build/config/android/r...
> > File build/config/android/rules.gni (right):
> > 
> >
https://codereview.chromium.org/1278573002/diff/200001/build/config/android/r...
> > build/config/android/rules.gni:1088: #   supports_android: If true, Android
> > targets (android_library, android_apk)
> > On 2015/08/24 at 15:28:44, Yaron wrote:
> > > Am I missing something? I don't see where this is enforced. Also, wouldn't
> > this be the default? We have few targets which don't work on android.
> > 
> > requires_android and supports_android are enforced here:
> >
https://code.google.com/p/chromium/codesearch#chromium/src/build/android/gyp/...
> > 
> > Neither is enabled by default for java_X, but they are for android_X.
> > 
> >
https://codereview.chromium.org/1278573002/diff/200001/chrome/android/BUILD.gn
> > File chrome/android/BUILD.gn (right):
> > 
> >
https://codereview.chromium.org/1278573002/diff/200001/chrome/android/BUILD.g...
> > chrome/android/BUILD.gn:518: "//base:base_junit_test_support",
> > On 2015/08/24 at 15:28:44, Yaron wrote:
> > > what about content? It also depends on BaseChromiumApplication
> > 
> > The test case in content_junit_tests doesn't currently depend on
> > BaseChromiumApplication. I can add it if you'd prefer, though.
> 
> Depends on how obvious the failure would be?

I mean, I thought it was pretty obvious, but I'm a bit biased.

> 
> lgtm

[jbudorick, 2015-08-25 15:52:00]

The CQ bit was checked by jbudorick@chromium.org

[jbudorick, 2015-08-25 15:52:00]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1278573002/#ps220001
(title: "rebase")

[commit-bot: I haz the power, 2015-08-25 15:52:43]

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

[commit-bot: I haz the power, 2015-08-25 17:03:58]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-08-25 17:04:31]

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

[jbudorick, 2015-08-25 17:38:55]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/1315833007/ by jbudorick@chromium.org.

The reason for reverting is: Somehow broke compilation on the main waterfall:
https://build.chromium.org/p/chromium/builders/Android/builds/45388.