Wait for store flush in CookieMonster::Delete*Task

Wait for store flush in CookieMonster::Delete*Task

Second attempt to commit this, after fixing a flaky test:
http://code.google.com/p/chromium/issues/detail?id=302914

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

[Randy Smith (Not in Mondays), 2013-06-27 14:30:20]

We shouldn't commit anything without Erik's approval; he's the boss of cookies. 
Having said that, I *think* (calendar doesn't confirm) that he's on vacation, so
I'll put my two cents in to allow you to keep moving:

* This looks like the right direction.
* Yeah, this->cookie_monster() should probably be pulled out into a variable.
* We want to modify the deletion tasks that may occur because of user action,
and not the ones that don't.  Specifically, I think it would be good to make
sure that deletion sparked by garbage collection/expiration does not flush to
store.
* Arrggh.  I personally don't feel comfortable with having a "the callback will
be run on a random thread" in a high-level consumer facing method.  I think this
means that we're either going to need to provide more guarantees around
FlushStore, or wrap the callback so that it'll be run on the thread that the
delete is called on.  Or maybe Erik will be more comfortable with the randomness
than I (but my guess is not).  Even if he is, we'd need to go through the
callers and make sure none of them need to be modified.  
* Yeah, I think mocking the store then writing a test that asserts that when the
callback comes back the store has seen the delete should do it.  Do that first
and make sure it fails :-}.

https://codereview.chromium.org/18032002/diff/1/net/cookies/cookie_monster.cc
File net/cookies/cookie_monster.cc (left):

https://codereview.chromium.org/18032002/diff/1/net/cookies/cookie_monster.cc...
net/cookies/cookie_monster.cc:595: base::Unretained(&callback_), num_deleted));
Erik: Why was the base::Unretained() here?  It gives me the heebee jeebees. 
Where's your lifetime guarantee if InvokeCallback hands this off to a different
thread?  Is there any place else in this file where we need to worry about this?

[Randy Smith (Not in Mondays), 2013-06-27 14:31:21]

On 2013/06/27 08:20:17, philipj wrote:

Also: In general you want to specify a reviewer or reviewers for even this level
of query, just so that people on the cc list know that they shouldn't ignore
your email :-}.  Erik and I are the appropriate reviewers for this type of
change.

[philipj_slow, 2013-06-28 07:09:47]

On 2013/06/27 14:31:21, rdsmith wrote:
> On 2013/06/27 08:20:17, philipj wrote:
> 
> Also: In general you want to specify a reviewer or reviewers for even this
level
> of query, just so that people on the cc list know that they shouldn't ignore
> your email :-}.  Erik and I are the appropriate reviewers for this type of
> change.

Sure thing! (Sometimes it's a bit hard to figure out who an appropriate reviewer
is, but in this case the OWNERS file was pretty clear: erikwright@chromium.org)

[philipj_slow, 2013-06-28 07:38:41]

Another approach to this to consider is to instead give the clients access to a
flush call with a callback, so that those code paths that represent a user
deletion can explicitly wait for flush. This would keep deletion and flushing
separate, and my hunch is that it requires fewer changes. If calling
CookieMonster::FlushStore directly is acceptable, the change to
BrowsingDataRemover could be:

cookie_store->GetCookieMonster()->FlushStore(base::Bind(&BrowsingDataRemover::OnFlushedCookies,
base::Unretained(this)))

Also add OnFlushedCookies and make BrowsingDataRemover wait for both deletion
and flush.

Would this be workable, and are there very many other code paths where users
delete cookies? (On Android, this is the only one.)

[Randy Smith (Not in Mondays), 2013-06-28 19:53:34]

On 2013/06/28 07:38:41, philipj wrote:
> Another approach to this to consider is to instead give the clients access to
a
> flush call with a callback, so that those code paths that represent a user
> deletion can explicitly wait for flush. This would keep deletion and flushing
> separate, and my hunch is that it requires fewer changes. If calling
> CookieMonster::FlushStore directly is acceptable, the change to
> BrowsingDataRemover could be:
> 
>
cookie_store->GetCookieMonster()->FlushStore(base::Bind(&BrowsingDataRemover::OnFlushedCookies,
> base::Unretained(this)))
> 
> Also add OnFlushedCookies and make BrowsingDataRemover wait for both deletion
> and flush.
> 
> Would this be workable, and are there very many other code paths where users
> delete cookies? (On Android, this is the only one.)

I'm inclined to keep the interface simple.  The only thing I'm concerned about
is cases where some internal, batch process will be deleting a whole lot of
cookies at once where we don't care about commitment.  The only such process I
can think of is cookie expiry, and it doesn't go through the async interface
anyway.  So I think the right thing is just making all async deletion interfaces
flush to the store.

[erikwright (departed), 2013-07-05 19:43:35]

> Is this an acceptable solution at all?

Yes.

> 2. Is the !callback_.is_null() check still necessary? It appears not.

Yes - somewhere, before attempting to invoke it, you need to check that; else it
may crash. I don't know what happens if you rebind a callback... does the new
(wrapping) callback report 'is_null() == true' or does it just crash when
calling the inner callback. In the former case, I assume that Flush would do the
right thing if _its_ callback is null. In the latter case it will crash even if
it tries to detect null callbacks.

In any case, given my suggesting to bind to InvokeCallback what you'll need to
do instead is the equivalent of

Flush(callback_.is_null() ? base::Closure() : base::Bind(InvokeCallback, this,
num_deleted))

> 3. Should this->cookie_monster() be extracted into a variable?

I don't see why. It's an accessor, in lower_case, the implication is that it is
zero cost (and, in this case, presumed to be inlined).

> 4. Should all CookieMonster::Delete*Task be modified accordingly?

Yes. It would be appropriate, however, to scan through the code for their
callers and make sure that it is appropriate in all cases.

> 5. Is new documentation required mirroring the warning about thread safety
from CookieMonster::FlushStore?

No, because you can restore the thread guarantee as mentioned in my comment.

> 6. Is the correct way to unittest this just by using a
FlushablePersistentStore and counting?

Probably, yeah.

On 2013/06/28 19:53:34, rdsmith wrote:
> On 2013/06/28 07:38:41, philipj wrote:
> > Another approach to this to consider is to instead give the clients access
to
> a
> > flush call with a callback, so that those code paths that represent a user
> > deletion can explicitly wait for flush. This would keep deletion and
flushing
> > separate, and my hunch is that it requires fewer changes. If calling
> > CookieMonster::FlushStore directly is acceptable, the change to
> > BrowsingDataRemover could be:
> > 
> >
>
cookie_store->GetCookieMonster()->FlushStore(base::Bind(&BrowsingDataRemover::OnFlushedCookies,
> > base::Unretained(this)))
> > 
> > Also add OnFlushedCookies and make BrowsingDataRemover wait for both
deletion
> > and flush.
> > 
> > Would this be workable, and are there very many other code paths where users
> > delete cookies? (On Android, this is the only one.)
> 
> I'm inclined to keep the interface simple.  The only thing I'm concerned about
> is cases where some internal, batch process will be deleting a whole lot of
> cookies at once where we don't care about commitment.  The only such process I
> can think of is cookie expiry, and it doesn't go through the async interface
> anyway.  So I think the right thing is just making all async deletion
interfaces
> flush to the store.

Agreed.

[philipj_slow, 2013-07-17 12:05:19]

Sorry for the delay, but I've now found some time to look into this again. Here
are my notes:

The objective is to make CookieMonster::Delete*Async run the callback only after
CookieMonster::FlushStore has been run internally.

Delete*Async creates and queues/runs Delete*Task.

Delete*Task::Run is where the callback is invoked. After ::Run has returned the
task object will be destroyed, so either its lifetime must be prolonged or a new
object must be created to store the delete_count and callback, with appropriate
lifetime. However, CookieMonster::CookieMonsterTask::InvokeCallback does
everything we want to guarantee that the callback is on the correct thread, so
duplicating that doesn't seem like fun. (The documentation of ::InvokeCallback
also doesn't give me confidence that I understand all of the details.)

