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

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

In r180907, we started saving the sync ID to a pref.  By now, many
clients will have exercised that code path and copied their sync ID
into that preference.  Prior to this commit, we never read from that
preference.  Most pre-existing clients will now have two copies of
their sync ID: one in the sync DB, and one in their preferences.

Meanwhile, in r185721 (part 1 of this set of patches), we extended the
protocol to support separate notifier and sync client IDs.  Prior to
that patch, we always used a single ID for both the invalidation client
and sync client.  In that patch, we made sure that the value of both IDs
were identical.

This commit takes advantage of that prior work to create an invalidator
ID that is separate from the sync ID.  It's important that the
invalidator client ID never change once it has been initialized, so this
commit tries to use the old client ID (which is equal to the sync ID at
the time of this migration) on existing clients.  New clients have no
such requirement, so their sync and invalidator IDs will be different.

This is implemented by using the preference (which may have been
initialized earlier, thanks to r180907) as the source of the invalidator
ID.  If the invalidator ID was not previously set, the code that
initializes the invalidator will create a new one.  This process
is entirely independent from the sync code.

The syncer will continue to report the sync client ID as before, but it
will now be provided with the invalidator ID during its construction.
It no longer has any control over this ID.

As part of this change, the many invalidator implementations have been
modified to accept a 'notifier_client_id' parameter in their
constructor.  This API change better reflects the fact that the client
ID may not be changed at runtime, and is made possible by the fact that
the notifier client ID is now available before the notifier is
constructed.

Finally, this code removes the hacks that were used to help support this
migration.  This includes the logic to forward the invalidator ID to the
preferences store when it was initialized on the sync thread, and the
code that sets the invalidator ID to be equal to the sync ID.

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

[rlarocque, 2013-03-15 21:08:30]

I can't commit this change until the server adds support for the separate sync
and invalidator IDs, which should happen early next week.  

You're welcome to review it early, though.

[rlarocque, 2013-03-20 20:27:54]

I think the server support has been pushed.  PTAL.

[Nicolas Zea, 2013-03-20 21:17:33]

LGTM with some some nits

https://codereview.chromium.org/12847003/diff/1/sync/notifier/invalidator_fac...
File sync/notifier/invalidator_factory.cc (right):

https://codereview.chromium.org/12847003/diff/1/sync/notifier/invalidator_fac...
sync/notifier/invalidator_factory.cc:72:
invalidation_state_tracker->SetInvalidatorClientId(
why not have the clear call part of the SetInvalidatorClientId call?

https://codereview.chromium.org/12847003/diff/1/sync/notifier/non_blocking_in...
File sync/notifier/non_blocking_invalidator.h (right):

https://codereview.chromium.org/12847003/diff/1/sync/notifier/non_blocking_in...
sync/notifier/non_blocking_invalidator.h:82: const std::string
invalidator_client_id_;
do we need to keep this around? it doesn't look like it's used for anything

[rlarocque, 2013-03-20 23:25:33]

New patch uploaded.  

I might have to upload yet another patch after this one.  I just realized that I
haven't tried building the stand-alone sync client against these changes...

https://codereview.chromium.org/12847003/diff/1/sync/notifier/invalidator_fac...
File sync/notifier/invalidator_factory.cc (right):

https://codereview.chromium.org/12847003/diff/1/sync/notifier/invalidator_fac...
sync/notifier/invalidator_factory.cc:72:
invalidation_state_tracker->SetInvalidatorClientId(
On 2013/03/20 21:17:33, Nicolas Zea wrote:
> why not have the clear call part of the SetInvalidatorClientId call?

That's a great idea!  It should help prevent bugs like crbug.com/175847.

https://codereview.chromium.org/12847003/diff/1/sync/notifier/non_blocking_in...
File sync/notifier/non_blocking_invalidator.h (right):

https://codereview.chromium.org/12847003/diff/1/sync/notifier/non_blocking_in...
sync/notifier/non_blocking_invalidator.h:82: const std::string
invalidator_client_id_;
On 2013/03/20 21:17:33, Nicolas Zea wrote:
> do we need to keep this around? it doesn't look like it's used for anything

You're right.  Fixed.

[commit-bot: I haz the power, 2013-03-20 23:49:01]

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

[commit-bot: I haz the power, 2013-03-21 00:24:31]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-03-21 18:16:09]

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

[commit-bot: I haz the power, 2013-03-22 18:50:49]

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

[rlarocque, 2013-03-22 22:58:41]

I was working on a local branch with this patch applied and found that
reflection blocking was broken when the sync and invalidator IDs differ.  I
decided to abort this commit to save us the trouble of having to revert it
later.

[rlarocque, 2013-03-27 18:41:20]

On 2013/03/22 22:58:41, rlarocque wrote:
> I was working on a local branch with this patch applied and found that
> reflection blocking was broken when the sync and invalidator IDs differ.  I
> decided to abort this commit to save us the trouble of having to revert it
> later.

Server support is now landed and switched on.  I'm going to try once again to
land this patch.

[commit-bot: I haz the power, 2013-03-27 18:44:19]

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

[commit-bot: I haz the power, 2013-03-27 23:00:08]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-03-27 23:05:21]

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

[commit-bot: I haz the power, 2013-03-28 01:34:57]

Change committed as 191087