I know we don't have the resources to tackle this this anytime soon, but I want to make sure it's tracked so we can tackle it once we find resources. Currently we expose site data under permissions (the persistent storage permission), but this does not make much sense, as clearing the permission doesn't actually change the abilities of the site in question. It will still have persistent data and the data won't be gone. So instead we should not expose site data under permissions, but have it under security where we also place cookies. We just need one option there: Site Data [Remove] (This same criticism applies to the "Show site information" dialog. Ideally we fix both at the same time.)

(In reply to Anne (:annevk) from comment #0) > I know we don't have the resources to tackle this this anytime soon, but I > want to make sure it's tracked so we can tackle it once we find resources. > > Currently we expose site data under permissions (the persistent storage > permission), but this does not make much sense, as clearing the permission > doesn't actually change the abilities of the site in question. It will still > have persistent data and the data won't be gone. > > So instead we should not expose site data under permissions, but have it > under security where we also place cookies. We just need one option there: > > Site Data [Remove] > > (This same criticism applies to the "Show site information" dialog. Ideally > we fix both at the same time.) Do you mean here under privacy & history? https://www.dropbox.com/s/pt0m68meeuw5eru/screen.jpg?dl=0

Yes, that seems like the logical place, especially with our larger goal of putting all storage-related items together (or even turn them into a single thing) over time. (The attachment applies to my remark in parentheses. This is where I would expect we'd also have this option to clear storage, if there's anything to clear, right below permissions for instance.)

Got another bug 1409352 reported.

In bug 1334411 we're going to go ahead and remove the "Offline Storage" section including the "Clear Storage" button, we should consider picking up this bug afterwards. I wonder if we should just link to site data settings instead of offering a button to clear storage here (we'll have UX come up with a concept for this). IMO less places to do the same thing is better and "Clear Storage" has been misleading from the start as it only clears IndexedDB, nothing else.

This is mostly done, but I'm not requesting review yet because I'm having troubles correctly integrating the removal functionality of SiteDataManager.jsm into anything other than the dialog it was originally written for. My final plan is to merge all removal functionality into Sanitizer.jsm, though I was intending to do that after writing the UI parts. I'm going to look into Sanitizer improvements first and may come back to this and just duplicate the clearing functionality while slapping a big "TODO: use Sanitizer.jsm" onto it.

