Closed Bug 1422163 Opened 7 years ago Closed 6 years ago

Make a new confirm dialog for clearing all site data that allows you to clear cache

Categories

(Firefox :: Settings UI, enhancement, P1)

Product:
Firefox
Component:
Settings UI
Version:
58 Branch
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- verified

People

(Reporter: johannh, Assigned: johannh)

References

(Depends on 1 open bug)

Details

(Whiteboard: [storage-v2])

Attachments

(2 files)

Bug 1422163 - Part 1 - Make a new confirm dialog for clearing all site data that allows you to clear cache.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1422163 - Part 2 - Make a new test for clearing all site data with the new dialog.
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
See bug 1421690 for specs. We'd like to merge the "Clear site data" and "Clear cache" buttons and need a dialog that can do both and allows the user to choose between clearing either or both options.
Flags: qe-verify+
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Priority: P2 → P1
Blocks: 1406901
Gijs, I know you've been reviewing the previous site data things in preferences, so I thought you might want to take this. Please note that this is part of a larger bit of redesign/reorganization we're doing in 60 (bug 1421690) and as such it duplicates functionality with the network cache "Clear All" button that I'll remove separately. I think it's fine to have a bit of this WIP state in Nightly, but I'm happy to get your input on that if you differ :)

The strings are not definitely final, I'll try to meet with Jacqueline next week to sort out the details and there's also potential change from the results of the user study underway right now.

Thanks!
Comment on attachment 8941382 [details]
Bug 1422163 - Part 1 - Make a new confirm dialog for clearing all site data that allows you to clear cache.

https://reviewboard.mozilla.org/r/211702/#review220194

Almost finished, but given the event listener and line height stuff I may as well look at this again.

I'm assuming you've checked this looks OK on Windows/Mac/Linux?

::: browser/components/preferences/clearSiteData.css:19
(Diff revision 2)
> +  padding-bottom: 16px;
> +}
> +
> +.option-description {
> +  color: #737373;
> +  line-height: 1em;

Why the line-height? Not sure this is necessary, and slightly worried it won't work well with e.g. Traditional Chinese or various Japanese glyphs.

::: browser/components/preferences/clearSiteData.js:24
(Diff revision 2)
> +    SiteDataManager.getTotalUsage().then(bytes => {
> +      let size = DownloadUtils.convertByteUnits(bytes);
> +      document.getElementById("clearSiteDataLabel").value =
> +        this._bundle.formatStringFromName("clearSiteDataWithEstimates.label", size, 2);
> +    });

I'm tempted to suggest the following helper:

```js
async showSizeInLabel(sizePromise, elemID, stringID) {
  let size = DownloadUtils.convertByteUnits(await sizePromise);
  let label = document.getElementById(elemID);
  label.value =
    this._bundle.formatStringFromName(stringID, size, 1);
}
```

and then call from `init()`:

```js
this.showSizeInlabel(SiteDataManager.getTotalUsage(), "clearSiteDataLabel", ...);
```

etc.

But won't block review on that. :-)

::: browser/components/preferences/clearSiteData.js:30
(Diff revision 2)
> +      let size = DownloadUtils.convertByteUnits(bytes);
> +      document.getElementById("clearSiteDataLabel").value =
> +        this._bundle.formatStringFromName("clearSiteDataWithEstimates.label", size, 2);
> +    });
> +    SiteDataManager.getCacheSize().then(bytes => {
> +      let size = DownloadUtils.convertByteUnits(bytes);

Even if you don't like my suggestion, can you clarify that this is an array of 2 things?

::: browser/components/preferences/clearSiteData.js:43
(Diff revision 2)
> +    if (event.keyCode == KeyEvent.DOM_VK_ESCAPE)
> +      window.close();

I'm surprised this is necessary. I thought subdialogs took care of this?

::: browser/components/preferences/clearSiteData.xul:18
(Diff revision 2)
> +        onload="gClearSiteDataDialog.onLoad();"
> +        onunload="gClearSiteDataDialog.uninit();"

Can you add these from the JS file instead?

I think we'd like to move to a world where we can CSP all the in-content chrome stuff, so we might as well start. :-)

::: browser/components/preferences/clearSiteData.xul:56
(Diff revision 2)
> +      <button oncommand="close();" icon="close" id="cancel"
> +              label="&button.cancel.label;" accesskey="&button.cancel.accesskey;" />
> +      <button id="clearButton" oncommand="gClearSiteDataDialog.onClear();" icon="save"

Same here? You already have `command` listeners in the JS file so I think moving all the listeners there would make sense. :-)

::: browser/components/preferences/in-content/privacy.js:1520
(Diff revision 2)
>      let intValue = parseInt(cacheSizeElem.value, 10);
>      cachePref.value = isNaN(intValue) ? 0 : intValue * 1024;
>    },
>  
>    clearSiteData() {
> -    let flags =
> +    gSubDialog.open("chrome://browser/content/preferences/clearSiteData.xul");

Should we update the content here once the removal happens?

::: browser/locales/en-US/chrome/browser/siteData.properties:5
(Diff revision 2)
> +clearSiteDataPromptTitle=Clear all cookies and site data
> +clearSiteDataPromptText=Selecting ‘Clear Now’ will clear all cookies and site data stored by Firefox. This may sign you out of websites and remove offline web content.
> +clearSiteDataNow=Clear Now

Please check with flod if you need new ids given you're moving these to a different file, but keeping the same content. I guess it's fine, but I don't actually know.
Attachment #8941382 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8941383 [details]
Bug 1422163 - Part 2 - Make a new test for clearing all site data with the new dialog.

https://reviewboard.mozilla.org/r/211704/#review220218

Can you mark the new test as a copy of the one from which you're extracting stuff so the diff is easier to read? :-)
Attachment #8941383 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8941382 [details]
Bug 1422163 - Part 1 - Make a new confirm dialog for clearing all site data that allows you to clear cache.

https://reviewboard.mozilla.org/r/211702/#review220194

> I'm tempted to suggest the following helper:
> 
> ```js
> async showSizeInLabel(sizePromise, elemID, stringID) {
>   let size = DownloadUtils.convertByteUnits(await sizePromise);
>   let label = document.getElementById(elemID);
>   label.value =
>     this._bundle.formatStringFromName(stringID, size, 1);
> }
> ```
> 
> and then call from `init()`:
> 
> ```js
> this.showSizeInlabel(SiteDataManager.getTotalUsage(), "clearSiteDataLabel", ...);
> ```
> 
> etc.
> 
> But won't block review on that. :-)

I think I won't take your suggestion on that, I usually prefer not to abstract things away before it's really necessary :)

