487556 - satchel should send a notification when adding/removing/changing an item

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement)

Description

[Justin Dolske [:Dolske]]

Weave needs to know when data is changed, so for syncing form history it needs a notification to avoid hacky problems as mentioned in bug 487541. We fixed this for password manager over in bug 475634.

Comment 1

[Matthew N. [:MattN]]

Attachment: v.1 with tests

Comment 2

[Justin Dolske [:Dolske]]

From a quick look, this patch generally looks fine...

Thunder/Mardak -- can you confirm that the notifications this is sending will be sufficient for Weave's needs?

Comment 3

[Ed Lee :Mardak]

So... the notifications are..

addEntry: [name, value]
modifyEntry: [name, value]
removeEntry: [name, value]

removeEntriesByTimeframe: [startTime, endTime]
removeEntriesForName: fieldName
removeAllEntries: null
expireOldEntries: expireTime

The notifications with name, value are useful now because we're computing a "guid" based on those. Eventually, if there's a built-in guid, I suppose it could be provided as the 3rd value in the array subject.

Just to double check, none of the bulk remove/expire send individual notifications?

Comment 4

[Justin Dolske [:Dolske]]

Comment on attachment 389646 [details] [diff] [review]
v.1 with tests


>+  nsCOMPtr<nsIMutableArray> notifData;
>+  rv = CreateNotificationData(aName, aValue, notifData);
>+  NS_ENSURE_SUCCESS(rv, rv);
...
>+    SendNotification(NS_LITERAL_STRING("modifyEntry"), notifData);

It would be cleaner to just make a single call to create + send the notification, instead of doing it in separate steps. [Though the single call might, in turn, call another function, since there are a couple of CreateNotificationData() implementations that can share some code this way.]

s/notifData/notifyData/. We can afford the extra byte. :)


>+nsresult
>+nsFormHistory::SendNotification(const nsAString &aChangeType, nsISupports *aData)
>+{
>+  return mObserverService->NotifyObservers(aData, "satchel-storage-changed", ToNewUnicode(aChangeType));
>+}

ToNewUnicode() allocates, so this leaks.

I think what you want here is PromiseFlatString(aChangeType).get()

https://developer.mozilla.org/en/XPCOM_string_guide

