base: Remove most uses of MessageLoopProxy

base: Remove most uses of MessageLoopProxy

Replace most usage of MessageLoopProxy under base/ with SingleThreadTaskRunner
and ThreadTaskRunnerHandle (excluding the implementation of MessageLoopProxy
itself which will removed later).

This patch was mostly autogenerated with
https://codereview.chromium.org/1010073002.

Depends on https://codereview.chromium.org/1086733002/.

BUG=465354
TBR=nkostylev@chromium.org,pkasting@chromium.org,pauljensen@chromium.org

Committed: https://crrev.com/054861d0934c663de3cea933e1d65fd8319856a6
Cr-Commit-Position: refs/heads/master@{#327755}

[Sami, 2015-04-21 19:10:51]

skyostil@chromium.org changed reviewers:
+ danakj@chromium.org

[Sami, 2015-04-21 19:12:47]

Dana, would you be up to reviewing this since you've already got some context? I
understand if you don't want to see anything like this after the cc/ patch, so
feel free to tell me to find someone else :)

[danakj, 2015-04-21 20:16:31]

Sorry for the tons of vaguely related "while you're here clean things up" nits.

A couple more "explain things to me" about lifetimes tho too.

https://codereview.chromium.org/1100773004/diff/1/base/chromeos/memory_pressu...
File base/chromeos/memory_pressure_monitor_chromeos.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/chromeos/memory_pressu...
base/chromeos/memory_pressure_monitor_chromeos.cc:10: #include
"base/message_loop/message_loop.h"
still needed?

https://codereview.chromium.org/1100773004/diff/1/base/files/file_path_watcher.h
File base/files/file_path_watcher.h (right):

