Commit e6f97cc5 authored by Piotr Bialecki's avatar Piotr Bialecki Committed by Commit Bot

Reland "Reland "Move away from using DFM to distribute AR-related code""

This is a reland of 3e21b750

Additional fix needed for a reland:
This change prevents the inclusion of ARCore-related entries to the
manifest when enable_arcore is false (for example for x86 builds).

Changes:
- include_arcore_manifest should be equal to enable_arcore on targets
  that have ARCore support (monochrome, trichrome, but *not*
  chrome_modern, chrome)
- trichrome.gni entry should not be needed since the variables for both
  monochrome and trichrome are set in
  monochrome_public_apk_or_module_tmpl

Validated with chrome APK, chrome_modern bundle, monochrome bundle and
trichrome_chrome bundle, both for arm and x86.

This change *must* land at the same time as https://crrev.com/i/2428538,
otherwise the push to Play Store will fail.

Original change's description:
> Reland "Move away from using DFM to distribute AR-related code"
>
> This is a reland of cd842897
>
> Original CL caused Chrome Canary to be rejected from Play Store.
> Play Store validates whether ARCore SDK's manifest entries are present
> only for applications that contain entry about ARCore being either
> required or optional.
>
> Additional changes needed for reland:
> - ArCoreShimImpl.java should not be included in java_sources - it will
>   be brought in by ar_java target on supported configurations since
>   it depends on ARCore SDK which is included only for specific targets
> - relaxed the change in arcore_consent_prompt.cc - the original change
>   would cause builds that are not supporting ARCore (by design) to
>   crash
>
> Original change's description:
> > Move away from using DFM to distribute AR-related code
> >
> > This change will make Chrome for Android stop using DFM to ship the
> > ARCore SDK. This is done to enable us to provide AR experiences for
> > apps using WebView. The changes are introduced in a way to enable us to
> > switch back to using AR DFM if need be.
> >
> > Changes summary:
> > - chrome_java target (defined in chrome/android/BUILD.gn) now also
> >   compiles ArCoreShimImpl.java (see java_sources.gni) - this means that
> >   java parts from ARCore SDK have to be added to its deps
> > - libarcore_sdk_c.so needs to be listed in loadable modules - this is
> >   done in monochrome_public_common_apk_or_module_tmpl (only if target
> >   is a base module when ARCore is enabled)
> > - AndroidManifest.xml no longer needs to contain the min_apk_version
> >   and InstallActivity entries explicitly - they will be brought in
> >   auto-magically by the build system and were needed only because
> >   manifest merging did not work for DFMs
> > - ArCoreInstallUtils (java) should never report that we can request
> >   installation of AR DFM
> > - ArCoreConsentPrompt (native) should never think it needs to request
> >   installation of AR DFM
> > - Added a DCHECK and graceful handling of null jstring for java_path
> >   (path to ARCore SDK .so) - it can be null if loadable_modules do not
> >   contain correct entry - with the DCHECK, debug builds will still
> >   crash but the release builds should just report that the WebXR
> >   specified session configuration (session mode == "immersive-ar") is
> >   not supported
> >
> > Binary-Size: Tracked in crbug/1040289
> > Bug: 1031623, 1040289
> > Change-Id: Ia2f305d1685f1c7207cc187d253f59a6331c36fc
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1988834
> > Commit-Queue: Andrew Grieve <agrieve@chromium.org>
> > Reviewed-by: Andrew Grieve <agrieve@chromium.org>
> > Reviewed-by: Alexander Cooper <alcooper@chromium.org>
> > Auto-Submit: Piotr Bialecki <bialpio@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#729818}
>
> Binary-Size: Tracked in crbug/1040289
> Bug: 1031623, 1040289
> Change-Id: I87eac0bc3748973e22190c6dba1187e822f83082
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1995769
> Reviewed-by: Ted Choc <tedchoc@chromium.org>
> Reviewed-by: Alexander Cooper <alcooper@chromium.org>
> Commit-Queue: Piotr Bialecki <bialpio@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#731011}

