Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2075753002:
aw: Try strip out last component of app name for android_res (Closed)
Descriptionaw: Try strip out last component of app name for android_res
Gradle allows the R class of the app to differ from the
application name. This CL specifically handles the case where
the R class is the application name with suffixes added. Eg:
app: com.example.app
R class: com.example.app.free.R
BUG=599869
Committed: https://crrev.com/3730ff3155bc7c214865a01e0bfbe7221eac3e62
Cr-Commit-Position: refs/heads/master@{#400985}
Patch Set 1 #Patch Set 2 : check strip #
Total comments: 4
Patch Set 3 : review #Patch Set 4 : minor clean up #Messages
Total messages: 19 (7 generated)
boliu@chromium.org changed reviewers: + torne@chromium.org
Something like this? I don't have a good way to test this though. Maybe just a java unit test for the strip method. Would be the first java unit test in webview. For end-to-end, I don't actually know how to do what gradle does.. Plus creating a whole other app just to test this seems overkill. Did a specific app break that prompted you to file the bug?
https://codereview.chromium.org/2075753002/diff/20001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AndroidProtocolHandler.java (right): https://codereview.chromium.org/2075753002/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AndroidProtocolHandler.java:83: // Strip out last component so gradle generated application uffix such as I assumed we'd strip off an increasing number of components until it worked, but in practise just stripping one is probably enough. I don't think gradle *stops* you from adding a suffix like .foo.bar but it's probably not common? https://codereview.chromium.org/2075753002/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AndroidProtocolHandler.java:87: d = getClazz(context, packageName, assetType); This should probably catch the ClassNotFoundException and throw the original one instead, otherwise the error in logcat will be confusing.
On 2016/06/16 19:19:26, boliu wrote: > Something like this? > > I don't have a good way to test this though. Maybe just a java unit test for the > strip method. Would be the first java unit test in webview. I wouldn't think it's worth adding a unit test just for that really. > For end-to-end, I don't actually know how to do what gradle does.. Plus creating > a whole other app just to test this seems overkill. Did a specific app break > that prompted you to file the bug? I don't actually know how to do what gradle is doing here either, without just using gradle - some combination of aapt parameters and different manifest contents? No actual deployed app broke because of this, because this has never worked, but there are a *lot* of people who have recently started using the applicationIdSuffix feature in gradle to build test/dev/debug/etc versions of their app, and discovered that it broke their usage of file:///android_res/ (both on the original android bug, plus an internal bug filed by a partner). I assume that they have either stopped trying to use applicationIdSuffix or stopped using file:///android_res for now. We could at least test this manually once by building a trivial app with Android Studio, if you haven't already done that.
https://codereview.chromium.org/2075753002/diff/20001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AndroidProtocolHandler.java (right): https://codereview.chromium.org/2075753002/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AndroidProtocolHandler.java:83: // Strip out last component so gradle generated application uffix such as On 2016/06/17 10:40:52, Torne wrote: > I assumed we'd strip off an increasing number of components until it worked, but > in practise just stripping one is probably enough. I don't think gradle *stops* > you from adding a suffix like .foo.bar but it's probably not common? I will find out.. https://codereview.chromium.org/2075753002/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AndroidProtocolHandler.java:87: d = getClazz(context, packageName, assetType); On 2016/06/17 10:40:52, Torne wrote: > This should probably catch the ClassNotFoundException and throw the original one > instead, otherwise the error in logcat will be confusing. confusing in the sense that it only prints a partial package name at this point?
On 2016/06/17 15:21:04, boliu wrote: > https://codereview.chromium.org/2075753002/diff/20001/android_webview/java/sr... > File > android_webview/java/src/org/chromium/android_webview/AndroidProtocolHandler.java > (right): > > https://codereview.chromium.org/2075753002/diff/20001/android_webview/java/sr... > android_webview/java/src/org/chromium/android_webview/AndroidProtocolHandler.java:83: > // Strip out last component so gradle generated application uffix such as > On 2016/06/17 10:40:52, Torne wrote: > > I assumed we'd strip off an increasing number of components until it worked, > but > > in practise just stripping one is probably enough. I don't think gradle > *stops* > > you from adding a suffix like .foo.bar but it's probably not common? > > I will find out.. > > https://codereview.chromium.org/2075753002/diff/20001/android_webview/java/sr... > android_webview/java/src/org/chromium/android_webview/AndroidProtocolHandler.java:87: > d = getClazz(context, packageName, assetType); > On 2016/06/17 10:40:52, Torne wrote: > > This should probably catch the ClassNotFoundException and throw the original > one > > instead, otherwise the error in logcat will be confusing. > > confusing in the sense that it only prints a partial package name at this point? Yeah, it will complain about a missing class that actually is expected not to exist for most apps, instead of the "normal" class name.
ok, set up android studio just for this, manually tested this android studio certainly doesn't prevent you from setting an application id that has more than one suffix append to the application name, actually as you pointed out, they might have nothing to do with each other comments should be addressed
Description was changed from ========== aw: Try strip out last component of app name for android_res BUG=599869 ========== to ========== aw: Try strip out last component of app name for android_res It's possible for the application name and the application id to differ. This specifically handles the case where the ID is the name with suffixes added. Eg: name: com.example.app ID: com.example.app.free Note context.getPackageName returns the ID BUG=599869 ==========
Description was changed from ========== aw: Try strip out last component of app name for android_res It's possible for the application name and the application id to differ. This specifically handles the case where the ID is the name with suffixes added. Eg: name: com.example.app ID: com.example.app.free Note context.getPackageName returns the ID BUG=599869 ========== to ========== aw: Try strip out last component of app name for android_res It's possible for the application name and the application id to differ. This specifically handles the case where the ID is the name with suffix(es) added. Eg: name: com.example.app ID: com.example.app.free Note context.getPackageName returns the ID BUG=599869 ==========
On 2016/06/20 21:44:30, boliu wrote: > ok, set up android studio just for this, manually tested this > > android studio certainly doesn't prevent you from setting an application id that > has more than one suffix append to the application name, actually as you pointed > out, they might have nothing to do with each other > > comments should be addressed Code LGTM, but the description maybe could be a bit clearer that this is a feature of the gradle build and not Android (there's not actually any such thing as an application name or application id in the platform - there are only package names)
Description was changed from ========== aw: Try strip out last component of app name for android_res It's possible for the application name and the application id to differ. This specifically handles the case where the ID is the name with suffix(es) added. Eg: name: com.example.app ID: com.example.app.free Note context.getPackageName returns the ID BUG=599869 ========== to ========== aw: Try strip out last component of app name for android_res Gradle allows the R class of the app to differ from the application name. This CL specifically handles the case where the R class is the application name with suffixes added. Eg: app: com.example.app R class: com.example.app.free.R BUG=599869 ==========
On 2016/06/21 10:03:38, Torne wrote: > On 2016/06/20 21:44:30, boliu wrote: > > ok, set up android studio just for this, manually tested this > > > > android studio certainly doesn't prevent you from setting an application id > that > > has more than one suffix append to the application name, actually as you > pointed > > out, they might have nothing to do with each other > > > > comments should be addressed > > Code LGTM, but the description maybe could be a bit clearer that this is a > feature of the gradle build and not Android (there's not actually any such thing > as an application name or application id in the platform - there are only > package names) how about now? fwiw app id vs name thing came from gradle documentation: http://tools.android.com/tech-docs/new-build-system/applicationid-vs-packagename
On 2016/06/21 13:44:21, boliu wrote: > On 2016/06/21 10:03:38, Torne wrote: > > On 2016/06/20 21:44:30, boliu wrote: > > > ok, set up android studio just for this, manually tested this > > > > > > android studio certainly doesn't prevent you from setting an application id > > that > > > has more than one suffix append to the application name, actually as you > > pointed > > > out, they might have nothing to do with each other > > > > > > comments should be addressed > > > > Code LGTM, but the description maybe could be a bit clearer that this is a > > feature of the gradle build and not Android (there's not actually any such > thing > > as an application name or application id in the platform - there are only > > package names) > > how about now? Yeah, that looks reasonable. > fwiw app id vs name thing came from gradle documentation: > http://tools.android.com/tech-docs/new-build-system/applicationid-vs-packagename Yeah, I realise, but none of these actually *mean anything* in terms of the contents of the APK, or at runtime - there's just "whatever the R class happens to be called", which in most usages doesn't matter whatsoever and is only the business of the java compiler (file:///android_res/ is the weird odd-one-out case), and what the APK manifest actually says it's called ;)
The CQ bit was checked by boliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2075753002/60001
Message was sent while issue was closed.
Description was changed from ========== aw: Try strip out last component of app name for android_res Gradle allows the R class of the app to differ from the application name. This CL specifically handles the case where the R class is the application name with suffixes added. Eg: app: com.example.app R class: com.example.app.free.R BUG=599869 ========== to ========== aw: Try strip out last component of app name for android_res Gradle allows the R class of the app to differ from the application name. This CL specifically handles the case where the R class is the application name with suffixes added. Eg: app: com.example.app R class: com.example.app.free.R BUG=599869 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== aw: Try strip out last component of app name for android_res Gradle allows the R class of the app to differ from the application name. This CL specifically handles the case where the R class is the application name with suffixes added. Eg: app: com.example.app R class: com.example.app.free.R BUG=599869 ========== to ========== aw: Try strip out last component of app name for android_res Gradle allows the R class of the app to differ from the application name. This CL specifically handles the case where the R class is the application name with suffixes added. Eg: app: com.example.app R class: com.example.app.free.R BUG=599869 Committed: https://crrev.com/3730ff3155bc7c214865a01e0bfbe7221eac3e62 Cr-Commit-Position: refs/heads/master@{#400985} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/3730ff3155bc7c214865a01e0bfbe7221eac3e62 Cr-Commit-Position: refs/heads/master@{#400985} |