Yes, strings suck. :(

>+  if (!fieldName) {
>+    rv = NS_ERROR_OUT_OF_MEMORY;
>+    return rv;
>+  }

Don't set rv, just return the value directly. You could, in fact, just make this a void function and ignore the return type...

Comment 5

[Matthew N. [:MattN]]

Attachment: v.2 address review comments

(In reply to comment #3)
> Just to double check, none of the bulk remove/expire send individual
> notifications?

Correct, a single event notification gets sent for bulk removals and does not contain any info about the actual entries that were deleted as there is too much overhead in this.

Comment 6

[Justin Dolske [:Dolske]]

Comment on attachment 390362 [details] [diff] [review]
v.2 address review comments

>-nsFormHistory::GetExistingEntryID (const nsAString &aName, 
>+nsFormHistory::GetExistingEntryID (const nsAString &aName,

??? Looks like a stray whitespace change.

>-nsFormHistory::EntryExists(const nsAString &aName, 
>+nsFormHistory::EntryExists(const nsAString &aName,

And here too.

>+  nsCOMPtr<nsISupportsString> fieldName = do_CreateInstance(NS_SUPPORTS_STRING_CONTRACTID);
>+  if (!fieldName)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+
>+  fieldName->SetData(aName);
>+  SendNotification(NS_LITERAL_STRING("removeEntriesForName"), fieldName);

Shouldn't need to create fieldName here. Add a SendNotification() that takes just 1 string.

>-nsFormHistory::Observe(nsISupports *aSubject, const char *aTopic, const PRUnichar *aData) 
>+nsFormHistory::Observe(nsISupports *aSubject, const char *aTopic, const PRUnichar *aData)

??? this and a number of other whitespace only changes... Please remove.

 
>+  nsCOMPtr<nsISupportsPRInt64> expireTimeNotify = do_CreateInstance(NS_SUPPORTS_PRINT64_CONTRACTID);
>+  if (!expireTimeNotify) {
>+    rv = NS_ERROR_OUT_OF_MEMORY;
>+    return rv;
>+  }
>+  expireTimeNotify->SetData(expireTime);
>+  SendNotification(NS_LITERAL_STRING("expireOldEntries"), expireTimeNotify);

Seems like expireTimeNotify isn't needed here either. Add a SendNotification() that just takes an int.

>+/*
>+ * Send a notification when stored data is changed
>+ */
>+nsresult
>+nsFormHistory::SendNotification(const nsAString &aChangeType, nsISupports *aData)
>+{
>+  return mObserverService->NotifyObservers(aData, "satchel-storage-changed", PromiseFlatString(aChangeType).get());
>+}

Cleanup line wrapping.

>+  nsCOMPtr<nsISupportsString> fieldValue = do_CreateInstance(NS_SUPPORTS_STRING_CONTRACTID);
>+  if (!fieldValue) {
>+    rv = NS_ERROR_OUT_OF_MEMORY;
>+    return rv;
>+  }

Tsk!

>+  nsCOMPtr<nsISupportsPRInt64> valOne = do_CreateInstance(NS_SUPPORTS_PRINT64_CONTRACTID);
>+  if (!valOne) {
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  }

No braces.

>+  nsCOMPtr<nsISupportsPRInt64> valTwo = do_CreateInstance(NS_SUPPORTS_PRINT64_CONTRACTID);
>+  if (!valTwo) {
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  }

Ditto.

>+   rv = SendNotification(aChangeType, notifyData);
>+  NS_ENSURE_SUCCESS(rv, rv);

Stray space before rv.



>+++ b/toolkit/components/satchel/test/unit/test_notify.js
...
>+const STORAGE_TYPE = "mozStorage";

Don't need this.

>+fh = Cc["@mozilla.org/satchel/form-history;1"].
>+       getService(Ci.nsIFormHistory2);

Cc/getService alignment.

>+/* ========== 9 ========== */
>+testnum++;
>+testdesc = "expireOldEntries";
>+
>+expectedNotification = "expireOldEntries";
>+expectedData = null;

Shouldn't expectedData be a timestamp here? How is this passing as-is?

Comment 7

[Ed Lee :Mardak]

(In reply to comment #5)
> > Just to double check, none of the bulk remove/expire send individual
> > notifications?
> Correct, a single event notification gets sent for bulk removals and does not
> contain any info about the actual entries that were deleted as there is too
> much overhead in this.
Weave will need a "before-<remove-action>" notification so that we know what things to upload as deleted. Basically when weave gets the before-removeEntriesForName, we'll do a query for all those entries and afterwards let satchel continue with removing.

As long as weave has a way to get at the data before it's actually removed, we'll be good to go.

Comment 8

[Matthew N. [:MattN]]

Attachment: v.3 add "before-" notifications

Added before-remove notifications per the request in comment #7

(In reply to comment #6)
> >+/* ========== 9 ========== */
> >+testnum++;
> >+testdesc = "expireOldEntries";
> >+
> >+expectedNotification = "expireOldEntries";
> >+expectedData = null;
> 
> Shouldn't expectedData be a timestamp here? How is this passing as-is?

There is a check in the event callback that simply checks that the timestamp > 0

Comment 9

[Ed Lee :Mardak]

http://hg.mozilla.org/labs/weave/file/ff9b610379ca/source/modules/util.js#l149
Here's some code to generate a 10-character GUID.

For now, it could be okay to just put this inline/part of the file. We'll probably want to split it to a separate jsm/xpcom component (for places?) so that Weave and other parts of Firefox can use shorter guids.

Comment 10

[Justin Dolske [:Dolske]]

Attachment: Patch v.4

Updated to trunk. Note that this is on top of the WIP patch for bug 506402.

Comment 11

[Justin Dolske [:Dolske]]

Attachment: Patch v.5

Updated to trunk again, fixed tests due to GUID format change from bug 506402.

Comment 12

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/aa8d539cf6e1

(zomg, finally :)