422163 - Split History container in the Library (like sidebar BY DATE)

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, enhancement)

Description

[Matthew Turnbull [Bluefang]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b5pre) Gecko/2008031004 Minefield/3.0b5pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b5pre) Gecko/2008031004 Minefield/3.0b5pre

The Places Library, as it stands today, lacks some critical features which are provided by the history sidebar and it is not as streamlined as the Pre-Places bookmarks organizer.

First, instead of providing the History, Tags, and Bookmarks in a tree, they should be provided as a series of views (much like the History Sidebar dose).

+------------------------+
| View Menu              |
+------------------------+
| Bookmarks              |
| Tags                   |
| History                |
| Search                 |
+------------------------+
| [] Split-view          |
+------------------------+
| Show Columns         > |
| Sort                 > |
+------------------------+

(Or, I suppose the Bookmarks/Tags/History/Search group could be added to the Organize menu... Either could work for me)

In addition, a 'Sort by Date and Site' sorting option should be added to provide missing functionality from the History Sidebar. This is a very useful sorting mode and it should not be missing from the main Organizer.

When searching, the search bar should automatically limit it's search scope to the type of content that is being viewed.
I.E. when looking at bookmarks, the search bar should search only bookmarks, and when viewing history it would search only history, etc...

Finally, and I know this is a contention among many users, there should be an option to switch between a single-view and a split-view.
-The single-view would be much like the Pre-Places bookmarks organize was. The Bookmark folders (and history sites) would be presented in a hierarchical tree with their contents listed below them
-The split-view would be what currently exists, with the folder/site hierarchy viewable in the left panel and the folder contents viewed in the right panel. 


