Change shopping offer price acquisition to a fetcher model

In the code as is, there exists the possibility mModel
will change before the callback returns which would mean
the price is set on an incorrect or out of bounds index.
In addition, it better from a resource utilization perspective
to only acquire the price when it is actually needed.

Bug: 1139031
Change-Id: I2f46851745b70bf6ddfceb33a09c5ef5700b2880
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2476811Reviewed-by: Yusuf Ozuysal <yusufo@chromium.org>
Reviewed-by: Mei Liang <meiliang@chromium.org>
Reviewed-by: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Commit-Queue: David Maunder <davidjm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#823234}

chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabGridViewBinder.java

@@ -213,21 +213,25 @@ class TabGridViewBinder {
                 pageInfoButton.setVisibility(View.GONE);
             } else {
                 // Search query and price string are mutually exclusive
-                assert TextUtils.isEmpty(model.get(TabProperties.PRICE_STRING));
+                assert model.get(TabProperties.SHOPPING_PERSISTED_TAB_DATA_FETCHER) == null;
                 pageInfoButton.setVisibility(View.VISIBLE);
                 pageInfoButton.getPrimaryTextView().setText(query);
             }
-        } else if (TabProperties.PRICE_STRING == propertyKey) {
-            String priceString = model.get(TabProperties.PRICE_STRING);
-            ChipView pageInfoButton = (ChipView) view.fastFindViewById(R.id.page_info_button);
-            if (TextUtils.isEmpty(priceString)) {
-                pageInfoButton.setVisibility(View.GONE);
-            } else {
-                // Price string and search query are mutually exclusive
-                assert TextUtils.isEmpty(model.get(TabProperties.SEARCH_QUERY));
-                pageInfoButton.setVisibility(View.VISIBLE);
-                pageInfoButton.getPrimaryTextView().setText(priceString);
-            }
+        } else if (TabProperties.SHOPPING_PERSISTED_TAB_DATA_FETCHER == propertyKey) {
+            model.get(TabProperties.SHOPPING_PERSISTED_TAB_DATA_FETCHER)
+                    .fetch((shoppingPersistedTabData) -> {
+                        ChipView pageInfoButton =
+                                (ChipView) view.fastFindViewById(R.id.page_info_button);
+                        if (TextUtils.isEmpty(shoppingPersistedTabData.getPriceString())) {
+                            pageInfoButton.setVisibility(View.GONE);
+                        } else {
+                            // Price string and search query are mutually exclusive
+                            assert TextUtils.isEmpty(model.get(TabProperties.SEARCH_QUERY));
+                            pageInfoButton.setVisibility(View.VISIBLE);
+                            pageInfoButton.getPrimaryTextView().setText(
+                                    shoppingPersistedTabData.getPriceString());
+                        }
+                    });
         } else if (TabProperties.PAGE_INFO_LISTENER == propertyKey) {
             TabListMediator.TabActionListener listener =
                     model.get(TabProperties.PAGE_INFO_LISTENER);

chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabListMediator.java

@@ -163,6 +163,28 @@ class TabListMediator {
         boolean isReorderAction(int action);
     }
 
+    /**
+     * Provides capability to asynchronously acquire {@link ShoppingPersistedTabData}
+     */
+    static class ShoppingPersistedTabDataFetcher {
+        protected Tab mTab;
+
+        /**
+         * @param tab {@link Tab} {@link ShoppingPersistedTabData} will be acquired for
+         */
+        ShoppingPersistedTabDataFetcher(Tab tab) {
+            mTab = tab;
+        }
+
+        /**
+         * Asynchronously acquire {@link ShoppingPersistedTabData}
+         * @param callback {@link Callback} to pass {@link ShoppingPersistedTabData} back in
+         */
+        public void fetch(Callback<ShoppingPersistedTabData> callback) {
+            ShoppingPersistedTabData.from(mTab, (res) -> { callback.onResult(res); });
+        }
+    }
+
     /**
      * The object to set to {@link TabProperties#THUMBNAIL_FETCHER} for the TabGridViewBinder to
      * obtain the thumbnail asynchronously.
@@ -1045,11 +1067,8 @@ class TabListMediator {
 
         if (TabUiFeatureUtilities.ENABLE_PRICE_TRACKING.getValue() && mMode == TabListMode.GRID
                 && pseudoTab.hasRealTab()) {
-            ShoppingPersistedTabData.from(pseudoTab.getTab(), (res) -> {
-                if (!TextUtils.isEmpty(res.getPriceString())) {
-                    mModel.get(index).model.set(TabProperties.PRICE_STRING, res.getPriceString());
-                }
-            });
+            mModel.get(index).model.set(TabProperties.SHOPPING_PERSISTED_TAB_DATA_FETCHER,
+                    new ShoppingPersistedTabDataFetcher(pseudoTab.getTab()));
         }
 
         updateFaviconForTab(pseudoTab, null);

chrome/android/features/tab_ui/java/src/org/chromium/chrome/browser/tasks/tab_management/TabProperties.java

@@ -103,8 +103,8 @@ public class TabProperties {
     public static final WritableObjectPropertyKey<String> SEARCH_QUERY =
             new WritableObjectPropertyKey<>();
 
-    public static final WritableObjectPropertyKey<String> PRICE_STRING =
-            new WritableObjectPropertyKey<>();
+    public static final WritableObjectPropertyKey<TabListMediator.ShoppingPersistedTabDataFetcher>
+            SHOPPING_PERSISTED_TAB_DATA_FETCHER = new WritableObjectPropertyKey<>(true);
 
     public static final WritableObjectPropertyKey<TabListMediator.TabActionListener>
             PAGE_INFO_LISTENER = new WritableObjectPropertyKey<>();
@@ -126,7 +126,8 @@ public class TabProperties {
             SELECTABLE_TAB_ACTION_BUTTON_BACKGROUND,
             SELECTABLE_TAB_ACTION_BUTTON_SELECTED_BACKGROUND, URL_DOMAIN, ACCESSIBILITY_DELEGATE,
             SEARCH_QUERY, PAGE_INFO_LISTENER, PAGE_INFO_ICON_DRAWABLE_ID, CARD_TYPE,
-            CONTENT_DESCRIPTION_STRING, CLOSE_BUTTON_DESCRIPTION_STRING, PRICE_STRING};
+            CONTENT_DESCRIPTION_STRING, CLOSE_BUTTON_DESCRIPTION_STRING,
+            SHOPPING_PERSISTED_TAB_DATA_FETCHER};
 
     public static final PropertyKey[] ALL_KEYS_TAB_STRIP =
             new PropertyKey[] {TAB_ID, TAB_SELECTED_LISTENER, TAB_CLOSED_LISTENER, FAVICON,

chrome/android/features/tab_ui/javatests/src/org/chromium/chrome/browser/tasks/tab_management/TabListViewHolderTest.java

@@ -29,7 +29,9 @@ import org.junit.runner.RunWith;
 
 import org.chromium.base.Callback;
 import org.chromium.base.test.UiThreadTest;
+import org.chromium.chrome.browser.tab.MockTab;
 import org.chromium.chrome.browser.tab.Tab;
+import org.chromium.chrome.browser.tab.state.ShoppingPersistedTabData;
 import org.chromium.chrome.tab_ui.R;
 import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
 import org.chromium.components.browser_ui.widget.selectable_list.SelectionDelegate;
@@ -561,14 +563,39 @@ public class TabListViewHolderTest extends DummyUiActivityTestCase {
         testGridSelected(mTabGridView, mGridModel);
         ChipView pageInfoButton = mTabGridView.findViewById(R.id.page_info_button);
 
-        mGridModel.set(TabProperties.PRICE_STRING, EXPECTED_PRICE_STRING);
+        Tab tab = MockTab.createAndInitialize(1, false);
+        MockShoppingPersistedTabDataFetcher fetcher = new MockShoppingPersistedTabDataFetcher(tab);
+        fetcher.setPriceString(EXPECTED_PRICE_STRING);
+        mGridModel.set(TabProperties.SHOPPING_PERSISTED_TAB_DATA_FETCHER, fetcher);
         Assert.assertEquals(View.VISIBLE, pageInfoButton.getVisibility());
         Assert.assertEquals(EXPECTED_PRICE_STRING, pageInfoButton.getPrimaryTextView().getText());
 
-        mGridModel.set(TabProperties.PRICE_STRING, null);
+        fetcher.setPriceString("");
+        mGridModel.set(TabProperties.SHOPPING_PERSISTED_TAB_DATA_FETCHER, fetcher);
         Assert.assertEquals(View.GONE, pageInfoButton.getVisibility());
     }
 
+    /**
+     * Mock {@link TabListMediator.ShoppingPersistedTabDataFetcher} for testing purposes
+     */
+    static class MockShoppingPersistedTabDataFetcher
+            extends TabListMediator.ShoppingPersistedTabDataFetcher {
+        MockShoppingPersistedTabDataFetcher(Tab tab) {
+            super(tab);
+        }
+        public void setPriceString(String priceString) {
+            ShoppingPersistedTabData shoppingPersistedTabData = new ShoppingPersistedTabData(mTab);
+            shoppingPersistedTabData.setPriceString(priceString, null);
+            mTab.getUserDataHost().setUserData(
+                    ShoppingPersistedTabData.class, shoppingPersistedTabData);
+        }
+
+        @Override
+        public void fetch(Callback<ShoppingPersistedTabData> callback) {
+            callback.onResult(mTab.getUserDataHost().getUserData(ShoppingPersistedTabData.class));
+        }
+    }
+
     @Test
     @MediumTest
     @UiThreadTest

chrome/android/features/tab_ui/junit/src/org/chromium/chrome/browser/tasks/tab_management/TabListMediatorUnitTest.java

@@ -1861,8 +1861,16 @@ public class TabListMediatorUnitTest {
 
         mMediator.resetWithListOfTabs(
                 PseudoTab.getListOfPseudoTab(tabs), /*quickMode =*/false, /*mruMode =*/false);
-        assertThat(mModel.get(0).model.get(TabProperties.PRICE_STRING), equalTo("$3.14"));
-        assertThat(mModel.get(1).model.get(TabProperties.PRICE_STRING), equalTo(null));
+        mModel.get(0)
+                .model.get(TabProperties.SHOPPING_PERSISTED_TAB_DATA_FETCHER)
+                .fetch((shoppingPersistedTabData) -> {
+                    assertThat(shoppingPersistedTabData.getPriceString(), equalTo("$3.14"));
+                });
+        mModel.get(1)
+                .model.get(TabProperties.SHOPPING_PERSISTED_TAB_DATA_FETCHER)
+                .fetch((shoppingPersistedTabData) -> {
+                    assertThat(shoppingPersistedTabData.getPriceString(), equalTo(""));
+                });
     }
 
     @Test

chrome/browser/tab/java/src/org/chromium/chrome/browser/tab/state/ShoppingPersistedTabData.java

@@ -61,7 +61,8 @@ public class ShoppingPersistedTabData extends PersistedTabData {
     private String mPreviousPriceString = "";
     public long mLastPriceChangeTimeMs = NO_TRANSITIONS_OCCURRED;
 
-    protected ShoppingPersistedTabData(Tab tab) {
+    @VisibleForTesting(otherwise = VisibleForTesting.PROTECTED)
+    public ShoppingPersistedTabData(Tab tab) {
         super(tab,
                 PersistedTabDataConfiguration.get(ShoppingPersistedTabData.class, tab.isIncognito())
                         .storage,
@@ -135,7 +136,8 @@ public class ShoppingPersistedTabData extends PersistedTabData {
      * @param priceString a string representing the price of the shopping offer
      * @param previousShoppingPersistedTabData {@link ShoppingPersistedTabData} from previous fetch
      */
-    protected void setPriceString(
+    @VisibleForTesting(otherwise = VisibleForTesting.PROTECTED)
+    public void setPriceString(
             String priceString, ShoppingPersistedTabData previousShoppingPersistedTabData) {
         mPriceString = priceString;
         // Detect price transition