The current scheme for expressing types in the moz_bookmarks table is: item_child is null, then item is folder folder_child is null, then item is bookmark both are null, then item is separator Instead, we should have a |type| column, and a column for holding the foreign key for that type, or null for separators. This will allow for simpler queries, it's more extensible (will allow for more types to be added), and will be easier for core and extension developers to understand and work with.

The attached patch implements the solution described in comment #1. - removes the item_child and folder_child columns - adds interface constants for the item types (bookmark, folder, separator) - adds a |type| column, for holding the item type - adds a |fk| column, for holding the foreign key the record is associated with (null for separators, moz_places.id for bookmarks, moz_bookmarks_folders.id for folders) - updates all queries and callers to use the new type system

Seth, can you do a first-review on the architecture part of this? If this approach looks ok, I'll update the patch against current cvs (i think it's rotted at the moment).

dietrich, a couple comments and questions: + const short TYPE_BOOKMARK = 1; + const short TYPE_FOLDER = 2; + const short TYPE_SEPARATOR = 3; + "WHERE a.parent = ?1 AND a.type = 1 AND a.position >= ?2 AND a.position <= ?3"); + "WHERE a.parent = ?1 AND a.type = 2 AND a.position >= ?2 AND a.position <= ?3"); + "WHERE a.type = 3 AND a.parent = ?1 AND a.position >= ?2 AND a.position <= ?3"); + rv = DBConn()->CreateStatement(NS_LITERAL_CSTRING("SELECT c.id FROM moz_bookmarks a JOIN moz_bookmarks_folders c ON a.fk = c.id WHERE a.parent = ?1 AND a.type = 2 AND c.name = ?2"), getter_AddRefs(statement)); these appear to be added, but I can't figure out where the statement changed? a) + mDBIndexOfFolder->BindInt64Parameter(2, TYPE_FOLDER); b) + mDBFindURIBookmarks->BindInt32Parameter(1, TYPE_BOOKMARK);

Comment on attachment 257327 [details] [diff] [review] wip patch clearing the request, until dietrich comments on my comment, but overall, r=sspitzer

