As requested, separating out the schema changes for bug 566489.

I forgot to comment about which Firefox version that schema V13 uses, I'll do that before checkin.

Comment on attachment 571923 [details] [diff] [review] Patch Review of attachment 571923 [details] [diff] [review]: ----------------------------------------------------------------- I'd probably like to see a test to ensure the triggers functionality, but I'd be fine with just a followup bug for it. This looks good, there is some minor thing to fix before a full r+, but I'm pretty much positive about that. ::: toolkit/components/places/Database.cpp @@ +1321,5 @@ > + > +nsresult > +Database::MigrateV13Up() > +{ > + MOZ_ASSERT(NS_IsMainThread()); As I previously said, the Migrate steps are executed even if a user downgrades and then upgrades again. so here, before doing any work, you should check if the moz_hosts table already exists (iirc there is a tableExists method in Storage). And considering the implementation you likely just want to bail out if so. @@ +1329,5 @@ > + "DROP TABLE IF EXISTS moz_hostnames")); > + NS_ENSURE_SUCCESS(rv, rv); > + rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING( > + "DROP INDEX IF EXISTS moz_hostnames_frecencyindex")); > + NS_ENSURE_SUCCESS(rv, rv); I'd remove the index before the table, makes more sense (and I honestly don't know if sqlite complains when you remove a table but leave in an index) @@ +1347,5 @@ > + NS_ENSURE_SUCCESS(rv, rv); > + rv = mMainConn->ExecuteSimpleSQL(CREATE_PLACES_AFTERUPDATE_TRIGGER); > + NS_ENSURE_SUCCESS(rv, rv); > + > + // Delete any adaptive entries that won't help in ordering anymore. this comment looks completely unrelated... @@ +1348,5 @@ > + rv = mMainConn->ExecuteSimpleSQL(CREATE_PLACES_AFTERUPDATE_TRIGGER); > + NS_ENSURE_SUCCESS(rv, rv); > + > + // Delete any adaptive entries that won't help in ordering anymore. > + nsCOMPtr<mozIStorageAsyncStatement> fillHostnameTable; nit: we usually suffix statements names with Stmt, so maybe fillHostsStmt? @@ +1353,5 @@ > + rv = mMainConn->CreateAsyncStatement(NS_LITERAL_CSTRING( > + "INSERT OR IGNORE INTO moz_hosts (id, host, frecency) " > + "SELECT (SELECT id FROM moz_hosts WHERE host = fixup_url(get_unreversed_host(h.rev_host))) AS id, " > + "fixup_url(get_unreversed_host(h.rev_host)) AS host, " > + "MAX(h.frecency) as frecency " this "as" should be uppercase as the others ::: toolkit/components/places/SQLFunctions.cpp @@ +739,5 @@ > +//////////////////////////////////////////////////////////////////////////////// > +//// Fixup URL Function > + > + ////////////////////////////////////////////////////////////////////////////// > + //// GetUnreversedHostFunction this comment should be updated. I should likely get rid of some of these boilerplate comments in future... oh well. ::: toolkit/components/places/nsPlacesIndexes.h @@ +150,5 @@ > + */ > + > +#define CREATE_IDX_MOZ_HOSTS_FRECENCY \ > + CREATE_PLACES_IDX( \ > + "frecencyindex", "moz_hosts", "frecency, host", "" \ rename to frecencyhostindex and CREATE_IDX_MOZ_HOSTS_FRECENCYHOST per naming convention

Comment on attachment 572478 [details] [diff] [review] Patch 2 Review of attachment 572478 [details] [diff] [review]: ----------------------------------------------------------------- Please land this along with bug 566489. Or, to better clarify: - if bug 566489 lands for Firefox 10, land this, obviously. - If bug 566489 moves to Firefox 11, land this just _after_ the Aurora uplift, so we can then proceed with patches touching the same code, without immediately affecting a release that would not be using these changes yet. ::: toolkit/components/places/Database.cpp @@ +1325,5 @@ > +Database::MigrateV13Up() > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + bool tableExists; init to false @@ +1326,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + bool tableExists; > + mMainConn->TableExists(NS_LITERAL_CSTRING("moz_hosts"), &tableExists); assign rv and NS_ENSURE_SUCCESS it @@ +1338,5 @@ > + "DROP INDEX IF EXISTS moz_hostnames_frecencyindex")); > + NS_ENSURE_SUCCESS(rv, rv); > + nsresult rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING( > + "DROP TABLE IF EXISTS moz_hostnames")); > + NS_ENSURE_SUCCESS(rv, rv); I think this doesn't compile, since nsresult rv is defined after its first use. I suppose it's just a copy/paste issue, the usual last-minute change :) @@ +1341,5 @@ > + "DROP TABLE IF EXISTS moz_hostnames")); > + NS_ENSURE_SUCCESS(rv, rv); > + > + // Add the moz_hosts table so we can get hostnames for URL > + // autocomplete. nit: doesn't look like you need to newline to stay below 80 chars @@ +1348,5 @@ > + rv = mMainConn->ExecuteSimpleSQL(CREATE_IDX_MOZ_HOSTS_FRECENCYHOST); > + NS_ENSURE_SUCCESS(rv, rv); > + > + // Add the triggers that update the moz_hosts table as > + // necessary. nit: doesn't look like you need to newline to stay in 80 chars @@ +1365,5 @@ > + "fixup_url(get_unreversed_host(h.rev_host)) AS host, " > + "MAX(h.frecency) AS frecency " > + "FROM moz_places h " > + "WHERE h.hidden = 0 AND LENGTH(h.rev_host) > 1 " > + "GROUP BY host"), this group by looks slow to me, I think grouping by h.rev_host is faster and has the same effect ::: toolkit/components/places/SQLFunctions.h @@ +280,5 @@ > + * > + * @param url > + * A URL. > + * @return > + * The same URL, but may have redundant parts removed. oneline as "@return The same URL, with redundant parts removed."

