460086 - Library context menu for history items should include "Forget About This Site"

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Johnathan Nightingale [:johnath]]

Alex mentioned this in email and I think it's a great idea, particularly in conjunction with bug 403961.  Obviously deleting individual entries is still a thing people may do, and I know that people could just sort by site and then multi-select+delete but this feels like an easy, happy-making thing to do.

Comment 1

[Marco Bonardo [:mak]]

our history views are already working like that, if you delete from history we think you want to get rid of the uri from the history, so all visits to that uri are removed... That was done by design from the start.

So that is practically asking a string change for the context menu "delete"

Comment 2

[Marco Bonardo [:mak]]

oh wait, do you mean a delete by domain?

Comment 3

[Johnathan Nightingale [:johnath]]

(In reply to comment #2)
> oh wait, do you mean a delete by domain?

Yes, exactly.  "Delete all visits to bugzilla.mozilla.org" since, if you're in a history-deleting mood, this might well be your intended behaviour (pretend I was never here).

Comment 4

[Marco Bonardo [:mak]]

so this is something we should have, exactly the option should be valid for both history and bookmarks, so when i right click on a places node i can choose "Clear history for 'www.domain.com'" (or "delete all visits to 'www.domain.com'"), my only fear is that this will make the context menu really large, so how can we solve this?

Comment 5

[Marco Bonardo [:mak]]

or maybe we could add a submenu
Delete all visits to > | this page       |
                       | www.domain.com" |

hwv the first option would practically duplicate the "delete" context option functionality for history entries (on delete we are removing all visits to that page), so we should remove the "delete" option from history entries and the user should always have to go to the submenu.

Comment 6

[Alex Faaborg [:faaborg] (Firefox UX)]

Perhaps "Delete all Visits to this Site" instead of including the domain name, since some domain names are obviously very long.  This still isn't that short of a command though.

Comment 7

[Marco Bonardo [:mak]]

(In reply to comment #6)
> Perhaps "Delete all Visits to this Site" instead of including the domain name,
> since some domain names are obviously very long.  This still isn't that short
> of a command though.

what for hosted websites (www.hostingcompany/site or www.servicecompany/service1)? delete visits to this site is different than delete visits for this domain, so it could be misinterpreted.

Comment 8

[Shawn Wilsher :sdwilsh]

Would this also delete visits to sites on subdomains for the given domain?

Comment 9

[Alex Faaborg [:faaborg] (Firefox UX)]

>Would this also delete visits to sites on subdomains for the given domain?

yeah, I think it should

Comment 10

[Shawn Wilsher :sdwilsh]

This clears all relevant data for the following:
history
cache
cookies
downloads
passwords
permissions
content preferences

So, what should the string be, and what about the accesskey?

Comment 11

[Alex Faaborg [:faaborg] (Firefox UX)]

if you are clearing all of that, the string should be "Clear all History of
this Site"  we want all that stuff to == "history" everywhere in the UI.  Since
C and A are taken, I guess the access key goes to H?

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

That seems pretty cumbersome as far as context menu actions go. Why not something simpler like "Forget This Site" or "Forget %s" where %s is the domain name.

I'm still pretty concerned that it will be hard for users to differentiate between "Delete" and this function, which doesn't just delete data for foo.org/bar/baz, but actually deletes foo.org/*

Comment 13

[Shawn Wilsher :sdwilsh]

I think I'm going to go with "Forget This Site" since the domain name can be long and cumbersome too.

Also note, that we will be deleting *foo.org/* from your example.

Comment 14

[Shawn Wilsher :sdwilsh]

Of course, now we need a new accesskey - I'm assuming F which looks to be safe.

Comment 15

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

No automated tests, but I'll get to those tomorrow.  I've verified each piece that we clear works by hand.

Comment 16

[[e-mail]]

Comment on attachment 346389 [details] [diff] [review]
v1.0

>+    // Cookies
>+    let (cm = Cc["@mozilla.org/cookiemanager;1"].
>+              getService(Ci.nsICookieManager)) {
>+      let enumerator = cm.enumerator;
>+      while (enumerator.hasMoreElements()) {
>+        let cookie = enumerator.getNext().QueryInterface(Ci.nsICookie);
>+        if (cookie.host.indexOf(aDomain) != -1)

wouldn't you want to do an "ends with" match? otherwise you could be matching foo.com against foo.comerica.com...

Comment 17

[[e-mail]]

also you may need to worry about url encodings (utf8 vs ace and normalization) depending on where aDomain comes from. (cookie hosts are ace encoded, fyi.)

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #13)
> I think I'm going to go with "Forget This Site" since the domain name can be
> long and cumbersome too.

Shawn and I talked about this and decided that the real issue was ensuring that users understood the difference between removing a single page from history and forgetting everything about the site. We ended up with:

 |----------------------------
 | Delete This Page
 | Forget About This Site
 '----------------------------

This resolves two issues:

1. Illustrates the difference between page and site
2. Illustrates the difference between delete (from the list the user is looking at) and removing all history (forget about)

Comment 19

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

Addresses comments.  I /think/ we are OK with the encoding stuff, but I'm not 100% sure.

Comment 20

[Shawn Wilsher :sdwilsh]

This patch also has the nice side effect of adding this to the history side bar.

Comment 21

[(no longer active)]

The way this is implemented now, if one of the services throw during the
clearing process, then the rest of the work is aborted.  Is this
desired/expected?

Comment 22

[Dietrich Ayala (:dietrich)]

Comment on attachment 346406 [details] [diff] [review]
v1.1

r=me

Comment 23

[Shawn Wilsher :sdwilsh]

I talked about this with Johnathan.  We really don't expect them to throw, and if they do, we should propagate that exception.

Comment 24

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

I forgot to call cancelDownload before trying to remove it for active downloads.  Noticed that during mconnor's review.  That is fixed, and this attachment is good to land (has the commit message embedded in it so it can be hg imported).

Comment 25

[Shawn Wilsher :sdwilsh]

I don't think this is going to need a litmus test.  I do not see why I cannot fully automate the testing here.

Comment 26

[Justin Dolske [:Dolske]]

(In reply to comment #23)
> We really don't expect them to throw, and
> if they do, we should propagate that exception.

Well, getAllLogins() will throw if it triggers a master password prompt and the user clicks cancels. That's probably something that should be caught and ignored.

Comment 27

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/dcec193ba5d7

I'll get tests later this week.

Comment 28

[Shawn Wilsher :sdwilsh]

(In reply to comment #26)
> Well, getAllLogins() will throw if it triggers a master password prompt and the
> user clicks cancels. That's probably something that should be caught and
> ignored.
Can you do me a favor and file a new bug on that?

Comment 29

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 346409 [details] [diff] [review]
v1.2

>+    // Content Preferences

This part of the patch is ok, although I'm somewhat concerned about how it assumes some of the implementation details of the content pref service.

Comment 30

[Shawn Wilsher :sdwilsh]

(In reply to comment #29)
> This part of the patch is ok, although I'm somewhat concerned about how it
> assumes some of the implementation details of the content pref service.
I'm open to suggestions, but as the current API stands, I don't have much of a choice.  The unit tests will make sure that if the implementation changes, the test will end up failing.

Comment 31

[Shawn Wilsher :sdwilsh]

Attachment: tests v0.1

This doesn't actually pass because I've exposed a bug in the implementation.  This includes tests for history, cache, and cookies.

Comment 32

[Shawn Wilsher :sdwilsh]

Attachment: tests v0.1

hg adding a file is helpful...

Comment 33

[Shawn Wilsher :sdwilsh]

Attachment: tests & correctness fixes v1.0 [Checkin: Comment 37]

Found a few minor bugs while writing the tests.

Comment 34

[(no longer active)]

Comment on attachment 346789 [details] [diff] [review]
tests & correctness fixes v1.0
[Checkin: Comment 37]

>diff --git a/browser/components/privatebrowsing/test/unit/test_removeDataFromDomain.js b/browser/components/privatebrowsing/test/unit/test_removeDataFromDomain.js

>+function uri(aURIString)

How about moving this to the head file?

>+function test_cache_cleared()
>+{
...
>+  // NOTE: We could be more extensive with this test and actually add an entry
>+  //       to the cache, and then make sure it is gone.  However, we trust that
>+  //       the API is well tested, and that when we get the observer
>+  //       notification, we have actually cleared the cache.

Please note that the cache service doesn't have any automated tests (see bug 460431), so I think it would be better to test this explicitly.

Comment 35

[Shawn Wilsher :sdwilsh]

(In reply to comment #34)
> >+function uri(aURIString)
> 
> How about moving this to the head file?
Note that that function is different in each file I use it in.

> Please note that the cache service doesn't have any automated tests (see bug
> 460431), so I think it would be better to test this explicitly.
That's awesome, but it'd probably take at least half a day (if not a full day) to write that comprehensive test.  I think it would probably be better to ensure that there is a litmus test to ensure that the cache is cleared on clear private data (I bet there already is one too).  There's only one way to clear the cache, and everyone uses it.

Comment 36

[Mike Connor [:mconnor]]

Comment on attachment 346789 [details] [diff] [review]
tests & correctness fixes v1.0
[Checkin: Comment 37]

looks good, test coverage yay!

Comment 37

[Dave Townsend [:mossop]]

Pushed: http://hg.mozilla.org/mozilla-central/rev/f49c32a186b8

Comment 38

[Marcia Knous [:marcia]]

I know this got a "-" for inlitmus?, but I still think there should be a manual test in Litmus for the functionality.

Comment 39

[Shawn Wilsher :sdwilsh]

(In reply to comment #38)
> I know this got a "-" for inlitmus?, but I still think there should be a manual
> test in Litmus for the functionality.
It has full automated coverage, so there's not need to waste human time on it IMHO

Comment 40

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Updated summary based on current implementation.

Comment 41

[Tracy Walker [:tracy]]

verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090127 Shiretoko/3.1b3pre

Comment 42

[Chris Ilias [:cilias]]

user-doc-complete
This was really applied to a whole bunch of articles, but the main one was: https://support.mozilla.com/en-US/kb/Clear+Recent+History#Clearing_history_for_a_single_site

Comment 43

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv