Moves SQLitePersistentCookieStore to net/extras/sqlite.

Moves SQLitePersistentCookieStore to net/extras/sqlite.

The application of special storage policy is split out into a new class,
QuotaPolicyCookieStore, in content/browser/net.

BUG=467596
TEST=No visible impact.

Committed: https://crrev.com/57adf29fb4e46a49272610140b097f85e1e805f5
Cr-Commit-Position: refs/heads/master@{#329950}

[rohitrao (ping after 24h), 2015-03-17 23:05:39]

rohitrao@chromium.org changed reviewers:
+ rsleevi@chromium.org

[rohitrao (ping after 24h), 2015-03-17 23:05:40]

Not ready for a real review, but the basic structure is in place now.

I haven't yet figured out how to delete cookies on destruction.  The plumbing is
in place, but I need to generate and provide a list of cookies to delete.  In
the example CL, https://codereview.chromium.org/381073002/, we keep a cache of
channel IDs in the chrome-level class and use that.  I'm not yet sure if that's
possible to do here as well.

(The "easy" solution would be to expose cookies_per_origin_ to the subclass, but
that probably sucks =) )

[rohitrao (ping after 24h), 2015-03-18 14:37:34]

Question inline about blocking tasks during shutdown.

https://codereview.chromium.org/1016643004/diff/60001/content/browser/net/quo...
File content/browser/net/quota_policy_cookie_store_unittest.cc (right):

https://codereview.chromium.org/1016643004/diff/60001/content/browser/net/quo...
content/browser/net/quota_policy_cookie_store_unittest.cc:81:
pool_owner_->pool()->Shutdown(3);
@rsleevi For some reason, the indirection through this new
QuotaPolicyCookieStore class is causing blocking tasks to be posted during
shutdown.  Without allowing new blocking tasks, the test DCHECKs.  I think I am
seeing similar errors in the full app, where the Close() task is getting dropped
on the floor during shutdown.

I'm trying to debug, but I don't really understand why this is happening.  Did
you see something similar when converting the channel id class?

[rohitrao (ping after 24h), 2015-03-18 20:33:09]

I think what's happening is that, at the point where I call DestroyStore(), some
task is still running and holding a reference to the store.  This task
eventually completes and releases store_ (on a background thread, it turns out),
but that happens *after* we continue on to call Shutdown() on the pool.

I can work around this by calling FlushForTesting() on the pool before
Shutdown(), which seems ok for the test, but I'm worried that this will happen
in the real app as well.  I'm seeing related brokenness where tests aren't
passing and it's somehow related to the indirection through the new class. 
Still investigating that.

[rohitrao (ping after 24h), 2015-03-18 23:56:39]

Ok, trybots seem pretty happy now.  I believe this is ready for a real review.

I follow the same pattern as in the channel id change, so things should look
pretty similar to that.

Presubmit wants me to run git-cl format over the net/ code, but that's
introducing a ton of changes, so I'm holding off on that until after the bulk of
the review is completed.

[Ryan Sleevi, 2015-03-19 03:37:51]

An initial first pass

https://codereview.chromium.org/1016643004/diff/140001/content/browser/net/qu...
File content/browser/net/quota_policy_cookie_store.cc (right):

https://codereview.chromium.org/1016643004/diff/140001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:56: it !=
cookies_per_origin_.end(); ++it) {
Doesn't this vend a mutable |it| due to auto binding to the non-const invocation
of .begin() (since C++14 is cbegin)? ;(

why not use range-based and let it do the right thing?

for (const auto& cookie : cookies_per_orgin_) {

}

https://codereview.chromium.org/1016643004/diff/140001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:115: for (auto it =
cookies.begin(); it != cookies.end(); ++it) {
ditto const auto& + range

https://codereview.chromium.org/1016643004/diff/140001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:166:
BrowserThread::GetBlockingPool()->GetSequenceToken());
Is it right that every cookie store should be given their own SequenceToken?

Would it make more sense to use a common token for all the cookie stores? It
would still guarantee the same FIFOness, it just wouldn't burn a token. 

The only reason to use a SequenceToken is to allow multiple participants to get
the same STR from the blocking pool if needed, but you seem entirely unreliant
on needing that, so just using a single named pool is all the same.

https://codereview.chromium.org/1016643004/diff/140001/net/extras/sqlite/DEPS
File net/extras/sqlite/DEPS (right):

https://codereview.chromium.org/1016643004/diff/140001/net/extras/sqlite/DEPS...
net/extras/sqlite/DEPS:5: "+third_party/sqlite",
Is this correct? Why?

https://codereview.chromium.org/1016643004/diff/140001/net/extras/sqlite/sqli...
File net/extras/sqlite/sqlite_persistent_cookie_store.cc (right):

https://codereview.chromium.org/1016643004/diff/140001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store.cc:127: void
DeleteAllInList(const std::list<CookieOrigin> cookies);
const-ref

https://codereview.chromium.org/1016643004/diff/140001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store.cc:217: void
BackgroundDeleteAllInList(const std::list<CookieOrigin> cookies);
const-ref

https://codereview.chromium.org/1016643004/diff/140001/net/extras/sqlite/sqli...
File net/extras/sqlite/sqlite_persistent_cookie_store.h (right):

https://codereview.chromium.org/1016643004/diff/140001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store.h:47: void
DeleteAllInList(std::list<CookieOrigin> cookies);
Pass by const-ref

https://codereview.chromium.org/1016643004/diff/140001/net/extras/sqlite/sqli...
File net/extras/sqlite/sqlite_persistent_cookie_store_perftest.cc (right):

https://codereview.chromium.org/1016643004/diff/140001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store_perftest.cc:10: #include
"base/message_loop/message_loop.h"
Use a TaskRunner instead

[Ryan Sleevi, 2015-03-19 03:38:38]

rsleevi@chromium.org changed reviewers:
+ mef@chromium.org

[Ryan Sleevi, 2015-03-19 03:38:39]

Oh, and adding mef for shadowing the review / so he can own this code too :D

[rohitrao (ping after 24h), 2015-03-19 14:55:42]

Note that this CL also contains the same forced-Commit() call as the ChannelID
CL I sent out yesterday.  In this case, the existing code was already forcing a
commit in InternalBackgroundClose(), so the overall impact of the additional
Commit() should be minimal.  I'll work on optimizing/fixing this in both
databases.

https://codereview.chromium.org/1016643004/diff/140001/content/browser/net/qu...
File content/browser/net/quota_policy_cookie_store.cc (right):

https://codereview.chromium.org/1016643004/diff/140001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:56: it !=
cookies_per_origin_.end(); ++it) {
On 2015/03/19 03:37:51, Ryan Sleevi wrote:
> Doesn't this vend a mutable |it| due to auto binding to the non-const
invocation
> of .begin() (since C++14 is cbegin)? ;(
> 
> why not use range-based and let it do the right thing?
> 
> for (const auto& cookie : cookies_per_orgin_) {
> 
> }

Did not know that was possible, but I like it way better =)  Done.

https://codereview.chromium.org/1016643004/diff/140001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:115: for (auto it =
cookies.begin(); it != cookies.end(); ++it) {
On 2015/03/19 03:37:51, Ryan Sleevi wrote:
> ditto const auto& + range

Done.

https://codereview.chromium.org/1016643004/diff/140001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:166:
BrowserThread::GetBlockingPool()->GetSequenceToken());
On 2015/03/19 03:37:51, Ryan Sleevi wrote:
> Is it right that every cookie store should be given their own SequenceToken?
> 
> Would it make more sense to use a common token for all the cookie stores? It
> would still guarantee the same FIFOness, it just wouldn't burn a token. 
> 
> The only reason to use a SequenceToken is to allow multiple participants to
get
> the same STR from the blocking pool if needed, but you seem entirely unreliant
> on needing that, so just using a single named pool is all the same.

