Closed
Bug 506402
Opened 15 years ago
Closed 15 years ago
Add GUIDs to satchel form entries
Categories
(Toolkit :: Form Manager, defect)
Toolkit
Form Manager
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a5
People
(Reporter: Mardak, Assigned: Dolske)
References
Details
Attachments
(1 file, 2 obsolete files)
21.93 KB,
patch
|
zpao
:
review+
Gavin
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•15 years ago
|
||
Assignee | ||
Updated•15 years ago
|
Attachment #390606 -
Attachment is patch: true
Attachment #390606 -
Attachment mime type: application/octet-stream → text/plain
Assignee | ||
Comment 2•15 years ago
|
||
Bah, submitted before adding a comment...
This is a WIP patch, seems to work OK but needs tests.
Assignee | ||
Updated•15 years ago
|
Assignee: nobody → dolske
Assignee | ||
Comment 3•15 years ago
|
||
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.
Attachment #390606 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Assignee | ||
Comment 4•15 years ago
|
||
Attachment #436109 -
Attachment is obsolete: true
Attachment #438199 -
Flags: review?(paul)
Comment 5•15 years ago
|
||
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.
Attachment #438199 -
Flags: review?(paul)
Attachment #438199 -
Flags: review?(gavin.sharp)
Attachment #438199 -
Flags: review+
Comment 6•15 years ago
|
||
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 :)
Attachment #438199 -
Flags: review?(gavin.sharp) → review+
Comment 7•15 years ago
|
||
Oh, I guess it was for compat with bug 546768's patch (i.e. Weave's Utils.makeGUID())?
Reporter | ||
Comment 8•15 years ago
|
||
(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.
Assignee | ||
Comment 9•15 years ago
|
||
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
You need to log in
before you can comment on or make changes to this bug.
Description
•