Reland "Tab Search: Infinite list - Efficient list DOM item updates"

This is a reland of 339975cc

This CL was reverted because it broke the Tab search list item contents
that are supposed to be visible once a search filter is applied and then
removed. This is now fixed in the updated patchset.

Original change's description:
> Tab Search: Infinite list - Efficient list DOM item updates
>
> Leverage the ListPropertyUpdateBehavior logic that uses Polymer's
> splices and calculateSplices functions to efficiently modify a single
> list item on tab updates, and avoid unnecessary scroll height
> calculations.
>
> Additionally, tab updates for not yet rendered items do not result in
> DOM updates.
>
> Proposal doc & benchmarks: https://docs.google.com/document/d/1FcuhM98moL-BYB1DJGXrpwf-JqMz1aI-RQV08giArBY/edit#heading=h.xzptrog8pyxf
>
> Most relevant pinpoint benchmarks:
> https://pinpoint-dot-chromeperf.appspot.com/job/15db6158d20000
> https://pinpoint-dot-chromeperf.appspot.com/job/107ab7f4d20000
> https://pinpoint-dot-chromeperf.appspot.com/job/16ccacccd20000
>
> Bug: 1149400
> Change-Id: I07fae967033b13830708da3035db3e8b2f0ff9d6
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2578119
> Commit-Queue: Roman Arora <romanarora@chromium.org>
> Reviewed-by: dpapad <dpapad@chromium.org>
> Reviewed-by: Esmael Elmoslimany <aee@chromium.org>
> Reviewed-by: Thomas Lukaszewicz <tluk@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#842182}

Bug: 1149400
Change-Id: Ia8ccf6d9ecdd80006de7a132fefcbe0357a68d5c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2622723Reviewed-by: Esmael Elmoslimany <aee@chromium.org>
Reviewed-by: Thomas Lukaszewicz <tluk@chromium.org>
Commit-Queue: Roman Arora <romanarora@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842787}

chrome/browser/resources/settings/site_settings/chooser_exception_list.js

@@ -197,7 +197,7 @@ Polymer({
 
     if (!this.updateList(
             'chooserExceptions', x => x.displayName, exceptions,
-            true /* uidBasedUpdate */)) {
+            true /* identityBasedUpdate= */)) {
       // The chooser objects have not been changed, so check if their site
       // permissions have changed. The |exceptions| and |this.chooserExceptions|
       // arrays should be the same length.

chrome/browser/resources/tab_search/BUILD.gn

@@ -182,6 +182,7 @@ js_library("infinite_list") {
   deps = [
     "//third_party/polymer/v3_0/components-chromium/iron-selector:iron-selector",
     "//third_party/polymer/v3_0/components-chromium/polymer:polymer_bundled",
+    "//ui/webui/resources/js:list_property_update_behavior.m",
   ]
 }
 

chrome/browser/resources/tab_search/app.html

@@ -58,7 +58,7 @@
     search-result-text="[[searchResultText_]]">
 </tab-search-search-field>
 <div hidden="[[!filteredOpenTabs_.length]]">
-  <infinite-list id="tabsList" items="[[filteredOpenTabs_]]" >
+  <infinite-list id="tabsList" items="[[filteredOpenTabs_]]">
     <template is="dom-repeat">
       <tab-search-item id="[[item.tab.tabId]]" aria-label="[[ariaLabel_(item)]]"
           class="mwb-list-item" data="[[item]]" on-click="onItemClick_"

chrome/browser/resources/tab_search/app.js

@@ -40,10 +40,9 @@ export class TabSearchAppElement extends PolymerElement {
         value: '',
       },
 
-      /** @private {?Array<!WindowTabs>} */
+      /** @private {?Array<!TabData>}*/
       openTabs_: {
         type: Array,
-        observer: 'openTabsChanged_',
       },
 
       /** @private {!Array<!TabData>} */
@@ -169,7 +168,7 @@ export class TabSearchAppElement extends PolymerElement {
           'Tabs.TabSearch.WebUI.TabListDataReceived',
           Math.round(Date.now() - getTabsStartTimestamp));
 
-      this.openTabs_ = profileTabs.windows;
+      this.openTabsChanged_(profileTabs.windows);
     });
   }
 
