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

This reverts commit 339975cc.

Reason for revert: Search filter in Tab Search dialog not working as expected with this change. 

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}

TBR=dpapad@chromium.org,aee@chromium.org,chromium-scoped@luci-project-accounts.iam.gserviceaccount.com,tluk@chromium.org,romanarora@chromium.org

Change-Id: Ic4e8289e6ade648a8a006dad3b5310443e4c8c4c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1149400
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2622721Reviewed-by: Roman Arora <romanarora@chromium.org>
Commit-Queue: Roman Arora <romanarora@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842582}

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

@@ -197,7 +197,7 @@ Polymer({
 
     if (!this.updateList(
             'chooserExceptions', x => x.displayName, exceptions,
-            true /* identityBasedUpdate= */)) {
+            true /* uidBasedUpdate */)) {
       // 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,7 +182,6 @@ 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,9 +40,10 @@ export class TabSearchAppElement extends PolymerElement {
         value: '',
       },
 
-      /** @private {?Array<!TabData>}*/
+      /** @private {?Array<!WindowTabs>} */
       openTabs_: {
         type: Array,
+        observer: 'openTabsChanged_',
       },
 
       /** @private {!Array<!TabData>} */
@@ -168,7 +169,7 @@ export class TabSearchAppElement extends PolymerElement {
           'Tabs.TabSearch.WebUI.TabListDataReceived',
           Math.round(Date.now() - getTabsStartTimestamp));
 
-      this.openTabsChanged_(profileTabs.windows);
+      this.openTabs_ = profileTabs.windows;
     });
   }
 
@@ -177,13 +178,19 @@ export class TabSearchAppElement extends PolymerElement {
    * @private
    */
   onTabUpdated_(updatedTab) {
-    // 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;
+    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;
+          }
+        }
       }
     }
   }
@@ -193,21 +200,14 @@ export class TabSearchAppElement extends PolymerElement {
    * @private
    */
   onTabsRemoved_(tabIds) {
-    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);
+    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)));
       }
+      this.openTabs_ = windows.concat();
     }
-
-    this.filteredOpenTabs_ =
-        this.filteredOpenTabs_.filter(tabData => !ids.has(tabData.tab.tabId));
   }
 
   /**
@@ -304,17 +304,11 @@ export class TabSearchAppElement extends PolymerElement {
   }
 
   /**
-   * @param {!Array<!WindowTabs>} newOpenWindowTabs
+   * @param {!Array<!WindowTabs>} newOpenTabs
    * @private
    */
-  openTabsChanged_(newOpenWindowTabs) {
-    this.openTabs_ = [];
-    newOpenWindowTabs.forEach(({active, tabs}) => {
-      tabs.forEach(tab => {
-        this.openTabs_.push(this.tabData_(tab, active));
-      });
-    });
-    this.updateFilteredTabs_(this.openTabs_);
+  openTabsChanged_(newOpenTabs) {
+    this.updateFilteredTabs_(newOpenTabs);
 
     // If there was no previously selected index, set the first item as
     // selected; else retain the currently selected index. If the list
@@ -421,22 +415,19 @@ export class TabSearchAppElement extends PolymerElement {
   }
 
   /**
-   * @param {!Tab} tab
-   * @param {boolean} inActiveWindow
-   * @return {!TabData}
+   * @param {!Array<!WindowTabs>} windowTabs
    * @private
    */
-  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) => {
+  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) => {
       // Move the active tab to the bottom of the list
       // because it's not likely users want to click on it.
       if (this.moveActiveTabToBottom_) {
@@ -453,9 +444,8 @@ export class TabSearchAppElement extends PolymerElement {
               a.tab.lastActiveTimeTicks.internalValue) :
           0;
     });
-
     this.filteredOpenTabs_ =
-        fuzzySearch(this.searchText_, tabs, this.fuzzySearchOptions_);
+        fuzzySearch(this.searchText_, result, this.fuzzySearchOptions_);
     this.searchResultText_ = this.getA11ySearchResultText_();
   }
 

chrome/browser/resources/tab_search/fuzzy_search.js

@@ -11,12 +11,11 @@ import {TabData} from './tab_data.js';
  * @param {string} input
  * @param {!Array<!TabData>} records
  * @param {!Object} options
- * @return {!Array<!TabData>} A new array of entries satisfying the input. If no
- *     search input is present, returns a shallow copy of the records.
+ * @return {!Array<!TabData>}
  */
 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,6 +9,7 @@
 </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,7 +20,6 @@
 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';
 
