Move TransportSecurityPersister completely to IO thread.

Move TransportSecurityPersister completely to IO thread.

This should fix problems caused by calling ImportantFileWriter
on wrong threads.

BUG=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=102139

[willchan no longer on Chromium, 2011-09-20 01:29:50]

http://codereview.chromium.org/7966005/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

http://codereview.chromium.org/7966005/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_impl_io_data.cc:191: NewRunnableFunction(
This is going to be deprecated soon (stay tuned for an email next week). Please
use base::Bind() instead.

http://codereview.chromium.org/7966005/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/7966005/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_io_data.cc:439:
transport_security_persister_->Init();
Is this Init() necessary now? Can't it just be done in the constructor?

http://codereview.chromium.org/7966005/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/7966005/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_io_data.h:280: mutable
scoped_refptr<net::TransportSecurityState> transport_security_state_;
I think it doesn't have to be refcounted anymore, does it?

http://codereview.chromium.org/7966005/diff/1/chrome/browser/transport_securi...
File chrome/browser/transport_security_persister.cc (right):

http://codereview.chromium.org/7966005/diff/1/chrome/browser/transport_securi...
chrome/browser/transport_security_persister.cc:39: BrowserThread::PostTask(
Can you use BrowserThread::PostTaskAndReply() and pass a base::WeakPtr instead
to TransportSecurityPersister::Load()? I think that will help eliminate the need
for the refcounting here.

[Paweł Hajdan Jr., 2011-09-20 18:32:49]

PTAL

I'm not sure if base::Unretained and deleter(this) is better than refcounting
Loader, but I've just done what you recommended. I can change this to
refcounting Loader if that'd be better.

[willchan no longer on Chromium, 2011-09-20 19:09:04]

I agree with your point about base::Unretained(). I'm ambivalent to either
solution, they both suck. I think we should support a PassOwnPtr<T> or
shared_ptr<T> or both. But we don't yet, so either one of the suboptimal
solutions is fine by me.

http://codereview.chromium.org/7966005/diff/6001/chrome/browser/profiles/off_...
File chrome/browser/profiles/off_the_record_profile_impl.h (right):

http://codereview.chromium.org/7966005/diff/6001/chrome/browser/profiles/off_...
chrome/browser/profiles/off_the_record_profile_impl.h:134: const base::Time&
time) OVERRIDE;
btw, Time is an int64 so passing by reference doesn't help. I don't really care
but some people do. Thought I'd point it out in case you didn't know.

http://codereview.chromium.org/7966005/diff/6001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/7966005/diff/6001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_io_data.cc:433: transport_security_state_ = new
net::TransportSecurityState(
I think the only reason TransportSecurityState was refcounted before was because
it was created on the UI thread and needed refcounting for the FILE thread too.
Now that those have been solved, can we remove it here?

http://codereview.chromium.org/7966005/diff/6001/chrome/browser/transport_sec...
File chrome/browser/transport_security_persister.h (right):

http://codereview.chromium.org/7966005/diff/6001/chrome/browser/transport_sec...
chrome/browser/transport_security_persister.h:49: public
base::SupportsWeakPtr<TransportSecurityPersister> {
Can you use base::WeakPtrFactory as a private member variable instead? I'd
rather not add to the public API unless it's necessary.

[Paweł Hajdan Jr., 2011-09-21 17:59:19]

PTAL

http://codereview.chromium.org/7966005/diff/6001/chrome/browser/profiles/off_...
File chrome/browser/profiles/off_the_record_profile_impl.h (right):

http://codereview.chromium.org/7966005/diff/6001/chrome/browser/profiles/off_...
chrome/browser/profiles/off_the_record_profile_impl.h:134: const base::Time&
time) OVERRIDE;
On 2011/09/20 19:09:04, willchan wrote:
> btw, Time is an int64 so passing by reference doesn't help. I don't really
care
> but some people do. Thought I'd point it out in case you didn't know.

Done. I didn't know.

http://codereview.chromium.org/7966005/diff/6001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/7966005/diff/6001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_io_data.cc:433: transport_security_state_ = new
net::TransportSecurityState(
On 2011/09/20 19:09:04, willchan wrote:
> I think the only reason TransportSecurityState was refcounted before was
because
> it was created on the UI thread and needed refcounting for the FILE thread
too.
> Now that those have been solved, can we remove it here?

I'd prefer to do that in a separate CL. I'm not sure if its ownership is clear
now that refcounting has been used for a while. Switching this to "full"
ownership might uncover some memory issues. I've added a TODO for now, does that
sound good?

http://codereview.chromium.org/7966005/diff/6001/chrome/browser/transport_sec...
File chrome/browser/transport_security_persister.h (right):

http://codereview.chromium.org/7966005/diff/6001/chrome/browser/transport_sec...
chrome/browser/transport_security_persister.h:49: public
base::SupportsWeakPtr<TransportSecurityPersister> {
On 2011/09/20 19:09:04, willchan wrote:
> Can you use base::WeakPtrFactory as a private member variable instead? I'd
> rather not add to the public API unless it's necessary.

Good idea, done.

[willchan no longer on Chromium, 2011-09-21 18:07:48]

LGTM

http://codereview.chromium.org/7966005/diff/6001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/7966005/diff/6001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_io_data.cc:433: transport_security_state_ = new
net::TransportSecurityState(
On 2011/09/21 17:59:19, Paweł Hajdan Jr. wrote:
> On 2011/09/20 19:09:04, willchan wrote:
> > I think the only reason TransportSecurityState was refcounted before was
> because
> > it was created on the UI thread and needed refcounting for the FILE thread
> too.
> > Now that those have been solved, can we remove it here?
> 
> I'd prefer to do that in a separate CL. I'm not sure if its ownership is clear
> now that refcounting has been used for a while. Switching this to "full"
> ownership might uncover some memory issues. I've added a TODO for now, does
that
> sound good?

OK!

[agl, 2011-09-21 18:40:43]

willchan knows about lifetime stuff far better than I so I defer to him. LGTM