Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2634563002:
android_webview: support building a stub WebView. (Closed)
Descriptionandroid_webview: support building a stub WebView.
Allwo building a WebView APK which doesn't contain any code or assets,
and no resources other than the icon. This APK has a special manifest
tag which will direct the system to provide the missing files from
another "donor" package at runtime that has compatible code (this will
be Monochrome).
To enable this to work, a new APK variable is defined called
"generate_buildconfig_java" so that the BuildConfig.java file can be
omitted from this APK (leaving the generated classes.dex containing
nothing but the AAPT-generated R class for the single icon resource).
The runtime doesn't like having multiple APKs in the classpath which
define the same classes, so we need to omit this BuildConfig to avoid
clashing with Monochrome's.
BUG=664456
Patch Set 1 #
Total comments: 4
Patch Set 2 : add comment #
Total comments: 6
Patch Set 3 : rebase #Patch Set 4 : new approach - no java code #Patch Set 5 : add comment describing new option #
Total comments: 8
Patch Set 6 : Update comments, remove commented build target for now #Patch Set 7 : rebase #Messages
Total messages: 34 (11 generated)
torne@chromium.org changed reviewers: + jbudorick@chromium.org, michaelbai@chromium.org
Not planning to land this just yet as I'm still working on the Android side code to be able to load this (it's partly functional but not finished). Just wanted to get some initial comments on how I'm handling the "fake" shared library support in GN, and any other general thoughts.
I'm generally ok with this. https://codereview.chromium.org/2634563002/diff/1/build/config/android/rules.gni File build/config/android/rules.gni (left): https://codereview.chromium.org/2634563002/diff/1/build/config/android/rules.... build/config/android/rules.gni:1708: "$target_gen_dir/$target_name.ordered_libararies.json" :O https://codereview.chromium.org/2634563002/diff/1/build/config/android/rules.gni File build/config/android/rules.gni (right): https://codereview.chromium.org/2634563002/diff/1/build/config/android/rules.... build/config/android/rules.gni:1721: deps = _native_libs_deps + [ ":$build_config_target" ] Can you add a comment noting why _unpackaged_shared_libs isn't listed in deps here? While I think it's correct, I also think it'd be easy to come back at a later time and mistakenly think otherwise.
https://codereview.chromium.org/2634563002/diff/1/build/config/android/rules.gni File build/config/android/rules.gni (left): https://codereview.chromium.org/2634563002/diff/1/build/config/android/rules.... build/config/android/rules.gni:1708: "$target_gen_dir/$target_name.ordered_libararies.json" On 2017/01/13 20:45:48, jbudorick wrote: > :O Yeah, I noticed that when looking in the out directory ;) https://codereview.chromium.org/2634563002/diff/1/build/config/android/rules.gni File build/config/android/rules.gni (right): https://codereview.chromium.org/2634563002/diff/1/build/config/android/rules.... build/config/android/rules.gni:1721: deps = _native_libs_deps + [ ":$build_config_target" ] On 2017/01/13 20:45:48, jbudorick wrote: > Can you add a comment noting why _unpackaged_shared_libs isn't listed in deps > here? While I think it's correct, I also think it'd be easy to come back at a > later time and mistakenly think otherwise. Done.
Ah, one more problem: the locale list in BuildConfig in the stub ends up empty (since there's no locale paks) and then things don't work at runtime. I guess I'll need to fake that as well.. :/
https://codereview.chromium.org/2634563002/diff/20001/android_webview/apk/jav... File android_webview/apk/java/AndroidManifest.xml (right): https://codereview.chromium.org/2634563002/diff/20001/android_webview/apk/jav... android_webview/apk/java/AndroidManifest.xml:51: android:value="{{ donor_package }}" /> Do you want to have AOSP donor? if not, is it internal only, and move to downstream? https://codereview.chromium.org/2634563002/diff/20001/build/android/gyp/write... File build/android/gyp/write_ordered_libraries.py (right): https://codereview.chromium.org/2634563002/diff/20001/build/android/gyp/write... build/android/gyp/write_ordered_libraries.py:125: all_libraries = libraries + unpackaged_libraries Is all_libraries actually used? https://codereview.chromium.org/2634563002/diff/20001/build/config/android/ru... File build/config/android/rules.gni (right): https://codereview.chromium.org/2634563002/diff/20001/build/config/android/ru... build/config/android/rules.gni:1537: if (defined(invoker.unpackaged_shared_libraries)) { Please update template description with this variable.
https://codereview.chromium.org/2634563002/diff/20001/android_webview/apk/jav... File android_webview/apk/java/AndroidManifest.xml (right): https://codereview.chromium.org/2634563002/diff/20001/android_webview/apk/jav... android_webview/apk/java/AndroidManifest.xml:51: android:value="{{ donor_package }}" /> On 2017/01/18 22:59:21, michaelbai wrote: > Do you want to have AOSP donor? if not, is it internal only, and move to > downstream? Yes, I intend to make this work for system_webview_apk + monochrome_public_apk as well, but I haven't done the work to put this together and test it yet; once I have the Android change working I'll paste it into my AOSP checkout and test that there, and add the definition in chromium. https://codereview.chromium.org/2634563002/diff/20001/build/android/gyp/write... File build/android/gyp/write_ordered_libraries.py (right): https://codereview.chromium.org/2634563002/diff/20001/build/android/gyp/write... build/android/gyp/write_ordered_libraries.py:125: all_libraries = libraries + unpackaged_libraries On 2017/01/18 22:59:21, michaelbai wrote: > Is all_libraries actually used? Yes, on the changed line immediately below, to generate java_libraries_list. We need to insert it into the list of libraries in NativeLibraries.java so that LibraryLoader will try to load it, but not into the other lists which will cause the build system to try to package/build it. https://codereview.chromium.org/2634563002/diff/20001/build/config/android/ru... File build/config/android/rules.gni (right): https://codereview.chromium.org/2634563002/diff/20001/build/config/android/ru... build/config/android/rules.gni:1537: if (defined(invoker.unpackaged_shared_libraries)) { On 2017/01/18 22:59:21, michaelbai wrote: > Please update template description with this variable. Added in my next version (not uploaded yet as I'm still trying to come up with a way to get the locale list to be correct in the APK).
OK, this is going to have to be handled pretty differently. The system doesn't actually like having two APKs loaded that declare duplicate java classes, and it was only working in very controlled circumstances. The stub needs to *also* not contain any java code, or at least not any with the same classnames as Monochrome, to work, so the magic done here to mangle NativeLibraries and so on isn't required - we'll need to just omit that entirely from the stub and then Monochrome's (already correct) version will be used. I'll post a new CL when I have something working; one challenge with this approach is making the license viewer activity still work, it'll probably have to be compiled into a different package name :/
Description was changed from ========== android_webview: support building a stub WebView. Allow a WebView APK target to set "webview_stub_only=true" to build a stub WebView. This stub does not include any native code or asset files, which are expected to be provided at runtime via a "donor" package that has compatible library and asset files included. The package name of the donor is set as a manifest metadata tag. This is to allow preinstalled WebView APKs on devices that also preinstall Monochrome to load their assets and libraries from the Monochrome APK, reducing the size of the preinstalled WebView. Since the versions must precisely match for this to work, it's only suitable for preinstalled versions. To enable this to work, a new APK variable is defined called "unpackaged_shared_libraries". This is a list of libraries which are not expected to be built or included in the APK, but will still be included in the NaitveLibraries.java generated file and loaded at runtime. BUG=664456 ========== to ========== android_webview: support building a stub WebView. Allwo building a WebView APK which doesn't contain any code or assets, and no resources other than the icon. This APK has a special manifest tag which will direct the system to provide the missing files from another "donor" package at runtime that has compatible code (this will be Monochrome). To enable this to work, a new APK variable is defined called "generate_buildconfig_java" so that the BuildConfig.java file can be omitted from this APK (leaving the generated classes.dex containing nothing but the AAPT-generated R class for the single icon resource). The runtime doesn't like having multiple APKs in the classpath which define the same classes, so we need to omit this BuildConfig to avoid clashing with Monochrome's. BUG=664456 ==========
Updated. This new approach omits all the java code from the stub APK, and therefore avoids having conflicting class definitions. I've also added a commented-out definition for an upstream version of the stub, which we can't use yet as there's no SDK version that supports this, but should be a reference for how this works.
This currently doesn't include the license activity, which we will need before we actually ship this. I'm looking at how to make this work, but this can land as-is to make testing easier and I'll add the license activity/content provider in a followup.
John, Michael, will you be able to take a look at this today? I'd like to get this in ASAP so that the release builders can start producing the stub APKs; it's very difficult to test this "realistically" without a lot of weird system image hacking right now because it's hard to get properly named/signed APKs that match.
On 2017/02/28 14:57:05, Torne wrote: > John, Michael, will you be able to take a look at this today? I'd like to get > this in ASAP so that the release builders can start producing the stub APKs; > it's very difficult to test this "realistically" without a lot of weird system > image hacking right now because it's hard to get properly named/signed APKs that > match. Ah, sorry, I missed your update last week. I'll try to look today.
No problem. Also: Michael, I reformatted the manifest xml to make it indented consistently, which is why the diff is fairly big - unfortunately the rietveld diff viewer isn't doing quite such a good job of displaying this as "git diff -w" :/
Looks pretty reasonable. https://codereview.chromium.org/2634563002/diff/80001/android_webview/BUILD.gn File android_webview/BUILD.gn (right): https://codereview.chromium.org/2634563002/diff/80001/android_webview/BUILD.g... android_webview/BUILD.gn:31: # TODO(torne): uncomment this once Android is updated to support loading this I think I'd prefer this be omitted until Android is updated w/ such support. Until then, there's nothing to prevent this from bitrotting. https://codereview.chromium.org/2634563002/diff/80001/build/config/android/ru... File build/config/android/rules.gni (right): https://codereview.chromium.org/2634563002/diff/80001/build/config/android/ru... build/config/android/rules.gni:1469: # is true. nit: mention that the default is true for non-test APKs.
On 2017/02/28 16:36:28, jbudorick wrote: > Looks pretty reasonable. er, actually, lgtm w/ nits > > https://codereview.chromium.org/2634563002/diff/80001/android_webview/BUILD.gn > File android_webview/BUILD.gn (right): > > https://codereview.chromium.org/2634563002/diff/80001/android_webview/BUILD.g... > android_webview/BUILD.gn:31: # TODO(torne): uncomment this once Android is > updated to support loading this nit: > I think I'd prefer this be omitted until Android is updated w/ such support. > Until then, there's nothing to prevent this from bitrotting. > > https://codereview.chromium.org/2634563002/diff/80001/build/config/android/ru... > File build/config/android/rules.gni (right): > > https://codereview.chromium.org/2634563002/diff/80001/build/config/android/ru... > build/config/android/rules.gni:1469: # is true. > nit: mention that the default is true for non-test APKs.
LGTM, with a question https://codereview.chromium.org/2634563002/diff/80001/android_webview/BUILD.gn File android_webview/BUILD.gn (right): https://codereview.chromium.org/2634563002/diff/80001/android_webview/BUILD.g... android_webview/BUILD.gn:31: # TODO(torne): uncomment this once Android is updated to support loading this On 2017/02/28 16:36:27, jbudorick wrote: > I think I'd prefer this be omitted until Android is updated w/ such support. > Until then, there's nothing to prevent this from bitrotting. Agreed, since there is internal target, once Android is updated, you will upstream internal target to here, and these commented code might already out of date. https://codereview.chromium.org/2634563002/diff/80001/android_webview/apk/jav... File android_webview/apk/java/AndroidManifest.xml (right): https://codereview.chromium.org/2634563002/diff/80001/android_webview/apk/jav... android_webview/apk/java/AndroidManifest.xml:29: {% if donor_package is not defined %} You might mention in somewhere else, what's the plan for LicenseActivity?
https://codereview.chromium.org/2634563002/diff/80001/android_webview/BUILD.gn File android_webview/BUILD.gn (right): https://codereview.chromium.org/2634563002/diff/80001/android_webview/BUILD.g... android_webview/BUILD.gn:31: # TODO(torne): uncomment this once Android is updated to support loading this On 2017/02/28 18:58:39, michaelbai wrote: > On 2017/02/28 16:36:27, jbudorick wrote: > > I think I'd prefer this be omitted until Android is updated w/ such support. > > Until then, there's nothing to prevent this from bitrotting. > > Agreed, since there is internal target, once Android is updated, you will > upstream internal target to here, and these commented code might already out of > date. No, we won't upstream the internal target - it has a different name and definition since it's the google-branded version. But, it's simple enough to redefine it when it's useful, so I'll remove it. https://codereview.chromium.org/2634563002/diff/80001/android_webview/apk/jav... File android_webview/apk/java/AndroidManifest.xml (right): https://codereview.chromium.org/2634563002/diff/80001/android_webview/apk/jav... android_webview/apk/java/AndroidManifest.xml:29: {% if donor_package is not defined %} On 2017/02/28 18:58:39, michaelbai wrote: > You might mention in somewhere else, what's the plan for LicenseActivity? I'm going to template it so that we can just compile it twice with different package names; I'll do this in a later CL. It's just two files and we never really change it, so having it get generated from a template is probably not too inconvenient. https://codereview.chromium.org/2634563002/diff/80001/build/config/android/ru... File build/config/android/rules.gni (right): https://codereview.chromium.org/2634563002/diff/80001/build/config/android/ru... build/config/android/rules.gni:1469: # is true. On 2017/02/28 16:36:27, jbudorick wrote: > nit: mention that the default is true for non-test APKs. Done.
The CQ bit was checked by torne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelbai@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/2634563002/#ps100001 (title: "Update comments, remove commented build target for now")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by torne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelbai@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/2634563002/#ps120001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488366377433460,
"parent_rev": "b4c90f556db7994f1ba708c10f0d8885938ea0bf", "commit_rev":
"cbafd406aff624f7191f76d64e3c0fcb66b60653"}
The CQ bit was unchecked by commit-bot@chromium.org
Prior attempt to commit was detected, but we were not able to check whether the issue was successfully committed. Please check Git history manually and re-check CQ or close this issue as needed.
The CQ bit was checked by torne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by torne@chromium.org
Hm, not sure what happened there, but the change did get committed...
Message was sent while issue was closed.
https://codereview.chromium.org/2634563002/diff/80001/android_webview/BUILD.gn File android_webview/BUILD.gn (right): https://codereview.chromium.org/2634563002/diff/80001/android_webview/BUILD.g... android_webview/BUILD.gn:31: # TODO(torne): uncomment this once Android is updated to support loading this On 2017/03/01 10:56:29, Torne wrote: > On 2017/02/28 18:58:39, michaelbai wrote: > > On 2017/02/28 16:36:27, jbudorick wrote: > > > I think I'd prefer this be omitted until Android is updated w/ such support. > > > Until then, there's nothing to prevent this from bitrotting. > > > > Agreed, since there is internal target, once Android is updated, you will > > upstream internal target to here, and these commented code might already out > of > > date. > > No, we won't upstream the internal target - it has a different name and > definition since it's the google-branded version. But, it's simple enough to > redefine it when it's useful, so I'll remove it. Thanks, I actually thought about the same thing, 'to have public version of internal target' |
