[Sync] Register PROXY_TABS with ModelTypeRegistry to include it in enabled types

[Sync] Register PROXY_TABS with ModelTypeRegistry to include it in enabled types

The issue is that PROXY_TABS type doesn't represent a type that gets synced
with the server, no entries are committed by the client or downloaded from the
server. Because of this PROXY_TABS doesn't need update handler in
ModelTypeRegistry. Unfortunately value of tabs_datatype_enabled is decided
based on the presence of PROXY_TABS in the set of enabled types.

The change I made is to register the type anyway thus including it in the set
of enabled directory types. It is not the ideal solution, but it is the
smallest, least invasive one. It limits ugliness to ProxyDataTypeController.

BUG=688045
R=skym@chromium.org

Review-Url: https://codereview.chromium.org/2684533003
Cr-Commit-Position: refs/heads/master@{#448873}
Committed: https://chromium.googlesource.com/chromium/src/+/5402cfa4cf2e6841bf3269bdfb8c3ee2bb3788cc

[pavely, 2017-02-07 20:37:19]

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

[commit-bot: I haz the power, 2017-02-07 20:38:01]

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, 2017-02-07 21:51:03]

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

[commit-bot: I haz the power, 2017-02-07 21:51:03]

Dry run: This issue passed the CQ dry run.

[skym, 2017-02-07 22:40:11]

lgtm

https://codereview.chromium.org/2684533003/diff/1/components/sync/driver/prox...
File components/sync/driver/proxy_data_type_controller.cc (right):

https://codereview.chromium.org/2684533003/diff/1/components/sync/driver/prox...
components/sync/driver/proxy_data_type_controller.cc:32: // by the presence of
PROXY_TABS in the list of enabled types.
This class is trying to not assume it is for PROXY_TABS. Can you change the
wording to something like the following to remove this assumption:

We still need to register proxy types because some logic checks for proxy types
being enabled, such as AddClientConfigParamsToMessage when setting value of
tabs_datatype_enabled.

https://codereview.chromium.org/2684533003/diff/1/components/sync/driver/prox...
components/sync/driver/proxy_data_type_controller.cc:33:
configurer->RegisterDirectoryDataType(type(), GROUP_PASSIVE);
Okay, so correct me if I'm wrong, it sounds like this is less than ideal because
now we're paying memory cost for holding into an update handler forever? Other
than that, this approach seems pretty clean to me.

[pavely, 2017-02-08 01:35:27]

https://codereview.chromium.org/2684533003/diff/1/components/sync/driver/prox...
File components/sync/driver/proxy_data_type_controller.cc (right):

https://codereview.chromium.org/2684533003/diff/1/components/sync/driver/prox...
components/sync/driver/proxy_data_type_controller.cc:32: // by the presence of
PROXY_TABS in the list of enabled types.
On 2017/02/07 22:40:11, skym wrote:
> This class is trying to not assume it is for PROXY_TABS. Can you change the
> wording to something like the following to remove this assumption:
> 
> We still need to register proxy types because some logic checks for proxy
types
> being enabled, such as AddClientConfigParamsToMessage when setting value of
> tabs_datatype_enabled.

In the long run I want to get rid of this peculiarity so I wanted this comment
to convey that AddClientConfigParamsToMessage is the only place where
registering proxy types matter. This way it is easier to find where to clean up
stuff.
I've adjusted comment to not mention PROXY_TABS.

https://codereview.chromium.org/2684533003/diff/1/components/sync/driver/prox...
components/sync/driver/proxy_data_type_controller.cc:33:
configurer->RegisterDirectoryDataType(type(), GROUP_PASSIVE);
On 2017/02/07 22:40:10, skym wrote:
> Okay, so correct me if I'm wrong, it sounds like this is less than ideal
because
> now we're paying memory cost for holding into an update handler forever? Other
> than that, this approach seems pretty clean to me.

Yes, you are right. We create update handler and commit contributor objects that
will never be used. I tried two more different approaches yesterday (keeping
proxy types as a separate set in ModelTypeRegistry and passing flag to
AddClientConfigigParams... directly from cycle context). Both approaches seem
more out of place than this one, their ugliness spreads over more interfaces
that are more global than ProxyDTC.

[pavely, 2017-02-08 01:35:48]

The CQ bit was checked by pavely@chromium.org

[pavely, 2017-02-08 01:35:49]

The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org
Link to the patchset: https://codereview.chromium.org/2684533003/#ps20001
(title: "Update comment.")

[commit-bot: I haz the power, 2017-02-08 01:36:26]

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, 2017-02-08 02:49:00]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1486517748680460,
"parent_rev": "096fa853801729da981d17b5346698acb4cca48e", "commit_rev":
"5402cfa4cf2e6841bf3269bdfb8c3ee2bb3788cc"}

[commit-bot: I haz the power, 2017-02-08 02:49:39]

Description was changed from

==========
[Sync] Register PROXY_TABS with ModelTypeRegistry to include it in enabled types

The issue is that PROXY_TABS type doesn't represent a type that gets synced
with the server, no entries are committed by the client or downloaded from the
server. Because of this PROXY_TABS doesn't need update handler in
ModelTypeRegistry. Unfortunately value of tabs_datatype_enabled is decided
based on the presence of PROXY_TABS in the set of enabled types.

The change I made is to register the type anyway thus including it in the set
of enabled directory types. It is not the ideal solution, but it is the
smallest, least invasive one. It limits ugliness to ProxyDataTypeController.

BUG=688045
R=skym@chromium.org
==========

to

==========
[Sync] Register PROXY_TABS with ModelTypeRegistry to include it in enabled types

The issue is that PROXY_TABS type doesn't represent a type that gets synced
with the server, no entries are committed by the client or downloaded from the
server. Because of this PROXY_TABS doesn't need update handler in
ModelTypeRegistry. Unfortunately value of tabs_datatype_enabled is decided
based on the presence of PROXY_TABS in the set of enabled types.

The change I made is to register the type anyway thus including it in the set
of enabled directory types. It is not the ideal solution, but it is the
smallest, least invasive one. It limits ugliness to ProxyDataTypeController.

BUG=688045
R=skym@chromium.org

Review-Url: https://codereview.chromium.org/2684533003
Cr-Commit-Position: refs/heads/master@{#448873}
Committed:
https://chromium.googlesource.com/chromium/src/+/5402cfa4cf2e6841bf3269bdfb8c...
==========

[commit-bot: I haz the power, 2017-02-08 02:49:40]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/5402cfa4cf2e6841bf3269bdfb8c...