Should be able to search sites listed in Settings of Site Data on host

Comment on attachment 8824915 [details] Bug 1312374 - Search sites listed in Settings of Site Data on host, Gijs, This patch implements the search textbox in [1]; User can search sites by hostname. One test case is added as well. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd18f6c0730335187bcade5773c3f46169e2ccb1 [1] https://mozilla.invisionapp.com/share/4Y87EJO39#/screens/179637924 Thanks

Comment on attachment 8824915 [details] Bug 1312374 - Search sites listed in Settings of Site Data on host, https://reviewboard.mozilla.org/r/103252/#review103818 ::: browser/components/preferences/in-content/tests/browser_advanced_siteData.js:350 (Diff revision 1) > + let mockOrigins = []; > + mockSiteDataManager.sites.forEach((data, origin) => mockOrigins.push(origin)); ```js let mockOrigins = Array.from(mockSiteDataManager.sites.values()); ``` ::: browser/components/preferences/siteDataSettings.js:98 (Diff revision 1) > }, > > - _buildSitesList(sites) { > + /** > + * @param sites {Array} array of metadata of sites > + * @param keyword {String} Optional. > + * If given, only sites which its host contains the keyword would be displayed English nit: please replace "which its" with "whose". ::: browser/components/preferences/siteDataSettings.js:102 (Diff revision 1) > while (this._list.childNodes.length > 1) { > this._list.removeChild(this._list.lastChild); > } Driveby, but did I review this? If so, I must not have paid attention... Fetching childNodes.length is very inefficient, and you also don't need `removeChild` Instead: ```js while(this._list.hasChildNodes()) { this._list.lastChild.remove(); } ``` ::: browser/components/preferences/siteDataSettings.js:109 (Diff revision 1) > } > > let prefStrBundle = document.getElementById("bundlePreferences"); > for (let data of sites) { > + let host = data.uri.host; > + if (keyword && host.indexOf(keyword) == -1) { Nit: ```js if (keyword && !host.includes(keyword)) { ... } ``` would be more idiomatic in modern JS. We should also ignore leading/trailing whitespace in keywords, so we should probably use `keyword.trim()`, but see below - all of that can probably go at the top of this method instead of taking a parameter. ::: browser/components/preferences/siteDataSettings.js:125 (Diff revision 1) > this._list.appendChild(item); > } > }, > > onClickTreeCol(e) { > + let keyword = this._searchBox.value.toLowerCase(); Is the host guaranteed to be lowercase? ::: browser/components/preferences/siteDataSettings.js:131 (Diff revision 1) > this._sortSites(this._sites, e.target); > - this._buildSitesList(this._sites); > + this._buildSitesList(this._sites, keyword); > + }, > + > + onCommandSearch() { > + let keyword = this._searchBox.value.toLowerCase(); We always fetch the keyword the same way, so fetching it should probably go into the `_buildSitesList` method, and we shouldn't have a parameter. ::: browser/components/preferences/siteDataSettings.xul:29 (Diff revision 1) > > <vbox flex="1"> > <description>&settings.description;</description> > <separator class="thin"/> > > + <hbox align="center"> I'd expect the alignment to be in the CSS file - use -moz-box-align: center for the equivalent effect. Is it really necessary here?

Comment on attachment 8824915 [details] Bug 1312374 - Search sites listed in Settings of Site Data on host, https://reviewboard.mozilla.org/r/103252/#review103818 > ```js > let mockOrigins = Array.from(mockSiteDataManager.sites.values()); > ``` Updated, thanks > Driveby, but did I review this? If so, I must not have paid attention... Fetching childNodes.length is very inefficient, and you also don't need `removeChild` Instead: > > ```js > while(this._list.hasChildNodes()) { > this._list.lastChild.remove(); > } > ``` Here checks "this._list.childNodes.length > 1" to avoid removing the list haeders. Originally I though caching the length such as "let count = this._list.childNodes.length - 1" but this way is implicit. Then the length of HTMLCollection returned by getElementsByTagName would change during removal operation, have to do some extra handling. So in the end, using NodeList returned by querySelectorAll for removal operation, which is more explict and no concern about live update of length. Please just let me know if any better or more expressive way, thanks. > Nit: > > ```js > if (keyword && !host.includes(keyword)) { > ... > } > ``` > > would be more idiomatic in modern JS. > > We should also ignore leading/trailing whitespace in keywords, so we should probably use `keyword.trim()`, but see below - all of that can probably go at the top of this method instead of taking a parameter. Both here and the test are updated, thanks > Is the host guaranteed to be lowercase? Yes > We always fetch the keyword the same way, so fetching it should probably go into the `_buildSitesList` method, and we shouldn't have a parameter. Updated, thanks > I'd expect the alignment to be in the CSS file - use -moz-box-align: center for the equivalent effect. Is it really necessary here? OK, move into the CSS file, thanks

(In reply to Fischer [:Fischer] from comment #5) > > Driveby, but did I review this? If so, I must not have paid attention... Fetching childNodes.length is very inefficient, and you also don't need `removeChild` Instead: > > > > ```js > > while(this._list.hasChildNodes()) { > > this._list.lastChild.remove(); > > } > > ``` > > Here checks "this._list.childNodes.length > 1" to avoid removing the list > haeders. > Originally I though caching the length such as "let count = > this._list.childNodes.length - 1" but this way is implicit. > Then the length of HTMLCollection returned by getElementsByTagName would > change during removal operation, have to do some extra handling. > So in the end, using NodeList returned by querySelectorAll for removal > operation, which is more explict and no concern about live update of length. > Please just let me know if any better or more expressive way, thanks. Oh ugh, I missed that, sorry. You could get the list header and see if it has a nextSibling, and if so remove that? Will look at the rest of the patch in a bit. Thanks for clarifying. :-)

Comment on attachment 8824915 [details] Bug 1312374 - Search sites listed in Settings of Site Data on host, https://reviewboard.mozilla.org/r/103252/#review104220 ::: browser/components/preferences/siteDataSettings.js:100 (Diff revisions 1 - 2) > - while (this._list.childNodes.length > 1) { > - this._list.removeChild(this._list.lastChild); > + let oldItems = this._list.querySelectorAll("richlistitem"); > + for (let item of oldItems) { > + item.remove(); > } As noted on the bug, this or finding the header container and then progressively removing their nextSibling until none are left.

(In reply to :Gijs from comment #7) > > + let oldItems = this._list.querySelectorAll("richlistitem"); > > + for (let item of oldItems) { > > + item.remove(); > > } > > As noted on the bug, this or finding the header container and then > progressively removing their nextSibling until none are left. Thanks. OK, let's go `querySelectorAll` because it explicitly expresses "Removing" <richlistitem>s

TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf9779c3490292c497655aa9a7160885f510d9a4

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/c56a1f366d7b Search sites listed in Settings of Site Data on host, r=Gijs