485773 - Forget This Site should only be enabled for single selections

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[(no longer active)]

Currently the Forget This Site places context menu item is enabled for multiple selections as well, but we don't actually handle this command for multiple nodes.  We need to make sure that it's only enabled for single selections.

The attached patch does this, and also includes an automated test.

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Comment 2

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

Wouldn't it be better to say: "Forget about these sites" and remove all traces from the selected domains?

Comment 3

[(no longer active)]

(In reply to comment #2)
> Wouldn't it be better to say: "Forget about these sites" and remove all traces
> from the selected domains?

No, I don't think so, because of two reasons:

1. This may be error prone, as the user may have selected a site and scrolled away so that the selection is no longer visible, and may end up deleting unwanted history items.

2. If a user actively wants to clear multiple sites, it would be awkward to search through the list manually and select the desired sites.  Instead, a much more streamlined interaction flow would be to use the search bar to search for the desired sites, and remove each of them one by one.

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 369872 [details] [diff] [review]
Patch (v1)

>diff --git a/browser/components/places/tests/browser/browser_forgetthissite_single.js b/browser/components/places/tests/browser/browser_forgetthissite_single.js
>+function test() {
>+  // initialization
>+  let ww = Cc["@mozilla.org/embedcomp/window-watcher;1"].
>+           getService(Ci.nsIWindowWatcher);
>+  waitForExplicitFinish();
>+
>+  // Add a history entry.
>+  let TEST_URIs = ["http://www.mozilla.org/test1", "http://www.mozilla.org/test2"];
>+  ok(PlacesUtils, "checking PlacesUtils, running in chrome context?");
>+  let history = PlacesUtils.history;
>+  TEST_URIs.forEach(function(TEST_URI) {
>+    history.addVisit(PlacesUtils._uri(TEST_URI), Date.now() * 1000,
>+                     null, PlacesUtils.history.TRANSITION_TYPED, false, 0);
>+  });

check visits have been effectively added (check returned visitId or use IsURIStringVisited

>+  function testForgetThisSiteVisibility(selectionCount, funcNext) {
>+    let observer = {
>+      observe: function(aSubject, aTopic, aData) {
>+        if (aTopic === "domwindowopened") {
>+          ww.unregisterNotification(this);
>+          let organizer = aSubject.QueryInterface(Ci.nsIDOMWindow);
>+          organizer.addEventListener("load", function onLoad(event) {
>+            organizer.removeEventListener("load", onLoad, false);
>+            executeSoon(function () {
>+              // Select History in the left pane.
>+              organizer.PlacesOrganizer.selectLeftPaneQuery('History');
>+              // Select the first history entry.
>+              let doc = organizer.document;
>+              let tree = doc.getElementById("placeContent");

organizer.PlacesOrganizer._content

>+              let selection = tree.view.selection;
>+              selection.clearSelection();
>+              selection.rangedSelect(0, selectionCount - 1, true);

check selection range is as big as expected

>+              // Open the context menu
>+              let contextmenu = doc.getElementById("placesContext");
>+              contextmenu.addEventListener("popupshown", function() {
>+                contextmenu.removeEventListener("popupshown", arguments.callee, false);
>+                let forgetThisSite = doc.getElementById("placesContext_deleteHost");
>+                let expected = (selectionCount == 1);

expected var name is a bit generic in this case, you could maybe call this hideForgetThisSite = selectionCount != 1, just for readability

>+              let event = document.createEvent("MouseEvents");
>+              event.initMouseEvent("contextmenu", true, true, organizer, 0,
>+                                   0, 0, 0, 0, false, false, false, false,
>+                                   0, null);
>+              tree.dispatchEvent(event);

synthesizeMouse?

>+        history.QueryInterface(Ci.nsIBrowserHistory)
>+               .removePage(PlacesUtils._uri(TEST_URI));

.removeAllPages() please

Comment 5

[(no longer active)]

Attachment: Patch (for check-in)

I updated the patch according to the review comments, and also made sure it passes with bug 422163 landed.

Please note that synthesizeMouse could not be used, because it only works in the |window| of the JS scope.

Comment 6

[(no longer active)]

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

Comment 7

[(no longer active)]

Comment on attachment 370822 [details] [diff] [review]
Patch (for check-in)

This is a very low risk patch which comes with an automated test.  Requesting approval to land on 1.9.1.

Comment 8

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

Verified fixed on trunk with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090403 Minefield/3.6a1pre ID:20090403030700 and on Windows.

Comment 9

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

Comment on attachment 370822 [details] [diff] [review]
Patch (for check-in)

a191=beltzner

Comment 10

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3ba1384ab3b8

Comment 11

[(no longer active)]

Fixed a typo in the test on both trunk and branch:

<http://hg.mozilla.org/mozilla-central/rev/6451bf07db7e>
<http://hg.mozilla.org/releases/mozilla-1.9.1/rev/98b68ac484cc>

Comment 12

[Marcia Knous [:marcia]]

Verified fixed on the 1.9.1 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090416 Shiretoko/3.5b4pre as well as the equivalent Windows Vista nightly.

Comment 13

[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