1312380 - Should be able to remove data of all sites visible on the list in Settings of Site Data

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Fischer [:Fischer]]

Should be able to remove all sites data in Settings of Site Data.
Once user confirms to remove, the following items should be removed for all sites:
 - persistent permission
 - cookies
 - HTTP Cache
 - appcache
 - IndexedDB
 - localStorage
and the site list should be cleared.

Comment 1

[Fischer [:Fischer]]

(In reply to Fischer [:Fischer] from comment #0)
Update the descriptions:
----------------------------------------------------------
Should be able to remove data of all sites visible on the list in Settings of Site Data.
Once user confirms to remove, the following items should be removed:
 - persistent permission
 - cookies
 - HTTP Cache
 - appcache
 - IndexedDB
 - localStorage

Comment 2

[Fischer [:Fischer]]

Attachment: Bug 1312380 - Should be able to remove data of all sites visible on the list in Settings of Site Data,

Review commit: https://reviewboard.mozilla.org/r/108456/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/108456/

Comment 3

[Fischer [:Fischer]]

Comment on attachment 8833240 [details]
Bug 1312380 - Should be able to remove data of all sites visible on the list in Settings of Site Data,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/108456/diff/1-2/

Comment 4

[Fischer [:Fischer]]

Comment on attachment 8833240 [details]
Bug 1312380 - Should be able to remove data of all sites visible on the list in Settings of Site Data,

Jaws,

This patch implements the “Remove All” button in Settings dialog of Site Data.

The removal rules are as below:
- remove sites listed on the list. Don’t remove sites filtered out by search keyword.
- the button label is “Remove All” when not searching by keyword [1]
- the button label is “Remove All Shown” when searching by keyword [2]

One test case is added and try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e808d59ef79dde1570052c97ab42d621a0ed0401


[1] https://mozilla.invisionapp.com/share/4Y87EJO39#/screens/181229584
[2] https://mozilla.invisionapp.com/share/4Y87EJO39#/screens/213203217

Thanks

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8833240 [details]
Bug 1312380 - Should be able to remove data of all sites visible on the list in Settings of Site Data,

https://reviewboard.mozilla.org/r/108456/#review111144

We shouldn't change the "Remove All" button to "Remove All Shown" because we don't do that for any of the other dialogs where searching is implemented like this (see the Cookies dialog for example).

Please move that change out to a separate bug where the change is made for all dialogs so we don't introduce new inconsistencies.

::: browser/components/preferences/in-content/tests/browser_advanced_siteData.js:200
(Diff revision 2)
> +  let sitesList = frameDoc.getElementById("sitesList");
> +  let totalSitesNumber = sitesList.getElementsByTagName("richlistitem").length;
> +  is(totalSitesNumber, origins.length, "Should list the right sites number");
> +  origins.forEach(origin => {
> +    let site = sitesList.querySelector(`richlistitem[data-origin="${origin}"]`);
> +    ok(site instanceof XULElement, `Should list the site of ${origin}`);

This should check what the visible text of the richlistitem is.

Comment 6

[Fischer [:Fischer]]

Comment on attachment 8833240 [details]
Bug 1312380 - Should be able to remove data of all sites visible on the list in Settings of Site Data,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/108456/diff/2-3/

Comment 7

[Fischer [:Fischer]]

Comment on attachment 8833240 [details]
Bug 1312380 - Should be able to remove data of all sites visible on the list in Settings of Site Data,

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> Comment on attachment 8833240 [details]
> 
> We shouldn't change the "Remove All" button to "Remove All Shown" because we
> don't do that for any of the other dialogs where searching is implemented
> like this (see the Cookies dialog for example).
> 
> Please move that change out to a separate bug where the change is made for
> all dialogs so we don't introduce new inconsistencies.
> 
OK, will do this "Remove All"-to-"Remove All Shown" in another bug.
Just check out about:preferences, there are "Saved Logins" dialog and Cookies dialog to do as well.

> :::
> browser/components/preferences/in-content/tests/browser_advanced_siteData.js:
> 200
> (Diff revision 2)
> > +    ok(site instanceof XULElement, `Should list the site of ${origin}`);
> 
> This should check what the visible text of the richlistitem is.
Updated to check if displayed host text matches target origin, thanks.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8833240 [details]
Bug 1312380 - Should be able to remove data of all sites visible on the list in Settings of Site Data,

https://reviewboard.mozilla.org/r/108456/#review111596

::: browser/components/preferences/siteDataSettings.js:60
(Diff revision 3)
>  
>    _updateButtonsState() {
>      let items = this._list.getElementsByTagName("richlistitem");
> -    let removeBtn = document.getElementById("removeSelected");
> -    removeBtn.disabled = !(items.length > 0);
> +    let removeSelectedBtn = document.getElementById("removeSelected");
> +    let removeAllBtn = document.getElementById("removeAll");
> +    removeSelectedBtn.disabled = !(items.length > 0);

Please change this to:
removeSelectedBtn.disabled = items.length == 0;

Comment 9

[Fischer [:Fischer]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> Comment on attachment 8833240 [details]
> Bug 1312380 - Should be able to remove data of all sites visible on the list
> in Settings of Site Data
> 
> https://reviewboard.mozilla.org/r/108456/#review111596
> 
> ::: browser/components/preferences/siteDataSettings.js:60
> (Diff revision 3)
> > +    removeSelectedBtn.disabled = !(items.length > 0);
> 
> Please change this to:
> removeSelectedBtn.disabled = items.length == 0;
OK, will update, thanks.

Comment 10

[Fischer [:Fischer]]

Comment on attachment 8833240 [details]
Bug 1312380 - Should be able to remove data of all sites visible on the list in Settings of Site Data,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/108456/diff/3-4/

Comment 11

[Fischer [:Fischer]]

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c2209ae25f0b
Should be able to remove data of all sites visible on the list in Settings of Site Data, r=jaws

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c2209ae25f0b

Comment 14

[Surentharan]

[bugday-20170301]