Add Site Data section into Network of Advanced of about:preferences for storage management. This section should: - display total storage usage of sites - carry the "learn more" link - one "Clear All Data" button to trigger data clearing - one "Settings" button to open detail settings sub-dialog

Comment on attachment 8807509 [details] Bug 1313003 - Add Site Data section into Network of Advanced of about:preferences, @Jaws, This patch adds the "Site Data" section into about:preferences > Advanced > Network [1]. - Align the pref "browser.storageManager.enabled" with the pref "dom.storageManager.enabled". More convenient when searching about:config. - The "Learn more" link will be added in the bug 1313602 - The "Clear All Data" button will be added in the bug 1312361. - The "Settings" button will be added in the bug 1312372 - The SiteDataManageer.jsm is created because many functions are going to be shared with the Settings subdialog later. [1] https://mozilla.invisionapp.com/share/4Y87EJO39#/screens/179637920

Comment on attachment 8807509 [details] Bug 1313003 - Add Site Data section into Network of Advanced of about:preferences, https://reviewboard.mozilla.org/r/90630/#review92102 Looks pretty good with the exception of the items mentioned below. I'd like to see it again with the changes before granting r+ ::: browser/components/preferences/SiteDataManager.jsm:20 (Diff revision 1) > + > + _diskCache: Services.cache2.diskCacheStorage(Services.loadContextInfo.default, false), > + > + _appCache: Cc["@mozilla.org/network/application-cache-service;1"].getService(Ci.nsIApplicationCacheService), > + > + // A Map of sites using the persistent-storage API (have requested persitent-storage permission) typo, persistent-storage, not persitent-storage. ::: browser/components/preferences/SiteDataManager.jsm:29 (Diff revision 1) > + // - status: the permission granted/rejected status > + // - quotaUsage: the usage of indexedDB and localStorage. > + // - appCacheList: an array of app cache; instances of nsIApplicationCache > + // - diskCacheList: an array. Each element is object holding metadata of http cache: > + // - dataSize: that http cache size > + // - IdEnhance: the id extension of that http cache 'idEnhance' please, to match the camelCase of the other variable names. ::: browser/components/preferences/SiteDataManager.jsm:42 (Diff revision 1) > + this._sites = new Map(); > + > + // Collect sites granted/rejected with the persistent-storage permission > + var perm = null; > + var status = null; > + var e = Services.perms.enumerator; use 'let' instead of 'var' for all new code, here and elsewhere in your patch. ::: browser/components/preferences/SiteDataManager.jsm:64 (Diff revision 1) > + this._updateAppCache(); > + this._updateDiskCache(); > + }, > + > + _updateQuota() { > + this._updateQuotaPromise = new Promise(res => { please rename 'res' to 'resolve' ::: browser/components/preferences/SiteDataManager.jsm:66 (Diff revision 1) > + let count = this._sites.size; > + // Start collecting usage for each site > + this._sites.forEach(site => { > + let callback = { > + onUsageResult: function (request) { > + site.quotaUsage = request.usage; > + count--; > + if (!count) { > + res(); // Resolve once no more site left Please remove the count variable and refactor the code to create an array of promises, and then return 'Promise.all(promises)' ::: browser/components/preferences/SiteDataManager.jsm:81 (Diff revision 1) > + } > + }; > + // XXX: The work of integrating localStorage into Quota Manager is in progress. > + // After the bug 742822 and 1286798 landed, localStorage usage will be included. > + // So currently only get indexedDB usage. > + this._qms.getUsageForPrincipal(site.perm.principal, callback); The IDL for this function states that its return value must be used. This can be used to cancel the request. We can cache these return values and add a new function that can cancel their request if some other action happens, such as navigating away from the preferences. ::: browser/components/preferences/SiteDataManager.jsm:103 (Diff revision 1) > + } > + }); > + }, > + > + _updateDiskCache() { > + this._updateDiskCachePromise = new Promise(res => { rename 'res' to 'resolve' ::: browser/components/preferences/SiteDataManager.jsm:112 (Diff revision 1) > + onCacheEntryInfo: function (uri, IdEnhance, dataSize) { > + for (let [key, site] of sites) { > + if (site.perm.matchesURI(uri, true)) { > + site.diskCacheList.push({ > + dataSize: dataSize, > + IdEnhance: IdEnhance You don't need to repeat the variable if the name of the variable is the same as the name you want it to have in the object. For example: site.diskCacheList.push({ dataSize, idEnhance }); ::: browser/components/preferences/SiteDataManager.jsm:135 (Diff revision 1) > + site.appCacheList.forEach(cache => usage += cache.usage); > + site.diskCacheList.forEach(cache => usage += cache.dataSize); Please use for..of instead of forEach since for..of won't require new functions to be created and called. ::: browser/components/preferences/in-content/advanced.js:346 (Diff revision 1) > + SiteDataManager.getTotalUsage() > + .then(usage => { > + var size = DownloadUtils.convertByteUnits(usage); > + var prefStrBundle = document.getElementById("bundlePreferences"); > + var totalSiteDataSizeLabel = document.getElementById("totalSiteDataSize"); > + totalSiteDataSizeLabel.value = prefStrBundle.getFormattedString("totalSiteDataSize", size); Using .value might not let the text wrap. Please change this to .textContent instead.