Fundamentally, the current CookieMonsterTask classes are just queued operations,
AFAICT none of them need to wait for an asynchronous call to finish, like
FlushStore. Because of this all my attempts at a quick and simple fix have
failed. (The submitted code simply crashes, because the callback will come after
the task object has been destroyed.)

Somehow increasing the lifetime of the CookieMonsterTask seems sensible, but
it's refcounted from ::Delete*Async, so it can't take a reference to itself
while waiting for the callback. Also, one must wait until after deleting the
cookie before calling FlushStore (I suppose, I haven't tested if changing the
order causes raciness) so one can't call FlushStore right in ::Delete*Async
where the refcounting would work.

Also, how is it currently guaranteed that the CookieMonsterTask instances (with
a CookieMonster pointer) don't outlive the CookieMonster?

In short, I don't know how to proceed and would appreciate some pointers. (I'll
be away until next Wednesday, so if you deem it quicker to just make the fix
yourself I will not be offended.)

[erikwright (departed), 2013-07-17 16:53:32]

Sorry, I had some unsent drafts. I'll read through today's message now.

https://codereview.chromium.org/18032002/diff/1/net/cookies/cookie_monster.cc
File net/cookies/cookie_monster.cc (left):

https://codereview.chromium.org/18032002/diff/1/net/cookies/cookie_monster.cc...
net/cookies/cookie_monster.cc:595: base::Unretained(&callback_), num_deleted));
On 2013/06/27 14:30:20, rdsmith wrote:
> Erik: Why was the base::Unretained() here?  It gives me the heebee jeebees. 
> Where's your lifetime guarantee if InvokeCallback hands this off to a
different
> thread?  Is there any place else in this file where we need to worry about
this?

This was written prior to the ability to re-bind callbacks.

InvokeCallback, if it posts to another thread, posts a task that add-refs the
CookieMonsterTask. |callback_| is a member of CookierMonsterTask. So it is safe.

But rebinding is much better and it would be ideal to update this code to use
it.

https://codereview.chromium.org/18032002/diff/1/net/cookies/cookie_monster.cc
File net/cookies/cookie_monster.cc (right):

https://codereview.chromium.org/18032002/diff/1/net/cookies/cookie_monster.cc...
net/cookies/cookie_monster.cc:594:
this->cookie_monster()->FlushStore(base::Bind(callback_, num_deleted));
Instead of binding (callback_, num_deleted) directly, probably you should bind
(InvokeCallback, this, num_deleted).

That will restore the original thread guarantees.

[erikwright (departed), 2013-07-17 17:26:22]

On 2013/07/17 12:05:19, philipj wrote:
> Sorry for the delay, but I've now found some time to look into this again.
Here
> are my notes:
> 
> The objective is to make CookieMonster::Delete*Async run the callback only
after
> CookieMonster::FlushStore has been run internally.
> 
> Delete*Async creates and queues/runs Delete*Task.
> 
> Delete*Task::Run is where the callback is invoked. After ::Run has returned
the
> task object will be destroyed, so either its lifetime must be prolonged or a
new
> object must be created to store the delete_count and callback, with
appropriate
> lifetime. However, CookieMonster::CookieMonsterTask::InvokeCallback does
> everything we want to guarantee that the callback is on the correct thread, so
> duplicating that doesn't seem like fun. (The documentation of ::InvokeCallback
> also doesn't give me confidence that I understand all of the details.)
> 
> Fundamentally, the current CookieMonsterTask classes are just queued
operations,
> AFAICT none of them need to wait for an asynchronous call to finish, like
> FlushStore. Because of this all my attempts at a quick and simple fix have
> failed. (The submitted code simply crashes, because the callback will come
after
> the task object has been destroyed.)
> 
> Somehow increasing the lifetime of the CookieMonsterTask seems sensible, but
> it's refcounted from ::Delete*Async, so it can't take a reference to itself
> while waiting for the callback.

