Finalize a CL originally by departed intern ycxiao@ that detaches the loading of cookies from the...

Finalize a CL originally by departed intern ycxiao@ that detaches the loading of cookies from the IO thread.

They are now loaded on the DB thread. Cookie operations received in the meantime are queued and executed, on the IO thread, in the order they were received, when loading completes.

A few straggler clients are updated to use the asynchronous CookieStore/CookieMonster API as part of this CL, as the synchronous API is removed.

BUG=68657
TEST=net_unittests / DeferredCookieTaskTest.* and CookieMonsterTest.*

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

[erikwright (departed), 2011-09-06 17:33:59]

This CL removes the deprecated synchronous cookies API and detaches cookie
loading from the IO thread.

timsteele: Only for the removal of dead code in
chrome/browser/ui/webui/ntp/new_tab_page_sync_handler.cc

estade: Only for chrome/browser/fast_shutdown_uitest.cc . I removed a check for
the contents of the cookie store before the test runs because (1) it is not safe
to check while the browser has the DB open and (2) the automation instance
launched starts by definition with a clear profile.

tony: Only for changes in
webkit/tools/test_shell/simple_resource_loader_bridge.cc as you are the OWNER

rdsmith: Please take a look at everything. Note my responses to your previous
comments here: http://codereview.chromium.org/7598001/ (unfortunately, I am not
the owner of that CL so I needed to start a new one).

http://codereview.chromium.org/7833042/diff/1/net/base/cookie_monster.cc
File net/base/cookie_monster.cc (left):

http://codereview.chromium.org/7833042/diff/1/net/base/cookie_monster.cc#oldc...
net/base/cookie_monster.cc:1019: TimeTicks beginning_time(TimeTicks::Now());
It's noteworthy that this changes the meaning of this histogram.

It now only measures the time to store the cookies after they are loaded from
disk. It's not obvious what the correct metric is. Suggestions:

1)Break it up into two metrics, one for DB loading time and one for parsing and
insertion time.
2) Take the time in InitStore, bind it into the OnLoaded callback.

In the former case, we should ideally back port this into M15 (if this patch
doesn't make it in there) in order to have a comparable metric. Of note is that,
as we further break down the loading into per-domain chunks, we would presumably
want to aggregate the loading time for all chunks, but the data gradually gets
less and less comparable (though still perhaps meaningful as a trend).

In the latter case, our data becomes a little less comparable, as it's no longer
"contiguous time the IO thread was blocked" but rather "total wall time spent
across two threads and multiple tasks, including arbitrary unrelated task
execution."

If it is theoretically possible to get this CL into M15 I'd rather do it and fix
the metric later.

[Evan Stade, 2011-09-06 17:51:30]

probably better to get sreeram to review
chrome/browser/fast_shutdown_interactive_uitest.cc
rather than me.

[sreeram, 2011-09-06 18:13:04]

LGTM for chrome/browser/fast_shutdown_uitest.cc

[tony, 2011-09-06 19:12:12]

test_shell LGTM

[tim (not reviewing), 2011-09-07 17:21:40]

new_tab_page_sync_handler LGTM

[Randy Smith (Not in Mondays), 2011-09-07 19:04:07]

LGTM modulo notes below, most of which are suggestions (i.e. not binding)
anyway.  I would like the commenting nits and the BaseCallback::run() change,
but the rest is at your discretion.

http://codereview.chromium.org/7833042/diff/1/net/base/cookie_monster.cc
File net/base/cookie_monster.cc (left):

http://codereview.chromium.org/7833042/diff/1/net/base/cookie_monster.cc#oldc...
net/base/cookie_monster.cc:1019: TimeTicks beginning_time(TimeTicks::Now());
On 2011/09/06 17:33:59, erikwright wrote:
> It's noteworthy that this changes the meaning of this histogram.
> 
> It now only measures the time to store the cookies after they are loaded from
> disk. It's not obvious what the correct metric is. Suggestions:
> 
> 1)Break it up into two metrics, one for DB loading time and one for parsing
and
> insertion time.
> 2) Take the time in InitStore, bind it into the OnLoaded callback.
> 
> In the former case, we should ideally back port this into M15 (if this patch
> doesn't make it in there) in order to have a comparable metric. Of note is
that,
> as we further break down the loading into per-domain chunks, we would
presumably
> want to aggregate the loading time for all chunks, but the data gradually gets
> less and less comparable (though still perhaps meaningful as a trend).
> 
> In the latter case, our data becomes a little less comparable, as it's no
longer
> "contiguous time the IO thread was blocked" but rather "total wall time spent
> across two threads and multiple tasks, including arbitrary unrelated task
> execution."
> 
> If it is theoretically possible to get this CL into M15 I'd rather do it and
fix
> the metric later.

Long-term: For this CL, I'd like to keep the metric as close to "how much the
cookie store loading is adding to startup" as we can manage.  For this CL I
think that means storing the beginning time before the task is dispatched, and
recording the elapsed wall-clock time when we've finished loading the
cookies--that won't be perfect (because anything else that happens in that
period will be counted) but I think it'll be comparable (because the dispatch
will occur on first web page load, which is after much of browser load, and
there won't be much else going on except for web page loads at that point).   As
the breakup is done, it'll get more complex, but we could try to find some way
to mark "when startup (including page restore) is done" and add up (ignoring
overlaps) the time between dispatch and completion for each store load that's
done before startup is complete.