This entire block was copied verbatim from the old content/ class.  I am not
sure what the intent was, but I can figure out who wrote it and file a bug.

It sounds like the current code isn't incorrect, just wasteful of tokens?

https://codereview.chromium.org/1016643004/diff/140001/net/extras/sqlite/DEPS
File net/extras/sqlite/DEPS (right):

https://codereview.chromium.org/1016643004/diff/140001/net/extras/sqlite/DEPS...
net/extras/sqlite/DEPS:5: "+third_party/sqlite",
On 2015/03/19 03:37:51, Ryan Sleevi wrote:
> Is this correct? Why?

Looks like the header in sqlite_persistent_cookie_store.cc was unnecessary. 
Removed here and there.

https://codereview.chromium.org/1016643004/diff/140001/net/extras/sqlite/sqli...
File net/extras/sqlite/sqlite_persistent_cookie_store.cc (right):

https://codereview.chromium.org/1016643004/diff/140001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store.cc:127: void
DeleteAllInList(const std::list<CookieOrigin> cookies);
On 2015/03/19 03:37:51, Ryan Sleevi wrote:
> const-ref

Done.

https://codereview.chromium.org/1016643004/diff/140001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store.cc:217: void
BackgroundDeleteAllInList(const std::list<CookieOrigin> cookies);
On 2015/03/19 03:37:51, Ryan Sleevi wrote:
> const-ref

Done.  Just to double-check, will base::Bind() make a copy of the list when
PostTask()ing it to the background thread?

https://codereview.chromium.org/1016643004/diff/140001/net/extras/sqlite/sqli...
File net/extras/sqlite/sqlite_persistent_cookie_store.h (right):

https://codereview.chromium.org/1016643004/diff/140001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store.h:47: void
DeleteAllInList(std::list<CookieOrigin> cookies);
On 2015/03/19 03:37:51, Ryan Sleevi wrote:
> Pass by const-ref

Done.

https://codereview.chromium.org/1016643004/diff/140001/net/extras/sqlite/sqli...
File net/extras/sqlite/sqlite_persistent_cookie_store_perftest.cc (right):

https://codereview.chromium.org/1016643004/diff/140001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store_perftest.cc:10: #include
"base/message_loop/message_loop.h"
On 2015/03/19 03:37:51, Ryan Sleevi wrote:
> Use a TaskRunner instead

All of the code is using TaskRunners, but it seemed like I needed to create a
MessageLoop in the test so that MessageLoop::current() would return something
non-NULL.  Is there a way around that?

(The old code did not need to create one because ContentTestSuite sets one up
automatically.  NetTestSuite does not.)

[Ryan Sleevi, 2015-04-02 06:54:19]

Not that I'm dying for more reviews, bug ping on this and the related CL?

[rohitrao (ping after 24h), 2015-04-02 14:44:11]

Hah, I was about to ping you to take a look =)

I've addressed your first set of comments, rebased, and left a few questions of
my own.

I haven't made any progress on the related CL, but I'll get back to working on
it this week.  I've used the more-naive solution in this CL (along with a TODO
to fix), because the old code was already calling Commit(), and this change is
essentially just moving it up a bit earlier.

[mef, 2015-04-06 21:00:27]

https://codereview.chromium.org/1016643004/diff/170001/content/browser/net/qu...
File content/browser/net/quota_policy_cookie_store.cc (right):

https://codereview.chromium.org/1016643004/diff/170001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
nit: loose (c)

https://codereview.chromium.org/1016643004/diff/170001/net/extras/sqlite/sqli...
File net/extras/sqlite/sqlite_persistent_cookie_store.h (right):

https://codereview.chromium.org/1016643004/diff/170001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store.h:46: typedef
std::pair<std::string, bool> CookieOrigin;
Add comment about the bool?

https://codereview.chromium.org/1016643004/diff/170001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store.h:59: 
nit: spurious nl.

[rohitrao (ping after 24h), 2015-04-07 14:43:47]

I also had a few questions in the comments in Patchset #8.

https://codereview.chromium.org/1016643004/diff/170001/content/browser/net/qu...
File content/browser/net/quota_policy_cookie_store.cc (right):

https://codereview.chromium.org/1016643004/diff/170001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
On 2015/04/06 21:00:27, mef wrote:
> nit: loose (c)

Done.

https://codereview.chromium.org/1016643004/diff/170001/net/extras/sqlite/sqli...
File net/extras/sqlite/sqlite_persistent_cookie_store.h (right):

https://codereview.chromium.org/1016643004/diff/170001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store.h:46: typedef
std::pair<std::string, bool> CookieOrigin;
On 2015/04/06 21:00:27, mef wrote:
> Add comment about the bool?

Done.

[rohitrao (ping after 24h), 2015-04-27 14:04:36]

This is rebased and ready for another look.

erikchen has a CL out that will fix the iOS trybot failures.

[rohitrao (ping after 24h), 2015-05-04 23:14:40]

Ryan, will you have a chance to look at this CL?  Thanks!

(I'm continuing to refrain from running git cl format until the real review is
done, to make the diffs easier to read.)

[Ryan Sleevi, 2015-05-06 00:44:19]

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
File content/browser/net/quota_policy_cookie_store.cc (right):

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:41: crypto_delegate)) {
DESIGN: Does it make more sense to have the caller pass in the
SQLitePersistentCookieStore?

I don't see us use |path|, |client_task_runner|, |background_task_runner|,
|restore_old_session_cookies|, or |crypto_delegate| beyond this invocation.

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:43:
DCHECK(background_task_runner);
These aren't needed here, are they? They're invariants for the
SQLitePersistentCookieStore to ensure, since we don't seem to do anything with
them.

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:58: }
Wat?

It seems like "<0" is an impossible condition, given this DCHECK. As a result,
making this size_t and just checking == 0 seems eminently reasonable. You could
then get rid of the DCHECK on line 56.

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:72:
base::Bind(&QuotaPolicyCookieStore::OnLoad, this, loaded_callback));
DESIGN: It does strike me that this may be potentially dangerous

assume the following

LoadedCallback something = base::Bind(&whatever);
{
  scoped_refptr<QuotaPolicyCookieStore> cookie_store;
  cookie_store->Load(something);
}

^-- will still fire something, even after |cookie_store| has gone out of scope,
due to this binding taking a reference

The existing SQLitePersistentCookieStore tries to handle this a little (c.f "if
(!db_)" and handling Close()), but this doesn't, and I'm curious what the
implications may or may not be.

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:86: cc.IsSecure())]++;
This is valid, but pretty unreadable.

net::SQLitePersistentCookieStore::CookieOrigin origin(cc.Domain(),
cc.IsSecure());
++cookies_per_origin_[origin];
persistent_store_->AddCookie(cc);

?

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:98: cc.IsSecure())]--;
ditto here

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:103: special_storage_policy_ =
NULL;
nullptr

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:122:
CookieStoreConfig::CookieStoreConfig()
It seems everything from here & below isn't defined in the header. Shouldn't
this all be nuked/nukable? What am I missing?

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:124: crypto_delegate(NULL) {
nullptr

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:137: crypto_delegate(NULL) {
nullptr

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:149: net::CookieMonster*
cookie_monster = NULL;
nullptr

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:177:
CookieStoreConfig::RESTORED_SESSION_COOKIES),
Is this all formatted with git cl format?

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
File content/browser/net/quota_policy_cookie_store.h (right):

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.h:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
no (c)

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.h:71: const
std::vector<net::CanonicalCookie*>& cookies);
Document?

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.h:75: typedef
std::map<net::SQLitePersistentCookieStore::CookieOrigin, int>
int? not size_t?

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
File content/browser/net/quota_policy_cookie_store_unittest.cc (right):

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store_unittest.cc:73: true, NULL, NULL);
nullptr, nullptr

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store_unittest.cc:92: store_ = NULL;
nullptr

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store_unittest.cc:94: // resetting the
owner (whose destructor blocks on the pool completion).
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/NH-S6KCkr2M

I tried rewording, and even I can't figure out quite what it's trying to say

// Ensure that |store_|'s destructor has run by shutting down the pool and
// then forcing the pool to be destructed. This will ensure that all the
// tasks that block pool shutdown (e.g. |store_|'s cleanup) have run
// before yielding control.

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store_unittest.cc:109: base::MessageLoop
main_loop_;
Being in //content, should this be using a TestBrowserThreadBundle? Especially
since the code in question calls into the content:: browser thread code?

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store_unittest.cc:133: true, NULL,
NULL);
nullptr blah blah

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store_unittest.cc:208: }
for (const auto& cookie : cookies) {
  EXPECT_NE("nonpersistent.com", cookie->Domain());
}

?

https://codereview.chromium.org/1016643004/diff/350001/net/extras/sqlite/sqli...
File net/extras/sqlite/sqlite_persistent_cookie_store.cc (right):

https://codereview.chromium.org/1016643004/diff/350001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store.cc:1259: // list of pending
operations.
Bug to track this follow-up work so I can incessantly nag you if you don't? :D

https://codereview.chromium.org/1016643004/diff/350001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store.cc:1275: for (auto it =
cookies.begin(); it != cookies.end(); ++it) {
for (const auto& cookie : cookies) {

}

https://codereview.chromium.org/1016643004/diff/350001/net/extras/sqlite/sqli...
File net/extras/sqlite/sqlite_persistent_cookie_store.h (right):

https://codereview.chromium.org/1016643004/diff/350001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store.h:47: typedef
std::pair<std::string, bool> CookieOrigin;
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...

https://codereview.chromium.org/1016643004/diff/350001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store.h:49: // Deletes all of the
cookies in |cookies|.
This comment needs rewording, since an "std::list<CookieOrigin>&" is not "all of
the cookies"

https://codereview.chromium.org/1016643004/diff/350001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store.h:50: void
DeleteAllInList(const std::list<CookieOrigin>& cookies);
DESIGN: It's not clear why this uses a std::list. Any reason?

In particular, it seems like duplicate CookieOrigins can get inserted here.
Would this make more sense as a map of origin/bool pairs? Why or why not?

[Later follow-up] It seems like duplicates are desired, so you can delete both
secure and non-secure cookies. Still, it may result in duplication. I'm
wondering whether map<origin, enum> makes more sense?

https://codereview.chromium.org/1016643004/diff/350001/net/extras/sqlite/sqli...
File net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc (right):

https://codereview.chromium.org/1016643004/diff/350001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc:362: false, NULL);
Since you have to change all these, does it make sense to update to nullptr as
well?

https://codereview.chromium.org/1016643004/diff/350001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc:640:
CanonicalCookie* cookie_other = NULL;
nullptr

[Ryan Sleevi, 2015-05-06 00:48:43]

rsleevi@chromium.org changed reviewers:
+ nharper@chromium.org

[Ryan Sleevi, 2015-05-06 00:48:44]

Also adding nharper@ for shadow reviewing and to spam in inbox ;)

[rohitrao (ping after 24h), 2015-05-11 19:25:31]

Most comments addressed.

I had a followup question about the "potentially dangerous" code in
QuotaPolicyCookieStore::Load(), because I'm not sure if this should be handled
by the caller or by the DB.