Comment on attachment 8807509 [details] Bug 1313003 - Add Site Data section into Network of Advanced of about:preferences, https://reviewboard.mozilla.org/r/90630/#review92248 ::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd:60 (Diff revision 1) > > <!ENTITY httpCache.label "Cached Web Content"> > > <!ENTITY offlineStorage2.label "Offline Web Content and User Data"> > > +<!ENTITY siteData.label "Site Data"> Would it be possible to have a short comment explaining what "Site Data" are in this context?

(In reply to Francesco Lodolo [:flod] from comment #4) > ::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd:60 > (Diff revision 1) > > +<!ENTITY siteData.label "Site Data"> > > Would it be possible to have a short comment explaining what "Site Data" are > in this context? Yes, I can add a comment when updating the next patch.

Comment on attachment 8807509 [details] Bug 1313003 - Add Site Data section into Network of Advanced of about:preferences, @Jaws, Update the patch according to the review comments. And the "browser.storageManager.enabled" pref is moved from firefox.js to all.js because :Gijs mentioned that could put in all.js[1] and it is right below "dom.storageManager.enabled" [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1309123#c8

Comment on attachment 8807509 [details] Bug 1313003 - Add Site Data section into Network of Advanced of about:preferences, https://reviewboard.mozilla.org/r/90630/#review92770

Comment on attachment 8807509 [details] Bug 1313003 - Add Site Data section into Network of Advanced of about:preferences, https://reviewboard.mozilla.org/r/90630/#review92782 ::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd:60 (Diff revision 2) > > <!ENTITY httpCache.label "Cached Web Content"> > > <!ENTITY offlineStorage2.label "Offline Web Content and User Data"> > > +<!-- Site Date section manages sites using Storage API and is under Network --> Typo (Site DatA).

(In reply to Francesco Lodolo [:flod] from comment #9) > Comment on attachment 8807509 [details] > Bug 1313003 - Add Site Data section into Network of Advanced of > about:preferences > > https://reviewboard.mozilla.org/r/90630/#review92782 > > ::: browser/locales/en-US/chrome/browser/preferences/advanced.dtd:60 > (Diff revision 2) > > > > <!ENTITY httpCache.label "Cached Web Content"> > > > > <!ENTITY offlineStorage2.label "Offline Web Content and User Data"> > > > > +<!-- Site Date section manages sites using Storage API and is under Network --> > > Typo (Site DatA). Thanks, fixed.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=083e9e64d01f6464c8ed42fc0092eccc21d86ddd

mozreview lists 12 issues - can we fix this in mozreview so we can land this via the autolander function

(In reply to Carsten Book [:Tomcat] from comment #14) > mozreview lists 12 issues - can we fix this in mozreview so we can land this > via the autolander function Done, thanks.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/502977d27c28 Add Site Data section into Network of Advanced of about:preferences, r=jaws

Sorry had to back out this for eslint failure, https://treeherder.mozilla.org/logviewer.html#?job_id=6923285&repo=autoland#L8155

Backout by ihsiao@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2b5599ceb135 Backed out changeset 502977d27c28 for eslint failure

(In reply to Iris Hsiao [:ihsiao] from comment #17) > Sorry had to back out this for eslint failure, > https://treeherder.mozilla.org/logviewer. > html#?job_id=6923285&repo=autoland#L8155 Per discussion with Iris, although the eslint test in Comment 13 was passed, maybe this eslint failure is caused by merge. I will rebase and update the patch again.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c45a4f617f7bacafd45fc73cbd9c27dab66c814e

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/e2e6505f3fc0 Add Site Data section into Network of Advanced of about:preferences, r=jaws

This bug was about to Add Site Data section into Network Tab inside of Advanced of about:preferences for storage management with the following components: display total storage usage of sites, carry the "learn more" link, one "Clear All Data" button to trigger data clearing, one "Settings" button to open detail settings sub-dialog as mentioned Comment 0. I have seen the features being implemented with Aurora 53.0a2 on Windows 8.1 , 64 bit! This bug's fix is verified on Latest Aurora 53.0a2. Build ID : 20170222004022 User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0 [bugday-20170222]

Thanks!