Separate invalidator and sync client ID (part 1/2)

Separate invalidator and sync client ID (part 1/2)

This change implements support for setting the sync client ID and
invalidator client ID separately.  The two IDs are managed separately
and both of them are sent up to the server.  This change includes some
additional changes to support the transition, such as a new field in
the about:sync UI.

At this point, the IDs are not different.  Both are initialized from the
same source: the sync database.  The work to store and manage the
invalidator's ID separate from sync was begun in r180907 and will be
finished in a separate commit.  We will not be able to complete that
work until the server supports separate IDs.

BUG=124142
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=185721

[rlarocque, 2013-02-14 21:41:00]

This is a set of client-side changes that will make it possible to use different
IDs for sync and the invalidations client.  It includes protobuf changes that
will let the client specify the two IDs independently.

This change *does not* change the ID the invalidation client uses to register
itself with the invalidations server.  The invalidation client ID will continue
to match the sync client ID.  The notification client ID reported to the sync
server will also be equal to the sync client ID.

This change will allow the server team to implement and test support for the
separate IDs while maintaining compatibility with existing clients.

Raz: Please review the protobuf changes.
Nicolas: Please review the rest.

[rlarocque, 2013-02-20 22:41:36]

On 2013/02/14 21:41:00, rlarocque wrote:
> This is a set of client-side changes that will make it possible to use
different
> IDs for sync and the invalidations client.  It includes protobuf changes that
> will let the client specify the two IDs independently.
> 
> This change *does not* change the ID the invalidation client uses to register
> itself with the invalidations server.  The invalidation client ID will
continue
> to match the sync client ID.  The notification client ID reported to the sync
> server will also be equal to the sync client ID.
> 
> This change will allow the server team to implement and test support for the
> separate IDs while maintaining compatibility with existing clients.
> 
> Raz: Please review the protobuf changes.
> Nicolas: Please review the rest.

ping.

[Nicolas Zea, 2013-02-20 23:23:15]

Couple nits and a question

https://codereview.chromium.org/12256033/diff/1/sync/engine/all_status.h
File sync/engine/all_status.h (right):

https://codereview.chromium.org/12256033/diff/1/sync/engine/all_status.h#newc...
sync/engine/all_status.h:62: void SetInvalidatorClientId(const std::string&
invalidator_client_id);
nit: guid -> sync_id

https://codereview.chromium.org/12256033/diff/1/sync/protocol/sync.proto
File sync/protocol/sync.proto (right):

https://codereview.chromium.org/12256033/diff/1/sync/protocol/sync.proto#newc...
sync/protocol/sync.proto:572: optional string invalidator_client_id = 14;
should this be part of the ClientToServerMessage or the CommitMessage?

https://codereview.chromium.org/12256033/diff/1/sync/sessions/sync_session_co...
File sync/sessions/sync_session_context.h (right):

https://codereview.chromium.org/12256033/diff/1/sync/sessions/sync_session_co...
sync/sessions/sync_session_context.h:186: const std::string
invalidator_client_id_;
Comment about what why we keep this field here.

[rlarocque, 2013-02-20 23:53:32]

https://codereview.chromium.org/12256033/diff/1/sync/engine/all_status.h
File sync/engine/all_status.h (right):

https://codereview.chromium.org/12256033/diff/1/sync/engine/all_status.h#newc...
sync/engine/all_status.h:62: void SetInvalidatorClientId(const std::string&
invalidator_client_id);
On 2013/02/20 23:23:15, Nicolas Zea wrote:
> nit: guid -> sync_id

Done.

https://codereview.chromium.org/12256033/diff/1/sync/protocol/sync.proto
File sync/protocol/sync.proto (right):

https://codereview.chromium.org/12256033/diff/1/sync/protocol/sync.proto#newc...
sync/protocol/sync.proto:572: optional string invalidator_client_id = 14;
On 2013/02/20 23:23:15, Nicolas Zea wrote:
> should this be part of the ClientToServerMessage or the CommitMessage?

