[Sync] Make SESSIONS an implicit type

[Sync] Make SESSIONS an implicit type

We add TABS as a new local type, which is now what is enabled via the user
settings. It implicitly enables SESSIONS.

Similarly, History now corresponds with Typed URLs, which implicitly enables 
history delete directives if the proper command line flag is passed.

Finally, History delete directives implicitly enables SESSIONS, which we can do
because Sessions is no longer a user selectable type.

BUG=170162
TBR=jhawkins@chromium.org

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

[Nicolas Zea, 2013-01-17 02:27:15]

This, built off https://codereview.chromium.org/11958029/, introduces TABS as a
local type, and adds the necessary history/sessions logic we have discussed.

Raz, note that I add a placeholder field in the sync proto so the server can
identify the tabs type in the config_params message.

[Raz Mathias, 2013-01-19 01:29:17]

https://codereview.chromium.org/11961030/diff/23004/sync/protocol/sync.proto
File sync/protocol/sync.proto (right):

https://codereview.chromium.org/11961030/diff/23004/sync/protocol/sync.proto#...
sync/protocol/sync.proto:111: optional string tab = 174470;  // Placeholder for
tabs, a virtual datatype.
i didn't realize that local/virtual types would have representation in
sync.proto.  i'm a little worried that server-side we key a lot of behavioral
mappings off the proto field number.  Of course, we could put in stubs,  and
asserts to prevent strange behavior but it really does feel like a very specific
client detail that will bleed into the server.  would it be possible to
implement local/virtual types client-side in a way that the server never needs
to see it?

[Nicolas Zea, 2013-01-19 01:51:09]

On 2013/01/19 01:29:17, Raz Mathias wrote:
> https://codereview.chromium.org/11961030/diff/23004/sync/protocol/sync.proto
> File sync/protocol/sync.proto (right):
> 
>
https://codereview.chromium.org/11961030/diff/23004/sync/protocol/sync.proto#...
> sync/protocol/sync.proto:111: optional string tab = 174470;  // Placeholder
for
> tabs, a virtual datatype.
> i didn't realize that local/virtual types would have representation in
> sync.proto.  i'm a little worried that server-side we key a lot of behavioral
> mappings off the proto field number.  Of course, we could put in stubs,  and
> asserts to prevent strange behavior but it really does feel like a very
specific
> client detail that will bleed into the server.  would it be possible to
> implement local/virtual types client-side in a way that the server never needs
> to see it?

The client needs to share some sort of identifier with the server, otherwise the
server won't be able to know whether the type is enabled or not. Specifically,
the client is going to include the 174470 value in the set of enabled types sent
within |config_params|. Without this field, the server won't understand that
number.

I'm certainly open to suggestions if there is a better approach to sharing the
value though. Using the field number seems the most consistent/straightforward.

[Nicolas Zea, 2013-01-31 00:03:11]

PTAL again. Raz, notice the tabs_datatype_enabled field in ClientConfigParams.

[Nicolas Zea, 2013-02-08 19:01:15]

ping? I've rebased onto the proxytypes change.

[tim (not reviewing), 2013-02-09 01:23:58]

Make sure you grep for syncer::SESSIONS, for example I think
foreign_session_handler.cc also needs to be updated.  

Otherwise this LGTM with nits

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

https://codereview.chromium.org/11961030/diff/56001/sync/internal_api/public/...
sync/internal_api/public/base/model_type.h:98: TABS,
Perhaps we should prefix proxy types by convention with PROXY_TYPE_, so it's
clearer when in context.

https://codereview.chromium.org/11961030/diff/56001/sync/protocol/sync.proto
File sync/protocol/sync.proto (right):

https://codereview.chromium.org/11961030/diff/56001/sync/protocol/sync.proto#...
sync/protocol/sync.proto:370: // types).
'does not include proxy types, as these don't have protocol field numbers.'
might be helpful to mention.

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

https://codereview.chromium.org/11961030/diff/56001/sync/syncable/model_type....
sync/syncable/model_type.cc:567: return "";
nit - prefer std::string()

[Raz Mathias, 2013-02-09 01:33:48]

LGTM for the proto

https://codereview.chromium.org/11961030/diff/56001/sync/protocol/sync.proto
File sync/protocol/sync.proto (right):

https://codereview.chromium.org/11961030/diff/56001/sync/protocol/sync.proto#...
sync/protocol/sync.proto:374: optional bool tabs_datatype_enabled = 2;
Please add a definition of "proxy type" to the proto here.  This comment
essentially becomes the reference point for the logs proto as well.

[Nicolas Zea, 2013-02-12 23:44:49]

Comments addressed. +TBR James for rename changes in the ui folders.

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

https://codereview.chromium.org/11961030/diff/56001/sync/internal_api/public/...
sync/internal_api/public/base/model_type.h:98: TABS,
On 2013/02/09 01:23:59, timsteele wrote:
> Perhaps we should prefix proxy types by convention with PROXY_TYPE_, so it's
> clearer when in context. 

Done.

https://codereview.chromium.org/11961030/diff/56001/sync/protocol/sync.proto
File sync/protocol/sync.proto (right):

https://codereview.chromium.org/11961030/diff/56001/sync/protocol/sync.proto#...
sync/protocol/sync.proto:370: // types).
On 2013/02/09 01:23:59, timsteele wrote:
> 'does not include proxy types, as these don't have protocol field numbers.'
> might be helpful to mention.

Done.

https://codereview.chromium.org/11961030/diff/56001/sync/protocol/sync.proto#...
sync/protocol/sync.proto:374: optional bool tabs_datatype_enabled = 2;
On 2013/02/09 01:33:48, Raz Mathias wrote:
> Please add a definition of "proxy type" to the proto here.  This comment
> essentially becomes the reference point for the logs proto as well.

Added to the comment above.

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

https://codereview.chromium.org/11961030/diff/56001/sync/syncable/model_type....
sync/syncable/model_type.cc:567: return "";
On 2013/02/09 01:23:59, timsteele wrote:
> nit - prefer std::string()

Done.

[commit-bot: I haz the power, 2013-02-14 00:54:57]

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

[commit-bot: I haz the power, 2013-02-14 01:23:51]

Retried try job too often on linux_aura for step(s) aura_unittests,
browser_tests, compositor_unittests, content_browsertests, content_unittests,
interactive_ui_tests, unit_tests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-02-14 01:29:13]

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

[commit-bot: I haz the power, 2013-02-14 01:58:01]

Retried try job too often on linux_aura for step(s) aura_unittests,
browser_tests, compositor_unittests, content_browsertests, content_unittests,
interactive_ui_tests, unit_tests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-02-14 18:28:34]

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

[commit-bot: I haz the power, 2013-02-14 20:24:24]

Change committed as 182527