[Sync] Move thread checking into the ModelSafeWorker interface.

[Sync] Move thread checking into the ModelSafeWorker interface.

Previously, several task runners (UI, DB, FILE) were piped through
several layers of sync code simply to CHECK that the correct thread was
being used. This CL adds ModelSafeWorker::IsOnModelThread() instead.

Note that HISTORY and PASSWORDS still don't have an easy way to perform
this check, but the situation is no worse than before and sets the stage
for future improvement if necessary.

BUG=663125
Committed: https://crrev.com/509cc91f68ef68aebfc761bf6bd282c113a1632e
Cr-Commit-Position: refs/heads/master@{#430684}

[maxbogue, 2016-11-08 01:10:19]

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

[commit-bot: I haz the power, 2016-11-08 01:10:47]

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-08 01:25:55]

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

[commit-bot: I haz the power, 2016-11-08 01:25:56]

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

[maxbogue, 2016-11-08 01:28:30]

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

[commit-bot: I haz the power, 2016-11-08 01:29:06]

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-08 02:38:55]

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

[commit-bot: I haz the power, 2016-11-08 02:38:57]

Dry run: This issue passed the CQ dry run.

[maxbogue, 2016-11-08 03:06:18]

maxbogue@chromium.org changed reviewers:
+ sky@chromium.org, vabr@chromium.org, zea@chromium.org

[maxbogue, 2016-11-08 03:06:19]

PTAL! zea@ for sync, sky@ for history, vabr@ for passwords.

[vabr (Chromium), 2016-11-08 08:32:45]

Passwords component LGTM with the comment below addressed.

Thanks!
Vaclav

https://codereview.chromium.org/2489433002/diff/40001/components/password_man...
File components/password_manager/sync/browser/password_model_worker.cc (right):

https://codereview.chromium.org/2489433002/diff/40001/components/password_man...
components/password_manager/sync/browser/password_model_worker.cc:66: // Can't
do better without modifying PasswordStore.
Please explain the issue better. Ideally, the issue would be described in a bug
in more detail (what needs to be done in PasswordStore, how it relates to the
worker, etc.), and the comment here could just say: TODO(crbug.com/XXX) Can't do
better without modifying PasswordStore.

[sky, 2016-11-08 18:17:00]

LGTM

https://codereview.chromium.org/2489433002/diff/40001/components/history/core...
File components/history/core/browser/history_model_worker.cc (right):

https://codereview.chromium.org/2489433002/diff/40001/components/history/core...
components/history/core/browser/history_model_worker.cc:128: // Can't do better
without modifying HistoryService.
As someone not familiar with this function the comment doesn't make sense. I
recommend removing the comment, or better documenting what you mean. Ideally the
comment would also way what true means.

[Nicolas Zea, 2016-11-08 18:31:04]

