New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 867801 link

Starred by 2 users
Status: Started
Owner:
jkrcal@chromium.org
Cc:
mastiz@chromium.org
yn...@vivaldi.com
jkrcal@chromium.org
█ jul...@vivaldi.com
█ tschumann@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Task

Blocked on:
issue 855010
issue 882777


Sign in to add a comment

Most ModelTypeController instances need no posting of tasks

Project Member Reported by mastiz@chromium.org, Jul 26 
For datatypes that live in the UI thread, ModelTypeController should directly talk to the delegate without the need to PostTask() to the very same UI thread.

This is simpler conceptually, avoids an unnecessary intermediate state (which required RunUntilIdle() in some tests).

Once a proxy delegate object is factored out (blocking bug), the solution here would be to introduce another delegate that:
1. Forwards all the calls to the real delegate (which the controller cannot uniquely own).
2. Does not need a delegate provider and can directly take a raw pointer to the real delegate.

Comment 1 by mastiz@chromium.org, Jul 26

Summary: Most ModelTypeController instances need no posting of tasks (was: Most ModelTypeController instances need to posting of tasks)
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 1 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1f26c180b25af04d85e86a7ba4bfafee3aea59a6

commit 1f26c180b25af04d85e86a7ba4bfafee3aea59a6
Author: Mikel Astiz <mastiz@chromium.org>
Date: Wed Aug 01 05:04:29 2018

Introduce ConfigureContext struct

This struct allows easy plumbing of fields that are representative of
why a datatype is running. This avoids the need to inject SyncClient
in multiple places, which is undesirable because it carries over a long
list of dependencies, including a subtle dependency cycle in some cases
(DeviceInfo) that needs a workaround (the controller's delegate must be
obtained lazily).

We plan to extend this struct with new state information in follow-up
patches.

Bug:  866814 ,867801
Change-Id: I35142c3a04f2f5abba22a5ec23d475c8da8bd292
Reviewed-on: https://chromium-review.googlesource.com/1154532
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579705}
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/autofill/core/browser/autofill_wallet_data_type_controller_unittest.cc
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/browser_sync/profile_sync_components_factory_impl.cc
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/browser_sync/profile_sync_service_unittest.cc
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/history/core/browser/typed_url_model_type_controller.cc
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/history/core/browser/typed_url_model_type_controller.h
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/search_engines/search_engine_data_type_controller_unittest.cc
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/sync/BUILD.gn
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/sync/driver/async_directory_type_controller.cc
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/sync/driver/async_directory_type_controller.h
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/sync/driver/async_directory_type_controller_mock.cc
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/sync/driver/async_directory_type_controller_mock.h
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/sync/driver/async_directory_type_controller_unittest.cc
[add] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/sync/driver/configure_context.h
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/sync/driver/data_type_controller.h
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/sync/driver/data_type_manager.h
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/sync/driver/data_type_manager_impl.cc
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/sync/driver/data_type_manager_impl.h
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/sync/driver/data_type_manager_impl_unittest.cc
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/sync/driver/data_type_manager_mock.cc
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/sync/driver/data_type_manager_mock.h
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/sync/driver/fake_data_type_controller.cc
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/sync/driver/fake_data_type_controller.h
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/sync/driver/frontend_data_type_controller.cc
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/sync/driver/frontend_data_type_controller.h
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/sync/driver/frontend_data_type_controller_mock.cc
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/sync/driver/frontend_data_type_controller_mock.h
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/sync/driver/frontend_data_type_controller_unittest.cc
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/sync/driver/model_association_manager.cc
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/sync/driver/model_association_manager.h
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/sync/driver/model_association_manager_unittest.cc
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/sync/driver/model_type_controller.cc
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/sync/driver/model_type_controller.h
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/sync/driver/model_type_controller_unittest.cc
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/sync/driver/proxy_data_type_controller.cc
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/sync/driver/proxy_data_type_controller.h
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/sync_bookmarks/bookmark_data_type_controller_unittest.cc
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/sync_sessions/session_data_type_controller_unittest.cc
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/sync_sessions/session_model_type_controller.cc
[modify] https://crrev.com/1f26c180b25af04d85e86a7ba4bfafee3aea59a6/components/sync_sessions/session_model_type_controller.h
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 2 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f85f820a29239c4e2896b7250c289a0e4585b0ce

commit f85f820a29239c4e2896b7250c289a0e4585b0ce
Author: Mikel Astiz <mastiz@chromium.org>
Date: Thu Aug 02 15:24:28 2018

Avoid thread hopping in ModelTypeController tests

We do this for the control methods to the delegate to simplify tests,
by migrating away from ProxyModelTypeControllerDelegate.

