257281 - add browser reset functionality

Status: RESOLVED FIXED

(Camino Graveyard :: General, enhancement)

Description

[Josh Aas]

A nice addition to Camino's privacy tools would be reset browser functionality.
Browser reset should remove cookies, cache, site prefs, history, saved usernames
and passwords, and downloads.

Comment 1

[Josh Aas]

Attachment: reset browser patch v1.0

Includes a patch, a modified main menu nib file to add the menu item, and a
modified Localizable.strings.

Comment 2

[Geoff "temporarily off bugmail" Beier]

(In reply to comment #1)
> Created an attachment (id=157286)
> reset browser patch v1.0
> 
> Includes a patch, a modified main menu nib file to add the menu item, and a
> modified Localizable.strings.

This looks good and works as stated. It seems that it would be a good idea
(though this should probably be a separate patch and a separate bug) to use
EvictEntries(nsICache::STORE_ANYWHERE) instead of just STORE_ON_DISK so that the
entire cache is cleared, including the RAM cache.

Since I don't have permission to edit the flag:
r=[e-mail]

Comment 3

[Josh Aas]

To clarify... nsICache::STORE_ON_DISK is used in the current pref pane to clear
cache, and that is why it is used in my patch. If the correct thing to do is use
nsICache::STORE_ANYWHERE, then that it a separate issue and must be resolved in
the pref pane and my patch.

Comment 4

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 157286 [details]
reset browser patch v1.0

> +- (IBAction)resetBrowser:(id)sender {

put brace on new line

> +    int i;
> +    
> +    // close all windows
> +    NSArray *windows = [NSApp orderedWindows];
> +    for (i = 0; i < [windows count]; i++) {

for (int i = 0; ....)

> +    if (mPermissionManager) {
> +      mPermissionManager->RemoveAll();
> +    }

lose the braces on loops/conditionals that only have 1 statement.

> +    nsCOMPtr<nsICookieManager> cm(do_GetService(NS_COOKIEMANAGER_CONTRACTID));
> +    nsICookieManager* mCookieManager = cm.get();
> +    NS_IF_ADDREF(mCookieManager);
> +    if (mCookieManager) {
> +      mCookieManager->RemoveAll();
> +    }
> +    NS_IF_RELEASE(mCookieManager);

why are you doing an additional addref/release? that's what the comPtr does.
you can remove those entirely.

> +  kcfindinternetpassword(NULL, NULL, NULL, kAnyPort, kKCProtocolTypeHTTP, kKCAuthTypeHTTPDigest,
> +                         kBufferLen, buffer, &actualSize, &itemRef);
> +  while (itemRef) {
> +    KCDeleteItem(itemRef);
> +    itemRef = NULL;
> +    kcfindinternetpassword(NULL, NULL, NULL, kAnyPort, kKCProtocolTypeHTTP, kKCAuthTypeHTTPDigest,
> +                           kBufferLen, buffer, &actualSize, &itemRef);
> +  }

i don't really like loops that duplicate code like this...how about using a
bool flag instead that you prime with true to get started and set to false when
|itemRef| is nil?

> +  for (int i = 0; i < [mProgressViewControllers count]; i++) {
> +    // the ProgressViewController method "cancel:" has a sanity check, so its ok to call on anything
> +    // make sure downloads are not active before removing them
> +    [[mProgressViewControllers objectAtIndex:i] cancel:self];
> +    [self removeDownload:[mProgressViewControllers objectAtIndex:i]]; // remove the download
> +    i--; // leave index at the same position because the dl there got removed
> +  }

you decrement |i| and then increment it. why bother? why not just not increment
it?

Comment 5

[Josh Aas]

Attachment: reset browser patch v2.0

This new patch fixes everything mentioned in Mike's comments, changes cache
clearing flag to STORED_ANYWHERE, and makes the alert panel a critical alert
panel instead of just a normal one.

Comment 6

[Geoff "temporarily off bugmail" Beier]

Comment on attachment 157449 [details]
reset browser patch v2.0

This patch appears to address Mike's concerns, and works as described.

r=[e-mail]

Comment 7

[Geoff "temporarily off bugmail" Beier]

I have opened Bug 257491 to address the policy flag for EvictEntries().

Comment 8

[Josh Aas]

*** Bug 172063 has been marked as a duplicate of this bug. ***

Comment 9

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 157449 [details]
reset browser patch v2.0

A few things:
- doesn't compile (stray brace)
- in the alert, the cancel button should be on the left and the default, not on
the right. That's the format of the "are you sure?" dialog.
- The title text should be more explanatory, something more than "reset camino"
- the secondary explanatory text should mention that this is not undoable

Comment 10

[Josh Aas]

I'll take care of that stuff ASAP.

Comment 11

[Josh Aas]

Mike - the problem with using NSAlert on 10.3 and the alert panel function on
panther is that since we compile against the 10.2.8 SDK there is no NSAlert
object to compile against. Its OK to call a method that might not exist, but we
can't refer to objects that don't exist in the SDK.

That means we have 2 options:
- write the warning panel from scratch (bad)
- use the existing appkit function as in v2.0 of the patch (not as bad)

The latter option means the button order will the the reverse of what you want,
but I don't think its a big enough deal to warrant writing this panel from scratch.

Comment 12

[Josh Aas]

Attachment: reset browser patch v3.0

This version of the patch fixes problems in v2.0 and resolve the button naming
argument per an IRC conversation with Pinkerton and others yesterday. The patch
files have also been updated to apply cleanly to today's code (e.g. takes into
account yesterday's Localizable.strings changes).

Comment 13

[Geoff "temporarily off bugmail" Beier]

(In reply to comment #12)
> This version of the patch fixes problems in v2.0 and resolve the button naming
> argument per an IRC conversation with Pinkerton and others yesterday. The patch
> files have also been updated to apply cleanly to today's code (e.g. takes into
> account yesterday's Localizable.strings changes).

I missed the IRC conversation yesterday, and the content wasn't posted here so I can't be sure... but the 
patch now has cancel on the left, with the destructive option as the default. Is having the destructive 
item for the default intentional? If so, r=[e-mail]. Destroys data as advertised.

Comment 14

[Geoff "temporarily off bugmail" Beier]

I just noticed this patch doesn't clear the number of visits stats kept for bookmarks. To be complete, 
should it do that? (maybe not, but thought i'd ask)

Comment 15

[Josh Aas]

I don't think it should. I think we should leave bookmarks alone.

The IRC conversation basically came down to looking at Mail and some other Apple
apps where the destructive option (e.g. deleting a Mail.app mailbox) is default
as it is in the new patch. You're already getting a warning, which is the point
of the dialog box in the first place.

Comment 16

[Mike Pinkerton (not reading bugmail)]

so that begs the question, how does someone clear the bookmark visits easily?
should we provide some ui?

also, do we want to provide a separate menu item to just clear cache?

Comment 17

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 161532 [details]
reset browser patch v3.0

+- (void)removeAllUsernamesAndPasswords {

put the brace on a separate line.

make sure when you land this, you correctly merge the localized.strings file.
I'm pretty sure it's changed since this patch got going.

sr=pink

Comment 18

[Josh Aas]

landed