Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2079613002:
Revamping -keeps for @annotations (Closed)
DescriptionFixing -keep structure for @annotations
The keeps we had for annotations do not work as advertised. The way we were
doing them was equivalent to what I changed it to PLUS
-keep class com.google.android.apps.chrome.**,org.chromium.**
This change saves us ~207kb and 499 methods. See
https://paste.googleplex.com/6354796046974976 for what was removed.
BUG=619937
Committed: https://crrev.com/8b6b47fa26bc2f1931e2706c19fbe4af0d02eade
Cr-Commit-Position: refs/heads/master@{#401602}
Patch Set 1 #Patch Set 2 : Fixed some of the broken tests #Patch Set 3 : Fixing tests #Patch Set 4 : Rebased #Patch Set 5 : keeping protocol buffer classes to pass tests #Patch Set 6 : Cleaning up for review #Patch Set 7 : Minor fragment change #Patch Set 8 : Changing how we keep fragments #
Total comments: 8
Patch Set 9 : Fixing comments in flags file #
Messages
Total messages: 35 (17 generated)
The CQ bit was checked by smaier@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2079613002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by smaier@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2079613002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by smaier@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2079613002/120001
Description was changed from ========== TEST to see if passes buildbots BUG=619937 ========== to ========== Fixing -keep structure for @annotations The keeps we had for annotations do not work as advertised. The way we were doing them was equivalent to what I changed it to PLUS -keep class com.google.android.apps.chrome.**,org.chromium.** This change saves us ~227kb BUG=619937 ==========
Description was changed from ========== Fixing -keep structure for @annotations The keeps we had for annotations do not work as advertised. The way we were doing them was equivalent to what I changed it to PLUS -keep class com.google.android.apps.chrome.**,org.chromium.** This change saves us ~227kb BUG=619937 ========== to ========== Fixing -keep structure for @annotations The keeps we had for annotations do not work as advertised. The way we were doing them was equivalent to what I changed it to PLUS -keep class com.google.android.apps.chrome.**,org.chromium.** This change saves us ~227kb and 689 methods. See https://paste.googleplex.com/5840860260335616 for what was removed. BUG=619937 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from ========== Fixing -keep structure for @annotations The keeps we had for annotations do not work as advertised. The way we were doing them was equivalent to what I changed it to PLUS -keep class com.google.android.apps.chrome.**,org.chromium.** This change saves us ~227kb and 689 methods. See https://paste.googleplex.com/5840860260335616 for what was removed. BUG=619937 ========== to ========== Fixing -keep structure for @annotations The keeps we had for annotations do not work as advertised. The way we were doing them was equivalent to what I changed it to PLUS -keep class com.google.android.apps.chrome.**,org.chromium.** This change saves us ~227kb and 689 methods. See https://paste.googleplex.com/6354796046974976 for what was removed. BUG=619937 ==========
Description was changed from ========== Fixing -keep structure for @annotations The keeps we had for annotations do not work as advertised. The way we were doing them was equivalent to what I changed it to PLUS -keep class com.google.android.apps.chrome.**,org.chromium.** This change saves us ~227kb and 689 methods. See https://paste.googleplex.com/6354796046974976 for what was removed. BUG=619937 ========== to ========== Fixing -keep structure for @annotations The keeps we had for annotations do not work as advertised. The way we were doing them was equivalent to what I changed it to PLUS -keep class com.google.android.apps.chrome.**,org.chromium.** This change saves us ~207kb and 499 methods. See https://paste.googleplex.com/6354796046974976 for what was removed. BUG=619937 ==========
The CQ bit was checked by smaier@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2079613002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
smaier@chromium.org changed reviewers: + agrieve@chromium.org, avayvod@chromium.org, dfalcantara@chromium.org
avayvod@chromium.org: Please review changes in org/chromium/chrome/browser/media/remote agrieve@chromium.org: Please review changes in proguard.flags dfalcantara@chromium.org: Please review changes in org/chromium/chrome/browser/banners
https://codereview.chromium.org/2079613002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/banners/AppDetailsDelegate.java (right): https://codereview.chromium.org/2079613002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/banners/AppDetailsDelegate.java:12: @VisibleForTesting This isn't the right annotation for this. AppDetailsDelegate is the interface for an internal class, which isn't used for testing.
https://codereview.chromium.org/2079613002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/banners/AppDetailsDelegate.java (right): https://codereview.chromium.org/2079613002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/banners/AppDetailsDelegate.java:12: @VisibleForTesting On 2016/06/22 17:14:45, dfalcantara wrote: > This isn't the right annotation for this. AppDetailsDelegate is the interface > for an internal class, which isn't used for testing. Upstream, this class is only used constructed in tests. Proguard stripping this class's constructor does not affect how chrome_public_apk runs, but it does ruin the tests. What annotation would you suggest?
lgtm You're good. Thanks! https://codereview.chromium.org/2079613002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/banners/AppDetailsDelegate.java (right): https://codereview.chromium.org/2079613002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/banners/AppDetailsDelegate.java:12: @VisibleForTesting On 2016/06/22 17:25:16, smaier wrote: > On 2016/06/22 17:14:45, dfalcantara wrote: > > This isn't the right annotation for this. AppDetailsDelegate is the interface > > for an internal class, which isn't used for testing. > > Upstream, this class is only used constructed in tests. Proguard stripping this > class's constructor does not affect how chrome_public_apk runs, but it does ruin > the tests. What annotation would you suggest? Huh. Completely forgot that this was even used upstream without the tests. Works for me.
https://codereview.chromium.org/2079613002/diff/140001/chrome/android/java/pr... File chrome/android/java/proguard.flags (right): https://codereview.chromium.org/2079613002/diff/140001/chrome/android/java/pr... chrome/android/java/proguard.flags:19: # This can be improved upon - see b/622023. nit: Please use either crbug/622023, or http://crbug.com/622023. b/ often refers to Google's bug tracker. https://codereview.chromium.org/2079613002/diff/140001/chrome/android/java/pr... chrome/android/java/proguard.flags:23: # This block will keep all members that are annotated to be kept. nit: this -> these, can you also elaborate to explain why this is different from -keep. E.g.: # Keep these class members if and only if the containing class is kept. https://codereview.chromium.org/2079613002/diff/140001/chrome/android/java/pr... chrome/android/java/proguard.flags:52: @**.CalledByNative <methods>; I think we'd need a -keep for the ByNative ones as well?
https://codereview.chromium.org/2079613002/diff/140001/chrome/android/java/pr... File chrome/android/java/proguard.flags (right): https://codereview.chromium.org/2079613002/diff/140001/chrome/android/java/pr... chrome/android/java/proguard.flags:19: # This can be improved upon - see b/622023. On 2016/06/22 18:08:06, agrieve wrote: > nit: Please use either crbug/622023, or http://crbug.com/622023. > > b/ often refers to Google's bug tracker. Done. https://codereview.chromium.org/2079613002/diff/140001/chrome/android/java/pr... chrome/android/java/proguard.flags:23: # This block will keep all members that are annotated to be kept. On 2016/06/22 18:08:06, agrieve wrote: > nit: this -> these, can you also elaborate to explain why this is different from > -keep. E.g.: > # Keep these class members if and only if the containing class is kept. Discussed offline about these comments.
On 2016/06/22 18:24:06, smaier wrote: > https://codereview.chromium.org/2079613002/diff/140001/chrome/android/java/pr... > File chrome/android/java/proguard.flags (right): > > https://codereview.chromium.org/2079613002/diff/140001/chrome/android/java/pr... > chrome/android/java/proguard.flags:19: # This can be improved upon - see > b/622023. > On 2016/06/22 18:08:06, agrieve wrote: > > nit: Please use either crbug/622023, or http://crbug.com/622023. > > > > b/ often refers to Google's bug tracker. > > Done. > > https://codereview.chromium.org/2079613002/diff/140001/chrome/android/java/pr... > chrome/android/java/proguard.flags:23: # This block will keep all members that > are annotated to be kept. > On 2016/06/22 18:08:06, agrieve wrote: > > nit: this -> these, can you also elaborate to explain why this is different > from > > -keep. E.g.: > > # Keep these class members if and only if the containing class is kept. > > Discussed offline about these comments. lgtm
The CQ bit was checked by smaier@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2079613002/#ps160001 (title: "Fixing comments in flags file")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2079613002/160001
Message was sent while issue was closed.
Description was changed from ========== Fixing -keep structure for @annotations The keeps we had for annotations do not work as advertised. The way we were doing them was equivalent to what I changed it to PLUS -keep class com.google.android.apps.chrome.**,org.chromium.** This change saves us ~207kb and 499 methods. See https://paste.googleplex.com/6354796046974976 for what was removed. BUG=619937 ========== to ========== Fixing -keep structure for @annotations The keeps we had for annotations do not work as advertised. The way we were doing them was equivalent to what I changed it to PLUS -keep class com.google.android.apps.chrome.**,org.chromium.** This change saves us ~207kb and 499 methods. See https://paste.googleplex.com/6354796046974976 for what was removed. BUG=619937 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Fixing -keep structure for @annotations The keeps we had for annotations do not work as advertised. The way we were doing them was equivalent to what I changed it to PLUS -keep class com.google.android.apps.chrome.**,org.chromium.** This change saves us ~207kb and 499 methods. See https://paste.googleplex.com/6354796046974976 for what was removed. BUG=619937 ========== to ========== Fixing -keep structure for @annotations The keeps we had for annotations do not work as advertised. The way we were doing them was equivalent to what I changed it to PLUS -keep class com.google.android.apps.chrome.**,org.chromium.** This change saves us ~207kb and 499 methods. See https://paste.googleplex.com/6354796046974976 for what was removed. BUG=619937 Committed: https://crrev.com/8b6b47fa26bc2f1931e2706c19fbe4af0d02eade Cr-Commit-Position: refs/heads/master@{#401602} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/8b6b47fa26bc2f1931e2706c19fbe4af0d02eade Cr-Commit-Position: refs/heads/master@{#401602} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
