Get ModelTypeStoreFactory for reading list from ProfileSyncService

Get ModelTypeStoreFactory for reading list from ProfileSyncService

This CL switches ReadingListModelFactory to using ProfileSyncService's factory
for ModelTypeStore. This allows ReadingList to share store with other types.

R=olivierrobin@chromium.org
BUG=664920
Review-Url: https://codereview.chromium.org/2663553003
Cr-Commit-Position: refs/heads/master@{#447390}
Committed: https://chromium.googlesource.com/chromium/src/+/9d905484261b3bf9205c65d95b86f306914a8581

[pavely, 2017-01-27 23:43:59]

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

[commit-bot: I haz the power, 2017-01-27 23:44:51]

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-01-27 23:52:54]

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

[commit-bot: I haz the power, 2017-01-27 23:52:58]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[pavely, 2017-01-27 23:57:58]

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

[commit-bot: I haz the power, 2017-01-27 23:58:28]

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-01-28 00:06:12]

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

[commit-bot: I haz the power, 2017-01-28 00:06:13]

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

[pavely, 2017-01-28 00:23:20]

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

[commit-bot: I haz the power, 2017-01-28 00:24: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-01-28 00:30:36]

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

[commit-bot: I haz the power, 2017-01-28 00:30:36]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[pavely, 2017-01-31 02:00:56]

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

[commit-bot: I haz the power, 2017-01-31 02:01:22]

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-01-31 02:09:40]

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

[commit-bot: I haz the power, 2017-01-31 02:09:42]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[pavely, 2017-01-31 02:17:44]

Patchset #4 (id:60001) has been deleted

[pavely, 2017-01-31 02:19:18]

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

[commit-bot: I haz the power, 2017-01-31 02:19:37]

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-01-31 03:05:55]

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

[commit-bot: I haz the power, 2017-01-31 03:05:55]

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, 2017-01-31 12:26:19]

LGTM, but please wait until we determine the correct migration requirements.

[pavely, 2017-01-31 18:42:54]

The CQ bit was checked by pavely@chromium.org

[commit-bot: I haz the power, 2017-01-31 18:43: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, 2017-01-31 19:17:40]

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

[commit-bot: I haz the power, 2017-01-31 19:17:41]

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, 2017-01-31 20:45:55]

The CQ bit was checked by olivierrobin@chromium.org

[commit-bot: I haz the power, 2017-01-31 20:46:15]

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-01-31 21:34:40]

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

[commit-bot: I haz the power, 2017-01-31 21:34:42]

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

[pavely, 2017-01-31 22:13:45]

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

[commit-bot: I haz the power, 2017-01-31 22:14:23]

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

[pavely, 2017-01-31 22:17:04]

Description was changed from

==========
Get ModelTypeStoreFactory for reading list from ProfileSyncService

This CL switches ReadingListModelFactory to using ProfileSyncService's factory
for ModelTypeStore. This allows ReadingList to share store with other types.

R=olivierrobin@chromium.org
BUG=664920
==========

to

==========
Get ModelTypeStoreFactory for reading list from ProfileSyncService

This CL switches ReadingListModelFactory to using ProfileSyncService's factory
for ModelTypeStore. This allows ReadingList to share store with other types.

R=olivierrobin@chromium.org
BUG=664920
==========

[pavely, 2017-01-31 22:17:04]

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

[pavely, 2017-01-31 22:25:08]

+rohitrao@ Could you review changes in tab_model_order_controller_unittest.mm?

My change broke TabModelOrderControllerTest. The issue was that
TabModelSyncedWindowDelegatesGetter::GetSyncedWindowDelegates() tried to
dereference GetApplicationContext()->GetChromeBrowserStateManager() which for
this test wasn't setup and thus returned nullptr.
I fixed it in PS#5 by adding IOSChromeScopedTestingChromeBrowserStateManager to
the test.

[commit-bot: I haz the power, 2017-01-31 23:43:20]

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

[commit-bot: I haz the power, 2017-01-31 23:43:21]

Dry run: This issue passed the CQ dry run.

[rohitrao (ping after 24h), 2017-02-01 00:04:39]

On 2017/01/31 22:25:08, pavely wrote:
> +rohitrao@ Could you review changes in tab_model_order_controller_unittest.mm?
> 
> My change broke TabModelOrderControllerTest. The issue was that
> TabModelSyncedWindowDelegatesGetter::GetSyncedWindowDelegates() tried to
> dereference GetApplicationContext()->GetChromeBrowserStateManager() which for
> this test wasn't setup and thus returned nullptr.
> I fixed it in PS#5 by adding IOSChromeScopedTestingChromeBrowserStateManager
to
> the test.