@@ -178,19 +177,13 @@ export class TabSearchAppElement extends PolymerElement {
    * @private
    */
   onTabUpdated_(updatedTab) {
-    const updatedTabId = updatedTab.tabId;
-    const windows = this.openTabs_;
-    if (windows) {
-      for (const window of windows) {
-        const {tabs} = window;
-        for (let i = 0; i < tabs.length; ++i) {
-          // Replace the tab with the same tabId and trigger rerender.
-          if (tabs[i].tabId === updatedTabId) {
-            tabs[i] = updatedTab;
-            this.openTabs_ = windows.concat();
-            return;
-          }
-        }
+    // Replace the tab with the same tabId and trigger rerender.
+    for (let i = 0; i < this.openTabs_.length; ++i) {
+      if (this.openTabs_[i].tab.tabId === updatedTab.tabId) {
+        this.openTabs_[i] =
+            this.tabData_(updatedTab, this.openTabs_[i].inActiveWindow);
+        this.updateFilteredTabs_(this.openTabs_);
+        return;
       }
     }
   }
@@ -200,14 +193,21 @@ export class TabSearchAppElement extends PolymerElement {
    * @private
    */
   onTabsRemoved_(tabIds) {
-    const windows = this.openTabs_;
-    if (windows) {
-      const ids = new Set(tabIds);
-      for (const window of windows) {
-        window.tabs = window.tabs.filter(tab => (!ids.has(tab.tabId)));
+    if (!this.openTabs_) {
+      return;
+    }
+
+    const ids = new Set(tabIds);
+    // Splicing in descending index order to avoid affecting preceding indices
+    // that are to be removed.
+    for (let i = this.openTabs_.length - 1; i >= 0; i--) {
+      if (ids.has(this.openTabs_[i].tab.tabId)) {
+        this.openTabs_.splice(i, 1);
       }
-      this.openTabs_ = windows.concat();
     }
+
+    this.filteredOpenTabs_ =
+        this.filteredOpenTabs_.filter(tabData => !ids.has(tabData.tab.tabId));
   }
 
   /**
@@ -304,11 +304,17 @@ export class TabSearchAppElement extends PolymerElement {
   }
 
   /**
-   * @param {!Array<!WindowTabs>} newOpenTabs
+   * @param {!Array<!WindowTabs>} newOpenWindowTabs
    * @private
    */
-  openTabsChanged_(newOpenTabs) {
-    this.updateFilteredTabs_(newOpenTabs);
+  openTabsChanged_(newOpenWindowTabs) {
+    this.openTabs_ = [];
+    newOpenWindowTabs.forEach(({active, tabs}) => {
+      tabs.forEach(tab => {
+        this.openTabs_.push(this.tabData_(tab, active));
+      });
+    });
+    this.updateFilteredTabs_(this.openTabs_);
 
     // If there was no previously selected index, set the first item as
     // selected; else retain the currently selected index. If the list
@@ -415,19 +421,22 @@ export class TabSearchAppElement extends PolymerElement {
   }
 
   /**
-   * @param {!Array<!WindowTabs>} windowTabs
+   * @param {!Tab} tab
+   * @param {boolean} inActiveWindow
+   * @return {!TabData}
    * @private
    */
-  updateFilteredTabs_(windowTabs) {
-    const result = [];
-    windowTabs.forEach(window => {
-      window.tabs.forEach(tab => {
-        const hostname = new URL(tab.url).hostname;
-        const inActiveWindow = window.active;
-        result.push({hostname, inActiveWindow, tab});
-      });
-    });
-    result.sort((a, b) => {
+  tabData_(tab, inActiveWindow) {
+    const hostname = new URL(tab.url).hostname;
+    return /** @type {!TabData} */ ({hostname, inActiveWindow, tab});
+  }
+
+  /**
+   * @param {!Array<!TabData>} tabs
+   * @private
+   */
+  updateFilteredTabs_(tabs) {
+    tabs.sort((a, b) => {
       // Move the active tab to the bottom of the list
       // because it's not likely users want to click on it.
       if (this.moveActiveTabToBottom_) {
@@ -444,8 +453,9 @@ export class TabSearchAppElement extends PolymerElement {
               a.tab.lastActiveTimeTicks.internalValue) :
           0;
     });
+
     this.filteredOpenTabs_ =
-        fuzzySearch(this.searchText_, result, this.fuzzySearchOptions_);
+        fuzzySearch(this.searchText_, tabs, this.fuzzySearchOptions_);
     this.searchResultText_ = this.getA11ySearchResultText_();
   }
 

chrome/browser/resources/tab_search/fuzzy_search.js

@@ -11,11 +11,12 @@ import {TabData} from './tab_data.js';
  * @param {string} input
  * @param {!Array<!TabData>} records
  * @param {!Object} options
- * @return {!Array<!TabData>}
+ * @return {!Array<!TabData>} A new array of entries satisfying the input. If no
+ *     search input is present, returns a shallow copy of the records.
  */
 export function fuzzySearch(input, records, options) {
   if (input.length === 0) {
-    return records;
+    return [...records];
   }
   // Fuse does not handle exact match searches well. It indiscriminately
   // searches for direct matches that appear anywhere in the string. This

chrome/browser/resources/tab_search/infinite_list.html

@@ -9,7 +9,6 @@
 </style>
 <div id="items">
   <iron-selector id="selector" on-keydown="onKeyDown_"
-      on-iron-items-changed="updateScrollerSize_"
       on-iron-select="onSelectedChanged_" role="listbox"
       selected-class="selected">
     <slot></slot>

chrome/browser/resources/tab_search/infinite_list.js

@@ -20,6 +20,7 @@
 import 'chrome://resources/polymer/v3_0/iron-selector/iron-selector.js';
 
 import {assert, assertInstanceof} from 'chrome://resources/js/assert.m.js';
+import {updateListProperty} from 'chrome://resources/js/list_property_update_behavior.m.js';
 import {listenOnce} from 'chrome://resources/js/util.m.js';
 import {afterNextRender, DomRepeat, html, PolymerElement} from 'chrome://resources/polymer/v3_0/polymer/polymer_bundled.min.js';
 
@@ -92,9 +93,9 @@ export class InfiniteList extends PolymerElement {
 
   /** @private */
   getDomItems_() {
-    const selectorChildren = this.$.selector.children;
+    const selector = /** @type {!IronSelectorElement} */ (this.$.selector);
     return Array.prototype.slice.call(
-        selectorChildren, 0, selectorChildren.length - 1);
+        selector.children, 0, selector.children.length - 1);
   }
 
   /**
@@ -155,6 +156,7 @@ export class InfiniteList extends PolymerElement {
       if (aboveScrollTopItemCount + this.chunkItemThreshold >
           this.domRepeat_.items.length) {
         this.ensureDomItemsAvailableStartingAt_(aboveScrollTopItemCount);
+        this.updateScrollerSize_();
       }
     }
   }
@@ -189,12 +191,13 @@ export class InfiniteList extends PolymerElement {
    * @private
    */
   domItemAverageHeight_() {
-    if (!this.$.selector.items || this.$.selector.items.length === 0) {
+    const selector = /** @type {!IronSelectorElement} */ (this.$.selector);
+    if (!selector.items || selector.items.length === 0) {
       return 0;
     }
 
-    const domItemCount = this.$.selector.items.length;
-    const lastDomItem = this.$.selector.items[domItemCount - 1];
+    const domItemCount = selector.items.length;
+    const lastDomItem = selector.items[domItemCount - 1];
     return (lastDomItem.offsetTop + lastDomItem.offsetHeight) / domItemCount;
   }
 
@@ -202,17 +205,36 @@ export class InfiniteList extends PolymerElement {
    * Ensures that when the items property changes, only a chunk of the items
    * needed to fill the current scroll position view are added to the DOM, thus
    * improving rendering performance.
+   *
+   * @param {!Array} newItems
+   * @param {!Array} oldItems
    * @private
    */
-  onItemsChanged_() {
-    if (this.domRepeat_ && this.items) {
-      const domItemAvgHeight = this.domItemAverageHeight_();
-      const aboveScrollTopItemCount = domItemAvgHeight !== 0 ?
-          Math.round(this.scrollTop / domItemAvgHeight) :
-          0;
+  onItemsChanged_(newItems, oldItems) {
+    if (!this.domRepeat_) {
+      return;
+    }
 
+    if (!oldItems || oldItems.length === 0) {
       this.domRepeat_.set('items', []);
-      this.ensureDomItemsAvailableStartingAt_(aboveScrollTopItemCount);
+      this.ensureDomItemsAvailableStartingAt_(0);
+      listenOnce(this.$.selector, 'iron-items-changed', () => {
+        this.updateScrollerSize_();
+      });
+
+      return;
+    }
+
+    updateListProperty(
+        this.domRepeat_, 'items', tabData => tabData,
+        newItems.slice(
+            0,
+            Math.min(
+                Math.max(this.domRepeat_.items.length, this.chunkItemCount),
+                newItems.length)),
+        true /* identityBasedUpdate= */);
+
+    if (newItems.length !== oldItems.length) {
       this.updateScrollerSize_();
     }
   }

chrome/test/data/webui/resources/list_property_update_behavior_tests.js

@@ -4,7 +4,7 @@
 
 /** @fileoverview Suite of tests for the ListPropertyUpdateBehavior.  */
 
-// #import {ListPropertyUpdateBehavior} from 'chrome://resources/js/list_property_update_behavior.m.js';
+// #import {ListPropertyUpdateBehavior, updateListProperty} from 'chrome://resources/js/list_property_update_behavior.m.js';
 // #import {Polymer} from 'chrome://resources/polymer/v3_0/polymer/polymer_bundled.min.js';
 
 suite('ListPropertyUpdateBehavior', function() {
@@ -76,7 +76,7 @@ suite('ListPropertyUpdateBehavior', function() {
       updateComplexArray(newArray) {
         if (this.updateList(
                 'complexArray', x => x.letter, newArray,
-                true /* uidBasedUpdate */)) {
+                true /* identityBasedUpdate */)) {
           return {topArrayChanged: true, wordsArrayChanged: false};
         }
 
@@ -305,4 +305,48 @@ suite('ListPropertyUpdateBehavior', function() {
     assertTrue(testElement.updateList('complexArray', x => x.letter, newArray));
     assertDeepEquals(['apricot'], testElement.complexArray[0].words);
   });
+
+  test('updateListProperty() function triggers notifySplices()', () => {
+    // Ensure that the array is updated when an element is removed from the
+    // end.
+    let elementRemoved = testElement.complexArray.slice(0, 2);
+    updateListProperty(
+        testElement, 'complexArray', obj => obj, elementRemoved, true);
+    assertComplexArrayEquals(testElement.complexArray, elementRemoved);
+
+    // Ensure that the array is updated when an element is removed from the
+    // beginning.
+    testElement.resetComplexArray();
+    elementRemoved = testElement.complexArray.slice(1);
+    updateListProperty(
+        testElement, 'complexArray', obj => obj, elementRemoved, true);
+    assertComplexArrayEquals(testElement.complexArray, elementRemoved);
+
+    // Ensure that the array is updated when an element is added to the end.
+    testElement.resetComplexArray();
+    let elementAdded = testElement.complexArray.slice();
+    elementAdded.push({letter: 'd', words: ['door', 'dice']});
+    updateListProperty(
+        testElement, 'complexArray', obj => obj, elementAdded, true);
+    assertComplexArrayEquals(testElement.complexArray, elementAdded);
+
+    // Ensure that the array is updated when an element is added to the
+    // beginning.
+    testElement.resetComplexArray();
+    elementAdded = [{letter: 'A', words: ['Alphabet']}];
+    elementAdded.push(...testElement.complexArray);
+    updateListProperty(
+        testElement, 'complexArray', obj => obj, elementAdded, true);
+    assertComplexArrayEquals(testElement.complexArray, elementAdded);
+
+    // Ensure that the array is updated when the entire array is different.
+    testElement.resetComplexArray();
+    const newArray = [
+      {letter: 'w', words: ['water', 'woods']},
+      {letter: 'x', words: ['xylophone']}, {letter: 'y', words: ['yo-yo']},
+      {letter: 'z', words: ['zebra', 'zephyr']}
+    ];
+    updateListProperty(testElement, 'complexArray', obj => obj, newArray, true);
+    assertComplexArrayEquals(testElement.complexArray, newArray);
+  });
 });

chrome/test/data/webui/tab_search/infinite_list_test.js

@@ -87,6 +87,7 @@ suite('InfiniteListTest', () => {
   test('ScrollHeight', async () => {
     const tabItems = sampleTabItems(sampleSiteNames());
     await setupTest(tabItems);
+    await waitAfterNextRender(infiniteList);
 
     assertEquals(0, infiniteList.scrollTop);
 
@@ -99,6 +100,28 @@ suite('InfiniteListTest', () => {
     assertEquals(tabItemHeight * tabItems.length, infiniteList.scrollHeight);
   });
 
+  test('ListUpdates', async () => {
+    let siteNames = Array.from({length: 1}, (_, i) => 'site' + (i + 1));
+    const tabItems = sampleTabItems(siteNames);
+    await setupTest(tabItems);
+    assertEquals(1, queryRows().length);
+
+    // Ensure that on updating the list with an array smaller in size
+    // than the chunkItemCount property, all the array items are rendered.
+    siteNames = Array.from({length: 3}, (_, i) => 'site' + (i + 1));
+    infiniteList.items = sampleTabItems(siteNames);
+    await waitAfterNextRender(infiniteList);
+    assertEquals(3, queryRows().length);
+
+    // Ensure that on updating the list with an array greater in size than
+    // the chunkItemCount property, only a chunk of array items are rendered.
+    siteNames =
+        Array.from({length: 2 * CHUNK_ITEM_COUNT}, (_, i) => 'site' + (i + 1));
+    infiniteList.items = sampleTabItems(siteNames);
+    await waitAfterNextRender(infiniteList);
+    assertEquals(CHUNK_ITEM_COUNT, queryRows().length);
+  });
+
   test('SelectedIndex', async () => {
     const siteNames = Array.from({length: 50}, (_, i) => 'site' + (i + 1));
     const tabItems = sampleTabItems(siteNames);

ui/webui/resources/js/list_property_update_behavior.js

@@ -21,44 +21,60 @@
 /* #export */ const ListPropertyUpdateBehavior = {
   /**
    * @param {string} propertyPath
-   * @param {function(!Object): string} itemUidGetter
+   * @param {function(!Object): (!Object|string)} identityGetter
    * @param {!Array<!Object>} updatedList
-   * @param {boolean} uidBasedUpdate
+   * @param {boolean=} identityBasedUpdate
    * @returns {boolean} True if notifySplices was called.
    */
-  updateList(propertyPath, itemUidGetter, updatedList, uidBasedUpdate = false) {
-    const list = this.get(propertyPath);
-    const splices = Polymer.ArraySplice.calculateSplices(
-        updatedList.map(itemUidGetter), list.map(itemUidGetter));
+  updateList(
+      propertyPath, identityGetter, updatedList, identityBasedUpdate = false) {
+    return updateListProperty(
+        this, propertyPath, identityGetter, updatedList, identityBasedUpdate);
+  },
+};
 
-    splices.forEach(splice => {
-      const index = splice.index;
-      const deleteCount = splice.removed.length;
-      // Transform splices to the expected format of notifySplices().
-      // Convert !Array<string> to !Array<!Object>.
-      splice.removed = list.slice(index, index + deleteCount);
-      splice.object = list;
-      splice.type = 'splice';
+/**
+ * @param {Object} instance
+ * @param {string} propertyPath
+ * @param {function(!Object): (!Object|string)} identityGetter
+ * @param {!Array<!Object>} updatedList
+ * @param {boolean=} identityBasedUpdate
+ * @returns {boolean} True if notifySplices was called.
+ */
+/* #export */ function updateListProperty(
+    instance, propertyPath, identityGetter, updatedList,
+    identityBasedUpdate = false) {
+  const list = instance.get(propertyPath);
+  const splices = Polymer.ArraySplice.calculateSplices(
+      updatedList.map(identityGetter), list.map(identityGetter));
 
-      const added = updatedList.slice(index, index + splice.addedCount);
-      const spliceParams = [index, deleteCount].concat(added);
-      list.splice.apply(list, spliceParams);
-    });
+  splices.forEach(splice => {
+    const index = splice.index;
+    const deleteCount = splice.removed.length;
+    // Transform splices to the expected format of notifySplices().
+    // Convert !Array<string> to !Array<!Object>.
+    splice.removed = list.slice(index, index + deleteCount);
+    splice.object = list;
+    splice.type = 'splice';
 
-    let updated = splices.length > 0;
-    if (!uidBasedUpdate) {
-      list.forEach((item, index) => {
-        const updatedItem = updatedList[index];
-        if (JSON.stringify(item) !== JSON.stringify(updatedItem)) {
-          this.set([propertyPath, index], updatedItem);
-          updated = true;
-        }
-      });
-    }
+    const added = updatedList.slice(index, index + splice.addedCount);
+    const spliceParams = [index, deleteCount].concat(added);
+    list.splice.apply(list, spliceParams);
+  });
 
-    if (splices.length > 0) {
-      this.notifySplices(propertyPath, splices);
-    }
-    return updated;
-  },
-};
+  let updated = splices.length > 0;
+  if (!identityBasedUpdate) {
+    list.forEach((item, index) => {
+      const updatedItem = updatedList[index];
+      if (JSON.stringify(item) !== JSON.stringify(updatedItem)) {
+        instance.set([propertyPath, index], updatedItem);
+        updated = true;
+      }
+    });
+  }
+
+  if (splices.length > 0) {
+    instance.notifySplices(propertyPath, splices);
+  }
+  return updated;
+}