This is slightly orthogonal to bug 487556 as _what_ gets sent in the notification doesn't need to provide a GUID. Weave should be able to query the DB to get at the GUID, and in the common case of not needing a guid (no weave installed), actions aren't slower with extra guid overhead. GUIDs are necessary for weave to identify a satchel form entry across machines, and if the entries are the same across machines, we need to update the GUIDs to match. Form entries will need to.. 1) create a GUID on a new entry 2) provide a way to change its GUID 3) send a notification if its GUID changes 4) be looked up by GUID

Bah, submitted before adding a comment... This is a WIP patch, seems to work OK but needs tests.

Updated to trunk. Still needs tests, and Mardak noted in bug 487556 comment 9 that there's some Weave code to generate smaller GUIDs, which would be good to use here.

Comment on attachment 438199 [details] [diff] [review] Patch v.3 >+nsresult >+nsFormHistory::MigrateToVersion3() >+{ >+ nsresult rv; >+ >+ // Check to see if the new column already exists (could be a v3 DB that >+ // was downgraded to v2). If they exist, we don't need to add them. >+ nsCOMPtr<mozIStorageStatement> stmt; >+ rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING( >+ "SELECT guid FROM moz_formhistory"), >+ getter_AddRefs(stmt)); >+ >+ PRBool columnExists = !!NS_SUCCEEDED(rv); Nit: Don't need !! >+ >+ if (!columnExists) { >+ rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING( >+ "ALTER TABLE moz_formhistory ADD COLUMN guid TEXT")); >+ NS_ENSURE_SUCCESS(rv, rv); >+ } >+ >+ rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING("CREATE INDEX IF NOT EXISTS moz_formhistory_guid_index ON moz_formhistory (guid)")); >+ NS_ENSURE_SUCCESS(rv, rv); Nit: Can we break that line up? Even just doing as you did for the other statements here makes it a little better (still have a long line, but better). I know we're rewriting it soon anyway, but might as well be as awesome as possible anyway. r=zpao, but as we talked about, over to Gavin for another r? since my C++ is not at review level.

Comment on attachment 438199 [details] [diff] [review] Patch v.3 >diff --git a/toolkit/components/satchel/src/nsStorageFormHistory.cpp b/toolkit/components/satchel/src/nsStorageFormHistory.cpp >+nsFormHistory::GenerateGUID(nsACString &guidString) { >+ // Encode 12 bytes (96bits) of randomness into a 16 character base-64 string. >+ char *b64 = PL_Base64Encode(reinterpret_cast<const char *>(&rawguid), 12, nsnull); Why not use the full 16 bytes (sizeof(rawguid))? Was it just to avoid the padding in the resultant base64 string? You could use 15 bytes for that. I suppose it doesn't really matter :)

Oh, I guess it was for compat with bug 546768's patch (i.e. Weave's Utils.makeGUID())?

(In reply to comment #7) > Oh, I guess it was for compat with bug 546768's patch (i.e. Weave's > Utils.makeGUID())? Weave's guids are 10 characters long storing 70 values per character.

Pushed http://hg.mozilla.org/mozilla-central/rev/6f4d91d7f221