Closed Bug 1312380 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Preferences, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 54
Tracking Status
firefox54 --- verified

People

(Reporter: Fischer, Assigned: Fischer)

References

Details

(Whiteboard: [storage-v1])

Attachments

(1 file)

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.
No longer depends on: 1312377
(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
Summary: Should be able to remove all sites data in Settings of Site Data → Should be able to remove data of all sites visible on the list in Settings of Site Data
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
Attachment #8833240 - Flags: review?(jaws)
Assignee: nobody → fliu
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.
Attachment #8833240 - Flags: review?(jaws) → review-
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.
Attachment #8833240 - Flags: review?(jaws)
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;
Attachment #8833240 - Flags: review?(jaws) → review+
(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.
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c2209ae25f0b
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Blocks: 1338036
No longer blocks: 1338036
Blocks: 1338036
[bugday-20170301]
You need to log in before you can comment on or make changes to this bug.