(Sorry in advance if this should habe been broken up in to multiple bugs or added to a meta/tracker bug. This is my first bug so I'm not quite sure on the etiquette)

Reproducible: Always

Steps to Reproduce:
1. Open Places Library
2.
3.
Actual Results:  
History, Tags, and Bookmarks are presented in tree view with no option for advanced history views

History->Show All History opens Library with the History folder selected
Bookmarks->Organize Bookmarks opens Library with the All Bookmarks folder selected

Searching automatically searches all Places entries

Expected Results:  
History->Show All History should open Library in the History view
Bookmarks->Organize Bookmarks should open Library in the Bookmarks view

View/Organize menu should provide options to switch between the Tags, Bookmarks, Advanced History, and Search views

Search should be context sensitive and limit search to content of current view (History/Tags/Bookmarks)

Comment 1

[Jim Jeffery not reading bug-mail 1/2/11]

Possibly dupes of these: 
https://bugzilla.mozilla.org/show_bug.cgi?id=405887
https://bugzilla.mozilla.org/show_bug.cgi?id=407416
https://bugzilla.mozilla.org/show_bug.cgi?id=378798

Proposed mock-up 
https://bugzilla.mozilla.org/show_bug.cgi?id=393511 

Comment 2

[Matthew Turnbull [Bluefang]]

This isn't a dup of any of those. Those all seem to do strictly with search functionality. This has to do with the presentation of the bookmarks/tags/history. Completely different.

Yes, I do mention some searching capabilities in this bug, but there is a lot more than just that.

Comment 3

[Marco Bonardo [:mak]]

We should at least split the default History folder in days folder like in the sidebar with a RESULTS_AS_DATE_QUERY

notice that large treeviews are too slow and you will end up not being able to scroll into it. 

So definately since we will have to rebuild the left pane before final, we should replace the query with a splitted one.

confirming and asking blocking to split the default history into days (like sidebar BY DATE)

Comment 4

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

I think it's pretty valuable to have an All History folder, so I think/hope Marco's asking for:

 v History
    Today
    Yesterday 
    2 days ago
    3 days ago
    4 days ago 
    Older than 6 days

(aside: what, uh, happened to 5 days?)

TBH, I don't think that's as useful as the ability to search through history and add the Visit Date column, though, so I don't think this blocks.

Comment 5

[Marco Bonardo [:mak]]

Mike the problem is that if history grows the treeview becomes unusable... splitting will mitigate this

hwv yes i'm asking for this:
https://bugzilla.mozilla.org/attachment.cgi?id=310897

history contains day containers like we have in the sidebar when you choose BY DATE

 v History
    Today
    Yesterday 
    2 days ago
    3 days ago
    4 days ago 
    Older than 6 days

this is ok apart from this splitting is really bad since having "older than 6 days" on a 90 days history is a pain.

We can do this splitting at almost no cost after fixing tags containers in bug 419731

Comment 6

[Marco Bonardo [:mak]]

(In reply to comment #5)
> this is ok apart from this splitting is really bad since having "older than 6
> days" on a 90 days history is a pain.

hwv this can be addressed later quite easily, so the main purpose is splitting

splitting will also allow to set an history container icon easily since we can recognize the query by its resulttype

Comment 7

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Comment 8

[Marco Bonardo [:mak]]

trybuilds including this and bug 390614:
https://build.mozilla.org/tryserver-builds/2009-02-27_17:[e-mail]-try-5df62683408/

Comment 9

[Marco Bonardo [:mak]]

Attachment: patch v1.1

fix wrong comment in history idl.

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 367054 [details] [diff] [review]
patch v1.1

asking ui-r for this, it's practically using the same splitting we have in sidebar for the Library History item. 

Notice actually in many cases clicking History in Library will hang for seconds (large histories), due to splitting clicking history will instead show splitted containers, that's quite faster.

trybuilds: https://build.mozilla.org/tryserver-builds/2009-03-12_09:[e-mail]-try-69d31316094/

this contains old version of split history patch and this patch, the only difference is that delete won't work here (works with the new patch) and month names localization could be messed up (works with new patch), if you need a new trybuild i'll create one.

Comment 11

[Marco Bonardo [:mak]]

Attachment: screenshot

Comment 12

[Marco Bonardo [:mak]]

notice i should check why container node title does not appeat in the details pane

Comment 13

[Dietrich Ayala (:dietrich)]

Comment on attachment 367054 [details] [diff] [review]
patch v1.1

r=me

is this already covered by tests? if not, please add a new test for left folder pane migration.

Comment 14

[Marco Bonardo [:mak]]

Attachment: patch v1.2

added a test, fixed another browser chrome test that was relying on Library's History containing history nodes, fixed Library details pane forcing a name for query containers without an itemId.

Comment 15

[Dietrich Ayala (:dietrich)]

Comment on attachment 370029 [details] [diff] [review]
patch v1.2

r=me thanks!

Comment 16

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/38ed1339d3a8

Comment 17

[Marco Bonardo [:mak]]

backed out to try solving a crash in browser chrome tests
http://hg.mozilla.org/mozilla-central/rev/ad090ca26c6b

Comment 18

[Marco Bonardo [:mak]]

Attachment: patch v1.3

this has migration test.
Unluckily with this patch tinderboxes are crashing during browser-chrome test runs, and i'm completely unable to crash locally.
I will probably need help, can anyone reproduce the crash or give me a crash stack?

Comment 19

[Marco Bonardo [:mak]]

here is the crashstack:

Thread 0 (crashed)
 0  libxul.so!nsNavHistory::FilterResultSet(nsNavHistoryQueryResultNode*, nsCOMArray<nsNavHistoryResultNode> const&, nsCOMArray<nsNavHistoryResultNode>*, nsCOMArray<nsNavHistoryQuery> const&, nsNavHistoryQueryOptions*) [nsNavHistoryResult.h:b39c0c0674fb : 335 + 0x2]
    eip = 0x4094f6e6   esp = 0xbf88492c   ebp = 0xbf884bfc   ebx = 0x40deea10
    esi = 0xbf884c88   edi = 0x45273690   eax = 0x00000000   ecx = 0x00000000
    edx = 0xbf884bbc   efl = 0x00010297
 1  libxul.so!nsNavHistory::EvaluateQueryForNode(nsCOMArray<nsNavHistoryQuery> const&, nsNavHistoryQueryOptions*, nsNavHistoryResultNode*) [nsNavHistory.cpp:b39c0c0674fb : 2345 + 0x1d]
    eip = 0x40950e66   esp = 0xbf884c04   ebp = 0xbf884d4c
 2  libxul.so!nsNavHistoryQueryResultNode::OnVisit(nsIURI*, long long, long long, long long, long long, unsigned int, unsigned int*) [nsNavHistoryResult.cpp:b39c0c0674fb : 2685 + 0x19]
    eip = 0x4096881c   esp = 0xbf884d54   ebp = 0xbf884e0c
 3  libxul.so!nsNavHistoryResult::OnVisit(nsIURI*, long long, long long, long long, long long, unsigned int, unsigned int*) [nsNavHistoryResult.cpp:b39c0c0674fb : 4306 + 0x36]
    eip = 0x40967b7c   esp = 0xbf884e14   ebp = 0xbf884e7c
 4  libxul.so!nsNavHistory::AddVisit(nsIURI*, long long, nsIURI*, int, int, long long, long long*) [nsNavHistory.cpp:b39c0c0674fb : 2783 + 0x3c]
    eip = 0x40952441   esp = 0xbf884e84   ebp = 0xbf884f4c
 5  libxul.so!NS_GetXPTCallStub_P + 0x30
    eip = 0x40a92217   esp = 0xbf884f54   ebp = 0xbf884f9c
 6  libxul.so!XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) [xpcwrappednative.cpp:b39c0c0674fb : 2480 + 0x20]
    eip = 0x401ee560   esp = 0xbf884fa4   ebp = 0xbf8851ec
 7  libxul.so!XPC_WN_CallMethod(JSContext*, JSObject*, unsigned int, int*, int*) [xpcwrappednativejsops.cpp:b39c0c0674fb : 1585 + 0x9]
    eip = 0x401f49ed   esp = 0xbf8851f4   ebp = 0xbf8852bc
 8  libmozjs.so!js_Invoke [jsinterp.cpp:b39c0c0674fb : 1368 + 0x10]
    eip = 0x40e68726   esp = 0xbf8852c4   ebp = 0xbf88538c
 9  libmozjs.so!js_Interpret [jsinterp.cpp:b39c0c0674fb : 5046 + 0x20]
    eip = 0x40e5bdb4   esp = 0xbf885394   ebp = 0xbf8855fc
10  libmozjs.so!js_Invoke [jsinterp.cpp:b39c0c0674fb : 1376 + 0xa]
    eip = 0x40e68739   esp = 0xbf885604   ebp = 0xbf8856bc
11  libxul.so!nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) [xpcwrappedjsclass.cpp:b39c0c0674fb : 1608 + 0x1a]
    eip = 0x401ebf5d   esp = 0xbf8856c4   ebp = 0xbf88589c
