Closed Bug 1312374 Opened 8 years ago Closed 7 years ago

Search sites listed in Settings of Site Data on host

Categories

(Firefox :: Settings UI, defect)

Product:
Firefox
Component:
Settings UI
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: Fischer, Assigned: Fischer)

References

Details

(Whiteboard: [storage-v1])

Attachments

(1 file)

Bug 1312374 - Search sites listed in Settings of Site Data on host,
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
Should be able to search sites listed in Settings of Site Data on host
Depends on: 1312375
No longer depends on: 1312375
Depends on: 1312377
No longer depends on: 1312377
Blocks: 1323391
No longer blocks: 1323391
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
Attachment #8824915 - Flags: review?(gijskruitbosch+bugs)
Assignee: nobody → fliu
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?
Attachment #8824915 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8824915 - Flags: review?(gijskruitbosch+bugs) → review+
(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
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c56a1f366d7b
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Blocks: 1331318
No longer blocks: 1331318
Blocks: 1340967
Blocks: 1343477
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: