[Sync] Add support for proxy types

[Sync] Add support for proxy types

Proxy types are those that have no sync representation, and are used as
placeholder to implicitly enable other types. They are never communicated to
the server, and have no presence in the local sync directory.

BUG=170162, 139726
TBR=jam@chromium.org

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

[Nicolas Zea, 2013-01-17 02:26:08]

This implements my design for local types.
https://codereview.chromium.org/11961030/ introduces TABS as a local type.

[tim (not reviewing), 2013-01-18 20:56:33]

https://codereview.chromium.org/11958029/diff/13001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://codereview.chromium.org/11958029/diff/13001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host.cc:686:
types_to_download.RemoveAll(syncer::LocalTypes());
It looks like most of this CL is actually removing virtual types from places
where they don't match the expectation of the code around it.  Are we really
sure "virtual types" aren't purely a glue/ layer translation concept that don't
need to be plumbed down to the sync engine?

https://codereview.chromium.org/11958029/diff/13001/chrome/browser/sync/test/...
File chrome/browser/sync/test/integration/enable_disable_test.cc (right):

https://codereview.chromium.org/11958029/diff/13001/chrome/browser/sync/test/...
chrome/browser/sync/test/integration/enable_disable_test.cc:57: if
(syncer::LocalTypes().Has(it.Get()))
Shouldn't we also cover enabling virtual types?

https://codereview.chromium.org/11958029/diff/13001/sync/internal_api/sync_en...
File sync/internal_api/sync_encryption_handler_impl_unittest.cc (left):

https://codereview.chromium.org/11958029/diff/13001/sync/internal_api/sync_en...
sync/internal_api/sync_encryption_handler_impl_unittest.cc:375:
encrypted_user_types.Remove(HISTORY_DELETE_DIRECTIVES);
Where did this come from?

[Nicolas Zea, 2013-01-19 00:03:13]

PTAL

https://codereview.chromium.org/11958029/diff/13001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://codereview.chromium.org/11958029/diff/13001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host.cc:686:
types_to_download.RemoveAll(syncer::LocalTypes());
On 2013/01/18 20:56:33, timsteele wrote:
> It looks like most of this CL is actually removing virtual types from places
> where they don't match the expectation of the code around it.  Are we really
> sure "virtual types" aren't purely a glue/ layer translation concept that
don't
> need to be plumbed down to the sync engine?

How do you mean? If we're passing information via the routing info, it has to be
understood by the engine layer.

That said, most of the places being adjusted are tests. I also think I was
prematurely adjusting some of them, and so have slimmed this patch down a bit.

https://codereview.chromium.org/11958029/diff/13001/chrome/browser/sync/test/...
File chrome/browser/sync/test/integration/enable_disable_test.cc (right):

https://codereview.chromium.org/11958029/diff/13001/chrome/browser/sync/test/...
chrome/browser/sync/test/integration/enable_disable_test.cc:57: if
(syncer::LocalTypes().Has(it.Get()))
On 2013/01/18 20:56:33, timsteele wrote:
> Shouldn't we also cover enabling virtual types?

Hmm, yeah. Should probably just special case tabs to check sessions below
(similar to autofill). Moved that logic into the patch introducing TABS.

https://codereview.chromium.org/11958029/diff/13001/sync/internal_api/sync_en...
File sync/internal_api/sync_encryption_handler_impl_unittest.cc (left):

https://codereview.chromium.org/11958029/diff/13001/sync/internal_api/sync_en...
sync/internal_api/sync_encryption_handler_impl_unittest.cc:375:
encrypted_user_types.Remove(HISTORY_DELETE_DIRECTIVES);
On 2013/01/18 20:56:33, timsteele wrote:
> Where did this come from?

Oh, this is just a cleanup that I noticed while making sure things were working.
History delete directives are already not a part of EncryptableUserTypes.

[Nicolas Zea, 2013-01-30 23:59:20]

This is once again ready for review. Based on Raz's feedback, we now ensure no
virtual type information is communicated to the server (which is enforced by not
having field numbers or root tags to communicate with).

[Nicolas Zea, 2013-02-01 18:45:14]

ping?

[tim (not reviewing), 2013-02-04 19:23:56]

On 2013/02/01 18:45:14, Nicolas Zea wrote:
> ping?

The patch that stops sending virtual type info to the server only seems to
reinforce the doubt in my mind that virtual types should be a concept known to
sync/ syncable/ engine. I feel like they shouldn't be a ModelType, perhaps I'm
missing something.  Let's chat in person?

[tim (not reviewing), 2013-02-05 02:41:58]

Our in-person chat revealed the preferable option of creating a separate
EnumSet, so that real types can be used and passed around separately.  Another
idea occurred to me after, so adding it below for posterity. Let me know how
you'd like to proceed given the 3 options.

https://codereview.chromium.org/11958029/diff/19013/chrome/browser/sync/sync_...
File chrome/browser/sync/sync_prefs.cc (right):

https://codereview.chromium.org/11958029/diff/19013/chrome/browser/sync/sync_...
chrome/browser/sync/sync_prefs.cc:74: user_types.PutAll(syncer::VirtualTypes());
If you happen to love ascii art, this wouldn't be a bad time to crack out a Venn
diagram for all these different sets of data types :)

https://codereview.chromium.org/11958029/diff/29001/sync/internal_api/sync_ma...
File sync/internal_api/sync_manager_impl_unittest.cc (right):

https://codereview.chromium.org/11958029/diff/29001/sync/internal_api/sync_ma...
sync/internal_api/sync_manager_impl_unittest.cc:2873: for
(ModelTypeSet::Iterator iter = ModelTypeSet::All().First(); iter.Good();
Random thought, another option to this and to creating a separate EnumSet would
be to have Iterators that filter for you categorically.  It would avoid a lot of
the continue statements in this patch and having to remember to add some in the
future (reminiscent of how we used to always have to treat TOP_LEVEL_FOLDER
separately).

https://codereview.chromium.org/11958029/diff/29001/sync/syncable/model_type.cc
File sync/syncable/model_type.cc (right):

https://codereview.chromium.org/11958029/diff/29001/sync/syncable/model_type....
sync/syncable/model_type.cc:314: // though.
'Their underlying implicit types may or may not be encrypted though" is
confusing...  How about, "Note however that a virtual type maps to one or more
real ModelTypes which may or may not be encrypted."

[Nicolas Zea, 2013-02-07 01:18:48]

PTAL

https://codereview.chromium.org/11958029/diff/29001/sync/internal_api/sync_ma...
File sync/internal_api/sync_manager_impl_unittest.cc (right):

https://codereview.chromium.org/11958029/diff/29001/sync/internal_api/sync_ma...
sync/internal_api/sync_manager_impl_unittest.cc:2873: for
(ModelTypeSet::Iterator iter = ModelTypeSet::All().First(); iter.Good();
On 2013/02/05 02:41:58, timsteele wrote:
> Random thought, another option to this and to creating a separate EnumSet
would
> be to have Iterators that filter for you categorically.  It would avoid a lot
of
> the continue statements in this patch and having to remember to add some in
the
> future (reminiscent of how we used to always have to treat TOP_LEVEL_FOLDER
> separately).

I went ahea and added a ProtocolTypes() method in additon to the VirtualTypes()
one, and cleaned up number of places that should be using it instead.

https://codereview.chromium.org/11958029/diff/29001/sync/syncable/model_type.cc
File sync/syncable/model_type.cc (right):

https://codereview.chromium.org/11958029/diff/29001/sync/syncable/model_type....
sync/syncable/model_type.cc:314: // though.
On 2013/02/05 02:41:58, timsteele wrote:
> 'Their underlying implicit types may or may not be encrypted though" is
> confusing...  How about, "Note however that a virtual type maps to one or more
> real ModelTypes which may or may not be encrypted."

Done.

[tim (not reviewing), 2013-02-07 18:16:43]

LGTM ++, but read comment :)

https://codereview.chromium.org/11958029/diff/42001/sync/internal_api/public/...
File sync/internal_api/public/base/model_type.h (right):

https://codereview.chromium.org/11958029/diff/42001/sync/internal_api/public/...
sync/internal_api/public/base/model_type.h:158: // Virtual types are placeholder
types for handling implicitly enabling real
I hadn't realized it when suggesting it but the fact that Virtual is a subset of
Real is just fundamentally breaking down my view of the world :(  We take "Real"
to mean "not unspecified or top_level_folder", but when you throw virtual in
there the true meaning becomes far more ambiguous. ProxyTypes might be less
egregious? 

Just do what you think is best.  I think 'Real' is the true misnomer, but... we
can't really change that easily.

[Nicolas Zea, 2013-02-07 21:41:34]

Went ahead and switched to ProxyTypes. Committing.

https://codereview.chromium.org/11958029/diff/42001/sync/internal_api/public/...
File sync/internal_api/public/base/model_type.h (right):

https://codereview.chromium.org/11958029/diff/42001/sync/internal_api/public/...
sync/internal_api/public/base/model_type.h:158: // Virtual types are placeholder
types for handling implicitly enabling real
On 2013/02/07 18:16:43, timsteele wrote:
> I hadn't realized it when suggesting it but the fact that Virtual is a subset
of
> Real is just fundamentally breaking down my view of the world :(  We take
"Real"
> to mean "not unspecified or top_level_folder", but when you throw virtual in
> there the true meaning becomes far more ambiguous. ProxyTypes might be less
> egregious? 
> 
> Just do what you think is best.  I think 'Real' is the true misnomer, but...
we
> can't really change that easily.

Done.

[commit-bot: I haz the power, 2013-02-07 21:55:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/11958029/52003

[commit-bot: I haz the power, 2013-02-07 21:55:38]

Presubmit check for 11958029-52003 failed and returned exit status 1.

INFO:root:Found 21 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/sync/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/sync/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 2.1s to calculate.

[Nicolas Zea, 2013-02-07 22:40:48]

TBR'ing jam chrome_browser.gypi ownership

[commit-bot: I haz the power, 2013-02-07 22:41:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/11958029/52003

[jam, 2013-02-08 01:03:21]

On 2013/02/07 22:40:48, Nicolas Zea wrote:
> TBR'ing jam chrome_browser.gypi ownership

lgtm. in the future please send reviews to others in chrome/owners

[commit-bot: I haz the power, 2013-02-08 01:20:28]

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

[commit-bot: I haz the power, 2013-02-08 19:02:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/11958029/52003

[commit-bot: I haz the power, 2013-02-08 19:36:43]

Change committed as 181534