12  libxul.so!nsXPCWrappedJS::CallMethod(unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) [xpcwrappedjs.cpp:b39c0c0674fb : 561 + 0x13]
    eip = 0x401e724d   esp = 0xbf8858a4   ebp = 0xbf8858cc
13  libxul.so!PrepareAndDispatch [xptcstubs_gcc_x86_unix.cpp:b39c0c0674fb : 95 + 0x1d]
    eip = 0x40a92d2d   esp = 0xbf8858d4   ebp = 0xbf88599c
14  libxul.so!nsNavBookmarks::RunInBatchMode(nsINavHistoryBatchCallback*, nsISupports*) [nsNavBookmarks.cpp:b39c0c0674fb : 2975 + 0xa]
    eip = 0x4096cbd7   esp = 0xbf8859a4   ebp = 0xbf8859bc
15  libxul.so!NS_GetXPTCallStub_P + 0x30
    eip = 0x40a92217   esp = 0xbf8859c4   ebp = 0xbf8859e8
16  libxul.so!XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) [xpcwrappednative.cpp:b39c0c0674fb : 2480 + 0x20]
    eip = 0x401ee560   esp = 0xbf8859f0   ebp = 0xbf885c38
17  libxul.so!XPC_WN_CallMethod(JSContext*, JSObject*, unsigned int, int*, int*) [xpcwrappednativejsops.cpp:b39c0c0674fb : 1585 + 0x9]
    eip = 0x401f49ed   esp = 0xbf885c40   ebp = 0xbf885d08
18  libmozjs.so!js_Invoke [jsinterp.cpp:b39c0c0674fb : 1368 + 0x10]
    eip = 0x40e68726   esp = 0xbf885d10   ebp = 0xbf885dd8
19  libmozjs.so!js_Interpret [jsinterp.cpp:b39c0c0674fb : 5046 + 0x20]
    eip = 0x40e5bdb4   esp = 0xbf885de0   ebp = 0xbf886048
20  libmozjs.so!js_Invoke [jsinterp.cpp:b39c0c0674fb : 1376 + 0xa]
    eip = 0x40e68739   esp = 0xbf886050   ebp = 0xbf886108
21  libxul.so!nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) [xpcwrappedjsclass.cpp:b39c0c0674fb : 1608 + 0x1a]
    eip = 0x401ebf5d   esp = 0xbf886110   ebp = 0xbf8862e8
22  libxul.so!nsXPCWrappedJS::CallMethod(unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) [xpcwrappedjs.cpp:b39c0c0674fb : 561 + 0x13]
    eip = 0x401e724d   esp = 0xbf8862f0   ebp = 0xbf886318
23  libxul.so!PrepareAndDispatch [xptcstubs_gcc_x86_unix.cpp:b39c0c0674fb : 95 + 0x1d]
    eip = 0x40a92d2d   esp = 0xbf886320   ebp = 0xbf8863e8
24  libxul.so!nsThread::ProcessNextEvent(int, int*) [nsThread.cpp:b39c0c0674fb : 510 + 0x5]
    eip = 0x40a84e3d   esp = 0xbf8863f0   ebp = 0xbf886428
