Skip to content

Commit

Permalink
Strip autocompleted text from UrlTextChangeListener#onTextChanged().
Browse files Browse the repository at this point in the history 
This change removes useless parameter `textWithAutocomplete` from
being passed to `UrlTextChangeListener#onTextChanged()`.

This parameter has never been used by any logic - because it's never
really mattered: the `TextWithAutocomplete` originates from the very
source that supplies it: `AutocompleteMediator`, and the only place
where it matters is the UrlBar (to offer hint to the user that "we
believe this is what they are after").

Change-Id: I29869517a4f5daf8edcd23c90565e1291734ff6e
Fixed: 1003080
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4795123
Reviewed-by: Patrick Noland <pnoland@chromium.org>
Commit-Queue: Tomasz Wiszkowski <ender@google.com>
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Auto-Submit: Tomasz Wiszkowski <ender@google.com>
Commit-Queue: Patrick Noland <pnoland@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1185421}
  • Loading branch information
@tomasz-wiszkowski
tomasz-wiszkowski authored and Chromium LUCI CQ committed Aug 18, 2023
1 parent fdbf9b2 commit 6b0b866
Show file tree
Hide file tree
Showing 10 changed files with 35 additions and 44 deletions.
3 changes: 1 addition & 2 deletions ...id/java/src/org/chromium/chrome/browser/searchwidget/SearchActivityLocationBarLayout.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,7 @@ void onDeferredStartup(@SearchType int searchType,
String textWithAutocomplete = mUrlCoordinator.getTextWithAutocomplete();
// Do not prefetch suggestions here; instead, we're asking the server for ZPS directly.
// Issuing multiple requests would result with only the final one being executed.
mAutocompleteCoordinator.onTextChanged(
mUrlCoordinator.getTextWithoutAutocomplete(), textWithAutocomplete);
mAutocompleteCoordinator.onTextChanged(mUrlCoordinator.getTextWithoutAutocomplete());

if (mPendingBeginQuery) {
beginQueryInternal(searchType, voiceRecognitionHandler, windowAndroid);
Expand Down
12 changes: 6 additions & 6 deletions chrome/android/javatests/src/org/chromium/chrome/browser/omnibox/UrlBarTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ private void verifySelectionState(String text, String inlineAutocomplete, int se
final CallbackHelper autocompleteHelper = new CallbackHelper();
final AtomicReference<String> requestedAutocompleteText = new AtomicReference<String>();
final AtomicBoolean didPreventInlineAutocomplete = new AtomicBoolean();
mUrlBar.setUrlTextChangeListener((textWithoutAutocomplete, textWithAutocomplete) -> {
mUrlBar.setUrlTextChangeListener((textWithoutAutocomplete) -> {
autocompleteHelper.notifyCalled();
requestedAutocompleteText.set(textWithoutAutocomplete);
didPreventInlineAutocomplete.set(!mUrlBar.shouldAutocomplete());
Expand Down Expand Up @@ -259,7 +259,7 @@ public void testAutocompleteUpdatedOnSelection() throws TimeoutException {
public void testSendCursorPosition() throws TimeoutException {
final CallbackHelper autocompleteHelper = new CallbackHelper();
final AtomicInteger cursorPositionUsed = new AtomicInteger();
mUrlBar.setUrlTextChangeListener((textWithoutAutocomplete, textWithAutocomplete) -> {
mUrlBar.setUrlTextChangeListener((textWithoutAutocomplete) -> {
int cursorPosition = mUrlBar.getSelectionEnd() == mUrlBar.getSelectionStart()
? mUrlBar.getSelectionStart()
: -1;
Expand Down Expand Up @@ -325,7 +325,7 @@ public void testAutocompleteAllowedWhenReplacingText() throws TimeoutException {

final CallbackHelper autocompleteHelper = new CallbackHelper();
final AtomicBoolean didPreventInlineAutocomplete = new AtomicBoolean();
mUrlBar.setUrlTextChangeListener((textWithoutAutocomplete, textWithAutocomplete) -> {
mUrlBar.setUrlTextChangeListener((textWithoutAutocomplete) -> {
if (!TextUtils.equals(textToBeEntered, mUrlBar.getTextWithoutAutocomplete())) return;
didPreventInlineAutocomplete.set(!mUrlBar.shouldAutocomplete());
autocompleteHelper.notifyCalled();
Expand All @@ -350,7 +350,7 @@ public void testSuggestionsUpdatedWhenDeletingInlineAutocomplete() throws Timeou

final CallbackHelper autocompleteHelper = new CallbackHelper();
final AtomicBoolean didPreventInlineAutocomplete = new AtomicBoolean();
mUrlBar.setUrlTextChangeListener((textWithoutAutocomplete, textWithAutocomplete) -> {
mUrlBar.setUrlTextChangeListener((textWithoutAutocomplete) -> {
if (!TextUtils.equals("test", mUrlBar.getTextWithoutAutocomplete())) return;
didPreventInlineAutocomplete.set(!mUrlBar.shouldAutocomplete());
autocompleteHelper.notifyCalled();
Expand Down Expand Up @@ -491,13 +491,13 @@ public void testUrlTextChangeListener() {
mUrlBar.setUrlTextChangeListener(listener);

mOmnibox.setText("onomatop");
Mockito.verify(listener).onTextChanged("onomatop", "onomatop");
Mockito.verify(listener).onTextChanged("onomatop");

// Setting autocomplete does not send a change update.
mOmnibox.setAutocompleteText("oeia");

mOmnibox.setText("");
Mockito.verify(listener).onTextChanged("", "");
Mockito.verify(listener).onTextChanged("");
}

@Test
Expand Down
4 changes: 2 additions & 2 deletions .../ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/LocationBarMediator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -905,7 +905,7 @@ public void onAnimationEnd(Animator animation) {
/* package */ void forceOnTextChanged() {
String textWithoutAutocomplete = mUrlCoordinator.getTextWithoutAutocomplete();
String textWithAutocomplete = mUrlCoordinator.getTextWithAutocomplete();
mAutocompleteCoordinator.onTextChanged(textWithoutAutocomplete, textWithAutocomplete);
mAutocompleteCoordinator.onTextChanged(textWithoutAutocomplete);
}

/* package */ boolean shouldClearOmniboxOnFocus() {
Expand Down Expand Up @@ -1253,7 +1253,7 @@ public void setUrlBarFocus(boolean shouldBeFocused, @Nullable String pastedText,
the restored omnibox text input.
*/
if (reason == OmniboxFocusReason.FOLD_TRANSITION_RESTORATION) {
mAutocompleteCoordinator.onTextChanged(pastedText, pastedText);
mAutocompleteCoordinator.onTextChanged(pastedText);
} else {
forceOnTextChanged();
}
Expand Down
2 changes: 1 addition & 1 deletion ...android/omnibox/java/src/org/chromium/chrome/browser/omnibox/LocationBarMediatorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -730,7 +730,7 @@ public void testSetUrlBarFocus_pastedText() {
.setUrlBarData(argThat(matchesUrlBarDataForQuery("pastedText")),
eq(UrlBar.ScrollType.NO_SCROLL),
eq(UrlBarCoordinator.SelectionState.SELECT_END));
verify(mAutocompleteCoordinator).onTextChanged("text", "textWith");
verify(mAutocompleteCoordinator).onTextChanged("text");
}

@Test
Expand Down
7 changes: 2 additions & 5 deletions chrome/browser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/UrlBar.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,10 +177,8 @@ public interface UrlTextChangeListener {
/**
* Called when the text state has changed.
* @param textWithoutAutocomplete The url bar text without autocompletion.
* @param textWithAutocomplete The url bar text with autocompletion.
*/
// TODO(crbug.com/1003080): Consider splitting these into two different callbacks.
void onTextChanged(String textWithoutAutocomplete, String textWithAutocomplete);
void onTextChanged(String textWithoutAutocomplete);
}

/** Delegate that provides the additional functionality to the textual context menus. */
Expand Down Expand Up @@ -992,8 +990,7 @@ public void onAutocompleteTextStateChanged(boolean updateDisplay) {
// crbug.com/764749
Log.w(TAG, "Text change observed, triggering autocomplete.");

mUrlTextChangeListener.onTextChanged(
getTextWithoutAutocomplete(), getTextWithAutocomplete());
mUrlTextChangeListener.onTextChanged(getTextWithoutAutocomplete());
}

/**
Expand Down
5 changes: 2 additions & 3 deletions ...owser/ui/android/omnibox/java/src/org/chromium/chrome/browser/omnibox/UrlBarMediator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -348,10 +348,9 @@ private static String getUrlContentsPrePath(String url, String host) {

/** @see UrlTextChangeListener */
@Override
public void onTextChanged(String textWithoutAutocomplete, String textWithAutocomplete) {
public void onTextChanged(String textWithoutAutocomplete) {
for (int i = 0; i < mUrlTextChangeListeners.size(); i++) {
mUrlTextChangeListeners.get(i).onTextChanged(
textWithoutAutocomplete, textWithAutocomplete);
mUrlTextChangeListeners.get(i).onTextChanged(textWithoutAutocomplete);
}
}
}
13 changes: 5 additions & 8 deletions .../android/omnibox/java/src/org/chromium/chrome/browser/omnibox/UrlBarMediatorUnitTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -263,16 +263,13 @@ public void urlTextChangeListenerCompositeObserver() {

String text = "foo";
String textWithAutocomplete = "foo.bar";
mMediator.onTextChanged(text, textWithAutocomplete);
Mockito.verify(mMockUrlTextListener, Mockito.times(1))
.onTextChanged(text, textWithAutocomplete);
mMediator.onTextChanged(text);
Mockito.verify(mMockUrlTextListener, Mockito.times(1)).onTextChanged(text);

mMediator.addUrlTextChangeListener(mAnotherUrlTextMockListener);
mMediator.onTextChanged(text, textWithAutocomplete);
Mockito.verify(mMockUrlTextListener, Mockito.times(2))
.onTextChanged(text, textWithAutocomplete);
Mockito.verify(mAnotherUrlTextMockListener, Mockito.times(1))
.onTextChanged(text, textWithAutocomplete);
mMediator.onTextChanged(text);
Mockito.verify(mMockUrlTextListener, Mockito.times(2)).onTextChanged(text);
Mockito.verify(mAnotherUrlTextMockListener, Mockito.times(1)).onTextChanged(text);
}

private static SpannableStringBuilder spannable(String text) {
Expand Down
4 changes: 2 additions & 2 deletions ...box/java/src/org/chromium/chrome/browser/omnibox/suggestions/AutocompleteCoordinator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -389,8 +389,8 @@ public boolean handleKeyEvent(int keyCode, KeyEvent event) {
}

@Override
public void onTextChanged(String textWithoutAutocomplete, String textWithAutocomplete) {
mMediator.onTextChanged(textWithoutAutocomplete, textWithAutocomplete);
public void onTextChanged(String textWithoutAutocomplete) {
mMediator.onTextChanged(textWithoutAutocomplete);
}

/**
Expand Down
7 changes: 3 additions & 4 deletions ...mnibox/java/src/org/chromium/chrome/browser/omnibox/suggestions/AutocompleteMediator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,7 @@ void onUrlFocusChange(boolean hasFocus) {
postAutocompleteRequest(this::startZeroSuggest, SCHEDULE_FOR_IMMEDIATE_EXECUTION);
} else {
String text = mUrlBarEditingTextProvider.getTextWithoutAutocomplete();
onTextChanged(text, text);
onTextChanged(text);
}
} else {
stopMeasuringSuggestionRequestToUiModelTime();
Expand Down Expand Up @@ -449,8 +449,7 @@ public void onRefineSuggestion(AutocompleteMatch suggestion) {
if (isSearchSuggestion) refineText = TextUtils.concat(refineText, " ").toString();

mDelegate.setOmniboxEditingText(refineText);
onTextChanged(mUrlBarEditingTextProvider.getTextWithoutAutocomplete(),
mUrlBarEditingTextProvider.getTextWithAutocomplete());
onTextChanged(mUrlBarEditingTextProvider.getTextWithoutAutocomplete());

if (isSearchSuggestion) {
// Note: the logic below toggles assumes individual values to be represented by
Expand Down Expand Up @@ -691,7 +690,7 @@ private int findSuggestionInAutocompleteResult(AutocompleteMatch suggestion, int
* Notifies the autocomplete system that the text has changed that drives autocomplete and the
* autocomplete suggestions should be updated.
*/
public void onTextChanged(String textWithoutAutocomplete, String textWithAutocomplete) {
public void onTextChanged(String textWithoutAutocomplete) {
if (mShouldPreventOmniboxAutocomplete) return;

mIgnoreOmniboxItemSelection = true;
Expand Down
22 changes: 11 additions & 11 deletions ...ava/src/org/chromium/chrome/browser/omnibox/suggestions/AutocompleteMediatorUnitTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,7 @@ public void onTextChanged_emptyTextTriggersZeroSuggest() {
when(mTextStateProvider.getTextWithAutocomplete()).thenReturn("");

mMediator.onNativeInitialized();
mMediator.onTextChanged("", "");
mMediator.onTextChanged("");
verify(mAutocompleteController).startZeroSuggest("", url, pageClassification, title);
}

Expand All @@ -391,7 +391,7 @@ public void onTextChanged_nonEmptyTextTriggersSuggestions() {
when(mTextStateProvider.getSelectionEnd()).thenReturn(4);

mMediator.onNativeInitialized();
mMediator.onTextChanged("test", "testing");
mMediator.onTextChanged("test");
ShadowLooper.runUiThreadTasksIncludingDelayedTasks();
verify(mAutocompleteController).start(url, pageClassification, "test", 4, false);
}
Expand All @@ -410,8 +410,8 @@ public void onTextChanged_cancelsPendingRequests() {
when(mTextStateProvider.getSelectionEnd()).thenReturn(4);

mMediator.onNativeInitialized();
mMediator.onTextChanged("test", "testing");
mMediator.onTextChanged("nottest", "nottesting");
mMediator.onTextChanged("test");
mMediator.onTextChanged("nottest");
ShadowLooper.runUiThreadTasksIncludingDelayedTasks();
verify(mAutocompleteController, times(1))
.start(url, pageClassification, "nottest", 4, false);
Expand Down Expand Up @@ -490,9 +490,9 @@ public void onUrlFocusChange_textChangeCancelsOustandingZeroSuggestRequest() {

// Simulate URL being focus changes, and that user typed text and deleted it.
mMediator.onUrlFocusChange(true);
mMediator.onTextChanged("A", "Abc");
mMediator.onTextChanged("", "");
mMediator.onTextChanged("A", "Abc");
mMediator.onTextChanged("A");
mMediator.onTextChanged("");
mMediator.onTextChanged("A");

ShadowLooper.runUiThreadTasks();
verify(mAutocompleteController, never())
Expand Down Expand Up @@ -523,8 +523,8 @@ public void onUrlFocusChange_textChangeCancelsIntermediateZeroSuggestRequests()
when(mTextStateProvider.getTextWithAutocomplete()).thenReturn("");

// Simulate URL being focus changes, and that user typed text and deleted it.
mMediator.onTextChanged("A", "Abc");
mMediator.onTextChanged("", "");
mMediator.onTextChanged("A");
mMediator.onTextChanged("");

ShadowLooper.runUiThreadTasks();
verify(mAutocompleteController, never())
Expand Down Expand Up @@ -653,7 +653,7 @@ public void onTextChanged_editSessionActivatedByUserInput() {
mMediator.onNativeInitialized();
mMediator.onUrlFocusChange(true);
Assert.assertEquals(mMediator.getEditSessionStateForTest(), EditSessionState.INACTIVE);
mMediator.onTextChanged("n", "news");
mMediator.onTextChanged("n");
Assert.assertEquals(
mMediator.getEditSessionStateForTest(), EditSessionState.ACTIVATED_BY_USER_INPUT);

Expand Down Expand Up @@ -931,7 +931,7 @@ public void requestToUiModelTime_subsequentKeyStrokesReportTimeSinceLastKeystrok

// No change on key press. No unexpected recordings.
// Need to run looper here to flush the pending operation.
mMediator.onTextChanged("a", "a");
mMediator.onTextChanged("a");
ShadowLooper.runUiThreadTasksIncludingDelayedTasks();
verifySuggestionRequestToUiModelHistograms(1, 150, 0, null);

Expand Down

0 comments on commit 6b0b866

Please sign in to comment.