You can take a reference to a refcounted object from a raw pointer. If 'this'
extends RefCounted or (RCThreadSafe) you can do

scoped_refptr<MyClass> my_ref(this);

That will happily increment the refcount. Your comment seems to imply that you
expect a shared_ptr semantic, but ref counting in Chrome is invasive and
therefore permits the above.

> Also, one must wait until after deleting the
> cookie before calling FlushStore (I suppose, I haven't tested if changing the
> order causes raciness) so one can't call FlushStore right in ::Delete*Async
> where the refcounting would work.
> 
> Also, how is it currently guaranteed that the CookieMonsterTask instances
(with
> a CookieMonster pointer) don't outlive the CookieMonster?

At the moment it's not an issue because the tasks are exclusively owned and used
by the CookieMonster. Because, at the moment, they do post themselves to
arbitrary threads for InvokeCallback it seems theoretically possible that they
outlive the CM but they don't use the CM pointer when they finally run there.
The use of rebinding would be a better solution that would eliminate that
ambiguity.

You will be creating a callback to do this->InvokeCallback(num_deleted), where
'this' is the original task. Since 'this' is refcounted it will be guaranteed to
live until the flush call completes.

> 
> In short, I don't know how to proceed and would appreciate some pointers.
(I'll
> be away until next Wednesday, so if you deem it quicker to just make the fix
> yourself I will not be offended.)

[philipj_slow, 2013-08-08 13:30:03]

(Sorry for the delay.)

Thanks for the pointers, erikwright! You were right that I was confused about
refcounting in Chromium, and with that information things became a lot simpler.

This new patch set shows a solution which I think should work if used for all of
Delete*Async. I had to change NewMockPersistentCookieStore to get the old tests
passing at all, but I don't know if you approve of that. I haven't added a new
specific test, since the existing tests would fail if FlushStore weren't called.

[erikwright (departed), 2013-08-09 17:31:51]

LG. Small suggestion about the OO design. Go ahead and apply to the other delete
methods?

https://codereview.chromium.org/18032002/diff/12001/net/cookies/cookie_monste...
File net/cookies/cookie_monster.cc (right):