@@ -93,9 +92,9 @@ export class InfiniteList extends PolymerElement {
 
   /** @private */
   getDomItems_() {
-    const selector = /** @type {!IronSelectorElement} */ (this.$.selector);
+    const selectorChildren = this.$.selector.children;
     return Array.prototype.slice.call(
-        selector.children, 0, selector.children.length - 1);
+        selectorChildren, 0, selectorChildren.length - 1);
   }
 
   /**
@@ -156,7 +155,6 @@ export class InfiniteList extends PolymerElement {
       if (aboveScrollTopItemCount + this.chunkItemThreshold >
           this.domRepeat_.items.length) {
         this.ensureDomItemsAvailableStartingAt_(aboveScrollTopItemCount);
-        this.updateScrollerSize_();
       }
     }
   }
@@ -191,13 +189,12 @@ export class InfiniteList extends PolymerElement {
    * @private
    */
   domItemAverageHeight_() {
-    const selector = /** @type {!IronSelectorElement} */ (this.$.selector);
-    if (!selector.items || selector.items.length === 0) {
+    if (!this.$.selector.items || this.$.selector.items.length === 0) {
       return 0;
     }
 
-    const domItemCount = selector.items.length;
-    const lastDomItem = selector.items[domItemCount - 1];
+    const domItemCount = this.$.selector.items.length;
+    const lastDomItem = this.$.selector.items[domItemCount - 1];
     return (lastDomItem.offsetTop + lastDomItem.offsetHeight) / domItemCount;
   }
 
@@ -205,32 +202,17 @@ 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_(newItems, oldItems) {
-    if (!this.domRepeat_) {
-      return;
-    }
+  onItemsChanged_() {
+    if (this.domRepeat_ && this.items) {
+      const domItemAvgHeight = this.domItemAverageHeight_();
+      const aboveScrollTopItemCount = domItemAvgHeight !== 0 ?
+          Math.round(this.scrollTop / domItemAvgHeight) :
+          0;
 
-    if (!oldItems || oldItems.length === 0) {
       this.domRepeat_.set('items', []);
-      this.ensureDomItemsAvailableStartingAt_(0);
-      listenOnce(this.$.selector, 'iron-items-changed', () => {
-        this.updateScrollerSize_();
-      });
-
-      return;
-    }
-
-    updateListProperty(
-        this.domRepeat_, 'items', tabData => tabData,
-        newItems.slice(0, this.domRepeat_.items.length),
-        true /* identityBasedUpdate= */);
-
-    if (newItems.length !== oldItems.length) {
+      this.ensureDomItemsAvailableStartingAt_(aboveScrollTopItemCount);
       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, updateListProperty} from 'chrome://resources/js/list_property_update_behavior.m.js';
+// #import {ListPropertyUpdateBehavior} 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 /* identityBasedUpdate */)) {
+                true /* uidBasedUpdate */)) {
           return {topArrayChanged: true, wordsArrayChanged: false};
         }
 
@@ -305,48 +305,4 @@ 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,7 +87,6 @@ suite('InfiniteListTest', () => {
   test('ScrollHeight', async () => {
     const tabItems = sampleTabItems(sampleSiteNames());
     await setupTest(tabItems);
-    await waitAfterNextRender(infiniteList);
 
     assertEquals(0, infiniteList.scrollTop);
 

ui/webui/resources/js/list_property_update_behavior.js

@@ -21,60 +21,44 @@
 /* #export */ const ListPropertyUpdateBehavior = {
   /**
    * @param {string} propertyPath
-   * @param {function(!Object): (!Object|string)} identityGetter
+   * @param {function(!Object): string} itemUidGetter
    * @param {!Array<!Object>} updatedList
-   * @param {boolean=} identityBasedUpdate
+   * @param {boolean} uidBasedUpdate
    * @returns {boolean} True if notifySplices was called.
    */
-  updateList(
-      propertyPath, identityGetter, updatedList, identityBasedUpdate = false) {
-    return updateListProperty(
-        this, propertyPath, identityGetter, updatedList, identityBasedUpdate);
-  },
-};
-
-/**
- * @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));
+  updateList(propertyPath, itemUidGetter, updatedList, uidBasedUpdate = false) {
+    const list = this.get(propertyPath);
+    const splices = Polymer.ArraySplice.calculateSplices(
+        updatedList.map(itemUidGetter), list.map(itemUidGetter));
 
-  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';
+    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';
 
-    const added = updatedList.slice(index, index + splice.addedCount);
-    const spliceParams = [index, deleteCount].concat(added);
-    list.splice.apply(list, spliceParams);
-  });
-
-  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;
-      }
+      const added = updatedList.slice(index, index + splice.addedCount);
+      const spliceParams = [index, deleteCount].concat(added);
+      list.splice.apply(list, spliceParams);
     });
-  }
 
-  if (splices.length > 0) {
-    instance.notifySplices(propertyPath, splices);
-  }
-  return updated;
-}
+    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;
+        }
+      });
+    }
+
+    if (splices.length > 0) {
+      this.notifySplices(propertyPath, splices);
+    }
+    return updated;
+  },
+};