Moved JsonPrefStore to use SequencedWorkerPool

Moved JsonPrefStore to use SequencedWorkerPool instead of FILE thread. The pool also ensures that the same file requests are written in order received and that they block on shutdown. 

BUG=153367
TEST=existing unit/browser tests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=166603

[eroman, 2012-10-05 20:38:35]

while this seems like an improvement, it doesn't seem to fix the underlying
problem of 153367 (if extension installation is taking that long, won't we just
"hang" at a later stage of shutdown when joining those threads?).

Please have jar review the changes to metrics_service.cc.

https://codereview.chromium.org/11027070/diff/2001/chrome/common/json_pref_st...
File chrome/common/json_pref_store.cc (right):

https://codereview.chromium.org/11027070/diff/2001/chrome/common/json_pref_st...
chrome/common/json_pref_store.cc:145: // by using
this sentence doesn't end.

https://codereview.chromium.org/11027070/diff/2001/chrome/common/json_pref_st...
chrome/common/json_pref_store.cc:170: 
is the extra space intentional?

[zel, 2012-10-05 20:54:28]

You are right, this CL does not solve this problem completely. It's just for (a)
part of the problem as described in http://crbug.com/153367 - see comment #14.

By moving JsonPrefStores from FILE pool, we will at least have opportunity to
run these tasks at shutdown even if something big (like extension install) is
still pegging FILE thread at that time. This will at least ensure that these
important files are flushed before ChromeOS session manager kills the entire
process.

The extension install part will be fixed in subsequent CLs... and I bet you we
will find more culprits than this one in other crashes.

https://codereview.chromium.org/11027070/diff/2001/chrome/common/json_pref_st...
File chrome/common/json_pref_store.cc (right):

https://codereview.chromium.org/11027070/diff/2001/chrome/common/json_pref_st...
chrome/common/json_pref_store.cc:145: // by using
On 2012/10/05 20:38:36, eroman wrote:
> this sentence doesn't end.

Done.

https://codereview.chromium.org/11027070/diff/2001/chrome/common/json_pref_st...
chrome/common/json_pref_store.cc:170: 
On 2012/10/05 20:38:36, eroman wrote:
> is the extra space intentional?

Done.

[jar (doing other things), 2012-10-05 21:58:18]

metrics_service changes in patch set 3 LGTM.

I do think that IF you can guarantee we get the important file written, then it
is safe to remove the wait.  I would not have subjected to world to this crash
if it were not for the urgency of the prefs write.

There will probably be other things that get wedged... and there will be
discovery of them.... but this does seem like a good change (if the associated
write works, and does not induce problems with your implementation... which I
didn't look at).

Thanks!

[willchan no longer on Chromium, 2012-10-05 22:38:34]

https://codereview.chromium.org/11027070/diff/12/chrome/common/json_pref_store.h
File chrome/common/json_pref_store.h (right):

https://codereview.chromium.org/11027070/diff/12/chrome/common/json_pref_stor...
chrome/common/json_pref_store.h:35: static JsonPrefStore* Create(const FilePath&
pref_filename,
Do we have many other places that use the 2nd Create() instead of this one? If
not, why not just provide this one?

The thing is, if you provide the second one, then people can pass in a
sequenced_worker_pool->GetTaskRunnerWithShutdownBehavior(value !=
BLOCK_SHUTDOWN). If you want to make sure it's BLOCK_SHUTDOWN, then you should
only provide this one. If you need the second one, then it may make sense to get
rid of this one and have people that want the BLOCK_SHUTDOWN behavior to simply
call sequenced_worker_pool->GetTaskRunnerWithShutdownBehavior(BLOCK_SHUTDOWN)
themselves.

Overloading functions is disallowed by the style guide if behaviors differ among
the overloaded functions, which arguably seems to be the case here (although
you're using the default shutdown behavior, so maybe it's obvious).

https://codereview.chromium.org/11027070/diff/12/chrome/common/json_pref_stor...
chrome/common/json_pref_store.h:40: base::TaskRunner* task_runner);
I think you want a SequencedTaskRunner. TaskRunner allows multiple tasks to run
at the same time, which I suspect would be buggy here.

https://codereview.chromium.org/11027070/diff/12/chrome/common/json_pref_stor...
chrome/common/json_pref_store.h:81: scoped_refptr<base::TaskRunner>
task_runner_;
SequencedTaskRunner.

[zel, 2012-10-10 01:27:50]

https://codereview.chromium.org/11027070/diff/12/chrome/common/json_pref_store.h
File chrome/common/json_pref_store.h (right):

https://codereview.chromium.org/11027070/diff/12/chrome/common/json_pref_stor...
chrome/common/json_pref_store.h:35: static JsonPrefStore* Create(const FilePath&
pref_filename,
On 2012/10/05 22:38:34, willchan wrote:
> Do we have many other places that use the 2nd Create() instead of this one? If
> not, why not just provide this one?
> 
> The thing is, if you provide the second one, then people can pass in a
> sequenced_worker_pool->GetTaskRunnerWithShutdownBehavior(value !=
> BLOCK_SHUTDOWN). If you want to make sure it's BLOCK_SHUTDOWN, then you should
> only provide this one. If you need the second one, then it may make sense to
get
> rid of this one and have people that want the BLOCK_SHUTDOWN behavior to
simply
> call sequenced_worker_pool->GetTaskRunnerWithShutdownBehavior(BLOCK_SHUTDOWN)
> themselves.
> 
> Overloading functions is disallowed by the style guide if behaviors differ
among
> the overloaded functions, which arguably seems to be the case here (although
> you're using the default shutdown behavior, so maybe it's obvious).

Yes, FILE thread flavor was still used from ServiceProcess. I've modified that
path (+few unit tests), added blocking pool there too. It's removed now, only
one Create() here.

https://codereview.chromium.org/11027070/diff/12/chrome/common/json_pref_stor...
chrome/common/json_pref_store.h:40: base::TaskRunner* task_runner);
On 2012/10/05 22:38:34, willchan wrote:
> I think you want a SequencedTaskRunner. TaskRunner allows multiple tasks to
run
> at the same time, which I suspect would be buggy here.

Done.

https://codereview.chromium.org/11027070/diff/12/chrome/common/json_pref_stor...
chrome/common/json_pref_store.h:81: scoped_refptr<base::TaskRunner>
task_runner_;
On 2012/10/05 22:38:34, willchan wrote:
> SequencedTaskRunner.

Done.

[zel, 2012-10-10 01:28:57]

+abodenha for chrome/service part

[akalin, 2012-10-10 01:43:15]

few more

https://codereview.chromium.org/11027070/diff/21019/chrome/common/json_pref_s...
File chrome/common/json_pref_store.cc (right):

https://codereview.chromium.org/11027070/diff/21019/chrome/common/json_pref_s...
chrome/common/json_pref_store.cc:16: #include "base/task_runner.h"
sequenced_task_runner.h

https://codereview.chromium.org/11027070/diff/21019/chrome/common/json_pref_s...
chrome/common/json_pref_store.cc:31: base::TaskRunner* task_runner)
Sequenced

https://codereview.chromium.org/11027070/diff/21019/chrome/common/json_pref_s...
chrome/common/json_pref_store.cc:47: // Deserializes JSON on the file thread.
update comment

https://codereview.chromium.org/11027070/diff/21019/chrome/common/json_pref_s...
chrome/common/json_pref_store.cc:90: scoped_refptr<base::TaskRunner>
task_runner_;
probably should be named file_task_runner_ or file_sequenced_task_runner_

https://codereview.chromium.org/11027070/diff/21019/chrome/common/json_pref_s...
chrome/common/json_pref_store.cc:156: task_runner_(task_runner),
file_task_runner_/file_sequenced_task_runner_?

https://codereview.chromium.org/11027070/diff/21019/chrome/common/json_pref_s...
File chrome/common/json_pref_store.h (right):

https://codereview.chromium.org/11027070/diff/21019/chrome/common/json_pref_s...
chrome/common/json_pref_store.h:34: // sequenced properly and that such request
block shutdown.
request -> requests

https://codereview.chromium.org/11027070/diff/21019/chrome/common/json_pref_s...
chrome/common/json_pref_store.h:35: static JsonPrefStore* Create(const FilePath&
pref_filename,
return scoped_ptr?

https://codereview.chromium.org/11027070/diff/21019/chrome/common/json_pref_s...
chrome/common/json_pref_store.h:77: scoped_refptr<base::SequencedTaskRunner>
task_runner_;
make this const

[Albert Bodenhamer, 2012-10-10 16:07:03]

I don't see anything scary, but Gene is a much better choice for this review.

[gene, 2012-10-10 17:06:44]

lgtm

[zel, 2012-10-10 23:24:59]

+mnissler for chrome/browser/prefs part
+asargent for chrome/browser/extensions part

OK, to make JsonPrefStore use exclusively blocking pool, as Will suggested,
ended up being yak shaving... especially in tons of unit tests. Added additional
OWNER to bless those changes as well.

https://codereview.chromium.org/11027070/diff/21019/chrome/common/json_pref_s...
File chrome/common/json_pref_store.cc (right):

https://codereview.chromium.org/11027070/diff/21019/chrome/common/json_pref_s...
chrome/common/json_pref_store.cc:16: #include "base/task_runner.h"
On 2012/10/10 01:43:16, akalin wrote:
> sequenced_task_runner.h

Done.

https://codereview.chromium.org/11027070/diff/21019/chrome/common/json_pref_s...
chrome/common/json_pref_store.cc:31: base::TaskRunner* task_runner)
On 2012/10/10 01:43:16, akalin wrote:
> Sequenced

Done.

https://codereview.chromium.org/11027070/diff/21019/chrome/common/json_pref_s...
chrome/common/json_pref_store.cc:47: // Deserializes JSON on the file thread.
On 2012/10/10 01:43:16, akalin wrote:
> update comment

Done.

https://codereview.chromium.org/11027070/diff/21019/chrome/common/json_pref_s...
chrome/common/json_pref_store.cc:90: scoped_refptr<base::TaskRunner>
task_runner_;
On 2012/10/10 01:43:16, akalin wrote:
> probably should be named file_task_runner_ or file_sequenced_task_runner_

Done - sequenced_task_runner_