Adding IOSChromeScopedTestingChromeBrowserStateManager lgtm but why did this CL
trigger that error?  Why is the new code causing GetSyncedWindowDelegates() to
be called at all?

[rohitrao (ping after 24h), 2017-02-01 00:05:44]

Also please consider having sdefresne@ review the keyedservice and circular
dependency changes.

[pavely, 2017-02-01 00:36:34]

On 2017/02/01 00:04:39, rohitrao wrote:
> On 2017/01/31 22:25:08, pavely wrote:
> > +rohitrao@ Could you review changes in
tab_model_order_controller_unittest.mm?
> > 
> > My change broke TabModelOrderControllerTest. The issue was that
> > TabModelSyncedWindowDelegatesGetter::GetSyncedWindowDelegates() tried to
> > dereference GetApplicationContext()->GetChromeBrowserStateManager() which
for
> > this test wasn't setup and thus returned nullptr.
> > I fixed it in PS#5 by adding IOSChromeScopedTestingChromeBrowserStateManager
> to
> > the test.
> 
> Adding IOSChromeScopedTestingChromeBrowserStateManager lgtm but why did this
CL
> trigger that error?  Why is the new code causing GetSyncedWindowDelegates() to
> be called at all?

My change caused initialization of ProfileSyncService which (indirectly) causes
creation of IOSChromeLocalSessionEventRouter. It in turn registers itself as an
observer of GlobalWebStateEventTracker. Before my change notifications in
GlobalWebStateEventTracker didn't execute any code. After my change they end up
invoking IOSChromeLocalSessionEventRouter with the following stack:
TabModelSyncedWindowDelegatesGetter::GetSyncedWindowDelegates()
TabModelSyncedWindowDelegatesGetter::FindById(int)
IOSChromeSyncedTabDelegate::ShouldSync(sync_sessions::SyncSessionsClient*)
IOSChromeLocalSessionEventRouter::OnWebStateChange(web::WebState*)
IOSChromeLocalSessionEventRouter::WebStateDestroyed(web::WebState*)
web::GlobalWebStateEventTracker::WebStateDestroyed(web::WebState*)

[pavely, 2017-02-01 00:37:15]

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

[pavely, 2017-02-01 00:41:47]

+sdefresne@: Could you review how I resolved circular include between
i/c/b/reading_list and i/c/b/sync. I added reading_list in
allow_circular_includes_from in i/c/b/sync/BUILD.gn. I followed approach used
for passwords.

I'm going to land this change as is. Let me know if you thinks dependencies
should be organized differently and I'll address it in separate CL.

[pavely, 2017-02-01 00:42:07]

The CQ bit was checked by pavely@chromium.org

[pavely, 2017-02-01 00:42:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
olivierrobin@chromium.org
Link to the patchset: https://codereview.chromium.org/2663553003/#ps100001
(title: "Fix TabModelOrderControllerTest")

[commit-bot: I haz the power, 2017-02-01 00:42:40]

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-01 00:51:49]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1485909727996070,
"parent_rev": "09cee5005baf9f8a5f216750be0688fca6f73f92", "commit_rev":
"9d905484261b3bf9205c65d95b86f306914a8581"}

[commit-bot: I haz the power, 2017-02-01 00:52:25]

Description was changed from

==========
Get ModelTypeStoreFactory for reading list from ProfileSyncService

This CL switches ReadingListModelFactory to using ProfileSyncService's factory
for ModelTypeStore. This allows ReadingList to share store with other types.

R=olivierrobin@chromium.org
BUG=664920
==========

to

==========
Get ModelTypeStoreFactory for reading list from ProfileSyncService

This CL switches ReadingListModelFactory to using ProfileSyncService's factory
for ModelTypeStore. This allows ReadingList to share store with other types.

R=olivierrobin@chromium.org
BUG=664920

Review-Url: https://codereview.chromium.org/2663553003
Cr-Commit-Position: refs/heads/master@{#447390}
Committed:
https://chromium.googlesource.com/chromium/src/+/9d905484261b3bf9205c65d95b86...
==========

[commit-bot: I haz the power, 2017-02-01 00:52:26]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/9d905484261b3bf9205c65d95b86...