Short-term: I don't like changing the behavior of this metric until we have to,
so if it doesn't cost you too much, I'd like you to switch back to pre-loading
time.  But we don't have a desperate need to compare this across releases, so if
you want to change this metric's meaning, I don't have major objections--just
remember it when you're evaluating the histograms :-}.

This does bring up the point that now that this load is async, you might be able
to get an immediate improvement in startup time by kicking off the load from
Browser startup as opposed to waiting to first web page load.  Won't help a lot
for session restore, but will make the wait disappear for other workflows.

http://codereview.chromium.org/7833042/diff/14009/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/7833042/diff/14009/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:30: // database thread.  It
batches operations and commits them on a timer.
nit: Could you add a comment here about thread access?  This class is accessed
on multiple threads, and a couple of member functions are protected by locks,
and at least one (db_) is protected by serialization (though less so than it
used to be) and I'd feel easier if there was a comment somewhere that summarized
all of that.

http://codereview.chromium.org/7833042/diff/14009/net/base/cookie_monster.cc
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/7833042/diff/14009/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:578: : public
base::RefCountedThreadSafe<CookieMonsterTask> {
I feel as if RefCountedThreadSafe<> is what you want to use when there isn't a
clear owner of an object.  When there is a clear owner (even if that owner
changes over the lifetime of the object) you want scoped_ptr or just
transferring the pointer and deleting it when you're done.  Did you look into
that option?  (I don't know how easy it is to implement with Bind() and the new
callback mechanism, but channeling willchan@, there should be a way to do it.)

http://codereview.chromium.org/7833042/diff/14009/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:615: if (!callback.is_null()) {
nit: Probably easier to read code if you reverse the test and return.

http://codereview.chromium.org/7833042/diff/14009/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:666: if (!callback_.is_null()) {
This test (and its siblings) seems redundant with the test inside of
InvokeCallback; can one of them be eliminated?

http://codereview.chromium.org/7833042/diff/14009/net/base/cookie_monster_per...
File net/base/cookie_monster_perftest.cc (left):

http://codereview.chromium.org/7833042/diff/14009/net/base/cookie_monster_per...
net/base/cookie_monster_perftest.cc:18: class ParsedCookieTest : public
testing::Test { };
Why is it ok to get rid of this?  (I'm assuming it is--question is for my own
education.)

http://codereview.chromium.org/7833042/diff/14009/net/base/cookie_monster_per...
File net/base/cookie_monster_perftest.cc (right):

http://codereview.chromium.org/7833042/diff/14009/net/base/cookie_monster_per...
net/base/cookie_monster_perftest.cc:58: bool run() { return run_; }
nit: const?

http://codereview.chromium.org/7833042/diff/14009/net/base/cookie_monster_per...
net/base/cookie_monster_perftest.cc:59: void reset() { run_ = false; }
nit: The semantic difference between BaseCallback::run() and <Child of
BaseCallback>::Run() is great enough (state probe vs. take action) that I'd be
inclined to make the names more different.  Maybe has_run_, has_run() and
reset_has_run()?

http://codereview.chromium.org/7833042/diff/14009/net/base/cookie_monster_per...
net/base/cookie_monster_perftest.cc:109:
MessageLoop::current()->RunAllPending();
Does this guarantee that we've received the callback?  I guess if everything's
on the same message loop, it does, because by the time the first task has been
run the return one will have been posted.  Still, that reasoning probably
deserves a comment.

[erikwright (departed), 2011-09-08 02:48:58]

Great. Barring negative results from the try servers I will hit the commit
button Thursday morning.

http://codereview.chromium.org/7833042/diff/14009/chrome/browser/net/sqlite_p...
File chrome/browser/net/sqlite_persistent_cookie_store.cc (right):

http://codereview.chromium.org/7833042/diff/14009/chrome/browser/net/sqlite_p...
chrome/browser/net/sqlite_persistent_cookie_store.cc:30: // database thread.  It
batches operations and commits them on a timer.
On 2011/09/07 19:04:07, rdsmith wrote:
> nit: Could you add a comment here about thread access?  This class is accessed
> on multiple threads, and a couple of member functions are protected by locks,
> and at least one (db_) is protected by serialization (though less so than it
> used to be) and I'd feel easier if there was a comment somewhere that
summarized
> all of that.

I hope my new comment seems an improvement to you. db_ should no longer be
accessed off the DB thread (except a DCHECK in Load() on db_ == NULL).

There is an implementation comment on lock_ regarding the members it protects.

http://codereview.chromium.org/7833042/diff/14009/net/base/cookie_monster.cc
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/7833042/diff/14009/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:578: : public
base::RefCountedThreadSafe<CookieMonsterTask> {
On 2011/09/07 19:04:07, rdsmith wrote:
> I feel as if RefCountedThreadSafe<> is what you want to use when there isn't a
> clear owner of an object.  When there is a clear owner (even if that owner
> changes over the lifetime of the object) you want scoped_ptr or just
> transferring the pointer and deleting it when you're done.  Did you look into
> that option?  (I don't know how easy it is to implement with Bind() and the
new
> callback mechanism, but channeling willchan@, there should be a way to do it.)

See the comment I have added on CookieMonsterTask::InvokeCallback. Anything you
want to bind a callback to should either be ref-counted or otherwise guaranteed
to outlive the callback. In this case, for obscure reasons that deserved an
alert, we have to bind callbacks to CookieMonsterTasks and they need to be
ref-counted to guarantee they are not deleted.

http://codereview.chromium.org/7833042/diff/14009/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:615: if (!callback.is_null()) {
On 2011/09/07 19:04:07, rdsmith wrote:
> nit: Probably easier to read code if you reverse the test and return.

Done.

http://codereview.chromium.org/7833042/diff/14009/net/base/cookie_monster.cc#...
net/base/cookie_monster.cc:666: if (!callback_.is_null()) {
On 2011/09/07 19:04:07, rdsmith wrote:
> This test (and its siblings) seems redundant with the test inside of
> InvokeCallback; can one of them be eliminated?

Done.

http://codereview.chromium.org/7833042/diff/14009/net/base/cookie_monster_per...
File net/base/cookie_monster_perftest.cc (left):

http://codereview.chromium.org/7833042/diff/14009/net/base/cookie_monster_per...
net/base/cookie_monster_perftest.cc:18: class ParsedCookieTest : public
testing::Test { };
On 2011/09/07 19:04:07, rdsmith wrote:
> Why is it ok to get rid of this?  (I'm assuming it is--question is for my own
> education.)

No one actually used it. The TEST definitions do not require a pre-defined
class. TEST_F does.

http://codereview.chromium.org/7833042/diff/14009/net/base/cookie_monster_per...
File net/base/cookie_monster_perftest.cc (right):

http://codereview.chromium.org/7833042/diff/14009/net/base/cookie_monster_per...
net/base/cookie_monster_perftest.cc:58: bool run() { return run_; }
On 2011/09/07 19:04:07, rdsmith wrote:
> nit: const?

Done.

http://codereview.chromium.org/7833042/diff/14009/net/base/cookie_monster_per...
net/base/cookie_monster_perftest.cc:59: void reset() { run_ = false; }
On 2011/09/07 19:04:07, rdsmith wrote:
> nit: The semantic difference between BaseCallback::run() and <Child of
> BaseCallback>::Run() is great enough (state probe vs. take action) that I'd be
> inclined to make the names more different.  Maybe has_run_, has_run() and
> reset_has_run()?

Done.

http://codereview.chromium.org/7833042/diff/14009/net/base/cookie_monster_per...
net/base/cookie_monster_perftest.cc:109:
MessageLoop::current()->RunAllPending();
On 2011/09/07 19:04:07, rdsmith wrote:
> Does this guarantee that we've received the callback?  I guess if everything's
> on the same message loop, it does, because by the time the first task has been
> run the return one will have been posted.  Still, that reasoning probably
> deserves a comment.

Done.

[commit-bot: I haz the power, 2011-09-08 10:44:38]

Presubmit check for 7833042-22001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/browser/renderer_host/render_message_filter.cc

Presubmit checks took 3.4s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[erikwright (departed), 2011-09-08 13:48:52]

Hi John,

Would you mind giving an LGTM for the change in
content/browser/renderer_host/render_message_filter.cc ? We removed some
convenience methods and as a result, this caller needs to pass in the default
options explicitly.

http://codereview.chromium.org/7833042/diff/22001/content/browser/renderer_ho...

-Erik

[jam, 2011-09-08 16:26:53]

content lgtm

[commit-bot: I haz the power, 2011-09-08 18:03:34]

Change committed as 100188