Reduce usage of MessageLoopProxy in base/

Reduce the dependency on MessageLoopProxy in base/ as much as possible

Specifically:
- Replace MessageLoopProxy::current() with ThreadTaskRunnerHandle::Get()
- Replace MessageLoopProxy with Scoped/SingleThreadTaskRunner as appropriate

In support of this, there were two renames that occurred
- Rename RefCountedDeleteOnMessageLoop to RefCountedDeleteOnTaskRunner, to better reflect what it does
- Rename PrefMember::MoveToThread to PrefMember::UseAlternateTaskRunner

BUG=391045

[Ryan Sleevi, 2014-07-03 02:16:02]

ajwong because of his G+ +1 ;)

But seriously, don't review this until after holidays...

[awong, 2014-07-07 21:50:00]

rubber stamp LGTM w/o nit

Die MLP! Die!

https://codereview.chromium.org/369703003/diff/1/base/memory/ref_counted_dele...
File base/memory/ref_counted_delete_on_task_runner.h (right):

https://codereview.chromium.org/369703003/diff/1/base/memory/ref_counted_dele...
base/memory/ref_counted_delete_on_task_runner.h:36:
RefCountedDeleteOnTaskRunner(
explicit?

[Ryan Sleevi, 2014-07-10 20:41:52]

The CQ bit was checked by rsleevi@chromium.org

[commit-bot: I haz the power, 2014-07-10 20:42:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rsleevi@chromium.org/369703003/60001

[Ryan Sleevi, 2014-07-10 20:59:30]

The CQ bit was unchecked by rsleevi@chromium.org

[Ryan Sleevi, 2014-07-10 21:04:59]

darin for chrome/ rubberstamp
blundell for components/ rubberstamp

(Both of these were affected by the renaming mentioned in the CL description)

[darin (slow to review), 2014-07-10 22:12:06]

https://codereview.chromium.org/369703003/diff/60001/base/prefs/pref_member.h
File base/prefs/pref_member.h (right):

https://codereview.chromium.org/369703003/diff/60001/base/prefs/pref_member.h...
base/prefs/pref_member.h:68: void MoveToTaskRunner(
Hmm, MoveToThread seems like a better name. The concept of moving something to a
TaskRunner is quite awkward. The previous code used MLP as a proxy for
identifying a thread. Now you are just using a TaskRunner for that purpose.
Otherwise, nothing is really changing, right?

https://codereview.chromium.org/369703003/diff/60001/base/prefs/pref_member.h...
base/prefs/pref_member.h:69: const scoped_refptr<base::SequencedTaskRunner>&
task_runner);
shouldn't this be a SingleThreadTaskRunner given the IsOnCorrectThread
implementation?

https://codereview.chromium.org/369703003/diff/60001/components/signin/core/b...
File components/signin/core/browser/webdata/token_web_data.cc (right):

https://codereview.chromium.org/369703003/diff/60001/components/signin/core/b...
components/signin/core/browser/webdata/token_web_data.cc:18: : public
base::RefCountedDeleteOnTaskRunner<TokenWebDataBackend> {
I wonder if "Via" wouldn't be better than "On" here.

[Ryan Sleevi, 2014-07-10 22:46:37]

https://codereview.chromium.org/369703003/diff/60001/base/prefs/pref_member.h
File base/prefs/pref_member.h (right):

https://codereview.chromium.org/369703003/diff/60001/base/prefs/pref_member.h...
base/prefs/pref_member.h:68: void MoveToTaskRunner(
On 2014/07/10 22:12:05, darin wrote:
> Hmm, MoveToThread seems like a better name. The concept of moving something to
a
> TaskRunner is quite awkward. The previous code used MLP as a proxy for
> identifying a thread. Now you are just using a TaskRunner for that purpose.
> Otherwise, nothing is really changing, right?

Correct, but with the caveat explained (below) that the specific concept of a
Thread is a more restrictive API than the generic concept of a "Task Runner",
for reasons below.

https://codereview.chromium.org/369703003/diff/60001/base/prefs/pref_member.h...
base/prefs/pref_member.h:69: const scoped_refptr<base::SequencedTaskRunner>&
task_runner);
On 2014/07/10 22:12:05, darin wrote:
> shouldn't this be a SingleThreadTaskRunner given the IsOnCorrectThread
> implementation?

This is not actually coupled to a particular thread, though. That is, many of
our "UI thread"-type dependencies are expressions of "the same task runner I was
on before", which the caller supplies.

The (generally speaking) only time we should be using SingleThreadTaskRunner, as
opposed to SequencedTaskRunner, is when we're expressly making use of
thread-local state - whether through ThreadLocalStorage or through some API
call.

Prefs doesn't have this TLS dependency, hence the 'broadening' of the acceptable
API to SequencedTaskRunner.

Note that a SequencedTaskRunner guarantees that two tasks as part of the same
sequence will not run in parallel, so this has the same implied semantics that
we say when we talk about "threads" are still present (in terms of single,
ordered steps of execution). The only change is with respect to thread-local
storage.

[darin (slow to review), 2014-07-10 23:01:47]

On Thu, Jul 10, 2014 at 3:46 PM, <rsleevi@chromium.org> wrote:

>
> https://codereview.chromium.org/369703003/diff/60001/base/
> prefs/pref_member.h
> File base/prefs/pref_member.h (right):
>
> https://codereview.chromium.org/369703003/diff/60001/base/
> prefs/pref_member.h#newcode68
> base/prefs/pref_member.h:68: void MoveToTaskRunner(
> On 2014/07/10 22:12:05, darin wrote:
>
>> Hmm, MoveToThread seems like a better name. The concept of moving
>>
> something to a
>
>> TaskRunner is quite awkward. The previous code used MLP as a proxy for
>> identifying a thread. Now you are just using a TaskRunner for that
>>
> purpose.
>
>> Otherwise, nothing is really changing, right?
>>
>
> Correct, but with the caveat explained (below) that the specific concept
> of a Thread is a more restrictive API than the generic concept of a
> "Task Runner", for reasons below.


Yup. Maybe SetTaskRunner or UseAlternateTaskRunner would be a better name
then? I kind of prefer the latter. It could have a comment re-inforcing the
idea that by default a PrefMember uses that TaskRunner of the thread on
which it was constructed.



>
>
> https://codereview.chromium.org/369703003/diff/60001/base/
> prefs/pref_member.h#newcode69
> base/prefs/pref_member.h:69: const
> scoped_refptr<base::SequencedTaskRunner>& task_runner);
> On 2014/07/10 22:12:05, darin wrote:
>
>> shouldn't this be a SingleThreadTaskRunner given the IsOnCorrectThread
>> implementation?
>>
>
> This is not actually coupled to a particular thread, though. That is,
> many of our "UI thread"-type dependencies are expressions of "the same
> task runner I was on before", which the caller supplies.
>
> The (generally speaking) only time we should be using
> SingleThreadTaskRunner, as opposed to SequencedTaskRunner, is when we're
> expressly making use of thread-local state - whether through
> ThreadLocalStorage or through some API call.
>
> Prefs doesn't have this TLS dependency, hence the 'broadening' of the
> acceptable API to SequencedTaskRunner.
>
> Note that a SequencedTaskRunner guarantees that two tasks as part of the
> same sequence will not run in parallel, so this has the same implied
> semantics that we say when we talk about "threads" are still present (in
> terms of single, ordered steps of execution). The only change is with
> respect to thread-local storage.
>

Yup, I get all of that. I don't see how it addresses my point though. My
concern is with the IsOnCorrectThread assertion--that it might not permit
any more than a single thread to be used. However, I see now that that is
just implemented by calling the task runner's BelongsToCurrentThread()
method. OK.

-Darin



>
> https://codereview.chromium.org/369703003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ryan Sleevi, 2014-07-10 23:23:23]

On 2014/07/10 23:01:47, darin wrote:
> Yup. Maybe SetTaskRunner or UseAlternateTaskRunner would be a better name
> then? I kind of prefer the latter. It could have a comment re-inforcing the
> idea that by default a PrefMember uses that TaskRunner of the thread on
> which it was constructed.

UseAlternateTaskRunner SGTM. Will update (which is largely the //components and
//chrome bits)

> Yup, I get all of that. I don't see how it addresses my point though. My
> concern is with the IsOnCorrectThread assertion--that it might not permit
> any more than a single thread to be used. However, I see now that that is
> just implemented by calling the task runner's BelongsToCurrentThread()
> method. OK.
> 
> -Darin

Ah. Missed that this was your concern. Yeah, IsOnCurrentThread() was safe for
any/all task runners, in as much as it makes sure it's on the same task runner
it was before.

[darin (slow to review), 2014-07-10 23:33:08]

On 2014/07/10 23:23:23, Ryan Sleevi wrote:
> On 2014/07/10 23:01:47, darin wrote:
> > Yup. Maybe SetTaskRunner or UseAlternateTaskRunner would be a better name
> > then? I kind of prefer the latter. It could have a comment re-inforcing the
> > idea that by default a PrefMember uses that TaskRunner of the thread on
> > which it was constructed.
> 
> UseAlternateTaskRunner SGTM. Will update (which is largely the //components
and
> //chrome bits)
> 
> > Yup, I get all of that. I don't see how it addresses my point though. My
> > concern is with the IsOnCorrectThread assertion--that it might not permit
> > any more than a single thread to be used. However, I see now that that is
> > just implemented by calling the task runner's BelongsToCurrentThread()
> > method. OK.
> > 
> > -Darin
> 
> Ah. Missed that this was your concern. Yeah, IsOnCurrentThread() was safe for
> any/all task runners, in as much as it makes sure it's on the same task runner
> it was before.

LGTM w/ that change.

[darin (slow to review), 2014-07-10 23:33:11]

On 2014/07/10 23:23:23, Ryan Sleevi wrote:
> On 2014/07/10 23:01:47, darin wrote:
> > Yup. Maybe SetTaskRunner or UseAlternateTaskRunner would be a better name
> > then? I kind of prefer the latter. It could have a comment re-inforcing the
> > idea that by default a PrefMember uses that TaskRunner of the thread on
> > which it was constructed.
> 
> UseAlternateTaskRunner SGTM. Will update (which is largely the //components
and
> //chrome bits)
> 
> > Yup, I get all of that. I don't see how it addresses my point though. My
> > concern is with the IsOnCorrectThread assertion--that it might not permit
> > any more than a single thread to be used. However, I see now that that is
> > just implemented by calling the task runner's BelongsToCurrentThread()
> > method. OK.
> > 
> > -Darin
> 
> Ah. Missed that this was your concern. Yeah, IsOnCurrentThread() was safe for
> any/all task runners, in as much as it makes sure it's on the same task runner
> it was before.

LGTM w/ that change.

[blundell, 2014-07-11 06:35:12]

//components LGTM

[Ryan Sleevi, 2014-07-11 06:36:28]

The CQ bit was checked by rsleevi@chromium.org

[commit-bot: I haz the power, 2014-07-11 06:37:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rsleevi@chromium.org/369703003/80001

[commit-bot: I haz the power, 2014-07-11 11:09:51]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-07-11 12:21:26]

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

[commit-bot: I haz the power, 2014-07-11 12:21:28]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[Ryan Sleevi, 2014-08-20 06:01:07]

The CQ bit was checked by rsleevi@chromium.org

[commit-bot: I haz the power, 2014-08-20 06:01:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rsleevi@chromium.org/369703003/140001

[commit-bot: I haz the power, 2014-08-20 09:57:42]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-20 10:09:23]

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

[commit-bot: I haz the power, 2014-08-20 10:09:25]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ryan Sleevi, 2014-08-20 22:12:49]

The CQ bit was checked by rsleevi@chromium.org

[commit-bot: I haz the power, 2014-08-20 22:13:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rsleevi@chromium.org/369703003/180001

[commit-bot: I haz the power, 2014-08-21 01:45:04]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-21 01:47:31]

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

[commit-bot: I haz the power, 2014-08-21 01:47:33]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[Ryan Sleevi, 2014-08-21 02:35:09]

The CQ bit was checked by rsleevi@chromium.org

[commit-bot: I haz the power, 2014-08-21 02:36:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rsleevi@chromium.org/369703003/180001

[commit-bot: I haz the power, 2014-08-21 04:14:55]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-21 05:18:11]

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

[commit-bot: I haz the power, 2014-08-21 05:18:13]

Try jobs failed on following builders:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ryan Sleevi, 2014-08-21 06:42:23]

The CQ bit was checked by rsleevi@chromium.org

[commit-bot: I haz the power, 2014-08-21 06:43:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rsleevi@chromium.org/369703003/200001

[commit-bot: I haz the power, 2014-08-21 13:57:41]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/44686)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/49868)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-21 14:01:58]

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

[commit-bot: I haz the power, 2014-08-21 14:01:59]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[blundell, 2015-08-03 16:03:09]

Is this CL still active?

[blundell, 2015-08-19 13:21:42]

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