https://codereview.chromium.org/1100773004/diff/1/base/files/file_path_watche...
base/files/file_path_watcher.h:66: const
scoped_refptr<base::SingleThreadTaskRunner>& task_runner) {
nit: pass by value and .Pass() it into task_runner_

https://codereview.chromium.org/1100773004/diff/1/base/files/file_path_watche...
File base/files/file_path_watcher_fsevents.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/files/file_path_watche...
base/files/file_path_watcher_fsevents.cc:192: void
FilePathWatcherFSEvents::CancelOnMessageLoopThread() {
Do you think this function name should change or is this ok?

https://codereview.chromium.org/1100773004/diff/1/base/files/file_path_watche...
File base/files/file_path_watcher_kqueue.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/files/file_path_watche...
base/files/file_path_watcher_kqueue.cc:331: io_task_runner_ =
ThreadTaskRunnerHandle::Get();
remind me.. is the destruction of the TaskRunnerHandle going to be tied to the
MessageLoop::current()'s destruction in the same way that the MLP was?

https://codereview.chromium.org/1100773004/diff/1/base/files/file_path_watche...
File base/files/file_path_watcher_kqueue.h (right):

https://codereview.chromium.org/1100773004/diff/1/base/files/file_path_watche...
base/files/file_path_watcher_kqueue.h:63: void CancelOnMessageLoopThread()
override;
same question for this

https://codereview.chromium.org/1100773004/diff/1/base/memory/ref_counted_del...
File base/memory/ref_counted_delete_on_message_loop.h (right):

https://codereview.chromium.org/1100773004/diff/1/base/memory/ref_counted_del...
base/memory/ref_counted_delete_on_message_loop.h:36:
RefCountedDeleteOnMessageLoop(
should this be renamed?

https://codereview.chromium.org/1100773004/diff/1/base/memory/weak_ptr_unitte...
File base/memory/weak_ptr_unittest.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/memory/weak_ptr_unitte...
base/memory/weak_ptr_unittest.cc:29:
creator_thread.message_loop()->task_runner()->PostTask(
why message_loop()->task_runner() instead of just task_runner()?

https://codereview.chromium.org/1100773004/diff/1/base/memory/weak_ptr_unitte...
base/memory/weak_ptr_unittest.cc:69: message_loop()->task_runner()->PostTask(
dittos

https://codereview.chromium.org/1100773004/diff/1/base/posix/unix_domain_sock...
File base/posix/unix_domain_socket_linux_unittest.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/posix/unix_domain_sock...
base/posix/unix_domain_socket_linux_unittest.cc:37:
message_thread.message_loop()->task_runner()->PostTask(
thread.task_runner() here too? I think there's some more too, can your script
find these?

https://codereview.chromium.org/1100773004/diff/1/base/prefs/json_pref_store_...
File base/prefs/json_pref_store_unittest.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/prefs/json_pref_store_...
base/prefs/json_pref_store_unittest.cc:119: new JsonPrefStore(bogus_input_file,
message_loop_.task_runner().get(),
nit: no get, it's taking a refptr

same for all other JsonPrefStore constructors

https://codereview.chromium.org/1100773004/diff/1/base/prefs/pref_member.cc
File base/prefs/pref_member.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/prefs/pref_member.cc#n...
base/prefs/pref_member.cc:127: thread_task_runner_ = task_runner;
nit: pass it by value to the method and .Pass() it here.

https://codereview.chromium.org/1100773004/diff/1/base/sequence_checker_unitt...
File base/sequence_checker_unittest.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/sequence_checker_unitt...
base/sequence_checker_unittest.cc:87:
other_thread()->message_loop()->task_runner()->PostTask(
no message_loop() needed here too?

https://codereview.chromium.org/1100773004/diff/1/base/single_thread_task_run...
File base/single_thread_task_runner.h (right):

https://codereview.chromium.org/1100773004/diff/1/base/single_thread_task_run...
base/single_thread_task_runner.h:19: //   - Post tasks to an existing thread's
MessageLoop.
Is there maybe somewhere else you could refer to here for more info?

https://codereview.chromium.org/1100773004/diff/1/base/synchronization/cancel...
File base/synchronization/cancellation_flag_unittest.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/synchronization/cancel...
base/synchronization/cancellation_flag_unittest.cc:60:
t.message_loop()->task_runner()->PostTask(FROM_HERE,
no message_loop()->?

https://codereview.chromium.org/1100773004/diff/1/base/synchronization/condit...
File base/synchronization/condition_variable_unittest.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/synchronization/condit...
base/synchronization/condition_variable_unittest.cc:225:
thread.message_loop()->task_runner()->PostTask(
dittos

https://codereview.chromium.org/1100773004/diff/1/base/task/cancelable_task_t...
File base/task/cancelable_task_tracker.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/task/cancelable_task_t...
base/task/cancelable_task_tracker.cc:89:
DCHECK(base::ThreadTaskRunnerHandle::Get());
Get() already DCHECKs. Do you want to IsSet()?

https://codereview.chromium.org/1100773004/diff/1/base/task/cancelable_task_t...
base/task/cancelable_task_tracker.cc:117:
DCHECK(base::ThreadTaskRunnerHandle::Get());
dittos

https://codereview.chromium.org/1100773004/diff/1/base/test/launcher/test_lau...
File base/test/launcher/test_launcher.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/test/launcher/test_lau...
base/test/launcher/test_launcher.cc:355: scoped_refptr<SingleThreadTaskRunner>
task_runner,
nit: can you change this to a STTR*? It doesn't want a ref.

https://codereview.chromium.org/1100773004/diff/1/base/test/thread_test_helpe...
File base/test/thread_test_helper.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/test/thread_test_helpe...
base/test/thread_test_helper.cc:16: target_thread_(target_thread),
and .Pass() here

https://codereview.chromium.org/1100773004/diff/1/base/test/thread_test_helper.h
File base/test/thread_test_helper.h (right):

https://codereview.chromium.org/1100773004/diff/1/base/test/thread_test_helpe...
base/test/thread_test_helper.h:22: const scoped_refptr<SingleThreadTaskRunner>&
target_thread);
nit: pass by value?

https://codereview.chromium.org/1100773004/diff/1/base/thread_task_runner_han...
File base/thread_task_runner_handle_unittest.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/thread_task_runner_han...
base/thread_task_runner_handle_unittest.cc:11: 
can you put the tests in an anon namespace inside base?

https://codereview.chromium.org/1100773004/diff/1/base/threading/sequenced_wo...
File base/threading/sequenced_worker_pool.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/threading/sequenced_wo...
base/threading/sequenced_worker_pool.cc:1161: // Note: the task runner is unset
when SequencedWorkerPool is constructed
Uhm.. I'm not sure what this comment means. Can you reword it or expand it?

https://codereview.chromium.org/1100773004/diff/1/base/threading/sequenced_wo...
base/threading/sequenced_worker_pool.cc:1179:
DCHECK(constructor_task_runner_.get());
nit: no .get()

https://codereview.chromium.org/1100773004/diff/1/base/trace_event/trace_even...
File base/trace_event/trace_event_impl.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/trace_event/trace_even...
base/trace_event/trace_event_impl.cc:1410: DCHECK(!flush_task_runner_.get());
nit: no get()

https://codereview.chromium.org/1100773004/diff/1/base/trace_event/trace_even...
base/trace_event/trace_event_impl.cc:1684: // 1. Flush() is called in threadA
whose task runner is saved in
s/threadA/thread

https://codereview.chromium.org/1100773004/diff/1/base/trace_event/trace_even...
base/trace_event/trace_event_impl.cc:1714: DCHECK(!flush_task_runner_.get());
nit: no get()

https://codereview.chromium.org/1100773004/diff/1/base/trace_event/trace_even...
base/trace_event/trace_event_impl.cc:1718: DCHECK(!thread_message_loops_.size()
|| flush_task_runner_.get());
DCHECK_IMPLIES(loops_.size(), runner_.get()) ?

https://codereview.chromium.org/1100773004/diff/1/base/trace_event/trace_even...
base/trace_event/trace_event_impl.cc:1818: if (!CheckGeneration(generation) ||
!flush_task_runner_.get()) {
nit: no get

https://codereview.chromium.org/1100773004/diff/1/base/trace_event/trace_even...
base/trace_event/trace_event_impl.cc:1828: if (!CheckGeneration(generation) ||
!flush_task_runner_.get() ||
nit: no get

https://codereview.chromium.org/1100773004/diff/1/base/trace_event/trace_even...
base/trace_event/trace_event_impl.cc:1839: if (!CheckGeneration(generation) ||
!flush_task_runner_.get()) {
nit: no get

https://codereview.chromium.org/1100773004/diff/1/base/trace_event/trace_even...
File base/trace_event/trace_event_impl.h (right):

https://codereview.chromium.org/1100773004/diff/1/base/trace_event/trace_even...
base/trace_event/trace_event_impl.h:802: // added into the local event buffer.
Not using SingleThreadTaskRunner
Can you verify that this comment is as correct as it was before? Lifetimes of
STTR and MessageLoop are not clear to me.

https://codereview.chromium.org/1100773004/diff/1/base/trace_event/trace_even...
File base/trace_event/trace_event_memory.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/trace_event/trace_even...
base/trace_event/trace_event_memory.cc:152: : task_runner_(task_runner),
.Pass() here

https://codereview.chromium.org/1100773004/diff/1/chromeos/login/auth/online_...
File chromeos/login/auth/online_attempt.h (right):

https://codereview.chromium.org/1100773004/diff/1/chromeos/login/auth/online_...
chromeos/login/auth/online_attempt.h:23: class MessageLoopProxy;
TODO somewhere?

https://codereview.chromium.org/1100773004/diff/1/chromeos/login/auth/online_...
File chromeos/login/auth/online_attempt_host.h (right):

https://codereview.chromium.org/1100773004/diff/1/chromeos/login/auth/online_...
chromeos/login/auth/online_attempt_host.h:18: class MessageLoopProxy;
TODO somewhere?

https://codereview.chromium.org/1100773004/diff/1/components/webdata/common/w...
File components/webdata/common/web_data_service_base.h (right):

https://codereview.chromium.org/1100773004/diff/1/components/webdata/common/w...
components/webdata/common/web_data_service_base.h:21: class MessageLoopProxy;
TODO somewhere?

https://codereview.chromium.org/1100773004/diff/1/net/disk_cache/simple/simpl...
File net/disk_cache/simple/simple_entry_impl.cc (right):

https://codereview.chromium.org/1100773004/diff/1/net/disk_cache/simple/simpl...
net/disk_cache/simple/simple_entry_impl.cc:146: using base::MessageLoopProxy;
i don't actually see this class being used?

[Sami, 2015-04-23 17:48:26]

> Sorry for the tons of vaguely related "while you're here clean things up"
nits.

No problem, seems like a good opportunity for that :) Thanks for the comments.

https://codereview.chromium.org/1100773004/diff/1/base/chromeos/memory_pressu...
File base/chromeos/memory_pressure_monitor_chromeos.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/chromeos/memory_pressu...
base/chromeos/memory_pressure_monitor_chromeos.cc:10: #include
"base/message_loop/message_loop.h"
On 2015/04/21 20:16:30, danakj wrote:
> still needed?

Nope, removed.

https://codereview.chromium.org/1100773004/diff/1/base/files/file_path_watcher.h
File base/files/file_path_watcher.h (right):

https://codereview.chromium.org/1100773004/diff/1/base/files/file_path_watche...
base/files/file_path_watcher.h:66: const
scoped_refptr<base::SingleThreadTaskRunner>& task_runner) {
On 2015/04/21 20:16:30, danakj wrote:
> nit: pass by value and .Pass() it into task_runner_

Done.

https://codereview.chromium.org/1100773004/diff/1/base/files/file_path_watche...
File base/files/file_path_watcher_fsevents.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/files/file_path_watche...
base/files/file_path_watcher_fsevents.cc:192: void
FilePathWatcherFSEvents::CancelOnMessageLoopThread() {
On 2015/04/21 20:16:30, danakj wrote:
> Do you think this function name should change or is this ok?

I was originally going to rename it but ended up not doing so because the
FilePathWatcher API has explicit dependencies to running under a
MessageLoopForIO (see Watch() for example).

https://codereview.chromium.org/1100773004/diff/1/base/files/file_path_watche...
File base/files/file_path_watcher_kqueue.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/files/file_path_watche...
base/files/file_path_watcher_kqueue.cc:331: io_task_runner_ =
ThreadTaskRunnerHandle::Get();
On 2015/04/21 20:16:30, danakj wrote:
> remind me.. is the destruction of the TaskRunnerHandle going to be tied to the
> MessageLoop::current()'s destruction in the same way that the MLP was?

Yes, both MLP and TTRH are cleared in the same place in ~MessageLoop(). You can
still post tasks to the task runner after that, but they'll be dropped on the
floor just as with MLP.

https://codereview.chromium.org/1100773004/diff/1/base/files/file_path_watche...
File base/files/file_path_watcher_kqueue.h (right):

https://codereview.chromium.org/1100773004/diff/1/base/files/file_path_watche...
base/files/file_path_watcher_kqueue.h:63: void CancelOnMessageLoopThread()
override;
On 2015/04/21 20:16:30, danakj wrote:
> same question for this

Same answer :) FWIW the implementation of this starts with this DCHECK so it
really wants to be on a specific type of message loop:

    DCHECK(MessageLoopForIO::current());

https://codereview.chromium.org/1100773004/diff/1/base/memory/ref_counted_del...
File base/memory/ref_counted_delete_on_message_loop.h (right):

https://codereview.chromium.org/1100773004/diff/1/base/memory/ref_counted_del...
base/memory/ref_counted_delete_on_message_loop.h:36:
RefCountedDeleteOnMessageLoop(
On 2015/04/21 20:16:30, danakj wrote:
> should this be renamed?

Yes, I think it should but I wanted to do that separately since this is used in
about different 20 places. I'll add a TODO here.

https://codereview.chromium.org/1100773004/diff/1/base/memory/weak_ptr_unitte...
File base/memory/weak_ptr_unittest.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/memory/weak_ptr_unitte...
base/memory/weak_ptr_unittest.cc:29:
creator_thread.message_loop()->task_runner()->PostTask(
On 2015/04/21 20:16:30, danakj wrote:
> why message_loop()->task_runner() instead of just task_runner()?

Hmm, I thought I applied that shortcut everywhere but looks like I missed some
places (here and elsewhere). Thanks for pointing this out!

https://codereview.chromium.org/1100773004/diff/1/base/memory/weak_ptr_unitte...
base/memory/weak_ptr_unittest.cc:69: message_loop()->task_runner()->PostTask(
On 2015/04/21 20:16:30, danakj wrote:
> dittos

Done.

https://codereview.chromium.org/1100773004/diff/1/base/posix/unix_domain_sock...
File base/posix/unix_domain_socket_linux_unittest.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/posix/unix_domain_sock...
base/posix/unix_domain_socket_linux_unittest.cc:37:
message_thread.message_loop()->task_runner()->PostTask(
On 2015/04/21 20:16:30, danakj wrote:
> thread.task_runner() here too? I think there's some more too, can your script
> find these?

Yep, these should be fixed now.

https://codereview.chromium.org/1100773004/diff/1/base/prefs/json_pref_store_...
File base/prefs/json_pref_store_unittest.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/prefs/json_pref_store_...
base/prefs/json_pref_store_unittest.cc:119: new JsonPrefStore(bogus_input_file,
message_loop_.task_runner().get(),
On 2015/04/21 20:16:30, danakj wrote:
> nit: no get, it's taking a refptr
> 
> same for all other JsonPrefStore constructors

Done.

https://codereview.chromium.org/1100773004/diff/1/base/prefs/pref_member.cc
File base/prefs/pref_member.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/prefs/pref_member.cc#n...
base/prefs/pref_member.cc:127: thread_task_runner_ = task_runner;
On 2015/04/21 20:16:30, danakj wrote:
> nit: pass it by value to the method and .Pass() it here.

Done.

https://codereview.chromium.org/1100773004/diff/1/base/sequence_checker_unitt...
File base/sequence_checker_unittest.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/sequence_checker_unitt...
base/sequence_checker_unittest.cc:87:
other_thread()->message_loop()->task_runner()->PostTask(
On 2015/04/21 20:16:30, danakj wrote:
> no message_loop() needed here too?

Done.

https://codereview.chromium.org/1100773004/diff/1/base/single_thread_task_run...
File base/single_thread_task_runner.h (right):

https://codereview.chromium.org/1100773004/diff/1/base/single_thread_task_run...
base/single_thread_task_runner.h:19: //   - Post tasks to an existing thread's
MessageLoop.
On 2015/04/21 20:16:30, danakj wrote:
> Is there maybe somewhere else you could refer to here for more info?

Good point, MessageLoop::task_runner() is probably the best reference.

https://codereview.chromium.org/1100773004/diff/1/base/synchronization/cancel...
File base/synchronization/cancellation_flag_unittest.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/synchronization/cancel...
base/synchronization/cancellation_flag_unittest.cc:60:
t.message_loop()->task_runner()->PostTask(FROM_HERE,
On 2015/04/21 20:16:30, danakj wrote:
> no message_loop()->?

Done.

https://codereview.chromium.org/1100773004/diff/1/base/synchronization/condit...
File base/synchronization/condition_variable_unittest.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/synchronization/condit...
base/synchronization/condition_variable_unittest.cc:225:
thread.message_loop()->task_runner()->PostTask(
On 2015/04/21 20:16:30, danakj wrote:
> dittos

Done.

https://codereview.chromium.org/1100773004/diff/1/base/task/cancelable_task_t...
File base/task/cancelable_task_tracker.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/task/cancelable_task_t...
base/task/cancelable_task_tracker.cc:89:
DCHECK(base::ThreadTaskRunnerHandle::Get());
On 2015/04/21 20:16:30, danakj wrote:
> Get() already DCHECKs. Do you want to IsSet()?

Yes, that makes more sense (checked for any other instances too -- only two are
in this file).

https://codereview.chromium.org/1100773004/diff/1/base/task/cancelable_task_t...
base/task/cancelable_task_tracker.cc:117:
DCHECK(base::ThreadTaskRunnerHandle::Get());
On 2015/04/21 20:16:30, danakj wrote:
> dittos

Done.

https://codereview.chromium.org/1100773004/diff/1/base/test/launcher/test_lau...
File base/test/launcher/test_launcher.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/test/launcher/test_lau...
base/test/launcher/test_launcher.cc:355: scoped_refptr<SingleThreadTaskRunner>
task_runner,
On 2015/04/21 20:16:30, danakj wrote:
> nit: can you change this to a STTR*? It doesn't want a ref.

Sure, done.

https://codereview.chromium.org/1100773004/diff/1/base/test/thread_test_helpe...
File base/test/thread_test_helper.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/test/thread_test_helpe...
base/test/thread_test_helper.cc:16: target_thread_(target_thread),
On 2015/04/21 20:16:31, danakj wrote:
> and .Pass() here

Done.

https://codereview.chromium.org/1100773004/diff/1/base/test/thread_test_helper.h
File base/test/thread_test_helper.h (right):

https://codereview.chromium.org/1100773004/diff/1/base/test/thread_test_helpe...
base/test/thread_test_helper.h:22: const scoped_refptr<SingleThreadTaskRunner>&
target_thread);
On 2015/04/21 20:16:31, danakj wrote:
> nit: pass by value?

Done.

https://codereview.chromium.org/1100773004/diff/1/base/thread_task_runner_han...
File base/thread_task_runner_handle_unittest.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/thread_task_runner_han...
base/thread_task_runner_handle_unittest.cc:11: 
On 2015/04/21 20:16:31, danakj wrote:
> can you put the tests in an anon namespace inside base?

Ack, this file shouldn't have been included in this patch at all :( It didn't
get compiled or run so I completely missed that it was here. Removed.

https://codereview.chromium.org/1100773004/diff/1/base/threading/sequenced_wo...
File base/threading/sequenced_worker_pool.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/threading/sequenced_wo...
base/threading/sequenced_worker_pool.cc:1161: // Note: the task runner is unset
when SequencedWorkerPool is constructed
On 2015/04/21 20:16:31, danakj wrote:
> Uhm.. I'm not sure what this comment means. Can you reword it or expand it?

Oh, sorry, accidentally left this in since I initially was going to change how
SequencedWorkerPool works but turns out there's no need to do that. Removed.

https://codereview.chromium.org/1100773004/diff/1/base/threading/sequenced_wo...
base/threading/sequenced_worker_pool.cc:1179:
DCHECK(constructor_task_runner_.get());
On 2015/04/21 20:16:31, danakj wrote:
> nit: no .get()

Done.

https://codereview.chromium.org/1100773004/diff/1/base/trace_event/trace_even...
File base/trace_event/trace_event_impl.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/trace_event/trace_even...
base/trace_event/trace_event_impl.cc:1410: DCHECK(!flush_task_runner_.get());
On 2015/04/21 20:16:31, danakj wrote:
> nit: no get()

Done.

https://codereview.chromium.org/1100773004/diff/1/base/trace_event/trace_even...
base/trace_event/trace_event_impl.cc:1684: // 1. Flush() is called in threadA
whose task runner is saved in
On 2015/04/21 20:16:31, danakj wrote:
> s/threadA/thread

I think that's supposed to mean "thread A" -- as in some arbitrary thread.
Changed to "thread A" here and below.

https://codereview.chromium.org/1100773004/diff/1/base/trace_event/trace_even...
base/trace_event/trace_event_impl.cc:1714: DCHECK(!flush_task_runner_.get());
On 2015/04/21 20:16:31, danakj wrote:
> nit: no get()

Done.

https://codereview.chromium.org/1100773004/diff/1/base/trace_event/trace_even...
base/trace_event/trace_event_impl.cc:1718: DCHECK(!thread_message_loops_.size()
|| flush_task_runner_.get());
On 2015/04/21 20:16:31, danakj wrote:
> DCHECK_IMPLIES(loops_.size(), runner_.get()) ?

Ah, neat. TIL.

https://codereview.chromium.org/1100773004/diff/1/base/trace_event/trace_even...
base/trace_event/trace_event_impl.cc:1818: if (!CheckGeneration(generation) ||
!flush_task_runner_.get()) {
On 2015/04/21 20:16:31, danakj wrote:
> nit: no get

Done.

https://codereview.chromium.org/1100773004/diff/1/base/trace_event/trace_even...
base/trace_event/trace_event_impl.cc:1828: if (!CheckGeneration(generation) ||
!flush_task_runner_.get() ||
On 2015/04/21 20:16:31, danakj wrote:
> nit: no get

Done.

https://codereview.chromium.org/1100773004/diff/1/base/trace_event/trace_even...
base/trace_event/trace_event_impl.cc:1839: if (!CheckGeneration(generation) ||
!flush_task_runner_.get()) {
On 2015/04/21 20:16:31, danakj wrote:
> nit: no get

Done.

https://codereview.chromium.org/1100773004/diff/1/base/trace_event/trace_even...
File base/trace_event/trace_event_impl.h (right):

https://codereview.chromium.org/1100773004/diff/1/base/trace_event/trace_even...
base/trace_event/trace_event_impl.h:802: // added into the local event buffer.
Not using SingleThreadTaskRunner
On 2015/04/21 20:16:31, danakj wrote:
> Can you verify that this comment is as correct as it was before? Lifetimes of
> STTR and MessageLoop are not clear to me. 

It's still accurate: the reason concrete MessageLoops are needed here as opposed
to MLP/STTR is that you can only attach a destruction observer to a MessageLoop.
The change from MLP to STTR doesn't affect that since the ThreadTaskRunnerHandle
"lifetime" is the same as an MLP (I say "lifetime" because both are refcounted
so you can hang onto them for as long as you want).

https://codereview.chromium.org/1100773004/diff/1/base/trace_event/trace_even...
File base/trace_event/trace_event_memory.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/trace_event/trace_even...
base/trace_event/trace_event_memory.cc:152: : task_runner_(task_runner),
On 2015/04/21 20:16:31, danakj wrote:
> .Pass() here

Done. Seems like we should have some automated check for this since it's very
easy to miss :\

https://codereview.chromium.org/1100773004/diff/1/chromeos/login/auth/online_...
File chromeos/login/auth/online_attempt.h (right):

https://codereview.chromium.org/1100773004/diff/1/chromeos/login/auth/online_...
chromeos/login/auth/online_attempt.h:23: class MessageLoopProxy;
On 2015/04/21 20:16:31, danakj wrote:
> TODO somewhere?

Done.

https://codereview.chromium.org/1100773004/diff/1/chromeos/login/auth/online_...
File chromeos/login/auth/online_attempt_host.h (right):

https://codereview.chromium.org/1100773004/diff/1/chromeos/login/auth/online_...
chromeos/login/auth/online_attempt_host.h:18: class MessageLoopProxy;
On 2015/04/21 20:16:31, danakj wrote:
> TODO somewhere?

Done.

https://codereview.chromium.org/1100773004/diff/1/components/webdata/common/w...
File components/webdata/common/web_data_service_base.h (right):

https://codereview.chromium.org/1100773004/diff/1/components/webdata/common/w...
components/webdata/common/web_data_service_base.h:21: class MessageLoopProxy;
On 2015/04/21 20:16:31, danakj wrote:
> TODO somewhere?

Done.

https://codereview.chromium.org/1100773004/diff/1/net/disk_cache/simple/simpl...
File net/disk_cache/simple/simple_entry_impl.cc (right):

https://codereview.chromium.org/1100773004/diff/1/net/disk_cache/simple/simpl...
net/disk_cache/simple/simple_entry_impl.cc:146: using base::MessageLoopProxy;
On 2015/04/21 20:16:31, danakj wrote:
> i don't actually see this class being used?

Heh, well spotted. Removed.

[danakj, 2015-04-23 18:17:54]

LGTM

https://codereview.chromium.org/1100773004/diff/1/base/trace_event/trace_even...
File base/trace_event/trace_event_memory.cc (right):

https://codereview.chromium.org/1100773004/diff/1/base/trace_event/trace_even...
base/trace_event/trace_event_memory.cc:152: : task_runner_(task_runner),
On 2015/04/23 17:48:25, Sami wrote:
> On 2015/04/21 20:16:31, danakj wrote:
> > .Pass() here
> 
> Done. Seems like we should have some automated check for this since it's very
> easy to miss :\

I'm not sure we can, we could say "hey you assigned it but didn't use it after,
use Pass", I guess. But it's more like programmer intent here.

[Sami, 2015-04-27 18:03:37]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2015-04-27 18:03:38]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1100773004/#ps60001
(title: "Rebased.")

[commit-bot: I haz the power, 2015-04-27 18:04:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1100773004/60001

[commit-bot: I haz the power, 2015-04-27 20:22:35]

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

[commit-bot: I haz the power, 2015-04-27 20:22:36]

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

[Sami, 2015-04-29 10:11:45]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2015-04-29 10:11:45]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1100773004/#ps80001
(title: "Rebased.")

[commit-bot: I haz the power, 2015-04-29 10:11:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1100773004/80001

[commit-bot: I haz the power, 2015-04-29 10:15:28]

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

[commit-bot: I haz the power, 2015-04-29 10:15:28]

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

[Sami, 2015-04-29 11:18:44]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2015-04-29 11:18:44]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1100773004/#ps100001
(title: "Rebase again.")

[commit-bot: I haz the power, 2015-04-29 11:19:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1100773004/100001

[commit-bot: I haz the power, 2015-04-29 14:27:17]

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

[commit-bot: I haz the power, 2015-04-29 14:27:18]

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

[Sami, 2015-04-29 15:45:36]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2015-04-29 15:45:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1100773004/100001

[commit-bot: I haz the power, 2015-04-29 16:57:00]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-04-29 16:57:54]

Patchset 6 (id:??) landed as
https://crrev.com/62aa5ca413e15738ebebbb9acd271138ec808739
Cr-Commit-Position: refs/heads/master@{#327512}

[stevenjb, 2015-04-29 21:17:29]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1113953002/ by stevenjb@chromium.org.

The reason for reverting is: This CL caused this failure:
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...

I would strongly recommend doing this in smaller pieces since it combines
mechanical changes with more subtle ones (base/prefs, base/task).
.

[Sami, 2015-04-30 16:41:21]

On 2015/04/29 21:17:29, stevenjb wrote:
> A revert of this CL (patchset #6 id:100001) has been created in
> https://codereview.chromium.org/1113953002/ by mailto:stevenjb@chromium.org.
> 
> The reason for reverting is: This CL caused this failure:
>
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...
> 
> I would strongly recommend doing this in smaller pieces since it combines
> mechanical changes with more subtle ones (base/prefs, base/task).
> .

Thanks for linking to the failures. I've now left out the following files from
this patch:

base/prefs/pref_member.cc
base/prefs/pref_member.h
base/threading/sequenced_worker_pool.cc
base/threading/sequenced_worker_pool.h

I believe these are the changes which are likely to cause failures in non-cq
tests, because all the other instances (including /base/task) already implicitly
require an active task runner.

[stevenjb, 2015-04-30 17:10:37]

On 2015/04/30 16:41:21, Sami wrote:
> On 2015/04/29 21:17:29, stevenjb wrote:
> > A revert of this CL (patchset #6 id:100001) has been created in
> > https://codereview.chromium.org/1113953002/ by mailto:stevenjb@chromium.org.
> > 
> > The reason for reverting is: This CL caused this failure:
> >
>
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...
> > 
> > I would strongly recommend doing this in smaller pieces since it combines
> > mechanical changes with more subtle ones (base/prefs, base/task).
> > .
> 
> Thanks for linking to the failures. I've now left out the following files from
> this patch:
> 
> base/prefs/pref_member.cc
> base/prefs/pref_member.h
> base/threading/sequenced_worker_pool.cc
> base/threading/sequenced_worker_pool.h
> 
> I believe these are the changes which are likely to cause failures in non-cq
> tests, because all the other instances (including /base/task) already
implicitly
> require an active task runner.

sg. please make sure to run linux_chromium_compile_dbg_ng (it's not part of the
cq) with the new CL.

[Sami, 2015-04-30 18:57:47]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2015-04-30 18:57:48]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1100773004/#ps140001
(title: "Added some missing includes.")

[commit-bot: I haz the power, 2015-04-30 18:58:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1100773004/140001

[commit-bot: I haz the power, 2015-04-30 19:06:50]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-04-30 19:08:07]

Patchset 8 (id:??) landed as
https://crrev.com/054861d0934c663de3cea933e1d65fd8319856a6
Cr-Commit-Position: refs/heads/master@{#327755}