Enable RL sync by default on iOS

Enable RL sync by default

BUG=

Committed: https://crrev.com/0c6cd0ca95b91d4b76fae8fc27a39f404fd756aa
Cr-Commit-Position: refs/heads/master@{#434402}

[Olivier, 2016-11-17 12:54:24]

Description was changed from

==========
Enable RL sync by default

BUG=
==========

to

==========
Enable RL sync by default

BUG=
==========

[Olivier, 2016-11-17 12:56:30]

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

[commit-bot: I haz the power, 2016-11-17 12:56:44]

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

[Olivier, 2016-11-17 12:57:36]

olivierrobin@chromium.org changed reviewers:
+ jif@chromium.org, pavely@chromium.org

[Olivier, 2016-11-17 12:57:36]

Hi

I try to enable the sync on iOS.
As the other platforms don't have implementations, I added an ifdef, but I am
not sure it is the best way to do it.
Is there a standard way for this?

Thanks

[commit-bot: I haz the power, 2016-11-17 13:36:08]

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

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

Dry run: This issue passed the CQ dry run.

[pavely, 2016-11-20 02:31:58]

https://codereview.chromium.org/2511723002/diff/40001/chrome/browser/sync/chr...
File chrome/browser/sync/chrome_sync_client.cc (right):

https://codereview.chromium.org/2511723002/diff/40001/chrome/browser/sync/chr...
chrome/browser/sync/chrome_sync_client.cc:439: case syncer::READING_LIST:
You can add NOTREACHED() as well as this function shouldn't be called on iOS.

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

https://codereview.chromium.org/2511723002/diff/40001/components/browser_sync...
components/browser_sync/profile_sync_components_factory_impl.cc:278: #if
defined(OS_IOS)
Yes, this is a correct way to only enable type on supported platform.
https://cs.chromium.org/chromium/src/chrome/browser/sync/chrome_sync_client.c...

https://codereview.chromium.org/2511723002/diff/40001/ios/chrome/browser/sync...
File ios/chrome/browser/sync/ios_chrome_sync_client.mm (right):

https://codereview.chromium.org/2511723002/diff/40001/ios/chrome/browser/sync...
ios/chrome/browser/sync/ios_chrome_sync_client.mm:341: if
(ReadingListModelFactory::IsReadingListEnabled()) {
It is an issue when GetSyncBridgeForModelType returns invalid WeakPtr. During
startup sync engine will post a call to Bridge::OnSyncStarting, passing a
callback. It will block initialization waiting for bridge to call this callback.
If WeakRef is invalid then this call will never happen.

Here is how to observe this:
- Disable reading list through experimental flag
- Start browser, sign in
- Check datatypes' status on about:sync page.

One way to address this is not to register datatype controller in
profile_sync_components_factory_impl.cc based on experiment value.

Another way is to implement custom ModelTypeController for reading list
overriding  DataTypeController::ReadyForStart method to return false if
experiment is disabled:
https://cs.chromium.org/chromium/src/components/sync/driver/data_type_control...

Here is an example of how it is done for autofill walled datatype:
https://cs.chromium.org/chromium/src/components/autofill/core/browser/autofil...

[Olivier, 2016-11-20 10:32:32]

olivierrobin@chromium.org changed reviewers:
+ sdefresne@chromium.org

[Olivier, 2016-11-20 10:32:33]

https://codereview.chromium.org/2511723002/diff/40001/chrome/browser/sync/chr...
File chrome/browser/sync/chrome_sync_client.cc (right):

https://codereview.chromium.org/2511723002/diff/40001/chrome/browser/sync/chr...
chrome/browser/sync/chrome_sync_client.cc:439: case syncer::READING_LIST:
On 2016/11/20 02:31:58, pavely wrote:
> You can add NOTREACHED() as well as this function shouldn't be called on iOS.

Done.

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

https://codereview.chromium.org/2511723002/diff/40001/components/browser_sync...
components/browser_sync/profile_sync_components_factory_impl.cc:278: #if
defined(OS_IOS)
On 2016/11/20 02:31:58, pavely wrote:
> Yes, this is a correct way to only enable type on supported platform.
>
https://cs.chromium.org/chromium/src/chrome/browser/sync/chrome_sync_client.c...

Acknowledged.

https://codereview.chromium.org/2511723002/diff/40001/ios/chrome/browser/sync...
File ios/chrome/browser/sync/ios_chrome_sync_client.mm (right):

https://codereview.chromium.org/2511723002/diff/40001/ios/chrome/browser/sync...
ios/chrome/browser/sync/ios_chrome_sync_client.mm:341: if
(ReadingListModelFactory::IsReadingListEnabled()) {
On 2016/11/20 02:31:58, pavely wrote:
> It is an issue when GetSyncBridgeForModelType returns invalid WeakPtr. During
> startup sync engine will post a call to Bridge::OnSyncStarting, passing a
> callback. It will block initialization waiting for bridge to call this
callback.
> If WeakRef is invalid then this call will never happen.
> 
> Here is how to observe this:
> - Disable reading list through experimental flag
> - Start browser, sign in
> - Check datatypes' status on about:sync page.
> 
> One way to address this is not to register datatype controller in
> profile_sync_components_factory_impl.cc based on experiment value.
> 
> Another way is to implement custom ModelTypeController for reading list
> overriding  DataTypeController::ReadyForStart method to return false if
> experiment is disabled:
>
https://cs.chromium.org/chromium/src/components/sync/driver/data_type_control...
> 
> Here is an example of how it is done for autofill walled datatype:
>
https://cs.chromium.org/chromium/src/components/autofill/core/browser/autofil...
> 

Problem is that ReadingListService is in ios/chrome/browser at the moment, and
the switch is in 	ios/chrome/browser/chrome_switches.h. 
So I cannot easily check the switch, and if I create a custom
DataTypeController, as it will also be in ios/chrome/browser

+sdefresne, our referent components and dependency to see if he has an idea.

[Olivier, 2016-11-21 15:47:22]

Hi Pavely,

I now use the new Reading list component introduced in
https://codereview.chromium.org/2514333003/

PTAL

[pavely, 2016-11-22 00:33:43]

lgtm

[Olivier, 2016-11-23 08:31:42]

The CQ bit was checked by olivierrobin@chromium.org

[commit-bot: I haz the power, 2016-11-23 08:31:53]

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-11-23 08:35:01]

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

[commit-bot: I haz the power, 2016-11-23 08:35:01]

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_...)
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Olivier, 2016-11-23 09:06:38]

