Last Comment Bug 741336 - Provide UI to easily clear cookies from the menus
: Provide UI to easily clear cookies from the menus
Status: RESOLVED FIXED
[mentor=standard8][lang=js|xul]
:
Product: Thunderbird
Classification: Client Software
Component: Mail Window Front End (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 17.0
Assigned To: awjohnston1
:
Mentors:
: 777867 (view as bug list)
Depends on:
Blocks: 789678
  Show dependency treegraph
 
Reported: 2012-04-02 03:53 PDT by Mark Banner (:standard8)
Modified: 2012-09-08 03:02 PDT (History)
5 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
clearHistoryPatchv1 (54.85 KB, patch)
2012-07-07 12:24 PDT, awjohnston1
no flags Details | Diff | Review
clearHistoryPatchv2 (54.47 KB, patch)
2012-07-07 16:08 PDT, awjohnston1
no flags Details | Diff | Review
interfacePatchv1 (57.37 KB, patch)
2012-07-09 15:01 PDT, awjohnston1
no flags Details | Diff | Review
interfacePatchv2 (57.93 KB, patch)
2012-07-09 16:16 PDT, awjohnston1
no flags Details | Diff | Review
clearHistoryPatchv3 (57.95 KB, patch)
2012-07-12 12:55 PDT, awjohnston1
no flags Details | Diff | Review
clearHistoryPatchv4 (57.93 KB, patch)
2012-07-12 13:13 PDT, awjohnston1
no flags Details | Diff | Review
clearHistoryv5 (57.03 KB, patch)
2012-07-12 13:48 PDT, awjohnston1
standard8: feedback+
Details | Diff | Review
clearHistoryPatchv6 (57.12 KB, patch)
2012-07-16 13:22 PDT, awjohnston1
no flags Details | Diff | Review
obsolete (57.00 KB, patch)
2012-07-16 14:05 PDT, awjohnston1
no flags Details | Diff | Review
clearHistoryPatchv7 (57.00 KB, patch)
2012-07-16 14:08 PDT, awjohnston1
bwinton: ui‑review+
Details | Diff | Review
clearHistoryPatchv8 (57.26 KB, patch)
2012-07-20 11:42 PDT, awjohnston1
no flags Details | Diff | Review
clearHistoryPatchv9 (57.26 KB, patch)
2012-07-20 12:13 PDT, awjohnston1
no flags Details | Diff | Review
clearHistoryUpdatev1 (5.26 KB, patch)
2012-07-30 13:17 PDT, awjohnston1
standard8: review+
Details | Diff | Review

Description Mark Banner (:standard8) 2012-04-02 03:53:09 PDT
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.
Comment 1 Mark Banner (:standard8) 2012-04-25 07:37:13 PDT
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.
Comment 2 awjohnston1 2012-07-02 09:24:10 PDT
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
Comment 3 awjohnston1 2012-07-04 11:50:03 PDT
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?
Comment 4 Mark Banner (:standard8) 2012-07-05 02:28:02 PDT
(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.
Comment 5 awjohnston1 2012-07-07 12:24:47 PDT
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.
Comment 6 awjohnston1 2012-07-07 12:30:39 PDT
Also, should consider a Ctrl+Shift+Del command as in Firefox.
Comment 7 awjohnston1 2012-07-07 16:08:09 PDT
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.
Comment 8 awjohnston1 2012-07-09 15:01:10 PDT
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 Kent James (:rkent) 2012-07-09 15:25:21 PDT
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.
Comment 10 awjohnston1 2012-07-09 16:16:17 PDT
Created attachment 640407 [details] [diff] [review]
interfacePatchv2

corrected some typos.
Comment 11 awjohnston1 2012-07-12 12:55:43 PDT
Created attachment 641568 [details] [diff] [review]
clearHistoryPatchv3

Fixed a syntax error. The UI now appears to clear data successfully.
Comment 12 awjohnston1 2012-07-12 13:13:32 PDT
Created attachment 641572 [details] [diff] [review]
clearHistoryPatchv4

removed an unnecessary variable declaration.
Comment 13 awjohnston1 2012-07-12 13:19:42 PDT
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.
Comment 14 awjohnston1 2012-07-12 13:21:15 PDT
I also flagged the latest patch for review. Is there anything else I should be doing in regards to this?
Comment 15 awjohnston1 2012-07-12 13:48:46 PDT
Created attachment 641582 [details] [diff] [review]
clearHistoryv5

removed some code which is not necessary for thunderbird.
Comment 16 Mark Banner (:standard8) 2012-07-16 04:40:01 PDT
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.
Comment 17 awjohnston1 2012-07-16 13:22:39 PDT
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.
Comment 18 awjohnston1 2012-07-16 14:05:20 PDT
Created attachment 642720 [details] [diff] [review]
obsolete
Comment 19 awjohnston1 2012-07-16 14:08:17 PDT
Created attachment 642721 [details] [diff] [review]
clearHistoryPatchv7

fixed error, dialog appears to function as it should.
Comment 20 Blake Winton (:bwinton) (:☕️) 2012-07-16 14:34:47 PDT
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 21 Blake Winton (:bwinton) (:☕️) 2012-07-20 09:03:31 PDT
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.
Comment 22 awjohnston1 2012-07-20 11:42:46 PDT
Created attachment 644407 [details] [diff] [review]
clearHistoryPatchv8

trying to rebase. currently this patch fails to build.
Comment 23 awjohnston1 2012-07-20 12:13:35 PDT
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.
Comment 24 Blake Winton (:bwinton) (:☕️) 2012-07-20 12:17:28 PDT
(Did you change the UI between v7 and v9?  Cause if not, you can just carry forward my previous ui-r+…)
Comment 25 awjohnston1 2012-07-20 12:47:49 PDT
no, I did not change the UI. Didn't realize you had already reviewed :)
Comment 26 awjohnston1 2012-07-23 11:36:37 PDT
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?
Comment 27 Ryan VanderMeulen [:RyanVM] 2012-07-23 16:32:57 PDT
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
Comment 28 Mark Banner (:standard8) 2012-07-30 07:13:15 PDT
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?
Comment 29 Mark Banner (:standard8) 2012-07-30 07:24:43 PDT
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.
Comment 30 Mark Banner (:standard8) 2012-07-30 07:32:51 PDT
*** Bug 777867 has been marked as a duplicate of this bug. ***
Comment 31 awjohnston1 2012-07-30 13:17:03 PDT
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.
Comment 32 Mark Banner (:standard8) 2012-07-31 02:26:28 PDT
Comment on attachment 647271 [details] [diff] [review]
clearHistoryUpdatev1

That's great, thanks, I'll check this in in a few minutes.
Comment 33 Mark Banner (:standard8) 2012-07-31 02:55:40 PDT
Checked in: https://hg.mozilla.org/comm-central/rev/899e1f145586

Many thanks Andrew!

Note You need to log in before you can comment on or make changes to this bug.