(In reply to comment #4) > dietrich, a couple comments and questions: > > + const short TYPE_BOOKMARK = 1; > + const short TYPE_FOLDER = 2; > + const short TYPE_SEPARATOR = 3; > > + "WHERE a.parent = ?1 AND a.type = 1 AND a.position >= ?2 AND a.position <= > ?3"); > > + "WHERE a.parent = ?1 AND a.type = 2 AND a.position >= ?2 AND a.position <= > ?3"); > > + "WHERE a.type = 3 AND a.parent = ?1 AND a.position >= ?2 AND a.position <= > ?3"); > > + rv = DBConn()->CreateStatement(NS_LITERAL_CSTRING("SELECT c.id FROM > moz_bookmarks a JOIN moz_bookmarks_folders c ON a.fk = c.id WHERE a.parent = ?1 > AND a.type = 2 AND c.name = ?2"), > getter_AddRefs(statement)); > > these appear to be added, but I can't figure out where the statement changed? The statements changed by the addition of the "a.type = XXX" parts in each. Although, I need to use nsPrintfCString or something there, instead of hard-coding those. > > a) > > + mDBIndexOfFolder->BindInt64Parameter(2, TYPE_FOLDER); > > b) > > + mDBFindURIBookmarks->BindInt32Parameter(1, TYPE_BOOKMARK); > What's the comment here? (In reply to comment #5) > (From update of attachment 257327 [details] [diff] [review]) > clearing the request, until dietrich comments on my comment, but overall, > r=sspitzer > Missing the rest of the comment here :)

sorry for the confusing review. > I need to use nsPrintfCString or something there, instead of hard-coding those. yes, that was my intended request. as for the other part: > + mDBIndexOfFolder->BindInt64Parameter(2, TYPE_FOLDER); > + mDBFindURIBookmarks->BindInt32Parameter(1, TYPE_BOOKMARK); you added these "binds", but where is the correspondingly added "?1" (and "?2") to the statements? That is what I meant by: "these appear to be added, but I can't figure out where the statement changed?"

(In reply to comment #7) > as for the other part: > > > + mDBIndexOfFolder->BindInt64Parameter(2, TYPE_FOLDER); > > + mDBFindURIBookmarks->BindInt32Parameter(1, TYPE_BOOKMARK); > > you added these "binds", but where is the correspondingly added "?1" (and "?2") > to the statements? > > That is what I meant by: "these appear to be added, but I can't figure out > where the statement changed?" > hm, both of those queries were changed properly in the first patch. the statements are created in nsNavBookmarks::Init(), and both have " type = ?X " added to them.

> hm, both of those queries were changed properly in the first patch. the > statements are created in nsNavBookmarks::Init(), and both have " type = ?X " > added to them. oops, you are right. I didn't see that. reviewing now...

Comment on attachment 257762 [details] [diff] [review] patch v2 - addresses seth's comments r=sspitzer

per irc conversation with sspitzer, holding off on checkin until bug 373239 is fixed.

Comment on attachment 257762 [details] [diff] [review] patch v2 - addresses seth's comments can i get additional review please?

Comment on attachment 257762 [details] [diff] [review] patch v2 - addresses seth's comments Please file a bug on adding GetItemType(aItemId) to nsINavBookmarksService. I didn't review the changes to each SQL statement here, I assume Seth did.

Mano, did you intend to cancel review?

Checking in browser/components/places/tests/bookmarks/test_bookmarks.js; /cvsroot/mozilla/browser/components/places/tests/bookmarks/test_bookmarks.js,v <-- test_bookmarks.js new revision: 1.5; previous revision: 1.4 done Checking in toolkit/components/places/public/nsINavBookmarksService.idl; /cvsroot/mozilla/toolkit/components/places/public/nsINavBookmarksService.idl,v <-- nsINavBookmarksService.idl new revision: 1.32; previous revision: 1.31 done Checking in toolkit/components/places/src/nsNavBookmarks.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.cpp,v <-- nsNavBookmarks.cpp new revision: 1.72; previous revision: 1.71 done Checking in toolkit/components/places/src/nsNavBookmarks.h; /cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.h,v <-- nsNavBookmarks.h new revision: 1.33; previous revision: 1.32 done Checking in toolkit/components/places/src/nsNavHistory.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v <-- nsNavHistory.cpp new revision: 1.108; previous revision: 1.107 done Checking in toolkit/components/places/src/nsNavHistoryAutoComplete.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp,v <-- nsNavHistoryAutoComplete.cpp new revision: 1.6; previous revision: 1.5 done Checking in toolkit/components/places/src/nsNavHistoryExpire.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryExpire.cpp,v <-- nsNavHistoryExpire.cpp new revision: 1.4; previous revision: 1.3 done

backed out due to causing 374241.

- updated against latest code - genericizes the dump/re-init/import migration method - revs db schema version for proper migration i tested these migration scenarios: v0 -> v2 v1 -> v2 v2 -> v1 (v1/v2 -> v0 is not a supported downgrade migration scenario, as noted previously, and in the code)

Comment on attachment 260282 [details] [diff] [review] fix v3 r=sspitzer, but two comments: 1) +const PRInt32 nsNavBookmarks::kFindBookmarksIndex_Fk = 2; Fk here (and "fk" other places) is folder key, right? Any reason not use something more descriptive FolderKey and folderkey instead of fk? 2) I should have caught this during bug #373239, but why: + if (PLACES_SCHEMA_VERSION == 1 || PLACES_SCHEMA_VERSION == 2) { We are keeping that in sync with the #define in this file, which is 2. I think we can remove that if (..) { and the "// Further migration code goes here." comment. I think we want: if (PLACES_SCHEMA_VERSION != schemaVersion) { // NOTE: We don't support downgrading back to History-only. If you want to go from // schema V1 back to V0, you'll need to blow away your sqlite file. // Subsequent up/downgrades will have backwards and forward migration code. // // Migrating from schema V0 - V1 to V2. if (schemaVersion < 2) { // perform upgrade rv = MigrateFromVnToV1(mDBConn); rv = ForceMigrateBookmarksDB(mDBConn); NS_ENSURE_SUCCESS(rv, rv); } else { // perform downgrade // XXX Migrations from V>=3 must add downgrade migration code here, // replacing this! rv = MigrateFromVnToV1(mDBConn); rv = ForceMigrateBookmarksDB(mDBConn); NS_ENSURE_SUCCESS(rv, rv); }

oops, that snippet should have been: // Migrating from schema V0 - V1 to V2. if (schemaVersion < 2) { // perform upgrade rv = ForceMigrateBookmarksDB(mDBConn); NS_ENSURE_SUCCESS(rv, rv); } else { // perform downgrade // XXX Migrations from V>=3 must add downgrade migration code here, // replacing this! rv = ForceMigrateBookmarksDB(mDBConn); NS_ENSURE_SUCCESS(rv, rv); }