I also had a question about using std::list, and whether you think a std::set
would be better.

Thanks!

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
File content/browser/net/quota_policy_cookie_store.cc (right):

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:41: crypto_delegate)) {
On 2015/05/06 00:44:18, Ryan Sleevi wrote:
> DESIGN: Does it make more sense to have the caller pass in the
> SQLitePersistentCookieStore?
> 
> I don't see us use |path|, |client_task_runner|, |background_task_runner|,
> |restore_old_session_cookies|, or |crypto_delegate| beyond this invocation.

Makes sense, changed the API.

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:43:
DCHECK(background_task_runner);
On 2015/05/06 00:44:18, Ryan Sleevi wrote:
> These aren't needed here, are they? They're invariants for the
> SQLitePersistentCookieStore to ensure, since we don't seem to do anything with
> them.

Obsolete now.

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:58: }
On 2015/05/06 00:44:17, Ryan Sleevi wrote:
> Wat?
> 
> It seems like "<0" is an impossible condition, given this DCHECK. As a result,
> making this size_t and just checking == 0 seems eminently reasonable. You
could
> then get rid of the DCHECK on line 56.

Done.  I added a new DCHECK to DeleteCookie() to catch cases where we might try
to decrement below 0.

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:72:
base::Bind(&QuotaPolicyCookieStore::OnLoad, this, loaded_callback));
On 2015/05/06 00:44:18, Ryan Sleevi wrote:
> DESIGN: It does strike me that this may be potentially dangerous
> 
> assume the following
> 
> LoadedCallback something = base::Bind(&whatever);
> {
>   scoped_refptr<QuotaPolicyCookieStore> cookie_store;
>   cookie_store->Load(something);
> }
> 
> ^-- will still fire something, even after |cookie_store| has gone out of
scope,
> due to this binding taking a reference
> 
> The existing SQLitePersistentCookieStore tries to handle this a little (c.f
"if
> (!db_)" and handling Close()), but this doesn't, and I'm curious what the
> implications may or may not be.

Isn't the caller making lifetime assumptions that it shouldn't?  If you have a
scoped_refptr, having it go out of scope is meaningless.  If the calling object
is being destroyed and doesn't want the callback to be called, shouldn't it
explicitly need to use a WeakPtr or something?

I might be misunderstanding the problem, but this seems like it's a general case
of reference counting being an awful system.

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:86: cc.IsSecure())]++;
On 2015/05/06 00:44:17, Ryan Sleevi wrote:
> This is valid, but pretty unreadable.
> 
> net::SQLitePersistentCookieStore::CookieOrigin origin(cc.Domain(),
> cc.IsSecure());
> ++cookies_per_origin_[origin];
> persistent_store_->AddCookie(cc);
> 
> ?

Done.

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:98: cc.IsSecure())]--;
On 2015/05/06 00:44:18, Ryan Sleevi wrote:
> ditto here

Done.

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:103: special_storage_policy_ =
NULL;
On 2015/05/06 00:44:18, Ryan Sleevi wrote:
> nullptr

Done.

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:122:
CookieStoreConfig::CookieStoreConfig()
On 2015/05/06 00:44:18, Ryan Sleevi wrote:
> It seems everything from here & below isn't defined in the header. Shouldn't
> this all be nuked/nukable? What am I missing?

This is copied straight from the old
content/net/browser/sqlite_persistent_cookie_store.cc file.  I think no one ever
bothered to split it out into a separate file?  I can do that in a followup CL
and see what content owners think.

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:124: crypto_delegate(NULL) {
On 2015/05/06 00:44:18, Ryan Sleevi wrote:
> nullptr

Done.

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:137: crypto_delegate(NULL) {
On 2015/05/06 00:44:17, Ryan Sleevi wrote:
> nullptr

Done.

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:149: net::CookieMonster*
cookie_monster = NULL;
On 2015/05/06 00:44:17, Ryan Sleevi wrote:
> nullptr

Done.

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:177:
CookieStoreConfig::RESTORED_SESSION_COOKIES),
On 2015/05/06 00:44:17, Ryan Sleevi wrote:
> Is this all formatted with git cl format?

Not yet.  I figured we would just take the git cl format changes wholesale,
because they are mandatory for net/, and I didn't want to have random diffs
confusing the review.  There will be a bunch of formatting diffs, because this
code is being newly imported into net/.

(Actually, this specific file won't get git cl formatted at all, because it
still lives in content/.)

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
File content/browser/net/quota_policy_cookie_store.h (right):

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.h:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
On 2015/05/06 00:44:18, Ryan Sleevi wrote:
> no (c)

Done.

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.h:71: const
std::vector<net::CanonicalCookie*>& cookies);
On 2015/05/06 00:44:18, Ryan Sleevi wrote:
> Document?

Done.

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.h:75: typedef
std::map<net::SQLitePersistentCookieStore::CookieOrigin, int>
On 2015/05/06 00:44:18, Ryan Sleevi wrote:
> int? not size_t?

Done.

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
File content/browser/net/quota_policy_cookie_store_unittest.cc (right):

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store_unittest.cc:73: true, NULL, NULL);
On 2015/05/06 00:44:18, Ryan Sleevi wrote:
> nullptr, nullptr

Done.

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store_unittest.cc:92: store_ = NULL;
On 2015/05/06 00:44:18, Ryan Sleevi wrote:
> nullptr

Done.

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store_unittest.cc:94: // resetting the
owner (whose destructor blocks on the pool completion).
On 2015/05/06 00:44:18, Ryan Sleevi wrote:
> I tried rewording, and even I can't figure out quite what it's trying to say

Your rewording seems correct, inasmuch as I don't know what the original comment
was trying to say either =)

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store_unittest.cc:109: base::MessageLoop
main_loop_;
On 2015/05/06 00:44:18, Ryan Sleevi wrote:
> Being in //content, should this be using a TestBrowserThreadBundle? Especially
> since the code in question calls into the content:: browser thread code?

Makes sense, done.

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store_unittest.cc:133: true, NULL,
NULL);
On 2015/05/06 00:44:18, Ryan Sleevi wrote:
> nullptr blah blah

Fixed everywhere.

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store_unittest.cc:208: }
On 2015/05/06 00:44:18, Ryan Sleevi wrote:
> for (const auto& cookie : cookies) {
>   EXPECT_NE("nonpersistent.com", cookie->Domain());
> }
> 
> ?

