Skip to content

Commit

Permalink
MD Extensions: Never reset the array of apps/extensions dom-repeat.
Browse files Browse the repository at this point in the history
Using dom-repeat's filter functionality to implement filtering, as opposed
to manually resetting the array. As a result:

 - The list of apps/extensions is not unnecessarily re-rendered when any item
   is updated (for example enabled/disabled). Only the modified item is updated.
 - Allows using dom-repeat's chunked rendering (initial-count) without triggering
   accidental re-renders.

This change improves FMP by about 12% on my machine (with 30 extensions+apps).

Bug: 782514
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: Ica14de4d71442b9980dcf3290eb8e9caaf5772af
Reviewed-on: https://chromium-review.googlesource.com/759739
Reviewed-by: Dave Schuyler <dschuyler@chromium.org>
Commit-Queue: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515619}
  • Loading branch information
freshp86 authored and Commit Bot committed Nov 10, 2017
1 parent b915363 commit deb837b
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 29 deletions.
34 changes: 21 additions & 13 deletions chrome/browser/resources/md_extensions/item_list.html
Original file line number Diff line number Diff line change
Expand Up @@ -61,30 +61,38 @@
</div>
<div id="no-search-results" class="empty-list-message"
hidden$="[[!shouldShowEmptySearchMessage_(
shownApps_.length, shownExtensions_.length)]]">
shownAppsCount_, shownExtensionsCount_, apps, extensions)]]">
<span>$i18n{noSearchResults}</span>
</div>
<template is="dom-if" if="[[shownExtensions_.length]]">
<div class="items-container">
<template is="dom-repeat" items="[[shownExtensions_]]">
<extensions-item data="[[item]]" delegate="[[delegate]]"
in-dev-mode="[[inDevMode]]">
</extensions-item>
</template>
</div>
</template>
<template is="dom-if" if="[[shownApps_.length]]">
<div class="items-container" hidden="[[!shownExtensionsCount_]]">
<!-- Render only a few items first, to improve initial render time, then
render the remaining items on a different frame. Value of 3 was chosen
by experimentation, and it is a good tradeoff between initial render
time and total render time. -->
<template is="dom-repeat" items="[[extensions]]" initial-count="3"
filter="[[computeFilter_(filter)]]"
rendered-item-count="{{shownExtensionsCount_::dom-change}}"
notify-dom-change>
<extensions-item data="[[item]]" delegate="[[delegate]]"
in-dev-mode="[[inDevMode]]">
</extensions-item>
</template>
</div>
<div hidden="[[!shownAppsCount_]]">
<!-- app-title needs to left-align with the grid content below, and
the easiest way to achieve this is to make it a grid as well. -->
<h1 id="app-title" class="items-container">$i18n{appsTitle}</h1>
<div class="items-container">
<template is="dom-repeat" items="[[shownApps_]]">
<template is="dom-repeat" items="[[apps]]" initial-count="3"
filter="[[computeFilter_(filter)]]"
rendered-item-count="{{shownAppsCount_::dom-change}}"
notify-dom-change>
<extensions-item data="[[item]]" delegate="[[delegate]]"
in-dev-mode="[[inDevMode]]">
</extensions-item>
</template>
</div>
</template>
</div>
</div>
</template>
<script src="item_list.js"></script>
Expand Down
34 changes: 18 additions & 16 deletions chrome/browser/resources/md_extensions/item_list.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,29 +27,31 @@ cr.define('extensions', function() {

filter: String,

/** @private {!Array<!chrome.developerPrivate.ExtensionInfo>} */
shownApps_: {
type: Array,
computed: 'computeShownItems_(apps, apps.*, filter)',
/** @private */
shownExtensionsCount_: {
type: Number,
value: 0,
},

/** @private {!Array<!chrome.developerPrivate.ExtensionInfo>} */
shownExtensions_: {
type: Array,
computed: 'computeShownItems_(extensions, extensions.*, filter)',
}
/** @private */
shownAppsCount_: {
type: Number,
value: 0,
},
},

/**
* Computes the list of items to be shown.
* @param {!Array<!chrome.developerPrivate.ExtensionInfo>} items
* @return {!Array<!chrome.developerPrivate.ExtensionInfo>}
* Computes the filter function to be used for determining which items
* should be shown. A |null| value indicates that everything should be
* shown.
* return {?Function}
* @private
*/
computeShownItems_: function(items) {
computeFilter_: function() {
const formattedFilter = this.filter.trim().toLowerCase();
return items.filter(
item => item.name.toLowerCase().includes(formattedFilter));
return formattedFilter ?
i => i.name.toLowerCase().includes(formattedFilter) :
null;
},

/** @private */
Expand All @@ -61,7 +63,7 @@ cr.define('extensions', function() {
/** @private */
shouldShowEmptySearchMessage_: function() {
return !this.isGuest && !this.shouldShowEmptyItemsMessage_() &&
this.shownApps_.length === 0 && this.shownExtensions_.length === 0;
this.shownAppsCount_ === 0 && this.shownExtensionsCount_ === 0;
},
});

Expand Down
4 changes: 4 additions & 0 deletions chrome/test/data/webui/extensions/extension_item_list_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,20 +78,24 @@ cr.define('extension_item_list_tests', function() {
});

test(assert(TestNames.NoItemsMsg), function() {
Polymer.dom.flush();
testVisible('#no-items', false);
testVisible('#no-search-results', false);

itemList.extensions = [];
itemList.apps = [];
Polymer.dom.flush();
testVisible('#no-items', true);
testVisible('#no-search-results', false);
});

test(assert(TestNames.NoSearchResultsMsg), function() {
Polymer.dom.flush();
testVisible('#no-items', false);
testVisible('#no-search-results', false);

itemList.filter = 'non-existent name';
Polymer.dom.flush();
testVisible('#no-items', false);
testVisible('#no-search-results', true);
});
Expand Down

0 comments on commit deb837b

Please sign in to comment.