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

VERIFIED FIXED in Firefox 3.1b2

Status

()

Firefox
Bookmarks & History
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: johnath, Assigned: sdwilsh)

Tracking

(Blocks: 2 bugs, {user-doc-complete})

Trunk
Firefox 3.1b2
user-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

9 years ago
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.
OS: Mac OS X → All
Hardware: PC → All
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"
oh wait, do you mean a delete by domain?
(Reporter)

Comment 3

9 years ago
(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).
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?
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.
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.
(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.
(Assignee)

Comment 8

9 years ago
Would this also delete visits to sites on subdomains for the given domain?
Assignee: nobody → sdwilsh
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 3.1b2
>Would this also delete visits to sites on subdomains for the given domain?

yeah, I think it should
(Assignee)

Comment 10

9 years ago
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?
(Assignee)

Updated

9 years ago
Summary: Library context menu for history items should include "Delete all visits to <site>" → Library context menu for history items should include "Delete all data to <domain>"
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?
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/*
(Assignee)

Comment 13

9 years ago
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.
(Assignee)

Comment 14

9 years ago
Of course, now we need a new accesskey - I'm assuming F which looks to be safe.
(Assignee)

Comment 15

9 years ago
Created attachment 346389 [details] [diff] [review]
v1.0

No automated tests, but I'll get to those tomorrow.  I've verified each piece that we clear works by hand.
Attachment #346389 - Flags: review?(mconnor)
Attachment #346389 - Flags: review?(dietrich)

Comment 16

9 years ago
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

9 years ago
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.)
(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)
(Assignee)

Comment 19

9 years ago
Created attachment 346406 [details] [diff] [review]
v1.1

Addresses comments.  I /think/ we are OK with the encoding stuff, but I'm not 100% sure.
Attachment #346389 - Attachment is obsolete: true
Attachment #346406 - Flags: review?(mconnor)
Attachment #346406 - Flags: review?(dietrich)
Attachment #346389 - Flags: review?(mconnor)
Attachment #346389 - Flags: review?(dietrich)

Updated

9 years ago
Attachment #346406 - Flags: review?(mconnor) → review+
(Assignee)

Comment 20

9 years ago
This patch also has the nice side effect of adding this to the history side bar.
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 on attachment 346406 [details] [diff] [review]
v1.1

r=me
Attachment #346406 - Flags: review?(dietrich) → review?
(Assignee)

Comment 23

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

Comment 24

9 years ago
Created attachment 346409 [details] [diff] [review]
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).
Attachment #346406 - Attachment is obsolete: true
Attachment #346406 - Flags: review?
(Assignee)

Updated

9 years ago
Whiteboard: [has patch][has review][can land]
(Assignee)

Comment 25

9 years ago
I don't think this is going to need a litmus test.  I do not see why I cannot fully automate the testing here.
Flags: in-testsuite?
Flags: in-litmus-
(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.
(Assignee)

Comment 27

9 years ago
http://hg.mozilla.org/mozilla-central/rev/dcec193ba5d7

I'll get tests later this week.
Whiteboard: [has patch][has review][can land]
(Assignee)

Updated

9 years ago
Whiteboard: [needs tests]
(Assignee)

Comment 28

9 years ago
(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 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.
(Assignee)

Comment 30

9 years ago
(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.
(Assignee)

Comment 31

9 years ago
Created attachment 346574 [details] [diff] [review]
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.
(Assignee)

Comment 32

9 years ago
Created attachment 346575 [details] [diff] [review]
tests v0.1

hg adding a file is helpful...
Attachment #346574 - Attachment is obsolete: true
Depends on: 463445
(Assignee)

Comment 33

9 years ago
Created attachment 346789 [details] [diff] [review]
tests & correctness fixes v1.0
[Checkin: Comment 37]

Found a few minor bugs while writing the tests.
Attachment #346575 - Attachment is obsolete: true
Attachment #346789 - Flags: review?(mconnor)
(Assignee)

Updated

9 years ago
Whiteboard: [needs tests] → [has patch][needs review mconnor]
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.
(Assignee)

Comment 35

9 years ago
(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 on attachment 346789 [details] [diff] [review]
tests & correctness fixes v1.0
[Checkin: Comment 37]

looks good, test coverage yay!
Attachment #346789 - Flags: review?(mconnor)
Attachment #346789 - Flags: review+
Attachment #346789 - Flags: approval1.9.1b2+
(Assignee)

Updated

9 years ago
Whiteboard: [has patch][needs review mconnor] → [has patch][has review][can land]
Pushed: http://hg.mozilla.org/mozilla-central/rev/f49c32a186b8
Flags: in-testsuite? → in-testsuite+
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Depends on: 464909
Whiteboard: [has patch][has review][can land]
Attachment #346789 - Attachment description: tests & correctness fixes v1.0 → tests & correctness fixes v1.0 [Checkin: Comment 37]
Blocks: 464199
Blocks: 464417
Blocks: 464949
I know this got a "-" for inlitmus?, but I still think there should be a manual test in Litmus for the functionality.
(Assignee)

Comment 39

9 years ago
(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

Updated

9 years ago
Blocks: 465082
Depends on: 464942
Updated summary based on current implementation.
Summary: Library context menu for history items should include "Delete all data to <domain>" → Library context menu for history items should include "Forget About This Site"
verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090127 Shiretoko/3.1b3pre
Status: RESOLVED → VERIFIED
Depends on: 485773

Updated

8 years ago
Keywords: user-doc-needed
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
Keywords: user-doc-needed → user-doc-complete
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.