Instead, ForwardingModelTypeControllerDelegate is introduced, currently
used in tests only but with the goal to be adopted more broadly (see
crbug.com/867801).

A proxy processor is still in use, and that still requires some nested
loops, but that's left out for future patches.

Bug:  855010 ,867801
Change-Id: I1933355fee2093bf006830cde334c644f4391020
Reviewed-on: https://chromium-review.googlesource.com/1160642
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580189}
[modify] https://crrev.com/f85f820a29239c4e2896b7250c289a0e4585b0ce/components/sync/BUILD.gn
[modify] https://crrev.com/f85f820a29239c4e2896b7250c289a0e4585b0ce/components/sync/driver/model_type_controller_unittest.cc
[add] https://crrev.com/f85f820a29239c4e2896b7250c289a0e4585b0ce/components/sync/model_impl/forwarding_model_type_controller_delegate.cc
[add] https://crrev.com/f85f820a29239c4e2896b7250c289a0e4585b0ce/components/sync/model_impl/forwarding_model_type_controller_delegate.h

Comment 4 by jkrcal@chromium.org, Aug 9

Cc: yn...@vivaldi.com
This bug is related to https://chromium-review.googlesource.com/c/chromium/src/+/1167051, proposed by yngve@vivaldi.com. Based on the discussion, that CL is on hold and should be replaced by:

 a) Fixing this bug (at least for DEVICE_INFO but we will do it for all the data types that live on the UI thread).
 b) Replacing "if (local_info == nullptr)" by "DCHECK(local_info)".

We've agreed that it makes sense that a) gets fixed by the Chrome team. 
Mikel, will you address this bug (in September, after coming from vacation) or should I aim at doing it?

Yngve, maybe you could afterwards re-purpose your CL to do b)? What do you think?

Comment 5 by yn...@vivaldi.com, Aug 10

Cc: jul...@vivaldi.com
That should probably work OK

Comment 6 by mastiz@chromium.org, Sep 11

Blockedon: 882777

Comment 7 by mastiz@chromium.org, Oct 24

Labels: sync-fixit-2018q4

Comment 8 by mastiz@chromium.org, Oct 24

Labels: -Pri-3 Pri-2

Comment 9 by jkrcal@chromium.org, Nov 20

Owner: jkrcal@chromium.org
Status: Started (was: Available)
Project Member

Comment 10 by bugdroid1@chromium.org, Nov 22 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4e2ea188dc6fdb21c332d32f7cfc870202955015

commit 4e2ea188dc6fdb21c332d32f7cfc870202955015
Author: Jan Krcal <jkrcal@chromium.org>
Date: Thu Nov 22 17:22:31 2018

[Device info sync] Construct the controller in ProfileSyncService

This CL moves the construction of the sync controller for device info
directly into ProfileSyncService that owns the DeviceInfoSyncBridge.
This (a) heavily simplifies how the controller gets access to its
delegate (removing the accessor from SyncClients) and (b) allows to use
the forwarding delegate instead of the proxy delegate.

This CL is a preparation for using the simpler forwarding delegate for
all sync types that run on the UI sequence.

This CL also resolves a TODO by making it clear that DEVICE_INFO cannot
get disabled via the command-line switch.

Bug: 867801
Change-Id: Iea84d818613476f40d89eebd73266aa24d028fb6
Reviewed-on: https://chromium-review.googlesource.com/c/1348092
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610448}
[modify] https://crrev.com/4e2ea188dc6fdb21c332d32f7cfc870202955015/chrome/browser/sync/chrome_sync_client.cc
[modify] https://crrev.com/4e2ea188dc6fdb21c332d32f7cfc870202955015/components/browser_sync/profile_sync_components_factory_impl.cc
[modify] https://crrev.com/4e2ea188dc6fdb21c332d32f7cfc870202955015/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/4e2ea188dc6fdb21c332d32f7cfc870202955015/components/browser_sync/profile_sync_service.h
[modify] https://crrev.com/4e2ea188dc6fdb21c332d32f7cfc870202955015/components/browser_sync/profile_sync_service_autofill_unittest.cc
[modify] https://crrev.com/4e2ea188dc6fdb21c332d32f7cfc870202955015/ios/chrome/browser/sync/ios_chrome_sync_client.mm
[modify] https://crrev.com/4e2ea188dc6fdb21c332d32f7cfc870202955015/ios/web_view/internal/sync/web_view_sync_client.mm

Comment 11 by jkrcal@chromium.org, Yesterday (46 hours ago) 

This is close to fixing, I have a chain of CLs to land and this is done (still may take a few weeks as there is no push in this area and I have other more urgent tasks).
►

Sign in to add a comment