Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1218303002:
Update WebView proguard configuration. (Closed)
DescriptionUpdate WebView proguard configuration.
Update the proguard configuration file for the WebView APK to make it
reflect the current code. This does not re-enable actually running
proguard on the APK and so will not have any effect by itself, but it
appears to work with limited local testing.
BUG=442348
Committed: https://crrev.com/178208c0b13b0e65f23bdfce7310229bba4c46fb
Cr-Commit-Position: refs/heads/master@{#337585}
Patch Set 1 #
Total comments: 3
Messages
Total messages: 14 (3 generated)
torne@chromium.org changed reviewers: + dgn@chromium.org, sgurun@chromium.org
I've done superficial local testing that verifies we don't crash from missing classes, and I've eyeballed the list of removed code and nothing major jumps out that it shouldn't be removing. It reduces the size of the .dex to 863,864 bytes from 2,338,092 bytes, 63% reduction, which is because we were apparently pulling in the entire support library into webview before, and proguard deletes the entire thing because we don't reference a single android.support.* class. Probably we shouldn't have had a dependency on the support library in the first place... Note that this CL intentionally does not actually turn proguard on in the gyp file. I'll do that separately after this lands to allow it to be reverted easily without undoing these config updates.
Also: I'm allowing the obfuscation pass to run without letting it change any names; the primary effect of this is that it deletes the local variable tables, saving ~100kb of dex size, but making it harder to debug the java code interactively since the debugger won't be able to reconstruct local variable info. I've preserved enough that stack traces should be unaffected. I am assuming that we don't actually debug release builds interactively ever and so this doesn't matter. ;)
What is SmartClip? Is that the only thing that uses android.support in webview? https://codereview.chromium.org/1218303002/diff/1/android_webview/apk/java/pr... File android_webview/apk/java/proguard.flags (right): https://codereview.chromium.org/1218303002/diff/1/android_webview/apk/java/pr... android_webview/apk/java/proguard.flags:23: -keepclasseswithmembers class com.android.webview.chromium.**,org.chromium.** { Why not the grouped format like we do in chrome? -keepclasseswithmembers class com.android.webview.chromium.**,org.chromium.** { @**.AccessedByNative <fields>; @**.CalledByNative <methods>; @**.CalledByNativeUnchecked <methods>; native <methods>; }
On 2015/07/02 16:10:01, dgn wrote: > What is SmartClip? Is that the only thing that uses android.support in webview? SmartClip is a weird feature used by reflection and not otherwise directly referenced so it has to be kept manually. It doesn't have anything to do with the support library, whcih we don't use at all. > > https://codereview.chromium.org/1218303002/diff/1/android_webview/apk/java/pr... > File android_webview/apk/java/proguard.flags (right): > > https://codereview.chromium.org/1218303002/diff/1/android_webview/apk/java/pr... > android_webview/apk/java/proguard.flags:23: -keepclasseswithmembers class > com.android.webview.chromium.**,org.chromium.** { > Why not the grouped format like we do in chrome? > > -keepclasseswithmembers class com.android.webview.chromium.**,org.chromium.** { > @**.AccessedByNative <fields>; > @**.CalledByNative <methods>; > @**.CalledByNativeUnchecked <methods>; > native <methods>; > } That wouldn't work, as it would only keep classes with *all* of those members. Chrome uses -keep, not -keepclasseswithmembers, which means the wrong thing: it keeps the classes, as well as those members, even if the classes are otherwise unreferenced (and therefore chrome never deletes the classes even if it deletes every member of them).
torne@chromium.org changed reviewers: + aurimas@chromium.org
+aurimas who made a number of other proguard changes for chrome
lgtm with one question https://codereview.chromium.org/1218303002/diff/1/android_webview/apk/java/pr... File android_webview/apk/java/proguard.flags (left): https://codereview.chromium.org/1218303002/diff/1/android_webview/apk/java/pr... android_webview/apk/java/proguard.flags:57: # We need to keep these explicitly as they are parameters to methods which Why don't we need this section anymore?
LGTM it seems reasonable. Are we going to test this on Googlers first in Play?
I'm going to enable it on trunk after the M45 branch point so that we get manual QA and internal dogfooding of it on M46 dev (and then publicly with M46 beta). https://chromiumcodereview.appspot.com/1218303002/diff/1/android_webview/apk/... File android_webview/apk/java/proguard.flags (left): https://chromiumcodereview.appspot.com/1218303002/diff/1/android_webview/apk/... android_webview/apk/java/proguard.flags:57: # We need to keep these explicitly as they are parameters to methods which On 2015/07/06 17:37:24, sgurun wrote: > Why don't we need this section anymore? The descriptor classes are always referenced (by the methods that use them) and so can't actually be removed anyway - the only reason they needed to be kept before was apparently because Proguard is concerned that they might be *renamed* during the obfuscation phase which would change the JNI method signatures and break the JNI interface. For some reason (proguard isn't very clever) it warns about this even when you are using -dontobfuscate to completely prevent all renamings. However, we are now allowing obfuscation but explicitly forcing it to keep all the names the same with the global -keepnames wildcard, and so it no longer warns about these things because -keepnames is sufficient.
The CQ bit was checked by torne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1218303002/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/178208c0b13b0e65f23bdfce7310229bba4c46fb Cr-Commit-Position: refs/heads/master@{#337585} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