and please, file follow-up bug to add some testing for moz_hosts contents (triggers live-update and such)

Ping? I'd love to see this landing, so I can work on top of it, there are other outstanding schema changes out there.

Sorry, roc has pulled me away from this at the last minute to work on something else :( Would you have time to finish up this bug and bug 566489?

Just want you to make sure this is OK, as I had to increase this to schema #14

Comment on attachment 578093 [details] [diff] [review] Patch 3 - unbitrotten Review of attachment 578093 [details] [diff] [review]: ----------------------------------------------------------------- What is missing is a simple xpcshell-test that adds some visits and removes them, adds a bookmark, changes its uri, then removes it, and for each change checks that moz_hosts has correctly been updated. And these few comments: ::: toolkit/components/places/Database.cpp @@ +648,5 @@ > + rv = MigrateV14Up(); > + NS_ENSURE_SUCCESS(rv, rv); > + } > + > + // TODO: fix this comment. Firefox 1x uses schema version 14. This should be moved above the "// Firefox 11 uses schema version 13." comment and that comment should be updated as "// Firefox 11 uses schema version 14." The TODO comment should be removed. @@ +1350,5 @@ > +Database::MigrateV14Up() > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + bool tableExists; init to false @@ +1351,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + bool tableExists; > + mMainConn->TableExists(NS_LITERAL_CSTRING("moz_hosts"), &tableExists); check rv @@ +1357,5 @@ > + return NS_OK; > + } > + > + // For anyone who used in-development versions of this autocomplete, > + // drop the old tables. s/the old tables/the old table and its indexes./ @@ +1359,5 @@ > + > + // For anyone who used in-development versions of this autocomplete, > + // drop the old tables. > + rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING( > + "DROP INDEX IF EXISTS moz_hostnames_frecencyindex")); indent just once and move brackets on a new line as rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING( "DROP INDEX IF EXISTS moz_hostnames_frecencyindex" )); @@ +1361,5 @@ > + // drop the old tables. > + rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING( > + "DROP INDEX IF EXISTS moz_hostnames_frecencyindex")); > + NS_ENSURE_SUCCESS(rv, rv); > + nsresult rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING( rv redeclaration @@ +1362,5 @@ > + rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING( > + "DROP INDEX IF EXISTS moz_hostnames_frecencyindex")); > + NS_ENSURE_SUCCESS(rv, rv); > + nsresult rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING( > + "DROP TABLE IF EXISTS moz_hostnames")); ditto on indentation @@ +1366,5 @@ > + "DROP TABLE IF EXISTS moz_hostnames")); > + NS_ENSURE_SUCCESS(rv, rv); > + > + // Add the moz_hosts table so we can get hostnames for URL > + // autocomplete. oneline this, doesn't look over 80 chars @@ +1373,5 @@ > + rv = mMainConn->ExecuteSimpleSQL(CREATE_IDX_MOZ_HOSTS_FRECENCYHOST); > + NS_ENSURE_SUCCESS(rv, rv); > + > + // Add the triggers that update the moz_hosts table as > + // necessary. oneline this, as well @@ +1381,5 @@ > + NS_ENSURE_SUCCESS(rv, rv); > + rv = mMainConn->ExecuteSimpleSQL(CREATE_PLACES_AFTERUPDATE_TRIGGER); > + NS_ENSURE_SUCCESS(rv, rv); > + > + // Fill the moz_hosts table with all domains we have visited. s/domains we have visited/the domains in moz_places/ @@ +1390,5 @@ > + "fixup_url(get_unreversed_host(h.rev_host)) AS host, " > + "MAX(h.frecency) AS frecency " > + "FROM moz_places h " > + "WHERE h.hidden = 0 AND LENGTH(h.rev_host) > 1 " > + "GROUP BY host"), group by h.rev_host instead ::: toolkit/components/places/Database.h @@ +46,5 @@ > #include "mozilla/storage/StatementCache.h" > > // This is the schema version, update it at any schema change and add a > // corresponding migrateVxx method below. > +#define DATABASE_SCHEMA_VERSION 14 Just as a side note, this will likely land after favicons GUIDs, so we'll have to bump to 15. I suppose it's question of hours, depending on rnewman's available time. I'll notify here when that happens. ::: toolkit/components/places/SQLFunctions.h @@ +280,5 @@ > + * > + * @param url > + * A URL. > + * @return > + * The same URL, but may have redundant parts removed. "@return The same URL, with redundant parts removed." ::: toolkit/components/places/tests/migration/test_current_from_v10.js @@ +286,5 @@ > + + "FROM moz_hosts " > + ); > + stmt.finalize(); > + > + run_next_test(); could you please also add a query that checks the number of entries in moz_hosts equals the number of unique rev_host in moz_places? @@ +306,5 @@ > } > > do_check_true(db.indexExists("moz_bookmarks_guid_uniqueindex")); > do_check_true(db.indexExists("moz_places_guid_uniqueindex")); > + do_check_true(db.indexExists("moz_hosts_frecencyhostindex")); newline before the new check please, since it's not a guid index

