[SYNC] Refactor SyncSourceInfo and add support in chrome invalidation client ...

[SYNC] Refactor SyncSourceInfo and add support in chrome invalidation client and syncer thread for passing a datatype-specific payload originating in the invalidation server and directed at the sync frontend server. Also fixes bug with last_sync_time and PostTimeToTypeHistogram, which would get hit when the unit tests were being run. 

BUG=68572, 69558
TEST=unit

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

[Nicolas Zea, 2011-01-11 22:15:03]

+fred for chrome invalidation code, +tim for sync internals

[tim (not reviewing), 2011-01-11 23:20:02]

LGTM with a few comments

http://codereview.chromium.org/6182004/diff/16001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_thread.cc (right):

http://codereview.chromium.org/6182004/diff/16001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread.cc:663: sync_source_types.set();
hmm... just enabled types instead?

http://codereview.chromium.org/6182004/diff/16001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_thread.h (right):

http://codereview.chromium.org/6182004/diff/16001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread.h:208: // datacenter to poll from.
re: last bit of this comment - I'd rather not tie this to the implementation on
the server... the payloads are opaque to the client.

http://codereview.chromium.org/6182004/diff/16001/chrome/browser/sync/notifie...
File chrome/browser/sync/notifier/chrome_invalidation_client.cc (right):

http://codereview.chromium.org/6182004/diff/16001/chrome/browser/sync/notifie...
chrome/browser/sync/notifier/chrome_invalidation_client.cc:133:
listener_->OnInvalidate(model_type, "");
nit - use std::string() instead of "" to save on generated code

[Nicolas Zea, 2011-01-12 00:11:37]

http://codereview.chromium.org/6182004/diff/16001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_thread.cc (right):

http://codereview.chromium.org/6182004/diff/16001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread.cc:663: sync_source_types.set();
On 2011/01/11 23:20:02, timsteele wrote:
> hmm... just enabled types instead?

Done.

http://codereview.chromium.org/6182004/diff/16001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_thread.h (right):

http://codereview.chromium.org/6182004/diff/16001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread.h:208: // datacenter to poll from.
On 2011/01/11 23:20:02, timsteele wrote:
> re: last bit of this comment - I'd rather not tie this to the implementation
on
> the server... the payloads are opaque to the client.

Done.

http://codereview.chromium.org/6182004/diff/16001/chrome/browser/sync/notifie...
File chrome/browser/sync/notifier/chrome_invalidation_client.cc (right):

http://codereview.chromium.org/6182004/diff/16001/chrome/browser/sync/notifie...
chrome/browser/sync/notifier/chrome_invalidation_client.cc:133:
listener_->OnInvalidate(model_type, "");
On 2011/01/11 23:20:02, timsteele wrote:
> nit - use std::string() instead of "" to save on generated code

Done.

