[Sync] Add a sanity integration test for USS.

[Sync] Add a sanity integration test for USS.

This integration test overwrites the PREFERENCES data type to use a fake
ModelTypeService implementation. This is done using two new static
calls:

- ProfileSyncComponentsFactoryImpl::OverridePrefsForUssTest to force
  PREFERENCES to be registered as a USS type (with a
  ModelTypeController).
- ProfileSyncServiceFactory::SetSyncClientFactoryForTest to override the
  SyncClient that the real ProfileSyncService uses so that the
  ModelTypeService can be injected.

Additionally, this change fixes an issue with deletions under USS that
was uncovered while writing it. The specifics used for a deleted entity
must have ByteSize() == 0 for it to be detected as a deletion, but we
were setting it to the one sent by the server, which has ByteSize() ==
4 so the model type can be extracted from it.

BUG=643269
Committed: https://crrev.com/a1d69879d0c74f5edae8cb1a942ff6538fe398cb
Cr-Commit-Position: refs/heads/master@{#418404}

[maxbogue, 2016-09-12 17:28:55]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 17:29:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-12 17:52:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-12 17:52:51]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[maxbogue, 2016-09-12 18:42:43]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 18:43:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[maxbogue, 2016-09-12 19:00:36]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 19:01:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[maxbogue, 2016-09-12 19:03:10]

maxbogue@chromium.org changed reviewers:
+ pavely@chromium.org, skym@chromium.org

[maxbogue, 2016-09-12 19:03:11]

Hey Pavel + Sky, PTAL! I'm aware that the static overrides are a bit messy, but
I was unable to find a cleaner approach. Let me know what you think!

[commit-bot: I haz the power, 2016-09-12 19:07:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-12 19:07:16]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[maxbogue, 2016-09-12 19:14:03]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-12 19:14:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skym, 2016-09-12 19:57:50]

Sorry all my comments are on PS#3

https://codereview.chromium.org/2328393002/diff/40001/chrome/browser/sync/pro...
File chrome/browser/sync/profile_sync_service_factory.cc (right):

https://codereview.chromium.org/2328393002/diff/40001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service_factory.cc:173: if (!client_factory_) {
Might consider also having a default factory as a static field, that can then be
overriden. Instead of dealing with two different states here.

https://codereview.chromium.org/2328393002/diff/40001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service_factory.cc:212:
ProfileSyncServiceFactory::client_factory_ = nullptr;
Again, I'd set this in the .h personally.

https://codereview.chromium.org/2328393002/diff/40001/chrome/browser/sync/pro...
File chrome/browser/sync/profile_sync_service_factory.h (right):

https://codereview.chromium.org/2328393002/diff/40001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service_factory.h:53: // This is a raw pointer
so it can be statically initialized.
So does this mean you cannot statically initialize a std::unique_ptr ?

https://codereview.chromium.org/2328393002/diff/40001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/two_client_uss_sync_test.cc (right):

https://codereview.chromium.org/2328393002/diff/40001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:25:
TestSyncClient(Profile* profile, ModelTypeService* service)
Could probably get away with slapping const on like all of these raw pointers in
this file.

https://codereview.chromium.org/2328393002/diff/40001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:30: if (type !=
syncer::PREFERENCES) {
Might be more elegant as a ternary.

https://codereview.chromium.org/2328393002/diff/40001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:150:
ProfileSyncServiceFactory::SetSyncClientFactoryForTest(nullptr);
All these test cases run in serial, right? Even when testing locally?

https://codereview.chromium.org/2328393002/diff/40001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:151:
ProfileSyncComponentsFactoryImpl::OverridePrefsForUssTest(false);
Does this get called if you crash mid test? Could these pref values stay around
for other tests?

https://codereview.chromium.org/2328393002/diff/40001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:160: static
bool WaitForKeyPresent(TestModelTypeService* model,
Why not push these methods into the checker classes themselves?

https://codereview.chromium.org/2328393002/diff/40001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:192: // There
are three because the test infra creates a profile before the two
This seems awkward, like something we may want to fix eventually.

https://codereview.chromium.org/2328393002/diff/40001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:193: // made
for our tests. Ignore index 0 and use indexes 1 and 2.
So, technically 3 client?

https://codereview.chromium.org/2328393002/diff/40001/components/browser_sync...
File components/browser_sync/browser/profile_sync_components_factory_impl.cc
(right):

https://codereview.chromium.org/2328393002/diff/40001/components/browser_sync...
components/browser_sync/browser/profile_sync_components_factory_impl.cc:145:
void ProfileSyncComponentsFactoryImpl::RegisterCommonDataTypes(
This whole method makes me sad.

https://codereview.chromium.org/2328393002/diff/40001/components/browser_sync...
components/browser_sync/browser/profile_sync_components_factory_impl.cc:428:
bool ProfileSyncComponentsFactoryImpl::override_prefs_for_uss_test_ = false;
Why not do this assignment in the .h file?

https://codereview.chromium.org/2328393002/diff/40001/components/browser_sync...
File components/browser_sync/browser/profile_sync_components_factory_impl.h
(right):

https://codereview.chromium.org/2328393002/diff/40001/components/browser_sync...
components/browser_sync/browser/profile_sync_components_factory_impl.h:93:
static void OverridePrefsForUssTest(bool use_uss);
...So is there going to be an OverrideBookmarksForUssTest and an
OverrideSessionsForUssTest and etc?

https://codereview.chromium.org/2328393002/diff/40001/components/browser_sync...
components/browser_sync/browser/profile_sync_components_factory_impl.h:125:
static bool override_prefs_for_uss_test_;
I find this variable name super misleading. Sounds like you're changing
preference values.

https://codereview.chromium.org/2328393002/diff/40001/components/sync/engine_...
File components/sync/engine_impl/model_type_worker.cc (left):

https://codereview.chromium.org/2328393002/diff/40001/components/sync/engine_...
components/sync/engine_impl/model_type_worker.cc:126: // TODO(stanisc):
crbug.com/516866: this wouldn't be true for bookmarks.
Did you mean to strip out this TODO?

https://codereview.chromium.org/2328393002/diff/40001/components/sync/engine_...
File components/sync/engine_impl/model_type_worker.cc (right):

https://codereview.chromium.org/2328393002/diff/40001/components/sync/engine_...
components/sync/engine_impl/model_type_worker.cc:137: WorkerEntityTracker*
entity = GetOrCreateEntityTracker(data);
As discussed offline, this doesn't always work if the worker doesn't have the
mapping in cache yet, or the client has never seen the entity before getting a
tombstone.

https://codereview.chromium.org/2328393002/diff/40001/components/sync/engine_...
components/sync/engine_impl/model_type_worker.cc:142: 
I personally think you should remove this newline.

https://codereview.chromium.org/2328393002/diff/40001/components/sync/engine_...
components/sync/engine_impl/model_type_worker.cc:145: // Deleted entities must
use the default instance of EntitySpecifics in
Why? Says who? What's expecting the default instance? This is handling a GU
right? So this data is about to go to the proc, then the service.

https://codereview.chromium.org/2328393002/diff/40001/components/sync/engine_...
components/sync/engine_impl/model_type_worker.cc:445: // Fall back to iterative
search by id.
Can you explain why this shouldn't happen very often and why it doesn't make
sense to maintain a second mapping.

[skym, 2016-09-12 19:59:31]

https://codereview.chromium.org/2328393002/diff/40001/components/sync/engine_...
File components/sync/engine_impl/model_type_worker.cc (right):

https://codereview.chromium.org/2328393002/diff/40001/components/sync/engine_...
components/sync/engine_impl/model_type_worker.cc:445: // Fall back to iterative
search by id.
On 2016/09/12 19:57:50, skym wrote:
> Can you explain why this shouldn't happen very often and why it doesn't make
> sense to maintain a second mapping.

Err, I wasn't very clear, I want this explanation in a comment in the code.

[commit-bot: I haz the power, 2016-09-12 20:34:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-12 20:34:27]

Dry run: This issue passed the CQ dry run.

[maxbogue, 2016-09-13 02:34:25]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 02:34:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 02:53:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-13 02:53:32]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[maxbogue, 2016-09-13 03:40:15]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 03:40:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[maxbogue, 2016-09-13 03:44:55]

https://codereview.chromium.org/2328393002/diff/40001/chrome/browser/sync/pro...
File chrome/browser/sync/profile_sync_service_factory.cc (right):

https://codereview.chromium.org/2328393002/diff/40001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service_factory.cc:173: if (!client_factory_) {
On 2016/09/12 19:57:49, skym wrote:
> Might consider also having a default factory as a static field, that can then
be
> overriden. Instead of dealing with two different states here.

That's harder to unset, and I don't believe I'm supposed to have a non-POD
static member (as I said in the header).

https://codereview.chromium.org/2328393002/diff/40001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service_factory.cc:212:
ProfileSyncServiceFactory::client_factory_ = nullptr;
On 2016/09/12 19:57:49, skym wrote:
> Again, I'd set this in the .h personally.

Can't, sadly. It's a compile error:

error: non-const static data member must be initialized out of line

https://codereview.chromium.org/2328393002/diff/40001/chrome/browser/sync/pro...
File chrome/browser/sync/profile_sync_service_factory.h (right):

https://codereview.chromium.org/2328393002/diff/40001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service_factory.h:53: // This is a raw pointer
so it can be statically initialized.
On 2016/09/12 19:57:49, skym wrote:
> So does this mean you cannot statically initialize a std::unique_ptr ?

Non-const static variables must be POD types, from what I can tell:
https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

https://codereview.chromium.org/2328393002/diff/40001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/two_client_uss_sync_test.cc (right):

https://codereview.chromium.org/2328393002/diff/40001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:25:
TestSyncClient(Profile* profile, ModelTypeService* service)
On 2016/09/12 19:57:49, skym wrote:
> Could probably get away with slapping const on like all of these raw pointers
in
> this file.

Umm I've made the member vars in TestSyncClient and KeyChecker Foo* const. I
can't really make any of them const Foo* because they all do non-consty things
like AsWeakPtr, the ChromeSyncClient constructor, registering observers, etc.

https://codereview.chromium.org/2328393002/diff/40001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:30: if (type !=
syncer::PREFERENCES) {
On 2016/09/12 19:57:49, skym wrote:
> Might be more elegant as a ternary.

Done.

https://codereview.chromium.org/2328393002/diff/40001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:150:
ProfileSyncServiceFactory::SetSyncClientFactoryForTest(nullptr);
On 2016/09/12 19:57:49, skym wrote:
> All these test cases run in serial, right? Even when testing locally?

My understanding is that they run in serial within each process even if you do
multiple jobs.

https://codereview.chromium.org/2328393002/diff/40001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:151:
ProfileSyncComponentsFactoryImpl::OverridePrefsForUssTest(false);
On 2016/09/12 19:57:50, skym wrote:
> Does this get called if you crash mid test? Could these pref values stay
around
> for other tests?

The flag is just in memory. If we crash mid-test it isn't persisted anywhere.

https://codereview.chromium.org/2328393002/diff/40001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:160: static
bool WaitForKeyPresent(TestModelTypeService* model,
On 2016/09/12 19:57:49, skym wrote:
> Why not push these methods into the checker classes themselves?

I think this way reads a little better. Otherwise it'd be like
KeyPresentChecker::Wait() instead of WaitForKeyPresent(), etc.

https://codereview.chromium.org/2328393002/diff/40001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:192: // There
are three because the test infra creates a profile before the two
Yes, but I have no way of overwriting the thing on PSSFactory at the correct
time (between the base test class's SetUp method and the test method). I've
switched to using a boolean to ignore the first call to CreateSyncClient though;
that way it's not something that shows up in each test.

https://codereview.chromium.org/2328393002/diff/40001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:193: // made
for our tests. Ignore index 0 and use indexes 1 and 2.
On 2016/09/12 19:57:50, skym wrote:
> So, technically 3 client?

Nothing ever happens with sync in the original profile, so it doesn't affect the
test.

https://codereview.chromium.org/2328393002/diff/40001/components/browser_sync...
File components/browser_sync/browser/profile_sync_components_factory_impl.cc
(right):

https://codereview.chromium.org/2328393002/diff/40001/components/browser_sync...
components/browser_sync/browser/profile_sync_components_factory_impl.cc:145:
void ProfileSyncComponentsFactoryImpl::RegisterCommonDataTypes(
On 2016/09/12 19:57:50, skym wrote:
> This whole method makes me sad.

SAME! It's so awkward and clunky. Should probs be two methods: "is this datatype
enabled?" and "gimme the DTC for this datatype."

https://codereview.chromium.org/2328393002/diff/40001/components/browser_sync...
components/browser_sync/browser/profile_sync_components_factory_impl.cc:428:
bool ProfileSyncComponentsFactoryImpl::override_prefs_for_uss_test_ = false;
On 2016/09/12 19:57:50, skym wrote:
> Why not do this assignment in the .h file?

Can't :(

https://codereview.chromium.org/2328393002/diff/40001/components/browser_sync...
File components/browser_sync/browser/profile_sync_components_factory_impl.h
(right):

https://codereview.chromium.org/2328393002/diff/40001/components/browser_sync...
components/browser_sync/browser/profile_sync_components_factory_impl.h:93:
static void OverridePrefsForUssTest(bool use_uss);
On 2016/09/12 19:57:50, skym wrote:
> ...So is there going to be an OverrideBookmarksForUssTest and an
> OverrideSessionsForUssTest and etc?

Nope, any real datatype will have their own flag (ala DeviceInfo) to force USS
on. This one is just special because it's not a real datatype integration test,
it's just a fakey mcfake test for the sync USS infrastructure.

https://codereview.chromium.org/2328393002/diff/40001/components/browser_sync...
components/browser_sync/browser/profile_sync_components_factory_impl.h:125:
static bool override_prefs_for_uss_test_;
On 2016/09/12 19:57:50, skym wrote:
> I find this variable name super misleading. Sounds like you're changing
> preference values.

Changed to override_prefs_controller_to_uss_for_test_ as we discussed offline.

https://codereview.chromium.org/2328393002/diff/40001/components/sync/engine_...
File components/sync/engine_impl/model_type_worker.cc (left):

https://codereview.chromium.org/2328393002/diff/40001/components/sync/engine_...
components/sync/engine_impl/model_type_worker.cc:126: // TODO(stanisc):
crbug.com/516866: this wouldn't be true for bookmarks.
On 2016/09/12 19:57:50, skym wrote:
> Did you mean to strip out this TODO?

I thought I should but I was wrong. Re-added.

https://codereview.chromium.org/2328393002/diff/40001/components/sync/engine_...
File components/sync/engine_impl/model_type_worker.cc (right):

https://codereview.chromium.org/2328393002/diff/40001/components/sync/engine_...
components/sync/engine_impl/model_type_worker.cc:137: WorkerEntityTracker*
entity = GetOrCreateEntityTracker(data);
On 2016/09/12 19:57:50, skym wrote:
> As discussed offline, this doesn't always work if the worker doesn't have the
> mapping in cache yet, or the client has never seen the entity before getting a
> tombstone.

I've reverted the search by ID portion of this change and done the FakeServer
fix instead.

https://codereview.chromium.org/2328393002/diff/40001/components/sync/engine_...
components/sync/engine_impl/model_type_worker.cc:142: 
On 2016/09/12 19:57:50, skym wrote:
> I personally think you should remove this newline.

Done.

https://codereview.chromium.org/2328393002/diff/40001/components/sync/engine_...
components/sync/engine_impl/model_type_worker.cc:145: // Deleted entities must
use the default instance of EntitySpecifics in
On 2016/09/12 19:57:50, skym wrote:
> Why? Says who? What's expecting the default instance? This is handling a GU
> right? So this data is about to go to the proc, then the service.

https://cs.chromium.org/chromium/src/components/sync/api/entity_data.h?l=71

https://codereview.chromium.org/2328393002/diff/40001/components/sync/engine_...
components/sync/engine_impl/model_type_worker.cc:445: // Fall back to iterative
search by id.
On 2016/09/12 19:59:30, skym wrote:
> On 2016/09/12 19:57:50, skym wrote:
> > Can you explain why this shouldn't happen very often and why it doesn't make
> > sense to maintain a second mapping.
> 
> Err, I wasn't very clear, I want this explanation in a comment in the code.

I've removed this.

[commit-bot: I haz the power, 2016-09-13 04:58:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-13 04:58:54]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[skym, 2016-09-13 15:54:32]

lgtm

https://codereview.chromium.org/2328393002/diff/40001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/two_client_uss_sync_test.cc (right):

https://codereview.chromium.org/2328393002/diff/40001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:160: static
bool WaitForKeyPresent(TestModelTypeService* model,
On 2016/09/13 03:44:54, maxbogue wrote:
> On 2016/09/12 19:57:49, skym wrote:
> > Why not push these methods into the checker classes themselves?
> 
> I think this way reads a little better. Otherwise it'd be like
> KeyPresentChecker::Wait() instead of WaitForKeyPresent(), etc.

Remember that you have your arguments in there giving more context,
KeyPresentChecker::Wait(model, "key") , and you could change the function name
to WaitForKey if wanted.

Another approach would be to make Wait() return this->TimedOut();, so you could
replace WaitForKeyPresent(model, "key) with KeyPresentChecker(model,
"key").Wait().

https://codereview.chromium.org/2328393002/diff/40001/components/browser_sync...
File components/browser_sync/browser/profile_sync_components_factory_impl.h
(right):

https://codereview.chromium.org/2328393002/diff/40001/components/browser_sync...
components/browser_sync/browser/profile_sync_components_factory_impl.h:93:
static void OverridePrefsForUssTest(bool use_uss);
On 2016/09/13 03:44:54, maxbogue wrote:
> On 2016/09/12 19:57:50, skym wrote:
> > ...So is there going to be an OverrideBookmarksForUssTest and an
> > OverrideSessionsForUssTest and etc?
> 
> Nope, any real datatype will have their own flag (ala DeviceInfo) to force USS
> on. This one is just special because it's not a real datatype integration
test,
> it's just a fakey mcfake test for the sync USS infrastructure.

Still completely disagree, as we discussed offline. There are two things you're
changing here, the service and the controller. Yes, you're adding a fake
service, but you're adding a real working controller. And this is the section
where you decide what controller you add. There's no fakery here.

[maxbogue, 2016-09-13 18:06:40]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 18:07:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 18:10:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-13 18:10:01]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[pavely, 2016-09-13 19:04:11]

lgtm

[maxbogue, 2016-09-13 19:11:49]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 19:12:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[maxbogue, 2016-09-13 19:15:26]

https://codereview.chromium.org/2328393002/diff/40001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/two_client_uss_sync_test.cc (right):

https://codereview.chromium.org/2328393002/diff/40001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:160: static
bool WaitForKeyPresent(TestModelTypeService* model,
On 2016/09/13 15:54:32, skym wrote:
> On 2016/09/13 03:44:54, maxbogue wrote:
> > On 2016/09/12 19:57:49, skym wrote:
> > > Why not push these methods into the checker classes themselves?
> > 
> > I think this way reads a little better. Otherwise it'd be like
> > KeyPresentChecker::Wait() instead of WaitForKeyPresent(), etc.
> 
> Remember that you have your arguments in there giving more context,
> KeyPresentChecker::Wait(model, "key") , and you could change the function name
> to WaitForKey if wanted.
> 
> Another approach would be to make Wait() return this->TimedOut();, so you
could
> replace WaitForKeyPresent(model, "key) with KeyPresentChecker(model,
> "key").Wait().

I've gone with KeyPresentChecker(model, "key").Wait(). I forgot you can have
anonymous things on the stack in C++ -_-

https://codereview.chromium.org/2328393002/diff/40001/components/browser_sync...
File components/browser_sync/browser/profile_sync_components_factory_impl.h
(right):

https://codereview.chromium.org/2328393002/diff/40001/components/browser_sync...
components/browser_sync/browser/profile_sync_components_factory_impl.h:93:
static void OverridePrefsForUssTest(bool use_uss);
On 2016/09/13 15:54:32, skym wrote:
> On 2016/09/13 03:44:54, maxbogue wrote:
> > On 2016/09/12 19:57:50, skym wrote:
> > > ...So is there going to be an OverrideBookmarksForUssTest and an
> > > OverrideSessionsForUssTest and etc?
> > 
> > Nope, any real datatype will have their own flag (ala DeviceInfo) to force
USS
> > on. This one is just special because it's not a real datatype integration
> test,
> > it's just a fakey mcfake test for the sync USS infrastructure.
> 
> Still completely disagree, as we discussed offline. There are two things
you're
> changing here, the service and the controller. Yes, you're adding a fake
> service, but you're adding a real working controller. And this is the section
> where you decide what controller you add. There's no fakery here.

Right, but it's for a fake purpose, which is why there's a magical function on
this class instead of actually having a proper Flag like any real datatype
implementation will. We won't have to add more functions like this in the
future. In fact, we'll even be able to remove this one once PREFERENCES has a
USS implementation.

[commit-bot: I haz the power, 2016-09-13 20:17:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-13 20:17:59]

Dry run: This issue passed the CQ dry run.

[maxbogue, 2016-09-13 23:09:07]

The CQ bit was checked by maxbogue@chromium.org

[maxbogue, 2016-09-13 23:09:07]

The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org,
pavely@chromium.org
Link to the patchset: https://codereview.chromium.org/2328393002/#ps140001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-09-13 23:09:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 23:15:25]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-09-13 23:17:13]

Description was changed from

==========
[Sync] Add a sanity integration test for USS.

This integration test overwrites the PREFERENCES data type to use a fake
ModelTypeService implementation. This is done using two new static
calls:

- ProfileSyncComponentsFactoryImpl::OverridePrefsForUssTest to force
  PREFERENCES to be registered as a USS type (with a
  ModelTypeController).
- ProfileSyncServiceFactory::SetSyncClientFactoryForTest to override the
  SyncClient that the real ProfileSyncService uses so that the
  ModelTypeService can be injected.

Additionally, this change fixes an issue with deletions under USS that
was uncovered while writing it. The specifics used for a deleted entity
must have ByteSize() == 0 for it to be detected as a deletion, but we
were setting it to the one sent by the server, which has ByteSize() ==
4 so the model type can be extracted from it.

BUG=643269
==========

to

==========
[Sync] Add a sanity integration test for USS.

This integration test overwrites the PREFERENCES data type to use a fake
ModelTypeService implementation. This is done using two new static
calls:

- ProfileSyncComponentsFactoryImpl::OverridePrefsForUssTest to force
  PREFERENCES to be registered as a USS type (with a
  ModelTypeController).
- ProfileSyncServiceFactory::SetSyncClientFactoryForTest to override the
  SyncClient that the real ProfileSyncService uses so that the
  ModelTypeService can be injected.

Additionally, this change fixes an issue with deletions under USS that
was uncovered while writing it. The specifics used for a deleted entity
must have ByteSize() == 0 for it to be detected as a deletion, but we
were setting it to the one sent by the server, which has ByteSize() ==
4 so the model type can be extracted from it.

BUG=643269

Committed: https://crrev.com/a1d69879d0c74f5edae8cb1a942ff6538fe398cb
Cr-Commit-Position: refs/heads/master@{#418404}
==========

[commit-bot: I haz the power, 2016-09-13 23:17:13]

Patchset 8 (id:??) landed as
https://crrev.com/a1d69879d0c74f5edae8cb1a942ff6538fe398cb
Cr-Commit-Position: refs/heads/master@{#418404}