favicons guids landed, so the schema version has to be bumped again to v15.

One failing check, perhaps my query is wrong. No doubt my test is not following the code style as well

Comment on attachment 579171 [details] [diff] [review] Patch 4 Comments addressed, need feedback Review of attachment 579171 [details] [diff] [review]: ----------------------------------------------------------------- It looks fine, the tests need some adjustements, nothing crazy, we are near. here is the feedback! ::: toolkit/components/places/Database.cpp @@ +646,5 @@ > + > + if (currentSchemaVersion < 14) { > + rv = MigrateV14Up(); > + NS_ENSURE_SUCCESS(rv, rv); > + } Since Firefox 11 has not yet been released, the comment should be moved below the migrator call, not be above it. Btw, please update your local tree and bump to 15. @@ +1349,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + bool tableExists = false; > + nsresult rv; please declare rv on first use, that is just a row below. no newline between tableExists and the next row that is using it. @@ +1361,5 @@ > + > + // For anyone who used in-development versions of this autocomplete, > + // drop the old tables and its indexes. > + rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING( > + "DROP INDEX IF EXISTS moz_hostnames_frecencyindex" should be indented 2 spaces, as any other code, not 1 @@ +1365,5 @@ > + "DROP INDEX IF EXISTS moz_hostnames_frecencyindex" > + )); > + NS_ENSURE_SUCCESS(rv, rv); > + rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING( > + "DROP TABLE IF EXISTS moz_hostnames" ditto in indentation ::: toolkit/components/places/tests/migration/test_current_from_v10.js @@ +304,5 @@ > + ); > + stmt.executeStep(); > + let mozPlacesCount = stmt.getInt32(0); > + stmt.finalize(); > + do_check_eq(mozPlacesCount, mozHostsCount); You can coalesce the two queries into one, using subqueries. For example I think you could do a SELECT (SELECT count(host) from moz_hosts), (SELECT count(DISTINCT rev_host) FROM moz_hosts) then use getInt32(0) and getInt32(1) (you may even subtract in the query and check for 0, but getting both we preserve better output) Note that only valid hosts are stored in moz_hosts, so it's possible this won't pass, for example places queries have a null host, file:// has a . host. You may be able to filter on LENGTH as SELECT count(DISTINCT rev_host) from moz_hosts WHERE LENGTH(rev_host) > 1 @@ +309,5 @@ > + > + run_next_test(); > +} > + > +function test_moz_hosts_update() Could you please move this more specific test to its own test file under toolkit/components/places/tests/unit/test_hosts_triggers.js ? I'd like to have the migration/ folder dedicated only to migration tests, these is far more specific and by moving it to its own file you'll be able to move more freely. The other tests are fine here, since checking what happens in the migrator. If you put that file in a separate patch, we may be even able to r+ all the rest @@ +313,5 @@ > +function test_moz_hosts_update() > +{ > + // add some visits and remove them, add a bookmark, > + // change its uri, then remove it, and > + // for each change check that moz_hosts has correctly been updated. <3 @@ +315,5 @@ > + // add some visits and remove them, add a bookmark, > + // change its uri, then remove it, and > + // for each change check that moz_hosts has correctly been updated. > + > + Cu.import("resource://gre/modules/PlacesUtils.jsm"); You don't need to do this, all Places xpcshell tests have PlacesUtils in the scope already. @@ +328,5 @@ > + aTransitionType === undefined ? TRANSITION_LINK : aTransitionType; > + this.visitDate = aVisitTime || Date.now() * 1000; > + } > + > + let urls = ["http://visit1.mozilla.org", "http://visit2.mozilla.org",]; const URLS = please, one entry per line. use trailing commas only if they are trailing, not on oneliners please also add one entry with www. to check it's correctly stripped. @@ +339,5 @@ > + new VisitInfo(), > + ], > + }; > + places.push(place); > + }); nit: I'm sure you can find some better indentation for this @@ +344,5 @@ > + > + gHistory.updatePlaces(places); > + > + PlacesUtils.history.removePage(uri(urls[0])); > + PlacesUtils.history.removePage(uri(urls[1])); updatePlaces is asynchronous, you'll have to use its callback to be able to proceed. Otherwise this may happen at any time during your test. And, before removing uris, would be nice to check if all entries have been added (you could make an helper function like isHostInDatabase(host) to simplify checking) @@ +358,5 @@ > + + "FROM moz_hosts " > + + "WHERE host = :host" > + ); > + stmt.params.host = "test.mozilla.org"; > + stmt.executeStep(); do_check_true this @@ +364,5 @@ > + stmt.finalize(); > + > + // Change the hostname > + PlacesUtils.bookmarks.changeBookmarkURI(id, > + uri("http://different.mozilla.org/")); if possible please use NetUtil.newURI( everywhere instead of uri( @@ +372,5 @@ > + + "FROM moz_hosts " > + + "WHERE host = :host" > + ); > + stmt.params.host = "different.mozilla.org"; > + stmt.executeStep(); do_check_true @@ +388,5 @@ > + let rows = 0; // doesn't seme to be a way to get the number of rows returned > + while (statement.executeStep()) { > + rows++; > + } > + do_check_eq(rows, 0); actually you may do do_check_false(statement.executeStep) btw, should be stmt, not statement. And this test will fail, because removeItem does not remove the page synchronously, it only removes the bookmark, you should use waitForClearHistory(callback), check into the callback and then invoke run_next_test() @@ +411,5 @@ > > do_check_true(db.indexExists("moz_bookmarks_guid_uniqueindex")); > do_check_true(db.indexExists("moz_places_guid_uniqueindex")); > > + do_check_true(db.indexExists("moz_hosts_frecencyhostindex")); This test can stay here, it's related to the migration. @@ +432,5 @@ > test_moz_places_guid_exists, > test_place_guids_non_null, > test_place_guid_annotation_imported, > test_place_guid_annotation_removed, > + test_moz_hosts, FWIW you forgot to add the second test, so you didn't catch its failures, but it's fine, move it to its own file.

