Fixed toolbar capture ignoring optional button updates
Toolbar captures are skipped if the toolbar's state hasn't changed. The
container for toolbar state is PhoneCaptureStateToken. This class
keeps a reference to ButtonData, which represents the state of the
optional toolbar button and it uses standard equality checks to
determine of the optional button has changed. This works if the
button is updated with a new instance of ButtonData, but many providers
reuse the same instance, just changing its properties. This results in
button updates (e.g. when changing the button type from settings) not
triggering a new toolbar capture.
This CL updates ButtonData to implement its hashCode() method, this gets
used by PhoneCaptureStateToken to determine if the same instance of
ButtonData changed its attributes.
Bug: 1445540
Change-Id: I3b393051e875df1301ccb093f569820c2eaeed28
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4550198
Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
Reviewed-by: Matthew Jones <mdjones@chromium.org>
Commit-Queue: Salvador Guerrero Ramos <salg@google.com>
Cr-Commit-Position: refs/heads/main@{#1147502}
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/bookmarks/AddToBookmarksToolbarButtonController.java b/chrome/android/java/src/org/chromium/chrome/browser/bookmarks/AddToBookmarksToolbarButtonController.java
index 0bda134..0ff490e 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/bookmarks/AddToBookmarksToolbarButtonController.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/bookmarks/AddToBookmarksToolbarButtonController.java
@@ -26,6 +26,8 @@
import org.chromium.components.feature_engagement.FeatureConstants;
import org.chromium.components.feature_engagement.Tracker;
+import java.util.Objects;
+
/**
* Defines a toolbar button to add the current web page to bookmarks.
*/
@@ -110,7 +112,7 @@
mBookmarkModelSupplier.get().hasBookmarkIdForTab(mActiveTabSupplier.get());
ButtonSpec buttonSpecForCurrentTab =
isCurrentTabBookmarked ? mFilledButtonSpec : mEmptyButtonSpec;
- if (mButtonData.getButtonSpec() != buttonSpecForCurrentTab) {
+ if (!Objects.equals(mButtonData.getButtonSpec(), buttonSpecForCurrentTab)) {
mButtonData.setButtonSpec(buttonSpecForCurrentTab);
notifyObservers(mButtonData.canShow());
}
diff --git a/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/ButtonData.java b/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/ButtonData.java
index 1da61cc..7d5baad 100644
--- a/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/ButtonData.java
+++ b/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/ButtonData.java
@@ -15,6 +15,8 @@
import org.chromium.chrome.browser.toolbar.adaptive.AdaptiveToolbarFeatures;
import org.chromium.chrome.browser.user_education.IPHCommandBuilder;
+import java.util.Objects;
+
/**
* Data needed to show an optional toolbar button.
*
@@ -122,5 +124,32 @@
public boolean isDynamicAction() {
return mIsDynamicAction;
}
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (!(o instanceof ButtonSpec)) {
+ return false;
+ }
+ ButtonSpec that = (ButtonSpec) o;
+ return mSupportsTinting == that.mSupportsTinting
+ && mButtonVariant == that.mButtonVariant
+ && mIsDynamicAction == that.mIsDynamicAction
+ && mActionChipLabelResId == that.mActionChipLabelResId
+ && Objects.equals(mDrawable, that.mDrawable)
+ && Objects.equals(mOnClickListener, that.mOnClickListener)
+ && Objects.equals(mOnLongClickListener, that.mOnLongClickListener)
+ && Objects.equals(mContentDescription, that.mContentDescription)
+ && Objects.equals(mIPHCommandBuilder, that.mIPHCommandBuilder);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(mDrawable, mOnClickListener, mOnLongClickListener,
+ mContentDescription, mSupportsTinting, mIPHCommandBuilder, mButtonVariant,
+ mIsDynamicAction, mActionChipLabelResId);
+ }
}
}
diff --git a/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/ButtonDataImpl.java b/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/ButtonDataImpl.java
index 9f4ccfe..740de4b 100644
--- a/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/ButtonDataImpl.java
+++ b/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/ButtonDataImpl.java
@@ -15,6 +15,8 @@
import org.chromium.chrome.browser.toolbar.adaptive.AdaptiveToolbarButtonVariant;
import org.chromium.chrome.browser.user_education.IPHCommandBuilder;
+import java.util.Objects;
+
/** An implementation of the {@link ButtonData}. */
public class ButtonDataImpl implements ButtonData {
private boolean mCanShow;
@@ -103,4 +105,22 @@
currentSpec.getButtonVariant(), currentSpec.getActionChipLabelResId());
setButtonSpec(newSpec);
}
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (!(o instanceof ButtonDataImpl)) {
+ return false;
+ }
+ ButtonDataImpl that = (ButtonDataImpl) o;
+ return mCanShow == that.mCanShow && mIsEnabled == that.mIsEnabled
+ && Objects.equals(mButtonSpec, that.mButtonSpec);
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(mCanShow, mIsEnabled, mButtonSpec);
+ }
}
diff --git a/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/adaptive/AdaptiveToolbarButtonController.java b/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/adaptive/AdaptiveToolbarButtonController.java
index 32b1cec..a3164de0 100644
--- a/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/adaptive/AdaptiveToolbarButtonController.java
+++ b/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/adaptive/AdaptiveToolbarButtonController.java
@@ -41,6 +41,7 @@
import java.util.HashMap;
import java.util.Iterator;
import java.util.Map;
+import java.util.Objects;
/** Meta {@link ButtonDataProvider} which chooses the optional button variant that will be shown. */
public class AdaptiveToolbarButtonController
@@ -196,7 +197,7 @@
mButtonData.setEnabled(receivedButtonData.isEnabled());
final ButtonSpec receivedButtonSpec = receivedButtonData.getButtonSpec();
// ButtonSpec is immutable, so we keep the previous value when noting changes.
- if (receivedButtonSpec != mOriginalButtonSpec) {
+ if (!Objects.equals(receivedButtonSpec, mOriginalButtonSpec)) {
assert receivedButtonSpec.getOnLongClickListener()
== null
: "adaptive button variants are expected to not set a long click listener";
diff --git a/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/top/OptionalBrowsingModeButtonControllerTest.java b/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/top/OptionalBrowsingModeButtonControllerTest.java
index 6ae539a..d6d2829 100644
--- a/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/top/OptionalBrowsingModeButtonControllerTest.java
+++ b/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/top/OptionalBrowsingModeButtonControllerTest.java
@@ -46,9 +46,9 @@
@Mock
Tab mTab;
- ButtonDataImpl mButtonData1;
- ButtonDataImpl mButtonData2;
- ButtonDataImpl mButtonData3;
+ ButtonDataImpl mNewTabButtonData;
+ ButtonDataImpl mShareButtonData;
+ ButtonDataImpl mVoiceButtonData;
@Captor
ArgumentCaptor<ButtonDataProvider.ButtonDataObserver> mObserverCaptor1;
@@ -63,12 +63,12 @@
public void setUp() {
MockitoAnnotations.initMocks(this);
- mButtonData1 = createButtonData();
- mButtonData2 = createButtonData();
- mButtonData3 = createButtonData();
- doReturn(mButtonData1).when(mButtonDataProvider1).get(mTab);
- doReturn(mButtonData2).when(mButtonDataProvider2).get(mTab);
- doReturn(mButtonData3).when(mButtonDataProvider3).get(mTab);
+ mNewTabButtonData = createButtonData(AdaptiveToolbarButtonVariant.NEW_TAB);
+ mShareButtonData = createButtonData(AdaptiveToolbarButtonVariant.SHARE);
+ mVoiceButtonData = createButtonData(AdaptiveToolbarButtonVariant.VOICE);
+ doReturn(mNewTabButtonData).when(mButtonDataProvider1).get(mTab);
+ doReturn(mShareButtonData).when(mButtonDataProvider2).get(mTab);
+ doReturn(mVoiceButtonData).when(mButtonDataProvider3).get(mTab);
List<ButtonDataProvider> buttonDataProviders =
Arrays.asList(mButtonDataProvider1, mButtonDataProvider2, mButtonDataProvider3);
@@ -82,14 +82,14 @@
@Test
public void allProvidersEligible_highestPrecedenceShown() {
mButtonController.updateButtonVisibility();
- verify(mToolbarLayout, times(1)).updateOptionalButton(mButtonData1);
+ verify(mToolbarLayout, times(1)).updateOptionalButton(mNewTabButtonData);
}
@Test
public void noProvidersEligible_noneShown() {
- mButtonData1.setCanShow(false);
- mButtonData2.setCanShow(false);
- mButtonData3.setCanShow(false);
+ mNewTabButtonData.setCanShow(false);
+ mShareButtonData.setCanShow(false);
+ mVoiceButtonData.setCanShow(false);
mButtonController.updateButtonVisibility();
verify(mToolbarLayout, times(0)).updateOptionalButton(any());
@@ -97,70 +97,70 @@
@Test
public void noProvidersEligible_oneBecomesEligible() {
- mButtonData1.setCanShow(false);
- mButtonData2.setCanShow(false);
- mButtonData3.setCanShow(false);
+ mNewTabButtonData.setCanShow(false);
+ mShareButtonData.setCanShow(false);
+ mVoiceButtonData.setCanShow(false);
mButtonController.updateButtonVisibility();
verify(mToolbarLayout, times(0)).updateOptionalButton(any());
- mButtonData2.setCanShow(true);
+ mShareButtonData.setCanShow(true);
mButtonController.buttonDataProviderChanged(mButtonDataProvider2, true);
- verify(mToolbarLayout, times(1)).updateOptionalButton(mButtonData2);
+ verify(mToolbarLayout, times(1)).updateOptionalButton(mShareButtonData);
}
@Test
public void higherPrecedenceBecomesEligible() {
- mButtonData1.setCanShow(false);
- mButtonData2.setCanShow(false);
+ mNewTabButtonData.setCanShow(false);
+ mShareButtonData.setCanShow(false);
mButtonController.updateButtonVisibility();
- verify(mToolbarLayout, times(1)).updateOptionalButton(mButtonData3);
+ verify(mToolbarLayout, times(1)).updateOptionalButton(mVoiceButtonData);
- mButtonData2.setCanShow(true);
+ mShareButtonData.setCanShow(true);
mButtonController.buttonDataProviderChanged(mButtonDataProvider2, true);
- verify(mToolbarLayout, times(1)).updateOptionalButton(mButtonData2);
+ verify(mToolbarLayout, times(1)).updateOptionalButton(mShareButtonData);
- mButtonData1.setCanShow(true);
+ mNewTabButtonData.setCanShow(true);
mButtonController.buttonDataProviderChanged(mButtonDataProvider1, true);
- verify(mToolbarLayout, times(1)).updateOptionalButton(mButtonData1);
+ verify(mToolbarLayout, times(1)).updateOptionalButton(mNewTabButtonData);
}
@Test
public void lowerPrecedenceBecomesEligible() {
- mButtonData2.setCanShow(false);
- mButtonData3.setCanShow(false);
+ mShareButtonData.setCanShow(false);
+ mVoiceButtonData.setCanShow(false);
mButtonController.updateButtonVisibility();
- verify(mToolbarLayout, times(1)).updateOptionalButton(mButtonData1);
+ verify(mToolbarLayout, times(1)).updateOptionalButton(mNewTabButtonData);
- mButtonData2.setCanShow(true);
+ mShareButtonData.setCanShow(true);
mButtonController.buttonDataProviderChanged(mButtonDataProvider2, true);
- verify(mToolbarLayout, times(0)).updateOptionalButton(mButtonData2);
- verify(mToolbarLayout, times(1)).updateOptionalButton(mButtonData1);
+ verify(mToolbarLayout, times(0)).updateOptionalButton(mShareButtonData);
+ verify(mToolbarLayout, times(1)).updateOptionalButton(mNewTabButtonData);
}
@Test
public void updateCurrentlyShowingProvider() {
mButtonController.updateButtonVisibility();
- verify(mToolbarLayout, times(1)).updateOptionalButton(mButtonData1);
+ verify(mToolbarLayout, times(1)).updateOptionalButton(mNewTabButtonData);
- ButtonDataImpl newButtonData = createButtonData();
+ ButtonDataImpl newButtonData = mShareButtonData;
doReturn(newButtonData).when(mButtonDataProvider1).get(mTab);
mButtonController.buttonDataProviderChanged(mButtonDataProvider1, true);
verify(mToolbarLayout, times(1)).updateOptionalButton(newButtonData);
newButtonData.setCanShow(false);
mButtonController.buttonDataProviderChanged(mButtonDataProvider1, false);
- verify(mToolbarLayout, times(1)).updateOptionalButton(mButtonData2);
+ verify(mToolbarLayout, times(1)).updateOptionalButton(mShareButtonData);
}
@Test
public void updateCurrentlyNotShowingProvider() {
mButtonController.updateButtonVisibility();
- verify(mToolbarLayout, times(1)).updateOptionalButton(mButtonData1);
+ verify(mToolbarLayout, times(1)).updateOptionalButton(mNewTabButtonData);
- ButtonDataImpl newButtonData = createButtonData();
+ ButtonDataImpl newButtonData = mShareButtonData;
mButtonController.buttonDataProviderChanged(mButtonDataProvider2, true);
verify(mToolbarLayout, times(0)).updateOptionalButton(newButtonData);
@@ -172,10 +172,10 @@
@Test
public void noProvidersEligible_hideCalled() {
mButtonController.updateButtonVisibility();
- verify(mToolbarLayout, times(1)).updateOptionalButton(mButtonData1);
- mButtonData1.setCanShow(false);
- mButtonData2.setCanShow(false);
- mButtonData3.setCanShow(false);
+ verify(mToolbarLayout, times(1)).updateOptionalButton(mNewTabButtonData);
+ mNewTabButtonData.setCanShow(false);
+ mShareButtonData.setCanShow(false);
+ mVoiceButtonData.setCanShow(false);
mButtonController.updateButtonVisibility();
verify(mToolbarLayout, times(1)).hideOptionalButton();
@@ -183,12 +183,12 @@
@Test
public void hintContradictsTrueValue() {
- mButtonData1.setCanShow(false);
+ mNewTabButtonData.setCanShow(false);
mButtonController.updateButtonVisibility();
- verify(mToolbarLayout, times(1)).updateOptionalButton(mButtonData2);
+ verify(mToolbarLayout, times(1)).updateOptionalButton(mShareButtonData);
mButtonController.buttonDataProviderChanged(mButtonDataProvider1, true);
- verify(mToolbarLayout, never()).updateOptionalButton(mButtonData1);
+ verify(mToolbarLayout, never()).updateOptionalButton(mNewTabButtonData);
}
@Test
@@ -201,16 +201,16 @@
@Test
public void updateOptionalButtonIsOnEnabled() {
- mButtonData1.setEnabled(false);
+ mNewTabButtonData.setEnabled(false);
mButtonController.updateButtonVisibility();
- verify(mToolbarLayout, times(1)).updateOptionalButton(mButtonData1);
+ verify(mToolbarLayout, times(1)).updateOptionalButton(mNewTabButtonData);
}
- private static ButtonDataImpl createButtonData() {
+ private static ButtonDataImpl createButtonData(
+ @AdaptiveToolbarButtonVariant int buttonVariant) {
return new ButtonDataImpl(
/*canShow=*/true, /*drawable=*/null, /*onClickListener=*/null,
/*contentDescription=*/"", /*supportsTinting=*/false,
- /*iphCommandBuilder=*/null, /*isEnabled=*/true,
- AdaptiveToolbarButtonVariant.UNKNOWN);
+ /*iphCommandBuilder=*/null, /*isEnabled=*/true, buttonVariant);
}
}
diff --git a/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/top/PhoneCaptureStateToken.java b/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/top/PhoneCaptureStateToken.java
index 369fb78..cf385213 100644
--- a/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/top/PhoneCaptureStateToken.java
+++ b/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/top/PhoneCaptureStateToken.java
@@ -22,7 +22,7 @@
class PhoneCaptureStateToken {
private final @ColorInt int mTint;
private final int mTabCount;
- private final ButtonData mOptionalButtonData;
+ private final int mOptionalButtonDataHashCode;
private final @VisualState int mVisualState;
private final VisibleUrlText mVisibleUrlText;
private final @DrawableRes int mSecurityIcon;
@@ -39,7 +39,7 @@
boolean isPaintPreview, float progress, int unfocusedLocationBarLayoutWidth) {
mTint = tint;
mTabCount = tabCount;
- mOptionalButtonData = optionalButtonData;
+ mOptionalButtonDataHashCode = Objects.hashCode(optionalButtonData);
mVisualState = visualState;
mVisibleUrlText = visibleUrlText;
mSecurityIcon = securityIcon;
@@ -65,7 +65,7 @@
return ToolbarSnapshotDifference.TINT;
} else if (mTabCount != that.mTabCount) {
return ToolbarSnapshotDifference.TAB_COUNT;
- } else if (!Objects.equals(mOptionalButtonData, that.mOptionalButtonData)) {
+ } else if (mOptionalButtonDataHashCode != that.mOptionalButtonDataHashCode) {
return ToolbarSnapshotDifference.OPTIONAL_BUTTON;
} else if (mVisualState != that.mVisualState) {
return ToolbarSnapshotDifference.VISUAL_STATE;
diff --git a/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/top/PhoneCaptureStateTokenTest.java b/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/top/PhoneCaptureStateTokenTest.java
index f2e629a0..9029a33 100644
--- a/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/top/PhoneCaptureStateTokenTest.java
+++ b/chrome/browser/ui/android/toolbar/java/src/org/chromium/chrome/browser/toolbar/top/PhoneCaptureStateTokenTest.java
@@ -6,6 +6,7 @@
import android.content.res.ColorStateList;
import android.graphics.Color;
+import android.graphics.drawable.ColorDrawable;
import androidx.annotation.ColorInt;
import androidx.annotation.DrawableRes;
@@ -79,15 +80,44 @@
@Test
public void testDifferentOptionalButtonData() {
+ ButtonDataImpl otherButtonData = (ButtonDataImpl) makeButtonDate();
+ otherButtonData.setCanShow(!otherButtonData.canShow());
PhoneCaptureStateToken otherPhoneCaptureStateToken =
new PhoneCustomTabCaptureStateTokenBuilder()
- .setOptionalButtonData(makeButtonDate())
+ .setOptionalButtonData(otherButtonData)
.build();
Assert.assertEquals(ToolbarSnapshotDifference.OPTIONAL_BUTTON,
otherPhoneCaptureStateToken.getAnyDifference(mDefaultPhoneCaptureStateToken));
}
@Test
+ public void testNullOptionalButtonData() {
+ PhoneCaptureStateToken otherPhoneCaptureStateToken =
+ new PhoneCustomTabCaptureStateTokenBuilder().setOptionalButtonData(null).build();
+ Assert.assertEquals(ToolbarSnapshotDifference.OPTIONAL_BUTTON,
+ otherPhoneCaptureStateToken.getAnyDifference(mDefaultPhoneCaptureStateToken));
+ }
+
+ @Test
+ public void testSameOptionalButtonData_DifferentDrawable() {
+ ButtonDataImpl buttonData = (ButtonDataImpl) makeButtonDate();
+ // Create a capture state with the original ButtonData.
+ PhoneCaptureStateToken initialPhoneCaptureStateToken =
+ new PhoneCustomTabCaptureStateTokenBuilder()
+ .setOptionalButtonData(buttonData)
+ .build();
+ buttonData.updateDrawable(new ColorDrawable());
+ // Then create another capture when the button's drawable gets updated.
+ PhoneCaptureStateToken otherPhoneCaptureStateToken =
+ new PhoneCustomTabCaptureStateTokenBuilder()
+ .setOptionalButtonData(buttonData)
+ .build();
+
+ Assert.assertEquals(ToolbarSnapshotDifference.OPTIONAL_BUTTON,
+ initialPhoneCaptureStateToken.getAnyDifference(otherPhoneCaptureStateToken));
+ }
+
+ @Test
public void testDifferentVisualState() {
PhoneCaptureStateToken otherPhoneCaptureStateToken =
new PhoneCustomTabCaptureStateTokenBuilder()
