Commit 34bcb6f3 authored by Min Qin's avatar Min Qin Committed by Commit Bot

Fix an issue that Omnibox.SuggestionStarted.FromTile is not reported

correctly

This issue happens when user clicking a tile on omnibox, and then start
editing the text.
Because the focus change is not caused by QT, so further text change
will considered that the suggestion is not started by QT.
This CL fixes the issue by adding a variable to store that the
suggestion is triggered by clicking on a tile.

BUG=1147145

Change-Id: I641396b4c7c52bafcc6d34edb860f57f57e9e425
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2527008
Commit-Queue: Min Qin <qinmin@chromium.org>
Reviewed-by: default avatarTomasz Wiszkowski <ender@google.com>
Reviewed-by: default avatarTed Choc <tedchoc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#826953}
parent 499b2463
...@@ -110,12 +110,21 @@ class AutocompleteMediator implements OnSuggestionsReceivedListener, StartStopWi ...@@ -110,12 +110,21 @@ class AutocompleteMediator implements OnSuggestionsReceivedListener, StartStopWi
@SuggestionVisibilityState @SuggestionVisibilityState
private int mSuggestionVisibilityState; private int mSuggestionVisibilityState;
@IntDef({EditSessionState.INACTIVE, EditSessionState.ACTIVATED_BY_USER_INPUT,
EditSessionState.ACTIVATED_BY_QUERY_TILE})
@Retention(RetentionPolicy.SOURCE)
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
@interface EditSessionState {
int INACTIVE = 0; // Omnibox is not being edited.
int ACTIVATED_BY_USER_INPUT = 1; // The edit session is triggered by user input.
int ACTIVATED_BY_QUERY_TILE = 2; // The edit session is triggered from query tile.
}
@EditSessionState
private int mEditSessionState;
// The timestamp (using SystemClock.elapsedRealtime()) at the point when the user started // The timestamp (using SystemClock.elapsedRealtime()) at the point when the user started
// modifying the omnibox with new input. // modifying the omnibox with new input.
private long mNewOmniboxEditSessionTimestamp = -1; private long mNewOmniboxEditSessionTimestamp = -1;
// Set to true when the user has started typing new input in the omnibox, set to false
// when the omnibox loses focus or becomes empty.
private boolean mHasStartedNewOmniboxEditSession;
// Set at the end of the Omnibox interaction to indicate whether the user selected an item // Set at the end of the Omnibox interaction to indicate whether the user selected an item
// from the list (true) or left the Omnibox and suggestions list with no action taken (false). // from the list (true) or left the Omnibox and suggestions list with no action taken (false).
private boolean mOmniboxFocusResultedInNavigation; private boolean mOmniboxFocusResultedInNavigation;
...@@ -382,7 +391,7 @@ class AutocompleteMediator implements OnSuggestionsReceivedListener, StartStopWi ...@@ -382,7 +391,7 @@ class AutocompleteMediator implements OnSuggestionsReceivedListener, StartStopWi
SuggestionsMetrics.recordOmniboxFocusResultedInNavigation( SuggestionsMetrics.recordOmniboxFocusResultedInNavigation(
mOmniboxFocusResultedInNavigation); mOmniboxFocusResultedInNavigation);
setSuggestionVisibilityState(SuggestionVisibilityState.DISALLOWED); setSuggestionVisibilityState(SuggestionVisibilityState.DISALLOWED);
mHasStartedNewOmniboxEditSession = false; mEditSessionState = EditSessionState.INACTIVE;
mNewOmniboxEditSessionTimestamp = -1; mNewOmniboxEditSessionTimestamp = -1;
// Prevent any upcoming omnibox suggestions from showing once a URL is loaded (and as // Prevent any upcoming omnibox suggestions from showing once a URL is loaded (and as
// a consequence the omnibox is unfocused). // a consequence the omnibox is unfocused).
...@@ -449,7 +458,7 @@ class AutocompleteMediator implements OnSuggestionsReceivedListener, StartStopWi ...@@ -449,7 +458,7 @@ class AutocompleteMediator implements OnSuggestionsReceivedListener, StartStopWi
mDelegate.setOmniboxEditingText(refineText); mDelegate.setOmniboxEditingText(refineText);
mNewOmniboxEditSessionTimestamp = SystemClock.elapsedRealtime(); mNewOmniboxEditSessionTimestamp = SystemClock.elapsedRealtime();
mHasStartedNewOmniboxEditSession = true; mEditSessionState = EditSessionState.ACTIVATED_BY_QUERY_TILE;
mAutocomplete.start(mDataProvider.getProfile(), mDataProvider.getCurrentUrl(), mAutocomplete.start(mDataProvider.getProfile(), mDataProvider.getCurrentUrl(),
mDataProvider.getPageClassification(mDelegate.didFocusUrlFromFakebox()), mDataProvider.getPageClassification(mDelegate.didFocusUrlFromFakebox()),
...@@ -692,10 +701,10 @@ class AutocompleteMediator implements OnSuggestionsReceivedListener, StartStopWi ...@@ -692,10 +701,10 @@ class AutocompleteMediator implements OnSuggestionsReceivedListener, StartStopWi
mIgnoreOmniboxItemSelection = true; mIgnoreOmniboxItemSelection = true;
cancelPendingAutocompleteStart(); cancelPendingAutocompleteStart();
if (!mHasStartedNewOmniboxEditSession && mNativeInitialized) { if (mEditSessionState == EditSessionState.INACTIVE && mNativeInitialized) {
mAutocomplete.resetSession(); mAutocomplete.resetSession();
mNewOmniboxEditSessionTimestamp = SystemClock.elapsedRealtime(); mNewOmniboxEditSessionTimestamp = SystemClock.elapsedRealtime();
mHasStartedNewOmniboxEditSession = true; mEditSessionState = EditSessionState.ACTIVATED_BY_USER_INPUT;
} }
stopAutocomplete(false); stopAutocomplete(false);
...@@ -731,7 +740,8 @@ class AutocompleteMediator implements OnSuggestionsReceivedListener, StartStopWi ...@@ -731,7 +740,8 @@ class AutocompleteMediator implements OnSuggestionsReceivedListener, StartStopWi
mDataProvider.getPageClassification(mDelegate.didFocusUrlFromFakebox()); mDataProvider.getPageClassification(mDelegate.didFocusUrlFromFakebox());
mAutocomplete.start(profile, mDataProvider.getCurrentUrl(), pageClassification, mAutocomplete.start(profile, mDataProvider.getCurrentUrl(), pageClassification,
textWithoutAutocomplete, cursorPosition, preventAutocomplete, null, textWithoutAutocomplete, cursorPosition, preventAutocomplete, null,
mDelegate.didFocusUrlFromQueryTiles()); mDelegate.didFocusUrlFromQueryTiles()
|| mEditSessionState == EditSessionState.ACTIVATED_BY_QUERY_TILE);
}; };
if (mNativeInitialized) { if (mNativeInitialized) {
mHandler.postDelayed(mRequestSuggestions, OMNIBOX_SUGGESTION_START_DELAY_MS); mHandler.postDelayed(mRequestSuggestions, OMNIBOX_SUGGESTION_START_DELAY_MS);
...@@ -901,7 +911,7 @@ class AutocompleteMediator implements OnSuggestionsReceivedListener, StartStopWi ...@@ -901,7 +911,7 @@ class AutocompleteMediator implements OnSuggestionsReceivedListener, StartStopWi
private void startZeroSuggest() { private void startZeroSuggest() {
// Reset "edited" state in the omnibox if zero suggest is triggered -- new edits // Reset "edited" state in the omnibox if zero suggest is triggered -- new edits
// now count as a new session. // now count as a new session.
mHasStartedNewOmniboxEditSession = false; mEditSessionState = EditSessionState.INACTIVE;
mNewOmniboxEditSessionTimestamp = -1; mNewOmniboxEditSessionTimestamp = -1;
if (mNativeInitialized && mDelegate.isUrlBarFocused() if (mNativeInitialized && mDelegate.isUrlBarFocused()
...@@ -1113,4 +1123,10 @@ class AutocompleteMediator implements OnSuggestionsReceivedListener, StartStopWi ...@@ -1113,4 +1123,10 @@ class AutocompleteMediator implements OnSuggestionsReceivedListener, StartStopWi
recordMetrics(position, WindowOpenDisposition.CURRENT_TAB, suggestion); recordMetrics(position, WindowOpenDisposition.CURRENT_TAB, suggestion);
mDelegate.loadUrl(updatedUrl.getSpec(), PageTransition.LINK, mLastActionUpTimestamp); mDelegate.loadUrl(updatedUrl.getSpec(), PageTransition.LINK, mLastActionUpTimestamp);
} }
@VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
@EditSessionState
int getEditSessionStateForTest() {
return mEditSessionState;
}
} }
...@@ -45,17 +45,21 @@ import org.chromium.chrome.browser.flags.ChromeFeatureList; ...@@ -45,17 +45,21 @@ import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.omnibox.LocationBarDataProvider; import org.chromium.chrome.browser.omnibox.LocationBarDataProvider;
import org.chromium.chrome.browser.omnibox.OmniboxSuggestionType; import org.chromium.chrome.browser.omnibox.OmniboxSuggestionType;
import org.chromium.chrome.browser.omnibox.UrlBarEditingTextStateProvider; import org.chromium.chrome.browser.omnibox.UrlBarEditingTextStateProvider;
import org.chromium.chrome.browser.omnibox.suggestions.AutocompleteMediator.EditSessionState;
import org.chromium.chrome.browser.omnibox.suggestions.header.HeaderProcessor; import org.chromium.chrome.browser.omnibox.suggestions.header.HeaderProcessor;
import org.chromium.chrome.browser.profiles.Profile; import org.chromium.chrome.browser.profiles.Profile;
import org.chromium.chrome.test.util.browser.Features; import org.chromium.chrome.test.util.browser.Features;
import org.chromium.chrome.test.util.browser.Features.DisableFeatures; import org.chromium.chrome.test.util.browser.Features.DisableFeatures;
import org.chromium.chrome.test.util.browser.Features.EnableFeatures; import org.chromium.chrome.test.util.browser.Features.EnableFeatures;
import org.chromium.components.metrics.OmniboxEventProtos.OmniboxEventProto.PageClassification; import org.chromium.components.metrics.OmniboxEventProtos.OmniboxEventProto.PageClassification;
import org.chromium.components.query_tiles.QueryTile;
import org.chromium.content_public.browser.test.NativeLibraryTestUtils; import org.chromium.content_public.browser.test.NativeLibraryTestUtils;
import org.chromium.ui.base.PageTransition;
import org.chromium.ui.modelutil.MVCListAdapter.ModelList; import org.chromium.ui.modelutil.MVCListAdapter.ModelList;
import org.chromium.ui.modelutil.PropertyModel; import org.chromium.ui.modelutil.PropertyModel;
import java.util.ArrayList; import java.util.ArrayList;
import java.util.Arrays;
import java.util.List; import java.util.List;
/** /**
...@@ -68,11 +72,60 @@ public class AutocompleteMediatorUnitTest { ...@@ -68,11 +72,60 @@ public class AutocompleteMediatorUnitTest {
private static final int SUGGESTION_MIN_HEIGHT = 20; private static final int SUGGESTION_MIN_HEIGHT = 20;
private static final int HEADER_MIN_HEIGHT = 15; private static final int HEADER_MIN_HEIGHT = 15;
// Empty AutocompleteDelegate implementation for test. This is to work around an issue that
// mock doesn't work on inherited methods in some builds.
static class AutocompleteDelegateForTest implements AutocompleteDelegate {
@Override
public void setOmniboxEditingText(String text) {}
@Override
public void clearOmniboxFocus() {}
@Override
public void onUrlTextChanged() {}
@Override
public void onSuggestionsChanged(String autocompleteText) {}
@Override
public void onSuggestionsHidden() {}
@Override
public void setKeyboardVisibility(boolean shouldShow, boolean delayHide) {}
@Override
public boolean isKeyboardActive() {
return true;
}
@Override
public void loadUrl(String url, @PageTransition int transition, long inputStart) {}
@Override
public void loadUrlWithPostData(String url, @PageTransition int transition, long inputStart,
String postDataType, byte[] postData) {}
@Override
public boolean didFocusUrlFromFakebox() {
return false;
}
@Override
public boolean isUrlBarFocused() {
return false;
}
@Override
public boolean didFocusUrlFromQueryTiles() {
return false;
}
}
@Rule @Rule
public TestRule mProcessor = new Features.JUnitProcessor(); public TestRule mProcessor = new Features.JUnitProcessor();
@Mock @Mock
AutocompleteDelegate mAutocompleteDelegate; AutocompleteDelegateForTest mAutocompleteDelegate;
@Mock @Mock
UrlBarEditingTextStateProvider mTextStateProvider; UrlBarEditingTextStateProvider mTextStateProvider;
...@@ -517,4 +570,47 @@ public class AutocompleteMediatorUnitTest { ...@@ -517,4 +570,47 @@ public class AutocompleteMediatorUnitTest {
verify(mAutocompleteController) verify(mAutocompleteController)
.startZeroSuggest(profile, "", url, pageClassification, title); .startZeroSuggest(profile, "", url, pageClassification, title);
} }
@Test
@SmallTest
@UiThreadTest
@DisableFeatures(ChromeFeatureList.OMNIBOX_ADAPTIVE_SUGGESTIONS_COUNT)
public void onQueryTilesSelected_thenTextChanged_editSessionActivatedByQueryTile() {
mMediator.onNativeInitialized();
QueryTile childTile = new QueryTile("sports", "sports", "sports", "sports",
new String[] {"http://foo/sports.jpg"}, null /* searchParams */,
null /* children */);
QueryTile tile =
new QueryTile("news", "news", "news", "news", new String[] {"http://foo/news.jpg"},
null /* searchParams */, Arrays.asList(childTile));
mMediator.onUrlFocusChange(true);
Assert.assertEquals(mMediator.getEditSessionStateForTest(), EditSessionState.INACTIVE);
mMediator.onQueryTileSelected(tile);
Assert.assertEquals(
mMediator.getEditSessionStateForTest(), EditSessionState.ACTIVATED_BY_QUERY_TILE);
verify(mAutocompleteDelegate).setOmniboxEditingText("news ");
mMediator.onTextChanged("news s", "news sports");
Assert.assertEquals(
mMediator.getEditSessionStateForTest(), EditSessionState.ACTIVATED_BY_QUERY_TILE);
mMediator.onUrlFocusChange(false);
Assert.assertEquals(mMediator.getEditSessionStateForTest(), EditSessionState.INACTIVE);
}
@Test
@SmallTest
@UiThreadTest
@DisableFeatures(ChromeFeatureList.OMNIBOX_ADAPTIVE_SUGGESTIONS_COUNT)
public void onTextChanged_editSessionActivatedByUserInput() {
mMediator.onNativeInitialized();
mMediator.onUrlFocusChange(true);
Assert.assertEquals(mMediator.getEditSessionStateForTest(), EditSessionState.INACTIVE);
mMediator.onTextChanged("n", "news");
Assert.assertEquals(
mMediator.getEditSessionStateForTest(), EditSessionState.ACTIVATED_BY_USER_INPUT);
mMediator.onUrlFocusChange(false);
Assert.assertEquals(mMediator.getEditSessionStateForTest(), EditSessionState.INACTIVE);
}
} }
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