Skip to content

Commit

Permalink
Android: Refactor Module Installer for Testability
Browse files Browse the repository at this point in the history
The intrinsic dependencies encountered along the way made it difficult
(less effective) to break up this CL into multiple smaller CLs
(apologies to the reviewers in advance).

To simplify its understanding, please refer to the following doc:
https://docs.google.com/document/d/1ClZglFQroV53zSYEGIZ7yca8KIsPHGBmzMCbtreu5lg

The following are the major points to review:
- refactoring of the infra into builder, installer, logger, observer, util
- new design (IoC driven) for installers to enable for easy testability
- new design for emitted modules (cross-package communication)

Out-of-scope (following CLs):
- removal of bytecode processing for third-party activities (no longer needed)
- move code from ModuleInstallerConfig.java into BuildConfig.java
- unit tests for the remaining module_installer classes

Testing: this change was verified with vr, ar, autofill_assistant, dev_ui,
and test_dummy modules. It was tested with both -m and -f command line args.

Bug: 1005802
Change-Id: Icf357c06c4b71a96ed9fa0584f6322d6dc6143d7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1813520
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Tibor Goldschwendt <tiborg@chromium.org>
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Reviewed-by: Peter Wen <wnwen@chromium.org>
Commit-Queue: Fred Mello <fredmello@chromium.org>
Cr-Commit-Position: refs/heads/master@{#702165}
  • Loading branch information
Fred Mello authored and Commit Bot committed Oct 2, 2019
1 parent 06d698e commit 2623e05
Show file tree
Hide file tree
Showing 53 changed files with 1,621 additions and 973 deletions.
16 changes: 7 additions & 9 deletions chrome/android/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -1064,15 +1064,13 @@ if (enable_vr || enable_arcore) {
"javatests/src/org/chromium/chrome/browser/vr/rules/VrModuleNotInstalled.java",
]

deps =
chrome_test_xr_java_deps + [
"//chrome/android:chrome_test_xr_java",
"//third_party/gvr-android-sdk:controller_test_api_java",
"//third_party/gvr-android-sdk:gvr_common_java",
":chrome_test_util_java",
"//components/module_installer/android:module_installer_java",
"//components/module_installer/android:module_installer_test_java",
]
deps = chrome_test_xr_java_deps + [
"//chrome/android:chrome_test_xr_java",
"//third_party/gvr-android-sdk:controller_test_api_java",
"//third_party/gvr-android-sdk:gvr_common_java",
":chrome_test_util_java",
"//components/module_installer/android:module_installer_java",
]

data = [
"//chrome/android/shared_preference_files/test/",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
import org.chromium.chrome.browser.tab.Tab;
import org.chromium.chrome.browser.widget.ScrimView;
import org.chromium.chrome.browser.widget.bottomsheet.BottomSheetController;
import org.chromium.components.module_installer.ModuleInterface;
import org.chromium.components.module_installer.builder.ModuleInterface;
import org.chromium.content_public.browser.WebContents;

import java.util.Map;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
import org.chromium.chrome.browser.autofill_assistant.metrics.FeatureModuleInstallation;
import org.chromium.chrome.browser.modules.ModuleInstallUi;
import org.chromium.chrome.browser.tab.Tab;
import org.chromium.components.module_installer.ModuleInstaller;

/**
* Manages the loading of autofill assistant DFM, and provides implementation of
Expand Down Expand Up @@ -104,7 +103,7 @@ public void onFailureUiResponse(boolean retry) {
});
// Shows toast informing user about install start.
ui.showInstallStartUi();
ModuleInstaller.getInstance().install("autofill_assistant", (success) -> {
AutofillAssistantModule.install((success) -> {
if (success) {
// Don't show success UI from DFM, transition to autobot UI directly.
AutofillAssistantMetrics.recordFeatureModuleInstallation(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

package org.chromium.chrome.features.dev_ui;

import org.chromium.components.module_installer.ModuleInterface;
import org.chromium.components.module_installer.builder.ModuleInterface;

/** Interface to call into DevUI feature. */
@ModuleInterface(module = "dev_ui", impl = "org.chromium.chrome.features.dev_ui.DevUiImpl")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
import org.chromium.chrome.browser.tasks.TasksSurface;
import org.chromium.chrome.browser.tasks.tab_groups.TabGroupModelFilter;
import org.chromium.chrome.features.start_surface.StartSurface;
import org.chromium.components.module_installer.ModuleInterface;
import org.chromium.components.module_installer.builder.ModuleInterface;
import org.chromium.ui.modelutil.PropertyModel;

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

package org.chromium.chrome.browser.vr;

import org.chromium.components.module_installer.ModuleInterface;
import org.chromium.components.module_installer.builder.ModuleInterface;

/** Provides delegate interfaces that can be used to call into VR. */
@ModuleInterface(module = "vr", impl = "org.chromium.chrome.browser.vr.VrDelegateProviderImpl")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
import org.chromium.chrome.R;
import org.chromium.chrome.browser.modules.ModuleInstallUi;
import org.chromium.chrome.browser.tab.Tab;
import org.chromium.components.module_installer.OnModuleInstallFinishedListener;
import org.chromium.components.module_installer.engine.InstallListener;

import java.util.ArrayList;
import java.util.List;
Expand Down Expand Up @@ -88,7 +88,7 @@ public static void onExitVr() {
for (VrModeObserver observer : sVrModeObservers) observer.onExitVr();
}

/* package */ static void installModule(OnModuleInstallFinishedListener onFinishedListener) {
/* package */ static void installModule(InstallListener listener) {
VrModule.install((success) -> {
if (success) {
// Re-create delegate provider.
Expand All @@ -97,7 +97,7 @@ public static void onExitVr() {
assert !(delegate instanceof VrDelegateFallback);
delegate.initAfterModuleInstall();
}
onFinishedListener.onFinished(success);
listener.onComplete(success);
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@
import org.chromium.components.bookmarks.BookmarkId;
import org.chromium.components.feature_engagement.EventConstants;
import org.chromium.components.feature_engagement.Tracker;
import org.chromium.components.module_installer.Module;
import org.chromium.components.module_installer.builder.Module;
import org.chromium.content_public.browser.LoadUrlParams;
import org.chromium.content_public.browser.SelectionPopupController;
import org.chromium.content_public.browser.WebContents;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@
import org.chromium.chrome.browser.vr.OnExitVrRequestListener;
import org.chromium.chrome.browser.vr.VrModuleProvider;
import org.chromium.components.embedder_support.application.FontPreloadingWorkaround;
import org.chromium.components.module_installer.ModuleInstaller;
import org.chromium.components.module_installer.util.ModuleUtil;
import org.chromium.ui.base.ResourceBundle;

/**
Expand Down Expand Up @@ -121,15 +121,15 @@ protected void attachBaseContext(Context context) {

// Record via UMA all modules that have been requested and are currently installed. This
// will tell us the install penetration of each module over time.
ModuleInstaller.getInstance().recordModuleAvailability();
ModuleUtil.recordModuleAvailability();

// Set Chrome factory for mapping BackgroundTask classes to TaskIds.
ChromeBackgroundTaskFactory.setAsDefault();
}

// Write installed modules to crash keys. This needs to be done as early as possible so that
// these values are set before any crashes are reported.
ModuleInstaller.getInstance().updateCrashKeys();
ModuleUtil.updateCrashKeys();

BuildInfo.setFirebaseAppId(FirebaseConfig.getFirebaseAppId());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,7 @@
import org.chromium.components.background_task_scheduler.BackgroundTaskSchedulerPrefs;
import org.chromium.components.crash.browser.ChildProcessCrashObserver;
import org.chromium.components.minidump_uploader.CrashFileManager;
import org.chromium.components.module_installer.ModuleInstaller;
import org.chromium.components.module_installer.observers.ModuleActivityObserver;
import org.chromium.components.module_installer.util.ModuleUtil;
import org.chromium.content_public.browser.BrowserStartupController;
import org.chromium.content_public.browser.DeviceUtils;
import org.chromium.content_public.browser.SpeechRecognition;
Expand Down Expand Up @@ -237,7 +236,6 @@ private void preInflationStartup() {

DeviceUtils.addDeviceSpecificUserAgentSwitch();
ApplicationStatus.registerStateListenerForAllActivities(createActivityStateListener());
ApplicationStatus.registerStateListenerForAllActivities(new ModuleActivityObserver());

mPreInflationStartupComplete = true;
}
Expand Down Expand Up @@ -440,7 +438,7 @@ public void childCrashed(int pid) {
// Needed for field trial metrics to be properly collected in ServiceManager only mode.
FeatureUtilities.cacheNativeFlagsForServiceManagerOnlyMode();

ModuleInstaller.getInstance().recordStartupTime();
ModuleUtil.recordStartupTime();
}

private ActivityStateListener createActivityStateListener() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@
import org.chromium.chrome.browser.infobar.SimpleConfirmInfoBarBuilder;
import org.chromium.chrome.browser.modules.ModuleInstallUi;
import org.chromium.chrome.browser.tab.Tab;
import org.chromium.components.module_installer.ModuleInstaller;
import org.chromium.components.module_installer.engine.EngineFactory;
import org.chromium.components.module_installer.engine.InstallEngine;

/**
* Installs AR DFM and ArCore runtimes.
Expand Down Expand Up @@ -64,8 +65,6 @@ private static ArCoreInstallUtils create(long nativeArCoreInstallUtils) {

private ArCoreInstallUtils(long nativeArCoreInstallUtils) {
mNativeArCoreInstallUtils = nativeArCoreInstallUtils;
// Need to be called before trying to access the AR module.
ModuleInstaller.getInstance().init();
}

@Override
Expand Down Expand Up @@ -99,9 +98,13 @@ private boolean shouldRequestInstallArModule() {
@CalledByNative
private void requestInstallArModule(Tab tab) {
mTab = tab;

ModuleInstallUi ui = new ModuleInstallUi(mTab, R.string.ar_module_title, this);
InstallEngine installEngine = new EngineFactory().getEngine();

ui.showInstallStartUi();
ModuleInstaller.getInstance().install("ar", success -> {

installEngine.install("ar", success -> {
assert shouldRequestInstallArModule() != success;

if (success) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import org.junit.Assert;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.ExternalResource;
import org.junit.rules.RuleChain;
import org.junit.runner.RunWith;

Expand All @@ -28,8 +29,7 @@
import org.chromium.chrome.browser.vr.util.VrTestRuleUtils;
import org.chromium.chrome.test.ChromeActivityTestRule;
import org.chromium.chrome.test.ChromeJUnit4RunnerDelegate;
import org.chromium.components.module_installer.ModuleInstaller;
import org.chromium.components.module_installer.ModuleInstallerRule;
import org.chromium.components.module_installer.engine.InstallEngine;

import java.util.HashSet;
import java.util.List;
Expand All @@ -53,24 +53,13 @@ public class VrDaydreamReadyModuleInstallTest {
@Rule
public RuleChain mRuleChain;

private ModuleInstallerRule mModuleInstallerRule;

private ChromeActivityTestRule mVrTestRule;

private final Set<String> mModulesRequestedDeferred = new HashSet<>();

public VrDaydreamReadyModuleInstallTest(Callable<ChromeActivityTestRule> callable)
throws Exception {
mVrTestRule = callable.call();
mModuleInstallerRule = new ModuleInstallerRule(new ModuleInstaller() {
@Override
public void installDeferred(String moduleName) {
mModulesRequestedDeferred.add(moduleName);
}
});
mRuleChain =
RuleChain.outerRule(mModuleInstallerRule)
.around(VrTestRuleUtils.wrapRuleInActivityRestrictionRule(mVrTestRule));
RuleChain.outerRule(new VrModuleInstallerRule())
.around(VrTestRuleUtils.wrapRuleInActivityRestrictionRule(callable.call()));
}

/** Tests that the install is requested deferred. */
Expand All @@ -83,4 +72,29 @@ public void testDeferredRequestOnStartup() {
Assert.assertTrue("VR module should have been deferred installed at startup",
mModulesRequestedDeferred.contains("vr"));
}

private class VrModuleInstallerRule extends ExternalResource {
private final InstallEngine mOldModuleInstaller;
private final InstallEngine mStubModuleInstaller;

public VrModuleInstallerRule() {
mStubModuleInstaller = new InstallEngine() {
@Override
public void installDeferred(String moduleName) {
mModulesRequestedDeferred.add(moduleName);
}
};
mOldModuleInstaller = VrModule.getInstallEngine();
}

@Override
protected void before() {
VrModule.setInstallEngine(mStubModuleInstaller);
}

@Override
protected void after() {
VrModule.setInstallEngine(mOldModuleInstaller);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,8 @@
import org.chromium.chrome.browser.vr.rules.ChromeTabbedActivityVrTestRule;
import org.chromium.chrome.browser.vr.rules.CustomTabActivityVrTestRule;
import org.chromium.chrome.browser.vr.rules.VrActivityRestrictionRule;
import org.chromium.chrome.browser.vr.rules.VrModuleNotInstalled;
import org.chromium.chrome.browser.vr.rules.VrTestRule;
import org.chromium.chrome.browser.vr.rules.WebappActivityVrTestRule;
import org.chromium.components.module_installer.Module;

import java.util.ArrayList;
import java.util.concurrent.Callable;
Expand Down Expand Up @@ -59,10 +57,6 @@ public interface ChromeLaunchMethod { public void launch() throws Throwable; }
*/
public static void evaluateVrTestRuleImpl(final Statement base, final Description desc,
final VrTestRule rule, final ChromeLaunchMethod launcher) throws Throwable {
// Should be called before any other VR methods get called.
if (desc.getAnnotation(VrModuleNotInstalled.class) != null) {
Module.setForceUninstalled("vr");
}
TestVrShellDelegate.setDescription(desc);

VrTestRuleUtils.ensureNoVrActivitiesDisplayed();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

package org.chromium.chrome.modules.extra_icu;

import org.chromium.components.module_installer.ModuleInterface;
import org.chromium.components.module_installer.builder.ModuleInterface;

/** Interface into the extra ICU module. Only used to check whether module is installed. */
@ModuleInterface(module = "extra_icu", impl = "org.chromium.chrome.modules.extra_icu.ExtraIcuImpl")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
package org.chromium.chrome.modules.test_dummy;

import org.chromium.chrome.features.test_dummy.TestDummy;
import org.chromium.components.module_installer.ModuleInterface;
import org.chromium.components.module_installer.builder.ModuleInterface;

/** Provides the test dummy implementation. */
@ModuleInterface(module = "test_dummy",
Expand Down
Loading

0 comments on commit 2623e05

Please sign in to comment.