25  libxul.so!NS_ProcessNextEvent_P(nsIThread*, int) [nsThreadUtils.cpp : 230 + 0xd]
    eip = 0x40a52867   esp = 0xbf886430   ebp = 0xbf886458
26  libxul.so!nsBaseAppShell::Run() [nsBaseAppShell.cpp:b39c0c0674fb : 170 + 0x9]
    eip = 0x409abd62   esp = 0xbf886460   ebp = 0xbf886478
27  libxul.so!nsAppStartup::Run() [nsAppStartup.cpp:b39c0c0674fb : 192 + 0x5]
    eip = 0x408724e2   esp = 0xbf886480   ebp = 0xbf886498
28  libxul.so!XRE_main [nsAppRunner.cpp:b39c0c0674fb : 3232 + 0x7]
    eip = 0x401b8a27   esp = 0xbf8864a0   ebp = 0xbf886ab8
29  firefox-bin!main [nsBrowserApp.cpp:b39c0c0674fb : 156 + 0xe]
    eip = 0x080495b1   esp = 0xbf886ac0   ebp = 0xbf886b18
30  libc-2.5.so + 0x15deb
    eip = 0x00aa6dec   esp = 0xbf886b20   ebp = 0xbf886b88

Comment 20

[Marco Bonardo [:mak]]

i've been able to crash a couple times with the sidebar set to "by date & site" and a date and site containers open and visiting a subpage of the open site container. the item was added at the root level instead that in the site container, btw this is something i had already rarely seen in the past, could be the change is making an old crash bug more visible.

Comment 21

[Marco Bonardo [:mak]]

Attachment: patch v1.4

patch currently on tryservers, still waiting for results...

Comment 22

[Marco Bonardo [:mak]]

Comment on attachment 370663 [details] [diff] [review]
patch v1.4

this was green on all tryservers, the crash was basically a null pointer in EvaluateQueryForNode, indeed VisitIdToResultNode is not ensured to return always a valid node, for special resultTypes it will return early.

Comment 23

[Dietrich Ayala (:dietrich)]

Comment on attachment 370663 [details] [diff] [review]
patch v1.4