https://codereview.chromium.org/18032002/diff/12001/net/cookies/cookie_monste...
net/cookies/cookie_monster.cc:613: void
CookieMonster::DeleteAllCreatedBetweenTask::Run() {
Pull this method up to DeleteTask and delegate to a pure-virtual protected
method:

virtual int RunDeleteTask();

Then the subclass just needs to implement 

int RunDeleteTask() {
  return this->cookie_monster()->DeleteAll...(...);
}

The call to Bind and Flush will only appear in DeleteTask::Run.

DeleteTask may be private.

[philipj_slow, 2013-08-12 11:16:23]

I had the same direction in mind, so here's the new patch set which converts all
Delete*Tasks except 2:

DeleteCanonicalCookieTask uses DeleteCookieCallback which takes a bool instead
of the int that DeleteCallback uses.

DeleteCookieTask uses a plain base::Closure.

I'm not sure what to do with these. Both could be converted to use
DeleteCallback, returning 1 on success on 0 on failure. Alternatively,
RunDeleteTask could be made to return a close to be invoked after flush, which
would hide the num_deleted in a bunch of places, but would also require adding
back the Bind() everywhere. What do you think?

[philipj_slow, 2013-08-12 11:17:03]

> RunDeleteTask could be made to return a close to be invoked after flush, which

... return a closure ...

[philipj_slow, 2013-08-12 14:19:21]

Apologies for dragging this out, but I'll be away (funeral) for the rest of this
week.

[philipj_slow, 2013-08-20 07:20:06]

I'm back now, and need some feedback on comment #13 before proceeding.

[erikwright (departed), 2013-09-04 15:04:38]

https://codereview.chromium.org/18032002/diff/18001/net/cookies/cookie_monste...
File net/cookies/cookie_monster.cc (right):

https://codereview.chromium.org/18032002/diff/18001/net/cookies/cookie_monste...
net/cookies/cookie_monster.cc:534: class CookieMonster::DeleteTask
make this a template class

template <typename Result> class CookieMonster::DeleteTask;

Make the return type of RunDeleteTask be Result.

Make the signature of FlushDone be 'void(const Closure& callback)'.

Then in Run:

template <typename Result> void CookieMonster::DeleteTask<Result>::Run() {
  this->cookie_monster()->FlushStore(
      base:Bind(&CookieMonster::DeleteTask::FlushDone, this,
                RunDeleteTaskAndBindCallback(this, callback_)));
}

RunDeleteTaskAndBindCallback is a free-function template with an override for
void:

template <typename Result>
base::Closure RunDeleteTaskAndBindCallback(
    DeleteTask<Result>* task,
    const base::Callback<void(Result)>& callback) {
  return base::Bind(callback, task->RunDeleteTask());
}

base::Closure RunDeleteTaskAndBindCallback(
    DeleteTask<void>* task,
    const base::Closure& callback) {
  task->RunDeleteTask();
  return callback;
}

It's possible that the compiler will find that override ambiguous. If it does
you'll need to replace the free function with a struct having a single static
member function, and replace the override with a specialization for void.

https://codereview.chromium.org/18032002/diff/18001/net/cookies/cookie_monste...
net/cookies/cookie_monster.cc:585: 
remove blank line

[philipj_slow, 2013-09-05 14:33:40]

On 2013/09/04 15:04:38, erikwright wrote:
>
https://codereview.chromium.org/18032002/diff/18001/net/cookies/cookie_monste...
> File net/cookies/cookie_monster.cc (right):
> 
>
https://codereview.chromium.org/18032002/diff/18001/net/cookies/cookie_monste...
> net/cookies/cookie_monster.cc:534: class CookieMonster::DeleteTask
> make this a template class

Unfortunately, using the result type as the template parameter won't work,
because the callback member then becomes:

base::Callback<void(Result)> callback_;

Even though Result = void looks like it makes the callback equivalent to
base::Closure, it won't work because void isn't a type like others. Here's a
minimized example and what GCC has to say about it:

template <typename Result> class DeleteTask {
 public:
  DeleteTask(const base::Callback<void(Result)>& callback)
      : callback_(callback) {
  }

 private:
  base::Callback<void(Result)> callback_;

  DISALLOW_COPY_AND_ASSIGN(DeleteTask);
};

class DeleteCookieTask : public DeleteTask<void> {
};

../../net/cookies/cookie_monster.cc: In instantiation of ‘class
net::DeleteTask<void>’:
../../net/cookies/cookie_monster.cc:803:33:   required from here
../../net/cookies/cookie_monster.cc:793:3: error: invalid parameter type ‘void’
../../net/cookies/cookie_monster.cc:793:3: error: in declaration
‘net::DeleteTask<Result>::DeleteTask(const base::Callback<void(Result)>&)’
../../net/cookies/cookie_monster.cc:798:32: error: invalid parameter type ‘void’
../../net/cookies/cookie_monster.cc:798:32: error: in declaration
‘net::DeleteTask<Result>::callback_’

So what I've done instead is to make it template <typename Callback, typename
Result> class DeleteTask.

It's rather verbose and one line became too long without an obvious way to make
it shorter. Can I just remove the CookieMonster:: prefix where it isn't needed?
There are many places like that, both in the existing code and in my changes.

[philipj_slow, 2013-09-05 14:39:12]

https://codereview.chromium.org/18032002/diff/27001/net/cookies/cookie_monste...
File net/cookies/cookie_monster.cc (right):

https://codereview.chromium.org/18032002/diff/27001/net/cookies/cookie_monste...
net/cookies/cookie_monster.cc:564: template <typename Callback>
I wanted to do this:

template <>
base::Closure RunDeleteTaskAndBindCallback(
    CookieMonster::DeleteTask<base::Closure, void>* task,
    const base::Closure& callback) {
  task->RunDeleteTask();
  return callback;
}

But GCC disapproves:

../../net/cookies/cookie_monster.h:314:55: error: ‘template<class Callback,
class Result> class net::CookieMonster::DeleteTask’ is private
../../net/cookies/cookie_monster.cc:567:34: error: within this context

Not sure why that is.

[erikwright (departed), 2013-09-05 14:52:48]

On 2013/09/05 14:33:40, philipj wrote:
> On 2013/09/04 15:04:38, erikwright wrote:
> >
>
https://codereview.chromium.org/18032002/diff/18001/net/cookies/cookie_monste...
> > File net/cookies/cookie_monster.cc (right):
> > 
> >
>
https://codereview.chromium.org/18032002/diff/18001/net/cookies/cookie_monste...
> > net/cookies/cookie_monster.cc:534: class CookieMonster::DeleteTask
> > make this a template class
> 
> Unfortunately, using the result type as the template parameter won't work,
> because the callback member then becomes:
> 
> base::Callback<void(Result)> callback_;
> 
> Even though Result = void looks like it makes the callback equivalent to
> base::Closure, it won't work because void isn't a type like others. Here's a
> minimized example and what GCC has to say about it:
> 
> template <typename Result> class DeleteTask {
>  public:
>   DeleteTask(const base::Callback<void(Result)>& callback)
>       : callback_(callback) {
>   }
> 
>  private:
>   base::Callback<void(Result)> callback_;
> 
>   DISALLOW_COPY_AND_ASSIGN(DeleteTask);
> };
> 
> class DeleteCookieTask : public DeleteTask<void> {
> };
> 
> ../../net/cookies/cookie_monster.cc: In instantiation of ‘class
> net::DeleteTask<void>’:
> ../../net/cookies/cookie_monster.cc:803:33:   required from here
> ../../net/cookies/cookie_monster.cc:793:3: error: invalid parameter type
‘void’
> ../../net/cookies/cookie_monster.cc:793:3: error: in declaration
> ‘net::DeleteTask<Result>::DeleteTask(const base::Callback<void(Result)>&)’
> ../../net/cookies/cookie_monster.cc:798:32: error: invalid parameter type
‘void’
> ../../net/cookies/cookie_monster.cc:798:32: error: in declaration
> ‘net::DeleteTask<Result>::callback_’
> 
> So what I've done instead is to make it template <typename Callback, typename
> Result> class DeleteTask.

One way to solve this (and it may or may not be the right thing to do, there are
tradeoffs to all of this) is the following:

template <typename Result> struct CallbackType {
  typedef Callback<void(Result)> Type;
};

template <> struct CallbackType<void> {
  typedef Closure Type;
};

Then instead of Callback<void(Result)> on lines 793 and 798 you use
CallbackType<Result>::Type.

> 
> It's rather verbose and one line became too long without an obvious way to
make
> it shorter. Can I just remove the CookieMonster:: prefix where it isn't
needed?
> There are many places like that, both in the existing code and in my changes.

I don't mind removing the prefix anywhere it's not required.

[erikwright (departed), 2013-09-05 20:28:18]

https://codereview.chromium.org/18032002/diff/27001/net/cookies/cookie_monste...
File net/cookies/cookie_monster.cc (right):

https://codereview.chromium.org/18032002/diff/27001/net/cookies/cookie_monste...
net/cookies/cookie_monster.cc:564: template <typename Callback>
On 2013/09/05 14:39:12, philipj wrote:
> I wanted to do this:
> 
> template <>
> base::Closure RunDeleteTaskAndBindCallback(
>     CookieMonster::DeleteTask<base::Closure, void>* task,
>     const base::Closure& callback) {
>   task->RunDeleteTask();
>   return callback;
> }
> 
> But GCC disapproves:
> 
> ../../net/cookies/cookie_monster.h:314:55: error: ‘template<class Callback,
> class Result> class net::CookieMonster::DeleteTask’ is private
> ../../net/cookies/cookie_monster.cc:567:34: error: within this context
> 
> Not sure why that is.

Presumably in cookie_monster.h the declaration of 'class DeleteTask' is in the
private section.

The struct-based approach, with the struct being declared inline in
DeleteTask::Run, would probably work, as follows:

template <typename Result> void CookieMonster::DeleteTask<Result>::Run() {
  template <typename Result> struct Helper {
    base::Closure RunDeleteTask(
        DeleteTask<Result>* task,
        const base::Callback<void(Result)>& callback) {
      return base::Bind(callback, task->RunDeleteTask());
    }
  };

  template <> struct Helper<void> {
    base::Closure RunDeleteTask(
        DeleteTask<void>* task,
        const base::Closure& callback) {
      task->RunDeleteTask();
      return callback;
    }
  };

  this->cookie_monster()->FlushStore(
      base:Bind(&CookieMonster::DeleteTask::FlushDone, this,
                Helper::RunDeleteTask(this, callback_)));
}


If that doesn't work, you could make the struct a nested class of DeleteTask
too.

[philipj_slow, 2013-09-06 13:27:48]

Thanks Erik, some great ideas there. I went with the template struct to solve
the callback signature problem, but found that just making
RunDeleteTaskAndBindCallback a member function simplified things, so that's what
I did.

I'm submitting the prefix-removal as a separate changeset so that it can easily
be reverted if you don't like it, it touched rather many lines in the end. I
could also do it as a separate review after this if you prefer.

[erikwright (departed), 2013-09-12 17:25:44]

https://codereview.chromium.org/18032002/diff/37001/net/cookies/cookie_monste...
File net/cookies/cookie_monster.cc (right):

https://codereview.chromium.org/18032002/diff/37001/net/cookies/cookie_monste...
net/cookies/cookie_monster.cc:549: virtual Result RunDeleteTask() = 0;
can be private.

https://codereview.chromium.org/18032002/diff/37001/net/cookies/cookie_monste...
net/cookies/cookie_monster.cc:554: void FlushDone(const base::Closure&
callback);
can be private

https://codereview.chromium.org/18032002/diff/37001/net/cookies/cookie_monste...
net/cookies/cookie_monster.cc:567: return base::Bind(callback_,
RunDeleteTask());
if callback_.is_null() you will need to just do RunDeleteTask and then return
base::Closure().

[philipj_slow, 2013-09-13 07:09:07]

Hmm, I'm not sure if email is sent out for a new patch set (I didn't get one),
so ping it is. (Maybe everyone except the one who pushed the change gets mail?)