The server only needs access to it for commit messages, but there's no harm in
sending it up on every request.

IIRC, Raz had a slight preference for including it in all requests, but I forget
why that was.  Perhaps it had something to do with future-compatibility?

https://codereview.chromium.org/12256033/diff/1/sync/sessions/sync_session_co...
File sync/sessions/sync_session_context.h (right):

https://codereview.chromium.org/12256033/diff/1/sync/sessions/sync_session_co...
sync/sessions/sync_session_context.h:186: const std::string
invalidator_client_id_;
On 2013/02/20 23:23:15, Nicolas Zea wrote:
> Comment about what why we keep this field here.

Done.

[Nicolas Zea, 2013-02-20 23:56:40]

LGTM if server folk like it

https://codereview.chromium.org/12256033/diff/16001/sync/sessions/sync_sessio...
File sync/sessions/sync_session_context.h (right):

https://codereview.chromium.org/12256033/diff/16001/sync/sessions/sync_sessio...
sync/sessions/sync_session_context.h:186: // This is the identifier the
invalidations client used to identify itself to
used -> uses

[rlarocque, 2013-02-21 00:06:25]

https://codereview.chromium.org/12256033/diff/16001/sync/sessions/sync_sessio...
File sync/sessions/sync_session_context.h (right):

https://codereview.chromium.org/12256033/diff/16001/sync/sessions/sync_sessio...
sync/sessions/sync_session_context.h:186: // This is the identifier the
invalidations client used to identify itself to
On 2013/02/20 23:56:40, Nicolas Zea wrote:
> used -> uses

The past tense was intentional.  The term "uses" suggests that this is the
canonical version of the invalidations client ID, and that changing it would
cause the invalidations client to use a different ID.  That's not the case. 
This variable holds a copy of the ID that has already been passed to the
invalidation system.

Apparently my wording didn't have the desired effect.  Any suggestions on how I
could communicate this point more clearly?

[Nicolas Zea, 2013-02-21 00:16:58]

https://codereview.chromium.org/12256033/diff/16001/sync/sessions/sync_sessio...
File sync/sessions/sync_session_context.h (right):

https://codereview.chromium.org/12256033/diff/16001/sync/sessions/sync_sessio...
sync/sessions/sync_session_context.h:186: // This is the identifier the
invalidations client used to identify itself to
On 2013/02/21 00:06:25, rlarocque wrote:
> On 2013/02/20 23:56:40, Nicolas Zea wrote:
> > used -> uses
> 
> The past tense was intentional.  The term "uses" suggests that this is the
> canonical version of the invalidations client ID, and that changing it would
> cause the invalidations client to use a different ID.  That's not the case. 
> This variable holds a copy of the ID that has already been passed to the
> invalidation system.
> 
> Apparently my wording didn't have the desired effect.  Any suggestions on how
I
> could communicate this point more clearly?

I see. If you concern is someone modifying it, I'd just state something like:
"This is a copy of the invalidator's identifier, as determined at startup time."

[tim (not reviewing), 2013-02-21 00:20:55]

https://codereview.chromium.org/12256033/diff/16001/sync/sessions/sync_sessio...
File sync/sessions/sync_session_context.h (right):

https://codereview.chromium.org/12256033/diff/16001/sync/sessions/sync_sessio...
sync/sessions/sync_session_context.h:187: // the invalidation server.  We need
to provide this to the server when we
It's not clear what "the server" is here, either.

[rlarocque, 2013-02-21 00:31:57]

Comment updated.

[rlarocque, 2013-02-28 20:57:45]

On 2013/02/21 00:31:57, rlarocque wrote:
> Comment updated.

Ping?

I thought I had committed this, but apparently not.  Does anyone else still need
to review this?  Raz, are you OK with the protos?

I'll rebase and aim to get this committed today.

[commit-bot: I haz the power, 2013-03-01 22:16:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/12256033/25001

[commit-bot: I haz the power, 2013-03-02 16:04:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/12256033/25001

[commit-bot: I haz the power, 2013-03-02 17:32:33]

Change committed as 185721