Skip to content

Commit

Permalink
Move ownership of AutocompleteController to Java.
Browse files Browse the repository at this point in the history
This change swaps the ownership model of the AutocompleteController,
enabling Java to own the C++ class. Moving forward, all instances of
AutocompleteControllers are created and managed by the
AutocompleteControllerProvider class, which warrants that instances
can be shared (where necessary) within, but not across, windows.

As a result, the AutocompleteController clients are decoupled,
permitting multiple Omniboxes across multiple Windows to operate
independently, while correctly reporting metrics.

AutocompleteControllerProvider lifecycle is bound directly to
WindowAndroid, and each created instance of the AutocompleteController
is bound to a pair: (WindowAndroid, Profile). When Window gets closed
the AutocompleteControllerProvider warrants proper cleanup of all of
the assoiciated AutocompleteControllers.

For exceptional cases, where Autocomplete subsystem is needed ahead of
any windows being open, a dedicated static method is introduced, that
warrants proper setup and teardown of the ephemeral instance of an
AutocompleteController.

ShortcutsProvider:
This change makes ShortcutsProvider keep the scoped reference to the
ShortcutsBackend it talks to. This backend is guaranteed to be the
correct one, because each AutocompleteProviderClient (passed only
once upon instantiation and retained by all Providers) is associated
with only one Profile.

This change solves a clean-up problem, where upon Profile being
destroyed, ShortcutsProvider attempts to retrieve its related Backend
and fails, because the Key (Profile) is no longer valid.

Discussion: go/decoupling-autocomplete-controllers-proposal
Change-Id: I2130d4158f0508bdff7b836ccd067fd426830503
Fixed: 1295054
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4199060
Commit-Queue: Tomasz Wiszkowski <ender@google.com>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1100670}
  • Loading branch information
tomasz-wiszkowski authored and Chromium LUCI CQ committed Feb 2, 2023
1 parent 4640177 commit 3b2eff7
Show file tree
Hide file tree
Showing 26 changed files with 621 additions and 342 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.gsa.GSAState;
import org.chromium.chrome.browser.offlinepages.OfflinePageUtils;
import org.chromium.chrome.browser.omnibox.suggestions.AutocompleteCoordinator;
import org.chromium.chrome.browser.omnibox.suggestions.AutocompleteControllerProvider;
import org.chromium.chrome.browser.profiles.Profile;
import org.chromium.chrome.browser.renderer_host.ChromeNavigationUIData;
import org.chromium.chrome.browser.search_engines.TemplateUrlServiceFactory;
Expand Down Expand Up @@ -742,8 +742,13 @@ static String getUrlFromVoiceSearchResult(Intent intent) {
return null;
}
String query = results.get(0);
AutocompleteMatch match =
AutocompleteCoordinator.classify(Profile.getLastUsedRegularProfile(), query);

AutocompleteMatch match;
try (var controller = AutocompleteControllerProvider.createCloseableController(
Profile.getLastUsedRegularProfile())) {
match = controller.get().classify(query, false);
}

if (!match.isSearchSuggestion()) return match.getUrl().getSpec();

