Closed Bug 1313003 Opened 8 years ago Closed 8 years ago

Add Site Data section into Network of Advanced of about:preferences

Categories

(Firefox :: Settings UI, defect)

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

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox53 --- verified

People

(Reporter: Fischer, Assigned: Fischer)

References

Details

(Whiteboard: [storage-v1])

Attachments

(1 file)

Bug 1313003 - Add Site Data section into Network of Advanced of about:preferences,
58 bytes, text/x-review-board-request
jaws
: review+
Details 
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
No longer depends on: 1312361
Blocks: 1312361
Blocks: 1312372
Blocks: 1312349
Blocks: 1313602
Assignee: nobody → fliu
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
Attachment #8807509 - Flags: review?(jaws)
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.
Attachment #8807509 - Flags: review?(jaws) → review-
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
Attachment #8807509 - Flags: review?(jaws)
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
Attachment #8807509 - Flags: review?(jaws) → review+
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.
mozreview lists 12 issues - can we fix this in mozreview so we can land this via the autolander function
Flags: needinfo?(fliu)
Keywords: checkin-needed
(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.
Flags: needinfo?(fliu)
Keywords: checkin-needed
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
Keywords: checkin-needed
Sorry had to back out this for eslint failure, https://treeherder.mozilla.org/logviewer.html#?job_id=6923285&repo=autoland#L8155
Flags: needinfo?(fliu)
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.
Flags: needinfo?(fliu)
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e2e6505f3fc0
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
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!
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
Blocks: 1348252
Blocks: 1348733
No longer blocks: 1348733
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: