Closed Bug 485773 Opened 13 years ago Closed 13 years ago

Forget This Site should only be enabled for single selections


(Firefox :: Bookmarks & History, defect)

Not set



Firefox 3.6a1


(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)



(Keywords: verified1.9.1)


(2 files)

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.
Attached patch Patch (v1)Splinter Review
Attachment #369872 - Flags: review?(mak77)
Wouldn't it be better to say: "Forget about these sites" and remove all traces from the selected domains?
(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 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[";1"].
>+           getService(Ci.nsIWindowWatcher);
>+  waitForExplicitFinish();
>+  // Add a history entry.
>+  let TEST_URIs = ["", ""];
>+  ok(PlacesUtils, "checking PlacesUtils, running in chrome context?");
>+  let history = PlacesUtils.history;
>+  TEST_URIs.forEach(function(TEST_URI) {
>+    history.addVisit(PlacesUtils._uri(TEST_URI), * 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");


>+              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);


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

.removeAllPages() please
Attachment #369872 - Flags: review?(mak77) → review+
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.
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Attachment #370822 - Flags: approval1.9.1?
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.
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.
Target Milestone: --- → Firefox 3.6a1
Attachment #370822 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 370822 [details] [diff] [review]
Patch (for check-in)

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.
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.

Component: Places → Bookmarks & History
QA Contact: places → bookmarks
Depends on: 936135
You need to log in before you can comment on or make changes to this bug.