>diff --git a/browser/components/places/content/places.js b/browser/components/places/content/places.js
>--- a/browser/components/places/content/places.js
>+++ b/browser/components/places/content/places.js
>@@ -682,16 +682,26 @@ var PlacesOrganizer = {
>         itemId = concreteId;
>       else if (aSelectedNode.itemId != -1)
>         itemId = aSelectedNode.itemId;
>       else
>         itemId = PlacesUtils._uri(aSelectedNode.uri);
> 
>       gEditItemOverlay.initPanel(itemId, { hiddenRows: ["folderPicker"],
>                                            forceReadOnly: readOnly });
>+
>+      // Dynamically generated queries, like history date containers, have
>+      // itemId !=0 and do not exist in history.  For them the panel is
>+      // read-only, but empty, since it can't get a valid title for the object.
>+      // In such a case we force the title using the selectedNode one, for UI
>+      // polishness.
>+      if (aSelectedNode.itemId == -1 &&
>+          (PlacesUtils.nodeIsDay(aSelectedNode) ||
>+           PlacesUtils.nodeIsHost(aSelectedNode)))
>+        gEditItemOverlay._element("namePicker").value = aSelectedNode.title;
> 
>       this._detectAndSetDetailsPaneMinimalState(aSelectedNode);
>     }
>     else if (!aSelectedNode && aNodeList[0]) {
>       var itemIds = [];
>       for (var i = 0; i < aNodeList.length; i++) {
>         if (!PlacesUtils.nodeIsBookmark(aNodeList[i]) &&
>             !PlacesUtils.nodeIsURI(aNodeList[i])) {
>diff --git a/browser/components/places/content/utils.js b/browser/components/places/content/utils.js
>--- a/browser/components/places/content/utils.js
>+++ b/browser/components/places/content/utils.js
>@@ -57,17 +57,17 @@ __defineGetter__("PlacesUtils", function
> 
> const LOAD_IN_SIDEBAR_ANNO = "bookmarkProperties/loadInSidebar";
> const DESCRIPTION_ANNO = "bookmarkProperties/description";
> const GUID_ANNO = "placesInternal/GUID";
> const LMANNO_FEEDURI = "livemark/feedURI";
> const LMANNO_SITEURI = "livemark/siteURI";
> const ORGANIZER_FOLDER_ANNO = "PlacesOrganizer/OrganizerFolder";
> const ORGANIZER_QUERY_ANNO = "PlacesOrganizer/OrganizerQuery";
>-const ORGANIZER_LEFTPANE_VERSION = 5;
>+const ORGANIZER_LEFTPANE_VERSION = 6;
> const EXCLUDE_FROM_BACKUP_ANNO = "places/excludeFromBackup";
> 
> #ifdef XP_MACOSX
> // On Mac OSX, the transferable system converts "\r\n" to "\n\n", where we
> // really just want "\n".
> const NEWLINE= "\n";
> #else
> // On other platforms, the transferable system converts "\r\n" to "\n".
>@@ -1190,17 +1190,19 @@ var PlacesUIUtils = {
>         self.leftPaneQueries = { };
> 
>         // Left Pane Root Folder
>         leftPaneRoot = PlacesUtils.bookmarks.createFolder(PlacesUtils.placesRootId, "", -1);
>         // ensure immediate children can't be removed
>         PlacesUtils.bookmarks.setFolderReadonly(leftPaneRoot, true);
> 
>         // History Query
>-        let uri = PlacesUtils._uri("place:sort=4&");
>+        let uri = PlacesUtils._uri("place:type=" +
>+                                   Ci.nsINavHistoryQueryOptions.RESULTS_AS_DATE_QUERY +
>+                                   "&sort=4");
>         let title = self.getString("OrganizerQueryHistory");
>         let itemId = PlacesUtils.bookmarks.insertBookmark(leftPaneRoot, uri, -1, title);
>         PlacesUtils.annotations.setItemAnnotation(itemId, ORGANIZER_QUERY_ANNO,
>                                                   "History", 0, EXPIRE_NEVER);
>         PlacesUtils.annotations.setItemAnnotation(itemId,
>                                                   EXCLUDE_FROM_BACKUP_ANNO,
>                                                   1, 0, EXPIRE_NEVER);
>         self.leftPaneQueries["History"] = itemId;
>diff --git a/browser/components/places/tests/browser/Makefile.in b/browser/components/places/tests/browser/Makefile.in
>--- a/browser/components/places/tests/browser/Makefile.in
>+++ b/browser/components/places/tests/browser/Makefile.in
>@@ -39,16 +39,17 @@ srcdir		= @srcdir@
> srcdir		= @srcdir@
> VPATH			= @srcdir@
> relativesrcdir  = browser/components/places/tests/browser
> 
> include $(DEPTH)/config/autoconf.mk
> include $(topsrcdir)/config/rules.mk
> 
> _BROWSER_TEST_FILES = \
>+	browser_0_library_left_pane_migration.js \
> 	browser_425884.js \
> 	browser_423515.js \
> 	browser_410196_paste_into_tags.js \
> 	browser_457473_no_copy_guid.js \
> 	browser_sort_in_library.js \
> 	browser_library_open_leak.js \
> 	browser_library_panel_leak.js \
> 	browser_library_search.js \
>diff --git a/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js b/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js
>new file mode 100644
>--- /dev/null
>+++ b/browser/components/places/tests/browser/browser_0_library_left_pane_migration.js
>@@ -0,0 +1,144 @@
>+/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim:set ts=2 sw=2 sts=2 et: */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Places test code.
>+ *
>+ * The Initial Developer of the Original Code is Mozilla Corp.
>+ * Portions created by the Initial Developer are Copyright (C) 2009
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *  Marco Bonardo <[e-mail]> (Original Author)
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by devaring the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not devare
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+/**
>+ *  Test we correctly migrate Library left pane to the latest version.
>+ *  Note: this test MUST be the first between browser chrome tests, or results
>+ *        of next tests could be unexpected due to PlacesUIUtils getters.
>+ */
>+
>+const TEST_URI = "http://www.mozilla.org/";
>+
>+var ww = Cc["@mozilla.org/embedcomp/window-watcher;1"].
>+         getService(Ci.nsIWindowWatcher);
>+
>+var windowObserver = {
>+  observe: function(aSubject, aTopic, aData) {
>+    if (aTopic === "domwindowopened") {
>+      ww.unregisterNotification(this);
>+      var organizer = aSubject.QueryInterface(Ci.nsIDOMWindow);
>+      organizer.addEventListener("load", function onLoad(event) {
>+        organizer.removeEventListener("load", onLoad, false);
>+        executeSoon(function () {
>+          // Check left pane.
>+          ok(PlacesUIUtils.leftPaneFolderId > 0,
>+             "Left pane folder correctly created");
>+          var leftPaneItems =
>+            PlacesUtils.annotations
>+                       .getItemsWithAnnotation(ORGANIZER_FOLDER_ANNO, {});
>+          is(leftPaneItems.length, 1,
>+             "We correctly have only 1 left pane folder");
>+          var leftPaneRoot = leftPaneItems[0];
>+          is(leftPaneRoot, PlacesUIUtils.leftPaneFolderId,
>+             "leftPaneFolderId getter has correct value");
>+          // Check version has been upgraded.
>+          var version =
>+            PlacesUtils.annotations.getItemAnnotation(leftPaneRoot,
>+                                                      ORGANIZER_FOLDER_ANNO);
>+          is(version, ORGANIZER_LEFTPANE_VERSION,
>+             "Left pane version has been correctly upgraded");
>+
>+          // Check left pane is populated.
>+          organizer.PlacesOrganizer.selectLeftPaneQuery('History');
>+          is(organizer.PlacesOrganizer._places.selectedNode.itemId,
>+             PlacesUIUtils.leftPaneQueries["History"],
>+             "Library left pane is populated and working");
>+
>+          // Close Library window.
>+          organizer.close();
>+          // No need to cleanup anything, we have a correct left pane now.
>+          finish();
>+        });
>+      }, false);
>+    }
>+  }
>+};
>+
>+function test() {
>+  waitForExplicitFinish();
>+  // Sanity checks.
>+  ok(PlacesUtils, "PlacesUtils is running in chrome context");
>+  ok(PlacesUIUtils, "PlacesUIUtils is running in chrome context");
>+  ok(ORGANIZER_LEFTPANE_VERSION > 0,
>+     "Left pane version in chrome context, current version is: " + ORGANIZER_LEFTPANE_VERSION );
>+
>+  // Check if we have any left pane folder already set, remove it eventually.
>+  var leftPaneItems = PlacesUtils.annotations
>+                                 .getItemsWithAnnotation(ORGANIZER_FOLDER_ANNO, {});
>+  if (leftPaneItems.length > 0) {
>+    // The left pane has already been created, touching it now would cause
>+    // next tests to rely on wrong values (and possibly crash)
>+    is(leftPaneItems.length, 1, "We correctly have only 1 left pane folder");
>+    // Check version.
>+    var version = PlacesUtils.annotations.getItemAnnotation(leftPaneItems[0],
>+                                                            ORGANIZER_FOLDER_ANNO);
>+    is(version, ORGANIZER_LEFTPANE_VERSION, "Left pane version is actual");
>+    ok(true, "left pane has already been created, skipping test");
>+    finish();
>+    return;
>+  }
>+
>+  // Create a fake left pane folder with an old version (current version - 1).
>+  var fakeLeftPaneRoot =
>+    PlacesUtils.bookmarks.createFolder(PlacesUtils.placesRootId, "",
>+                                       PlacesUtils.bookmarks.DEFAULT_INDEX);
>+  PlacesUtils.annotations.setItemAnnotation(fakeLeftPaneRoot,
>+                                            ORGANIZER_FOLDER_ANNO,
>+                                            ORGANIZER_LEFTPANE_VERSION - 1,
>+                                            0,
>+                                            PlacesUtils.annotations.EXPIRE_NEVER);
>+
>+  // Check fake left pane root has been correctly created.
>+  var leftPaneItems =
>+    PlacesUtils.annotations.getItemsWithAnnotation(ORGANIZER_FOLDER_ANNO, {});
>+  is(leftPaneItems.length, 1, "We correctly have only 1 left pane folder");
>+  is(leftPaneItems[0], fakeLeftPaneRoot, "left pane root itemId is correct");
>+
>+  // Check version.
>+  var version = PlacesUtils.annotations.getItemAnnotation(fakeLeftPaneRoot,
>+                                                          ORGANIZER_FOLDER_ANNO);
>+  is(version, ORGANIZER_LEFTPANE_VERSION - 1, "Left pane version correctly set");
>+
>+  // Open Library, this will upgrade our left pane version.
>+  ww.registerNotification(windowObserver);
>+  ww.openWindow(null,
>+                "chrome://browser/content/places/places.xul",
>+                "",
>+                "chrome,toolbar=yes,dialog=no,resizable",
>+                null);
>+}
>diff --git a/browser/components/places/tests/browser/browser_410196_paste_into_tags.js b/browser/components/places/tests/browser/browser_410196_paste_into_tags.js
>--- a/browser/components/places/tests/browser/browser_410196_paste_into_tags.js
>+++ b/browser/components/places/tests/browser/browser_410196_paste_into_tags.js
>@@ -122,16 +122,19 @@ function test() {
>           }
>         },
> 
>         histNode: null,
> 
>         copyHistNode: function (){
>           // focus the history object
>           PO.selectLeftPaneQuery("History");
>+          var histContainer = PO._places.selectedNode.QueryInterface(Ci.nsINavHistoryContainerResultNode);
>+          histContainer.containerOpen = true;
>+          PO._places.selectNode(histContainer.getChild(0));
>           this.histNode = PO._content.view.nodeForTreeIndex(0);
>           PO._content.selectNode(this.histNode);
>           is(this.histNode.uri, MOZURISPEC,
>              "historyNode exists: " + this.histNode.uri);
>           // copy the history node
>           PO._content.controller.copy();
>         },
> 
>@@ -154,16 +157,19 @@ function test() {
>         pasteToTag: function (){
>           // paste history node into tag
>           this.focusTag(true);
>         },
> 
>         historyNode: function (){
>           // re-focus the history again
>           PO.selectLeftPaneQuery("History");
>+          var histContainer = PO._places.selectedNode.QueryInterface(Ci.nsINavHistoryContainerResultNode);
>+          histContainer.containerOpen = true;
>+          PO._places.selectNode(histContainer.getChild(0));
>           var histNode = PO._content.view.nodeForTreeIndex(0);
>           ok(histNode, "histNode exists: " + histNode.title);
>           // check to see if the history node is tagged!
>           var tags = PU.tagging.getTagsForURI(PU._uri(MOZURISPEC), {});
>           ok(tags.length == 1, "history node is tagged: " + tags.length);
>           // check if a bookmark was created
>           var isBookmarked = PU.bookmarks.isBookmarked(PU._uri(MOZURISPEC));
>           is(isBookmarked, true, MOZURISPEC + " is bookmarked");
>diff --git a/toolkit/components/places/public/nsINavHistoryService.idl b/toolkit/components/places/public/nsINavHistoryService.idl
>--- a/toolkit/components/places/public/nsINavHistoryService.idl
>+++ b/toolkit/components/places/public/nsINavHistoryService.idl
>@@ -973,17 +973,17 @@ interface nsINavHistoryQueryOptions : ns
>    * represented.
>    */
>   attribute unsigned short resultType;
> 
>   /**
>    * This option excludes all URIs and separators from a bookmarks query.
>    * This would be used if you just wanted a list of bookmark folders and
>    * queries (such as the left pane of the places page).
>-   * Ignored for queries over history. Defaults to false.
>+   * Defaults to false.
>    */
>   attribute boolean excludeItems;
> 
>   /**
>    * Set to true to exclude queries ("place:" URIs) from the query results.
>    * Simple folder queries (bookmark folder symlinks) will still be included.
>    * Defaults to false.
>    */
>diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp
>--- a/toolkit/components/places/src/nsNavHistory.cpp
>+++ b/toolkit/components/places/src/nsNavHistory.cpp
>@@ -2969,20 +2969,27 @@ PRBool IsHistoryMenuQuery(const nsCOMArr
> 
>   return PR_TRUE;
> }
> 
> static
> PRBool NeedToFilterResultSet(const nsCOMArray<nsNavHistoryQuery>& aQueries, 
>                              nsNavHistoryQueryOptions *aOptions)
> {
>+  // Never filter queries returning queries
>+  PRUint16 resultType = aOptions->ResultType();
>+  if (resultType == nsINavHistoryQueryOptions::RESULTS_AS_DATE_QUERY ||
>+      resultType == nsINavHistoryQueryOptions::RESULTS_AS_DATE_SITE_QUERY ||
>+      resultType == nsINavHistoryQueryOptions::RESULTS_AS_TAG_QUERY ||
>+      resultType == nsINavHistoryQueryOptions::RESULTS_AS_SITE_QUERY)
>+    return PR_FALSE;
>+
>   // Always filter bookmarks queries to avoid the inclusion of query nodes,
>   // but RESULTS AS TAG QUERY never needs to be filtered.
>-  if (aOptions->QueryType() == nsINavHistoryQueryOptions::QUERY_TYPE_BOOKMARKS &&
>-      aOptions->ResultType() != nsINavHistoryQueryOptions::RESULTS_AS_TAG_QUERY)
>+  if (aOptions->QueryType() == nsINavHistoryQueryOptions::QUERY_TYPE_BOOKMARKS)
>     return PR_TRUE;
> 
>   nsCString parentAnnotationToExclude;
>   nsresult rv = aOptions->GetExcludeItemIfParentHasAnnotation(parentAnnotationToExclude);
>   NS_ENSURE_SUCCESS(rv, PR_TRUE);
>   if (!parentAnnotationToExclude.IsEmpty())
>     return PR_TRUE;
> 
>@@ -6194,17 +6201,17 @@ nsNavHistory::FilterResultSet(nsNavHisto
>   for (PRInt32 nodeIndex = 0; nodeIndex < aSet.Count(); nodeIndex ++) {
>     // exclude-queries is implicit when searching, we're only looking at
>     // plan URI nodes
>     if (!aSet[nodeIndex]->IsURI())
>       continue;
> 
>     PRInt64 parentId = -1;
>     if (aSet[nodeIndex]->mItemId != -1) {
>-      if (aQueryNode->mItemId == aSet[nodeIndex]->mItemId)
>+      if (aQueryNode && aQueryNode->mItemId == aSet[nodeIndex]->mItemId)
>         continue;
>       rv = bookmarks->GetFolderIdForItem(aSet[nodeIndex]->mItemId, &parentId);
>       NS_ENSURE_SUCCESS(rv, rv);
>     }
> 
>     // if we are excluding items by parent annotation, 
>     // exclude items who's parent is a folder with that annotation
>     if (!parentAnnotationToExclude.IsEmpty() && parentFoldersToExclude.Contains(parentId))
>diff --git a/toolkit/components/places/src/nsNavHistoryResult.cpp b/toolkit/components/places/src/nsNavHistoryResult.cpp
>--- a/toolkit/components/places/src/nsNavHistoryResult.cpp
>+++ b/toolkit/components/places/src/nsNavHistoryResult.cpp
>@@ -194,17 +194,17 @@ nsNavHistoryResultNode::GetTags(nsAStrin
>   // Fetch the tags
>   nsNavHistory* history = nsNavHistory::GetHistoryService();
>   NS_ENSURE_STATE(history);
>   mozIStorageStatement* getTagsStatement = history->DBGetTags();
> 
>   mozStorageStatementScoper scoper(getTagsStatement);
>   nsresult rv = getTagsStatement->BindStringParameter(0, NS_LITERAL_STRING(", "));
>   NS_ENSURE_SUCCESS(rv, rv);
>-  rv = getTagsStatement->BindInt32Parameter(1, history->GetTagsFolder());
>+  rv = getTagsStatement->BindInt64Parameter(1, history->GetTagsFolder());
>   NS_ENSURE_SUCCESS(rv, rv);
>   rv = getTagsStatement->BindUTF8StringParameter(2, mURI);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   PRBool hasTags = PR_FALSE;
>   if (NS_SUCCEEDED(getTagsStatement->ExecuteStep(&hasTags)) && hasTags) {
>     rv = getTagsStatement->GetString(0, mTags);
>     NS_ENSURE_SUCCESS(rv, rv);
>@@ -2393,19 +2393,24 @@ nsNavHistoryQueryResultNode::FillChildre
>     while (mChildren.Count() > mOptions->MaxResults())
>       mChildren.RemoveObjectAt(mChildren.Count() - 1);
>   }
> 
>   nsNavHistoryResult* result = GetResult();
>   NS_ENSURE_TRUE(result, NS_ERROR_FAILURE);
> 
>   if (mOptions->QueryType() == nsINavHistoryQueryOptions::QUERY_TYPE_HISTORY ||
>-      mOptions->QueryType() == nsINavHistoryQueryOptions::QUERY_TYPE_UNIFIED) {
>-    // register with the result for history updates
>-    result->AddHistoryObserver(this);
>+       mOptions->QueryType() == nsINavHistoryQueryOptions::QUERY_TYPE_UNIFIED) {

nit: unindent one space

r=me otherwise

Comment 24

[Marco Bonardo [:mak]]

Attachment: patch v1.5

Comment 25

[Marco Bonardo [:mak]]

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

Comment 26

[Marco Bonardo [:mak]]

Attachment: patch for 1.9.1

merged patch with this patch and known regressions. All known regressions have been fixed and have unit tests.
TO BE LANDED AFTER BUG 485703, or some test could fail (we expect some var in the tests that is introduced there), i would expect that bug getting approval first, in case it's not, patches will need unbitrot.
 
In this merge i removed changes to test for bug 410196, since that has not yet landed on branch, in case it lands before this, changes should be added back.

Asking approval 1.9.1 for this change, reasons:
- increases Library usability and perf (actually opening History in Library is a pain)
- contains 2 null pointer fixes (crashes) and one possible endianness fix
- cleans up some bug in our observers paths
Risk is medium, but all changes and regressions have tests coverage.

Comment 27

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

Comment on attachment 371663 [details] [diff] [review]
patch for 1.9.1

a191=beltzner

Comment 28

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/969eb6567510

Comment 29

[Tracy Walker [:tracy]]

verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1b4pre) Gecko/20090414 Shiretoko/3.5b4pre

Comment 30

[Gervase Markham [:gerv]]

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