[Sync] Make P2PNotifier behave more like InvalidationNotifier

[Sync] Make P2PNotifier behave more like InvalidationNotifier

Encode the changed types into the XMPP push message and nudge only for those data types.

Filter out notifications sent from the originating client.

Instead of calling observers directly, synthesize a notification and send it to the server with target = self only.

BUG=92928
TEST=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98551

[akalin, 2011-08-26 00:46:12]

+evan for base/ stuff, +zea for everything else.

[Evan Martin, 2011-08-26 18:46:33]

+stevenjb for the values.h heads-up,
tfarina because he has been working on changing this

http://codereview.chromium.org/7745040/diff/1/base/values.h
File base/values.h (right):

http://codereview.chromium.org/7745040/diff/1/base/values.h#newcode105
base/values.h:105: virtual bool GetAsDictionary(const DictionaryValue**
out_value) const;
I think we're trying to move to a different API, like:

virtual DictionaryValue* AsDictionary() const;
which returns NULL for non-dictionaries.

[akalin, 2011-08-26 18:56:42]

http://codereview.chromium.org/7745040/diff/1/base/values.h
File base/values.h (right):

http://codereview.chromium.org/7745040/diff/1/base/values.h#newcode105
base/values.h:105: virtual bool GetAsDictionary(const DictionaryValue**
out_value) const;
On 2011/08/26 18:46:33, Evan Martin wrote:
> I think we're trying to move to a different API, like:
> 
> virtual DictionaryValue* AsDictionary() const;
> which returns NULL for non-dictionaries.

Okay.  It seems kinda weird to have a mixture of old/new APIs, though.  I'd
rather have it be consistent and them all be changed at once.

[tfarina, 2011-08-26 18:59:05]

http://codereview.chromium.org/7745040/diff/1/base/values.h
File base/values.h (right):

http://codereview.chromium.org/7745040/diff/1/base/values.h#newcode105
base/values.h:105: virtual bool GetAsDictionary(const DictionaryValue**
out_value) const;
On 2011/08/26 18:56:43, akalin wrote:
> On 2011/08/26 18:46:33, Evan Martin wrote:
> > I think we're trying to move to a different API, like:
> > 
> > virtual DictionaryValue* AsDictionary() const;
> > which returns NULL for non-dictionaries.
> 
> Okay.  It seems kinda weird to have a mixture of old/new APIs, though.  I'd
> rather have it be consistent and them all be changed at once.

Don't need to add AsDictionary in this patch. The others As* functions were
reverted by Steven already.

[akalin, 2011-08-26 19:54:59]

Okay.  Is that a LGTM for the base/ changes?

On Sat, Aug 27, 2011 at 2:59 AM,  <tfarina@chromium.org> wrote:
>
> http://codereview.chromium.org/7745040/diff/1/base/values.h
> File base/values.h (right):
>
> http://codereview.chromium.org/7745040/diff/1/base/values.h#newcode105
> base/values.h:105: virtual bool GetAsDictionary(const DictionaryValue**
> out_value) const;
> On 2011/08/26 18:56:43, akalin wrote:
>>
>> On 2011/08/26 18:46:33, Evan Martin wrote:
>> > I think we're trying to move to a different API, like:
>> >
>> > virtual DictionaryValue* AsDictionary() const;
>> > which returns NULL for non-dictionaries.
>
>> Okay.  It seems kinda weird to have a mixture of old/new APIs, though.
>
>  I'd
>>
>> rather have it be consistent and them all be changed at once.
>
> Don't need to add AsDictionary in this patch. The others As* functions
> were reverted by Steven already.
>
> http://codereview.chromium.org/7745040/
>

[tfarina, 2011-08-26 19:56:44]

On 2011/08/26 19:54:59, akalin wrote:
> Okay.  Is that a LGTM for the base/ changes?
> 
I don't own anything, so I leave that for Evan. Although it lg for me.

[Evan Martin, 2011-08-26 19:57:37]

You need Steven to sign off on this.  Otherwise base LGTM

[stevenjb, 2011-08-26 20:37:35]

Thanks Evan. This would in fact break ChromeOS again (boy, values.h is popular
this week! 10 months of no changes then 3 in a week!)

The removal of this dependency is in progress. I expect the code to be complete
today, but it will take another day or two for it to propagate fully. If you
could hold off on pushing this a few more days, that would be fantastic.

I will comment here when it is all clear.

[akalin, 2011-08-26 20:55:32]

On Sat, Aug 27, 2011 at 4:37 AM,  <stevenjb@chromium.org> wrote:
> Thanks Evan. This would in fact break ChromeOS again (boy, values.h is
> popular
> this week! 10 months of no changes then 3 in a week!)
>
> The removal of this dependency is in progress. I expect the code to be
> complete
> today, but it will take another day or two for it to propagate fully. If you
> could hold off on pushing this a few more days, that would be fantastic.
>
> I will comment here when it is all clear.

Hmm.  Since Values seems to be in flux, why don't I just revert my
changes to it (just using static_cast<>/GetType() instead) and put in
a TODO to use the new API when it lands?

I think I'll do that.

[Nicolas Zea, 2011-08-26 21:19:14]

Some comments

http://codereview.chromium.org/7745040/diff/1/chrome/browser/sync/notifier/in...
File chrome/browser/sync/notifier/invalidation_notifier_unittest.cc (right):

