564900 - Add folder to Library sidebar specifically for downloads

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, enhancement)

Description

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Patch to add the enhancement

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 1

[Shawn Wilsher :sdwilsh]

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

Comment 2

[Marco Bonardo [:mak]]

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

Comment 3

[Marco Bonardo [:mak]]

+        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?

Comment 4

[Mehdi Mulani [:mmm] (I don't check this)]

(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 5

[Marco Bonardo [:mak]]

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.

Comment 6

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Adds a test case and fixes a bug with History not updating on a download.

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

Comment 7

[Marco Bonardo [:mak]]

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 ([e-mail])

This should be

  Mehdi Mulani <[e-mail]> (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

Comment 8

[Shawn Wilsher :sdwilsh]

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

Comment 9

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Cleans up code and fixes tiny bug as pointed out by :adw.

Comment 10

[Marco Bonardo [:mak]]

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

Comment 11

[Marco Bonardo [:mak]]

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

Comment 12

[Mehdi Mulani [:mmm] (I don't check this)]

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

Comment 13

[Marco Bonardo [:mak]]

(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 14

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Fixed test failing on bug 510634.

Comment 15

[Marco Bonardo [:mak]]

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

Comment 16

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Fixed as per mak's comments.

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.

Comment 17

[tymerkaev]

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

Tryserver builds?

Comment 18

[Mehdi Mulani [:mmm] (I don't check this)]

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

Comment 19

[Marco Bonardo [:mak]]

(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 20

[Marco Bonardo [:mak]]

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

Comment 21

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Added additional checks and changed getResult to result.

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

Comment 22

[Mehdi Mulani [:mmm] (I don't check this)]

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 23

[Shawn Wilsher :sdwilsh]

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

Comment 24

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/210e4d354aec

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=3978c4ee890b

Suspected of causing Bug 582661

Comment 26

[tymerkaev]

(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)

Comment 27

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Fixes icon not working with aero.

Azat: good call.

Fixed the jar.mn file but have still not figured out the orange tree issue.

Comment 28

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Appears to fix failing bug from mak's advice.

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 29

[Marco Bonardo [:mak]]

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.

Comment 30

[Paul [pwd]]

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?

Comment 31

[Paul [pwd]]

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.

Comment 32

[:Paolo Amadini]

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!

Comment 33

[Mehdi Mulani [:mmm] (I don't check this)]

Attachment: Moved changes to different patch.

Ran this patch through TryServer with greens all around :D

Comment 34

[Mehdi Mulani [:mmm] (I don't check this)]

(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

Comment 35

[Alex Limi (:limi) — Firefox UX Team]

Are you aiming to land this for FF4?

Comment 36

[Mehdi Mulani [:mmm] (I don't check this)]

(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 37

[Marco Bonardo [:mak]]

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 38

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

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

(bookkeeping)

Comment 39

[:Paolo Amadini]

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.

Comment 40

[:Paolo Amadini]

Attachment: Same patch, updated to apply on trunk

Comment 41

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 42

[Jared Wein [:jaws] (please needinfo? me)]

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 43

[Jared Wein [:jaws] (please needinfo? me)]

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 44

[Marco Bonardo [:mak]]

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.

Comment 45

[Marco Bonardo [:mak]]

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

Comment 46

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 564900

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?

Comment 47

[Jared Wein [:jaws] (please needinfo? me)]

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

Comment 48

[Marco Bonardo [:mak]]

Attachment: for checkin

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.

Comment 49

[Marco Bonardo [:mak]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/bb71b8c9441d

Comment 50

[Marco Bonardo [:mak]]

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

Comment 51

[Shawn Wilsher :sdwilsh]

Hurray!

Comment 52

[Simona Badau, Desktop QA]

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?

Comment 53

[Marco Bonardo [:mak]]

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

Comment 54

[Simona Badau, Desktop QA]

Based on Comment 53 marking this as Verified Fixed. 

Thanks Marco!