Good change. LGTM with a nit (and sky/vabr's comments)

https://codereview.chromium.org/2489433002/diff/40001/components/sync/engine/...
File components/sync/engine/passive_model_worker.cc (right):

https://codereview.chromium.org/2489433002/diff/40001/components/sync/engine/...
components/sync/engine/passive_model_worker.cc:26: NOTREACHED();
nit: maybe add a comment here about why this is invalid to call?

[maxbogue, 2016-11-08 18:41:42]

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

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

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

[maxbogue, 2016-11-08 18:43:23]

Thanks for the quick reviews all!

https://codereview.chromium.org/2489433002/diff/40001/components/history/core...
File components/history/core/browser/history_model_worker.cc (right):

https://codereview.chromium.org/2489433002/diff/40001/components/history/core...
components/history/core/browser/history_model_worker.cc:128: // Can't do better
without modifying HistoryService.
On 2016/11/08 18:17:00, sky wrote:
> As someone not familiar with this function the comment doesn't make sense. I
> recommend removing the comment, or better documenting what you mean. Ideally
the
> comment would also way what true means.

Improved; let me know if anything is still unclear!

https://codereview.chromium.org/2489433002/diff/40001/components/password_man...
File components/password_manager/sync/browser/password_model_worker.cc (right):

https://codereview.chromium.org/2489433002/diff/40001/components/password_man...
components/password_manager/sync/browser/password_model_worker.cc:66: // Can't
do better without modifying PasswordStore.
On 2016/11/08 08:32:45, vabr (Chromium) wrote:
> Please explain the issue better. Ideally, the issue would be described in a
bug
> in more detail (what needs to be done in PasswordStore, how it relates to the
> worker, etc.), and the comment here could just say: TODO(crbug.com/XXX) Can't
do
> better without modifying PasswordStore.

I've improved the comment to hopefully be clearer about what's going on. I'm not
filing a bug because nobody actually intends to fix this; it's sat untouched in
sync code for years and we're beginning to move types to the new USS style,
which doesn't use this interface.

https://codereview.chromium.org/2489433002/diff/40001/components/sync/engine/...
File components/sync/engine/passive_model_worker.cc (right):

https://codereview.chromium.org/2489433002/diff/40001/components/sync/engine/...
components/sync/engine/passive_model_worker.cc:26: NOTREACHED();
On 2016/11/08 18:31:04, Nicolas Zea wrote:
> nit: maybe add a comment here about why this is invalid to call?

Done.

[maxbogue, 2016-11-08 18:43:36]

The CQ bit was unchecked by maxbogue@chromium.org

[maxbogue, 2016-11-08 18:43:40]

The CQ bit was checked by maxbogue@chromium.org

[maxbogue, 2016-11-08 18:43:41]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
zea@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2489433002/#ps60001
(title: "Improve/add comments.")

[commit-bot: I haz the power, 2016-11-08 18:43:47]

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

[vabr (Chromium), 2016-11-08 19:24:59]

Still LGTM, thanks!

https://codereview.chromium.org/2489433002/diff/40001/components/password_man...
File components/password_manager/sync/browser/password_model_worker.cc (right):

https://codereview.chromium.org/2489433002/diff/40001/components/password_man...
components/password_manager/sync/browser/password_model_worker.cc:66: // Can't
do better without modifying PasswordStore.
On 2016/11/08 18:43:23, maxbogue wrote:
> On 2016/11/08 08:32:45, vabr (Chromium) wrote:
> > Please explain the issue better. Ideally, the issue would be described in a
> bug
> > in more detail (what needs to be done in PasswordStore, how it relates to
the
> > worker, etc.), and the comment here could just say: TODO(crbug.com/XXX)
Can't
> do
> > better without modifying PasswordStore.
> 
> I've improved the comment to hopefully be clearer about what's going on. I'm
not
> filing a bug because nobody actually intends to fix this; it's sat untouched
in
> sync code for years and we're beginning to move types to the new USS style,
> which doesn't use this interface.

Acknowledged. If there was a need to implement this in PasswordStore, do let me
know, it should not be a problem to add this. Other than that, the new comment
looks clear enough to me and does not require a bug.

[commit-bot: I haz the power, 2016-11-08 19:31:00]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-08 19:40:26]

Description was changed from

==========
[Sync] Move thread checking into the ModelSafeWorker interface.

Previously, several task runners (UI, DB, FILE) were piped through
several layers of sync code simply to CHECK that the correct thread was
being used. This CL adds ModelSafeWorker::IsOnModelThread() instead.

Note that HISTORY and PASSWORDS still don't have an easy way to perform
this check, but the situation is no worse than before and sets the stage
for future improvement if necessary.

BUG=663125
==========

to

==========
[Sync] Move thread checking into the ModelSafeWorker interface.

Previously, several task runners (UI, DB, FILE) were piped through
several layers of sync code simply to CHECK that the correct thread was
being used. This CL adds ModelSafeWorker::IsOnModelThread() instead.

Note that HISTORY and PASSWORDS still don't have an easy way to perform
this check, but the situation is no worse than before and sets the stage
for future improvement if necessary.

BUG=663125

Committed: https://crrev.com/509cc91f68ef68aebfc761bf6bd282c113a1632e
Cr-Commit-Position: refs/heads/master@{#430684}
==========

[commit-bot: I haz the power, 2016-11-08 19:40:34]

Patchset 4 (id:??) landed as
https://crrev.com/509cc91f68ef68aebfc761bf6bd282c113a1632e
Cr-Commit-Position: refs/heads/master@{#430684}

[sky, 2016-11-08 23:06:50]

https://codereview.chromium.org/2489433002/diff/60001/components/history/core...
File components/history/core/browser/history_model_worker.cc (right):

https://codereview.chromium.org/2489433002/diff/60001/components/history/core...
components/history/core/browser/history_model_worker.cc:129: // history DB
thread. Since it doesn't, just return true to bypass a CHECK in
You actually could add such a check, but there is work going on to move
HistoryService to a TaskScheduler, which would mean said check wouldn't work.
That bug is here: https://codereview.chromium.org/2486603003/ . I'm adding you
to that bug in case there are issues with it and sync.