(In reply to comment #18) > (From update of attachment 260282 [details] [diff] [review]) > r=sspitzer, but two comments: > > 1) > > +const PRInt32 nsNavBookmarks::kFindBookmarksIndex_Fk = 2; > > Fk here (and "fk" other places) is folder key, right? > no, it's for "foreign key", as that field holds ids from moz_places and moz_bookmarks_folders (and possibly others in the future). > Any reason not use something more descriptive FolderKey and folderkey instead > of fk? > we do a lot of |a.fk == b.id| which seems more readable to me. i could claim that it's less bytes and that many fewer characters that the sql parser has to scan, but the perf win there is probably negligible :) > 2) duh, i'll make this change.

dietrich, thanks for the info about fk. could I convince you add "// fk == foreign key" where it makes sense? maybe in nsNavBookmarks::InitTables() and above where you define the kFindBookmarksIndex const? (or am I the only one with some rusty SQL?) Speaking of nsNavBookmarks::InitTables(), you have: rv = aDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING("CREATE TABLE moz_bookmarks (" "id INTEGER PRIMARY KEY," - "item_child INTEGER, " - "folder_child INTEGER, " + "type INTEGER, " + "fk INTEGER, " What about using FOREIGN KEY here? From http://www.sqlite.org/omitted.html: FOREIGN KEY constraints FOREIGN KEY constraints are parsed but are not enforced. "The order of this list gives some hint as to when a feature might be added to SQLite. Those features near the top of the list are likely to be added in the near future. There are no immediate plans to add features near the bottom of the list." Note, FOREIGN KEY is at the top of that list. Or, do you think switching to FOREIGN KEY is a better spin off bug?

(In reply to comment #21) > dietrich, thanks for the info about fk. > > could I convince you add "// fk == foreign key" where it makes sense? maybe in > nsNavBookmarks::InitTables() and above where you define the kFindBookmarksIndex > const? > > (or am I the only one with some rusty SQL?) i changed the constants to *ForeignKey instead of *Fk, which will make them clearer, and i added a comment in the table creation section. > Speaking of nsNavBookmarks::InitTables(), you have: > > rv = aDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING("CREATE TABLE > moz_bookmarks (" > "id INTEGER PRIMARY KEY," > - "item_child INTEGER, " > - "folder_child INTEGER, " > + "type INTEGER, " > + "fk INTEGER, " > > What about using FOREIGN KEY here? > > From http://www.sqlite.org/omitted.html: > > FOREIGN KEY constraints FOREIGN KEY constraints are parsed but > are not enforced. > > "The order of this list gives some hint as to when a feature might be added to > SQLite. Those features near the top of the list are likely to be added in the > near future. There are no immediate plans to add features near the bottom of > the list." > > Note, FOREIGN KEY is at the top of that list. > > Or, do you think switching to FOREIGN KEY is a better spin off bug? > regardless of whether sqlite actually respects fk constraints, this won't work for us because we're using a single column for foreign keys from multiple tables.

thanks for switching to ForeignKey. As for FOREIGN KEY, thanks for pointing that out. So, for us, we need to know the type to know which table the ForeignKey actually lives in? (Can you elaborate on why that is better or worse than using multiple columns?)

(In reply to comment #23) > thanks for switching to ForeignKey. > > As for FOREIGN KEY, thanks for pointing that out. > > So, for us, we need to know the type to know which table the ForeignKey > actually lives in? > > (Can you elaborate on why that is better or worse than using multiple columns?) > Having multiple columns is inflexible in that it requires alteration of the moz_bookmarks table in order to scale to more/less types (or you end up with wonky schemes like the separator type, where both being null = separator). It's possible that we could properly normalize these relationships via another table that maps bookmarks and fks, with proper constraints. However, that's more development and perf overhead (more joins!) with no real tangible benefit. This patch is not the end-all for the Places type system, just an incremental improvement that eases further api clean-up (eg bug 372508). I'd like to eventually move to a more dynamic system where external types (eg: from extensions, or web services) can register with Places, and be viewed/managed etc.

take 2. Checking in public/nsINavBookmarksService.idl; /cvsroot/mozilla/toolkit/components/places/public/nsINavBookmarksService.idl,v <-- nsINavBookmarksService.idl new revision: 1.34; previous revision: 1.33 done Checking in src/nsNavBookmarks.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.cpp,v <-- nsNavBookmarks.cpp new revision: 1.78; previous revision: 1.77 done Checking in src/nsNavBookmarks.h; /cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.h,v <-- nsNavBookmarks.h new revision: 1.36; previous revision: 1.35 done Checking in src/nsNavHistory.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v <-- nsNavHistory.cpp new revision: 1.114; previous revision: 1.113 done Checking in src/nsNavHistory.h; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v <-- nsNavHistory.h new revision: 1.73; previous revision: 1.72 done Checking in src/nsNavHistoryAutoComplete.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp,v <-- nsNavHistoryAutoComplete.cpp new revision: 1.8; previous revision: 1.7 done Checking in src/nsNavHistoryExpire.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryExpire.cpp,v <-- nsNavHistoryExpire.cpp new revision: 1.6; previous revision: 1.5 done Checking in tests/bookmarks/test_bookmarks.js; /cvsroot/mozilla/toolkit/components/places/tests/bookmarks/test_bookmarks.js,v <-- test_bookmarks.js new revision: 1.3; previous revision: 1.2 done Checking in tests/unit/test_history.js; /cvsroot/mozilla/toolkit/components/places/tests/unit/test_history.js,v <-- test_history.js new revision: 1.2; previous revision: 1.1 done

this patch is covered by the existing api tests.

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