Spinning out of bug 499985. nsNavHistoryFolderResultNode is the easiest place to get started.

WIP, which now looks very measly considering the vast quantities of mental energy around it. :( Need to quash FillStats's necessity, which can be triggered by a Refresh while the node is expanded. Need to do yet more joins on the annotation tables to sort by annotation. It might be better to tailor the SQL to different sorts if it's more performant, but ugh.

Getting close, all tests pass including a new one I added because we aren't testing sorting by item annotation at all. The only thing is: As I've mentioned before, folders get their access counts and times set by iterating over their children (FillStats). In this bug's case it's only an issue when folders are Refresh()ed (because otherwise folders are not expanded, and FillStats is basically a no-op.) I can do a sum and max in the SQL to replace FillStats I'm pretty sure, since FillStats effectively doesn't recurse into children in this case. But I realized that the issue of a folder's being Refresh()ed while both expanded and showing its access count or time *never occurs in Firefox*. Also, note that access counts and times are always zero or blank for folders in the library. Things that are causing me grief while working on this: * nsNavHistory.cpp makes assumptions about SQL created in nsNavBookmarks.cpp. * nsNavBookmarks.cpp makes assumptions about constants defined in nsNavHistory.cpp. * Some of the SQL that builds Places queries is in nsNavBookmarks, some in nsNavHistory. * Columns are addressed by index instead of name. Makes it difficult to address columns when they exist conditionally. I don't want to have to make new, static storage statements for each combination of columns that I need. Sorting in the SQL (by specific columns determined at run-time) exacerbates the problem. In general it makes the code more brittle. * I wish that SQL statements and all the k* constants that address their columns were more tightly bound, e.g., maybe together in a class.

Maybe I'm missing something, but it's weird how FillStats is used. It recurses into children, but a container doesn't have children until it's opened. The result, coupled with the fact that folder details aren't exposed to the user while the folder is open, is that access counts and times for folders are always zero in the UI. But even if a folder's details were shown while it's open, the access count and time wouldn't take into account children that themselves are folders, since their access counts and times are zero, since they start out closed and childless. If you want a some property of a container to aggregate properties of its descendants, then you either have to keep a running total along with the container in the DB, or you have to traverse the entire subtree rooted at the container. Otherwise you end up with misleading numbers that are more useless than no numbers at all.

Hey mak, is this bug still relevant what with the new bookmarks APIs we've been transitioning to?

The querying API and code didn't change, we are changing the backend API. The querying is still all synchronous and clunky. Ideally all of this area needs a rewrite, probably in js.