https://codereview.chromium.org/11027070/diff/21019/chrome/common/json_pref_s...
chrome/common/json_pref_store.cc:156: task_runner_(task_runner),
On 2012/10/10 01:43:16, akalin wrote:
> file_task_runner_/file_sequenced_task_runner_?

sequenced_task_runner_. Done.

https://codereview.chromium.org/11027070/diff/21019/chrome/common/json_pref_s...
File chrome/common/json_pref_store.h (right):

https://codereview.chromium.org/11027070/diff/21019/chrome/common/json_pref_s...
chrome/common/json_pref_store.h:34: // sequenced properly and that such request
block shutdown.
On 2012/10/10 01:43:16, akalin wrote:
> request -> requests

Done.

https://codereview.chromium.org/11027070/diff/21019/chrome/common/json_pref_s...
chrome/common/json_pref_store.h:77: scoped_refptr<base::SequencedTaskRunner>
task_runner_;
On 2012/10/10 01:43:16, akalin wrote:
> make this const

that's not going to work since we use non-const methods like PostTask()

[willchan no longer on Chromium, 2012-10-11 23:19:30]

http://codereview.chromium.org/11027070/diff/20015/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/11027070/diff/20015/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:221: path.AsUTF8Unsafe()),
The only thing that makes me feel nervous about this is the lack of enforcement
that the different requests all use this token. Where's the JsonPrefStore
created? Can we pass this specific SequencedTaskRunner to it somehow to enforce
that even if this token changes, they both end up using the same token?
Otherwise it feels a bit fragile.

[akalin, 2012-10-12 19:23:56]

http://codereview.chromium.org/11027070/diff/21019/chrome/common/json_pref_st...
File chrome/common/json_pref_store.h (right):

http://codereview.chromium.org/11027070/diff/21019/chrome/common/json_pref_st...
chrome/common/json_pref_store.h:77: scoped_refptr<base::SequencedTaskRunner>
task_runner_;
On 2012/10/10 23:25:00, zel wrote:
> On 2012/10/10 01:43:16, akalin wrote:
> > make this const
> 
> that's not going to work since we use non-const methods like PostTask()

I meant make the pointer const, like:

'const scoped_refptr<base::SequencedTaskRunner> file_task_runner_;'

That's a const pointer to a STR, not a pointer to a const STR, so you should
still be able to call PostTask.

[zel, 2012-10-12 23:15:32]

http://codereview.chromium.org/11027070/diff/20015/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/11027070/diff/20015/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:221: path.AsUTF8Unsafe()),
On 2012/10/11 23:19:31, willchan wrote:
> The only thing that makes me feel nervous about this is the lack of
enforcement
> that the different requests all use this token. Where's the JsonPrefStore
> created? Can we pass this specific SequencedTaskRunner to it somehow to
enforce
> that even if this token changes, they both end up using the same token?
> Otherwise it feels a bit fragile.

I have isolated SequencedTaskRunner and shared it for profile creation, this
unfortunately introduces two factory methods for JSonPrefStore again... PTAL

[akalin, 2012-10-17 16:59:27]

On 2012/10/12 23:15:32, zel wrote:
> I have isolated SequencedTaskRunner and shared it for profile creation, this
> unfortunately introduces two factory methods for JSonPrefStore again... PTAL

Can you explain why you had to introduce two factory methods again?  It looks
like when you use the SequencedTaskRunner one, you create it from the SWP with a
token that doesn't depend on the filename.  Is that necessary?  Can we instead
consolidate on using the SequencedWorkerPool factory method?

[zel, 2012-10-17 18:04:40]

On 2012/10/17 16:59:27, akalin wrote:
> On 2012/10/12 23:15:32, zel wrote:
> > I have isolated SequencedTaskRunner and shared it for profile creation, this
> > unfortunately introduces two factory methods for JSonPrefStore again... PTAL
> 
> Can you explain why you had to introduce two factory methods again?  It looks
> like when you use the SequencedTaskRunner one, you create it from the SWP with
a
> token that doesn't depend on the filename.  Is that necessary?  Can we instead
> consolidate on using the SequencedWorkerPool factory method?

The factory method variant that uses SequencedTaskRunner,
JsonPrefStore::Create(STR, ...), is needed when I care about how this
JsonPrefStore instance serialization will be run in regards to all other I/O
tasks. For example, profile creation performs certain async operations (i.e.
directory create) which need to be performed before its inner JsonPrefStore
serialization kicks in. Previously singleton FILE thread took care of this
problem. See profile_impl.cc for samples of that one.

In other cases where such dependency between JsonPrefStore and I/O operations
from other classes does not exist, it's simpler to use
JsonPrefStore::Create(SWP, ...).

[akalin, 2012-10-17 21:57:30]

On 2012/10/17 18:04:40, zel wrote:
> The factory method variant that uses SequencedTaskRunner,
> JsonPrefStore::Create(STR, ...), is needed when I care about how this
> JsonPrefStore instance serialization will be run in regards to all other I/O
> tasks. For example, profile creation performs certain async operations (i.e.
> directory create) which need to be performed before its inner JsonPrefStore
> serialization kicks in. Previously singleton FILE thread took care of this
> problem. See profile_impl.cc for samples of that one.
> 
> In other cases where such dependency between JsonPrefStore and I/O operations
> from other classes does not exist, it's simpler to use
> JsonPrefStore::Create(SWP, ...).

I see.  I'd actually like to see the Create(SWP, ...) variant go away.  It
sounds like it's mostly there for convenience.  In addition to just having two
different constructors, it encourages use of SequencedWorkerPool in tests, which
seem a bit overkill to me (unless you're actually testing the block_on_shutdown
behavior).

Perhaps we should have a function like:

scoped_refptr<SequencedTaskRunner> GetTaskRunnerForPath(FilePath path,
scoped_refptr<SWP> swp);

that'll encapsulate the logic of building the token, getting a shutdown-blocking
runner, etc.  Then non-test use cases can do:

JsonPrefStore::Create(GetTaskRunnerForPath(path, BrowserThread::BlockingPool()),
...);

and test use cases can do:

JsonPrefStore::Create(ThreadTaskRunnerHandle::Get(), ...);

What do you think?

[akalin, 2012-10-17 21:58:19]

http://codereview.chromium.org/11027070/diff/44001/chrome/browser/browser_pro...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/11027070/diff/44001/chrome/browser/browser_pro...
chrome/browser/browser_process_impl.cc:711:
scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner =
is there any way we can pass down the task runner from chrome_browser_main
instead?  It'll make it more obvious that they share the same task runner.

[zel, 2012-10-17 23:38:17]

On 2012/10/17 21:57:30, akalin wrote:
> On 2012/10/17 18:04:40, zel wrote:
> > The factory method variant that uses SequencedTaskRunner,
> > JsonPrefStore::Create(STR, ...), is needed when I care about how this
> > JsonPrefStore instance serialization will be run in regards to all other I/O
> > tasks. For example, profile creation performs certain async operations (i.e.
> > directory create) which need to be performed before its inner JsonPrefStore
> > serialization kicks in. Previously singleton FILE thread took care of this
> > problem. See profile_impl.cc for samples of that one.
> > 
> > In other cases where such dependency between JsonPrefStore and I/O
operations
> > from other classes does not exist, it's simpler to use
> > JsonPrefStore::Create(SWP, ...).
> 
> I see.  I'd actually like to see the Create(SWP, ...) variant go away.  It
> sounds like it's mostly there for convenience.  In addition to just having two
> different constructors, it encourages use of SequencedWorkerPool in tests,
which
> seem a bit overkill to me (unless you're actually testing the
block_on_shutdown
> behavior).
> 
> Perhaps we should have a function like:
> 
> scoped_refptr<SequencedTaskRunner> GetTaskRunnerForPath(FilePath path,
> scoped_refptr<SWP> swp);
> 
> that'll encapsulate the logic of building the token, getting a
shutdown-blocking
> runner, etc.  Then non-test use cases can do:
> 
> JsonPrefStore::Create(GetTaskRunnerForPath(path,
BrowserThread::BlockingPool()),
> ...);
> 
> and test use cases can do:
> 
> JsonPrefStore::Create(ThreadTaskRunnerHandle::Get(), ...);
> 
> What do you think?

I am ok with that approach as well, but I will have no ability to control
semantics of how STR is generated before being passed to JsonPrefStore::Create()
- which is what that other factory method does.

[willchan no longer on Chromium, 2012-10-17 23:41:19]

chrome/browser/profiles LGTM

I did not survey all the callsites of the blocking pool vs SequencedTaskRunner
variant of JsonPrefStore. Someone else who owns that code can comment. I don't
feel strongly about making the callsite create the STR or allowing it to pass in
a SWP. So, I'll let whoever owns that code make the call :)

http://codereview.chromium.org/11027070/diff/44001/chrome/service/service_pro...
File chrome/service/service_process_prefs.cc (right):