(In reply to Mark Liang(:mark_liang) from comment #1) > Do you mean here under privacy & history? > https://www.dropbox.com/s/pt0m68meeuw5eru/screen.jpg?dl=0 This link doesn't seem to work anymore. (Keeping with the topic in parenthesis... Would be great to also have a section under Permissions in the Control Center where you could clear cookies and site data, thought we probably need a new bug for that.)

Comment on attachment 8961463 [details] Bug 1348223 - Part 2 - Move SiteDataManager.jsm to components and improve functionality for getting and removing by host. https://reviewboard.mozilla.org/r/230226/#review236572 ::: browser/modules/SiteDataManager.jsm:234 (Diff revision 1) > + * You need to call SiteDataManager.updateSites() before you > + * can use this method for the first time (and whenever you want > + * to get an updated set of list.) > + * > + * @param {String} [optional] baseDomain - if specified, it will > + * only remove data for sites with I don't think this method will "remove data". ::: browser/modules/SiteDataManager.jsm:243 (Diff revision 1) > + */ > + getSites(baseDomain) { > return this._getQuotaUsagePromise.then(() => { > let list = []; > for (let [host, site] of this._sites) { > + if (!baseDomain || site.baseDomain == baseDomain) { nit: I would reverse the condition and use 'continue' instead of indenting the whole block.

Comment on attachment 8943175 [details] Bug 1348223 - Part 4 - Add and update tests for removing site data and cookies in the page info window. https://reviewboard.mozilla.org/r/213508/#review236586

Comment on attachment 8961465 [details] Bug 1348223 - Part 5 - Remove the old cookie manager. https://reviewboard.mozilla.org/r/230230/#review236592 Please remove the related CSS code in browser/themes/{windows,linux}/preferences/preferences.css

Comment on attachment 8961464 [details] Bug 1348223 - Part 3 - Show and clear all site data in page info (not just cookies). https://reviewboard.mozilla.org/r/230228/#review236582 ::: browser/base/content/pageinfo/pageInfo.xul:366 (Diff revision 1) > readonly="true"/> > </row> > - <!-- Cookies --> > - <row id="security-privacy-cookies-row"> > - <label id="security-privacy-cookies-label" > - control="security-privacy-cookies-value" > + <!-- Site Data & Cookies --> > + <row id="security-privacy-sitedata-row"> > + <label id="security-privacy-sitedata-label" > + control="security-privacy-sitedata-value" I was going to suggest fixing the indent here while you are touching these lines, but there's so many <label> tags in this file that have broken indent that it may be better to do that in a good-first-bug follow-up. ::: browser/base/content/pageinfo/security.js:147 (Diff revision 1) > - var win = Services.wm.getMostRecentWindow("Browser:Cookies"); > + > + let pageInfoBundle = document.getElementById("pageinfobundle"); > + > + if (!this.siteData.length) { > + let noStr = pageInfoBundle.getString("securitySiteDataNo"); > + setText("security-privacy-sitedata-value", noStr); When setting the answer to "No" I think we should disable the button. ::: browser/base/content/pageinfo/security.js:155 (Diff revision 1) > > - var eTLD; > - try { > - eTLD = Services.eTLD.getBaseDomain(this.uri); > - } catch (e) { > - // getBaseDomain will fail if the host is an IP address or is empty > + let usage = this.siteData.reduce((acc, site) => acc + site.usage, 0); > + if (usage > 0) { > + let size = DownloadUtils.convertByteUnits(usage); > + let usageText = pageInfoBundle.getFormattedString("securitySiteDataUsage", size); > + setText("security-privacy-sitedata-value", usageText); I had a case (on https://perf-html.io) where the string remained "Unknown". No obviously related error in the browser console. This suggests you have a promise that sometimes never resolves. ::: browser/base/content/pageinfo/security.js:166 (Diff revision 1) > + }, > > - if (win) { > - win.gCookiesWindow.setFilter(eTLD); > - win.focus(); > - } else > + /** > + * Clear Site Data and Cookies > + */ > + clearSiteData() { I'm a bit worried of the risk of dataloss here, when this button is aligned with 2 other harmless buttons that just show more info. As much as I hate modal dialogs, I wonder if we should we prompt for a confirmation. ::: browser/locales/en-US/chrome/browser/pageInfo.dtd:69 (Diff revision 1) > <!ENTITY securityView.identity.verifier "Verified by:"> > <!ENTITY securityView.identity.validity "Expires on:"> > > <!ENTITY securityView.privacy.header "Privacy &amp; History"> > <!ENTITY securityView.privacy.history "Have I visited this website prior to today?"> > -<!ENTITY securityView.privacy.cookies "Is this website storing information (cookies) on my computer?"> > +<!ENTITY securityView.privacy.siteData "Is this website storing information (cookies and site data) on my computer?"> I dislike this "cookies and site data" phrasing because it somehow implies that cookies aren't part of site data, which makes me think "site data" is some jargon to mean something specific. I understand we need to keep the word "cookies" as it's what users are looking for, but I would prefer a phrasing that said "including cookies". Alternative proposal - remove the parenthesis, and use these strings: No Yes, only cookies. Yes, cookies and nnUnit of site data Yes, nnUnit of site data ::: browser/locales/en-US/chrome/browser/pageInfo.properties:64 (Diff revision 1) > # %1$S = size (in bytes or megabytes, ...) > # %2$S = unit of measure (bytes, KB, MB, ...) > -indexedDBUsage=This website is using %1$S %2$S > +securitySiteDataUsage=Yes, %1$S %2$S > +# LOCALIZATION NOTE(securitySiteDataYes,securitySiteDataNo): > +# This is for site data and cookies usage. It answers the question "Is this website using site data?" > +securitySiteDataYes=Yes I think it'll be confusing that sometimes this says the size and sometimes it doesn't.

Comment on attachment 8961462 [details] Bug 1348223 - Part 1 - Add SiteDataTestUtils.jsm. https://reviewboard.mozilla.org/r/230224/#review237434 ::: browser/base/content/test/sanitize/StorageTestUtils.jsm:4 (Diff revision 1) > +"use strict"; > + > +var EXPORTED_SYMBOLS = [ > + "StorageTestUtils", I'm honestly not thrilled by the name, for a couple reasons. Historically storage has been used to indicate mozStorage, or more in particulare an abstraction layer (like the new KV storage) and this is unrelated to that, so it may be confusing. Even forgetting that, "storage" doesn't seem to indicate it's a module to add dummy data to the profile. I agree with the general idea though, a module to add dummy profile data to components that store it differently. Maybe this could be named something after Data, like ProfileDataTestUtils, ProfileFeederTestUtils or DataFeederTestUtils? ::: browser/base/content/test/sanitize/StorageTestUtils.jsm:18 (Diff revision 1) > + * > + * @param {String} origin - the origin of the site to add test data for > + * > + * @returns a Promise that resolves when the data was added successfully. > + */ > + addIndexedDBTestData(origin) { A second naming suggestion; since this may include different methods to add different kind of data, it would be great for our memory if methods would all sound similar. For example addToIndexedDB, addToCookies, addToBookmarks. ::: browser/base/content/test/sanitize/StorageTestUtils.jsm:32 (Diff revision 1) > + request.onsuccess = function(e) { > + let db = e.target.result; > + let tx = db.transaction("TestStore", "readwrite"); > + let store = tx.objectStore("TestStore"); > + tx.oncomplete = resolve; > + store.put({ id: "test_id", description: "IndexedDB Test"}); It may be useful to either document that the consumer must manually cleanup the additions, and eventually provide an accumulator that the consumer can call at the end of the test to cleanup all of the additions. Especially if this ends up being used in mochitest-browser tests. ::: browser/base/content/test/sanitize/StorageTestUtils.jsm:47 (Diff revision 1) > + * @param {String} value - the cookie value > + */ > + addCookie(origin, name, value) { > + let uri = Services.io.newURI(origin); > + Services.cookies.add(uri.host, uri.pathQueryRef, name, value, > + false, false, false, Date.now() + 1000 * 60 * 60); it sounds dangerous when DST changes and this hour disappears nowhere. could we make this a little bit longer?

Comment on attachment 8961462 [details] Bug 1348223 - Part 1 - Add SiteDataTestUtils.jsm. https://reviewboard.mozilla.org/r/230224/#review237434 > I'm honestly not thrilled by the name, for a couple reasons. > Historically storage has been used to indicate mozStorage, or more in particulare an abstraction layer (like the new KV storage) and this is unrelated to that, so it may be confusing. Even forgetting that, "storage" doesn't seem to indicate it's a module to add dummy data to the profile. > > I agree with the general idea though, a module to add dummy profile data to components that store it differently. > > Maybe this could be named something after Data, like ProfileDataTestUtils, ProfileFeederTestUtils or DataFeederTestUtils? I changed it to SiteDataTestUtils. I think I don't want this module to be a profile-data-filler module, but rather a helper for testing storage that is manageable by websites, e.g. cookies and quota storage (in contrast to PlacesTestUtils which IMO is responsible for data the user can set). > A second naming suggestion; since this may include different methods to add different kind of data, it would be great for our memory if methods would all sound similar. For example addToIndexedDB, addToCookies, addToBookmarks. I think it's fine as long as it starts with "add", but I changed it (and changed the addToIndexedDB function to be a little more generic and and fill indexedDB with more data on each call). > It may be useful to either document that the consumer must manually cleanup the additions, and eventually provide an accumulator that the consumer can call at the end of the test to cleanup all of the additions. Especially if this ends up being used in mochitest-browser tests. Added a helper function for that. > it sounds dangerous when DST changes and this hour disappears nowhere. could we make this a little bit longer? Good idea!

Comment on attachment 8961464 [details] Bug 1348223 - Part 3 - Show and clear all site data in page info (not just cookies). https://reviewboard.mozilla.org/r/230228/#review236582 > When setting the answer to "No" I think we should disable the button. Yeah, that seems like a good idea. > I had a case (on https://perf-html.io) where the string remained "Unknown". No obviously related error in the browser console. This suggests you have a promise that sometimes never resolves. I was unfortunately not able to reproduce that, so in the interest of moving forward I'd prefer to solve it separately, whenever we can come up with a way to reproduce it reliably... :/ > I'm a bit worried of the risk of dataloss here, when this button is aligned with 2 other harmless buttons that just show more info. As much as I hate modal dialogs, I wonder if we should we prompt for a confirmation. Good idea. I reused the one that already shows the hosts to remove in the site data manager in about:preferences, which is unfortunately kinda broken right now because of bug 1447056. You can still use enter and escape keys to confirm or cancel the dialog for testing, though. > I dislike this "cookies and site data" phrasing because it somehow implies that cookies aren't part of site data, which makes me think "site data" is some jargon to mean something specific. I understand we need to keep the word "cookies" as it's what users are looking for, but I would prefer a phrasing that said "including cookies". > > Alternative proposal - remove the parenthesis, and use these strings: > No > Yes, only cookies. > Yes, cookies and nnUnit of site data > Yes, nnUnit of site data Yeah, I changed it to mention cookies more explicitly, as you suggested. Unfortunately this makes the label pretty variable in length which causes a bit of jumping when clearing site data, I'm not sure how to best solve that (setting textContent instead of value already mitigated it a bit).

Comment on attachment 8961465 [details] Bug 1348223 - Part 5 - Remove the old cookie manager. https://reviewboard.mozilla.org/r/230230/#review239812

Comment on attachment 8961464 [details] Bug 1348223 - Part 3 - Show and clear all site data in page info (not just cookies). https://reviewboard.mozilla.org/r/230228/#review239820 I feel much better about this new version of the patch, thanks for the update!

Pushed by jhofmann@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7390643a0bc1 Part 1 - Add SiteDataTestUtils.jsm. r=mak https://hg.mozilla.org/integration/autoland/rev/e4715e0cc3fe Part 2 - Move SiteDataManager.jsm to components and improve functionality for getting and removing by host. r=florian https://hg.mozilla.org/integration/autoland/rev/8dec8e29532e Part 3 - Show and clear all site data in page info (not just cookies). r=florian https://hg.mozilla.org/integration/autoland/rev/0f1aa8fea370 Part 4 - Add and update tests for removing site data and cookies in the page info window. r=florian https://hg.mozilla.org/integration/autoland/rev/2be604b3a338 Part 5 - Remove the old cookie manager. r=florian

https://hg.mozilla.org/mozilla-central/rev/7390643a0bc1 https://hg.mozilla.org/mozilla-central/rev/e4715e0cc3fe https://hg.mozilla.org/mozilla-central/rev/8dec8e29532e https://hg.mozilla.org/mozilla-central/rev/0f1aa8fea370 https://hg.mozilla.org/mozilla-central/rev/2be604b3a338

It looks like these changes broke selective deletion of cookies. See bug 1457935.