List<String> urls = IntentUtils.safeGetStringArrayListExtra(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
import org.chromium.chrome.browser.native_page.ContextMenuManager;
import org.chromium.chrome.browser.omnibox.OmniboxFocusReason;
import org.chromium.chrome.browser.omnibox.OmniboxStub;
import org.chromium.chrome.browser.omnibox.suggestions.AutocompleteControllerProvider;
import org.chromium.chrome.browser.omnibox.voice.VoiceRecognitionHandler;
import org.chromium.chrome.browser.privacy.settings.PrivacyPreferencesManagerImpl;
import org.chromium.chrome.browser.profiles.Profile;
Expand Down Expand Up @@ -428,12 +429,14 @@ public void onPauseWithNative() {
// For example, the user changes theme when a NTP is showing, which leads to the recreation
// of the ChromeTabbedActivity and showing the NTP as the last visited Tab.
if (mTabModelSelector.isTabStateInitialized()) {
mayCreateSearchResumptionModule(profile);
mayCreateSearchResumptionModule(
profile, AutocompleteControllerProvider.from(windowAndroid));
} else {
mTabModelSelector.addObserver(new TabModelSelectorObserver() {
@Override
public void onTabStateInitialized() {
mayCreateSearchResumptionModule(profile);
mayCreateSearchResumptionModule(
profile, AutocompleteControllerProvider.from(windowAndroid));
mTabModelSelector.removeObserver(this);
}
});
Expand Down Expand Up @@ -1019,13 +1022,14 @@ private int getLogoMargin(boolean isTopMargin) {
R.dimen.ntp_logo_margin_bottom);
}

private void mayCreateSearchResumptionModule(Profile profile) {
private void mayCreateSearchResumptionModule(
Profile profile, AutocompleteControllerProvider provider) {
// The module is disabled on tablets.
if (mIsTablet) return;

mSearchResumptionModuleCoordinator =
SearchResumptionModuleUtils.mayCreateSearchResumptionModule(mNewTabPageLayout,
mTabModelSelector.getCurrentModel(), mTab, profile,
provider, mTabModelSelector.getCurrentModel(), mTab, profile,
R.id.search_resumption_module_container_stub);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;

import androidx.test.filters.MediumTest;

import org.junit.After;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
Expand All @@ -23,10 +23,9 @@

import org.chromium.base.test.util.CommandLineFlags;
import org.chromium.base.test.util.Feature;
import org.chromium.base.test.util.JniMocker;
import org.chromium.chrome.browser.flags.ChromeSwitches;
import org.chromium.chrome.browser.omnibox.suggestions.AutocompleteController;
import org.chromium.chrome.browser.omnibox.suggestions.AutocompleteControllerJni;
import org.chromium.chrome.browser.omnibox.suggestions.AutocompleteControllerProvider;
import org.chromium.chrome.browser.profiles.Profile;
import org.chromium.chrome.browser.profiles.ProfileManager;
import org.chromium.chrome.browser.tab.Tab;
Expand All @@ -45,9 +44,6 @@
public class IncognitoProfileDestroyerIntegrationTest {
@Rule
public ChromeTabbedActivityTestRule mActivityTestRule = new ChromeTabbedActivityTestRule();
@Rule
public JniMocker mJniMocker = new JniMocker();

private TabModel mIncognitoTabModel;

@Mock
Expand All @@ -56,15 +52,10 @@ public class IncognitoProfileDestroyerIntegrationTest {
@Mock
AutocompleteController mAutocompleteController;

@Mock
AutocompleteController.Natives mAutocompleteControllerJniMock;

@Before
public void setUp() throws InterruptedException {
MockitoAnnotations.initMocks(this);
mJniMocker.mock(AutocompleteControllerJni.TEST_HOOKS, mAutocompleteControllerJniMock);
doReturn(mAutocompleteController).when(mAutocompleteControllerJniMock).getForProfile(any());

AutocompleteControllerProvider.setControllerForTesting(mAutocompleteController);
mActivityTestRule.startMainActivityOnBlankPage();
TestThreadUtils.runOnUiThreadBlocking(() -> {
ProfileManager.addObserver(mMockProfileManagerObserver);
Expand All @@ -73,6 +64,11 @@ public void setUp() throws InterruptedException {
});
}

@After
public void tearDown() {
AutocompleteControllerProvider.setControllerForTesting(null);
}

@Test
@MediumTest
@Feature({"OffTheRecord"})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
import static org.mockito.Mockito.any;
import static org.mockito.Mockito.anyInt;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.endsWith;
import static org.mockito.Mockito.eq;
import static org.mockito.Mockito.never;
Expand All @@ -23,11 +22,11 @@
import androidx.recyclerview.widget.RecyclerView.LayoutManager;
import androidx.test.filters.MediumTest;

import org.junit.After;
import org.junit.Assert;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.ClassRule;
import org.junit.Rule;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.ArgumentCaptor;
Expand All @@ -38,14 +37,13 @@
import org.chromium.base.test.util.Batch;
import org.chromium.base.test.util.CommandLineFlags;
import org.chromium.base.test.util.CriteriaHelper;
import org.chromium.base.test.util.JniMocker;
import org.chromium.chrome.R;
import org.chromium.chrome.browser.ChromeTabbedActivity;
import org.chromium.chrome.browser.flags.ChromeSwitches;
import org.chromium.chrome.browser.omnibox.LocationBarLayout;
import org.chromium.chrome.browser.omnibox.OmniboxSuggestionType;
import org.chromium.chrome.browser.omnibox.suggestions.AutocompleteController;
import org.chromium.chrome.browser.omnibox.suggestions.AutocompleteControllerJni;
import org.chromium.chrome.browser.omnibox.suggestions.AutocompleteControllerProvider;
import org.chromium.chrome.browser.omnibox.suggestions.AutocompleteCoordinator;
import org.chromium.chrome.browser.omnibox.suggestions.OmniboxSuggestionUiType;
import org.chromium.chrome.browser.omnibox.suggestions.carousel.BaseCarouselSuggestionView;
Expand Down Expand Up @@ -90,18 +88,12 @@ public class MostVisitedTilesTest {
public static final ChromeTabbedActivityTestRule sActivityTestRule =
new ChromeTabbedActivityTestRule();

@Rule
public JniMocker mJniMocker = new JniMocker();

@Mock
private Profile mProfile;

@Mock
private AutocompleteController mController;

@Mock
private AutocompleteController.Natives mControllerJniMock;

@Captor
private ArgumentCaptor<AutocompleteController.OnSuggestionsReceivedListener> mListener;

Expand All @@ -128,8 +120,7 @@ public static void setUpClass() throws Exception {
@Before
public void setUp() throws Exception {
MockitoAnnotations.initMocks(this);
mJniMocker.mock(AutocompleteControllerJni.TEST_HOOKS, mControllerJniMock);
doReturn(mController).when(mControllerJniMock).getForProfile(any());
AutocompleteControllerProvider.setControllerForTesting(mController);
mActivity = sActivityTestRule.getActivity();
mOmnibox = new OmniboxTestUtils(mActivity);
mLocationBarLayout = mActivity.findViewById(R.id.location_bar);
Expand All @@ -155,6 +146,11 @@ public void setUp() throws Exception {
mCarousel = mOmnibox.getSuggestionByType(OmniboxSuggestionUiType.TILE_NAVSUGGEST);
}

@After
public void tearDown() {
AutocompleteControllerProvider.setControllerForTesting(null);
}

/**
* Initialize a small set of suggestions resembling what the user would see while visiting an
* URL.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,11 @@
import org.chromium.base.test.util.Batch;
import org.chromium.base.test.util.CommandLineFlags;
import org.chromium.base.test.util.Feature;
import org.chromium.base.test.util.JniMocker;
import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.flags.ChromeSwitches;
import org.chromium.chrome.browser.omnibox.suggestions.AutocompleteController;
import org.chromium.chrome.browser.omnibox.suggestions.AutocompleteControllerJni;
import org.chromium.chrome.browser.omnibox.suggestions.AutocompleteControllerProvider;
import org.chromium.chrome.browser.omnibox.suggestions.AutocompleteCoordinator;
import org.chromium.chrome.browser.omnibox.voice.VoiceRecognitionHandler.AssistantActionPerformed;
import org.chromium.chrome.browser.omnibox.voice.VoiceRecognitionHandler.TranslateBridgeWrapper;
import org.chromium.chrome.browser.omnibox.voice.VoiceRecognitionHandler.VoiceIntentTarget;
Expand Down Expand Up @@ -68,30 +68,27 @@ public class AssistantActionsHandlerTest {
public static ChromeTabbedActivityTestRule sActivityTestRule =
new ChromeTabbedActivityTestRule();
@Rule
public JniMocker mJniMocker = new JniMocker();
@Rule
public HistogramTestRule mHistograms = new HistogramTestRule();

@Mock
Intent mIntent;
private Intent mIntent;
@Mock
AssistantVoiceSearchService mAssistantVoiceSearchService;
private AssistantVoiceSearchService mAssistantVoiceSearchService;
@Mock
TranslateBridgeWrapper mTranslateBridgeWrapper;
private TranslateBridgeWrapper mTranslateBridgeWrapper;
@Mock
Tab mTab;
private Tab mTab;
@Mock
VoiceRecognitionHandler.Observer mObserver;
private VoiceRecognitionHandler.Observer mObserver;
@Mock
AutocompleteController mController;
private AutocompleteController mController;
@Mock
AutocompleteController.Natives mControllerJniMock;
private AutocompleteMatch mMatch;
@Mock
AutocompleteMatch mMatch;
private AutocompleteCoordinator mAutocompleteCoordinator;

private RecognitionTestHelper.TestDataProvider mDataProvider;
private RecognitionTestHelper.TestDelegate mDelegate;
private RecognitionTestHelper.TestAutocompleteCoordinator mAutocompleteCoordinator;
private RecognitionTestHelper.TestVoiceRecognitionHandler mHandler;
private RecognitionTestHelper.TestAndroidPermissionDelegate mPermissionDelegate;
private RecognitionTestHelper.TestWindowAndroid mWindowAndroid;
Expand All @@ -107,17 +104,16 @@ public static void setUpClass() {
@Before
public void setUp() throws InterruptedException, ExecutionException {
MockitoAnnotations.initMocks(this);
mJniMocker.mock(AutocompleteControllerJni.TEST_HOOKS, mControllerJniMock);
doReturn(mController).when(mControllerJniMock).getForProfile(any());
AutocompleteControllerProvider.setControllerForTesting(mController);
doReturn(mMatch).when(mController).classify(any(), anyBoolean());
doReturn(new GURL("https://www.google.com/search?q=abc")).when(mMatch).getUrl();
doReturn(true).when(mMatch).isSearchSuggestion();

TestThreadUtils.runOnUiThreadBlocking(() -> {
mProfileSupplier = new ObservableSupplierImpl<>();
RecognitionTestHelper testHelper =
new RecognitionTestHelper(mAssistantVoiceSearchService, mProfileSupplier,
sActivityTestRule.getActivity());
RecognitionTestHelper testHelper = new RecognitionTestHelper(mAutocompleteCoordinator,
mAssistantVoiceSearchService, mProfileSupplier,
sActivityTestRule.getActivity());
mDataProvider = testHelper.getDataProvider();
mDataProvider.setTab(mTab);
mPermissionDelegate = testHelper.getAndroidPermissionDelegate();
Expand All @@ -127,7 +123,6 @@ public void setUp() throws InterruptedException, ExecutionException {
mDelegate = testHelper.getDelegate();
mHandler = testHelper.getVoiceRecognitionHandler();
mHandler.addObserver(mObserver);
mAutocompleteCoordinator = testHelper.getAutocompleteCoordinator();
});

doReturn(new GURL(RecognitionTestHelper.DEFAULT_URL)).when(mTab).getUrl();
Expand Down Expand Up @@ -156,6 +151,7 @@ public void tearDown() {
VoiceRecognitionHandler.setIsRecognitionIntentPresentForTesting(null);
mWindowAndroid.destroy();
});
AutocompleteControllerProvider.setControllerForTesting(null);
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,10 @@
import org.chromium.base.test.util.Batch;
import org.chromium.base.test.util.CommandLineFlags;
import org.chromium.base.test.util.Feature;
import org.chromium.base.test.util.JniMocker;
import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.flags.ChromeSwitches;
import org.chromium.chrome.browser.omnibox.suggestions.AutocompleteController;
import org.chromium.chrome.browser.omnibox.suggestions.AutocompleteControllerJni;
import org.chromium.chrome.browser.omnibox.suggestions.AutocompleteControllerProvider;
import org.chromium.chrome.browser.omnibox.voice.VoiceRecognitionHandler.VoiceIntentTarget;
import org.chromium.chrome.browser.omnibox.voice.VoiceRecognitionHandler.VoiceInteractionSource;
import org.chromium.chrome.browser.profiles.Profile;
Expand All @@ -65,8 +64,6 @@ public class AssistantVoiceRecognitionHandlerTest {
public static ChromeTabbedActivityTestRule sActivityTestRule =
new ChromeTabbedActivityTestRule();
@Rule
public JniMocker mJniMocker = new JniMocker();
@Rule
public HistogramTestRule mHistograms = new HistogramTestRule();

@Mock
Expand All @@ -80,8 +77,6 @@ public class AssistantVoiceRecognitionHandlerTest {
@Mock
AutocompleteController mController;
@Mock
AutocompleteController.Natives mControllerJniMock;
@Mock
AutocompleteMatch mMatch;

private RecognitionTestHelper.TestDataProvider mDataProvider;
Expand All @@ -101,16 +96,16 @@ public static void setUpClass() {
@Before
public void setUp() throws InterruptedException, ExecutionException {
MockitoAnnotations.initMocks(this);
mJniMocker.mock(AutocompleteControllerJni.TEST_HOOKS, mControllerJniMock);
doReturn(mController).when(mControllerJniMock).getForProfile(any());

AutocompleteControllerProvider.setControllerForTesting(mController);
doReturn(mMatch).when(mController).classify(any(), anyBoolean());
doReturn(new GURL("https://www.google.com/search?q=abc")).when(mMatch).getUrl();
doReturn(true).when(mMatch).isSearchSuggestion();

TestThreadUtils.runOnUiThreadBlocking(() -> {
mProfileSupplier = new ObservableSupplierImpl<>();
RecognitionTestHelper testHelper =
new RecognitionTestHelper(mAssistantVoiceSearchService, mProfileSupplier,
new RecognitionTestHelper(null, mAssistantVoiceSearchService, mProfileSupplier,
sActivityTestRule.getActivity());
mDataProvider = testHelper.getDataProvider();
mDataProvider.setTab(mTab);
Expand Down Expand Up @@ -143,6 +138,7 @@ public void tearDown() {
VoiceRecognitionHandler.setIsRecognitionIntentPresentForTesting(null);
mWindowAndroid.destroy();
});
AutocompleteControllerProvider.setControllerForTesting(null);
}

@Test
Expand Down

0 comments on commit 3b2eff7

Please sign in to comment.