Hi sdefresne,

I had a deps issue (other platform must not depends on components/reading_list)
so I added a components/reading_list::switches.

Could you take a look?

[Olivier, 2016-11-23 09:07:33]

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

[commit-bot: I haz the power, 2016-11-23 09:07:42]

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-11-23 09:10:59]

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

[commit-bot: I haz the power, 2016-11-23 09:10:59]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Olivier, 2016-11-23 09:15:27]

It seems that other platforms still try to depend on ios/web, but there is no
dependency on it.
Should I define the target in a if(is_ios) condition?

[Olivier, 2016-11-23 09:21:59]

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

[commit-bot: I haz the power, 2016-11-23 09:22:10]

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

[Olivier, 2016-11-23 09:23:00]

ios/web is actually not needed. So removing it may solve the issue.
But the analyse problem hint me that I did something wrong with gn. So waiting
for a gn guru LGTM.

[commit-bot: I haz the power, 2016-11-23 09:25:24]

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

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

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

[Olivier, 2016-11-23 09:37:11]

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

[commit-bot: I haz the power, 2016-11-23 09:37: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-11-23 09:53:23]

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

[commit-bot: I haz the power, 2016-11-23 09:53:25]

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

[Olivier, 2016-11-23 09:57:16]

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

[commit-bot: I haz the power, 2016-11-23 09:57:29]

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-11-23 10:15:59]

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

[commit-bot: I haz the power, 2016-11-23 10:16:00]

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...)

[Olivier, 2016-11-23 10:28:52]

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

[commit-bot: I haz the power, 2016-11-23 10:29:02]

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

[sdefresne, 2016-11-23 10:34:01]

On 2016/11/23 09:23:00, Olivier Robin wrote:
> ios/web is actually not needed. So removing it may solve the issue.
> But the analyse problem hint me that I did something wrong with gn. So waiting
> for a gn guru LGTM.

Only targets that are a dependencies of //:gn_all are built, but when parsing a
BUILD.gn file, all the targets are generated and their dependencies loaded. So
it means that if a target depends on //components/reading_list:switches, then
//components/reading_list will be parsed (since it is in the same file) and all
its dependencies loaded (recursively). They won't be put as dependencies of
//:gn_all and won't be build if only gn_all is build, but if ninja is invoked
without any arguments, they will be built. See
https://groups.google.com/a/chromium.org/d/topic/gn-dev/CZJcvrg4c68/discussion
for discussion on this topic.

To come back to your CL, this means that if you want all platform to depend on
//components/reading_list:switches, you have two options:
1. move the target in a separate directory (i.e. make your component a
layered-component),
2. put all other targets that depends on //ios behind "if (is_ios)".

To be inline with the other components, I would recommend using approach number
1. That is give your component the following structure:

components/reading_list/
  core/
    BUILD.gn
    reading_list_switches.h
    reading_list_switches.cc
  ios/
    BUILD.gn
    reading_list_model.h
    ...

This is more consistent with other components and easier to maintain.

[commit-bot: I haz the power, 2016-11-23 11:12:08]

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

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

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

[jif, 2016-11-23 13:19:01]