Done.

https://codereview.chromium.org/1016643004/diff/350001/net/extras/sqlite/sqli...
File net/extras/sqlite/sqlite_persistent_cookie_store.cc (right):

https://codereview.chromium.org/1016643004/diff/350001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store.cc:1259: // list of pending
operations.
On 2015/05/06 00:44:18, Ryan Sleevi wrote:
> Bug to track this follow-up work so I can incessantly nag you if you don't? :D

Drat, you've found my secret plan to never fix it.

https://codereview.chromium.org/1016643004/diff/350001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store.cc:1275: for (auto it =
cookies.begin(); it != cookies.end(); ++it) {
On 2015/05/06 00:44:18, Ryan Sleevi wrote:
> for (const auto& cookie : cookies) {
> 
> }

Done.

https://codereview.chromium.org/1016643004/diff/350001/net/extras/sqlite/sqli...
File net/extras/sqlite/sqlite_persistent_cookie_store.h (right):

https://codereview.chromium.org/1016643004/diff/350001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store.h:47: typedef
std::pair<std::string, bool> CookieOrigin;
On 2015/05/06 00:44:19, Ryan Sleevi wrote:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...

Done.

https://codereview.chromium.org/1016643004/diff/350001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store.h:49: // Deletes all of the
cookies in |cookies|.
On 2015/05/06 00:44:19, Ryan Sleevi wrote:
> This comment needs rewording, since an "std::list<CookieOrigin>&" is not "all
of
> the cookies"

Done.

https://codereview.chromium.org/1016643004/diff/350001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store.h:50: void
DeleteAllInList(const std::list<CookieOrigin>& cookies);
On 2015/05/06 00:44:18, Ryan Sleevi wrote:
> DESIGN: It's not clear why this uses a std::list. Any reason?
> 
> In particular, it seems like duplicate CookieOrigins can get inserted here.
> Would this make more sense as a map of origin/bool pairs? Why or why not?
> 
> [Later follow-up] It seems like duplicates are desired, so you can delete both
> secure and non-secure cookies. Still, it may result in duplication. I'm
> wondering whether map<origin, enum> makes more sense?

I picked std::list because that's what QuotaPolicyChannelIDStore uses. 
Duplication doesn't bother me all that much, because there's no ambiguity about
what to do when duplicates are present.  I would be fine with using std::set
instead (hash_set or unordered_set?), to remove the possibility of duplicates. 
I don't think I like map<origin,enum>.

https://codereview.chromium.org/1016643004/diff/350001/net/extras/sqlite/sqli...
File net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc (right):

https://codereview.chromium.org/1016643004/diff/350001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc:362: false, NULL);
On 2015/05/06 00:44:19, Ryan Sleevi wrote:
> Since you have to change all these, does it make sense to update to nullptr as
> well?

Might as well, changed everywhere in this file.

https://codereview.chromium.org/1016643004/diff/350001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store_unittest.cc:640:
CanonicalCookie* cookie_other = NULL;
On 2015/05/06 00:44:19, Ryan Sleevi wrote:
> nullptr

Done.

[Ryan Sleevi, 2015-05-12 00:48:34]

LGTM % many many nits :)

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
File content/browser/net/quota_policy_cookie_store.cc (right):

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:72:
base::Bind(&QuotaPolicyCookieStore::OnLoad, this, loaded_callback));
On 2015/05/11 19:25:29, rohitrao wrote:
> On 2015/05/06 00:44:18, Ryan Sleevi wrote:
> Isn't the caller making lifetime assumptions that it shouldn't?  If you have a
> scoped_refptr, having it go out of scope is meaningless.  If the calling
object
> is being destroyed and doesn't want the callback to be called, shouldn't it
> explicitly need to use a WeakPtr or something?

What lifetime assumptions do you see it making?

It's an extremely common pattern in //net to allow an object to be deleted
before-or-during a callback. Indeed, all of our socket APIs rely on this - e.g.
it's valid to delete a StreamSocket while it's invoking its Read() callback, and
it's valid to delete a StreamSocket while you have an outstanding Read() that
hasn't invoked the callback.

The problem of design here is one that this class shares different semantics
than its base class, in that the base class goes out of its way to avoid
invoking callbacks after a Close(), but it seems we don't have that guarantee
here.

> 
> I might be misunderstanding the problem, but this seems like it's a general
case
> of reference counting being an awful system.

It's wholly independent of reference counting, and more a statement about what
callers can/should expect if they release the object.

https://codereview.chromium.org/1016643004/diff/350001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:122:
CookieStoreConfig::CookieStoreConfig()
On 2015/05/11 19:25:30, rohitrao wrote:
> On 2015/05/06 00:44:18, Ryan Sleevi wrote:
> > It seems everything from here & below isn't defined in the header. Shouldn't
> > this all be nuked/nukable? What am I missing?
> 
> This is copied straight from the old
> content/net/browser/sqlite_persistent_cookie_store.cc file.  I think no one
ever
> bothered to split it out into a separate file?  I can do that in a followup CL
> and see what content owners think.

Yeah, I think splitting it up sounds right, but happy to defer that. Definitely
a smell :)

https://codereview.chromium.org/1016643004/diff/350001/net/extras/sqlite/sqli...
File net/extras/sqlite/sqlite_persistent_cookie_store.h (right):

https://codereview.chromium.org/1016643004/diff/350001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store.h:50: void
DeleteAllInList(const std::list<CookieOrigin>& cookies);
On 2015/05/11 19:25:30, rohitrao wrote:
> I picked std::list because that's what QuotaPolicyChannelIDStore uses. 

Right, but it doesn't seem we need to use the same structure. That is, QPCIDS
(oof) ISTR needed to be able to quickly insert/delete elements, hence the list.
It may be that the list is now entirely overkill.

> Duplication doesn't bother me all that much, because there's no ambiguity
about
> what to do when duplicates are present.  I would be fine with using std::set
> instead (hash_set or unordered_set?), to remove the possibility of duplicates.

> I don't think I like map<origin,enum>.

Agreed, not a terrible fan of the map, even if it's implemented in terms of a
set ;) hash_set doesn't lend itself well (writing a custom hash function), same
w/ unordered_set

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
File content/browser/net/quota_policy_cookie_store.cc (right):

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:60:
base::Bind(&QuotaPolicyCookieStore::OnLoad, this, loaded_callback));
Per our comments about thread-safety, I think *this* class should use a WeakPtr
to itself when calling Load, at least from my read of CookieStore's API.

