Make ServerBoundCertStore interface async, move SQLiteServerBoundCertStore load onto DB thread.

Make ServerBoundCertStore interface async, move SQLiteServerBoundCertStore load onto DB thread.

Fix chromeos::ProfileAuthData::Transfer to only transfer server bound certs when cookies are being transferred.

BUG=89665, 166919
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=178742

[mattm, 2013-01-04 03:45:32]

markusheintz: chrome/browser/browsing_data/*
erikwright: *server_bound_cert_store*
wtc: net/base/server_bound_cert_service*

[markusheintz_, 2013-01-04 18:03:39]

browsing data changes LGTM with 2 nits

https://codereview.chromium.org/11742037/diff/2001/chrome/browser/browsing_da...
File chrome/browser/browsing_data/browsing_data_remover.h (right):

https://codereview.chromium.org/11742037/diff/2001/chrome/browser/browsing_da...
chrome/browser/browsing_data/browsing_data_remover.h:324: // Callback on IO
Thread when server bound certs have been deleted.  Clears
nit: Pls delete one space before "Clears".

https://codereview.chromium.org/11742037/diff/2001/chrome/browser/browsing_da...
File chrome/browser/browsing_data/browsing_data_server_bound_cert_helper.cc
(right):

https://codereview.chromium.org/11742037/diff/2001/chrome/browser/browsing_da...
chrome/browser/browsing_data/browsing_data_server_bound_cert_helper.cc:39: const
net::ServerBoundCertStore::ServerBoundCertList& cert_list);
nit: bad indentation

[markusheintz_, 2013-01-04 18:03:40]

browsing data changes LGTM with 2 nits

https://codereview.chromium.org/11742037/diff/2001/chrome/browser/browsing_da...
File chrome/browser/browsing_data/browsing_data_remover.h (right):

https://codereview.chromium.org/11742037/diff/2001/chrome/browser/browsing_da...
chrome/browser/browsing_data/browsing_data_remover.h:324: // Callback on IO
Thread when server bound certs have been deleted.  Clears
nit: Pls delete one space before "Clears".

https://codereview.chromium.org/11742037/diff/2001/chrome/browser/browsing_da...
File chrome/browser/browsing_data/browsing_data_server_bound_cert_helper.cc
(right):

https://codereview.chromium.org/11742037/diff/2001/chrome/browser/browsing_da...
chrome/browser/browsing_data/browsing_data_server_bound_cert_helper.cc:39: const
net::ServerBoundCertStore::ServerBoundCertList& cert_list);
nit: bad indentation

[erikwright (departed), 2013-01-04 19:20:09]

https://codereview.chromium.org/11742037/diff/2001/chrome/browser/net/sqlite_...
File chrome/browser/net/sqlite_server_bound_cert_store.cc (right):

https://codereview.chromium.org/11742037/diff/2001/chrome/browser/net/sqlite_...
chrome/browser/net/sqlite_server_bound_cert_store.cc:175: bool
SQLiteServerBoundCertStore::Backend::LoadOnDBThread(
why keep the bool exit code?

Also, consider simplifying by making the callback a parameter of LoadOnDBThread.

Then you just bind in the callback, and LoadOnDBThread, upon completion, just
re-binds the callback to the result ScopedVector and posts it back to the IO
thread.

https://codereview.chromium.org/11742037/diff/2001/net/base/default_server_bo...
File net/base/default_server_bound_cert_store.cc (right):

https://codereview.chromium.org/11742037/diff/2001/net/base/default_server_bo...
net/base/default_server_bound_cert_store.cc:120: expiration_time_, private_key_,
cert_);
indent

https://codereview.chromium.org/11742037/diff/2001/net/base/default_server_bo...
File net/base/default_server_bound_cert_store.h (right):

https://codereview.chromium.org/11742037/diff/2001/net/base/default_server_bo...
net/base/default_server_bound_cert_store.h:53: // methods will be run on the
calling thread.
This class is no longer threadsafe, right? If so the comment about calling
thread is misleading.

https://codereview.chromium.org/11742037/diff/2001/net/base/default_server_bo...
net/base/default_server_bound_cert_store.h:132: void EnqueueTask(Task* task);
"Takes ownership" should be reflected by a scoped_ptr<Task>, in which case the
comment is redundant.

https://codereview.chromium.org/11742037/diff/2001/net/base/default_server_bo...
net/base/default_server_bound_cert_store.h:135: void RunOrEnqueueTask(Task*
task);
ditto.

https://codereview.chromium.org/11742037/diff/2001/net/base/default_server_bo...
File net/base/default_server_bound_cert_store_unittest.cc (right):

https://codereview.chromium.org/11742037/diff/2001/net/base/default_server_bo...
net/base/default_server_bound_cert_store_unittest.cc:27: ASSERT_TRUE(false);
FAIL() << "Message." ?

Also, you should only use ASSERT & FAIL in a test case body (not a subroutine).
Otherwise you should use EXPECT and ADD_FAILURE.

https://codereview.chromium.org/11742037/diff/2001/net/base/server_bound_cert...
File net/base/server_bound_cert_service.cc (right):

https://codereview.chromium.org/11742037/diff/2001/net/base/server_bound_cert...
net/base/server_bound_cert_service.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
Any reason why the changes in this file are lumped in this CL?

[mattm, 2013-01-08 04:53:21]

updated.  (Still need to fix the cros compile, but I've addressed the review
comments.)

https://codereview.chromium.org/11742037/diff/2001/chrome/browser/browsing_da...
File chrome/browser/browsing_data/browsing_data_remover.h (right):

https://codereview.chromium.org/11742037/diff/2001/chrome/browser/browsing_da...
chrome/browser/browsing_data/browsing_data_remover.h:324: // Callback on IO
Thread when server bound certs have been deleted.  Clears
On 2013/01/04 18:03:40, markusheintz_ wrote:
> nit: Pls delete one space before "Clears".

Done.

https://codereview.chromium.org/11742037/diff/2001/chrome/browser/browsing_da...
File chrome/browser/browsing_data/browsing_data_server_bound_cert_helper.cc
(right):

https://codereview.chromium.org/11742037/diff/2001/chrome/browser/browsing_da...
chrome/browser/browsing_data/browsing_data_server_bound_cert_helper.cc:39: const
net::ServerBoundCertStore::ServerBoundCertList& cert_list);
On 2013/01/04 18:03:40, markusheintz_ wrote:
> nit: bad indentation

Done.

https://codereview.chromium.org/11742037/diff/2001/chrome/browser/net/sqlite_...
File chrome/browser/net/sqlite_server_bound_cert_store.cc (right):

https://codereview.chromium.org/11742037/diff/2001/chrome/browser/net/sqlite_...
chrome/browser/net/sqlite_server_bound_cert_store.cc:175: bool
SQLiteServerBoundCertStore::Backend::LoadOnDBThread(
On 2013/01/04 19:20:10, erikwright wrote:
> why keep the bool exit code?

removed.

> Also, consider simplifying by making the callback a parameter of
LoadOnDBThread.
> 
> Then you just bind in the callback, and LoadOnDBThread, upon completion, just
> re-binds the callback to the result ScopedVector and posts it back to the IO
> thread.

Well, because of all the early exits in LoadOnDBThread, I'd want to make a
wrapper method to do the actual posting instead of directly from LoadOnDBThread.
In that case I'm not sure it's really simpler, but I changed it.

https://codereview.chromium.org/11742037/diff/2001/net/base/default_server_bo...
File net/base/default_server_bound_cert_store.cc (right):

https://codereview.chromium.org/11742037/diff/2001/net/base/default_server_bo...
net/base/default_server_bound_cert_store.cc:120: expiration_time_, private_key_,
cert_);
On 2013/01/04 19:20:10, erikwright wrote:
> indent

Done.

https://codereview.chromium.org/11742037/diff/2001/net/base/default_server_bo...
File net/base/default_server_bound_cert_store.h (right):

https://codereview.chromium.org/11742037/diff/2001/net/base/default_server_bo...
net/base/default_server_bound_cert_store.h:53: // methods will be run on the
calling thread.
On 2013/01/04 19:20:10, erikwright wrote:
> This class is no longer threadsafe, right? If so the comment about calling
> thread is misleading.

Well, I wanted to make the distinction from FlushStore's callback behavior
clear.  But I'm okay with removing it too.

https://codereview.chromium.org/11742037/diff/2001/net/base/default_server_bo...
net/base/default_server_bound_cert_store.h:132: void EnqueueTask(Task* task);
On 2013/01/04 19:20:10, erikwright wrote:
> "Takes ownership" should be reflected by a scoped_ptr<Task>, in which case the
> comment is redundant.

Done.

https://codereview.chromium.org/11742037/diff/2001/net/base/default_server_bo...
net/base/default_server_bound_cert_store.h:135: void RunOrEnqueueTask(Task*
task);
On 2013/01/04 19:20:10, erikwright wrote:
> ditto.

Done.

https://codereview.chromium.org/11742037/diff/2001/net/base/default_server_bo...
File net/base/default_server_bound_cert_store_unittest.cc (right):

https://codereview.chromium.org/11742037/diff/2001/net/base/default_server_bo...
net/base/default_server_bound_cert_store_unittest.cc:27: ASSERT_TRUE(false);
On 2013/01/04 19:20:10, erikwright wrote:
> FAIL() << "Message." ?
> 
> Also, you should only use ASSERT & FAIL in a test case body (not a
subroutine).
> Otherwise you should use EXPECT and ADD_FAILURE.

Done.

https://codereview.chromium.org/11742037/diff/2001/net/base/server_bound_cert...
File net/base/server_bound_cert_service.cc (right):

https://codereview.chromium.org/11742037/diff/2001/net/base/server_bound_cert...
net/base/server_bound_cert_service.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2013/01/04 19:20:10, erikwright wrote:
> Any reason why the changes in this file are lumped in this CL?

The interface to the ServerBoundCertStore is changed, so this file needs to be
updated to the new way of using it.

[mattm, 2013-01-09 01:33:56]

+zel for chrome/browser/chromeos/login/
I've only verified it compiles and LoginUtils tests pass, but that doesn't test
it fully.

[zel, 2013-01-09 18:44:46]

lgtm

[erikwright (departed), 2013-01-10 22:03:30]

LGTM. I haven't had a chance to look at server_bound_cert_service yet, and I
have to leave now to look after sick kiddos (I anticipate being out tomorrow).
If someone else has reviewed that, SGTM.

Sorry.

https://codereview.chromium.org/11742037/diff/2001/chrome/browser/net/sqlite_...
File chrome/browser/net/sqlite_server_bound_cert_store.cc (right):

https://codereview.chromium.org/11742037/diff/2001/chrome/browser/net/sqlite_...
chrome/browser/net/sqlite_server_bound_cert_store.cc:175: bool
SQLiteServerBoundCertStore::Backend::LoadOnDBThread(
On 2013/01/08 04:53:21, mattm wrote:
> On 2013/01/04 19:20:10, erikwright wrote:
> > why keep the bool exit code?
> 
> removed.
> 
> > Also, consider simplifying by making the callback a parameter of
> LoadOnDBThread.
> > 
> > Then you just bind in the callback, and LoadOnDBThread, upon completion,
just
> > re-binds the callback to the result ScopedVector and posts it back to the IO
> > thread.
> 
> Well, because of all the early exits in LoadOnDBThread, I'd want to make a
> wrapper method to do the actual posting instead of directly from
LoadOnDBThread.
> In that case I'm not sure it's really simpler, but I changed it.

I see your point. I don't mind either way.

https://codereview.chromium.org/11742037/diff/2001/net/base/default_server_bo...
File net/base/default_server_bound_cert_store.h (right):

https://codereview.chromium.org/11742037/diff/2001/net/base/default_server_bo...
net/base/default_server_bound_cert_store.h:53: // methods will be run on the
calling thread.
On 2013/01/08 04:53:21, mattm wrote:
> On 2013/01/04 19:20:10, erikwright wrote:
> > This class is no longer threadsafe, right? If so the comment about calling
> > thread is misleading.
> 
> Well, I wanted to make the distinction from FlushStore's callback behavior
> clear.  But I'm okay with removing it too.

I see. OK, the comment does make sense in that context. Have you considered
updating FlushStore to be consistent with the new asynchronicity of the other
methods?

[commit-bot: I haz the power, 2013-01-24 03:53:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/11742037/47001

[commit-bot: I haz the power, 2013-01-24 05:52:53]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-01-24 22:34:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/11742037/52003

[commit-bot: I haz the power, 2013-01-25 03:57:33]

Change committed as 178742