http://codereview.chromium.org/11027070/diff/44001/chrome/service/service_pro...
chrome/service/service_process_prefs.cc:14: blocking_pool)) {
indent is off

[akalin, 2012-10-17 23:42:18]

On 2012/10/17 23:38:17, zel wrote:
> I am ok with that approach as well, but I will have no ability to control
> semantics of how STR is generated before being passed to
JsonPrefStore::Create()
> - which is what that other factory method does.

Yeah, that's the disadvantage.  However, the problem already exists now, since
it already has a STR constructor, right?  I feel like it's better to have a
single constructor, and we can audit all the non-test cases to make sure that
they're using the new utility method.

Another possibility would be to subclass SequencedTaskRunner (say,
BlockingSequencedTaskRunner) and have the constructor take only that subclass,
but that seems too heavyweight.

Another third possibility would be to add a BlocksShutdown() virtual member
function to TaskRunner et al. and have JsonPrefStore DCHECK that.

willchan@, what do you think of the above?

[akalin, 2012-10-17 23:47:18]

On 2012/10/17 23:42:18, akalin wrote:
> On 2012/10/17 23:38:17, zel wrote:
> > I am ok with that approach as well, but I will have no ability to control
> > semantics of how STR is generated before being passed to
> JsonPrefStore::Create()
> > - which is what that other factory method does.
> 
> Yeah, that's the disadvantage.  However, the problem already exists now, since
> it already has a STR constructor, right?  I feel like it's better to have a
> single constructor, and we can audit all the non-test cases to make sure that
> they're using the new utility method.
> 
> Another possibility would be to subclass SequencedTaskRunner (say,
> BlockingSequencedTaskRunner) and have the constructor take only that subclass,
> but that seems too heavyweight.
> 
> Another third possibility would be to add a BlocksShutdown() virtual member
> function to TaskRunner et al. and have JsonPrefStore DCHECK that.
> 
> willchan@, what do you think of the above?

Oh, I see willchan@ already commented re. not feeling strongly.  To clarify, the
possibilities I suggested above wouldn't go into this CL, but maybe a future
one.  I think I can live with just having the utility function to get a STR from
a SWP, using it for JsonPrefStore when appropriate, and having a single
constructor, and then maybe if something in the future needs a blocking pool,
add the virtual method. (Maybe file a BUG/write a TODO somewhere?)

[zel, 2012-10-18 00:31:15]

akalin@, I've also added a static method for getting appropriate
SequencedTaskRunner to JsonPrefStore as we agreed over IM.

http://codereview.chromium.org/11027070/diff/44001/chrome/browser/browser_pro...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/11027070/diff/44001/chrome/browser/browser_pro...
chrome/browser/browser_process_impl.cc:711:
scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner =
On 2012/10/17 21:58:19, akalin wrote:
> is there any way we can pass down the task runner from chrome_browser_main
> instead?  It'll make it more obvious that they share the same task runner.

Done.

[akalin, 2012-10-18 23:52:24]

Bunch more comments.  Basically I want to avoid more uses of SWP in unit tests
that don't specifically test SWP functionality.

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/browser_pro...
File chrome/browser/browser_process_impl.h (right):

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/browser_pro...
chrome/browser/browser_process_impl.h:45: explicit BrowserProcessImpl(
remove explicit

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/browser_pro...
chrome/browser/browser_process_impl.h:46: base::SequencedTaskRunner*
local_state_task_runner,
fwd-declare SequencedTaskRunner

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/browser_pro...
chrome/browser/browser_process_impl.h:46: base::SequencedTaskRunner*
local_state_task_runner,
add comment saying that local_state_task_runner must be a shutdown-blocking task
runner.

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/browser_pro...
chrome/browser/browser_process_impl.h:231:
scoped_refptr<base::SequencedTaskRunner> local_state_task_runner_;
const scoped_refptr<...> ...

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:271: // Returns the new local state
object, guaranteed non-NULL.
add a comment saying that local_state_task_runner must be a shutdown-blocking
task runner

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:712:
scoped_refptr<base::SequencedTaskRunner> local_state_task_runner =
can you instead get the local state path here and create the task runner using
it?

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs_unittest.cc:58: prefs_.reset(new
TestExtensionPrefs(blocking_pool_));
you can use message_loop_.message_loop_proxy() (see other comments)

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs_unittest.cc:81:
blocking_pool_->FlushForTesting();
can use message_loop_.RunAllPending() here

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/extensions/...
File chrome/browser/extensions/menu_manager_unittest.cc (right):

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/extensions/...
chrome/browser/extensions/menu_manager_unittest.cc:48: blocking_pool_ = new
base::SequencedWorkerPool(1, "ExtensionTestBlocking");
use current loop, etc.

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/extensions/...
File chrome/browser/extensions/test_extension_prefs.cc (right):

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/extensions/...
chrome/browser/extensions/test_extension_prefs.cc:79:
blocking_pool_->FlushForTesting();
i think you can replace the FlushForTesting call with:

void DoNothing() {}

    base::RunLoop run_loop;
    ASSERT_TRUE(
        task_runner_->PostTaskAndReply(
            FROM_HERE,
            base::Bind(&DoNothing),
            run_loop.QuitClosure()));
    run_loop.Run();

If CommitPendingWrite() just posts a task to the task_runner_, that should
suffice.

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/extensions/...
File chrome/browser/extensions/test_extension_prefs.h (right):

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/extensions/...
chrome/browser/extensions/test_extension_prefs.h:30: explicit
TestExtensionPrefs(base::SequencedWorkerPool* blocking_pool);
can you change to take an STR instead? (See my other comments for PrefStoreMock,
etc.)

In general, I'd like to minimize use of SWP in unit tests that aren't explicitly
testing its behavior, since it has some subtle behavior.

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/extensions/...
File chrome/browser/extensions/updater/extension_updater_unittest.cc (right):

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/extensions/...
chrome/browser/extensions/updater/extension_updater_unittest.cc:434:
prefs_.reset(new TestExtensionPrefs(blocking_pool_));
can pass in an loop_.message_loop_proxy() instead

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/extensions/...
chrome/browser/extensions/updater/extension_updater_unittest.cc:447:
blocking_pool_->FlushForTesting();
see similar comment in TestExtensionPrefs

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_service_mock_builder.cc (right):

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_service_mock_builder.cc:79:
PrefServiceMockBuilder::WithUserFilePrefs(const FilePath& prefs_file) {
hmmm...is there any way to remove this overload, and have everything use the one
below instead?

If there's too many places that call this, then maybe just add a TODO and have
it call the utility function from JsonPrefStore.

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_service_mock_builder.h (right):

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_service_mock_builder.h:54: base::SequencedWorkerPool*
blocking_pool);
can you make this take an STR instead?

http://codereview.chromium.org/11027070/diff/46005/chrome/common/json_pref_st...
File chrome/common/json_pref_store.h (right):

http://codereview.chromium.org/11027070/diff/46005/chrome/common/json_pref_st...
chrome/common/json_pref_store.h:33: // operations on the same file will be
executes in sequenced order.
executes -> executed

http://codereview.chromium.org/11027070/diff/46005/chrome/service/service_pro...
File chrome/service/service_process_prefs.h (right):

http://codereview.chromium.org/11027070/diff/46005/chrome/service/service_pro...
chrome/service/service_process_prefs.h:24: ServiceProcessPrefs(const FilePath&
pref_filename,
can you have this take a STR instead?  (Similar to PrefService)

[zel, 2012-10-19 01:20:31]

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/browser_pro...
File chrome/browser/browser_process_impl.h (right):

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/browser_pro...
chrome/browser/browser_process_impl.h:45: explicit BrowserProcessImpl(
On 2012/10/18 23:52:24, akalin wrote:
> remove explicit

Done.

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/browser_pro...
chrome/browser/browser_process_impl.h:46: base::SequencedTaskRunner*
local_state_task_runner,
On 2012/10/18 23:52:24, akalin wrote:
> fwd-declare SequencedTaskRunner

Done.

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/browser_pro...
chrome/browser/browser_process_impl.h:46: base::SequencedTaskRunner*
local_state_task_runner,
On 2012/10/18 23:52:24, akalin wrote:
> add comment saying that local_state_task_runner must be a shutdown-blocking
task
> runner.

Done.

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/browser_pro...
chrome/browser/browser_process_impl.h:231:
scoped_refptr<base::SequencedTaskRunner> local_state_task_runner_;
On 2012/10/18 23:52:24, akalin wrote:
> const scoped_refptr<...> ...

Done.

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:271: // Returns the new local state
object, guaranteed non-NULL.
On 2012/10/18 23:52:24, akalin wrote:
> add a comment saying that local_state_task_runner must be a shutdown-blocking
> task runner

Done.

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:712:
scoped_refptr<base::SequencedTaskRunner> local_state_task_runner =
On 2012/10/18 23:52:24, akalin wrote:
> can you instead get the local state path here and create the task runner using
> it?

Done.

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs_unittest.cc:58: prefs_.reset(new
TestExtensionPrefs(blocking_pool_));
On 2012/10/18 23:52:24, akalin wrote:
> you can use message_loop_.message_loop_proxy() (see other comments)

Done.

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs_unittest.cc:58: prefs_.reset(new
TestExtensionPrefs(blocking_pool_));
On 2012/10/18 23:52:24, akalin wrote:
> you can use message_loop_.message_loop_proxy() (see other comments)

Done.

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs_unittest.cc:58: prefs_.reset(new
TestExtensionPrefs(blocking_pool_));
On 2012/10/18 23:52:24, akalin wrote:
> you can use message_loop_.message_loop_proxy() (see other comments)

Done.

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs_unittest.cc:81:
blocking_pool_->FlushForTesting();
On 2012/10/18 23:52:24, akalin wrote:
> can use message_loop_.RunAllPending() here

Done.

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs_unittest.cc:81:
blocking_pool_->FlushForTesting();
On 2012/10/18 23:52:24, akalin wrote:
> can use message_loop_.RunAllPending() here

Done.

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs_unittest.cc:81:
blocking_pool_->FlushForTesting();
On 2012/10/18 23:52:24, akalin wrote:
> can use message_loop_.RunAllPending() here

Done.

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/extensions/...
File chrome/browser/extensions/test_extension_prefs.cc (right):

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/extensions/...
chrome/browser/extensions/test_extension_prefs.cc:79:
blocking_pool_->FlushForTesting();
On 2012/10/18 23:52:24, akalin wrote:
> i think you can replace the FlushForTesting call with:
> 
> void DoNothing() {}
> 
>     base::RunLoop run_loop;
>     ASSERT_TRUE(
>         task_runner_->PostTaskAndReply(
>             FROM_HERE,
>             base::Bind(&DoNothing),
>             run_loop.QuitClosure()));
>     run_loop.Run();
> 
> If CommitPendingWrite() just posts a task to the task_runner_, that should
> suffice.

Done.

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/extensions/...
File chrome/browser/extensions/test_extension_prefs.h (right):

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/extensions/...
chrome/browser/extensions/test_extension_prefs.h:30: explicit
TestExtensionPrefs(base::SequencedWorkerPool* blocking_pool);
On 2012/10/18 23:52:24, akalin wrote:
> can you change to take an STR instead? (See my other comments for
PrefStoreMock,
> etc.)
> 
> In general, I'd like to minimize use of SWP in unit tests that aren't
explicitly
> testing its behavior, since it has some subtle behavior.

Done.

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_service_mock_builder.cc (right):

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_service_mock_builder.cc:79:
PrefServiceMockBuilder::WithUserFilePrefs(const FilePath& prefs_file) {
On 2012/10/18 23:52:24, akalin wrote:
> hmmm...is there any way to remove this overload, and have everything use the
one
> below instead?
> 
> If there's too many places that call this, then maybe just add a TODO and have
> it call the utility function from JsonPrefStore.

I've did that on 2 more places and the only one remaining is in chrome_frame
which I am scared to touch (not even sure if my CL is even working on those
tests yet). I added TODO comment in the header.

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_service_mock_builder.h (right):

http://codereview.chromium.org/11027070/diff/46005/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_service_mock_builder.h:54: base::SequencedWorkerPool*
blocking_pool);
On 2012/10/18 23:52:24, akalin wrote:
> can you make this take an STR instead?

Done.

http://codereview.chromium.org/11027070/diff/46005/chrome/common/json_pref_st...
File chrome/common/json_pref_store.h (right):

http://codereview.chromium.org/11027070/diff/46005/chrome/common/json_pref_st...
chrome/common/json_pref_store.h:33: // operations on the same file will be
executes in sequenced order.
On 2012/10/18 23:52:24, akalin wrote:
> executes -> executed

Done.

[akalin, 2012-10-19 02:00:51]

one more pass.  mostly nits, but a few more substantial comments here and there

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/browser_pro...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/browser_pro...
chrome/browser/browser_process_impl.cc:19: #include
"base/threading/sequenced_worker_pool.h"
not needed anymore

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:28: #include
"base/threading/sequenced_worker_pool.h"
not needed anymore (if you use GetTaskRunnerForFile below)

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:715:
scoped_refptr<base::SequencedTaskRunner> local_state_task_runner =
did you forget to use JsonPrefStore::GetTaskRunnerForFile() here?

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs_unittest.h (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs_unittest.h:14: class
SequencedWorkerPool;
not needed anymore, right?

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs_unittest.h:50:
scoped_ptr<TestExtensionPrefs> prefs_;
why did you make this a scoped_ptr?  It looks like it's not necessary anymore;
you create it only in the constructor and destroy it only in the destructor.

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service_unittest.cc (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/extensions/...
chrome/browser/extensions/extension_service_unittest.cc:27: #include
"base/threading/sequenced_worker_pool.h"
this isn't needed anymore, is it?

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/extensions/...
chrome/browser/extensions/extension_service_unittest.cc:413:
BrowserThread::GetBlockingPool()->FlushForTesting();
this isn't needed anymore, is it?

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/extensions/...
File chrome/browser/extensions/menu_manager_unittest.cc (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/extensions/...
chrome/browser/extensions/menu_manager_unittest.cc:50: ~MenuManagerTest() {
destructor should be virtual

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/extensions/...
chrome/browser/extensions/menu_manager_unittest.cc:93:
scoped_ptr<TestExtensionPrefs> prefs_;
as above, don't think this needs to be a scoped_ptr anymore

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/extensions/...
File chrome/browser/extensions/test_extension_prefs.cc (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/extensions/...
chrome/browser/extensions/test_extension_prefs.cc:64: task_runner_ =
task_runner;
initialize in constructor list

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/extensions/...
chrome/browser/extensions/test_extension_prefs.cc:81:
pref_service_->CommitPendingWrite();
probably worth adding a comment.  Something like:

// Commit a pending write (which posts a task to task_runner_) and wait for it
to finish.

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/extensions/...
File chrome/browser/extensions/test_extension_prefs.h (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/extensions/...
chrome/browser/extensions/test_extension_prefs.h:11: #include
"base/message_loop.h"
is this needed?

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/extensions/...
File chrome/browser/extensions/updater/extension_updater_unittest.cc (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/extensions/...
chrome/browser/extensions/updater/extension_updater_unittest.cc:213:
TestExtensionPrefs* prefs_;
TestExtensionPrefs* const prefs_;

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/extensions/...
chrome/browser/extensions/updater/extension_updater_unittest.cc:431:
prefs_.reset(new TestExtensionPrefs(loop_.message_loop_proxy()));
if you move the code in SetUp/TearDown to constructor/destructor you can make
prefs_ a TestExtensionPrefs instead of a scoped_ptr<>.  (I think there's no
disadvantage to doing so)

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_service_mock_builder.cc (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_service_mock_builder.cc:91: prefs_file,
can these two lines fit on the previous lien?

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:215:
scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner =
use JsonPrefStore::CreateTaskRunnerForFile() here?

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.h:148:
scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner);
use a raw pointer here to be consistent with the other functions that take a
STR?

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/sync/creden...
File chrome/browser/sync/credential_cache_service_win.cc (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:698:
BrowserThread::GetBlockingPool());
change this one to use GetTaskRunnerForFile like above?

http://codereview.chromium.org/11027070/diff/44005/chrome/common/important_fi...
File chrome/common/important_file_writer.cc (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/common/important_fi...
chrome/common/important_file_writer.cc:15: #include "base/task_runner.h"
sequenced_task_runner

http://codereview.chromium.org/11027070/diff/44005/chrome/common/important_fi...
chrome/common/important_file_writer.cc:125: FROM_HERE,
base::Bind(&WriteToDiskTask, path_, data))) {
fix indent here

http://codereview.chromium.org/11027070/diff/44005/chrome/common/important_fi...
File chrome/common/important_file_writer.h (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/common/important_fi...
chrome/common/important_file_writer.h:99:
scoped_refptr<base::SequencedTaskRunner> task_runner_;
const scoped_refptr

http://codereview.chromium.org/11027070/diff/44005/chrome/common/json_pref_st...
File chrome/common/json_pref_store.cc (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/common/json_pref_st...
chrome/common/json_pref_store.cc:90: scoped_refptr<base::SequencedTaskRunner>
sequenced_task_runner_;
const scoped_refptr (may as well make the other scoped_refptrs const, too)

http://codereview.chromium.org/11027070/diff/44005/chrome/common/json_pref_st...
File chrome/common/json_pref_store.h (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/common/json_pref_st...
chrome/common/json_pref_store.h:34: static
scoped_refptr<base::SequencedTaskRunner> GetTaskRunnerForFile(
have this return a raw pointer like Create() below. (It's the recommended way,
counter-intuitively)

http://codereview.chromium.org/11027070/diff/44005/chrome/common/json_pref_st...
chrome/common/json_pref_store.h:39: // perform all file I/O operations on
|sequenced_task_runner|.
comment here that |sequenced_task_runner| must be a shutdown-blocking task
runner, and that it should (almost?) always be one created by
GetTaskRunnerForFile above.

http://codereview.chromium.org/11027070/diff/44005/chrome/common/json_pref_st...
chrome/common/json_pref_store.h:74: // |task_runner| is the TaskRunner for a
thread on which file I/O can be done.
update comment:

task_runner -> sequenced_task_runner
TaskRunner for a thread -> SequencedTaskRunner

http://codereview.chromium.org/11027070/diff/44005/chrome/common/json_pref_st...
chrome/common/json_pref_store.h:83: scoped_refptr<base::SequencedTaskRunner>
sequenced_task_runner_;
const scoped_refptr

http://codereview.chromium.org/11027070/diff/44005/chrome/common/json_pref_st...
File chrome/common/json_pref_store_unittest.cc (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/common/json_pref_st...
chrome/common/json_pref_store_unittest.cc:100: void
RunBasicJsonPrefStoreTest(base::SequencedWorkerPool* blocking_pool,
looks like you don't actually use the blocking pool in this test?  If you can
remove this, then I think you can just use a regular message loop proxy for all
the tests in this file.

(Unless you want to explicitly test behavior with BlockingPools here?  If so,
add a comment.)

http://codereview.chromium.org/11027070/diff/44005/chrome/common/json_pref_st...
chrome/common/json_pref_store_unittest.cc:101: JsonPrefStore *pref_store,
' *' -> '* '

http://codereview.chromium.org/11027070/diff/44005/chrome/common/json_pref_st...
chrome/common/json_pref_store_unittest.cc:217: MockReadErrorDelegate
*mock_error_delegate = new MockReadErrorDelegate;
' *' -> '* '

http://codereview.chromium.org/11027070/diff/44005/chrome/service/service_pro...
File chrome/service/service_process_prefs.h (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/service/service_pro...
chrome/service/service_process_prefs.h:25: base::SequencedWorkerPool*
blocking_pool);
can you make this take an STR instead?

(Did I mention this already?  I don't see a reply...)

[zel, 2012-10-19 18:45:07]

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/browser_pro...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/browser_pro...
chrome/browser/browser_process_impl.cc:19: #include
"base/threading/sequenced_worker_pool.h"
On 2012/10/19 02:00:51, akalin wrote:
> not needed anymore

Done.

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:28: #include
"base/threading/sequenced_worker_pool.h"
On 2012/10/19 02:00:51, akalin wrote:
> not needed anymore (if you use GetTaskRunnerForFile below)

Done.

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:715:
scoped_refptr<base::SequencedTaskRunner> local_state_task_runner =
On 2012/10/19 02:00:51, akalin wrote:
> did you forget to use JsonPrefStore::GetTaskRunnerForFile() here?

Done.

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs_unittest.h (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs_unittest.h:14: class
SequencedWorkerPool;
On 2012/10/19 02:00:51, akalin wrote:
> not needed anymore, right?

Done.

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs_unittest.h:50:
scoped_ptr<TestExtensionPrefs> prefs_;
On 2012/10/19 02:00:51, akalin wrote:
> why did you make this a scoped_ptr?  It looks like it's not necessary anymore;
> you create it only in the constructor and destroy it only in the destructor.

Done.

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service_unittest.cc (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/extensions/...
chrome/browser/extensions/extension_service_unittest.cc:27: #include
"base/threading/sequenced_worker_pool.h"
On 2012/10/19 02:00:51, akalin wrote:
> this isn't needed anymore, is it?

Done.

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/extensions/...
chrome/browser/extensions/extension_service_unittest.cc:413:
BrowserThread::GetBlockingPool()->FlushForTesting();
On 2012/10/19 02:00:51, akalin wrote:
> this isn't needed anymore, is it?

Done.

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/extensions/...
File chrome/browser/extensions/menu_manager_unittest.cc (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/extensions/...
chrome/browser/extensions/menu_manager_unittest.cc:50: ~MenuManagerTest() {
On 2012/10/19 02:00:51, akalin wrote:
> destructor should be virtual

Removed.

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/extensions/...
chrome/browser/extensions/menu_manager_unittest.cc:93:
scoped_ptr<TestExtensionPrefs> prefs_;
On 2012/10/19 02:00:51, akalin wrote:
> as above, don't think this needs to be a scoped_ptr anymore

Done.

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/extensions/...
File chrome/browser/extensions/test_extension_prefs.cc (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/extensions/...
chrome/browser/extensions/test_extension_prefs.cc:64: task_runner_ =
task_runner;
On 2012/10/19 02:00:51, akalin wrote:
> initialize in constructor list

Done.

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/extensions/...
chrome/browser/extensions/test_extension_prefs.cc:81:
pref_service_->CommitPendingWrite();
On 2012/10/19 02:00:51, akalin wrote:
> probably worth adding a comment.  Something like:
> 
> // Commit a pending write (which posts a task to task_runner_) and wait for it
> to finish.

Done.

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/extensions/...
File chrome/browser/extensions/test_extension_prefs.h (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/extensions/...
chrome/browser/extensions/test_extension_prefs.h:11: #include
"base/message_loop.h"
On 2012/10/19 02:00:51, akalin wrote:
> is this needed?

Done.

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_service_mock_builder.cc (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_service_mock_builder.cc:91: prefs_file,
On 2012/10/19 02:00:51, akalin wrote:
> can these two lines fit on the previous lien?

Done.

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:215:
scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner =
On 2012/10/19 02:00:51, akalin wrote:
> use JsonPrefStore::CreateTaskRunnerForFile() here?

Done.

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.h:148:
scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner);
On 2012/10/19 02:00:51, akalin wrote:
> use a raw pointer here to be consistent with the other functions that take a
> STR?

Done.

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/sync/creden...
File chrome/browser/sync/credential_cache_service_win.cc (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:698:
BrowserThread::GetBlockingPool());
On 2012/10/19 02:00:51, akalin wrote:
> change this one to use GetTaskRunnerForFile like above?

Done.

http://codereview.chromium.org/11027070/diff/44005/chrome/common/important_fi...
File chrome/common/important_file_writer.cc (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/common/important_fi...
chrome/common/important_file_writer.cc:125: FROM_HERE,
base::Bind(&WriteToDiskTask, path_, data))) {
On 2012/10/19 02:00:51, akalin wrote:
> fix indent here

Done.

http://codereview.chromium.org/11027070/diff/44005/chrome/common/json_pref_st...
File chrome/common/json_pref_store.cc (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/common/json_pref_st...
chrome/common/json_pref_store.cc:90: scoped_refptr<base::SequencedTaskRunner>
sequenced_task_runner_;
On 2012/10/19 02:00:51, akalin wrote:
> const scoped_refptr (may as well make the other scoped_refptrs const, too)

Done. Changed few additional ones that I could.

http://codereview.chromium.org/11027070/diff/44005/chrome/common/json_pref_st...
File chrome/common/json_pref_store.h (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/common/json_pref_st...
chrome/common/json_pref_store.h:34: static
scoped_refptr<base::SequencedTaskRunner> GetTaskRunnerForFile(
On 2012/10/19 02:00:51, akalin wrote:
> have this return a raw pointer like Create() below. (It's the recommended way,
> counter-intuitively)

Really? SequencedWorkerPool::GetSequencedTaskRunnerWithShutdownBehavior returns
scoped_refptr<SequencedTaskRunner> and I wanted to stay consistent with that.

Wouldn't we nuke the instance of SequencedTaskRunner object if the returned
scoped_refptr<SequencedTaskRunner> from
SequencedWorkerPool::GetSequencedTaskRunnerWithShutdownBehavior goes out of
scope at the end of this function?

http://codereview.chromium.org/11027070/diff/44005/chrome/common/json_pref_st...
chrome/common/json_pref_store.h:39: // perform all file I/O operations on
|sequenced_task_runner|.
On 2012/10/19 02:00:51, akalin wrote:
> comment here that |sequenced_task_runner| must be a shutdown-blocking task
> runner, and that it should (almost?) always be one created by
> GetTaskRunnerForFile above.

Done.

http://codereview.chromium.org/11027070/diff/44005/chrome/common/json_pref_st...
chrome/common/json_pref_store.h:74: // |task_runner| is the TaskRunner for a
thread on which file I/O can be done.
On 2012/10/19 02:00:51, akalin wrote:
> update comment:
> 
> task_runner -> sequenced_task_runner
> TaskRunner for a thread -> SequencedTaskRunner

Done.

http://codereview.chromium.org/11027070/diff/44005/chrome/common/json_pref_st...
chrome/common/json_pref_store.h:83: scoped_refptr<base::SequencedTaskRunner>
sequenced_task_runner_;
On 2012/10/19 02:00:51, akalin wrote:
> const scoped_refptr

Done.

http://codereview.chromium.org/11027070/diff/44005/chrome/common/json_pref_st...
File chrome/common/json_pref_store_unittest.cc (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/common/json_pref_st...
chrome/common/json_pref_store_unittest.cc:217: MockReadErrorDelegate
*mock_error_delegate = new MockReadErrorDelegate;
On 2012/10/19 02:00:51, akalin wrote:
> ' *' -> '* '

Done.

http://codereview.chromium.org/11027070/diff/44005/chrome/service/service_pro...
File chrome/service/service_process_prefs.h (right):

http://codereview.chromium.org/11027070/diff/44005/chrome/service/service_pro...
chrome/service/service_process_prefs.h:25: base::SequencedWorkerPool*
blocking_pool);
On 2012/10/19 02:00:51, akalin wrote:
> can you make this take an STR instead?
> 
> (Did I mention this already?  I don't see a reply...)

Done.

http://codereview.chromium.org/11027070/diff/44005/chrome/service/service_pro...
chrome/service/service_process_prefs.h:25: base::SequencedWorkerPool*
blocking_pool);
On 2012/10/19 02:00:51, akalin wrote:
> can you make this take an STR instead?
> 
> (Did I mention this already?  I don't see a reply...)

Done.

[akalin, 2012-10-19 23:01:16]

> Wouldn't we nuke the instance of SequencedTaskRunner object if the returned
> scoped_refptr<SequencedTaskRunner> from
> SequencedWorkerPool::GetSequencedTaskRunnerWithShutdownBehavior goes out of
> scope at the end of this function?

Yes.  Unfortunately, it is possible to assign the return value of a function
that returns scoped_refptr<T> to a T*.  In that case, the object assigned to the
T* will already be nuked, and so there's a use-after-free bug.  This is worse
than the memory leak you'd get if the function returned T* and you assigned to a
T*.  So that's why, the recommendation is to return a raw pointer.

For the record, I dislike the implicit conversion from scoped_refptr<T> to T*. 
But I think it's here to stay. :(

[akalin, 2012-10-19 23:12:21]

LGTM after nits below

http://codereview.chromium.org/11027070/diff/59033/chrome/browser/browser_pro...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/11027070/diff/59033/chrome/browser/browser_pro...
chrome/browser/browser_process_impl.cc:718:
PathService::Get(chrome::FILE_LOCAL_STATE, &local_state_path);
CHECK return value or otherwise handle?

http://codereview.chromium.org/11027070/diff/59033/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

http://codereview.chromium.org/11027070/diff/59033/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:713:
PathService::Get(chrome::FILE_LOCAL_STATE, &local_state_path);
hmm. CHECK or otherwise handle if Get() returns false and local_state_path isn't
filled in?

http://codereview.chromium.org/11027070/diff/59033/chrome/browser/extensions/...
File chrome/browser/extensions/test_extension_prefs.h (right):

http://codereview.chromium.org/11027070/diff/59033/chrome/browser/extensions/...
chrome/browser/extensions/test_extension_prefs.h:80:
scoped_refptr<base::SequencedTaskRunner> task_runner_;
const scoped_refptr

http://codereview.chromium.org/11027070/diff/59033/chrome/browser/extensions/...
File chrome/browser/extensions/updater/extension_updater_unittest.cc (right):

http://codereview.chromium.org/11027070/diff/59033/chrome/browser/extensions/...
chrome/browser/extensions/updater/extension_updater_unittest.cc:439:
prefs_.reset(NULL);
can just do ".reset()"

http://codereview.chromium.org/11027070/diff/59033/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/11027070/diff/59033/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_service.cc:19: #include
"base/threading/sequenced_worker_pool.h"
remove unused include

http://codereview.chromium.org/11027070/diff/59033/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/11027070/diff/59033/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.h:148: base::SequencedTaskRunner*
sequenced_task_runner);
fwd-declare

http://codereview.chromium.org/11027070/diff/59033/chrome/common/important_fi...
File chrome/common/important_file_writer.h (right):

http://codereview.chromium.org/11027070/diff/59033/chrome/common/important_fi...
chrome/common/important_file_writer.h:99:
scoped_refptr<base::SequencedTaskRunner> task_runner_;
const

http://codereview.chromium.org/11027070/diff/59033/chrome/common/json_pref_st...
File chrome/common/json_pref_store.h (right):

http://codereview.chromium.org/11027070/diff/59033/chrome/common/json_pref_st...
chrome/common/json_pref_store.h:34: static
scoped_refptr<base::SequencedTaskRunner> GetTaskRunnerForFile(
as discussed, please make this return a raw pointer

http://codereview.chromium.org/11027070/diff/59033/chrome/common/json_pref_st...
chrome/common/json_pref_store.h:76: // |sequenced_task_runner| is a
SequencedTaskRunner on which file I/O can
remove 'can'

http://codereview.chromium.org/11027070/diff/59033/chrome/service/cloud_print...
File chrome/service/cloud_print/connector_settings_unittest.cc (right):

http://codereview.chromium.org/11027070/diff/59033/chrome/service/cloud_print...
chrome/service/cloud_print/connector_settings_unittest.cc:12: #include
"base/threading/sequenced_worker_pool.h"
remove this include

http://codereview.chromium.org/11027070/diff/59033/chrome/service/service_pro...
File chrome/service/service_process.h (right):

http://codereview.chromium.org/11027070/diff/59033/chrome/service/service_pro...
chrome/service/service_process.h:127: scoped_refptr<base::SequencedWorkerPool>
blocking_pool_;
const?

http://codereview.chromium.org/11027070/diff/59033/chrome/service/service_pro...
File chrome/service/service_process_prefs.cc (right):

http://codereview.chromium.org/11027070/diff/59033/chrome/service/service_pro...
chrome/service/service_process_prefs.cc:7: #include "base/message_loop_proxy.h"
i think you can remove this include

http://codereview.chromium.org/11027070/diff/59033/chrome/service/service_pro...
File chrome/service/service_process_prefs_unittest.cc (right):

http://codereview.chromium.org/11027070/diff/59033/chrome/service/service_pro...
chrome/service/service_process_prefs_unittest.cc:10: #include
"base/threading/sequenced_worker_pool.h"
remove this include

http://codereview.chromium.org/11027070/diff/59033/chrome/service/service_pro...
chrome/service/service_process_prefs_unittest.cc:26: prefs_.reset(NULL);
you can just go ".reset();" (and I think that's the usual style)

http://codereview.chromium.org/11027070/diff/59033/chrome/test/ui/ui_test.h
File chrome/test/ui/ui_test.h (left):

http://codereview.chromium.org/11027070/diff/59033/chrome/test/ui/ui_test.h#o...
chrome/test/ui/ui_test.h:431: private:
is this still needed?  looks like you can revert this diff

[zel, 2012-10-21 20:03:18]

http://codereview.chromium.org/11027070/diff/59033/chrome/browser/browser_pro...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/11027070/diff/59033/chrome/browser/browser_pro...
chrome/browser/browser_process_impl.cc:718:
PathService::Get(chrome::FILE_LOCAL_STATE, &local_state_path);
On 2012/10/19 23:12:21, akalin wrote:
> CHECK return value or otherwise handle?

Done.

http://codereview.chromium.org/11027070/diff/59033/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

http://codereview.chromium.org/11027070/diff/59033/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:713:
PathService::Get(chrome::FILE_LOCAL_STATE, &local_state_path);
On 2012/10/19 23:12:21, akalin wrote:
> hmm. CHECK or otherwise handle if Get() returns false and local_state_path
isn't
> filled in?

Done.

http://codereview.chromium.org/11027070/diff/59033/chrome/browser/extensions/...
File chrome/browser/extensions/test_extension_prefs.h (right):

http://codereview.chromium.org/11027070/diff/59033/chrome/browser/extensions/...
chrome/browser/extensions/test_extension_prefs.h:80:
scoped_refptr<base::SequencedTaskRunner> task_runner_;
On 2012/10/19 23:12:21, akalin wrote:
> const scoped_refptr

Done.

http://codereview.chromium.org/11027070/diff/59033/chrome/browser/extensions/...
File chrome/browser/extensions/updater/extension_updater_unittest.cc (right):

http://codereview.chromium.org/11027070/diff/59033/chrome/browser/extensions/...
chrome/browser/extensions/updater/extension_updater_unittest.cc:439:
prefs_.reset(NULL);
On 2012/10/19 23:12:21, akalin wrote:
> can just do ".reset()"

Done.

http://codereview.chromium.org/11027070/diff/59033/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_service.cc (right):

http://codereview.chromium.org/11027070/diff/59033/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_service.cc:19: #include
"base/threading/sequenced_worker_pool.h"
On 2012/10/19 23:12:21, akalin wrote:
> remove unused include

Done.

http://codereview.chromium.org/11027070/diff/59033/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/11027070/diff/59033/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.h:148: base::SequencedTaskRunner*
sequenced_task_runner);
On 2012/10/19 23:12:21, akalin wrote:
> fwd-declare

Done.

http://codereview.chromium.org/11027070/diff/59033/chrome/common/important_fi...
File chrome/common/important_file_writer.h (right):

http://codereview.chromium.org/11027070/diff/59033/chrome/common/important_fi...
chrome/common/important_file_writer.h:99:
scoped_refptr<base::SequencedTaskRunner> task_runner_;
On 2012/10/19 23:12:21, akalin wrote:
> const

Done.

http://codereview.chromium.org/11027070/diff/59033/chrome/common/json_pref_st...
File chrome/common/json_pref_store.h (right):

http://codereview.chromium.org/11027070/diff/59033/chrome/common/json_pref_st...
chrome/common/json_pref_store.h:34: static
scoped_refptr<base::SequencedTaskRunner> GetTaskRunnerForFile(
On 2012/10/19 23:12:21, akalin wrote:
> as discussed, please make this return a raw pointer

Done.

http://codereview.chromium.org/11027070/diff/59033/chrome/common/json_pref_st...
chrome/common/json_pref_store.h:76: // |sequenced_task_runner| is a
SequencedTaskRunner on which file I/O can
On 2012/10/19 23:12:21, akalin wrote:
> remove 'can'

As I explained, that would be either cause a crash (confirmed in tests) or we
also need to change declaration of:

scoped_refptr<SequencedTaskRunner>
SequencedWorkerPool::GetSequencedTaskRunnerWithShutdownBehavior()

http://codereview.chromium.org/11027070/diff/59033/chrome/service/cloud_print...
File chrome/service/cloud_print/connector_settings_unittest.cc (right):

http://codereview.chromium.org/11027070/diff/59033/chrome/service/cloud_print...
chrome/service/cloud_print/connector_settings_unittest.cc:12: #include
"base/threading/sequenced_worker_pool.h"
On 2012/10/19 23:12:21, akalin wrote:
> remove this include

Done.

http://codereview.chromium.org/11027070/diff/59033/chrome/service/service_pro...
File chrome/service/service_process.h (right):

http://codereview.chromium.org/11027070/diff/59033/chrome/service/service_pro...
chrome/service/service_process.h:127: scoped_refptr<base::SequencedWorkerPool>
blocking_pool_;
On 2012/10/19 23:12:21, akalin wrote:
> const?

Does not work since this member is created in Initialize()

http://codereview.chromium.org/11027070/diff/59033/chrome/service/service_pro...
File chrome/service/service_process_prefs_unittest.cc (right):

http://codereview.chromium.org/11027070/diff/59033/chrome/service/service_pro...
chrome/service/service_process_prefs_unittest.cc:10: #include
"base/threading/sequenced_worker_pool.h"
On 2012/10/19 23:12:21, akalin wrote:
> remove this include

Done.

http://codereview.chromium.org/11027070/diff/59033/chrome/service/service_pro...
chrome/service/service_process_prefs_unittest.cc:26: prefs_.reset(NULL);
On 2012/10/19 23:12:21, akalin wrote:
> you can just go ".reset();" (and I think that's the usual style)

Done.

http://codereview.chromium.org/11027070/diff/59033/chrome/test/ui/ui_test.h
File chrome/test/ui/ui_test.h (left):

http://codereview.chromium.org/11027070/diff/59033/chrome/test/ui/ui_test.h#o...
chrome/test/ui/ui_test.h:431: private:
On 2012/10/19 23:12:21, akalin wrote:
> is this still needed?  looks like you can revert this diff

Yes, it's needed. One of the sub-classes need to access this member.

[commit-bot: I haz the power, 2012-10-22 16:43:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zelidrag@chromium.org/11027070/59036

[commit-bot: I haz the power, 2012-10-22 16:44:15]

Presubmit check for 11027070-59036 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/prefs
    chrome/browser/policy
    chrome
    chrome/browser/extensions/updater
    chrome/browser/extensions
    chrome/test

Presubmit checks took 3.4s to calculate.

[Mattias Nissler (ping if slow), 2012-10-22 17:28:20]

Given that I'm late to the party, it might be that most of my questions might
have been answered. Still checking though as this stuff causes big pain if it
breaks subtly.

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/browser/bro...
File chrome/browser/browser_process_impl.cc (right):

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/browser/bro...
chrome/browser/browser_process_impl.cc:151:
local_state_task_runner_(local_state_task_runner) {
This file uses well-known browser threads elsewhere, so it should probably
BrowserThread::GetBlockingPool() directly to obtain a task runner? Or is there a
good reason to pass this in?

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/browser/chr...
File chrome/browser/chrome_browser_main.cc (right):

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/browser/chr...
chrome/browser/chrome_browser_main.cc:721: 
nit: remove extra blank line.

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/browser/ext...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/browser/ext...
chrome/browser/extensions/extension_prefs_unittest.cc:55:
file_thread_(BrowserThread::FILE, &message_loop_),
Have you verified that we still need the mock file thread now that prefs use
their own proxy?

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/browser/ext...
chrome/browser/extensions/extension_prefs_unittest.cc:77:
message_loop_.RunAllPending();
Why are these two needed? Could use a comment.

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/browser/pre...
File chrome/browser/prefs/pref_service.h (right):

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/browser/pre...
chrome/browser/prefs/pref_service.h:118: base::SequencedTaskRunner*
sequenced_task_runner);
nit: move next to pref_filename and maybe rename to pref_io_task_runner to make
clear what it is used for.

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/browser/pre...
File chrome/browser/prefs/pref_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/browser/pre...
chrome/browser/prefs/pref_service_unittest.cc:304: // A message loop that we
need for firing timers.
It's not timers AFAICS.

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/browser/pro...
File chrome/browser/profiles/profile_impl.cc (right):

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/browser/pro...
chrome/browser/profiles/profile_impl.cc:225:
base::Bind(&CreateDirectoryNoResult, path));
This is dangerous if other code that is still on the FILE thread also access the
directory if that code assumes the directory exists. Have you checked?

edit: I found you've adjusted a couple of other file operations. If so, what are
we gaining? If the FILE thread was clogged before, so is the task runner
afterwards. Or is there more stuff going on on the FILE thread? It seems you
want the directory created and then have lots of parallelism instead of all file
consumers on the same task runner, no?

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/browser/syn...
File chrome/browser/sync/credential_cache_service_win.cc (right):

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/browser/syn...
chrome/browser/sync/credential_cache_service_win.cc:647:
BrowserThread::GetBlockingPool()));
Do we have the assumption that the profile directory has been created here? If
so, are we sure that has actually happened, given that the dir creation task may
run on a different blocking pool thread?

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/common/json...
File chrome/common/json_pref_store.cc (right):

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/common/json...
chrome/common/json_pref_store.cc:143: const FilePath& filename,
base::SequencedWorkerPool* worker_pool) {
parameters on separate lines.

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/common/json...
chrome/common/json_pref_store.cc:145:
worker_pool->GetNamedSequenceToken(filename.AsUTF8Unsafe()),
indentation

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/common/json...
chrome/common/json_pref_store.cc:145:
worker_pool->GetNamedSequenceToken(filename.AsUTF8Unsafe()),
The token name should probably be json_pref_store-<filename> to avoid namespace
collisions.

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/common/json...
chrome/common/json_pref_store.cc:150: const FilePath& filename,
base::SequencedTaskRunner* task_runner) {
ditto.

Also, what is this factory function good for if I can just as well invoke the
ctor directly?

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/common/json...
File chrome/common/json_pref_store_unittest.cc (right):

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/common/json...
chrome/common/json_pref_store_unittest.cc:51: // A message loop that we need for
timers.
It's actually not used for timers?

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/service/ser...
File chrome/service/service_process_prefs.cc (right):

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/service/ser...
chrome/service/service_process_prefs.cc:7: #include "base/message_loop_proxy.h"
rather base/sequenced_task_runner.h?

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/service/ser...
File chrome/service/service_process_prefs_unittest.cc (right):

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/service/ser...
chrome/service/service_process_prefs_unittest.cc:30: // A message loop that we
need for timers.
Same comment: Actually not used for timers?

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/test/ui/ui_...
File chrome/test/ui/ui_test.h (left):

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/test/ui/ui_...
chrome/test/ui/ui_test.h:431: private:
I guess the idea here was to use MessageLoop::current() instead, since ui test
seem to have an entire browser running.

[akalin, 2012-10-22 17:37:24]

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/browser/bro...
File chrome/browser/browser_process_impl.cc (right):

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/browser/bro...
chrome/browser/browser_process_impl.cc:151:
local_state_task_runner_(local_state_task_runner) {
On 2012/10/22 17:28:21, Mattias Nissler wrote:
> This file uses well-known browser threads elsewhere, so it should probably
> BrowserThread::GetBlockingPool() directly to obtain a task runner? Or is there
a
> good reason to pass this in?

To make sure that it uses the same one as the one in chrome_browser_main

[zel, 2012-10-24 02:20:10]

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/browser/bro...
File chrome/browser/browser_process_impl.cc (right):

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/browser/bro...
chrome/browser/browser_process_impl.cc:151:
local_state_task_runner_(local_state_task_runner) {
On 2012/10/22 17:37:25, akalin wrote:
> On 2012/10/22 17:28:21, Mattias Nissler wrote:
> > This file uses well-known browser threads elsewhere, so it should probably
> > BrowserThread::GetBlockingPool() directly to obtain a task runner? Or is
there
> a
> > good reason to pass this in?
> 
> To make sure that it uses the same one as the one in chrome_browser_main

Yep.

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/browser/chr...
File chrome/browser/chrome_browser_main.cc (right):

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/browser/chr...
chrome/browser/chrome_browser_main.cc:721: 
On 2012/10/22 17:28:21, Mattias Nissler wrote:
> nit: remove extra blank line.

Done.

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/browser/ext...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/browser/ext...
chrome/browser/extensions/extension_prefs_unittest.cc:55:
file_thread_(BrowserThread::FILE, &message_loop_),
On 2012/10/22 17:28:21, Mattias Nissler wrote:
> Have you verified that we still need the mock file thread now that prefs use
> their own proxy?

message_loop_.message_loop_proxy() parameter here is the
base::SequencedTaskRunner that TestExtensionPrefs use.

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/browser/pre...
File chrome/browser/prefs/pref_service.h (right):

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/browser/pre...
chrome/browser/prefs/pref_service.h:118: base::SequencedTaskRunner*
sequenced_task_runner);
On 2012/10/22 17:28:21, Mattias Nissler wrote:
> nit: move next to pref_filename and maybe rename to pref_io_task_runner to
make
> clear what it is used for.

Done.

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/browser/pre...
File chrome/browser/prefs/pref_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/browser/pre...
chrome/browser/prefs/pref_service_unittest.cc:304: // A message loop that we
need for firing timers.
On 2012/10/22 17:28:21, Mattias Nissler wrote:
> It's not timers AFAICS.

Done.

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/browser/pro...
File chrome/browser/profiles/profile_impl.cc (right):

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/browser/pro...
chrome/browser/profiles/profile_impl.cc:225:
base::Bind(&CreateDirectoryNoResult, path));
On 2012/10/22 17:28:21, Mattias Nissler wrote:
> This is dangerous if other code that is still on the FILE thread also access
the
> directory if that code assumes the directory exists. Have you checked?
> 

Yes, I've tested this and adjusted such FILE dependencies through number of
initially failing (warning) browser tests. This was a yak shaving that caused my
to touch all these tests to begin with.

> edit: I found you've adjusted a couple of other file operations. If so, what
are
> we gaining? If the FILE thread was clogged before, so is the task runner
> afterwards. Or is there more stuff going on on the FILE thread? It seems you
> want the directory created and then have lots of parallelism instead of all
file
> consumers on the same task runner, no?

I need parallelism during shutdown which triggers JsonPrefStore serialization. I
need to ensure that FILE thread is not clogged by other less important
operations at that time (i.e. extension install). Otherwise, JsonPrefStore
sometimes had chance to get serialized or even gets corrupted (due to SIGABRT
timeout sent from session manager). This condition caused top crash report on
ChromeOS. Please read http://crbug.com/153367 for more details.

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/browser/syn...
File chrome/browser/sync/credential_cache_service_win.cc (right):

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/browser/syn...
chrome/browser/sync/credential_cache_service_win.cc:647:
BrowserThread::GetBlockingPool()));
On 2012/10/22 17:28:21, Mattias Nissler wrote:
> Do we have the assumption that the profile directory has been created here? If
> so, are we sure that has actually happened, given that the dir creation task
may
> run on a different blocking pool thread?

I am not really sure where that actually happens for CredentialCacheService.
Since this is windows-specific... I am just reverting this to its original task
runner.

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/common/json...
File chrome/common/json_pref_store.cc (right):

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/common/json...
chrome/common/json_pref_store.cc:143: const FilePath& filename,
base::SequencedWorkerPool* worker_pool) {
On 2012/10/22 17:28:21, Mattias Nissler wrote:
> parameters on separate lines.

Done.

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/common/json...
chrome/common/json_pref_store.cc:145:
worker_pool->GetNamedSequenceToken(filename.AsUTF8Unsafe()),
On 2012/10/22 17:28:21, Mattias Nissler wrote:
> indentation

Done.

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/common/json...
chrome/common/json_pref_store.cc:145:
worker_pool->GetNamedSequenceToken(filename.AsUTF8Unsafe()),
On 2012/10/22 17:28:21, Mattias Nissler wrote:
> The token name should probably be json_pref_store-<filename> to avoid
namespace
> collisions.

Done.

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/common/json...
chrome/common/json_pref_store.cc:150: const FilePath& filename,
base::SequencedTaskRunner* task_runner) {
On 2012/10/22 17:28:21, Mattias Nissler wrote:
> ditto.
> 
> Also, what is this factory function good for if I can just as well invoke the
> ctor directly?

long time ago it used to do waaay more but then CR evolved it into
uselessness... removed.

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/common/json...
File chrome/common/json_pref_store_unittest.cc (right):

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/common/json...
chrome/common/json_pref_store_unittest.cc:51: // A message loop that we need for
timers.
On 2012/10/22 17:28:21, Mattias Nissler wrote:
> It's actually not used for timers?

hmm, still no idea where these comments come from. reverted.

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/service/ser...
File chrome/service/service_process_prefs.cc (right):

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/service/ser...
chrome/service/service_process_prefs.cc:7: #include "base/message_loop_proxy.h"
On 2012/10/22 17:28:21, Mattias Nissler wrote:
> rather base/sequenced_task_runner.h?

Done.

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/service/ser...
File chrome/service/service_process_prefs_unittest.cc (right):

https://chromiumcodereview.appspot.com/11027070/diff/59036/chrome/service/ser...
chrome/service/service_process_prefs_unittest.cc:30: // A message loop that we
need for timers.
On 2012/10/22 17:28:21, Mattias Nissler wrote:
> Same comment: Actually not used for timers?

still a mystery. reverted.

[Mattias Nissler (ping if slow), 2012-10-24 13:36:16]

My remaining concern with you current code is that stuff on the FILE thread
cannot assume the profile directory has been created, see my long comment on
more thoughts on that.

If you're adventurous and have convinced yourself that there are no races
between profile dir creation and profile initialization on the file thread, you
have my formal LGTM (with a question/nit regarding file_thread_ in
extension_prefs_unittest.cc) to give it a try :)

Note that I've also referenced a previous attempt at this CL which you may want
to follow up on, because it caused general flakiness, eventually got reverted
and was never relanded.

http://codereview.chromium.org/11027070/diff/59036/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

http://codereview.chromium.org/11027070/diff/59036/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs_unittest.cc:55:
file_thread_(BrowserThread::FILE, &message_loop_),
On 2012/10/24 02:20:11, zel wrote:
> On 2012/10/22 17:28:21, Mattias Nissler wrote:
> > Have you verified that we still need the mock file thread now that prefs use
> > their own proxy?
> 
> message_loop_.message_loop_proxy() parameter here is the
> base::SequencedTaskRunner that TestExtensionPrefs use.

I understand that, but is the local file_thread_ instance still needed? If this
test doesn't check for BrowserThread::FILE any longer, we should remove it.

http://codereview.chromium.org/11027070/diff/59036/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/11027070/diff/59036/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:225:
base::Bind(&CreateDirectoryNoResult, path));
On 2012/10/24 02:20:11, zel wrote:
> On 2012/10/22 17:28:21, Mattias Nissler wrote:
> > This is dangerous if other code that is still on the FILE thread also access
> the
> > directory if that code assumes the directory exists. Have you checked?
> > 
> 
> Yes, I've tested this and adjusted such FILE dependencies through number of
> initially failing (warning) browser tests. This was a yak shaving that caused
my
> to touch all these tests to begin with.

Well, those were probably the easier ones :) I'm concerned with flakes showing
up due to timing issues. In fact, somebody has tried this same CL earlier and
got reverted: https://chromiumcodereview.appspot.com/10344007/ (you might want
to follow up about the reason).

Fixing all these flakes might result in more and more stuff ending up on your
new task runner instance, which is bad (see below), but see my other idea even
further below ;)

> 
> > edit: I found you've adjusted a couple of other file operations. If so, what
> are
> > we gaining? If the FILE thread was clogged before, so is the task runner
> > afterwards. Or is there more stuff going on on the FILE thread? It seems you
> > want the directory created and then have lots of parallelism instead of all
> file
> > consumers on the same task runner, no?
> 
> I need parallelism during shutdown which triggers JsonPrefStore serialization.
I
> need to ensure that FILE thread is not clogged by other less important
> operations at that time (i.e. extension install). Otherwise, JsonPrefStore
> sometimes had chance to get serialized or even gets corrupted (due to SIGABRT
> timeout sent from session manager). This condition caused top crash report on
> ChromeOS. Please read http://crbug.com/153367 for more details.
> 

That's understood. What I'm trying to say is this: If we need to move additional
work to the task runner for correctness (because the directory needs to be
created before stuff accesses it), that'll mean all that stuff will be on the
very same task runner and thus the same thread on shutdown as well. If that's
what we end up doing, then we've just moved everything from BrowserThread::FILE
to a worker pool thread, but the level of parallelism is still the same.

Edit: I just realized that a better solution for the problem of having the
directory created before anything else happens on both the FILE thread and the
pref store task runner is to block both of them on creation. So how about
keeping directory creation on the task runner and posting a separate task to the
FILE thread that blocks on a signal from the other thread once the directory has
been created? That would make sure everything that gets posted to the FILE
thread afterwards can assume the directory is created.

[zel, 2012-10-24 16:49:48]

http://codereview.chromium.org/11027070/diff/59036/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

http://codereview.chromium.org/11027070/diff/59036/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs_unittest.cc:55:
file_thread_(BrowserThread::FILE, &message_loop_),
On 2012/10/24 13:36:16, Mattias Nissler wrote:
> On 2012/10/24 02:20:11, zel wrote:
> > On 2012/10/22 17:28:21, Mattias Nissler wrote:
> > > Have you verified that we still need the mock file thread now that prefs
use
> > > their own proxy?
> > 
> > message_loop_.message_loop_proxy() parameter here is the
> > base::SequencedTaskRunner that TestExtensionPrefs use.
> 
> I understand that, but is the local file_thread_ instance still needed? If
this
> test doesn't check for BrowserThread::FILE any longer, we should remove it.

Yes. Removed.

http://codereview.chromium.org/11027070/diff/59036/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/11027070/diff/59036/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:225:
base::Bind(&CreateDirectoryNoResult, path));
On 2012/10/24 13:36:16, Mattias Nissler wrote:
> On 2012/10/24 02:20:11, zel wrote:
> > On 2012/10/22 17:28:21, Mattias Nissler wrote:
> > > This is dangerous if other code that is still on the FILE thread also
access
> > the
> > > directory if that code assumes the directory exists. Have you checked?
> > > 
> > 
> > Yes, I've tested this and adjusted such FILE dependencies through number of
> > initially failing (warning) browser tests. This was a yak shaving that
caused
> my
> > to touch all these tests to begin with.
> 
> Well, those were probably the easier ones :) I'm concerned with flakes showing
> up due to timing issues. In fact, somebody has tried this same CL earlier and
> got reverted: https://chromiumcodereview.appspot.com/10344007/ (you might want
> to follow up about the reason).
> 
> Fixing all these flakes might result in more and more stuff ending up on your
> new task runner instance, which is bad (see below), but see my other idea even
> further below ;)
> 
> > 
> > > edit: I found you've adjusted a couple of other file operations. If so,
what
> > are
> > > we gaining? If the FILE thread was clogged before, so is the task runner
> > > afterwards. Or is there more stuff going on on the FILE thread? It seems
you
> > > want the directory created and then have lots of parallelism instead of
all
> > file
> > > consumers on the same task runner, no?
> > 
> > I need parallelism during shutdown which triggers JsonPrefStore
serialization.
> I
> > need to ensure that FILE thread is not clogged by other less important
> > operations at that time (i.e. extension install). Otherwise, JsonPrefStore
> > sometimes had chance to get serialized or even gets corrupted (due to
SIGABRT
> > timeout sent from session manager). This condition caused top crash report
on
> > ChromeOS. Please read http://crbug.com/153367 for more details.
> > 
> 
> That's understood. What I'm trying to say is this: If we need to move
additional
> work to the task runner for correctness (because the directory needs to be
> created before stuff accesses it), that'll mean all that stuff will be on the
> very same task runner and thus the same thread on shutdown as well. If that's
> what we end up doing, then we've just moved everything from
BrowserThread::FILE
> to a worker pool thread, but the level of parallelism is still the same.
> 
> Edit: I just realized that a better solution for the problem of having the
> directory created before anything else happens on both the FILE thread and the
> pref store task runner is to block both of them on creation. So how about
> keeping directory creation on the task runner and posting a separate task to
the
> FILE thread that blocks on a signal from the other thread once the directory
has
> been created? That would make sure everything that gets posted to the FILE
> thread afterwards can assume the directory is created.

Yeah, that's a great idea. Done - PTAL.

[Mattias Nissler (ping if slow), 2012-10-24 17:07:49]

LGTM with nits. Fingers crossed :)

http://codereview.chromium.org/11027070/diff/79001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/11027070/diff/79001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:161: void CreateDirectoryNoResult(const
FilePath& path,
nit: Rename to CreateDirectoryAndSignal or somesuch.

http://codereview.chromium.org/11027070/diff/79001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:185: BrowserThread::FILE, FROM_HERE,
nit: indentation

http://codereview.chromium.org/11027070/diff/79001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:236: 
remove extra newline

[zel, 2012-10-24 17:23:14]

http://codereview.chromium.org/11027070/diff/79001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/11027070/diff/79001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:161: void CreateDirectoryNoResult(const
FilePath& path,
On 2012/10/24 17:07:49, Mattias Nissler wrote:
> nit: Rename to CreateDirectoryAndSignal or somesuch.

Done.

http://codereview.chromium.org/11027070/diff/79001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:185: BrowserThread::FILE, FROM_HERE,
On 2012/10/24 17:07:49, Mattias Nissler wrote:
> nit: indentation

Done.

http://codereview.chromium.org/11027070/diff/79001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:236: 
On 2012/10/24 17:07:49, Mattias Nissler wrote:
> remove extra newline

Done.

[Aaron Boodman, 2012-10-25 22:37:03]

lgtm

[miket_OOO, 2012-10-25 22:42:25]

c/b/extensions LGTM

[ananta, 2012-10-25 23:07:49]

LGTM if cf try bots are happy

[sky, 2012-10-25 23:25:53]

LGTM

[Nikita (slow), 2012-11-06 08:53:08]

http://codereview.chromium.org/11027070/diff/94001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (left):

http://codereview.chromium.org/11027070/diff/94001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_utils_browsertest.cc:223: RunAllPending();
This is just to use proper method, right?
Since RunAllPending() already wall calling RunUntilIdle()

[Mattias Nissler (ping if slow), 2012-11-06 09:07:54]

http://codereview.chromium.org/11027070/diff/94001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (right):

http://codereview.chromium.org/11027070/diff/94001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_utils_browsertest.cc:575: 
I think you need a RunUntilIdle here as well? Or are the flakes gone for good
for a different reason?

http://codereview.chromium.org/11027070/diff/94001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/11027070/diff/94001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:157: // TODO(jhawkins): Remove once
IgnoreResult is fixed.
Remove this TODO, the signaling no requires a proper function.

[zel, 2012-11-06 23:25:53]

http://codereview.chromium.org/11027070/diff/94001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (left):

http://codereview.chromium.org/11027070/diff/94001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_utils_browsertest.cc:223: RunAllPending();
On 2012/11/06 08:53:08, Nikita Kostylev wrote:
> This is just to use proper method, right?
> Since RunAllPending() already wall calling RunUntilIdle()

Done.

http://codereview.chromium.org/11027070/diff/94001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (right):

http://codereview.chromium.org/11027070/diff/94001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_utils_browsertest.cc:575: 
On 2012/11/06 09:07:54, Mattias Nissler wrote:
> I think you need a RunUntilIdle here as well? Or are the flakes gone for good
> for a different reason?

That was my first reaction, there were other reasons that made me use the same
SequencedTaskRunner instance within ManagedModePolicyProvider. That took care
for all enterprise flakes.

http://codereview.chromium.org/11027070/diff/94001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/11027070/diff/94001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:157: // TODO(jhawkins): Remove once
IgnoreResult is fixed.
On 2012/11/06 09:07:54, Mattias Nissler wrote:
> Remove this TODO, the signaling no requires a proper function.

Done.

[Mattias Nissler (ping if slow), 2012-11-07 07:52:31]

LGTM again :)

http://codereview.chromium.org/11027070/diff/94001/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (right):

http://codereview.chromium.org/11027070/diff/94001/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/login_utils_browsertest.cc:575: 
On 2012/11/06 23:25:54, zel wrote:
> On 2012/11/06 09:07:54, Mattias Nissler wrote:
> > I think you need a RunUntilIdle here as well? Or are the flakes gone for
good
> > for a different reason?
> 
> That was my first reaction, there were other reasons that made me use the same
> SequencedTaskRunner instance within ManagedModePolicyProvider. That took care
> for all enterprise flakes. 

Ah, I see.

[Nico, 2014-01-23 21:14:41]

https://codereview.chromium.org/11027070/diff/100004/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/11027070/diff/100004/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:174: base::WaitableEvent* done_creating
= new base::WaitableEvent(false, false);
Who owns this? Is this ever deleted anywhere?

[zel, 2014-01-23 21:18:55]

https://codereview.chromium.org/11027070/diff/100004/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/11027070/diff/100004/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:174: base::WaitableEvent* done_creating
= new base::WaitableEvent(false, false);
On 2014/01/23 21:14:44, Nico wrote:
> Who owns this? Is this ever deleted anywhere?

see line 184 below

[Nico, 2014-01-23 21:24:40]

https://codereview.chromium.org/11027070/diff/100004/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/11027070/diff/100004/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:174: base::WaitableEvent* done_creating
= new base::WaitableEvent(false, false);
On 2014/01/23 21:18:57, zel wrote:
> On 2014/01/23 21:14:44, Nico wrote:
> > Who owns this? Is this ever deleted anywhere?
> 
> see line 184 below

D'oh, thanks!

(Maybe scoped_ptr and base::Passed would've made this slightly clearer, but it's
already pretty clear. Sorry for the noise!)