That is, if someone (else) holds on to a ref count for persistent_store_, but
this object goes out of scope (all callers gone), then we can still end up with
a reference being kept alive by this callback, which will then invoke the
callback.

That is, even if from the caller's perspective call copies of this store are
destroyed, it's still invoking things.

Using a WeakPtr here ensures that we don't take extra references during our
callbacks, and if the caller releases us after calling Load, then we behave
gracefully.

Same too with line 68 - indirecting through a WeakPtr

To that end, could you also add an explicit unit test for it?

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
File content/browser/net/quota_policy_cookie_store.h (right):

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.h:16: #include
"base/memory/scoped_ptr.h"
Unused

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.h:19: #include
"net/extras/sqlite/sqlite_persistent_cookie_store.h"
Strictly speaking, do you have to depend on the SQLite implementation? It seems
like we could get away with just depending on the CookieMonster if it had the
DeleteAll() method.

Just wondering out loud...

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.h:26: }
}  // namespace base

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.h:31: }
}  // namespace net

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.h:35: }
}  // namespace storage

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.h:47: // Wraps the passed-in
|cookie_store|..
nit: extra . ?

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.h:49:
scoped_refptr<net::SQLitePersistentCookieStore> cookie_store,
const-ref unless you want to force callers to .Pass(); prefer the former over
the latter.

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
File content/browser/net/quota_policy_cookie_store_unittest.cc (right):

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store_unittest.cc:59:
pool_owner_->pool()->GetNamedSequenceToken("background"));
There's no reason not to set these up during the ctor and cache them in member
variables, rather than indirecting through these two calls repeatedly.

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store_unittest.cc:110: }
No idea why this isn't simply moved to the dtor rather than teardown, but
*shrug*

https://codereview.chromium.org/1016643004/diff/390001/net/extras/sqlite/sqli...
File net/extras/sqlite/sqlite_persistent_cookie_store.cc (right):

https://codereview.chromium.org/1016643004/diff/390001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store.cc:10: #include <utility>
utility & list go away when you fix the header

https://codereview.chromium.org/1016643004/diff/390001/net/extras/sqlite/sqli...
File net/extras/sqlite/sqlite_persistent_cookie_store.h (right):

https://codereview.chromium.org/1016643004/diff/390001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store.h:12: 
IWYU: Include <utility> since you're using pair

https://codereview.chromium.org/1016643004/diff/390001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store.h:12: 
IWYU: Include <list> because of DeleteAllInList

https://codereview.chromium.org/1016643004/diff/390001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store.h:14: #include
"base/compiler_specific.h"
Unused?

https://codereview.chromium.org/1016643004/diff/390001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store.h:23: }
}  // namespace base

https://codereview.chromium.org/1016643004/diff/390001/net/extras/sqlite/sqli...
File net/extras/sqlite/sqlite_persistent_cookie_store_perftest.cc (right):

https://codereview.chromium.org/1016643004/diff/390001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store_perftest.cc:6: 
IWYU: vector (line 39)

https://codereview.chromium.org/1016643004/diff/390001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store_perftest.cc:62:
pool_owner_->pool()->GetNamedSequenceToken("client"));
Same remarks re: caching in test ctor

[mef, 2015-05-12 21:23:28]

lgtm

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
File content/browser/net/quota_policy_cookie_store.h (right):

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.h:5: // A cookie monster
persistent store that deletes session cookies on shutdown.
this comment seems out of date.

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
File content/browser/net/quota_policy_cookie_store_unittest.cc (right):

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store_unittest.cc:75: store_ = new
QuotaPolicyCookieStore(
nit: fits into one line.

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store_unittest.cc:136: // Reload and
test for persistence
nit: add .

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store_unittest.cc:140:
EXPECT_EQ("foo.com", cookies[0]->Domain());
are they guaranteed to be in this order?

[rohitrao (ping after 24h), 2015-05-13 17:05:08]

Ryan, can you please take another look at the changes to
quota_policy_cookie_store.cc and the associated unittests?

I had a hard time switching to WeakPtr because of threading issues.  The
QuotaPolicyCookie store is now effectively only usable from a single thread, but
that's not documented clearly anywhere.  Per IM, you mentioned that we tend to
treat classes as thread-unsafe unless otherwise specified.

The tests were a pain because we create and interact with the test DB on the
main thread.  I had to change client_task_runner_ to point to the main thread's
task runner, but that meant I also needed to manually spin the message loop
while Load()ing.  Is there a better way to write this test?

Thanks!

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
File content/browser/net/quota_policy_cookie_store.h (right):

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.h:5: // A cookie monster
persistent store that deletes session cookies on shutdown.
On 2015/05/12 21:23:27, mef wrote:
> this comment seems out of date.

I believe this is correct.  The job of this class is to delete session cookies
on shutdown.

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.h:16: #include
"base/memory/scoped_ptr.h"
On 2015/05/12 00:48:33, Ryan Sleevi wrote:
> Unused

Done.

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.h:19: #include
"net/extras/sqlite/sqlite_persistent_cookie_store.h"
On 2015/05/12 00:48:33, Ryan Sleevi wrote:
> Strictly speaking, do you have to depend on the SQLite implementation? It
seems
> like we could get away with just depending on the CookieMonster if it had the
> DeleteAll() method.
> 
> Just wondering out loud...

Hmmm.  I think we could depend on a CookieMonster::PersistentCookieStore if we
added the DeleteAllInList() method.  It's maybe a weird API to force into that
interface?  I'm not going to make any changes in this CL, but it's an
interesting idea.

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.h:26: }
On 2015/05/12 00:48:33, Ryan Sleevi wrote:
> }  // namespace base

Done.

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.h:31: }
On 2015/05/12 00:48:33, Ryan Sleevi wrote:
> }  // namespace net

Done.

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.h:35: }
On 2015/05/12 00:48:33, Ryan Sleevi wrote:
> }  // namespace storage

Done.

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.h:47: // Wraps the passed-in
|cookie_store|..
On 2015/05/12 00:48:34, Ryan Sleevi wrote:
> nit: extra . ?

Done.

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.h:49:
scoped_refptr<net::SQLitePersistentCookieStore> cookie_store,
On 2015/05/12 00:48:33, Ryan Sleevi wrote:
> const-ref unless you want to force callers to .Pass(); prefer the former over
> the latter.

