Closed
Bug 485773
Opened 16 years ago
Closed 16 years ago
Forget This Site should only be enabled for single selections
Categories
(Firefox :: Bookmarks & History, defect)
Firefox
Bookmarks & History
Tracking
()
VERIFIED
FIXED
Firefox 3.6a1
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
References
Details
(Keywords: verified1.9.1)
Attachments
(2 files)
(deleted),
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
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 2•16 years ago
|
||
Wouldn't it be better to say: "Forget about these sites" and remove all traces from the selected domains?
Assignee | ||
Comment 3•16 years ago
|
||
(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•16 years ago
|
||
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
Attachment #369872 -
Flags: review?(mak77) → review+
Assignee | ||
Comment 5•16 years ago
|
||
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.
Assignee | ||
Comment 6•16 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/b11b259831f2
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Assignee | ||
Updated•16 years ago
|
Attachment #370822 -
Flags: approval1.9.1?
Assignee | ||
Comment 7•16 years ago
|
||
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•16 years ago
|
||
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.
Status: RESOLVED → VERIFIED
Target Milestone: --- → Firefox 3.6a1
Updated•16 years ago
|
Attachment #370822 -
Flags: approval1.9.1? → approval1.9.1+
Comment 9•16 years ago
|
||
Comment on attachment 370822 [details] [diff] [review] Patch (for check-in) a191=beltzner
Assignee | ||
Comment 10•16 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3ba1384ab3b8
Keywords: fixed1.9.1
Assignee | ||
Comment 11•16 years ago
|
||
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•16 years ago
|
||
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.
Keywords: fixed1.9.1 → verified1.9.1
Comment 13•15 years ago
|
||
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.
Description
•