[akalin, 2011-01-12 09:55:19]

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncapi.cc:2287: int milliseconds_from_now = 250;
Change to something like "const int kSyncerThreadDelayMs = 250;"

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_thread.cc (right):

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread.cc:319: const TimeTicks next_poll =
last_sync_time +
can't last_sync_time be null here to?  If this is a bug, please file a bug or
fix in another CL.

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread.cc:807: vault_.datatype_payloads_[i] =
payloads[i];  // Overwrite old payloads.
why can we get away with overwriting?  Don't we need to merge (or is it already
merged)?

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_thread.h (right):

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread.h:139: // Same as |NudgeSyncer|, but
supports tracking the datatypes that caused
I think we can combine these two Nudge functions into one and simplify the API a
bit by using a std::map<syncable::model_type, std::string>.  The keys of the map
would be the data types, and the non-zero values would be any payloads to send.

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/notifie...
File chrome/browser/sync/notifier/chrome_invalidation_client.cc (right):

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/notifie...
chrome/browser/sync/notifier/chrome_invalidation_client.cc:129: if
(invalidation.has_payload()) {
No need for the has_payload() check, assuming the default value for that
protobuf field is the empty string.  If so, you can simply do:

listener_->OnInvalidate(model_type, invalidation.payload());

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/session...
File chrome/browser/sync/sessions/sync_session.h (right):

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/session...
chrome/browser/sync/sessions/sync_session.h:42: class SyncSourceInfo {
any reason why this can't be a plain struct with public member variables?

[Nicolas Zea, 2011-01-13 19:17:30]

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncapi.cc:2287: int milliseconds_from_now = 250;
On 2011/01/12 09:55:19, akalin wrote:
> Change to something like "const int kSyncerThreadDelayMs = 250;"

Done.

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_thread.cc (right):

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread.cc:319: const TimeTicks next_poll =
last_sync_time +
On 2011/01/12 09:55:19, akalin wrote:
> can't last_sync_time be null here to?  If this is a bug, please file a bug or
> fix in another CL.

Filed bug 69558 regarding last_sync_time and PostTimeToTypeHistogram. next_poll
was not affected, although Tim would better be able to say if perhaps we're
doing our first poll earlier than we should be (and if that matters)? I believe
that's a separate issue though.

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread.cc:807: vault_.datatype_payloads_[i] =
payloads[i];  // Overwrite old payloads.
On 2011/01/12 09:55:19, akalin wrote:
> why can we get away with overwriting?  Don't we need to merge (or is it
already
> merged)?

We merge across datatypes, but if we receive a payload for datatype x at time 1,
then a different payload for datatype x at time 2, we're only concerned about
the most recent version of datatype x, which is based on the most recent
payload. Feel free to correct if my logic is wrong.

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_thread.h (right):

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread.h:139: // Same as |NudgeSyncer|, but
supports tracking the datatypes that caused
My only hesitancy with this is that it then fully couples the notion of payloads
with a nudge. At the moment, nudges originating from internal causes (local
changes, etc.) don't need payloads and so use a method that doesn't involve
payloads.

On the other hand, nudges with external origin, which is where payloads come
into play, have their own nudge method.

So in my mind it makes sense to have two separate methods, due to their
different origins. Perhaps I should add some comments to that effect though.

On 2011/01/12 09:55:19, akalin wrote:
> I think we can combine these two Nudge functions into one and simplify the API
a
> bit by using a std::map<syncable::model_type, std::string>.  The keys of the
map
> would be the data types, and the non-zero values would be any payloads to
send.

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/notifie...
File chrome/browser/sync/notifier/chrome_invalidation_client.cc (right):

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/notifie...
chrome/browser/sync/notifier/chrome_invalidation_client.cc:129: if
(invalidation.has_payload()) {
The has_payload is method isn't a protobuf defined one, but from
invalidation-types.h. payload() has a CHECK() to ensure the payload exists.

On 2011/01/12 09:55:19, akalin wrote:
> No need for the has_payload() check, assuming the default value for that
> protobuf field is the empty string.  If so, you can simply do:
> 
> listener_->OnInvalidate(model_type, invalidation.payload());

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/session...
File chrome/browser/sync/sessions/sync_session.h (right):

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/session...
chrome/browser/sync/sessions/sync_session.h:42: class SyncSourceInfo {
On 2011/01/12 09:55:19, akalin wrote:
> any reason why this can't be a plain struct with public member variables?

It seemed cleaner to me since the private members here are never updated outside
of construction time (once a SyncSourceInfo is made, it's effectively a
constant), which I think fit better with Tim's SyncSession lockbox approach..

[akalin, 2011-01-13 19:47:40]

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_thread.cc (right):

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread.cc:807: vault_.datatype_payloads_[i] =
payloads[i];  // Overwrite old payloads.
On 2011/01/13 19:17:30, nzea wrote:
> On 2011/01/12 09:55:19, akalin wrote:
> > why can we get away with overwriting?  Don't we need to merge (or is it
> already
> > merged)?
> 
> We merge across datatypes, but if we receive a payload for datatype x at time
1,
> then a different payload for datatype x at time 2, we're only concerned about
> the most recent version of datatype x, which is based on the most recent
> payload. Feel free to correct if my logic is wrong.
> 

Okay, if that's the case, maybe expand the comment, like "// Overwrite old
payloads as we only need to send the most recent one."

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_thread.h (right):

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread.h:139: // Same as |NudgeSyncer|, but
supports tracking the datatypes that caused
On 2011/01/13 19:17:30, nzea wrote:
> My only hesitancy with this is that it then fully couples the notion of
payloads
> with a nudge. At the moment, nudges originating from internal causes (local
> changes, etc.) don't need payloads and so use a method that doesn't involve
> payloads.
> 
> On the other hand, nudges with external origin, which is where payloads come
> into play, have their own nudge method.
> 
> So in my mind it makes sense to have two separate methods, due to their
> different origins. Perhaps I should add some comments to that effect though.

Fair point.  I'm fine with two different methods.  I'd like the
nudge-with-payloads method to use a map<> though, as it makes the code more
understandable.

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/notifie...
File chrome/browser/sync/notifier/chrome_invalidation_client.cc (right):

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/notifie...
chrome/browser/sync/notifier/chrome_invalidation_client.cc:129: if
(invalidation.has_payload()) {
On 2011/01/13 19:17:30, nzea wrote:
> The has_payload is method isn't a protobuf defined one, but from
> invalidation-types.h. payload() has a CHECK() to ensure the payload exists.
> 
> On 2011/01/12 09:55:19, akalin wrote:
> > No need for the has_payload() check, assuming the default value for that
> > protobuf field is the empty string.  If so, you can simply do:
> > 
> > listener_->OnInvalidate(model_type, invalidation.payload());
> 

Ah, I forgot about that.  How about:

std::string payload;
// payload() CHECKs has_payload() so we need to check for it ourselves.
if (...has_payload()) {
  payload = invalidation.payload();
}
listener_->OnInvalidate(..., payload);

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/session...
File chrome/browser/sync/sessions/sync_session.h (right):

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/session...
chrome/browser/sync/sessions/sync_session.h:42: class SyncSourceInfo {
On 2011/01/13 19:17:30, nzea wrote:
> On 2011/01/12 09:55:19, akalin wrote:
> > any reason why this can't be a plain struct with public member variables?
> 
> It seemed cleaner to me since the private members here are never updated
outside
> of construction time (once a SyncSourceInfo is made, it's effectively a
> constant), which I think fit better with Tim's SyncSession lockbox approach..

In that case, what about a struct with const members and constructors?  You get
the same effect but less accessor boilerplate.

[Nicolas Zea, 2011-01-18 06:07:23]

Changes made based on feedback. Sorry about the multiple uploads, I forgot I
didn't have to upload in order to issue a try.

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_thread.cc (right):

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread.cc:807: vault_.datatype_payloads_[i] =
payloads[i];  // Overwrite old payloads.
On 2011/01/13 19:47:41, akalin wrote:
> On 2011/01/13 19:17:30, nzea wrote:
> > On 2011/01/12 09:55:19, akalin wrote:
> > > why can we get away with overwriting?  Don't we need to merge (or is it
> > already
> > > merged)?
> > 
> > We merge across datatypes, but if we receive a payload for datatype x at
time
> 1,
> > then a different payload for datatype x at time 2, we're only concerned
about
> > the most recent version of datatype x, which is based on the most recent
> > payload. Feel free to correct if my logic is wrong.
> > 
> 
> Okay, if that's the case, maybe expand the comment, like "// Overwrite old
> payloads as we only need to send the most recent one."

Done.

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_thread.h (right):

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread.h:139: // Same as |NudgeSyncer|, but
supports tracking the datatypes that caused
On 2011/01/13 19:47:41, akalin wrote:
> On 2011/01/13 19:17:30, nzea wrote:
> > My only hesitancy with this is that it then fully couples the notion of
> payloads
> > with a nudge. At the moment, nudges originating from internal causes (local
> > changes, etc.) don't need payloads and so use a method that doesn't involve
> > payloads.
> > 
> > On the other hand, nudges with external origin, which is where payloads come
> > into play, have their own nudge method.
> > 
> > So in my mind it makes sense to have two separate methods, due to their
> > different origins. Perhaps I should add some comments to that effect though.
> 
> Fair point.  I'm fine with two different methods.  I'd like the
> nudge-with-payloads method to use a map<> though, as it makes the code more
> understandable.

Done.

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/notifie...
File chrome/browser/sync/notifier/chrome_invalidation_client.cc (right):

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/notifie...
chrome/browser/sync/notifier/chrome_invalidation_client.cc:129: if
(invalidation.has_payload()) {
On 2011/01/13 19:47:41, akalin wrote:
> On 2011/01/13 19:17:30, nzea wrote:
> > The has_payload is method isn't a protobuf defined one, but from
> > invalidation-types.h. payload() has a CHECK() to ensure the payload exists.
> > 
> > On 2011/01/12 09:55:19, akalin wrote:
> > > No need for the has_payload() check, assuming the default value for that
> > > protobuf field is the empty string.  If so, you can simply do:
> > > 
> > > listener_->OnInvalidate(model_type, invalidation.payload());
> > 
> 
> Ah, I forgot about that.  How about:
> 
> std::string payload;
> // payload() CHECKs has_payload() so we need to check for it ourselves.
> if (...has_payload()) {
>   payload = invalidation.payload();
> }
> listener_->OnInvalidate(..., payload);

Done.

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/session...
File chrome/browser/sync/sessions/sync_session.h (right):

http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/session...
chrome/browser/sync/sessions/sync_session.h:42: class SyncSourceInfo {
I went ahead and just switched to a normal struct. Having const member caused
problems with how the object initialized and passed around.

[Nicolas Zea, 2011-01-19 18:24:50]

Fred: Ping.

On 2011/01/18 06:07:23, nzea wrote:
> Changes made based on feedback. Sorry about the multiple uploads, I forgot I
> didn't have to upload in order to issue a try.
> 
>
http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/engine/...
> File chrome/browser/sync/engine/syncer_thread.cc (right):
> 
>
http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/engine/...
> chrome/browser/sync/engine/syncer_thread.cc:807: vault_.datatype_payloads_[i]
=
> payloads[i];  // Overwrite old payloads.
> On 2011/01/13 19:47:41, akalin wrote:
> > On 2011/01/13 19:17:30, nzea wrote:
> > > On 2011/01/12 09:55:19, akalin wrote:
> > > > why can we get away with overwriting?  Don't we need to merge (or is it
> > > already
> > > > merged)?
> > > 
> > > We merge across datatypes, but if we receive a payload for datatype x at
> time
> > 1,
> > > then a different payload for datatype x at time 2, we're only concerned
> about
> > > the most recent version of datatype x, which is based on the most recent
> > > payload. Feel free to correct if my logic is wrong.
> > > 
> > 
> > Okay, if that's the case, maybe expand the comment, like "// Overwrite old
> > payloads as we only need to send the most recent one."
> 
> Done.
> 
>
http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/engine/...
> File chrome/browser/sync/engine/syncer_thread.h (right):
> 
>
http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/engine/...
> chrome/browser/sync/engine/syncer_thread.h:139: // Same as |NudgeSyncer|, but
> supports tracking the datatypes that caused
> On 2011/01/13 19:47:41, akalin wrote:
> > On 2011/01/13 19:17:30, nzea wrote:
> > > My only hesitancy with this is that it then fully couples the notion of
> > payloads
> > > with a nudge. At the moment, nudges originating from internal causes
(local
> > > changes, etc.) don't need payloads and so use a method that doesn't
involve
> > > payloads.
> > > 
> > > On the other hand, nudges with external origin, which is where payloads
come
> > > into play, have their own nudge method.
> > > 
> > > So in my mind it makes sense to have two separate methods, due to their
> > > different origins. Perhaps I should add some comments to that effect
though.
> > 
> > Fair point.  I'm fine with two different methods.  I'd like the
> > nudge-with-payloads method to use a map<> though, as it makes the code more
> > understandable.
> 
> Done.
> 
>
http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/notifie...
> File chrome/browser/sync/notifier/chrome_invalidation_client.cc (right):
> 
>
http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/notifie...
> chrome/browser/sync/notifier/chrome_invalidation_client.cc:129: if
> (invalidation.has_payload()) {
> On 2011/01/13 19:47:41, akalin wrote:
> > On 2011/01/13 19:17:30, nzea wrote:
> > > The has_payload is method isn't a protobuf defined one, but from
> > > invalidation-types.h. payload() has a CHECK() to ensure the payload
exists.
> > > 
> > > On 2011/01/12 09:55:19, akalin wrote:
> > > > No need for the has_payload() check, assuming the default value for that
> > > > protobuf field is the empty string.  If so, you can simply do:
> > > > 
> > > > listener_->OnInvalidate(model_type, invalidation.payload());
> > > 
> > 
> > Ah, I forgot about that.  How about:
> > 
> > std::string payload;
> > // payload() CHECKs has_payload() so we need to check for it ourselves.
> > if (...has_payload()) {
> >   payload = invalidation.payload();
> > }
> > listener_->OnInvalidate(..., payload);
> 
> Done.
> 
>
http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/session...
> File chrome/browser/sync/sessions/sync_session.h (right):
> 
>
http://codereview.chromium.org/6182004/diff/32001/chrome/browser/sync/session...
> chrome/browser/sync/sessions/sync_session.h:42: class SyncSourceInfo {
> I went ahead and just switched to a normal struct. Having const member caused
> problems with how the object initialized and passed around.

[akalin, 2011-01-19 22:14:52]

http://codereview.chromium.org/6182004/diff/51003/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/download_updates_command.cc (left):

http://codereview.chromium.org/6182004/diff/51003/chrome/browser/sync/engine/...
chrome/browser/sync/engine/download_updates_command.cc:63:
get_updates->mutable_caller_info()->set_source(
Add TODO here talking about how we need to actually send the data to the server.
 Also, save the return value from session->TestAndSetSource() since it clobbers
the existing one. (Actually, we may need to change that method to not clobber
the payloads -- not sure how we should handle this)

http://codereview.chromium.org/6182004/diff/51003/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncapi.cc (left):

http://codereview.chromium.org/6182004/diff/51003/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncapi.cc:2203: VLOG(1) << "Sync received server
notification: " <<
Save .service_url and .service_specific_data into local variables with more
accurate names and refer to the docs in server_notifier_thread.h.

http://codereview.chromium.org/6182004/diff/51003/chrome/browser/sync/notifie...
File chrome/browser/sync/notifier/server_notifier_thread.cc (right):

http://codereview.chromium.org/6182004/diff/51003/chrome/browser/sync/notifie...
chrome/browser/sync/notifier/server_notifier_thread.cc:78:
notification_data.service_url = model_types.to_string();
You should document the 'protocol' we're using, i.e. what data we stuff into
which strings, perhaps in the .h file.

http://codereview.chromium.org/6182004/diff/51003/chrome/browser/sync/session...
File chrome/browser/sync/sessions/sync_session.h (right):

http://codereview.chromium.org/6182004/diff/51003/chrome/browser/sync/session...
chrome/browser/sync/sessions/sync_session.h:56: syncable::ModelTypeBitSet types;
As discussed, we can combine types and payloads into a map : type -> payload.

http://codereview.chromium.org/6182004/diff/138001/chrome/browser/sync/engine...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/6182004/diff/138001/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncapi.cc:2216: if
(!notification_data.service_specific_data.empty()) {
no need for the empty check -- we can just always set the keys of payloads.

http://codereview.chromium.org/6182004/diff/138001/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncapi.cc:2242: // If this notification had a
payload, pass it along, else just pass the
Per the above, no need for this check either -- just call
NudgeSyncerWithPayloads.

http://codereview.chromium.org/6182004/diff/138001/chrome/browser/sync/engine...
File chrome/browser/sync/engine/syncer_thread.h (right):

http://codereview.chromium.org/6182004/diff/138001/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncer_thread.h:142: virtual void
NudgeSyncerWithDataTypes(
Is syncapi.cc the only place we called NudgeSyncerWithDataTypes?  Per the
comments there, if that's the only place, then we don't need this one after all.

http://codereview.chromium.org/6182004/diff/138001/chrome/browser/sync/tools/...
File chrome/browser/sync/tools/sync_listen_notifications.cc (left):

http://codereview.chromium.org/6182004/diff/138001/chrome/browser/sync/tools/...
chrome/browser/sync/tools/sync_listen_notifications.cc:158: <<
syncable::ModelTypeToString(model_type);
print payload here, too (do we know if it's a binary or a text string?) if
binary, base64-encode it before printing.

[Nicolas Zea, 2011-01-19 22:54:55]

Almost there!

http://codereview.chromium.org/6182004/diff/51003/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/download_updates_command.cc (left):

http://codereview.chromium.org/6182004/diff/51003/chrome/browser/sync/engine/...
chrome/browser/sync/engine/download_updates_command.cc:63:
get_updates->mutable_caller_info()->set_source(
On 2011/01/19 22:14:52, akalin wrote:
> Add TODO here talking about how we need to actually send the data to the
server.
>  Also, save the return value from session->TestAndSetSource() since it
clobbers
> the existing one. (Actually, we may need to change that method to not clobber
> the payloads -- not sure how we should handle this)

Done.

http://codereview.chromium.org/6182004/diff/51003/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncapi.cc (left):

http://codereview.chromium.org/6182004/diff/51003/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncapi.cc:2203: VLOG(1) << "Sync received server
notification: " <<
On 2011/01/19 22:14:52, akalin wrote:
> Save .service_url and .service_specific_data into local variables with more
> accurate names and refer to the docs in server_notifier_thread.h.

Done.

http://codereview.chromium.org/6182004/diff/51003/chrome/browser/sync/notifie...
File chrome/browser/sync/notifier/server_notifier_thread.cc (right):

http://codereview.chromium.org/6182004/diff/51003/chrome/browser/sync/notifie...
chrome/browser/sync/notifier/server_notifier_thread.cc:78:
notification_data.service_url = model_types.to_string();
On 2011/01/19 22:14:52, akalin wrote:
> You should document the 'protocol' we're using, i.e. what data we stuff into
> which strings, perhaps in the .h file.

Done.

http://codereview.chromium.org/6182004/diff/51003/chrome/browser/sync/session...
File chrome/browser/sync/sessions/sync_session.h (right):

http://codereview.chromium.org/6182004/diff/51003/chrome/browser/sync/session...
chrome/browser/sync/sessions/sync_session.h:56: syncable::ModelTypeBitSet types;
On 2011/01/19 22:14:52, akalin wrote:
> As discussed, we can combine types and payloads into a map : type -> payload.

Done.

http://codereview.chromium.org/6182004/diff/138001/chrome/browser/sync/engine...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/6182004/diff/138001/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncapi.cc:2216: if
(!notification_data.service_specific_data.empty()) {
On 2011/01/19 22:14:52, akalin wrote:
> no need for the empty check -- we can just always set the keys of payloads.

Done.

http://codereview.chromium.org/6182004/diff/138001/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncapi.cc:2242: // If this notification had a
payload, pass it along, else just pass the
On 2011/01/19 22:14:52, akalin wrote:
> Per the above, no need for this check either -- just call
> NudgeSyncerWithPayloads.

Done.

http://codereview.chromium.org/6182004/diff/138001/chrome/browser/sync/engine...
File chrome/browser/sync/engine/syncer_thread.h (right):

http://codereview.chromium.org/6182004/diff/138001/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncer_thread.h:142: virtual void
NudgeSyncerWithDataTypes(
On 2011/01/19 22:14:52, akalin wrote:
> Is syncapi.cc the only place we called NudgeSyncerWithDataTypes?  Per the
> comments there, if that's the only place, then we don't need this one after
all.

It is, but it's also called from syncapi's
HandleCalculateChangesChangeEventFromSyncApi, where no payloads are used.

http://codereview.chromium.org/6182004/diff/138001/chrome/browser/sync/tools/...
File chrome/browser/sync/tools/sync_listen_notifications.cc (left):

http://codereview.chromium.org/6182004/diff/138001/chrome/browser/sync/tools/...
chrome/browser/sync/tools/sync_listen_notifications.cc:158: <<
syncable::ModelTypeToString(model_type);
Done. Raz only told me it's opaque, so I'll print it as a string for now (should
be easier for testing anyways).

On 2011/01/19 22:14:52, akalin wrote:
> print payload here, too (do we know if it's a binary or a text string?) if
> binary, base64-encode it before printing.

[akalin, 2011-01-20 00:28:19]

http://codereview.chromium.org/6182004/diff/95003/chrome/browser/sync/session...
File chrome/browser/sync/sessions/sync_session.h (right):

http://codereview.chromium.org/6182004/diff/95003/chrome/browser/sync/session...
chrome/browser/sync/sessions/sync_session.h:57: syncable::ModelTypeBitSet types;
So since we have the payloads map, whose keys are the set of types we notify
for, isn't this BitSet unneeded?  Basically we should have a map everywhere we
used to have a ModelTypeBitSet.

[Nicolas Zea, 2011-01-20 02:17:32]

Decent amount rewritten this time. I feel close now!

http://codereview.chromium.org/6182004/diff/95003/chrome/browser/sync/session...
File chrome/browser/sync/sessions/sync_session.h (right):

http://codereview.chromium.org/6182004/diff/95003/chrome/browser/sync/session...
chrome/browser/sync/sessions/sync_session.h:57: syncable::ModelTypeBitSet types;
Again, correct :-/  (as long as we ensure blank payloads don't overwrite real
payloads). Done.

On 2011/01/20 00:28:19, akalin wrote:
> So since we have the payloads map, whose keys are the set of types we notify
> for, isn't this BitSet unneeded?  Basically we should have a map everywhere we
> used to have a ModelTypeBitSet.

[akalin, 2011-01-21 19:50:09]

Getting closer!  Mostly nits and requests for utility functions.

http://codereview.chromium.org/6182004/diff/107021/chrome/browser/sync/engine...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/6182004/diff/107021/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncapi.cc:2218: for (size_t i =
syncable::FIRST_REAL_MODEL_TYPE;
I think there's room for a utility function that converts a ModelTypeBitSet +
payload string to a map, since you do the same for loop a bunch of times.  Not
sure where to put it though; i'll leave that to you.

http://codereview.chromium.org/6182004/diff/107021/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncapi.cc:2226: (notification_data.service_url ==
revert indentation?

http://codereview.chromium.org/6182004/diff/107021/chrome/browser/sync/engine...
File chrome/browser/sync/engine/syncer_thread.cc (right):

http://codereview.chromium.org/6182004/diff/107021/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncer_thread.cc:11: #include <vector>
no need for this anymore

http://codereview.chromium.org/6182004/diff/107021/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncer_thread.cc:80: SyncSourceInfo::ModelTypeMap
model_types_with_payloads;
Could use utility function here, too

http://codereview.chromium.org/6182004/diff/107021/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncer_thread.cc:86: std::string("");
can be just std::string()

http://codereview.chromium.org/6182004/diff/107021/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncer_thread.cc:104: for
(ModelSafeRoutingInfo::const_iterator i = routes.begin();
Room for another util. function to go from ModelSafeRoutingInfo to ModelTypeMap?
 You do it twice in this file.

http://codereview.chromium.org/6182004/diff/107021/chrome/browser/sync/engine...
File chrome/browser/sync/engine/syncer_thread.h (right):

http://codereview.chromium.org/6182004/diff/107021/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncer_thread.h:13: #include <map>
No need for this since you're using the typedef from SyncSessions.

http://codereview.chromium.org/6182004/diff/107021/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncer_thread.h:299: const
sessions::SyncSourceInfo::ModelTypeMap& nudge_types,
rename to model_types_with_payloads to be consistent?

http://codereview.chromium.org/6182004/diff/107021/chrome/browser/sync/engine...
File chrome/browser/sync/engine/syncer_thread_unittest.cc (right):

http://codereview.chromium.org/6182004/diff/107021/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncer_thread_unittest.cc:178: for (size_t i =
syncable::FIRST_REAL_MODEL_TYPE;
I'm pretty sure a std::map already has operator== defined, which should work as
expected here.

http://codereview.chromium.org/6182004/diff/107021/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncer_thread_unittest.cc:197: bool
CompareNudgeTypesBitSetToVault(const syncable::ModelTypeBitSet& lhs) {
If you write the utility f'n to compare ModelTypeBitSet to ModelTypeMap, you can
invoke that and then compare the two ModelTypeMaps with operator==.

[Nicolas Zea, 2011-01-21 21:57:44]

Refactored to use util methods where necessary and addressed comments.

http://codereview.chromium.org/6182004/diff/107021/chrome/browser/sync/engine...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/6182004/diff/107021/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncapi.cc:2218: for (size_t i =
syncable::FIRST_REAL_MODEL_TYPE;
On 2011/01/21 19:50:09, akalin wrote:
> I think there's room for a utility function that converts a ModelTypeBitSet +
> payload string to a map, since you do the same for loop a bunch of times.  Not
> sure where to put it though; i'll leave that to you.

Done.

http://codereview.chromium.org/6182004/diff/107021/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncapi.cc:2226: (notification_data.service_url ==
On 2011/01/21 19:50:09, akalin wrote:
> revert indentation?

Done.

http://codereview.chromium.org/6182004/diff/107021/chrome/browser/sync/engine...
File chrome/browser/sync/engine/syncer_thread.cc (right):

http://codereview.chromium.org/6182004/diff/107021/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncer_thread.cc:11: #include <vector>
There is still use of vector for the model type worker. Removed map though.

On 2011/01/21 19:50:09, akalin wrote:
> no need for this anymore

http://codereview.chromium.org/6182004/diff/107021/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncer_thread.cc:80: SyncSourceInfo::ModelTypeMap
model_types_with_payloads;
On 2011/01/21 19:50:09, akalin wrote:
> Could use utility function here, too

Done.

http://codereview.chromium.org/6182004/diff/107021/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncer_thread.cc:86: std::string("");
On 2011/01/21 19:50:09, akalin wrote:
> can be just std::string()

Done.

http://codereview.chromium.org/6182004/diff/107021/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncer_thread.cc:104: for
(ModelSafeRoutingInfo::const_iterator i = routes.begin();
On 2011/01/21 19:50:09, akalin wrote:
> Room for another util. function to go from ModelSafeRoutingInfo to
ModelTypeMap?
>  You do it twice in this file.

Done.

http://codereview.chromium.org/6182004/diff/107021/chrome/browser/sync/engine...
File chrome/browser/sync/engine/syncer_thread.h (right):

http://codereview.chromium.org/6182004/diff/107021/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncer_thread.h:13: #include <map>
On 2011/01/21 19:50:09, akalin wrote:
> No need for this since you're using the typedef from SyncSessions.

Done.

http://codereview.chromium.org/6182004/diff/107021/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncer_thread.h:299: const
sessions::SyncSourceInfo::ModelTypeMap& nudge_types,
On 2011/01/21 19:50:09, akalin wrote:
> rename to model_types_with_payloads to be consistent?

Done.

http://codereview.chromium.org/6182004/diff/107021/chrome/browser/sync/engine...
File chrome/browser/sync/engine/syncer_thread_unittest.cc (right):

http://codereview.chromium.org/6182004/diff/107021/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncer_thread_unittest.cc:178: for (size_t i =
syncable::FIRST_REAL_MODEL_TYPE;
On 2011/01/21 19:50:09, akalin wrote:
> I'm pretty sure a std::map already has operator== defined, which should work
as
> expected here.

Done.

http://codereview.chromium.org/6182004/diff/107021/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncer_thread_unittest.cc:197: bool
CompareNudgeTypesBitSetToVault(const syncable::ModelTypeBitSet& lhs) {
On 2011/01/21 19:50:09, akalin wrote:
> If you write the utility f'n to compare ModelTypeBitSet to ModelTypeMap, you
can
> invoke that and then compare the two ModelTypeMaps with operator==.

Done.

[akalin, 2011-01-24 22:17:28]

Almost there!

http://codereview.chromium.org/6182004/diff/152002/chrome/browser/sync/engine...
File chrome/browser/sync/engine/syncer_thread.cc (right):

http://codereview.chromium.org/6182004/diff/152002/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncer_thread.cc:795: for
(ModelTypeMap::const_iterator i =
I think this logic is tricky enough that it should be its own utility function
in sync_session.h and have some unit tests.  Something like CoalescePayloads()?

May as well add unit tests for the other utility functions, too.

http://codereview.chromium.org/6182004/diff/152002/chrome/browser/sync/sessio...
File chrome/browser/sync/sessions/sync_session.h (right):

http://codereview.chromium.org/6182004/diff/152002/chrome/browser/sync/sessio...
chrome/browser/sync/sessions/sync_session.h:26: #include
"chrome/browser/sync/engine/model_safe_worker.h"
I think you can forward declare ModelSafeRoutingInfo here and move this include
to the .cc file.

http://codereview.chromium.org/6182004/diff/152002/chrome/browser/sync/sessio...
chrome/browser/sync/sessions/sync_session.h:44: typedef
std::map<syncable::ModelType, std::string> ModelTypeMap;
ModelTypeMap is a bit generic -- how about ModelTypePayloadMap or
TypePayloadMap?

http://codereview.chromium.org/6182004/diff/152002/chrome/browser/sync/sessio...
chrome/browser/sync/sessions/sync_session.h:47: void
BuildModelTypeMapFromModelTypeBitSet(
These names are a bit long.  I suggest:

ModelTypeBitSetToTypePayloadMap()
RoutingInfoToTypePayloadMap()

Also, I think it's safer to make it return the TypePayloadMap directly.

http://codereview.chromium.org/6182004/diff/152002/chrome/test/sync/engine/sy...
File chrome/test/sync/engine/syncer_command_test.h (right):

http://codereview.chromium.org/6182004/diff/152002/chrome/test/sync/engine/sy...
chrome/test/sync/engine/syncer_command_test.h:95: std::string(),
indentation

[Nicolas Zea, 2011-01-25 00:29:13]

Done and done.

http://codereview.chromium.org/6182004/diff/152002/chrome/browser/sync/engine...
File chrome/browser/sync/engine/syncer_thread.cc (right):

http://codereview.chromium.org/6182004/diff/152002/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncer_thread.cc:795: for
(ModelTypeMap::const_iterator i =
On 2011/01/24 22:17:29, akalin wrote:
> I think this logic is tricky enough that it should be its own utility function
> in sync_session.h and have some unit tests.  Something like
CoalescePayloads()?
> 
> May as well add unit tests for the other utility functions, too.

Done.

http://codereview.chromium.org/6182004/diff/152002/chrome/browser/sync/sessio...
File chrome/browser/sync/sessions/sync_session.h (right):

http://codereview.chromium.org/6182004/diff/152002/chrome/browser/sync/sessio...
chrome/browser/sync/sessions/sync_session.h:26: #include
"chrome/browser/sync/engine/model_safe_worker.h"
ModelSafeRoutingInfo is a typedef, so can't be forward declared (unless i
redefine the typedef here itself, but that seems to defeat the purpose).
Anyways, there were other uses of the type in here, so I just turned it back to
how it was.

http://codereview.chromium.org/6182004/diff/152002/chrome/browser/sync/sessio...
chrome/browser/sync/sessions/sync_session.h:44: typedef
std::map<syncable::ModelType, std::string> ModelTypeMap;
On 2011/01/24 22:17:29, akalin wrote:
> ModelTypeMap is a bit generic -- how about ModelTypePayloadMap or
> TypePayloadMap?

Done.

http://codereview.chromium.org/6182004/diff/152002/chrome/browser/sync/sessio...
chrome/browser/sync/sessions/sync_session.h:47: void
BuildModelTypeMapFromModelTypeBitSet(
On 2011/01/24 22:17:29, akalin wrote:
> These names are a bit long.  I suggest:
> 
> ModelTypeBitSetToTypePayloadMap()
> RoutingInfoToTypePayloadMap()
> 
> Also, I think it's safer to make it return the TypePayloadMap directly.

Done.

http://codereview.chromium.org/6182004/diff/152002/chrome/test/sync/engine/sy...
File chrome/test/sync/engine/syncer_command_test.h (right):

http://codereview.chromium.org/6182004/diff/152002/chrome/test/sync/engine/sy...
chrome/test/sync/engine/syncer_command_test.h:95: std::string(),
On 2011/01/24 22:17:29, akalin wrote:
> indentation

Done.

[akalin, 2011-01-26 01:15:17]

Almost almost there!

http://codereview.chromium.org/6182004/diff/152002/chrome/browser/sync/sessio...
File chrome/browser/sync/sessions/sync_session.h (right):

http://codereview.chromium.org/6182004/diff/152002/chrome/browser/sync/sessio...
chrome/browser/sync/sessions/sync_session.h:26: #include
"chrome/browser/sync/engine/model_safe_worker.h"
On 2011/01/25 00:29:14, nzea wrote:
> ModelSafeRoutingInfo is a typedef, so can't be forward declared (unless i
> redefine the typedef here itself, but that seems to defeat the purpose).
> Anyways, there were other uses of the type in here, so I just turned it back
to
> how it was.

Ah, actually if it's a typedef, then we should include model_safe_worker.h here.

http://codereview.chromium.org/6182004/diff/165004/chrome/browser/sync/engine...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/6182004/diff/165004/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncapi.cc:2229: model_types.set();
This should probably set only enabled types, i.e. read from the
ModelSafeRoutingInfo.  Feel free to just add a TODO here if it's not easy to get
the routing info.

http://codereview.chromium.org/6182004/diff/165004/chrome/browser/sync/sessio...
File chrome/browser/sync/sessions/sync_session.cc (right):

http://codereview.chromium.org/6182004/diff/165004/chrome/browser/sync/sessio...
chrome/browser/sync/sessions/sync_session.cc:5: #include
"chrome/browser/sync/engine/model_safe_worker.h"
move this to the .h file

http://codereview.chromium.org/6182004/diff/165004/chrome/browser/sync/sessio...
chrome/browser/sync/sessions/sync_session.cc:19: ++i) {
this can go on previous line

http://codereview.chromium.org/6182004/diff/165004/chrome/browser/sync/sessio...
chrome/browser/sync/sessions/sync_session.cc:32: ++i) {
so can this

http://codereview.chromium.org/6182004/diff/165004/chrome/browser/sync/sessio...
File chrome/browser/sync/sessions/sync_session.h (right):

http://codereview.chromium.org/6182004/diff/165004/chrome/browser/sync/sessio...
chrome/browser/sync/sessions/sync_session.h:54: // Coalesce |update| into
|original|, overwriting only when |update| has
I think it's safer to change the sig of this into:

void CoalescePayloads(TypePayloadMap*, const TypePayloadMap&);

and modify the first arg in-place, as then there's less danger of switching the
arguments accidentally.  This is semantically a |= operation, anyway.

[Nicolas Zea, 2011-01-26 01:41:28]

So close.

http://codereview.chromium.org/6182004/diff/165004/chrome/browser/sync/engine...
File chrome/browser/sync/engine/syncapi.cc (right):

http://codereview.chromium.org/6182004/diff/165004/chrome/browser/sync/engine...
chrome/browser/sync/engine/syncapi.cc:2229: model_types.set();
On 2011/01/26 01:15:17, akalin wrote:
> This should probably set only enabled types, i.e. read from the
> ModelSafeRoutingInfo.  Feel free to just add a TODO here if it's not easy to
get
> the routing info.

Done.

http://codereview.chromium.org/6182004/diff/165004/chrome/browser/sync/sessio...
File chrome/browser/sync/sessions/sync_session.cc (right):

http://codereview.chromium.org/6182004/diff/165004/chrome/browser/sync/sessio...
chrome/browser/sync/sessions/sync_session.cc:5: #include
"chrome/browser/sync/engine/model_safe_worker.h"
On 2011/01/26 01:15:17, akalin wrote:
> move this to the .h file

Done.

http://codereview.chromium.org/6182004/diff/165004/chrome/browser/sync/sessio...
chrome/browser/sync/sessions/sync_session.cc:19: ++i) {
On 2011/01/26 01:15:17, akalin wrote:
> this can go on previous line

Done.

http://codereview.chromium.org/6182004/diff/165004/chrome/browser/sync/sessio...
chrome/browser/sync/sessions/sync_session.cc:32: ++i) {
On 2011/01/26 01:15:17, akalin wrote:
> so can this

Done.

http://codereview.chromium.org/6182004/diff/165004/chrome/browser/sync/sessio...
File chrome/browser/sync/sessions/sync_session.h (right):

http://codereview.chromium.org/6182004/diff/165004/chrome/browser/sync/sessio...
chrome/browser/sync/sessions/sync_session.h:54: // Coalesce |update| into
|original|, overwriting only when |update| has
On 2011/01/26 01:15:17, akalin wrote:
> I think it's safer to change the sig of this into:
> 
> void CoalescePayloads(TypePayloadMap*, const TypePayloadMap&);
> 
> and modify the first arg in-place, as then there's less danger of switching
the
> arguments accidentally.  This is semantically a |= operation, anyway.

Done.

[akalin, 2011-01-26 10:35:26]

LGTM after fixing the nits below.

http://codereview.chromium.org/6182004/diff/69003/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/syncer_thread.cc (right):

http://codereview.chromium.org/6182004/diff/69003/chrome/browser/sync/engine/...
chrome/browser/sync/engine/syncer_thread.cc:35: using sessions::TypePayloadMap;
alphabetize

http://codereview.chromium.org/6182004/diff/69003/chrome/browser/sync/session...
File chrome/browser/sync/sessions/sync_session_unittest.cc (right):

http://codereview.chromium.org/6182004/diff/69003/chrome/browser/sync/session...
chrome/browser/sync/sessions/sync_session_unittest.cc:257: TypePayloadMap map =
ModelTypeBitSetToTypePayloadMap(types, payload);
Best not to name a variable the same as an existing type, at least in C++.  I
think you use 'map' in other places, too.

http://codereview.chromium.org/6182004/diff/69003/chrome/browser/sync/session...
chrome/browser/sync/sessions/sync_session_unittest.cc:267:
EXPECT_TRUE(map[syncable::BOOKMARKS] == payload);
use EXPECT_EQ