Done.

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
File content/browser/net/quota_policy_cookie_store_unittest.cc (right):

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store_unittest.cc:59:
pool_owner_->pool()->GetNamedSequenceToken("background"));
On 2015/05/12 00:48:34, Ryan Sleevi wrote:
> There's no reason not to set these up during the ctor and cache them in member
> variables, rather than indirecting through these two calls repeatedly.

Done.  It's a little complicated because we destroy the pool a few times in each
test, so I have to clear the cache when that happens.

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store_unittest.cc:75: store_ = new
QuotaPolicyCookieStore(
On 2015/05/12 21:23:27, mef wrote:
> nit: fits into one line.

Fixed.

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store_unittest.cc:110: }
On 2015/05/12 00:48:34, Ryan Sleevi wrote:
> No idea why this isn't simply moved to the dtor rather than teardown, but
> *shrug*

Acknowledged.

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store_unittest.cc:136: // Reload and
test for persistence
On 2015/05/12 21:23:27, mef wrote:
> nit: add .

Done.

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store_unittest.cc:140:
EXPECT_EQ("foo.com", cookies[0]->Domain());
On 2015/05/12 21:23:27, mef wrote:
> are they guaranteed to be in this order?

Nope, fixed.

https://codereview.chromium.org/1016643004/diff/390001/net/extras/sqlite/sqli...
File net/extras/sqlite/sqlite_persistent_cookie_store.cc (right):

https://codereview.chromium.org/1016643004/diff/390001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store.cc:10: #include <utility>
On 2015/05/12 00:48:34, Ryan Sleevi wrote:
> utility & list go away when you fix the header

Done.

https://codereview.chromium.org/1016643004/diff/390001/net/extras/sqlite/sqli...
File net/extras/sqlite/sqlite_persistent_cookie_store.h (right):

https://codereview.chromium.org/1016643004/diff/390001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store.h:12: 
On 2015/05/12 00:48:34, Ryan Sleevi wrote:
> IWYU: Include <utility> since you're using pair

Done.

https://codereview.chromium.org/1016643004/diff/390001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store.h:12: 
On 2015/05/12 00:48:34, Ryan Sleevi wrote:
> IWYU: Include <list> because of DeleteAllInList

Done.

https://codereview.chromium.org/1016643004/diff/390001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store.h:14: #include
"base/compiler_specific.h"
On 2015/05/12 00:48:34, Ryan Sleevi wrote:
> Unused?

Done.

https://codereview.chromium.org/1016643004/diff/390001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store.h:23: }
On 2015/05/12 00:48:34, Ryan Sleevi wrote:
> }  // namespace base

Done.

https://codereview.chromium.org/1016643004/diff/390001/net/extras/sqlite/sqli...
File net/extras/sqlite/sqlite_persistent_cookie_store_perftest.cc (right):

https://codereview.chromium.org/1016643004/diff/390001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store_perftest.cc:6: 
On 2015/05/12 00:48:34, Ryan Sleevi wrote:
> IWYU: vector (line 39)

Done.

https://codereview.chromium.org/1016643004/diff/390001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store_perftest.cc:62:
pool_owner_->pool()->GetNamedSequenceToken("client"));
On 2015/05/12 00:48:34, Ryan Sleevi wrote:
> Same remarks re: caching in test ctor

Done.

[Ryan Sleevi, 2015-05-14 01:19:35]

After reviewing the mess of things, I think you're fine to go pre-WeakPtr
change. As we chatted, https://crbug.com/472744 tracks cleaning up all this
awfulness, which I didn't realize how far it went.

The rest LGTM, % a few nits :)

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
File content/browser/net/quota_policy_cookie_store_unittest.cc (right):

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store_unittest.cc:59:
pool_owner_->pool()->GetNamedSequenceToken("background"));
On 2015/05/13 17:05:08, rohitrao wrote:
> On 2015/05/12 00:48:34, Ryan Sleevi wrote:
> > There's no reason not to set these up during the ctor and cache them in
member
> > variables, rather than indirecting through these two calls repeatedly.
> 
> Done.  It's a little complicated because we destroy the pool a few times in
each
> test, so I have to clear the cache when that happens.

Yeah, but why not just do that in DestroyStore?

Or did I miss other places where we reset pool_owner_?

I agree that this complexity here probably isn't worth it - either going with a
hybrid approach or using the original behaviour is fine by me.

https://codereview.chromium.org/1016643004/diff/410001/content/browser/net/qu...
File content/browser/net/quota_policy_cookie_store.h (right):

https://codereview.chromium.org/1016643004/diff/410001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.h:46: public
base::SupportsWeakPtr<QuotaPolicyCookieStore> {
Future note: Never inherit from this ;)

WeakPtr's should always be an implementation detail of the class, never exposed.

That's by having a WeakPtrFactory be the last member of the class.

(Why we still have this? Because fixing things takes time and everyone forgets
about code health :P)

Note: From follow-up chat discussion, let's remove this and defer the proper
solution to https://crbug.com/472744

https://codereview.chromium.org/1016643004/diff/410001/content/browser/net/qu...
File content/browser/net/quota_policy_cookie_store_unittest.cc (right):

https://codereview.chromium.org/1016643004/diff/410001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store_unittest.cc:51:
base::MessageLoop::current()->Run();
note for future: Use RunLoops for this

https://codereview.chromium.org/1016643004/diff/410001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store_unittest.cc:159:
EXPECT_TRUE(found_persistent_cookie);
For this case, I doubt it's fully justified, but gmock has a container matcher
that is nifty with this

https://code.google.com/p/googlemock/wiki/CheatSheet#Container_Matchers

EXPECT_THAT(cookies, UnorderedElementsAre(CookieDomainEquals("foo.com"),
CookieDomainEquals("persistent.com")));

MATCHER_P(CookieDomainEquals, domain, std::string("cookie domain ") + (negation
? "isn't : "is") + " " + domain) {
  return arg->Domain == domain;
}


But I mean, who would do that? :P

https://codereview.chromium.org/1016643004/diff/410001/net/extras/sqlite/sqli...
File net/extras/sqlite/sqlite_persistent_cookie_store_perftest.cc (right):

https://codereview.chromium.org/1016643004/diff/410001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store_perftest.cc:70: return
client_task_runner_;
Same comments about "oof, sorry" :)