lgtm, but of course fix what sylvain talked about.

[Olivier, 2016-11-23 13:40:57]

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

[commit-bot: I haz the power, 2016-11-23 13:41:14]

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

[Olivier, 2016-11-23 14:31:37]

I created two subdirectories.
This CL is still downstream compatible.
Sorry for the size inflation.

[commit-bot: I haz the power, 2016-11-23 14:58:25]

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

[commit-bot: I haz the power, 2016-11-23 14:58:26]

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_...)

[Olivier, 2016-11-23 14:59:38]

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

[commit-bot: I haz the power, 2016-11-23 14:59:49]

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-11-23 15:34:59]

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

[commit-bot: I haz the power, 2016-11-23 15:35:01]

Dry run: This issue passed the CQ dry run.

[sdefresne, 2016-11-24 17:35:21]

lgtm

https://codereview.chromium.org/2511723002/diff/200001/components/browser_syn...
File components/browser_sync/DEPS (right):

https://codereview.chromium.org/2511723002/diff/200001/components/browser_syn...
components/browser_sync/DEPS:14: "+components/reading_list",
Please use

  "+components/reading_list/core",

[Olivier, 2016-11-24 17:57:31]

Thanks all for the reviews!

https://codereview.chromium.org/2511723002/diff/200001/components/browser_syn...
File components/browser_sync/DEPS (right):

https://codereview.chromium.org/2511723002/diff/200001/components/browser_syn...
components/browser_sync/DEPS:14: "+components/reading_list",
On 2016/11/24 17:35:21, sdefresne wrote:
> Please use
> 
>   "+components/reading_list/core",

Done.

[Olivier, 2016-11-24 17:57:46]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2016-11-24 17:57:47]

The patchset sent to the CQ was uploaded after l-g-t-m from pavely@chromium.org,
jif@chromium.org, sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/2511723002/#ps220001
(title: "narrow deps")

[commit-bot: I haz the power, 2016-11-24 17:58:27]

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-11-24 18:25:06]

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

[commit-bot: I haz the power, 2016-11-24 18:25:08]

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

[Olivier, 2016-11-24 18:29:42]

The CQ bit was checked by olivierrobin@chromium.org

[commit-bot: I haz the power, 2016-11-24 18:29:59]

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-11-24 19:18:34]

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

[commit-bot: I haz the power, 2016-11-24 19:18:36]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Olivier, 2016-11-24 19:22:34]

The CQ bit was checked by olivierrobin@chromium.org

[commit-bot: I haz the power, 2016-11-24 19:22:54]

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-11-24 19:28:56]

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

[commit-bot: I haz the power, 2016-11-24 19:28:58]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Olivier, 2016-11-24 19:33:47]

The CQ bit was checked by olivierrobin@chromium.org

[commit-bot: I haz the power, 2016-11-24 19:34:02]

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

[Olivier, 2016-11-24 19:41:05]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2016-11-24 19:41:06]

The patchset sent to the CQ was uploaded after l-g-t-m from jif@chromium.org,
sdefresne@chromium.org, pavely@chromium.org
Link to the patchset: https://codereview.chromium.org/2511723002/#ps240001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-24 19:41: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-11-24 20:30:42]

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

[commit-bot: I haz the power, 2016-11-24 20:30:44]

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

[Olivier, 2016-11-24 20:31:26]

The CQ bit was checked by olivierrobin@chromium.org

[commit-bot: I haz the power, 2016-11-24 20:31:43]

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-11-24 21:42:14]

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

[commit-bot: I haz the power, 2016-11-24 21:42:17]

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

[Olivier, 2016-11-24 21:44:17]

The CQ bit was checked by olivierrobin@chromium.org

[commit-bot: I haz the power, 2016-11-24 21:44:33]

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-11-24 22:40:39]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1480023857750650,
"parent_rev": "eebdbe1e4c0382855ba8e24551204619421c41c2", "commit_rev":
"48683f0e223dbb9076fac1636288c84de0031fd6"}

[commit-bot: I haz the power, 2016-11-24 22:41:03]

Description was changed from

==========
Enable RL sync by default

BUG=
==========

to

==========
Enable RL sync by default

BUG=
==========

[commit-bot: I haz the power, 2016-11-24 22:41:04]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-11-24 22:42:42]

Description was changed from

==========
Enable RL sync by default

BUG=
==========

to

==========
Enable RL sync by default

BUG=

Committed: https://crrev.com/0c6cd0ca95b91d4b76fae8fc27a39f404fd756aa
Cr-Commit-Position: refs/heads/master@{#434402}
==========

[commit-bot: I haz the power, 2016-11-24 22:42:44]

Patchset 13 (id:??) landed as
https://crrev.com/0c6cd0ca95b91d4b76fae8fc27a39f404fd756aa
Cr-Commit-Position: refs/heads/master@{#434402}