(In reply to Marco Bonardo [:mak] from comment #12) > SELECT count(DISTINCT rev_host) from moz_hosts WHERE LENGTH(rev_host) > 1 Here I actually meant "FROM moz_places"

hmm. still seeing this error: TEST-UNEXPECTED-FAIL | /home/ddahl/code/moz/obj-x86_64-unknown-linux-gnu-debug/_tests/xpcshell/toolkit/components/places/tests/migration/test_current_from_v10.js | 4 == 6 - See following stack: JS frame :: /home/ddahl/code/moz/mozilla-central/testing/xpcshell/head.js :: do_throw :: line 453 JS frame :: /home/ddahl/code/moz/mozilla-central/testing/xpcshell/head.js :: _do_check_eq :: line 547 JS frame :: /home/ddahl/code/moz/mozilla-central/testing/xpcshell/head.js :: do_check_eq :: line 568 JS frame :: /home/ddahl/code/moz/obj-x86_64-unknown-linux-gnu-debug/_tests/xpcshell/toolkit/components/places/tests/migration/test_current_from_v10.js :: test_moz_hosts :: line 307 JS frame :: /home/ddahl/code/moz/mozilla-central/testing/xpcshell/head.js :: _run_next_test :: line 873 JS frame :: /home/ddahl/code/moz/mozilla-central/testing/xpcshell/head.js :: <TOP_LEVEL> :: line 420 this would be after the newly coalesced query: var query = "SELECT (" + "SELECT COUNT(host) " + "FROM moz_hosts), (" + "SELECT COUNT(DISTINCT rev_host) " + "FROM moz_places " + "WHERE LENGTH(rev_host) > 1)"; stmt = DBConn().createStatement(query); stmt.executeStep(); let mozPlacesCount = stmt.getInt32(0); let mozHostsCount = stmt.getInt32(1); do_check_eq(mozPlacesCount, mozHostsCount);

unbitrot, spent half of the day trying to figure out "is it async or not" toolkit/components/places/tests/unit/test_hosts_triggers.js

you can use dump_table(tablename); in any Places xpcshell-test to quickly print contents of a table and debug failures.

So, looking at what you showed me on IRC, you should remove the hidden = 0 condition from the trigger to fix the bookmarks issue. Correctly use callbacks in updatePlaces and waitForClearHistory (those should be the last calls on mtainthread for that specific test) Also, the new test can be splitted into different functions, one for updatePlaces, one for clearHistory and so on.

Comment on attachment 580491 [details] [diff] [review] Patch 6 - Trigger problems clearing per previous comment.

Comment on attachment 581027 [details] [diff] [review] Patch 7.1 - all checks work Review of attachment 581027 [details] [diff] [review]: ----------------------------------------------------------------- It looks done, the test is fine, some missing stmt.finalize(), mostly cleanups you may do, but nothing major or missing. if you want a further pass I'm available, but I don't see anything major I should look at further. ::: toolkit/components/places/Database.h @@ +295,1 @@ > nsresult CheckAndUpdateGUIDs(); please don't remove the newline between the latest migrator and and CheckAndUpdateGUIDs() ::: toolkit/components/places/tests/migration/test_current_from_v10.js @@ +305,5 @@ > + let mozHostsCount = stmt.getInt32(1); > + > + do_check_eq(mozPlacesCount, mozHostsCount); > + > + stmt.finalize(); please move stmt.finalize() before the do_check_eq() and remove some of the newlines from here, the function contents are a bit sparse :) Ad a side note, you may label the select fields like using AS places_count and AS hosts_count, and use stmt.row.places_count and stmt.row.hosts_count instead of getInt32. You may even avoid the temporary variables and rather put finalize() in a finally clause, like: try { stmt.executeStep(); do_check_eq(stmt.row.pages_count, stmt.row.hosts_count); } finally { stmt.finalize(); } ::: toolkit/components/places/tests/unit/test_hosts_triggers.js @@ +20,5 @@ > + result = true; > + break; > + } > + } > + return result; stmt.finalize() before the return. @@ +32,5 @@ > + + "FROM moz_hosts " > + + "WHERE host = :host" > + ); > + let result = false; > + LOG(url.host); debugging code? btw if you need to print something in the final version, you can use do_log_info(msg) @@ +40,5 @@ > + result = true; > + break; > + } > + } > + return result; stmt.finalize() before the return @@ +45,5 @@ > +} > + > +let urls = ["http://visit1.mozilla.org", > + "http://visit2.mozilla.org", > + "http://www.foo.mozilla.org",]; closing square on new line, aligned with open square @@ +57,5 @@ > + new VisitInfo(), > + ], > + }; > + places.push(place); > +}); "places" is only used in test_moz_hosts_update, so it should be defined and filled-up inside it. @@ +66,5 @@ > + aTransitionType === undefined ? TRANSITION_LINK : aTransitionType; > + this.visitDate = aVisitTime || Date.now() * 1000; > +} > + > +const NEW_HOSTNAME = "http://different.mozilla.org/"; this is not a hostname, it's a url, so may NEW_URL? or URL_WITH_NEW_HOSTNAME? @@ +72,5 @@ > +function test_moz_hosts_update() > +{ > + // add some visits and remove them, add a bookmark, > + // change its uri, then remove it, and > + // for each change check that moz_hosts has correctly been updated. please move this to the top comment in the test, with some punctuation fixes, since it's not actually describing this specific sub-test, but the global test. @@ +81,5 @@ > + > + gHistory.updatePlaces(places, { > + handleResult: function () { > + > + }, remove useless newline, it's ok to have empty body @@ +83,5 @@ > + handleResult: function () { > + > + }, > + handleError: function () { > + throw new Error("gHistory.updatePlaces() failed"); use do_throw() instead or the test may never catch this @@ +87,5 @@ > + throw new Error("gHistory.updatePlaces() failed"); > + }, > + handleCompletion: function () { > + dump_table("moz_hosts"); > + dump_table("moz_places"); comment out or remove these, not for final version of the test @@ +91,5 @@ > + dump_table("moz_places"); > + do_check_true(isHostInMozHosts(urls[0])); > + do_check_true(isHostInMozHosts(urls[1])); > + // strip the WWW from the url before testing... > + do_check_true(isHostInMozHosts("http://foo.mozilla.org")); nit: this may be better handled in the array, for example instead of having simple strings in the array you may have objects like { uri: NetUtil.newURI("http://www.foo.mozilla.org"), expected: "foo.mozilla.org" } and use .uri or .expected based on what you need... then you may use loops @@ +101,5 @@ > +function test_remove_places() > +{ > + PlacesUtils.history.removePage(NetUtil.newURI(urls[0])); > + PlacesUtils.history.removePage(NetUtil.newURI(urls[1])); > + PlacesUtils.history.removePage(NetUtil.newURI(urls[2])); I wonder if would not be better to have nsIURI objects in the array and just loop here (see above) as well as you could change your helpers to get nsIURI objects instead of creating them inside. @@ +106,5 @@ > + > + waitForClearHistory(function (){ > + do_check_false(isHostInMozHosts(urls[0])); > + do_check_false(isHostInMozHosts(urls[1])); > + do_check_false(isHostInMozHosts(urls[2])); as well as this could loop @@ +111,5 @@ > + run_next_test(); > + }); > +} > + > +ITEM_ID = null; let's avoid this global, use a local let itemId in test_bookmark_changes and instead in test_bookmark_removal do let itemId = PlacesUtils.bookmarks.getIdForItemAt(PlacesUtils.unfiledBookmarksFolderId, PlacesUtils.bookmarks.DEFAULT_INDEX); should give you the id of the last bookmark in unfiled, that is what you just added :) @@ +129,5 @@ > + PlacesUtils.bookmarks.changeBookmarkURI(ITEM_ID, NetUtil.newURI(NEW_HOSTNAME)); > + > + waitForClearHistory(function (){ > + do_check_true(isHostInMozPlaces(NEW_HOSTNAME)); > + do_check_true(isHostInMozHosts(NEW_HOSTNAME)); you may also check that the old host name (that you replaced) is not in the table anymore ::: toolkit/components/places/tests/unit/xpcshell.ini @@ +4,5 @@ > > [test_000_frecency.js] > [test_248970.js] > [test_317472.js] > +[test_hosts_triggers.js] please try to respect alphabetical order, how far that's possible

solves a minor glitch in the first import query

found a problem, when downgrading the triggers still exist (fine) but the fixup_url function does not :( If we make the triggers temporary, that is likely (and sadly) the best way out, we have to add all missing entries on each upgrade.

had to reintroduced the unique index on host, otherwise triggers are just too slow.

(In reply to Marco Bonardo [:mak] from comment #26) > Created attachment 582286 [details] [diff] [review] > Patch 7.4 > > had to reintroduced the unique index on host, otherwise triggers are just > too slow. In that case, should you change the comment? And would you still need the host-frecency index? +// The host column should be declared UNIQUE, but that will create an +// index that we never use, and we index on (host, frecency) later anyway. +#define CREATE_MOZ_HOSTS NS_LITERAL_CSTRING( \ + "CREATE TABLE moz_hosts (" \ + " id INTEGER PRIMARY KEY" \ + ", host TEXT NOT NULL UNIQUE" \ + ", frecency INTEGER" \ + ")" \ +)

(In reply to Michael Ventnor from comment #27) > (In reply to Marco Bonardo [:mak] from comment #26) > > Created attachment 582286 [details] [diff] [review] > > Patch 7.4 > > > > had to reintroduced the unique index on host, otherwise triggers are just > > too slow. > > In that case, should you change the comment? And would you still need the > host-frecency index? yes the comment should be updated, good catch. We need both since we have a (frecency,host) index, not an (host,frecency) index. The latter is a covering index for the search that ensures maximum speed, unfortunately it can't be used to match by host, that is needed by triggers. We may survive with an host and a frecency index, separated, though wouldn't be the most efficent env.

I wrongly wrote the latter, but it's (frecency,host) that is a covering index and the one we use.