http://codereview.chromium.org/7745040/diff/1/chrome/browser/sync/notifier/in...
chrome/browser/sync/notifier/invalidation_notifier_unittest.cc:37:
invalidation_notifier_.reset(new InvalidationNotifier(notifier_options,
If this takes ownership of the request_context_getter, a comment to that effect
would be useful. If not, where does it get deleted?

http://codereview.chromium.org/7745040/diff/1/chrome/browser/sync/notifier/p2...
File chrome/browser/sync/notifier/p2p_notifier.cc (right):

http://codereview.chromium.org/7745040/diff/1/chrome/browser/sync/notifier/p2...
chrome/browser/sync/notifier/p2p_notifier.cc:273: }
Should this check to see if the changed types are a subset of the enabled types?

http://codereview.chromium.org/7745040/diff/1/chrome/browser/sync/notifier/p2...
chrome/browser/sync/notifier/p2p_notifier.cc:275:
syncable::ModelTypePayloadMapFromBitSet(
We really need to standardize bitset vs set, but that's outside of the scope of
this.

http://codereview.chromium.org/7745040/diff/1/chrome/browser/sync/notifier/p2...
File chrome/browser/sync/notifier/p2p_notifier.h (right):

http://codereview.chromium.org/7745040/diff/1/chrome/browser/sync/notifier/p2...
chrome/browser/sync/notifier/p2p_notifier.h:68: bool FromString(const
std::string& str);
Since this overwrites the current data, how about ResetFromString?

http://codereview.chromium.org/7745040/diff/1/chrome/test/live_sync/migration...
File chrome/test/live_sync/migration_errors_test.cc (right):

http://codereview.chromium.org/7745040/diff/1/chrome/test/live_sync/migration...
chrome/test/live_sync/migration_errors_test.cc:138:
FAILS_MigratePrefsThenModifyBookmark) {
I'd kind of prefer this (and the other failing tests in here) be disabled, as
they're actually timing out and each take ~1min to complete when they do.

[akalin, 2011-08-26 22:42:27]

Addressed all comments, reverted all base/ changes (and removed base/
reviewers).

PTAL.

http://codereview.chromium.org/7745040/diff/1/chrome/browser/sync/notifier/in...
File chrome/browser/sync/notifier/invalidation_notifier_unittest.cc (right):

http://codereview.chromium.org/7745040/diff/1/chrome/browser/sync/notifier/in...
chrome/browser/sync/notifier/invalidation_notifier_unittest.cc:37:
invalidation_notifier_.reset(new InvalidationNotifier(notifier_options,
On 2011/08/26 21:19:14, nzea wrote:
> If this takes ownership of the request_context_getter, a comment to that
effect
> would be useful. If not, where does it get deleted?

It's ref-counted, so it's not owned by anything.  Added comment.

http://codereview.chromium.org/7745040/diff/1/chrome/browser/sync/notifier/p2...
File chrome/browser/sync/notifier/p2p_notifier.cc (right):

http://codereview.chromium.org/7745040/diff/1/chrome/browser/sync/notifier/p2...
chrome/browser/sync/notifier/p2p_notifier.cc:273: }
On 2011/08/26 21:19:14, nzea wrote:
> Should this check to see if the changed types are a subset of the enabled
types?

The InvalidationNotifier doesn't, so I don't think this does.  I think when we
discussed it w.r.t. InvalidationNotifier, we decided it wasn't worth worrying
about.

http://codereview.chromium.org/7745040/diff/1/chrome/browser/sync/notifier/p2...
chrome/browser/sync/notifier/p2p_notifier.cc:275:
syncable::ModelTypePayloadMapFromBitSet(
On 2011/08/26 21:19:14, nzea wrote:
> We really need to standardize bitset vs set, but that's outside of the scope
of
> this.

Yeah.  Although I think it's better to make a special class with the best
elements of both.

http://codereview.chromium.org/7745040/diff/1/chrome/browser/sync/notifier/p2...
File chrome/browser/sync/notifier/p2p_notifier.h (right):

http://codereview.chromium.org/7745040/diff/1/chrome/browser/sync/notifier/p2...
chrome/browser/sync/notifier/p2p_notifier.h:68: bool FromString(const
std::string& str);
On 2011/08/26 21:19:14, nzea wrote:
> Since this overwrites the current data, how about ResetFromString?

Done.

http://codereview.chromium.org/7745040/diff/1/chrome/test/live_sync/migration...
File chrome/test/live_sync/migration_errors_test.cc (right):

http://codereview.chromium.org/7745040/diff/1/chrome/test/live_sync/migration...
chrome/test/live_sync/migration_errors_test.cc:138:
FAILS_MigratePrefsThenModifyBookmark) {
On 2011/08/26 21:19:14, nzea wrote:
> I'd kind of prefer this (and the other failing tests in here) be disabled, as
> they're actually timing out and each take ~1min to complete when they do.

Done.

[Nicolas Zea, 2011-08-26 22:52:19]

LGTM

[commit-bot: I haz the power, 2011-08-26 23:45:48]

Can't apply patch for file
jingle/notifier/listener/push_notifications_listen_task.cc.
While running patch -p1 --forward --force;
patching file jingle/notifier/listener/push_notifications_listen_task.cc
Hunk #2 FAILED at 47.
Hunk #3 FAILED at 86.
2 out of 3 hunks FAILED -- saving rejects to file
jingle/notifier/listener/push_notifications_listen_task.cc.rej

[commit-bot: I haz the power, 2011-08-26 23:50:38]

Can't apply patch for file
jingle/notifier/listener/push_notifications_listen_task.cc.
While running patch -p1 --forward --force;
patching file jingle/notifier/listener/push_notifications_listen_task.cc
Hunk #2 FAILED at 47.
Hunk #3 FAILED at 86.
2 out of 3 hunks FAILED -- saving rejects to file
jingle/notifier/listener/push_notifications_listen_task.cc.rej