Provide UI to easily clear cookies from the menus

RESOLVED FIXED in Thunderbird 17.0

Status

Thunderbird
Mail Window Front End
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: standard8, Assigned: awjohnston1)

Tracking

Trunk
Thunderbird 17.0
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=standard8][lang=js|xul])

Attachments

(2 attachments, 11 obsolete attachments)

(Reporter)

Description

5 years ago
As suggested by the privacy review of the web search feature, we should have a UI that allows the user to clear the recent history easily (currently cookie clearing is buried in preferences).

This should be something like the Tools -> Clear Recent History option that Firefox has, probably in the same place for Thunderbird.
(Reporter)

Comment 1

5 years ago
The main work here is to copy sanitize.js/.xul from Firefox and to modify them for Thundebird's requirements - some of the options don't make sense for Thunderbird. Then obviously integrate those into the UI.
Whiteboard: [mentor=standard8][lang=js|xul]
(Assignee)

Comment 2

5 years ago
Notes for me: 

sanatize.js is located at http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/sanitize.js

The pop-up dialog window for the clear recent history option is http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sanitizeDialog.js
(Assignee)

Comment 3

5 years ago
The file
http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js
loads the sanitize.js script and implements the sanitize function of the interface nsIBrowserGlue. Should we have mailGlue.js inheriting from nsIBrowserGlue and add the required function?
(Reporter)

Comment 4

