Add folder to Library sidebar specifically for downloads

VERIFIED FIXED in Firefox 8

Status

()

Firefox
Bookmarks & History
--
enhancement
VERIFIED FIXED
7 years ago
5 years ago

People

(Reporter: mmm, Assigned: mmm)

Tracking

Trunk
Firefox 8
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment, 11 obsolete attachments)

20.38 KB, patch
mak
: review+
Details | Diff | Splinter Review
Created attachment 444481 [details] [diff] [review]
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.
Status: NEW → ASSIGNED
Assignee: nobody → mmulani
Attachment #444481 - Flags: review?(mak77)
Depends on: 564270

Updated

7 years ago
Attachment #444481 - Attachment is patch: true
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).
Attachment #444481 - Flags: feedback+

Updated

7 years ago
Blocks: 564934
(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.
Attachment #444481 - Flags: review?(mak77) → review-
Created attachment 444967 [details] [diff] [review]
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.
Attachment #444481 - Attachment is obsolete: true
Attachment #444967 - Flags: feedback?(mak77)
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
Attachment #444967 - Flags: feedback?(mak77) → feedback+
(In reply to comment #7)
> nit: you could use Services.ww
And by that you mean Services.jsm
Created attachment 445199 [details] [diff] [review]
Cleans up code and fixes tiny bug as pointed out by :adw.
Attachment #444967 - Attachment is obsolete: true
Attachment #445199 - Flags: review?(mak77)
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
Attachment #445199 - Flags: review?(mak77) → review+

Updated

7 years ago
Flags: in-testsuite?
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".
Created attachment 447112 [details] [diff] [review]
Fixed test failing on bug 510634.
Attachment #445199 - Attachment is obsolete: true
Attachment #447112 - Flags: review?(mak77)
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
Attachment #447112 - Flags: review?(mak77) → review+
Created attachment 450371 [details] [diff] [review]
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.
Attachment #447112 - Attachment is obsolete: true

Comment 17

7 years ago
(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
Attachment #450371 - Flags: review?(mak77)
(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
Attachment #450371 - Flags: review?(mak77) → review+
Created attachment 451131 [details] [diff] [review]
Added additional checks and changed getResult to result.

Added a check for the All Bookmarks folder by comparing tree.selectedNode.itemId and PlacesUIUtils.allBookmarksFolderId.
Attachment #450371 - Attachment is obsolete: true
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
Attachment #451131 - Flags: approval2.0+

Updated

7 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/210e4d354aec
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b3
Version: unspecified → Trunk
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=3978c4ee890b

Suspected of causing Bug 582661
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 26

7 years ago
(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)
Created attachment 461359 [details] [diff] [review]
Fixes icon not working with aero.

Azat: good call.

Fixed the jar.mn file but have still not figured out the orange tree issue.
Attachment #451131 - Attachment is obsolete: true
Created attachment 463270 [details] [diff] [review]
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.
Attachment #461359 - Attachment is obsolete: true
Attachment #463270 - Flags: review?(mak77)
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.
Attachment #463270 - Flags: review?(mak77) → review-

Comment 30

7 years ago
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

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

Updated

7 years ago
Blocks: 591289

Comment 32

7 years ago
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!

Updated

7 years ago
Blocks: 582154
Created attachment 478115 [details] [diff] [review]
Moved changes to different patch.

Ran this patch through TryServer with greens all around :D
Attachment #463270 - Attachment is obsolete: true
Attachment #478115 - Flags: review?(mak77)
(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

Updated

7 years ago
Summary: Add folder specifically for downloads to Library sidebar → Add folder to Library sidebar specifically for downloads
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.
Attachment #478115 - Flags: review?(mak77) → review+
Comment on attachment 451131 [details] [diff] [review]
Added additional checks and changed getResult to result.

(bookkeeping)
Attachment #451131 - Flags: approval2.0+

Comment 39

6 years ago
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.

Updated

6 years ago
Blocks: 669905

Comment 40

6 years ago
Created attachment 546615 [details] [diff] [review]
Same patch, updated to apply on trunk
Attachment #478115 - Attachment is obsolete: true

Updated

6 years ago
No longer blocks: 591289
Depends on: 591289
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.
Attachment #546615 - Flags: review?(mak77)
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.
Attachment #546615 - Flags: review?(mak77) → review+

Updated

6 years ago
Blocks: 675902

Updated

6 years ago
No longer depends on: 591289
I have moved all additional interaction with downloads to bug 675902, so this can land as it is.
Target Milestone: Firefox 4.0b3 → ---
Created attachment 550832 [details] [diff] [review]
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?
Attachment #546615 - Attachment is obsolete: true
Attachment #550832 - Flags: review?(mak77)
I have pushed the changes to try server:
http://tbpl.mozilla.org/?tree=Try&rev=d1a28f038d0a
Blocks: 676713
Created attachment 550905 [details] [diff] [review]
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.
Attachment #550832 - Attachment is obsolete: true
Attachment #550905 - Flags: review+
Attachment #550832 - Flags: review?(mak77)
http://hg.mozilla.org/integration/mozilla-inbound/rev/bb71b8c9441d
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/bb71b8c9441d
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 8
Hurray!

Updated

6 years ago
Depends on: 682678

Updated

6 years ago
Blocks: 402231
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!
Status: RESOLVED → VERIFIED

Updated

6 years ago
Blocks: 708595
Blocks: 725543
You need to log in before you can comment on or make changes to this bug.