[rohitrao (ping after 24h), 2015-05-14 15:48:34]

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
File content/browser/net/quota_policy_cookie_store_unittest.cc (right):

https://codereview.chromium.org/1016643004/diff/390001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store_unittest.cc:59:
pool_owner_->pool()->GetNamedSequenceToken("background"));
> Yeah, but why not just do that in DestroyStore?
> Or did I miss other places where we reset pool_owner_?
> 
> I agree that this complexity here probably isn't worth it - either going with
a
> hybrid approach or using the original behaviour is fine by me.

Ended up going back to the original behavior, mostly because it was less code
and a bit simpler to read.

https://codereview.chromium.org/1016643004/diff/410001/content/browser/net/qu...
File content/browser/net/quota_policy_cookie_store.h (right):

https://codereview.chromium.org/1016643004/diff/410001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.h:46: public
base::SupportsWeakPtr<QuotaPolicyCookieStore> {
On 2015/05/14 01:19:35, Ryan Sleevi wrote:
> Future note: Never inherit from this ;)
> 
> WeakPtr's should always be an implementation detail of the class, never
exposed.
> 
> That's by having a WeakPtrFactory be the last member of the class.
> 
> (Why we still have this? Because fixing things takes time and everyone forgets
> about code health :P)
> 
> Note: From follow-up chat discussion, let's remove this and defer the proper
> solution to https://crbug.com/472744

Acknowledged.

https://codereview.chromium.org/1016643004/diff/410001/content/browser/net/qu...
File content/browser/net/quota_policy_cookie_store_unittest.cc (right):

https://codereview.chromium.org/1016643004/diff/410001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store_unittest.cc:51:
base::MessageLoop::current()->Run();
On 2015/05/14 01:19:35, Ryan Sleevi wrote:
> note for future: Use RunLoops for this

Acknowledged.

https://codereview.chromium.org/1016643004/diff/410001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store_unittest.cc:159:
EXPECT_TRUE(found_persistent_cookie);
On 2015/05/14 01:19:35, Ryan Sleevi wrote:
> For this case, I doubt it's fully justified, but gmock has a container matcher
> that is nifty with this
> 
> https://code.google.com/p/googlemock/wiki/CheatSheet#Container_Matchers
> 
> EXPECT_THAT(cookies, UnorderedElementsAre(CookieDomainEquals("foo.com"),
> CookieDomainEquals("persistent.com")));
> 
> MATCHER_P(CookieDomainEquals, domain, std::string("cookie domain ") +
(negation
> ? "isn't : "is") + " " + domain) {
>   return arg->Domain == domain;
> }
> 
> 
> But I mean, who would do that? :P

Acknowledged.

[rohitrao (ping after 24h), 2015-05-14 15:49:41]

rohitrao@chromium.org changed reviewers:
+ avi@chromium.org

[rohitrao (ping after 24h), 2015-05-14 15:49:41]

rohitrao@chromium.org changed required reviewers:
+ avi@chromium.org

[rohitrao (ping after 24h), 2015-05-14 15:49:43]

+Avi for content OWNERS approval

No code has changed substantially, I'm just splitting the old cookie DB into two
classes and moving one into net/.

[Avi (use Gerrit), 2015-05-14 19:11:31]

lgtm

nitting away

https://codereview.chromium.org/1016643004/diff/450001/content/browser/net/qu...
File content/browser/net/quota_policy_cookie_store.h (right):

https://codereview.chromium.org/1016643004/diff/450001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.h:5: // A cookie monster
persistent store that deletes session cookies on shutdown.
Is this comment useful given the one on the class itself on line 38?

https://codereview.chromium.org/1016643004/diff/450001/net/extras/sqlite/sqli...
File net/extras/sqlite/sqlite_persistent_cookie_store.h (right):

https://codereview.chromium.org/1016643004/diff/450001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store.h:5: // A sqlite implementation
of a cookie monster persistent store.
Same complaint vis a vis the comment below.

[rohitrao (ping after 24h), 2015-05-14 20:32:11]

Thanks for the reviews!

https://codereview.chromium.org/1016643004/diff/450001/content/browser/net/qu...
File content/browser/net/quota_policy_cookie_store.h (right):

https://codereview.chromium.org/1016643004/diff/450001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.h:5: // A cookie monster
persistent store that deletes session cookies on shutdown.
On 2015/05/14 19:11:31, Avi wrote:
> Is this comment useful given the one on the class itself on line 38?

Done.

https://codereview.chromium.org/1016643004/diff/450001/net/extras/sqlite/sqli...
File net/extras/sqlite/sqlite_persistent_cookie_store.h (right):

https://codereview.chromium.org/1016643004/diff/450001/net/extras/sqlite/sqli...
net/extras/sqlite/sqlite_persistent_cookie_store.h:5: // A sqlite implementation
of a cookie monster persistent store.
On 2015/05/14 19:11:31, Avi wrote:
> Same complaint vis a vis the comment below.

Done.

[rohitrao (ping after 24h), 2015-05-14 20:32:20]

The CQ bit was checked by rohitrao@chromium.org

[rohitrao (ping after 24h), 2015-05-14 20:32:21]

The patchset sent to the CQ was uploaded after l-g-t-m from mef@chromium.org,
rsleevi@chromium.org, avi@chromium.org
Link to the patchset: https://codereview.chromium.org/1016643004/#ps470001
(title: "Review.")

[commit-bot: I haz the power, 2015-05-14 20:33:21]

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

[commit-bot: I haz the power, 2015-05-14 22:22:39]

Committed patchset #25 (id:470001)

[commit-bot: I haz the power, 2015-05-14 22:23:26]

Patchset 25 (id:??) landed as
https://crrev.com/57adf29fb4e46a49272610140b097f85e1e805f5
Cr-Commit-Position: refs/heads/master@{#329950}

[henrika (OOO until Aug 14), 2015-05-15 11:12:27]

A revert of this CL (patchset #25 id:470001) has been created in
https://codereview.chromium.org/1134113007/ by henrika@chromium.org.

The reason for reverting is: Seems to break content_unittests
QuotaPolicyCookieStoreTest.TestPolicy. See
https://build.chromium.org/p/chromium.webrtc/builders/Win7%20Tester for details.

Reverting to see if the bot turns green again. I am unable to find any other CL
that can cause this..