Binary-Size: Tracked in crbug/1040289
Bug: 1031623, 1040289
Change-Id: I90d7f6554026bf41c248a8d06f38963534efee28
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2003765Reviewed-by: default avatarAlexander Cooper <alcooper@chromium.org>
Reviewed-by: default avatarTed Choc <tedchoc@chromium.org>
Commit-Queue: Piotr Bialecki <bialpio@chromium.org>
Cr-Commit-Position: refs/heads/master@{#733413}
parent 2a6702f0
...@@ -451,6 +451,7 @@ android_library("chrome_java") { ...@@ -451,6 +451,7 @@ android_library("chrome_java") {
if (enable_vr) { if (enable_vr) {
deps += [ ":chrome_vr_java_resources" ] deps += [ ":chrome_vr_java_resources" ]
} }
srcjar_deps += [ ":chrome_vr_android_java_enums_srcjar" ] srcjar_deps += [ ":chrome_vr_android_java_enums_srcjar" ]
if (enable_feed_in_chrome) { if (enable_feed_in_chrome) {
deps += [ ":chrome_feed_java_resources" ] deps += [ ":chrome_feed_java_resources" ]
...@@ -1673,8 +1674,8 @@ template("monochrome_public_apk_or_module_tmpl") { ...@@ -1673,8 +1674,8 @@ template("monochrome_public_apk_or_module_tmpl") {
# of manually-instantiated manifests. # of manually-instantiated manifests.
jinja_template("${target_name}__android_manifest") { jinja_template("${target_name}__android_manifest") {
includes = [ "java/AndroidManifest.xml" ] includes = [ "java/AndroidManifest.xml" ]
variables = variables = chrome_public_jinja_variables +
chrome_public_jinja_variables + [ "include_arcore_manifest_flag=true" ] [ "include_arcore_manifest_flag=$enable_arcore" ]
if (_is_trichrome) { if (_is_trichrome) {
input = "java/AndroidManifest_trichrome_chrome.xml" input = "java/AndroidManifest_trichrome_chrome.xml"
variables += trichrome_jinja_variables variables += trichrome_jinja_variables
...@@ -1877,7 +1878,7 @@ jinja_template("monochrome_public_test_ar_apk_manifest") { ...@@ -1877,7 +1878,7 @@ jinja_template("monochrome_public_test_ar_apk_manifest") {
"target_sdk_version=$android_sdk_version", "target_sdk_version=$android_sdk_version",
"test_manifest_package=$chrome_public_test_manifest_package", "test_manifest_package=$chrome_public_test_manifest_package",
"webview_library=libmonochrome.so", "webview_library=libmonochrome.so",
"include_arcore_manifest_flag=true", "include_arcore_manifest_flag=$enable_arcore",
] ]
} }
......
...@@ -314,6 +314,12 @@ template("chrome_public_common_apk_or_module_tmpl") { ...@@ -314,6 +314,12 @@ template("chrome_public_common_apk_or_module_tmpl") {
template("monochrome_public_common_apk_or_module_tmpl") { template("monochrome_public_common_apk_or_module_tmpl") {
_is_bundle_module = defined(invoker.target_type) && _is_bundle_module = defined(invoker.target_type) &&
invoker.target_type == "android_app_bundle_module" invoker.target_type == "android_app_bundle_module"
if (_is_bundle_module) {
assert(
defined(invoker.is_base_module),
"_is_bundle_module is true but the invoker does not define is_base_module!")
}
if (defined(invoker.enable_multidex)) { if (defined(invoker.enable_multidex)) {
_enable_multidex = invoker.enable_multidex _enable_multidex = invoker.enable_multidex
} else { } else {
...@@ -341,6 +347,33 @@ template("monochrome_public_common_apk_or_module_tmpl") { ...@@ -341,6 +347,33 @@ template("monochrome_public_common_apk_or_module_tmpl") {
"//components/crash/android:handler_java", "//components/crash/android:handler_java",
] ]
if (_is_bundle_module && invoker.is_base_module && enable_arcore) {
# AR DFM is disabled - bring in the ar_java target along with ARCore SDK
# and set the loadable_modules / secondary_abi_loadable_modules to what
# would be brought in by the module.
# This needs to happen for monochrome & trichrome builds of base module if ARCore is enabled.
_deps += [
"//chrome/browser/android/vr:ar_java",
"//third_party/arcore-android-sdk-client:com_google_ar_core_java",
]
_libarcore_dir = get_label_info(
"//third_party/arcore-android-sdk-client:com_google_ar_core_java($default_toolchain)",
"target_out_dir") + "/com_google_ar_core_java/jni"
if (android_64bit_target_cpu) {
if (invoker.is_64_bit_browser) {
loadable_modules = [ "$_libarcore_dir/arm64-v8a/libarcore_sdk_c.so" ]
} else {
secondary_abi_loadable_modules =
[ "$_libarcore_dir/armeabi-v7a/libarcore_sdk_c.so" ]
}
} else {
loadable_modules = [ "$_libarcore_dir/armeabi-v7a/libarcore_sdk_c.so" ]
}
}
if (is_monochrome) { if (is_monochrome) {
product_config_java_packages = [ product_config_java_packages = [
"org.chromium.chrome.browser", "org.chromium.chrome.browser",
......
...@@ -175,21 +175,6 @@ by a child template that "extends" this file. ...@@ -175,21 +175,6 @@ by a child template that "extends" this file.
<!-- ARCore APK integration --> <!-- ARCore APK integration -->
<!-- This tag indicates that this application optionally uses ARCore. --> <!-- This tag indicates that this application optionally uses ARCore. -->
<meta-data android:name="com.google.ar.core" android:value="optional" /> <meta-data android:name="com.google.ar.core" android:value="optional" />
<!-- This value must match value present in ARCore SDK's .aar -->
<!-- TODO(https://crbug.com/917406): modify build scripts to
automatically pull this value in - the problem exists because in
bundle mode, the ARCore SDK is packaged into AR module and
the module installation will not automatically bring in DFM's
manifest entries. -->
<meta-data android:name="com.google.ar.core.min_apk_version"
android:value="190531000"/>
<activity
android:name="com.google.ar.core.InstallActivity"
android:configChanges="keyboardHidden|orientation|screenSize"
android:excludeFromRecents="true"
android:exported="false"
android:launchMode="singleTop"
android:theme="@android:style/Theme.Material.Light.Dialog.Alert" />
{% endif %} {% endif %}
<!-- Cast support --> <!-- Cast support -->
......
...@@ -6,7 +6,6 @@ package org.chromium.chrome.browser.vr; ...@@ -6,7 +6,6 @@ package org.chromium.chrome.browser.vr;
import android.app.Activity; import android.app.Activity;
import org.chromium.base.BundleUtils;
import org.chromium.base.ContextUtils; import org.chromium.base.ContextUtils;
import org.chromium.base.Log; import org.chromium.base.Log;
import org.chromium.base.annotations.CalledByNative; import org.chromium.base.annotations.CalledByNative;
...@@ -82,7 +81,8 @@ public class ArCoreInstallUtils implements ModuleInstallUi.FailureUiListener { ...@@ -82,7 +81,8 @@ public class ArCoreInstallUtils implements ModuleInstallUi.FailureUiListener {
@CalledByNative @CalledByNative
private boolean canRequestInstallArModule() { private boolean canRequestInstallArModule() {
// We can only try to install the AR module if we are in a bundle mode. // We can only try to install the AR module if we are in a bundle mode.
return BundleUtils.isBundle(); // Currently, AR DFM is disabled so we should never have to install it.
return false;
} }
@CalledByNative @CalledByNative
......
...@@ -60,7 +60,7 @@ if (enable_image_editor) { ...@@ -60,7 +60,7 @@ if (enable_image_editor) {
# Modules shipped in Monochrome (Android N+). # Modules shipped in Monochrome (Android N+).
monochrome_module_descs = chrome_modern_module_descs monochrome_module_descs = chrome_modern_module_descs
if (enable_arcore) { if (false) { # AR DFM is currently disabled
monochrome_module_descs += [ ar_module_desc ] monochrome_module_descs += [ ar_module_desc ]
} }
if (!disable_autofill_assistant_dfm) { if (!disable_autofill_assistant_dfm) {
......
...@@ -33,7 +33,6 @@ trichrome_jinja_variables = [ ...@@ -33,7 +33,6 @@ trichrome_jinja_variables = [
"trichrome_library=$trichrome_library_package", "trichrome_library=$trichrome_library_package",
"trichrome_certdigest=$trichrome_certdigest", "trichrome_certdigest=$trichrome_certdigest",
"use32bitAbi=android:use32bitAbi=\"true\"", "use32bitAbi=android:use32bitAbi=\"true\"",
"include_arcore_manifest_flag=true",
] ]
trichrome_synchronized_proguard = trichrome_synchronized_proguard =
......
...@@ -66,9 +66,8 @@ ArCoreConsentPrompt::ArCoreConsentPrompt() : weak_ptr_factory_(this) {} ...@@ -66,9 +66,8 @@ ArCoreConsentPrompt::ArCoreConsentPrompt() : weak_ptr_factory_(this) {}
ArCoreConsentPrompt::~ArCoreConsentPrompt() = default; ArCoreConsentPrompt::~ArCoreConsentPrompt() = default;
void ArCoreConsentPrompt::OnUserConsentResult( void ArCoreConsentPrompt::OnUserConsentResult(JNIEnv* env,
JNIEnv* env, jboolean is_granted) {
jboolean is_granted) {
jdelegate_.Reset(); jdelegate_.Reset();
if (!on_user_consent_callback_) if (!on_user_consent_callback_)
...@@ -139,9 +138,8 @@ void ArCoreConsentPrompt::RequestInstallSupportedArCore() { ...@@ -139,9 +138,8 @@ void ArCoreConsentPrompt::RequestInstallSupportedArCore() {
GetTabFromRenderer(render_process_id_, render_frame_id_)); GetTabFromRenderer(render_process_id_, render_frame_id_));
} }
void ArCoreConsentPrompt::OnRequestInstallArModuleResult( void ArCoreConsentPrompt::OnRequestInstallArModuleResult(JNIEnv* env,
JNIEnv* env, bool success) {
bool success) {
DVLOG(1) << __func__; DVLOG(1) << __func__;
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
...@@ -150,9 +148,8 @@ void ArCoreConsentPrompt::OnRequestInstallArModuleResult( ...@@ -150,9 +148,8 @@ void ArCoreConsentPrompt::OnRequestInstallArModuleResult(
} }
} }
void ArCoreConsentPrompt::OnRequestInstallSupportedArCoreResult( void ArCoreConsentPrompt::OnRequestInstallSupportedArCoreResult(JNIEnv* env,
JNIEnv* env, bool success) {
bool success) {
DVLOG(1) << __func__; DVLOG(1) << __func__;
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
DCHECK(on_request_arcore_install_or_update_result_callback_); DCHECK(on_request_arcore_install_or_update_result_callback_);
...@@ -162,15 +159,14 @@ void ArCoreConsentPrompt::OnRequestInstallSupportedArCoreResult( ...@@ -162,15 +159,14 @@ void ArCoreConsentPrompt::OnRequestInstallSupportedArCoreResult(
void ArCoreConsentPrompt::RequestArModule() { void ArCoreConsentPrompt::RequestArModule() {
DVLOG(1) << __func__; DVLOG(1) << __func__;
if (ShouldRequestInstallArModule()) { if (ShouldRequestInstallArModule()) {
if (!CanRequestInstallArModule()) { // AR DFM is disabled - if we think we should install AR module, then it
OnRequestArModuleResult(false); // means that we are using a build that does not support AR capabilities.
return; // Treat this as if the AR module installation failed.
} LOG(WARNING) << "AR is not supported on this build";
on_request_ar_module_result_callback_ = base::BindOnce( OnRequestArModuleResult(false);
&ArCoreConsentPrompt::OnRequestArModuleResult, GetWeakPtr());
RequestInstallArModule();
return; return;
} }
......
...@@ -19,10 +19,6 @@ using base::android::ScopedJavaLocalRef; ...@@ -19,10 +19,6 @@ using base::android::ScopedJavaLocalRef;
namespace vr { namespace vr {
namespace {
} // namespace
ArCoreJavaUtils::ArCoreJavaUtils() { ArCoreJavaUtils::ArCoreJavaUtils() {
JNIEnv* env = AttachCurrentThread(); JNIEnv* env = AttachCurrentThread();
if (!env) if (!env)
...@@ -109,6 +105,21 @@ bool ArCoreJavaUtils::EnsureLoaded() { ...@@ -109,6 +105,21 @@ bool ArCoreJavaUtils::EnsureLoaded() {
// absolute path. // absolute path.
ScopedJavaLocalRef<jstring> java_path = ScopedJavaLocalRef<jstring> java_path =
Java_ArCoreJavaUtils_getArCoreShimLibraryPath(env); Java_ArCoreJavaUtils_getArCoreShimLibraryPath(env);
// Crash in debug builds if `java_path` is a null pointer but handle this
// situation in release builds. This is done by design - the `java_path` will
// be null only if there was a regression introduced to our gn/gni files w/o
// causing a build break. In release builds, this approach will result in the
// site not being able to request an AR session.
DCHECK(java_path)
<< "Unable to find path to ARCore SDK library - please ensure that "
"loadable_modules and secondary_abi_loadable_modules are set "
"correctly when building";
if (!java_path) {
LOG(ERROR) << "Unable to find path to ARCore SDK library";
return false;
}
return LoadArCoreSdk(base::android::ConvertJavaStringToUTF8(env, java_path)); return LoadArCoreSdk(base::android::ConvertJavaStringToUTF8(env, java_path));
} }
......
...@@ -243,6 +243,7 @@ void XR::PendingSupportsSessionQuery::Resolve(bool supported, ...@@ -243,6 +243,7 @@ void XR::PendingSupportsSessionQuery::Resolve(bool supported,
if (supported) { if (supported) {
resolver_->Resolve(); resolver_->Resolve();
} else { } else {
DVLOG(2) << __func__ << ": session is unsupported - throwing exception";
RejectWithDOMException(DOMExceptionCode::kNotSupportedError, RejectWithDOMException(DOMExceptionCode::kNotSupportedError,
kSessionNotSupported, exception_state); kSessionNotSupported, exception_state);
} }
...@@ -742,6 +743,9 @@ ScriptPromise XR::InternalIsSessionSupported(ScriptState* script_state, ...@@ -742,6 +743,9 @@ ScriptPromise XR::InternalIsSessionSupported(ScriptState* script_state,
if (session_mode == device::mojom::blink::XRSessionMode::kImmersiveAr && if (session_mode == device::mojom::blink::XRSessionMode::kImmersiveAr &&
!RuntimeEnabledFeatures::WebXRARModuleEnabled(doc)) { !RuntimeEnabledFeatures::WebXRARModuleEnabled(doc)) {
DVLOG(2) << __func__
<< ": Immersive AR session is only supported if WebXRARModule "
"feature is enabled";
query->Resolve(false); query->Resolve(false);
return promise; return promise;
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment