Fix sync for reading list

Fix sync for reading list

Reading list was not added to the possible selected options for sync.
This CL fixes it and add an histogram for the reading list sync option.

BUG=669393
Committed: https://crrev.com/62073c7aeb03e4893f72f2646ea391ac2bc155c2
Cr-Commit-Position: refs/heads/master@{#438793}

[gambard, 2016-12-05 16:06:56]

gambard@chromium.org changed reviewers:
+ maxbogue@chromium.org, olivierrobin@chromium.org, rkaplow@chromium.org

[gambard, 2016-12-05 16:06:56]

PTAL.

[gambard, 2016-12-05 16:12:34]

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

[commit-bot: I haz the power, 2016-12-05 16:13:00]

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

[gambard, 2016-12-05 16:14:24]

gambard@chromium.org changed reviewers:
+ pavely@chromium.org

[gambard, 2016-12-05 16:14:25]

+pavely@ as the person having done most of the reading list reviews.

[rkaplow, 2016-12-05 16:44:44]

lgtm

[commit-bot: I haz the power, 2016-12-05 16:52:20]

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

[commit-bot: I haz the power, 2016-12-05 16:52:20]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pavely, 2016-12-05 18:38:03]

lgtm

[pavely, 2016-12-05 18:48:42]

https://codereview.chromium.org/2548413002/diff/1/components/sync/syncable/mo...
File components/sync/syncable/model_type.cc (right):

https://codereview.chromium.org/2548413002/diff/1/components/sync/syncable/mo...
components/sync/syncable/model_type.cc:156: const char*
kUserSelectableDataTypeNames[] = {
Please add reading list name in this list as well. Value should correspond to
the one used in javascript:
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/people_p...

[gambard, 2016-12-06 10:14:11]

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

[commit-bot: I haz the power, 2016-12-06 10:14:24]

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

[gambard, 2016-12-06 10:15:42]

Thanks, pavely@ a question about the javascript part of it.
PTAL.

https://codereview.chromium.org/2548413002/diff/1/components/sync/syncable/mo...
File components/sync/syncable/model_type.cc (right):

https://codereview.chromium.org/2548413002/diff/1/components/sync/syncable/mo...
components/sync/syncable/model_type.cc:156: const char*
kUserSelectableDataTypeNames[] = {
On 2016/12/05 18:48:42, pavely wrote:
> Please add reading list name in this list as well. Value should correspond to
> the one used in javascript:
>
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/people_p...

Thanks, I have added it here but I am not sure I need to add it to the
javascript: this options is available only for iOS.
WDYT?

[commit-bot: I haz the power, 2016-12-06 10:53:37]

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

[commit-bot: I haz the power, 2016-12-06 10:53:38]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pavely, 2016-12-07 06:29:28]

On 2016/12/06 10:15:42, gambard wrote:
> Thanks, pavely@ a question about the javascript part of it.
> PTAL.
> 
>
https://codereview.chromium.org/2548413002/diff/1/components/sync/syncable/mo...
> File components/sync/syncable/model_type.cc (right):
> 
>
https://codereview.chromium.org/2548413002/diff/1/components/sync/syncable/mo...
> components/sync/syncable/model_type.cc:156: const char*
> kUserSelectableDataTypeNames[] = {
> On 2016/12/05 18:48:42, pavely wrote:
> > Please add reading list name in this list as well. Value should correspond
to
> > the one used in javascript:
> >
>
https://cs.chromium.org/chromium/src/chrome/test/data/webui/settings/people_p...
> 
> Thanks, I have added it here but I am not sure I need to add it to the
> javascript: this options is available only for iOS.
> WDYT?

I looked at how UI is implemented. It seems datatypes are not listed in
javascript, but controls for them are present in HTML. The way HTML is written,
controls will be hidden for types that are not registered
(https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/people...).

I'm not sure if you need to add type to html. Owners of UI would advise better
(dbeam@ for example). I personally don't think you need to change html/js. You
just need to adjust SyncSetupHandlerTest and PeopleHandlerTest.

[gambard, 2016-12-07 15:07:06]

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

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

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-12-07 15:45:45]

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

[commit-bot: I haz the power, 2016-12-07 15:45:46]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[gambard, 2016-12-07 17:38:25]

gambard@chromium.org changed reviewers:
+ dbeam@chromium.org

[gambard, 2016-12-07 17:38:25]

dbeam@: PTAL.
The reading list option for sync will only be available for iOS. I have added
the field in the GetConfiguration method of the tests only, I don't know if I
need to do some changes to html/js.
I also don't understand why the tests are failing as they are passing locally
(mac build).

[gambard, 2016-12-12 17:09:03]

gambard@chromium.org changed reviewers:
+ stevenjb@chromium.org

[gambard, 2016-12-12 17:09:04]

+stevenjb for webui related question.
PTAL

[stevenjb, 2016-12-12 17:34:17]

On 2016/12/12 17:09:04, gambard wrote:
> +stevenjb for webui related question.
> PTAL

I don't know what we do for Settings in iOS, but I'm pretty sure you don't need
to change any WebUI.

[gambard, 2016-12-13 11:43:12]

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

[commit-bot: I haz the power, 2016-12-13 11:43:32]

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-12-13 13:17:26]

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

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