[erikwright (departed), 2013-09-13 14:54:29]

LGTM!

This is a good improvement. Thank you very much.

https://codereview.chromium.org/18032002/diff/46001/net/cookies/cookie_monste...
File net/cookies/cookie_monster.cc (right):

https://codereview.chromium.org/18032002/diff/46001/net/cookies/cookie_monste...
net/cookies/cookie_monster.cc:556: 
nit remove blank between these internal helper functions.

[erikwright (departed), 2013-09-13 14:54:48]

On 2013/09/13 07:09:07, philipj wrote:
> Hmm, I'm not sure if email is sent out for a new patch set (I didn't get one),
> so ping it is. (Maybe everyone except the one who pushed the change gets
mail?)

No email is sent for patchsets.

[commit-bot: I haz the power, 2013-09-16 06:48:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/philipj@opera.com/18032002/46001

[commit-bot: I haz the power, 2013-09-16 07:21:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/philipj@opera.com/18032002/69001

[commit-bot: I haz the power, 2013-09-16 10:15:06]

Change committed as 223324

[philipj_slow, 2013-09-16 11:21:56]

On 2013/09/13 14:54:29, erikwright wrote:
> LGTM!
> 
> This is a good improvement. Thank you very much.

This was my first commit landed in Chromium (excluding Blink), thanks for your
patience and feedback!

[commit-bot: I haz the power, 2013-10-03 06:21:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/philipj@opera.com/18032002/69001

[commit-bot: I haz the power, 2013-10-03 10:13:22]

Change committed as 226720