In the Library accessed by History -> Show All History, we want to add a folder for Downloads. faaborg suggested that it go below History but above Tags.

Comment on attachment 444481 [details] [diff] [review] Patch to add the enhancement >+++ b/browser/components/places/src/PlacesUIUtils.jsm Mon May 10 > // All queries but PlacesRoot. >- const EXPECTED_QUERY_COUNT = 6; >+ const EXPECTED_QUERY_COUNT = 7; This is just queries.length - 1 (for places root) right? Maybe we should just set it to that (check with mak). You should add a UI test (like these: http://mxr.mozilla.org/mozilla-central/source/browser/components/places/tests/browser/ browser_library*) to make sure this works as expected. The test should also test that passing the right open string will open it with downloads selected (may be able to modify an existing test for this as well).

(In reply to comment #1) > >+ const EXPECTED_QUERY_COUNT = 7; > This is just queries.length - 1 (for places root) right? Maybe we should just > set it to that (check with mak). queries is an object, not an array, and __count__ has been deprecated, I guess that's the reason this is hardcoded.

+ this.create_query("Downloads", leftPaneRoot, + "place:transition=" + + Ci.nsINavHistoryService.TRANSITION_DOWNLOAD + I see here "transition=number", how is the syntax if there is more than one transition specified? transition=number&transition=anotherNumber?

(In reply to comment #3) > I see here "transition=number", how is the syntax if there is more than one > transition specified? transition=number&transition=anotherNumber? Yes, that is exactly correct.

Comment on attachment 444481 [details] [diff] [review] Patch to add the enhancement As Sdwilsh said, any small b-c test you could add to ensure the "download" folder is there, selectable and working, will be much appreciated. You should also run browser-chrome tests to ensure they pass with this additional entry (some of the tests are checking left pane corruption handling). Also, you should ask UX for an icon (unless we can use one that is already there, like from the DM) and update places.css to add an OrganizerQuery_Downloads entry to it. See http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/browser/places/places.css#56 (Clearly both for winstripe, pinstripe and gnomestripe) This is an r- only for the icon part, otherwise it looks fine.

Contacted shorlander about getting an icon yesterday. In the meantime, feedback on this patch would be appreciated.

Comment on attachment 444967 [details] [diff] [review] Adds a test case and fixes a bug with History not updating on a download. >diff -r fc80c7dbc8e8 browser/components/places/tests/browser/browser_library_downloads.js >+ * The Initial Developer of the Original Code is Mozilla Corp. s/Mozilla Corp./the Mozilla Foundation/ >+ * Portions created by the Initial Developer are Copyright (C) 2009 2010 >+ * Contributor(s): >+ * Mehdi Mulani (mmulani@mozilla.com) This should be Mehdi Mulani <mmulani@mozilla.com> (original author) >+/* >+ * Tests bug 564900: Add folder specifically for downloads to Library sidebar >+ * https://bugzilla.mozilla.org/show_bug.cgi?id=564900 >+ */ Ok, but what the test will do and ensure? >+function test() { >+ waitForExplicitFinish(); >+ >+ // Open Library >+ let ww = Cc["@mozilla.org/embedcomp/window-watcher;1"]. >+ getService(Ci.nsIWindowWatcher); nit: you could use Services.ww >+ function windowObserver(aSubject, aTopic, aData) { >+ if (aTopic != "domwindowopened") >+ return; >+ ww.unregisterNotification(windowObserver); >+ let win = aSubject.QueryInterface(Ci.nsIDOMWindow); >+ win.addEventListener("load", function onLoad(event) { >+ win.removeEventListener("load", onLoad, false); >+ executeSoon(function () { >+ let PlacesOrg = win.PlacesOrganizer; >+ trailing spaces >+ //add visits to compare contents with Space after //, uppercase and end comment with a period. >+ fastAddVisit("http://mozilla.com", >+ PlacesUtils.history.TRANSITION_TYPED); >+ fastAddVisit("http://google.com", >+ PlacesUtils.history.TRANSITION_DOWNLOAD); >+ fastAddVisit("http://en.wikipedia.org", >+ PlacesUtils.history.TRANSITION_TYPED); >+ fastAddVisit("http://ubuntu.org", >+ PlacesUtils.history.TRANSITION_DOWNLOAD); >+ >+ //make sure Downloads is present ditto >+ isnot(PlacesOrg._places.selectedNode, null, >+ "Downloads is present and selected"); >+ >+ //make sure content in right pane exists ditto >+ let tree = win.document.getElementById("placeContent"); >+ isnot(tree, null, "placeContent tree exists"); >+ >+ //check results ditto >+ var content = win.document.getElementById("placeContent") >+ .getResult().root; .result.root should work, also i'd rename content to contentRoot >+ var len = content.childCount; >+ var testUris = ["http://ubuntu.org/", "http://google.com/"]; >+ for (var i = 0; i < len; i++) { >+ is(content.getChild(i).uri, testUris[i], >+ "Comparing downloads shown at index " + i); >+ } >+ >+ //done tests useless comment

(In reply to comment #7) > nit: you could use Services.ww And by that you mean Services.jsm

Comment on attachment 445199 [details] [diff] [review] Cleans up code and fixes tiny bug as pointed out by :adw. >diff -r fc80c7dbc8e8 browser/components/places/tests/browser/browser_library_downloads.js >+ * The Initial Developer of the Original Code is Mozilla Foundation well, this should be "is the Mozilla Foundation." (note "the" and the ending period) >+ // Open Library. you can kill this comment, the below showPlacesOrganizer("Downloads") is pretty clear (and you could just update the global test comment saying "This test adds visits... then opens the Library and checks..." >+ let ww = Services.ww; since there are only a couple call points for this, just hardcode Services.ww there, the temp var does not look useful in this case. >+ let PlacesOrg = win.PlacesOrganizer; this is used only in one place, just hardcode it there, local var is not useless. >+ // Clean up and terminate. >+ Cc["@mozilla.org/browser/nav-history-service;1"] >+ .getService(Ci.nsINavHistoryService) >+ .QueryInterface(Ci.nsIBrowserHistory).removeAllPages(); >+ win.close(); >+ finish(); you should change a bit this thing, look in mxr for a waitForClearHistory function, copy it in your test and do win.close(); waitForClearHistory(finish); this way you ensure everything has been cleaned up (since clear history is partially async). as a side note you can also use PlacesUtils.bhistory.removeAllPages(); (bhistory is a getter for browserHistory) >+function fastAddVisit(uri, transition) { >+ PlacesUtils.history.addVisit(PlacesUtils._uri(uri), Date.now() * 1000, >+ null, transition, false, 0); >+} This is going to give you an headache, you should have a global let now = Date.now(); and in your fastAddVisit you should pass now++ to addVisit Why? Because tests run in virtual machines, and due to timers issues in virtual machines two next calls to Date.now() can return the same exact value (timers are much lazier there). Since your query returns results ordered by date, you could be surprised to see that sometimes they are in "random" order

s/local var is not useless/local var is useless/

mak: should toolkit/components/places/src/PlacesDBUtils.jsm be changed to reflect this additional folder?

(In reply to comment #12) > mak: > should toolkit/components/places/src/PlacesDBUtils.jsm be changed to reflect > this additional folder? If it would touch the left pane folder I'd be really sad, have you found any point of that that worries you? The short answer is: "No, I don't think it touches this things at all, since LeftPane is completely handled in PlacesUIUtils".

Comment on attachment 447112 [details] [diff] [review] Fixed test failing on bug 510634. >diff --git a/browser/components/places/src/PlacesUIUtils.jsm b/browser/components/places/src/PlacesUIUtils.jsm >diff --git a/browser/components/places/tests/browser/browser_library_downloads.js b/browser/components/places/tests/browser/browser_library_downloads.js >+/* >+ * Tests bug 564900: Add folder specifically for downloads to Library sidebar nit: "Library left pane." >+ // Make sure content in right pane exists. >+ let tree = win.document.getElementById("placeContent"); >+ isnot(tree, null, "placeContent tree exists"); >+ >+ // Check results. >+ var contentRoot = win.document.getElementById("placeContent") >+ .getResult().root; does = tree.result.root; work? (I don't recall if we added a .result getter, but it should work) btw, you already assigned the element to "let tree", so no reasons to repeat the getElementById() call. >+function waitForClearHistory(aCallback) { >+ let hs = Cc["@mozilla.org/browser/nav-history-service;1"]. >+ getService(Ci.nsINavHistoryService); >+ hs.QueryInterface(Ci.nsIBrowserHistory).removeAllPages(); This could be just PlacesUtils.bHistory.removeAllPages(); >diff --git a/browser/components/places/tests/chrome/test_0_bug510634.xul b/browser/components/places/tests/chrome/test_0_bug510634.xul > // The query-property is set on the title column for each row. > let titleColumn = tree.treeBoxObject.columns.getColumnAt(0); > >- ["History", "Tags", "AllBookmarks", "BookmarksToolbar", >- "BookmarksMenu", "UnfiledBookmarks"].forEach( >+ // Open All Bookmarks >+ var itemsArr = ["History", "Downloads", "Tags", "AllBookmarks", >+ "BookmarksToolbar", "BookmarksMenu", "UnfiledBookmarks"]; >+ PlacesUtils.asContainer(tree.view >+ .nodeForTreeIndex(itemsArr.indexOf("AllBookmarks"))) >+ .containerOpen = true; the comment should be just before you open the node, but... ...to open the node you could just do: win.PlacesOrganizer.selectLeftPaneQuery("AllBookmarks"); it will select and open the node for you (crazy eh). then you can bring back the inline array as it was before and you have something like a one line change. r=me with these fixed

tree.result appears to not be defined so I stuck with tree.getResult() changed opening "AllBookmarks" to not rely on a constant. Also changed the checking of row properties to not rely on order or indices.

(In reply to comment #16) > Created an attachment (id=450371) [details] > Fixed as per mak's comments. Tryserver builds?

Comment on attachment 450371 [details] [diff] [review] Fixed as per mak's comments. Reply to Azat: Have pushed previous patches for this bug but not this specifically. Will be pushing one once the TryServer returns to normal. (tomorrow?) Also, requesting review from Marco as I've made a few mistakes on the previous patches and want to make sure this one is good :P

(In reply to comment #16) > Created an attachment (id=450371) [details] > Fixed as per mak's comments. > > tree.result appears to not be defined so I stuck with tree.getResult() strange it is correctly used in this test http://mxr.mozilla.org/mozilla-central/source/browser/components/places/tests/chrome/test_0_bug510634.xul#128 and I cannot find tree code that is using getResult(), indeed it changed recently to simple .result. Are you sure you're on the lastest mozilla-central? The only GetResult we still have is from a node. See this diff: http://hg.mozilla.org/mozilla-central/diff/d7393e28fb2d/browser/components/places/content/tree.xml

Comment on attachment 450371 [details] [diff] [review] Fixed as per mak's comments. >diff --git a/browser/components/places/tests/browser/browser_library_downloads.js b/browser/components/places/tests/browser/browser_library_downloads.js >+function waitForClearHistory(aCallback) { >+ const TOPIC_EXPIRATION_FINISHED = "places-expiration-finished"; IIRC I added PlacesUtils.TOPIC_EXPIRATION_FINISHED, you should be able to use that. >diff --git a/browser/components/places/tests/chrome/test_0_bug510634.xul b/browser/components/places/tests/chrome/test_0_bug510634.xul >- ["History", "Tags", "AllBookmarks", "BookmarksToolbar", >+ // Open All Bookmarks >+ tree.selectItems([PlacesUIUtils.leftPaneQueries["AllBookmarks"]]); >+ PlacesUtils.asContainer(tree.selectedNode).containerOpen = true; Can you add a sanity check that tree.selectedNode is what we expect? >+ let found = false; >+ for (let i = 0; i < tree.view.rowCount; i++) { >+ let rowProperties = createSupportsArray(); >+ tree.view.getCellProperties(i, titleColumn, rowProperties); >+ rowProperties = convertPropertiesToJSArray(rowProperties); >+ if (rowProperties.indexOf("OrganizerQuery_" + aQueryName) != -1) { >+ found = true; >+ break; you could: for (let i = 0; i < tree.view.rowCount && !found; i++) { and later: found = rowProperties.indexOf("OrganizerQuery_" + aQueryName) != -1; make sure that you're on latest mozilla-central for the getResult vs result thing as said above r=me with those fixed

Added a check for the All Bookmarks folder by comparing tree.selectedNode.itemId and PlacesUIUtils.allBookmarksFolderId.

Pushed this to try yesterday, failed to pass one test on Linux with the error messages: TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/browser/components/places/tests/chrome/test_bug427633_no_newfolder_if_noip.xul | [SimpleTest/SimpleTest.js, window.onerror] An error occurred - uncaught exception: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsINavHistoryService.executeQueries]" nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)" location: "JS frame :: chrome://browser/content/places/tree.xml :: load :: line 82" data: no] at :0 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/chrome/browser/components/places/tests/chrome/test_bug427633_no_newfolder_if_noip.xul | [SimpleTest/SimpleTest.js, window.onerror] An error occurred - tree.view is null at chrome://mochikit/content/chrome/browser/components/places/tests/chrome/test_bug427633_no_newfolder_if_noip.xul:107 Rebased and then pushed to try again, seems to be passing now.

Comment on attachment 451131 [details] [diff] [review] Added additional checks and changed getResult to result. Alrighty then. This has review, passes the try server, and is generally low risk. a=sdwilsh

http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=3978c4ee890b Suspected of causing Bug 582661

(In reply to comment #24) > http://hg.mozilla.org/mozilla-central/rev/210e4d354aec Seems like you've forget about icons for winstripe-aero (jar.mn)

Azat: good call. Fixed the jar.mn file but have still not figured out the orange tree issue.

The cause for the bug failing seemed to be the leftPaneFolderId being reset by an earlier test, this patch changes the test to restore the getter after using it. Pushed this to try last night and it passed with lots of debug info, removed the debug info and pushed again today and it also seems to be passing.

Comment on attachment 463270 [details] [diff] [review] Appears to fix failing bug from mak's advice. >diff --git a/browser/components/places/tests/chrome/test_0_multiple_left_pane.xul b/browser/components/places/tests/chrome/test_0_multiple_left_pane.xul >--- a/browser/components/places/tests/chrome/test_0_multiple_left_pane.xul >+++ b/browser/components/places/tests/chrome/test_0_multiple_left_pane.xul >@@ -86,19 +86,25 @@ > let fakeLeftPaneRoot = bs.createFolder(PlacesUtils.placesRootId, "", > bs.DEFAULT_INDEX); > as.setItemAnnotation(fakeLeftPaneRoot, PlacesUIUtils.ORGANIZER_FOLDER_ANNO, > PlacesUIUtils.ORGANIZER_LEFTPANE_VERSION, 0, > as.EXPIRE_NEVER); > fakeLeftPanes.push(fakeLeftPaneRoot); > } while (fakeLeftPanes.length < 2); > >+ let cachedLeftPaneFolderIdGetter = >+ PlacesUIUtils.__lookupGetter__("leftPaneFolderId"); >+ > // Initialize the left pane queries. > PlacesUIUtils.leftPaneFolderId; > >+ PlacesUIUtils.__defineGetter__("leftPaneFolderId", >+ cachedLeftPaneFolderIdGetter); >+ > // Check left pane. > ok(PlacesUIUtils.leftPaneFolderId > 0, This looks a no-op, because it's restoring a getter to call it (and thus remove it) immediately later. Btw, I don't think the issue is in this test, I think instead that test_0_bug510634.xul should not only save and restore leftPaneFolderId getter, but also allBookmarksFolderId getter. The latter depends in fact by values calculated by the former.

Sorry, I was directed here from another bug. Is this just to appear in a sidebar or will this also handle bookmarks history appearing in tab?

Apologise again, just seen this: "You should be able to have History in a tab eventually, but for 4.0 it'll probably still be in a window. Post 4.0, we'll invest some time in improving and rethinking history/bookmarks and related UI." Ignore the previous comment.

Hello Mehdi, as you may have seen I've created a patch in bug 591289 that makes the new Downloads history view much more useful by making the window display the file name in addition to the original URL. This is probably the missing piece for having the new Downloads UI in bug 564934 ready for the next beta. Do you think you are ready to take the final steps for your patch and have it completed, let's say next week? This would be of great help!

Ran this patch through TryServer with greens all around :D

(In reply to comment #33) > Created attachment 478115 [details] [diff] [review] > Moved changes to different patch. > > Ran this patch through TryServer with greens all around :D Sorry for spam, meant that I moved changes to different test as per comment 29

Are you aiming to land this for FF4?

(In reply to comment #35) > Are you aiming to land this for FF4? not specifically though apparently it could land if good enough. I would prefer to land this post-FF4 so that bug 564916 could land at the same time. (It will not land as it changes API.)

Comment on attachment 478115 [details] [diff] [review] Moved changes to different patch. >diff --git a/browser/components/places/tests/browser/browser_library_downloads.js b/browser/components/places/tests/browser/browser_library_downloads.js >new file mode 100644 >--- /dev/null >+++ b/browser/components/places/tests/browser/browser_library_downloads.js >@@ -0,0 +1,110 @@ >+/* ***** BEGIN LICENSE BLOCK ***** >+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1 ... >+ * ***** END LICENSE BLOCK ***** */ Please use /* Any copyright is dedicated to the Public Domain. * http://creativecommons.org/publicdomain/zero/1.0/ */ in new tests, so we avoid the long MPL boilerplate (it is allowed to use PD for tests, see http://www.mozilla.org/MPL/license-policy.html) >+let now = Date.now(); Please move this near fastAddVisit, so it's clearer where it's used nit: you could add a comment about the fact using now this way ensures correct order of visits and no duplicated times. the patch looks ok, I assume you tested it pretty well. You should file a followup to add updated icons for all platforms.

Comment on attachment 451131 [details] [diff] [review] Added additional checks and changed getResult to result. (bookkeeping)

Sinchan and I have discussed about what we need at a minimum from the Library window part of the panel-based Download Manager, and the result is here: https://wiki.mozilla.org/User:P.A./Panel-based_Download_Manager#Library_window The double-clicking behavior and the visual marking of download history entries are the two aspects that are more challenging to implement, if I understand correctly. There might also be some implementation caveats to be analyzed. For example, if we want to access the file system to determine if a download target still exists, we should determine at which time we want to do that, and make sure that the interface responsiveness is not affected if the download was saved to a slow medium.

The tests run fine on my machine and I have pushed the patch to tryserver: http://tbpl.mozilla.org/?tree=Try&rev=9db9b5011c4a

I didn't send the previous push to tryserver correctly. Here is an updated link to the tryserver: http://tbpl.mozilla.org/?tree=Try&rev=ad3d924d1fed

Comment on attachment 546615 [details] [diff] [review] Same patch, updated to apply on trunk Can you please review this patch? The tryserver builds were successful.

Comment on attachment 546615 [details] [diff] [review] Same patch, updated to apply on trunk Review of attachment 546615 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for pushing this out! ::: browser/components/places/tests/browser/browser_library_downloads.js @@ +88,5 @@ > + }, false); > + } > + > + Services.ww.registerNotification(windowObserver); > + PlacesCommandHook.showPlacesOrganizer("Downloads"); Could you add an additional optional argument to the openLibrary helper in head.js (that is the query name) and use that one instead, it's just matter of passing the eventual additional argument to the openDialog call there. This is a recent addition and since all tests go through it I prefer if we keep doing so. Then you can remove the ww observer and the load listener (and most likely the executeSoon). For reference see http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-places.js#467 and http://mxr.mozilla.org/mozilla-central/source/browser/components/places/tests/browser/head.js#18 and any test using openLibrary. @@ +106,5 @@ > + }; > + Services.obs.addObserver(observer, PlacesUtils.TOPIC_EXPIRATION_FINISHED, false); > + > + PlacesUtils.bhistory.removeAllPages(); > +} add this to head.js, please file a followup to use it in other tests that are currently defining their own.

I have moved all additional interaction with downloads to bug 675902, so this can land as it is.

Can you please take a look at browser_library_downloads.js to make sure that I made the changes that you requested? If it all looks good, can you land this for me?

I have pushed the changes to try server: http://tbpl.mozilla.org/?tree=Try&rev=d1a28f038d0a

I fixed some really minor style glitch in head.js, added your name, and corrected an issue with window.arguments in places.js (was not ready to get an undefined argument). The modified test was really perfect and much smaller, clearly a win. I'll land asap.

Hurray!

Mozilla/5.0 (Windows NT 5.1; rv:9.0a1) Gecko/20110831 Firefox/9.0a1 Verified on Win XP, Win 7, Ubuntu 11.04 and Mac OS X 10.6 that in the Library window between "History" and "Tags" there is a new folder named "Downloads". I've noticed that if "about:" pages are saved the corresponding downloaded entry does not appear in Library -> Downloads folder. Is that intended?

about: pages are special, in the sense they are never added to history, so it is somehow expected.

Based on Comment 53 marking this as Verified Fixed. Thanks Marco!