Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1291793007:
GN(android): Add scripts & runtime logic for installing _managed apks (Closed)
|
Created:
5 years, 4 months ago by agrieve Modified:
5 years, 3 months ago CC:
chromium-reviews, jbudorick+watch_chromium.org, klundberg+watch_chromium.org, yfriedman+watch_chromium.org, mfomitchev Base URL:
https://chromium.googlesource.com/chromium/src.git@gn-managed-install Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGN(android): Add scripts & runtime logic for installing _incremental apks
Currently only .so files are side-loaded (no .dex side-loading yet).
Does not require a rooted device.
Usage:
ninja -C out/Debug chrome_apk_incremental
out/Debug/bin/install_incremental_chrome_app_apk
BUG=520082
Committed: https://crrev.com/ae65db8da9a27bea569869f4b5a86de4b4857260
Cr-Commit-Position: refs/heads/master@{#346583}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Added --uninstall to script #
Total comments: 6
Patch Set 3 : javadoc & faster managed_install.py when --device specified #
Total comments: 20
Patch Set 4 : review comments #
Total comments: 15
Patch Set 5 : review comments #
Total comments: 3
Patch Set 6 : rebase #Patch Set 7 : fix compile #Messages
Total messages: 40 (10 generated)
agrieve@chromium.org changed reviewers: + jbudorick@chromium.org, nyquist@chromium.org
nyquist@chromium.org: Please review changes in /base jbudorick@chromium.org: Please review changes in /build Note: not sure if it's the best approach to stick these debug-only files right in base. Might make more sense as a sub-library? wdyt?
I personally like small and to-the-point targets, and like having a conditional DEPS better than conditional files. I don't feel strongly though. https://codereview.chromium.org/1291793007/diff/20001/base/android/java/debug... File base/android/java/debug_src/org/chromium/base/Reflect.java (right): https://codereview.chromium.org/1291793007/diff/20001/base/android/java/debug... base/android/java/debug_src/org/chromium/base/Reflect.java:5: package org.chromium.base; Could we put this in org.chromium.base.reflect? https://codereview.chromium.org/1291793007/diff/20001/base/android/java/debug... base/android/java/debug_src/org/chromium/base/Reflect.java:14: static void setField(Object instance, String name, Object val) If these become public, I guess you should add some explanatory comment. https://codereview.chromium.org/1291793007/diff/20001/base/android/java/debug... base/android/java/debug_src/org/chromium/base/Reflect.java:44: private static Field findField(Object instance, String name) throws NoSuchFieldException { Could you add a short comment explaining that this method recursively looks at parent classes?
I think having it in its own target will probably be the way to go, but I wanted to minimize build rule changes for now. There will be more code required for dex side-loading, and I think once I get there it will be more obvious what the better approach is. https://codereview.chromium.org/1291793007/diff/20001/base/android/java/debug... File base/android/java/debug_src/org/chromium/base/Reflect.java (right): https://codereview.chromium.org/1291793007/diff/20001/base/android/java/debug... base/android/java/debug_src/org/chromium/base/Reflect.java:5: package org.chromium.base; On 2015/08/19 17:13:15, nyquist (OOO - back 8-24) wrote: > Could we put this in org.chromium.base.reflect? We could, but then I'd have to make all the methods public (which I was thinking we should avoid until there's another client) https://codereview.chromium.org/1291793007/diff/20001/base/android/java/debug... base/android/java/debug_src/org/chromium/base/Reflect.java:14: static void setField(Object instance, String name, Object val) On 2015/08/19 17:13:15, nyquist (OOO - back 8-24) wrote: > If these become public, I guess you should add some explanatory comment. I should add JavaDoc now. (done) https://codereview.chromium.org/1291793007/diff/20001/base/android/java/debug... base/android/java/debug_src/org/chromium/base/Reflect.java:44: private static Field findField(Object instance, String name) throws NoSuchFieldException { On 2015/08/19 17:13:15, nyquist (OOO - back 8-24) wrote: > Could you add a short comment explaining that this method recursively looks at > parent classes? Done.
agrieve@chromium.org changed reviewers: + thestig@chromium.org
thestig@chromium.org: Please review changes in base/BUILD.gn, base/base.gyp
https://codereview.chromium.org/1291793007/diff/40001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1291793007/diff/40001/base/BUILD.gn#newcode1595 base/BUILD.gn:1595: [ "android/java/release_src/org/chromium/base/ManagedInstall.java" ] Can this be: java_files = [ "android/java/release_src/org/chromium/base/ManagedInstall.java" ] if (is_debug) { java_files += [ "android/java/debug_src/org/chromium/base/Reflect.java" ] }
https://codereview.chromium.org/1291793007/diff/40001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1291793007/diff/40001/base/BUILD.gn#newcode1595 base/BUILD.gn:1595: [ "android/java/release_src/org/chromium/base/ManagedInstall.java" ] On 2015/08/20 02:01:48, Lei Zhang wrote: > Can this be: > > java_files = [ "android/java/release_src/org/chromium/base/ManagedInstall.java" > ] > > if (is_debug) { > java_files += [ "android/java/debug_src/org/chromium/base/Reflect.java" ] > } No - the files with the same name are different (in release_src vs debug_src). The idea is that the calling code can call ManagedInstall.initialize(), and when not in debug mode the release_src version will just provide an empty function.
Well, I'm happy if the Android reviewers are happy. https://codereview.chromium.org/1291793007/diff/40001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1291793007/diff/40001/base/BUILD.gn#newcode1595 base/BUILD.gn:1595: [ "android/java/release_src/org/chromium/base/ManagedInstall.java" ] On 2015/08/20 02:11:05, agrieve wrote: > On 2015/08/20 02:01:48, Lei Zhang wrote: > > Can this be: > > > > java_files = [ > "android/java/release_src/org/chromium/base/ManagedInstall.java" > > ] > > > > if (is_debug) { > > java_files += [ "android/java/debug_src/org/chromium/base/Reflect.java" ] > > } > > No - the files with the same name are different (in release_src vs debug_src). > > The idea is that the calling code can call ManagedInstall.initialize(), and when > not in debug mode the release_src version will just provide an empty function. Whoops, n/m.
https://codereview.chromium.org/1291793007/diff/1/build/android/gyp/util/buil... File build/android/gyp/util/build_utils.py (right): https://codereview.chromium.org/1291793007/diff/1/build/android/gyp/util/buil... build/android/gyp/util/build_utils.py:326: # Support both argparse and optparse. TODO to get rid of this once we've moved to argparse. https://codereview.chromium.org/1291793007/diff/40001/build/android/gyp/creat... File build/android/gyp/create_managed_install_script.py (right): https://codereview.chromium.org/1291793007/diff/40001/build/android/gyp/creat... build/android/gyp/create_managed_install_script.py:67: def relativize(path): Why relative paths? https://codereview.chromium.org/1291793007/diff/40001/build/android/gyp/creat... build/android/gyp/create_managed_install_script.py:71: os.path.dirname(__file__), os.path.pardir, 'managed_install.py') Should use constants.DIR_SOURCE_ROOT: https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli... https://codereview.chromium.org/1291793007/diff/40001/build/android/gyp/util/... File build/android/gyp/util/build_utils.py (right): https://codereview.chromium.org/1291793007/diff/40001/build/android/gyp/util/... build/android/gyp/util/build_utils.py:326: # Support both argparse and optparse. TODO: get rid of optparse https://codereview.chromium.org/1291793007/diff/40001/build/android/managed_i... File build/android/managed_install.py (right): https://codereview.chromium.org/1291793007/diff/40001/build/android/managed_i... build/android/managed_install.py:29: ) ? https://codereview.chromium.org/1291793007/diff/40001/build/android/managed_i... build/android/managed_install.py:60: device.default_retries = 0 - Why no retries? - Why not pass this to the constructor (and add default_timeout and default_retries kwargs to HealthyDevices)? https://codereview.chromium.org/1291793007/diff/40001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1291793007/diff/40001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:220: def default_timeout(self): I don't mind the getter, though I don't really see the point. https://codereview.chromium.org/1291793007/diff/40001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:224: def default_timeout(self, value): I don't like the setter. I don't think we should allow changing the default after construction. https://codereview.chromium.org/1291793007/diff/40001/build/config/android/ru... File build/config/android/rules.gni (right): https://codereview.chromium.org/1291793007/diff/40001/build/config/android/ru... build/config/android/rules.gni:1385: _final_apk_path_no_ext_list = Are these only used in the managed_create_script action? If so, why not define them there? https://codereview.chromium.org/1291793007/diff/40001/build/config/android/ru... build/config/android/rules.gni:1809: action(_managed_create_script_rule_name) { nit: create_managed_script_rule_name?
https://codereview.chromium.org/1291793007/diff/1/build/android/gyp/util/buil... File build/android/gyp/util/build_utils.py (right): https://codereview.chromium.org/1291793007/diff/1/build/android/gyp/util/buil... build/android/gyp/util/build_utils.py:326: # Support both argparse and optparse. On 2015/08/23 02:01:28, jbudorick wrote: > TODO to get rid of this once we've moved to argparse. Done. https://codereview.chromium.org/1291793007/diff/40001/build/android/gyp/creat... File build/android/gyp/create_managed_install_script.py (right): https://codereview.chromium.org/1291793007/diff/40001/build/android/gyp/creat... build/android/gyp/create_managed_install_script.py:67: def relativize(path): On 2015/08/23 02:01:28, jbudorick wrote: > Why relative paths? It's copied from create_test_runner_script.py. I'd guess the reason is to transform relative input paths into relative paths to a known point. They end up as absolute paths when they finally get to the install_incremental.py script, so might be overcomplicating things to resolve them in the generated script. Still, it does make it a bit more portable to not have absolute paths in files, so I like it for this reason. https://codereview.chromium.org/1291793007/diff/40001/build/android/gyp/creat... build/android/gyp/create_managed_install_script.py:71: os.path.dirname(__file__), os.path.pardir, 'managed_install.py') On 2015/08/23 02:01:28, jbudorick wrote: > Should use constants.DIR_SOURCE_ROOT: > https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli... Done. https://codereview.chromium.org/1291793007/diff/40001/build/android/managed_i... File build/android/managed_install.py (right): https://codereview.chromium.org/1291793007/diff/40001/build/android/managed_i... build/android/managed_install.py:29: ) On 2015/08/23 02:01:28, jbudorick wrote: > ? Done. https://codereview.chromium.org/1291793007/diff/40001/build/android/managed_i... build/android/managed_install.py:60: device.default_retries = 0 On 2015/08/23 02:01:28, jbudorick wrote: > - Why no retries? > - Why not pass this to the constructor (and add default_timeout and > default_retries kwargs to HealthyDevices)? Since this is for local dev, and can sometimes fail for legitimate reasons, I've found the retries a nuisance. Moved to the ctor. https://codereview.chromium.org/1291793007/diff/40001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1291793007/diff/40001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:220: def default_timeout(self): On 2015/08/23 02:01:28, jbudorick wrote: > I don't mind the getter, though I don't really see the point. Done. https://codereview.chromium.org/1291793007/diff/40001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:224: def default_timeout(self, value): On 2015/08/23 02:01:28, jbudorick wrote: > I don't like the setter. I don't think we should allow changing the default > after construction. Done. https://codereview.chromium.org/1291793007/diff/40001/build/config/android/ru... File build/config/android/rules.gni (right): https://codereview.chromium.org/1291793007/diff/40001/build/config/android/ru... build/config/android/rules.gni:1385: _final_apk_path_no_ext_list = On 2015/08/23 02:01:28, jbudorick wrote: > Are these only used in the managed_create_script action? If so, why not define > them there? It's used at line 1785 as well. https://codereview.chromium.org/1291793007/diff/40001/build/config/android/ru... build/config/android/rules.gni:1809: action(_managed_create_script_rule_name) { On 2015/08/23 02:01:28, jbudorick wrote: > nit: create_managed_script_rule_name? Done.
base lgtm https://codereview.chromium.org/1291793007/diff/60001/base/android/java/debug... File base/android/java/debug_src/org/chromium/base/IncrementalInstall.java (right): https://codereview.chromium.org/1291793007/diff/60001/base/android/java/debug... base/android/java/debug_src/org/chromium/base/IncrementalInstall.java:31: if (currentDirs instanceof List) { Optional nit: Care to add a comment as to when these two cases exist in the ClassLoader? Could be helpful in the future if we want to deprecate an old Android version.
lgtm stamp https://codereview.chromium.org/1291793007/diff/60001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1291793007/diff/60001/base/BUILD.gn#newcode1605 base/BUILD.gn:1605: java_files = [ "android/java/release_src/org/chromium/base/IncrementalInstall.java" ] nit: 80 chars / line?
https://codereview.chromium.org/1291793007/diff/60001/build/android/gn/create... File build/android/gn/create_incremental_install_script.py (right): https://codereview.chromium.org/1291793007/diff/60001/build/android/gn/create... build/android/gn/create_incremental_install_script.py:1: #!/usr/bin/env python build/android/gn...? I'd rather not have one build script all on its own. https://codereview.chromium.org/1291793007/diff/60001/build/android/increment... File build/android/incremental_install.py (right): https://codereview.chromium.org/1291793007/diff/60001/build/android/increment... build/android/incremental_install.py:52: raise device_errors.DeviceUnreachableError(args.device) As written, this block will never be executed. https://codereview.chromium.org/1291793007/diff/60001/build/android/increment... build/android/incremental_install.py:73: # Install .apk(s) if any of them have changed.apk "changed.apk" ? https://codereview.chromium.org/1291793007/diff/60001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1291793007/diff/60001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:223: @default_timeout.setter nix the setters. https://codereview.chromium.org/1291793007/diff/60001/build/config/android/ru... File build/config/android/rules.gni (right): https://codereview.chromium.org/1291793007/diff/60001/build/config/android/ru... build/config/android/rules.gni:1494: forward_variables_from(invoker, [ "language_splits" ]) We're already forwarding language_splits.
https://codereview.chromium.org/1291793007/diff/60001/build/android/gn/create... File build/android/gn/create_incremental_install_script.py (right): https://codereview.chromium.org/1291793007/diff/60001/build/android/gn/create... build/android/gn/create_incremental_install_script.py:1: #!/usr/bin/env python On 2015/08/27 00:37:37, jbudorick wrote: > build/android/gn...? > > I'd rather not have one build script all on its own. Me neither! Figured it could keep https://code.google.com/p/chromium/codesearch#chromium/src/build/android/gn/z... company :) I can move it to gyp/ still if you'd like though. https://codereview.chromium.org/1291793007/diff/60001/build/android/increment... File build/android/incremental_install.py (right): https://codereview.chromium.org/1291793007/diff/60001/build/android/increment... build/android/incremental_install.py:52: raise device_errors.DeviceUnreachableError(args.device) On 2015/08/27 00:37:37, jbudorick wrote: > As written, this block will never be executed. Done. https://codereview.chromium.org/1291793007/diff/60001/build/android/increment... build/android/incremental_install.py:73: # Install .apk(s) if any of them have changed.apk On 2015/08/27 00:37:37, jbudorick wrote: > "changed.apk" ? Done. https://codereview.chromium.org/1291793007/diff/60001/build/android/pylib/dev... File build/android/pylib/device/device_utils.py (right): https://codereview.chromium.org/1291793007/diff/60001/build/android/pylib/dev... build/android/pylib/device/device_utils.py:223: @default_timeout.setter On 2015/08/27 00:37:37, jbudorick wrote: > nix the setters. whoops, done. https://codereview.chromium.org/1291793007/diff/60001/build/config/android/ru... File build/config/android/rules.gni (right): https://codereview.chromium.org/1291793007/diff/60001/build/config/android/ru... build/config/android/rules.gni:1494: forward_variables_from(invoker, [ "language_splits" ]) On 2015/08/27 00:37:37, jbudorick wrote: > We're already forwarding language_splits. Done.
https://codereview.chromium.org/1291793007/diff/60001/base/BUILD.gn File base/BUILD.gn (right): https://codereview.chromium.org/1291793007/diff/60001/base/BUILD.gn#newcode1605 base/BUILD.gn:1605: java_files = [ "android/java/release_src/org/chromium/base/IncrementalInstall.java" ] On 2015/08/26 20:49:07, Lei Zhang wrote: > nit: 80 chars / line? Leaving it to the gn formatter. From what I can tell it allows single strings to go over 80 chars.
https://codereview.chromium.org/1291793007/diff/60001/build/android/gn/create... File build/android/gn/create_incremental_install_script.py (right): https://codereview.chromium.org/1291793007/diff/60001/build/android/gn/create... build/android/gn/create_incremental_install_script.py:1: #!/usr/bin/env python On 2015/08/27 at 03:19:59, agrieve wrote: > On 2015/08/27 00:37:37, jbudorick wrote: > > build/android/gn...? > > > > I'd rather not have one build script all on its own. > > Me neither! Figured it could keep https://code.google.com/p/chromium/codesearch#chromium/src/build/android/gn/z... company :) > ... I probably should've looked for that beforehand. > I can move it to gyp/ still if you'd like though. No, this is fine. https://codereview.chromium.org/1291793007/diff/80001/build/android/increment... File build/android/incremental_install.py (right): https://codereview.chromium.org/1291793007/diff/80001/build/android/increment... build/android/incremental_install.py:45: constants.SetBuildType('Debug') Oh, I forgot to mention this. I take it we're not supporting this for release builds?
https://codereview.chromium.org/1291793007/diff/80001/build/android/increment... File build/android/incremental_install.py (right): https://codereview.chromium.org/1291793007/diff/80001/build/android/increment... build/android/incremental_install.py:45: constants.SetBuildType('Debug') On 2015/08/27 03:24:09, jbudorick wrote: > Oh, I forgot to mention this. I take it we're not supporting this for release > builds? Correct. It requires some runtime java code now, which I'm including only in debug builds.
https://codereview.chromium.org/1291793007/diff/80001/build/android/increment... File build/android/incremental_install.py (right): https://codereview.chromium.org/1291793007/diff/80001/build/android/increment... build/android/incremental_install.py:45: constants.SetBuildType('Debug') On 2015/08/27 at 04:11:13, agrieve wrote: > On 2015/08/27 03:24:09, jbudorick wrote: > > Oh, I forgot to mention this. I take it we're not supporting this for release > > builds? > > Correct. It requires some runtime java code now, which I'm including only in debug builds. Hm. It'd be nice to have some kind of hard failure in the script if someone tries to use a release APK. Not entirely sure how to go about that off the top of my head, though.
On 2015/08/27 13:49:29, jbudorick wrote: > https://codereview.chromium.org/1291793007/diff/80001/build/android/increment... > File build/android/incremental_install.py (right): > > https://codereview.chromium.org/1291793007/diff/80001/build/android/increment... > build/android/incremental_install.py:45: constants.SetBuildType('Debug') > On 2015/08/27 at 04:11:13, agrieve wrote: > > On 2015/08/27 03:24:09, jbudorick wrote: > > > Oh, I forgot to mention this. I take it we're not supporting this for > release > > > builds? > > > > Correct. It requires some runtime java code now, which I'm including only in > debug builds. > > Hm. It'd be nice to have some kind of hard failure in the script if someone > tries to use a release APK. Not entirely sure how to go about that off the top > of my head, though. I doubt it would come up. The GN rules for building _incremental targets exist only for is_debug = true, so you can't actually build release targets.
On 2015/08/27 at 21:39:30, agrieve wrote: > On 2015/08/27 13:49:29, jbudorick wrote: > > https://codereview.chromium.org/1291793007/diff/80001/build/android/increment... > > File build/android/incremental_install.py (right): > > > > https://codereview.chromium.org/1291793007/diff/80001/build/android/increment... > > build/android/incremental_install.py:45: constants.SetBuildType('Debug') > > On 2015/08/27 at 04:11:13, agrieve wrote: > > > On 2015/08/27 03:24:09, jbudorick wrote: > > > > Oh, I forgot to mention this. I take it we're not supporting this for > > release > > > > builds? > > > > > > Correct. It requires some runtime java code now, which I'm including only in > > debug builds. > > > > Hm. It'd be nice to have some kind of hard failure in the script if someone > > tries to use a release APK. Not entirely sure how to go about that off the top > > of my head, though. > > I doubt it would come up. The GN rules for building _incremental targets exist only for is_debug = true, so you can't actually build release targets. ah, ok.
lgtm
Why restrict to only debug? I still use gyp_managed_install when building for release. I guess gyp managed install doesn't have runtime/perf implications, where this probably would? On Thu, Aug 27, 2015 at 5:39 PM <agrieve@chromium.org> wrote: > On 2015/08/27 13:49:29, jbudorick wrote: > > > https://codereview.chromium.org/1291793007/diff/80001/build/android/increment... > > File build/android/incremental_install.py (right): > > > > https://codereview.chromium.org/1291793007/diff/80001/build/android/increment... > > build/android/incremental_install.py:45: constants.SetBuildType('Debug') > > On 2015/08/27 at 04:11:13, agrieve wrote: > > > On 2015/08/27 03:24:09, jbudorick wrote: > > > > Oh, I forgot to mention this. I take it we're not supporting this for > > release > > > > builds? > > > > > > Correct. It requires some runtime java code now, which I'm including > > only in > > debug builds. > > > Hm. It'd be nice to have some kind of hard failure in the script if > > someone > > tries to use a release APK. Not entirely sure how to go about that off > > the top > > of my head, though. > > I doubt it would come up. The GN rules for building _incremental targets > exist > only for is_debug = true, so you can't actually build release targets. > > https://codereview.chromium.org/1291793007/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
That's my thinking. Although, if it is actually useful to have it in release mode we could add a separate build flag to enable it. On Thu, Aug 27, 2015 at 2:45 PM, Yaron Friedman <yfriedman@chromium.org> wrote: > Why restrict to only debug? I still use gyp_managed_install when building > for release. I guess gyp managed install doesn't have runtime/perf > implications, where this probably would? > > On Thu, Aug 27, 2015 at 5:39 PM <agrieve@chromium.org> wrote: > >> On 2015/08/27 13:49:29, jbudorick wrote: >> >> >> https://codereview.chromium.org/1291793007/diff/80001/build/android/increment... >> > File build/android/incremental_install.py (right): >> >> >> >> https://codereview.chromium.org/1291793007/diff/80001/build/android/increment... >> > build/android/incremental_install.py:45: constants.SetBuildType('Debug') >> > On 2015/08/27 at 04:11:13, agrieve wrote: >> > > On 2015/08/27 03:24:09, jbudorick wrote: >> > > > Oh, I forgot to mention this. I take it we're not supporting this >> for >> > release >> > > > builds? >> > > >> > > Correct. It requires some runtime java code now, which I'm including >> > only in >> > debug builds. >> >> > Hm. It'd be nice to have some kind of hard failure in the script if >> > someone >> > tries to use a release APK. Not entirely sure how to go about that off >> > the top >> > of my head, though. >> >> I doubt it would come up. The GN rules for building _incremental targets >> exist >> only for is_debug = true, so you can't actually build release targets. >> >> https://codereview.chromium.org/1291793007/ >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by agrieve@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, nyquist@chromium.org Link to the patchset: https://codereview.chromium.org/1291793007/#ps80001 (title: "review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1291793007/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1291793007/80001
https://codereview.chromium.org/1291793007/diff/60001/base/android/java/debug... File base/android/java/debug_src/org/chromium/base/IncrementalInstall.java (right): https://codereview.chromium.org/1291793007/diff/60001/base/android/java/debug... base/android/java/debug_src/org/chromium/base/IncrementalInstall.java:31: if (currentDirs instanceof List) { On 2015/08/26 05:23:54, nyquist wrote: > Optional nit: Care to add a comment as to when these two cases exist in the > ClassLoader? Could be helpful in the future if we want to deprecate an old > Android version. Going to skip on this for now just because I don't actually know when the changeover happens, and because it's possible that we won't even need this code once we add in .dex loading.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by agrieve@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, nyquist@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/1291793007/#ps100001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1291793007/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1291793007/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
The CQ bit was checked by agrieve@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, nyquist@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/1291793007/#ps120001 (title: "fix compile")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1291793007/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1291793007/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/ae65db8da9a27bea569869f4b5a86de4b4857260 Cr-Commit-Position: refs/heads/master@{#346583} |