Dry run: This issue passed the CQ dry run.

[gambard, 2016-12-13 13:29:28]

On 2016/12/12 17:34:17, stevenjb wrote:
> On 2016/12/12 17:09:04, gambard wrote:
> > +stevenjb for webui related question.
> > PTAL
> 
> I don't know what we do for Settings in iOS, but I'm pretty sure you don't
need
> to change any WebUI.

Thanks for your answer, the iOS part is done.
PTAL at the webui _unittest changes.
If you think it is possible to do it without any changes to webui, tell me how.

[maxbogue, 2016-12-13 17:43:52]

Description was changed from

==========
Fix sync for reading list

Reading list was not added to the possible selected options for sync.
This CL fixes it and add an histogram for the reading list sync option.

BUG=669393
==========

to

==========
Fix sync for reading list

Reading list was not added to the possible selected options for sync.
This CL fixes it and add an histogram for the reading list sync option.

BUG=669393
==========

[maxbogue, 2016-12-13 17:43:53]

maxbogue@chromium.org changed reviewers:
- maxbogue@chromium.org

[stevenjb, 2016-12-13 18:17:57]

On 2016/12/13 13:29:28, gambard wrote:
> On 2016/12/12 17:34:17, stevenjb wrote:
> > On 2016/12/12 17:09:04, gambard wrote:
> > > +stevenjb for webui related question.
> > > PTAL
> > 
> > I don't know what we do for Settings in iOS, but I'm pretty sure you don't
> need
> > to change any WebUI.
> 
> Thanks for your answer, the iOS part is done.
> PTAL at the webui _unittest changes.
> If you think it is possible to do it without any changes to webui, tell me
how.

OK, I see. Yeah, this change would break Settings UI as-is, please don't just
make that change to people_handler_unittest.cc. It "fixes" the test without
changing the WebUI which will probably break.

Instead we should probably modify syncer::GetUserSelectableTypeNameMap() to not
provide "readingListSynced" on non iOS platforms, or provide a separate method
for desktop Settings UI to use.

[gambard, 2016-12-14 08:55:28]

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

[commit-bot: I haz the power, 2016-12-14 08:55:46]

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-12-14 09:54:05]

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

[commit-bot: I haz the power, 2016-12-14 09:54:06]

Dry run: This issue passed the CQ dry run.

[gambard, 2016-12-14 16:53:25]

Thanks.
pavely@: PTAL as I have made some changes.
stevenjb@: please check if this is OK for webui (should be)

[stevenjb, 2016-12-14 18:07:22]

lgtm

[pavely, 2016-12-15 01:40:25]

lgtm

https://codereview.chromium.org/2548413002/diff/80001/components/sync/driver/...
File components/sync/driver/user_selectable_sync_type.h (right):

https://codereview.chromium.org/2548413002/diff/80001/components/sync/driver/...
components/sync/driver/user_selectable_sync_type.h:34: READING_LIST = 11,
Could you move this line after WIFI_CREDENTIAL.

[Olivier, 2016-12-15 08:14:10]

LGTM one nit

https://codereview.chromium.org/2548413002/diff/100001/components/sync/syncab...
File components/sync/syncable/DEPS (right):

https://codereview.chromium.org/2548413002/diff/100001/components/sync/syncab...
components/sync/syncable/DEPS:2:
"+components/reading_list/core/reading_list_enable_flags.h",
stop at core

[gambard, 2016-12-15 08:18:02]

Thanks!

https://codereview.chromium.org/2548413002/diff/80001/components/sync/driver/...
File components/sync/driver/user_selectable_sync_type.h (right):

https://codereview.chromium.org/2548413002/diff/80001/components/sync/driver/...
components/sync/driver/user_selectable_sync_type.h:34: READING_LIST = 11,
On 2016/12/15 01:40:25, pavely wrote:
> Could you move this line after WIFI_CREDENTIAL.

Done.

https://codereview.chromium.org/2548413002/diff/100001/components/sync/syncab...
File components/sync/syncable/DEPS (right):

https://codereview.chromium.org/2548413002/diff/100001/components/sync/syncab...
components/sync/syncable/DEPS:2:
"+components/reading_list/core/reading_list_enable_flags.h",
On 2016/12/15 08:14:10, Olivier Robin wrote:
> stop at core

Done.

[gambard, 2016-12-15 08:18:10]

The CQ bit was checked by gambard@chromium.org

[gambard, 2016-12-15 08:18:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
rkaplow@chromium.org, stevenjb@chromium.org, pavely@chromium.org,
olivierrobin@chromium.org
Link to the patchset: https://codereview.chromium.org/2548413002/#ps120001
(title: "Update DEPS")

[commit-bot: I haz the power, 2016-12-15 08:18:25]

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-12-15 09:42:55]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1481789890876880,
"parent_rev": "ac4fdf5574bdadf5b3d6383f4c4efc1841a55c85", "commit_rev":
"49fa6044565e3020eca21e8e0537bf9f07ad94b2"}

[commit-bot: I haz the power, 2016-12-15 09:43:22]

Description was changed from

==========
Fix sync for reading list

Reading list was not added to the possible selected options for sync.
This CL fixes it and add an histogram for the reading list sync option.

BUG=669393
==========

to

==========
Fix sync for reading list

Reading list was not added to the possible selected options for sync.
This CL fixes it and add an histogram for the reading list sync option.

BUG=669393

Review-Url: https://codereview.chromium.org/2548413002
==========

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

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-12-15 09:46:04]

Description was changed from

==========
Fix sync for reading list

Reading list was not added to the possible selected options for sync.
This CL fixes it and add an histogram for the reading list sync option.

BUG=669393

Review-Url: https://codereview.chromium.org/2548413002
==========

to

==========
Fix sync for reading list

Reading list was not added to the possible selected options for sync.
This CL fixes it and add an histogram for the reading list sync option.

BUG=669393

Committed: https://crrev.com/62073c7aeb03e4893f72f2646ea391ac2bc155c2
Cr-Commit-Position: refs/heads/master@{#438793}
==========

[commit-bot: I haz the power, 2016-12-15 09:46:05]

Patchset 7 (id:??) landed as
https://crrev.com/62073c7aeb03e4893f72f2646ea391ac2bc155c2
Cr-Commit-Position: refs/heads/master@{#438793}