5 years ago
(In reply to awjohnston1 from comment #3)
> The file
> http://mxr.mozilla.org/mozilla-central/source/browser/components/
> nsBrowserGlue.js
> loads the sanitize.js script and implements the sanitize function of the
> interface nsIBrowserGlue. Should we have mailGlue.js inheriting from
> nsIBrowserGlue and add the required function?

What you'll need to do is create and nsIMailGlue.idl file based on nsIBrowserGlue.idl. The only function you'll need in nsIMailGlue is the sanitize function. You then need to hook that inferface into the Makefile.in (see XPIDLSRCS in the Makefile.in alongside nsIBrowserGlue), and into mailGlue.js.

For mailGlue.js, you should just have to add nsIMailGlue to the array passed to XPCOMUtils.generateQI, and then define the sanitize function itself.
(Assignee)

Comment 5

5 years ago
Created attachment 639988 [details] [diff] [review]
clearHistoryPatchv1

Tested in Windows 7, clears cookies and cache from Tools=>Clear Recent History menu option. Still need to create and implement interface nsIMailGlue. No automated tests yet.
(Assignee)

Updated

5 years ago
Attachment #639988 - Attachment description: first draft → clearHistoryPatchv1
(Assignee)

Comment 6

5 years ago
Also, should consider a Ctrl+Shift+Del command as in Firefox.
(Assignee)

Comment 7

5 years ago
Created attachment 640002 [details] [diff] [review]
clearHistoryPatchv2

Error in last patch, file listed in jar manifest which was not necessary and not tracked by hg. Also some debugging lines I forgot to remove. Should work now.
Attachment #639988 - Attachment is obsolete: true
(Assignee)

Comment 8

5 years ago
Created attachment 640379 [details] [diff] [review]
interfacePatchv1

Tried to use the interface nsIMailGlue.idl. The instructions for the Windows build of .xpt found here
https://developer.mozilla.org/en/How_to_Build_an_XPCOM_Component_in_Javascript/
generate nothing but errors on my system. Don't have access to a Linux box right now, if anyone can give me tips on using xpidl.py or can run this patch it would be much appreciated.

Comment 9

5 years ago
You don't normally need to run xpidl.py for files located in the tree, because it is done automatically.

I loaded your patch, and compiled the mail/components directory and it nsIMailGlue is compiling fine. You can see the file mail/components/_xpidlgen/nsIMailGlue.h in the object directory for example.

You should not call this nsIBrowserGlue though.

You need to be more specific than "nothing but errors" to say what is failing for you.
(Assignee)

Comment 10

5 years ago
Created attachment 640407 [details] [diff] [review]
interfacePatchv2

corrected some typos.
Attachment #640379 - Attachment is obsolete: true
(Assignee)

Comment 11

5 years ago
Created attachment 641568 [details] [diff] [review]
clearHistoryPatchv3

Fixed a syntax error. The UI now appears to clear data successfully.
Attachment #640002 - Attachment is obsolete: true
Attachment #640407 - Attachment is obsolete: true
Attachment #641568 - Flags: review+
(Assignee)

Comment 12

5 years ago
Created attachment 641572 [details] [diff] [review]
clearHistoryPatchv4

removed an unnecessary variable declaration.
Attachment #641568 - Attachment is obsolete: true
Attachment #641572 - Flags: review+
(Assignee)

Comment 13

5 years ago
What needs to be implemented for testing? Looking at the tests from Firefox, the following tests are related to this bug:

browser_sanitize-timespan.js
browser_sanitizeDialog.js

I think that the first is not relevant to us. In sanitize.js we can see that the timespan is never applied to the cache items. They don't have a time stamp and all are deleted. The cookies are not tested in sanitize_timespan.js  A comment I read in there states that nsICookieManager does not allow fake creation times.

The second seems relevant, although only for data in Tbird. Ie not history and downloads etc.
(Assignee)

Comment 14

5 years ago
I also flagged the latest patch for review. Is there anything else I should be doing in regards to this?
(Assignee)

Comment 15

5 years ago
Created attachment 641582 [details] [diff] [review]
clearHistoryv5

removed some code which is not necessary for thunderbird.
Attachment #641572 - Attachment is obsolete: true
Attachment #641582 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #641568 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #641572 - Flags: review+
(Reporter)

Comment 16

5 years ago
Comment on attachment 641582 [details] [diff] [review]
clearHistoryv5

Review of attachment 641582 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Andrew, this is looking really great. So I'm going to give it feedback+ and request ui-review from Blake, so we can get his input on it before we refine the patch fully (note: to request review, set the flag to '?' and then put the name/email in the field that appears).

I have gone through it and added some comments anyway, mainly about improving the style of the code, but there's some things like making sure we add default prefs so the dialog looks good for first time users.

::: mail/base/content/mailCore.js
@@ +351,5 @@
>  }
>  
> +function toSanitize()
> +{
> +  Components.classes["@mozilla.org/mail/mailglue;1"].getService(Components.interfaces.nsIMailGlue).sanitize(window);

nit: please start the .getService on the next line with the dot aligned with the dot of the Components.classes on the previous line.

You could probably wrap .sanitize onto a new line as well.

This just helps to keep line lengths down and makes it easier to read.

::: mail/base/content/mailWindowOverlay.xul
@@ +1672,5 @@
>          <menuitem id="menu_import" label="&importCmd.label;"
>                    accesskey="&importCmd.accesskey;"
>                    oncommand="toImport();"/>
>          <menuitem id="javascriptConsole" label="&errorConsoleCmd.label;" accesskey="&errorConsoleCmd.accesskey;" key="key_errorConsole" oncommand="toJavaScriptConsole();"/>
> +		<menuitem id="sanitizeHistory"

nit: In various places in the patch you've got tabs rather than spaces. We only use spaces so that formatting is maintained everywhere.

::: mail/base/content/sanitize.js
@@ +1,1 @@
> +# -*- Mode: Java; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-

I think we can just drop this modeline.

@@ +1,2 @@
> +# -*- Mode: Java; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-
> +# This Source Code Form is subject to the terms of the Mozilla Public

Please switch to the /* ... */ form (http://www.mozilla.org/MPL/headers/) and with the change to processing mentioned below, you can then drop the * at the start of the line for this file in jar.mn as preprocessing will no longer be required.

@@ +33,5 @@
> +  sanitize: function ()
> +  {
> +    var psvc = Components.classes["@mozilla.org/preferences-service;1"]
> +                         .getService(Components.interfaces.nsIPrefService);
> +    var branch = psvc.getBranch(this.prefDomain);

At the start of the file include the line:

Components.utils.import("resource://gre/modules/Services.jsm");

Then you can make this:

var branch = Services.prefs.getBranch(this.prefDomain);

(and remove the var psvc above it)

@@ +81,5 @@
> +                          getService(Ci.nsICacheService);
> +        try {
> +          // Cache doesn't consult timespan, nor does it have the
> +          // facility for timespan-based eviction.  Wipe it.
> +          cacheService.evictEntries(Ci.nsICache.STORE_ANYWHERE);

Services.cache.evict...

@@ +96,5 @@
> +    cookies: {
> +      clear: function ()
> +      {
> +        var cookieMgr = Components.classes["@mozilla.org/cookiemanager;1"]
> +                                  .getService(Ci.nsICookieManager);

cookieMgr can be replaced by Services.cookies

@@ +147,5 @@
> +        // clear any network geolocation provider sessions
> +        var psvc = Components.classes["@mozilla.org/preferences-service;1"]
> +                             .getService(Components.interfaces.nsIPrefService);
> +        try {
> +            var branch = psvc.getBranch("geo.wifi.access_token.");

Services.prefs

@@ +223,5 @@
> +
> +// Shows sanitization UI
> +Sanitizer.showUI = function(aParentWindow) 
> +{
> +  var ww = Components.classes["@mozilla.org/embedcomp/window-watcher;1"]

ww can be replaced by Services.ww

@@ +226,5 @@
> +{
> +  var ww = Components.classes["@mozilla.org/embedcomp/window-watcher;1"]
> +                     .getService(Components.interfaces.nsIWindowWatcher);
> +#ifdef XP_MACOSX
> +  ww.openWindow(null, // make this an app-modal window on Mac

Here we can use:

ww.openWindow(Application.platformIsMac ? null : aParentWindow, ...

We can then drop the #ifdefs, and remove the need for preprocessing.

@@ +245,5 @@
> +{
> +  Sanitizer.showUI(aParentWindow);
> +};
> +
> +Sanitizer.onStartup = function() 

I'm not convinced we need the startup/shutdown functions at the moment (you're not calling them at the moment anyway). Cookies can already have session expiry, and there's an item for that in the preferences window. We also don't have an option for clearing the shutdown cache, and I think given the way TB works we don't need that at the moment (although it could always be done in a follow-up if we wanted it).

::: mail/base/content/sanitize.xul
@@ +1,3 @@
> +<?xml version="1.0"?>
> +
> +# -*- Mode: Java; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-

This isn't a java file, so we should just drop this line. We generally don't need it anyway.

@@ +2,5 @@
> +
> +# -*- Mode: Java; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*-
> +# 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/.

Please use the <!-- --> form of this (available from http://www.mozilla.org/MPL/headers/).

Then drop the star at the start of the line in jar.mn which avoids the need for preprocessing.

@@ +21,5 @@
> +            xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> +            dlgbuttons="accept,cancel"
> +            title="&sanitizeDialog2.title;"
> +            noneverythingtitle="&sanitizeDialog2.title;"
> +            style="width: &dialog.width;;"

nit: only one ; necessary.

@@ +35,5 @@
> +    <script type="application/javascript"
> +            src="chrome://messenger/content/sanitizeDialog.js"/>
> +
> +    <preferences id="sanitizePreferences">
> +      <preference id="privacy.cpd.cookies"               name="privacy.cpd.cookies"               type="bool"/>

You need to copy the relevant default preferences for privacy.cpd.cookies and privacy.cpd.cache from Firefox's firefox.js to the all-thunderbird.js file. This will ensure that the first time user of the dialog has pre-seeded values for the check boxes.

@@ +40,5 @@
> +      <preference id="privacy.cpd.cache"                 name="privacy.cpd.cache"                 type="bool"/>
> +    </preferences>
> +    
> +    <preferences id="nonItemPreferences">
> +      <preference id="privacy.sanitize.timeSpan"

Also add privacy.sanitize.timeSpan to all-thunderbird.js

@@ +70,5 @@
> +
> +    <separator class="thin"/>
> +
> +
> +      <vbox id="sanitizeEverythingWarningBox">

nit: only one blank line before this.

::: mail/base/jar.mn
@@ +109,5 @@
>      content/messenger/safeMode.js                   (content/safeMode.js)
> +*	content/messenger/sanitize.xul					(content/sanitize.xul)
> +*	content/messenger/sanitize.js					(content/sanitize.js)
> +	content/messenger/sanitizeDialog.css			(content/sanitizeDialog.css)
> +*	content/messenger/sanitizeDialog.js				(content/sanitizeDialog.js)

sanitizeDialog.js doesn't need pre-processing, so the * can be dropped from the start of the line.

::: mail/components/Makefile.in
@@ +14,5 @@
> +XPIDL_MODULE = mailcompsbase
> +
> +XPIDLSRCS = \
> +nsIMailGlue.idl \
> +$(NULL)

nit: 2-space indent nsIMailGlue.idl \ and the $(NULL) please.

::: mail/components/mailGlue.js
@@ +35,1 @@
>  MailGlue.prototype = {

nit: no need for the added lines just before this.

@@ +109,5 @@
>  
>    // for XPCOM
>    classID: Components.ID("{eb239c82-fac9-431e-98d7-11cacd0f71b8}"),
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver,
> +										 Ci.nsIMailGlue]),

nit: Ci.nsIMailGlue should align with the Ci. on the previous line.
Attachment #641582 - Flags: ui-review?(bwinton)
Attachment #641582 - Flags: review+
Attachment #641582 - Flags: feedback+
(Reporter)

Updated

5 years ago
Assignee: nobody → awjohnston1
(Assignee)

Comment 17

5 years ago
Created attachment 642696 [details] [diff] [review]
clearHistoryPatchv6

made requested changes from Comment 16. everything works except for timespan. if select a timespan other than everything, cookies are not cleared appropriately. i will try to fix this.
(Assignee)

Comment 18

5 years ago
Created attachment 642720 [details] [diff] [review]
obsolete
Attachment #642720 - Flags: review?(bwinton)
(Assignee)

Updated

5 years ago
Attachment #642720 - Attachment is obsolete: true
Attachment #642720 - Flags: review?(bwinton)
(Assignee)

Comment 19

5 years ago
Created attachment 642721 [details] [diff] [review]
clearHistoryPatchv7

fixed error, dialog appears to function as it should.
Attachment #641582 - Attachment is obsolete: true
Attachment #642696 - Attachment is obsolete: true
Attachment #641582 - Flags: ui-review?(bwinton)
Attachment #642721 - Flags: ui-review?(bwinton)
(Assignee)

Updated

5 years ago
Attachment #642720 - Attachment description: clearHistoryPatchv7 → obsolete
This patch doesn't apply cleanly for me on the latest trunk build.  Could you please update your repository and attach a new one?

(In the meantime, I'll try to hand-edit it, and get it compiling…)

Thanks,
Blake.
Comment on attachment 642721 [details] [diff] [review]
clearHistoryPatchv7

The details seem small enough that I would be tempted to just inline them, but leaving it this way makes it consistent with Firefox, so I'll say ui-r=me.

Thanks,
Blake.
Attachment #642721 - Flags: ui-review?(bwinton) → ui-review+
(Assignee)

Comment 22

5 years ago
Created attachment 644407 [details] [diff] [review]
clearHistoryPatchv8

trying to rebase. currently this patch fails to build.
(Assignee)

Comment 23

5 years ago
Created attachment 644415 [details] [diff] [review]
clearHistoryPatchv9

rebased the patch with source I downloaded this morning. seems to be functional now. mconley has built and found an idiosyncracy with the build system. it does not like the fact that I try to add nsIMailGlue.idl He is trying a build to resolve.
Attachment #642721 - Attachment is obsolete: true
Attachment #644407 - Attachment is obsolete: true
Attachment #644415 - Flags: ui-review?(bwinton)
(Did you change the UI between v7 and v9?  Cause if not, you can just carry forward my previous ui-r+…)
(Assignee)

Updated

5 years ago
Attachment #644415 - Flags: ui-review?(bwinton)
(Assignee)

Comment 25

5 years ago
no, I did not change the UI. Didn't realize you had already reviewed :)
(Assignee)

Comment 26

5 years ago
mconley gave clearHistoryPatchv9 a try build. build was successful except for a known random orange in the mozmill testing. what is the next step I should take to get the patch committed?
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/76efc33d143a

Thanks for the patch, Andrew! One request - to make life easier for those checking in on your behalf, please make sure your future patches follow the directions at the link below so that all the necessary commit information is present. Thanks!
https://developer.mozilla.org/en/Creating_a_patch_that_can_be_checked_in
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 17.0
Depends on: 777867
(Reporter)

Comment 28

5 years ago
Hi Andrew,

Unfortunately we missed a step that your patch needs a complete review+ before landing.

Rather than back it out, I'll go through the patch and review it, then we can address an issues in a follow-up patch, if that is ok with you?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 29

5 years ago
Comment on attachment 644415 [details] [diff] [review]
clearHistoryPatchv9

Review of attachment 644415 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, there's a few comments here, if you can provide a patch which fixes those, and request review from me, then we can get those landed, and I think this will be fully fixed then.

BTW, sorry for not commenting earlier, I was on holiday.

::: mail/base/content/sanitize.js
@@ +210,5 @@
> +{
> +  return Sanitizer._prefs ? Sanitizer._prefs
> +    : Sanitizer._prefs = Components.classes["@mozilla.org/preferences-service;1"]
> +                         .getService(Components.interfaces.nsIPrefService)
> +                         .getBranch(Sanitizer.prefDomain);

Should be able to use Services.prefs.getBranch here.

::: mail/base/jar.mn
@@ +106,5 @@
>      content/messenger/browserRequest.js             (content/browserRequest.js)
>      content/messenger/browserRequest.xul            (content/browserRequest.xul)
>  *   content/messenger/safeMode.xul                  (content/safeMode.xul)
>      content/messenger/safeMode.js                   (content/safeMode.js)
> +	content/messenger/sanitize.xul					(content/sanitize.xul)

The jar.mn entries in this file are still using tabs.

::: mail/components/Makefile.in
@@ +10,5 @@
>  
>  include $(DEPTH)/config/autoconf.mk
> +
> +MODULE = mailcomps
> +XPIDL_MODULE = mailcompsbase

You need to add mailcompsbase to this file http://mxr.mozilla.org/comm-central/source/mail/installer/package-manifest.in#171 somewhere around the indicated line.

Otherwise the interface details aren't shipped in release builds, and the dialog won't work.

::: mail/components/mailGlue.js
@@ +19,5 @@
>   * MailUtils.js instead.
>   */
>  
>  function MailGlue() {
> +	XPCOMUtils.defineLazyGetter(this, "_sanitizer",

You've also still got tabs in this function, and when adding the sanitize function further down in the file.
(Reporter)

Updated

5 years ago
No longer depends on: 777867
Duplicate of this bug: 777867
(Assignee)

Comment 31

5 years ago
Created attachment 647271 [details] [diff] [review]
clearHistoryUpdatev1

Thanks for the feedback Mark. I made the requested changes to the latest trunk and had a successful build on my machine. The .xpt has been added to the manifest, hopefully this will resolve the issue noticed by others.
Attachment #647271 - Flags: review?(mbanner)
(Reporter)

Comment 32

5 years ago
Comment on attachment 647271 [details] [diff] [review]
clearHistoryUpdatev1

That's great, thanks, I'll check this in in a few minutes.
Attachment #647271 - Flags: review?(mbanner) → review+
(Reporter)

Comment 33

5 years ago
Checked in: https://hg.mozilla.org/comm-central/rev/899e1f145586

Many thanks Andrew!
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Blocks: 789678
You need to log in before you can comment on or make changes to this bug.