> Even if you don't like my suggestion, can you clarify that this is an array of 2 things?

Good idea, done!

> I'm surprised this is necessary. I thought subdialogs took care of this?

Yup, I was also surprised. Time for a follow-up bug?

> Can you add these from the JS file instead?
> 
> I think we'd like to move to a world where we can CSP all the in-content chrome stuff, so we might as well start. :-)

Good call, we should have a P5 bug for this stuff, in case someone wants to help us out (and to have a clear signal for authors/reviewers which direction we're going in).

> Should we update the content here once the removal happens?

Good idea.

> Please check with flod if you need new ids given you're moving these to a different file, but keeping the same content. I guess it's fine, but I don't actually know.

Checked that we don't need new IDs :)
(In reply to Johann Hofmann [:johannh] from comment #11)
> Comment on attachment 8941382 [details]
> > Can you add these from the JS file instead?
> > 
> > I think we'd like to move to a world where we can CSP all the in-content chrome stuff, so we might as well start. :-)
> 
> Good call, we should have a P5 bug for this stuff, in case someone wants to
> help us out (and to have a clear signal for authors/reviewers which
> direction we're going in).

We already removed all the inline handlers in the past. https://bugzilla.mozilla.org/show_bug.cgi?id=1016300 . I don't know if there are any that were reintroduced, and if eslint can help avoid further reintroductions. Mark?
Flags: needinfo?(standard8)
(In reply to :Gijs (lower availability until Jan 27) from comment #12)
> We already removed all the inline handlers in the past.
> https://bugzilla.mozilla.org/show_bug.cgi?id=1016300 . I don't know if there
> are any that were reintroduced, and if eslint can help avoid further
> reintroductions. Mark?

We don't do any parsing of xul at the moment - generally we don't have much js there if any, so it hasn't been worth trying. If we wanted to do something like it, we'd need to write a custom parser. Though for xul it might just be easier to write a custom tool to check the files for things we don't want.
Flags: needinfo?(standard8)
OK. In that case, Johann can you file a followup bug to create a browser mochitest in static/ (or perhaps topically in browser/components/preferences/ ) that checks for inline attributes in about:preferences and its subdialogs? Not 100% sure how it'd cover new subdialogs, though... Maybe Florian would have ideas.

I'll try to review the new patches here tomorrow.
Comment on attachment 8941382 [details]
Bug 1422163 - Part 1 - Make a new confirm dialog for clearing all site data that allows you to clear cache.

https://reviewboard.mozilla.org/r/211702/#review220858

r=me with the removeEventListener calls fixed. Also, did you intend to do the updating of sizes etc. when the clearing happens in a follow-up bug? I don't see it in the patch interdiff, but maybe I'm missing something?

::: browser/components/preferences/clearSiteData.js:81
(Diff revisions 2 - 3)
>      this._clearSiteDataCheckbox.removeEventListener("command", this);
>      this._clearCacheCheckbox.removeEventListener("command", this);

These no longer match up, and you never remove the other listeners. Is that going to be a problem, esp. for leaks? I don't think so, but I don't know how to be sure, except by removing the listeners. :-)
Attachment #8941382 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8941382 [details]
Bug 1422163 - Part 1 - Make a new confirm dialog for clearing all site data that allows you to clear cache.

https://reviewboard.mozilla.org/r/211702/#review220194

> Good idea.

Reopening this so we don't lose track of this particular thingy.
Comment on attachment 8941383 [details]
Bug 1422163 - Part 2 - Make a new test for clearing all site data with the new dialog.

https://reviewboard.mozilla.org/r/211704/#review220864

::: browser/components/preferences/in-content/tests/browser_clearSiteData.js:1
(Diff revision 3)
> -/* Any copyright is dedicated to the Public Domain.
> +Components.utils.import("resource:///modules/SitePermissions.jsm");

Please re-add the license header.
Attachment #8941383 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs (lower availability until Jan 27) from comment #18)
> Comment on attachment 8941383 [details]
> Bug 1422163 - Make a new test for clearing all site data with the new dialog.
> 
> https://reviewboard.mozilla.org/r/211704/#review220864
> 
> :::
> browser/components/preferences/in-content/tests/browser_clearSiteData.js:1
> (Diff revision 3)
> > -/* Any copyright is dedicated to the Public Domain.
> > +Components.utils.import("resource:///modules/SitePermissions.jsm");
> 
> Please re-add the license header.

... and "use strict" :-)
Comment on attachment 8941382 [details]
Bug 1422163 - Part 1 - Make a new confirm dialog for clearing all site data that allows you to clear cache.

https://reviewboard.mozilla.org/r/211702/#review221250

::: browser/components/preferences/clearSiteData.js:70
(Diff revisions 2 - 3)
> +      if (window.opener.gPrivacyPane) {
> +        window.opener.gPrivacyPane.updateActualCacheSize();
> +      }

Huh, don't know why/how I missed this.

Anyway, this is fine, but this code is confused about its assumptions. Either:

- we assume we're always opened from the preferences, and then we probably don't need to check this thing.
- we assume nothing. Which means we should nullcheck `window.opener` ;-)


I would tend to err on the side of nullchecking window.opener.
So it seems like debug builds were failing because the value labels were taking a very long time to appear. I raised the timeout for now and will just monitor for intermittents. I know this isn't great practice but it really seems like this is slowness-related to me.

I'll check the copy again with jsavory and land when we're good.
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/60cd03e9c5ac
Part 1 - Make a new confirm dialog for clearing all site data that allows you to clear cache. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/f8a8160484c8
Part 2 - Make a new test for clearing all site data with the new dialog. r=Gijs
Flags: needinfo?(jhofmann)
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7eed1367d341
Part 1 - Make a new confirm dialog for clearing all site data that allows you to clear cache. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/7ab250439e7b
Part 2 - Make a new test for clearing all site data with the new dialog. r=Gijs
Backed out for mochitest bc failure on /browser_clearSiteData.js

Log: https://treeherder.mozilla.org/logviewer.html#?job_id=159892444&repo=autoland&lineNumber=4928
Flags: needinfo?(jhofmann)
Backout by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7bb213beff5d
Backed out 2 changesets for mochitest bc failure on /browser_clearSiteData.js. on a CLOSED TREE
Comment on attachment 8941382 [details]
Bug 1422163 - Part 1 - Make a new confirm dialog for clearing all site data that allows you to clear cache.

https://reviewboard.mozilla.org/r/211702/#review223080

::: browser/locales/en-US/chrome/browser/preferences/clearSiteData.dtd:8
(Diff revision 6)
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<!ENTITY window.title                 "Clear Data">
> +<!ENTITY window.width                 "35em">
> +
> +<!ENTITY window.description           "Clearing all cookies and site data stored by Firefox may sign you out of websites and remove offline web content. Clearing cache data will not affect your logins.">

I know this string comes from another file, but should Firefox remain hard-coded here? Using brandShortName is cheaper in a .DTD than .properties

::: browser/locales/en-US/chrome/browser/preferences/clearSiteData.properties:5
(Diff revision 6)
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +clearSiteDataWithEstimates.label = Site Data and Cookies (%1$S %2$S)

Please add a comment explaining what replaces the placeholders (in both strings).

::: browser/locales/en-US/chrome/browser/siteData.properties:6
(Diff revision 6)
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +clearSiteDataPromptTitle=Clear all cookies and site data
> +clearSiteDataPromptText=Selecting ‘Clear Now’ will clear all cookies and site data stored by Firefox. This may sign you out of websites and remove offline web content.

Worth replacing the brand name at run-time?
This is frustrating... the cache value seems to intermittently change between its initial assertion and when its displayed in the dialog... I'm going to do another cache getter right before the waitForCondition call and do a try runs with lots of retriggers to see if that works... Otherwise I'll just have to remove the cache waitForCondition.

Flod, thank you for the comments. I totally didn't consider replacing the "Firefox" when moving these strings. Will do now.
Flags: needinfo?(jhofmann)
Does running with --verify locally not catch these issues?
(In reply to :Gijs from comment #39)
> Does running with --verify locally not catch these issues?

Nope, and verify doesn't fail on try, either...
So I came up with this "solution". It's not amazing but it'll at least assert that something is shown.

Gijs, the accumulated changes make me feel like I should get another quick rs from you before landing. Would you mind taking a brief look at the interdiff of the two patches?
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8941383 [details]
Bug 1422163 - Part 2 - Make a new test for clearing all site data with the new dialog.

https://reviewboard.mozilla.org/r/211704/#review223560

::: browser/components/preferences/in-content/tests/browser_clearSiteData.js:6
(Diff revisions 3 - 8)
> -Components.utils.import("resource:///modules/SitePermissions.jsm");
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
>  
> -const { SiteDataManager } = Cu.import("resource:///modules/SiteDataManager.jsm", {});
> -const { DownloadUtils } = Cu.import("resource://gre/modules/DownloadUtils.jsm", {});
> +"use strict";
> +
> +const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;

You already don't need this anymore.

::: browser/components/preferences/in-content/tests/browser_clearSiteData.js:8
(Diff revisions 3 - 8)
>  
> -const { SiteDataManager } = Cu.import("resource:///modules/SiteDataManager.jsm", {});
> -const { DownloadUtils } = Cu.import("resource://gre/modules/DownloadUtils.jsm", {});
> +"use strict";
> +
> +const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
> +
> +ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");

You don't seem to use this.

::: browser/components/preferences/in-content/tests/browser_siteData.js:8
(Diff revision 3)
>  
>  "use strict";
>  
>  const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
>  
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");

This is just an interdiff artifact, but given you *are* modifying this file, I guess you could now just get rid of this (and the Cc/Ci/Cu/Cr bits above).
(In reply to Johann Hofmann [:johannh] from comment #44)
> So I came up with this "solution". It's not amazing but it'll at least
> assert that something is shown.
> 
> Gijs, the accumulated changes make me feel like I should get another quick
> rs from you before landing. Would you mind taking a brief look at the
> interdiff of the two patches?

LGTM, see nits in the other comment.
Flags: needinfo?(gijskruitbosch+bugs)
No longer blocks: 1406901
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fd5e712f297c
Part 1 - Make a new confirm dialog for clearing all site data that allows you to clear cache. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/9904deec37b3
Part 2 - Make a new test for clearing all site data with the new dialog. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/fd5e712f297c
https://hg.mozilla.org/mozilla-central/rev/9904deec37b3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
More xul :(( This would have been a great opportunity to convert the dialog to HTML especially since there are already some HTML preference dialogs (like the form autofill settings).
Depends on: 1436395
Depends on: 1436931
Depends on: 1439184
Verified fixed using the latest Nightly (2018-02-20) on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.11.
Status: RESOLVED → VERIFIED
status-firefox60: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: