481536 - Re-implement form sync engine for 0.3

Status: RESOLVED FIXED

(Cloud Services :: General, defect, P3)

Description

[Jono Xia]

What it says.  We've got an older modules/engines/forms.js that can serve as a starting point.

Comment 1

[Dan Mills [:thunder]]

Punting this to Future

Comment 2

[Anant Narayanan [:anant]]

Done: http://hg.mozilla.org/labs/weave/rev/afc084b21a86

nsIFormSubmitObserver was made scriptable via bug #370561, so we now use it in the tracker. The tracker is still a little convoluted though because we don't have GUID support in nsIFormHistory2.

The GUID for a record is still SHA1 of the name:value pair - making the update() and changeItemID() methods without much meaning (hence unimplemented). Performance-wise, this is better than the earlier version because of the new tracker, worked fine in my tests (desktop).

Form sync is still turned off by default in the preferences, pending comments from Dan.

Comment 3

[Ed Lee :Mardak]

Why not just use a GUID for the GUID? recordLike should handle duplicate records.

Comment 4

[Dan Mills [:thunder]]

(In reply to comment #3)
> Why not just use a GUID for the GUID? recordLike should handle duplicate
> records.

Because the form db doesn't store GUIDs.  e.g., Store.update(id) needs to know how to look up 'id' in the database and modify it.

(In reply to comment #2)
> Form sync is still turned off by default in the preferences, pending comments
> from Dan.

Clearly I should've commented here, I can't remember what I said ;)

I think using hashes for IDs is very suboptimal, and potentially a privacy problem.

Comment 5

[Ed Lee :Mardak]

(In reply to comment #4)
> Because the form db doesn't store GUIDs
I wonder if weave should have its own sqlite table. We currently reuse the places db for history weave/guid with annotations. Something like.. string guid <-> string data

where history data is the place id.. form data is name,value ? json([name,value]) ?

Comment 6

[Anant Narayanan [:anant]]

(In reply to comment #5)
> I wonder if weave should have its own sqlite table.

An alternative is to convince the FF team to add GUID support in FormHistory. We did get that done for the Password Manager and it worked well for everyone :)

Speaking of our own tables, I had an idea sometime last summer: syncing sqlite databases without the engine being aware of its contents . There are some nice DB synchronization algorithms out there... but I figured it would be too much of a departure from our existing model.

Comment 7

[Dan Mills [:thunder]]

In general, it is better for the component that keeps the data to decide what makes something unique.  The risk of having Weave keep its own ID mappings is that it needs to be guarantee uniqueness over data it doesn't fully control.

For example, for passwords uniqueness would be defined by realm (if set), formURL (if set), and username.  And what if the underlying store changes over time?  It's a solvable problem, but it's an extra source of bugs.

That said, there is an advantage to having a built-in system for handling arbitrary ID-less data.  Let's keep discussing this outside of this bug :)