Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7598001:
Remove the old synchronous CookieMonster API. (Closed)
|
Created:
9 years, 4 months ago by ycxiao Modified:
9 years ago CC:
chromium-reviews, cbentzel+watch_chromium.org, kkania, Paweł Hajdan Jr., jam, joi+watch-content_chromium.org, wtc, darin-cc_chromium.org, rkn, estade+watch_chromium.org Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionRemove the old synchronous CookieMonster API. Move the cookie loading form IO thread to DB thread in SQLitePersistentCookieStore. Queue the request of CookieMonster while the loading isn't finished, and invoke the queue task when done.
BUG=68657
TEST=CookieMonsterTest, SQLitePersistentCookieStoreTest
Patch Set 1 : '' #
Total comments: 50
Patch Set 2 : '' #
Total comments: 6
Patch Set 3 : '' #
Total comments: 20
Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 6
Patch Set 7 : '' #
Total comments: 33
Messages
Total messages: 16 (0 generated)
Hi Erik, PTAL. Thanks!
Review of cookie_{store,monster}.{h,cc}
Still reviewing sqlite_persistent_cookie_store.{h,cc}, *test.cc, and the two
modified clients.
http://codereview.chromium.org/7598001/diff/7007/chrome/browser/net/sqlite_pe...
File chrome/browser/net/sqlite_persistent_cookie_store.h (right):
http://codereview.chromium.org/7598001/diff/7007/chrome/browser/net/sqlite_pe...
chrome/browser/net/sqlite_persistent_cookie_store.h:26: virtual
~SQLitePersistentCookieStore();
add OVERRIDE as appropriate.
http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster.cc
File net/base/cookie_monster.cc (right):
http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster.cc#n...
net/base/cookie_monster.cc:575: void InvokeSetCookiesCallbackOnOtherThread(
These functions should not be needed if you define CookieMonsterTask as
described in my review of the .h file.
http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster.cc#n...
net/base/cookie_monster.cc:797: void CookieMonster::DoCookieTask(const
base::Closure& task) {
We discussed offline a more minimal use of locking.
http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster.cc#n...
net/base/cookie_monster.cc:808: void CookieMonster::SetCookieWithDetailsTask(
See comments in .h file regarding CookieMonsterTask.
http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster.cc#n...
net/base/cookie_monster.cc:1343: // Only for CookieMonster unit test usage.
DCHECK(!store_) << "This method is only to be used by unit-tests.";
http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster.cc#n...
net/base/cookie_monster.cc:1412: void CookieMonster::InvokeQueue() {
Refer to offline discussion about minimal lock holding.
http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster.h
File net/base/cookie_monster.h (right):
http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster.h#ne...
net/base/cookie_monster.h:21: #include "base/message_loop_proxy.h"
Why is this added here?
http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster.h#ne...
net/base/cookie_monster.h:145: bool InitializeFrom(const CookieList& list);
Who uses this? Must it be public? If nothing else, the comment is incorrect.
http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster.h#ne...
net/base/cookie_monster.h:248: virtual void SetCookieWithOptionsAsync(const
GURL& url,
Add OVERRIDE as appropriate.
http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster.h#ne...
net/base/cookie_monster.h:280: void OnLoaded(const
std::vector<CanonicalCookie*>& cookies);
Why is this not private?
http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster.h#ne...
net/base/cookie_monster.h:551: class CookieAPIParameters {
In cookie_monster.cc, define a class named CookieMonsterTask.
Define subclasses such as
class GetCookiesWithOptionsTask : public CookieMonsterTask {
public:
GetCookiesWithOptionsTask(CookieMonster* cookie_monster,
const GURL& url,
const CookieOptions& options,
const GetCookiesCallback& callback)
: CookieMonsterTask(cookie_monster),
url_(url),
options_(options),
callback_(callback) {}
virtual void Run() OVERRIDE {
std::string cookies =
this->cookie_monster()->GetCookiesWithOptions(url_, options_);
// InvokeCallback takes a closure and runs it on the thread from which the
// CookieMonsterTask was originally constructed.
this->InvokeCallback(
base::Bind(&GetCookiesCallback::Run, &callback, cookies));
}
};
The subclasses must be friends of CookieMonster.
You can create closures from CookieMonsterTask::Run and a subclass instance and
store those in your queue.
http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster.h#ne...
net/base/cookie_monster.h:576: void DoCookieTask(const base::Closure& task);
All of these task methods go away and become classes defined in the .cc file.
http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster.h#ne...
net/base/cookie_monster.h:660: // Indicate whether the cookies are loaded from
disk.
"Indicates whether loading from the backend store is completed and calls may be
immediately processed."
http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster.h#ne...
net/base/cookie_monster.h:663: // Queue the calls of CookieMonster while the
loadind form the back
"Queues calls to CookieMonster until loading from the backend store is
completed."
http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster.h#ne...
net/base/cookie_monster.h:665: std::deque<base::Closure> queue_;
Can you define this as std::queue<base::Closure> instead? It uses a deque
internally but is slightly clearer about the intended use.
http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_store.h
File net/base/cookie_store.h (right):
http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_store.h#newc...
net/base/cookie_store.h:50: // Using for complete the asyn interface
Why are these added here?
Further comments on patchset 1. http://codereview.chromium.org/7598001/diff/7007/chrome/browser/net/sqlite_pe... File chrome/browser/net/sqlite_persistent_cookie_store.cc (right): http://codereview.chromium.org/7598001/diff/7007/chrome/browser/net/sqlite_pe... chrome/browser/net/sqlite_persistent_cookie_store.cc:169: loaded_callback_ = loaded_callback; DCHECK(loaded_callback_.is_null()); http://codereview.chromium.org/7598001/diff/7007/chrome/browser/net/sqlite_pe... chrome/browser/net/sqlite_persistent_cookie_store.cc:176: void SQLitePersistentCookieStore::Backend::LoadOnDBThread() { Write LoadAndNotifyOnDBThread that does Notify(LoadInternal()); Then you can just return true/false from LoadInternal. http://codereview.chromium.org/7598001/diff/7007/chrome/browser/net/sqlite_pe... chrome/browser/net/sqlite_persistent_cookie_store.cc:177: // Ensure the parent directory for storing cookies is created before reading Comment is now out of date. http://codereview.chromium.org/7598001/diff/7007/chrome/browser/net/sqlite_pe... chrome/browser/net/sqlite_persistent_cookie_store.cc:183: base::ThreadRestrictions::ScopedAllowIO allow_io; ScopedAllowIO, no longer required. Move other code to outer scope. http://codereview.chromium.org/7598001/diff/7007/chrome/browser/net/sqlite_pe... chrome/browser/net/sqlite_persistent_cookie_store.cc:208: db_->Preload(); Lines 184-208 should be a helper method like 'InitializeDatabase' or similar. http://codereview.chromium.org/7598001/diff/7007/chrome/browser/net/sqlite_pe... chrome/browser/net/sqlite_persistent_cookie_store.cc:251: void SQLitePersistentCookieStore::Backend::Notify(bool load_success) { It seems the success value is not used? Remove it? http://codereview.chromium.org/7598001/diff/7007/chrome/browser/net/sqlite_pe... chrome/browser/net/sqlite_persistent_cookie_store.cc:252: if (BrowserThread::CurrentlyOn(BrowserThread::DB)) { Instead of having a thread switch here, you can simply do the PostTask to IO thread in LoadAndNotifyOnDBThread. So rename this to NotifyOnIOThread and remove lines 252-258. http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster_stor... File net/base/cookie_monster_store_test.h (right): http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster_stor... net/base/cookie_monster_store_test.h:54: virtual bool Load(const LoadedCallback& loaded_callback); Use OVERRIDE as appropriate. http://codereview.chromium.org/7598001/diff/7007/net/url_request/url_request_... File net/url_request/url_request_unittest.cc (right): http://codereview.chromium.org/7598001/diff/7007/net/url_request/url_request_... net/url_request/url_request_unittest.cc:2014: MessageLoop::current()->RunAllPending(); Use a 'was_run' boolean to verify that the callback was invoked.
Further reviews based on patchset 2. http://codereview.chromium.org/7598001/diff/9001/net/base/cookie_monster.cc File net/base/cookie_monster.cc (right): http://codereview.chromium.org/7598001/diff/9001/net/base/cookie_monster.cc#n... net/base/cookie_monster.cc:621: GURL* gurl = new GURL(url); Who deletes this? In general, 'new T' should never appear outside of a call to scoped_ptr<T>::reset or scoped_ptr<T>::scoped_ptr(T). When you are done with the refactoring previously described, you should be able to have a member GURL instead of a member GURL* or scoped_ptr<GURL>. http://codereview.chromium.org/7598001/diff/9001/net/base/cookie_monster.cc#n... net/base/cookie_monster.cc:652: GURL* gurl = new GURL(url); ditto here and below... http://codereview.chromium.org/7598001/diff/9001/net/base/cookie_monster.cc#n... net/base/cookie_monster.cc:805: if (loaded_) { use the local variable!
http://codereview.chromium.org/7598001/diff/7007/chrome/browser/net/sqlite_pe... File chrome/browser/net/sqlite_persistent_cookie_store.cc (right): http://codereview.chromium.org/7598001/diff/7007/chrome/browser/net/sqlite_pe... chrome/browser/net/sqlite_persistent_cookie_store.cc:169: loaded_callback_ = loaded_callback; On 2011/08/11 15:13:52, erikwright wrote: > DCHECK(loaded_callback_.is_null()); Done. http://codereview.chromium.org/7598001/diff/7007/chrome/browser/net/sqlite_pe... chrome/browser/net/sqlite_persistent_cookie_store.cc:176: void SQLitePersistentCookieStore::Backend::LoadOnDBThread() { On 2011/08/11 15:13:52, erikwright wrote: > Write LoadAndNotifyOnDBThread that does Notify(LoadInternal()); > > Then you can just return true/false from LoadInternal. Done. http://codereview.chromium.org/7598001/diff/7007/chrome/browser/net/sqlite_pe... chrome/browser/net/sqlite_persistent_cookie_store.cc:177: // Ensure the parent directory for storing cookies is created before reading On 2011/08/11 15:13:52, erikwright wrote: > Comment is now out of date. Done. http://codereview.chromium.org/7598001/diff/7007/chrome/browser/net/sqlite_pe... chrome/browser/net/sqlite_persistent_cookie_store.cc:183: base::ThreadRestrictions::ScopedAllowIO allow_io; On 2011/08/11 15:13:52, erikwright wrote: > ScopedAllowIO, no longer required. Move other code to outer scope. Done. http://codereview.chromium.org/7598001/diff/7007/chrome/browser/net/sqlite_pe... chrome/browser/net/sqlite_persistent_cookie_store.cc:208: db_->Preload(); On 2011/08/11 15:13:52, erikwright wrote: > Lines 184-208 should be a helper method like 'InitializeDatabase' or similar. Done. http://codereview.chromium.org/7598001/diff/7007/chrome/browser/net/sqlite_pe... chrome/browser/net/sqlite_persistent_cookie_store.cc:251: void SQLitePersistentCookieStore::Backend::Notify(bool load_success) { On 2011/08/11 15:13:52, erikwright wrote: > It seems the success value is not used? Remove it? I also notice that it is not used. But this flag passes useful information which maybe wanted in the future. So I am not sure how to deal with it. http://codereview.chromium.org/7598001/diff/7007/chrome/browser/net/sqlite_pe... chrome/browser/net/sqlite_persistent_cookie_store.cc:252: if (BrowserThread::CurrentlyOn(BrowserThread::DB)) { On 2011/08/11 15:13:52, erikwright wrote: > Instead of having a thread switch here, you can simply do the PostTask to IO > thread in LoadAndNotifyOnDBThread. > > So rename this to NotifyOnIOThread and remove lines 252-258. Done. http://codereview.chromium.org/7598001/diff/7007/chrome/browser/net/sqlite_pe... File chrome/browser/net/sqlite_persistent_cookie_store.h (right): http://codereview.chromium.org/7598001/diff/7007/chrome/browser/net/sqlite_pe... chrome/browser/net/sqlite_persistent_cookie_store.h:26: virtual ~SQLitePersistentCookieStore(); On 2011/08/11 13:58:24, erikwright wrote: > add OVERRIDE as appropriate. Done. http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster.cc File net/base/cookie_monster.cc (right): http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster.cc#n... net/base/cookie_monster.cc:575: void InvokeSetCookiesCallbackOnOtherThread( On 2011/08/11 13:58:24, erikwright wrote: > These functions should not be needed if you define CookieMonsterTask as > described in my review of the .h file. Since callback is not reference counted thread safe, if I use PostTask(base::Bind(&***Task, base::Unretained(callback_))), the callback_ may be deleted before execution. I need these functions to post the passing callback to the caller thread. http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster.cc#n... net/base/cookie_monster.cc:797: void CookieMonster::DoCookieTask(const base::Closure& task) { On 2011/08/11 13:58:24, erikwright wrote: > We discussed offline a more minimal use of locking. Done. http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster.cc#n... net/base/cookie_monster.cc:808: void CookieMonster::SetCookieWithDetailsTask( On 2011/08/11 13:58:24, erikwright wrote: > See comments in .h file regarding CookieMonsterTask. Done. http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster.cc#n... net/base/cookie_monster.cc:1343: // Only for CookieMonster unit test usage. On 2011/08/11 13:58:24, erikwright wrote: > DCHECK(!store_) << "This method is only to be used by unit-tests."; Done. http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster.cc#n... net/base/cookie_monster.cc:1412: void CookieMonster::InvokeQueue() { On 2011/08/11 13:58:24, erikwright wrote: > Refer to offline discussion about minimal lock holding. Done. http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster.h File net/base/cookie_monster.h (right): http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster.h#ne... net/base/cookie_monster.h:21: #include "base/message_loop_proxy.h" On 2011/08/11 13:58:24, erikwright wrote: > Why is this added here? I think I can move this to .cc, and added an "class base::MessageLoopProxy" declaration. http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster.h#ne... net/base/cookie_monster.h:145: bool InitializeFrom(const CookieList& list); On 2011/08/11 13:58:24, erikwright wrote: > Who uses this? Must it be public? If nothing else, the comment is incorrect. The login_uitls.cc create this function. The previous version of this method take a cookie_monster as a parameter, and initialize this form the passing cookie monster. Since I want keep this method synchronous, I change the signature to the CookieList. http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster.h#ne... net/base/cookie_monster.h:248: virtual void SetCookieWithOptionsAsync(const GURL& url, On 2011/08/11 13:58:24, erikwright wrote: > Add OVERRIDE as appropriate. Done. http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster.h#ne... net/base/cookie_monster.h:280: void OnLoaded(const std::vector<CanonicalCookie*>& cookies); On 2011/08/11 13:58:24, erikwright wrote: > Why is this not private? You are right, it should be. http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster.h#ne... net/base/cookie_monster.h:551: class CookieAPIParameters { On 2011/08/11 13:58:24, erikwright wrote: > In cookie_monster.cc, define a class named CookieMonsterTask. > > Define subclasses such as > > class GetCookiesWithOptionsTask : public CookieMonsterTask { > public: > GetCookiesWithOptionsTask(CookieMonster* cookie_monster, > const GURL& url, > const CookieOptions& options, > const GetCookiesCallback& callback) > : CookieMonsterTask(cookie_monster), > url_(url), > options_(options), > callback_(callback) {} > > virtual void Run() OVERRIDE { > std::string cookies = > this->cookie_monster()->GetCookiesWithOptions(url_, options_); > // InvokeCallback takes a closure and runs it on the thread from which the > // CookieMonsterTask was originally constructed. > this->InvokeCallback( > base::Bind(&GetCookiesCallback::Run, &callback, cookies)); > } > }; > > The subclasses must be friends of CookieMonster. > > You can create closures from CookieMonsterTask::Run and a subclass instance and > store those in your queue. Done. http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster.h#ne... net/base/cookie_monster.h:576: void DoCookieTask(const base::Closure& task); On 2011/08/11 13:58:24, erikwright wrote: > All of these task methods go away and become classes defined in the .cc file. Done. http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster.h#ne... net/base/cookie_monster.h:660: // Indicate whether the cookies are loaded from disk. On 2011/08/11 13:58:24, erikwright wrote: > "Indicates whether loading from the backend store is completed and calls may be > immediately processed." Done. http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster.h#ne... net/base/cookie_monster.h:663: // Queue the calls of CookieMonster while the loadind form the back On 2011/08/11 13:58:24, erikwright wrote: > "Queues calls to CookieMonster until loading from the backend store is > completed." Done. http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster.h#ne... net/base/cookie_monster.h:665: std::deque<base::Closure> queue_; On 2011/08/11 13:58:24, erikwright wrote: > Can you define this as std::queue<base::Closure> instead? It uses a deque > internally but is slightly clearer about the intended use. Done. http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster_stor... File net/base/cookie_monster_store_test.h (right): http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_monster_stor... net/base/cookie_monster_store_test.h:54: virtual bool Load(const LoadedCallback& loaded_callback); On 2011/08/11 15:13:52, erikwright wrote: > Use OVERRIDE as appropriate. Done. http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_store.h File net/base/cookie_store.h (right): http://codereview.chromium.org/7598001/diff/7007/net/base/cookie_store.h#newc... net/base/cookie_store.h:50: // Using for complete the asyn interface On 2011/08/11 13:58:24, erikwright wrote: > Why are these added here? I just move these up, it was needed for the declaration of the member functions. http://codereview.chromium.org/7598001/diff/7007/net/url_request/url_request_... File net/url_request/url_request_unittest.cc (right): http://codereview.chromium.org/7598001/diff/7007/net/url_request/url_request_... net/url_request/url_request_unittest.cc:2014: MessageLoop::current()->RunAllPending(); On 2011/08/11 15:13:52, erikwright wrote: > Use a 'was_run' boolean to verify that the callback was invoked. Done. http://codereview.chromium.org/7598001/diff/9001/net/base/cookie_monster.cc File net/base/cookie_monster.cc (right): http://codereview.chromium.org/7598001/diff/9001/net/base/cookie_monster.cc#n... net/base/cookie_monster.cc:621: GURL* gurl = new GURL(url); On 2011/08/11 15:30:42, erikwright wrote: > Who deletes this? > > In general, 'new T' should never appear outside of a call to > scoped_ptr<T>::reset or scoped_ptr<T>::scoped_ptr(T). > > When you are done with the refactoring previously described, you should be able > to have a member GURL instead of a member GURL* or scoped_ptr<GURL>. Done. http://codereview.chromium.org/7598001/diff/9001/net/base/cookie_monster.cc#n... net/base/cookie_monster.cc:652: GURL* gurl = new GURL(url); On 2011/08/11 15:30:42, erikwright wrote: > ditto here and below... Done. http://codereview.chromium.org/7598001/diff/9001/net/base/cookie_monster.cc#n... net/base/cookie_monster.cc:805: if (loaded_) { On 2011/08/11 15:30:42, erikwright wrote: > use the local variable! Done.
Looking better. Refer also to our offline discussion about removing the need for task functions in cookie_monster.cc http://codereview.chromium.org/7598001/diff/19001/net/base/cookie_monster.cc File net/base/cookie_monster.cc (right): http://codereview.chromium.org/7598001/diff/19001/net/base/cookie_monster.cc#... net/base/cookie_monster.cc:621: void InvokeCallback(base::Closure callback); InvokeCallback and cookie_monster -> protected http://codereview.chromium.org/7598001/diff/19001/net/base/cookie_monster.cc#... net/base/cookie_monster.cc:627: CookieMonsterTask(); Add constructor param for cookie_monster_, initialize in initializer list. http://codereview.chromium.org/7598001/diff/19001/net/base/cookie_monster.cc#... net/base/cookie_monster.cc:630: CookieMonster* cookie_monster_; cookie_monster_, thread_ -> private http://codereview.chromium.org/7598001/diff/19001/net/base/cookie_monster.cc#... net/base/cookie_monster.cc:663: virtual ~SetCookieWithDetailsTask() { } protected or no declaration. (For each subclass.) http://codereview.chromium.org/7598001/diff/19001/net/base/cookie_monster.cc#... net/base/cookie_monster.cc:681: SetCookieWithDetailsTask::SetCookieWithDetailsTask( I think it's reasonable to inline the constructor definitions. It will save a substantial number of lines of boilerplate code. http://codereview.chromium.org/7598001/diff/19001/net/base/cookie_monster.cc#... net/base/cookie_monster.cc:799: bool sync_to_store, With BrowsingDataCookieHelper, do we still need sync_to_store? Does anyone still use 'false'? http://codereview.chromium.org/7598001/diff/19001/net/base/cookie_monster.cc#... net/base/cookie_monster.cc:1209: DCHECK(!task.is_null()); Remove the DCHECK, and perhaps move the bind to line 1220. On line 1217 you can simply call Run (save one bind in most cases). http://codereview.chromium.org/7598001/diff/19001/net/base/cookie_monster.h File net/base/cookie_monster.h (right): http://codereview.chromium.org/7598001/diff/19001/net/base/cookie_monster.h#n... net/base/cookie_monster.h:287: // For queueing the cookie monater calls. monater -> monster http://codereview.chromium.org/7598001/diff/19001/net/base/cookie_monster.h#n... net/base/cookie_monster.h:570: class CookieAPIParameters { Presumably you can now remove CookieAPIParameters? http://codereview.chromium.org/7598001/diff/19001/net/base/cookie_store.h File net/base/cookie_store.h (right): http://codereview.chromium.org/7598001/diff/19001/net/base/cookie_store.h#new... net/base/cookie_store.h:50: // Using for complete the asyn interface "// Callback definitions"
The CookieMonster unittest use DeleteAllAsync(false). Also the uinttest is the only user of DeleteAllAsync. Done with others. http://codereview.chromium.org/7598001/diff/19001/net/base/cookie_monster.cc File net/base/cookie_monster.cc (right): http://codereview.chromium.org/7598001/diff/19001/net/base/cookie_monster.cc#... net/base/cookie_monster.cc:621: void InvokeCallback(base::Closure callback); On 2011/08/12 16:00:34, erikwright wrote: > InvokeCallback and cookie_monster -> protected Done. http://codereview.chromium.org/7598001/diff/19001/net/base/cookie_monster.cc#... net/base/cookie_monster.cc:627: CookieMonsterTask(); On 2011/08/12 16:00:34, erikwright wrote: > Add constructor param for cookie_monster_, initialize in initializer list. Done. http://codereview.chromium.org/7598001/diff/19001/net/base/cookie_monster.cc#... net/base/cookie_monster.cc:630: CookieMonster* cookie_monster_; On 2011/08/12 16:00:34, erikwright wrote: > cookie_monster_, thread_ -> private Done. http://codereview.chromium.org/7598001/diff/19001/net/base/cookie_monster.cc#... net/base/cookie_monster.cc:663: virtual ~SetCookieWithDetailsTask() { } On 2011/08/12 16:00:34, erikwright wrote: > protected or no declaration. > > (For each subclass.) Done. http://codereview.chromium.org/7598001/diff/19001/net/base/cookie_monster.cc#... net/base/cookie_monster.cc:681: SetCookieWithDetailsTask::SetCookieWithDetailsTask( On 2011/08/12 16:00:34, erikwright wrote: > I think it's reasonable to inline the constructor definitions. It will save a > substantial number of lines of boilerplate code. Done. http://codereview.chromium.org/7598001/diff/19001/net/base/cookie_monster.cc#... net/base/cookie_monster.cc:799: bool sync_to_store, On 2011/08/12 16:00:34, erikwright wrote: > With BrowsingDataCookieHelper, do we still need sync_to_store? Does anyone still > use 'false'? The CookieMonster Unittest use DeleteAllAsync(false). I think the uinttest is the only of DeleteAllAsync. http://codereview.chromium.org/7598001/diff/19001/net/base/cookie_monster.cc#... net/base/cookie_monster.cc:1209: DCHECK(!task.is_null()); On 2011/08/12 16:00:34, erikwright wrote: > Remove the DCHECK, and perhaps move the bind to line 1220. > > On line 1217 you can simply call Run (save one bind in most cases). Done. http://codereview.chromium.org/7598001/diff/19001/net/base/cookie_monster.h File net/base/cookie_monster.h (right): http://codereview.chromium.org/7598001/diff/19001/net/base/cookie_monster.h#n... net/base/cookie_monster.h:287: // For queueing the cookie monater calls. On 2011/08/12 16:00:34, erikwright wrote: > monater -> monster Done. http://codereview.chromium.org/7598001/diff/19001/net/base/cookie_monster.h#n... net/base/cookie_monster.h:570: class CookieAPIParameters { On 2011/08/12 16:00:34, erikwright wrote: > Presumably you can now remove CookieAPIParameters? Done. http://codereview.chromium.org/7598001/diff/19001/net/base/cookie_store.h File net/base/cookie_store.h (right): http://codereview.chromium.org/7598001/diff/19001/net/base/cookie_store.h#new... net/base/cookie_store.h:50: // Using for complete the asyn interface On 2011/08/12 16:00:34, erikwright wrote: > "// Callback definitions" Done.
It would be good to remove the unnecessary 'sync_to_store' parameter wherever it occurs. Also, it will be important to have tests for the queuing behaviour: 1. For each method, there should be a test that verifies that it waits until the database is loaded before executing. 2. There must be a test to verify that a series of queued calls executes in order. 3. There must be a test to verify that new calls occurring during the invocation of queued calls are deferred until after all the originally queued calls. http://codereview.chromium.org/7598001/diff/20035/net/base/cookie_monster.cc File net/base/cookie_monster.cc (right): http://codereview.chromium.org/7598001/diff/20035/net/base/cookie_monster.cc#... net/base/cookie_monster.cc:579: virtual void Run() = 0; // Runs the task and invokes the client callback on the thread that originally constructed the task. http://codereview.chromium.org/7598001/diff/20035/net/base/cookie_monster.cc#... net/base/cookie_monster.cc:585: void InvokeCallback(base::Closure callback); // Invokes the callback immediately, if the current thread is the one that originated the task, or queues the callback for execution on the appropriate thread. Maintains a reference to this CookieMonsterTask instance until the callback completes. http://codereview.chromium.org/7598001/diff/20035/net/base/cookie_monster.cc#... net/base/cookie_monster.cc:1094: base::Closure task = base::Bind(&CookieMonsterTask::Run, task_item.get()); If possible, do the base::Bind directly in the parameter list to push(), without a temporary variable.
http://codereview.chromium.org/7598001/diff/20035/net/base/cookie_monster.cc File net/base/cookie_monster.cc (right): http://codereview.chromium.org/7598001/diff/20035/net/base/cookie_monster.cc#... net/base/cookie_monster.cc:579: virtual void Run() = 0; On 2011/08/12 18:18:42, erikwright wrote: > // Runs the task and invokes the client callback on the thread that originally > constructed the task. Done. http://codereview.chromium.org/7598001/diff/20035/net/base/cookie_monster.cc#... net/base/cookie_monster.cc:585: void InvokeCallback(base::Closure callback); On 2011/08/12 18:18:42, erikwright wrote: > // Invokes the callback immediately, if the current thread is the one that > originated the task, or queues the callback for execution on the appropriate > thread. Maintains a reference to this CookieMonsterTask instance until the > callback completes. Done. http://codereview.chromium.org/7598001/diff/20035/net/base/cookie_monster.cc#... net/base/cookie_monster.cc:1094: base::Closure task = base::Bind(&CookieMonsterTask::Run, task_item.get()); On 2011/08/12 18:18:42, erikwright wrote: > If possible, do the base::Bind directly in the parameter list to push(), without > a temporary variable. Done.
Hi Randy, Please take a first look at this. It requires a few new tests and a test fix, but your OK on the approach would be appreciated sooner. Cheers, Erik http://codereview.chromium.org/7598001/diff/26008/chrome/browser/fast_shutdow... File chrome/browser/fast_shutdown_interactive_uitest.cc (right): http://codereview.chromium.org/7598001/diff/26008/chrome/browser/fast_shutdow... chrome/browser/fast_shutdown_interactive_uitest.cc:46: This test fails in a racy way due to a failure to lock the DB. I suspect that it is because the DB cannot be opened by the test process while the browser process is accessing it. I suspect that it previously worked because the cookie loading blocked the IO thread, which blocked getting back an initial ACK over IPC until after the cookies were loaded. The solution is to have finer grain control of the browser launching, and to test the cookies only before launching the browser or after shutting it down. http://codereview.chromium.org/7598001/diff/26008/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/new_tab_page_sync_handler.cc (right): http://codereview.chromium.org/7598001/diff/26008/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/new_tab_page_sync_handler.cc:35: // struct GoogleCookieFilter { I guess we will remove this since it is apparently not used.
Erik: I'm going to go through and do a more detailed pass now, but I wanted to toss in one high level comment first: Do you have a before-and-after perftest run for this change? We're adding a fair amount more mechanism on the critical paths, and we should be tracking if/how much latency that adds (once the cookies have been loaded). If it's high enough, we might want to consider testing the loaded boolean and only constructing the tasks in the not-loaded-yet case. They joys of being on the "every web request" pathway :-} :-J. (At some point you should read through the perftests and decide if you think they did an ok job of probing for performance on the key performance paths, but that's part of the longer job of claiming ownership, not necessarily part of this CL.) http://codereview.chromium.org/7598001/diff/26008/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/new_tab_page_sync_handler.cc (right): http://codereview.chromium.org/7598001/diff/26008/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/new_tab_page_sync_handler.cc:35: // struct GoogleCookieFilter { On 2011/08/19 18:13:05, erikwright wrote: > I guess we will remove this since it is apparently not used. Have you checked with an OWNER of (or someone who's committed a fair amount of changes to) this file? idana@ or tim@ seem like good people (presuming they're still around). http://codereview.chromium.org/7598001/diff/26008/net/base/cookie_monster.cc File net/base/cookie_monster.cc (right): http://codereview.chromium.org/7598001/diff/26008/net/base/cookie_monster.cc#... net/base/cookie_monster.cc:1230: InternalDeleteCookie(curit, true, /*sync_to_store*/ nit: It isn't completely clear to me what the sync to store comment refers. One arg a line, or maybe put it before the comma? (Not sure if that's ok by the style guide).
More detailed notes. http://codereview.chromium.org/7598001/diff/26008/chrome/browser/fast_shutdow... File chrome/browser/fast_shutdown_interactive_uitest.cc (right): http://codereview.chromium.org/7598001/diff/26008/chrome/browser/fast_shutdow... chrome/browser/fast_shutdown_interactive_uitest.cc:46: On 2011/08/19 18:13:05, erikwright wrote: > This test fails in a racy way due to a failure to lock the DB. I suspect that it > is because the DB cannot be opened by the test process while the browser process > is accessing it. > > I suspect that it previously worked because the cookie loading blocked the IO > thread, which blocked getting back an initial ACK over IPC until after the > cookies were loaded. > > The solution is to have finer grain control of the browser launching, and to > test the cookies only before launching the browser or after shutting it down. Sounds good (I presume this is the test fix you mentioned). http://codereview.chromium.org/7598001/diff/26008/chrome/browser/net/sqlite_p... File chrome/browser/net/sqlite_persistent_cookie_store.cc (right): http://codereview.chromium.org/7598001/diff/26008/chrome/browser/net/sqlite_p... chrome/browser/net/sqlite_persistent_cookie_store.cc:117: LoadedCallback loaded_callback_; I'm not familiar with the new callback interface, but could these be passed via function call across the threads rather than in the object? I ask for two reasons: a) they're only used for one action by the store, b) as currently implemented, these values are accessed on multiple threads, and while it's safe because that access is strictly sequential, it's a bit of a violation of the principle of least astonishment. The obvious problem is leaks if the object is destroyed at the wrong time; do we have enough control over object lifetime to avoid that? Alternatively, we could pass around a scoped_ptr to an object that contains the vector. http://codereview.chromium.org/7598001/diff/26008/net/base/cookie_monster.cc File net/base/cookie_monster.cc (right): http://codereview.chromium.org/7598001/diff/26008/net/base/cookie_monster.cc#... net/base/cookie_monster.cc:576: class CookieMonsterTask I'm inclined to think that all of these classes should be nested inside the CookieMonster after private:; they're only used for the CM implementation. http://codereview.chromium.org/7598001/diff/26008/net/base/cookie_monster.cc#... net/base/cookie_monster.cc:1094: queue_.push(base::Bind(&CookieMonsterTask::Run, task_item.get())); A possible performance optimization would be to do the push inside the first lock block, to avoid taking the lock twice. http://codereview.chromium.org/7598001/diff/26008/net/base/cookie_monster.cc#... net/base/cookie_monster.cc:1094: queue_.push(base::Bind(&CookieMonsterTask::Run, task_item.get())); It probably doesn't matter either way, but any reason not to push the task_item rather than the task item bound to a method? The method to be called at dequeue time is not in doubt and it'd make the queue a bit simpler data structure. http://codereview.chromium.org/7598001/diff/26008/net/base/cookie_monster.cc#... net/base/cookie_monster.cc:1442: void CookieMonster::OnLoaded(const std::vector<CanonicalCookie*>& cookies) { This is a pretty small function; I'd be inclined to combine it with the one after. http://codereview.chromium.org/7598001/diff/26008/net/base/cookie_monster.h File net/base/cookie_monster.h (right): http://codereview.chromium.org/7598001/diff/26008/net/base/cookie_monster.h#n... net/base/cookie_monster.h:31: class MessageLoopProxy; Not used in this file--maybe it should be in cookie_monster.cc? http://codereview.chromium.org/7598001/diff/26008/net/base/cookie_monster.h#n... net/base/cookie_monster.h:296: friend class SetCookieWithOptionsTask; I'm feeling torn about this long list of friend classes, and the creation of all of the different tasks. It's both uglier and heresy, but I'm playing around with the idea of a case statement instead (i.e. have a single object that can be used for all the different operations that has the set union (literally; I'm not suggesting a C union) of all the information of all of these objects plus an enum to indicate which operation a particular object refers to, do a single "friend class CookieMonster" inside that class, and then pass objects of that class into a CookieMonster method (which is where the case statement will be) for execution. I think that results in a lot less code, many fewer friends of the CookieMonster (the CookieMonster should have no friends--more cookies for it!! :-}); it's not good object oriented polymorphism, but I think it would still be pretty clear. I won't insist on the idea, but I wanted to float it by you in case it appealed to you over this approach. http://codereview.chromium.org/7598001/diff/26008/net/base/cookie_monster.h#n... net/base/cookie_monster.h:386: bool SetCookieWithDetails(const GURL& url, Maybe a comment before these indicating how they're used and what the constraints are (I'm presuming by the async callbacks, only valid to call once the db is loaded). http://codereview.chromium.org/7598001/diff/26008/net/base/cookie_monster.h#n... net/base/cookie_monster.h:399: CookieList GetAllCookiesForURL(const GURL& url); nit: If this is now only a helper call called from a single place, you can remove it and force callers to use the more detailed version. http://codereview.chromium.org/7598001/diff/26008/net/base/cookie_monster.h#n... net/base/cookie_monster.h:447: // Reads the existing cookies loaded form the backing store. I'd suggest instead "Called when loading of the existing cookies on the backing store is complete." http://codereview.chromium.org/7598001/diff/26008/net/base/cookie_monster.h#n... net/base/cookie_monster.h:453: // Invoke the queueing calls of CookieMonster when loading cookies from nit: "queueing" -> "queued".
On 2011/08/19 20:54:08, rdsmith wrote: > Erik: I'm going to go through and do a more detailed pass now, but I wanted to > toss in one high level comment first: Do you have a before-and-after perftest > run for this change? We're adding a fair amount more mechanism on the critical > paths, and we should be tracking if/how much latency that adds (once the cookies > have been loaded). If it's high enough, we might want to consider testing the > loaded boolean and only constructing the tasks in the not-loaded-yet case. They > joys of being on the "every web request" pathway :-} :-J. > > (At some point you should read through the perftests and decide if you think > they did an ok job of probing for performance on the key performance paths, but > that's part of the longer job of claiming ownership, not necessarily part of > this CL.) Hi Randy, I have run the existing perf-tests, after updating them to use the new API. The bad news is that several of the tests were broken. The test cookie was 'secure' but the URL was HTTP, so they were testing a very short path. I haven't had a chance to go back in time and run a fixed version of the tests on the old code. The good news is that several tests were OK, and the overhead we add doesn't vary across tests, so we can extrapolate pretty comfortably: Cookie_monster_add_single_host: 1% increase Cookie_monster_query_single_host: 1.9% decrease This is over a wide range of revisions, so it's difficult to directly link this to the API changes, but I think we can safely say that the overhead of the callback mechanisms is minimal to none.
On 2011/08/30 15:30:34, erikwright wrote: > On 2011/08/19 20:54:08, rdsmith wrote: > > Erik: I'm going to go through and do a more detailed pass now, but I wanted to > > toss in one high level comment first: Do you have a before-and-after perftest > > run for this change? We're adding a fair amount more mechanism on the > critical > > paths, and we should be tracking if/how much latency that adds (once the > cookies > > have been loaded). If it's high enough, we might want to consider testing the > > loaded boolean and only constructing the tasks in the not-loaded-yet case. > They > > joys of being on the "every web request" pathway :-} :-J. > > > > (At some point you should read through the perftests and decide if you think > > they did an ok job of probing for performance on the key performance paths, > but > > that's part of the longer job of claiming ownership, not necessarily part of > > this CL.) > > Hi Randy, > > I have run the existing perf-tests, after updating them to use the new API. The > bad news is that several of the tests were broken. The test cookie was 'secure' > but the URL was HTTP, so they were testing a very short path. I haven't had a > chance to go back in time and run a fixed version of the tests on the old code. > > The good news is that several tests were OK, and the overhead we add doesn't > vary across tests, so we can extrapolate pretty comfortably: > > Cookie_monster_add_single_host: 1% increase > Cookie_monster_query_single_host: 1.9% decrease > > This is over a wide range of revisions, so it's difficult to directly link this > to the API changes, but I think we can safely say that the overhead of the > callback mechanisms is minimal to none. Sounds good; thanks for investigating.
Hi Randy, My responses are here, but the CL continues here: http://codereview.chromium.org/7833042/ Cheers, Erik http://codereview.chromium.org/7598001/diff/26008/chrome/browser/fast_shutdow... File chrome/browser/fast_shutdown_interactive_uitest.cc (right): http://codereview.chromium.org/7598001/diff/26008/chrome/browser/fast_shutdow... chrome/browser/fast_shutdown_interactive_uitest.cc:46: On 2011/08/19 21:40:05, rdsmith wrote: > On 2011/08/19 18:13:05, erikwright wrote: > > This test fails in a racy way due to a failure to lock the DB. I suspect that > it > > is because the DB cannot be opened by the test process while the browser > process > > is accessing it. > > > > I suspect that it previously worked because the cookie loading blocked the IO > > thread, which blocked getting back an initial ACK over IPC until after the > > cookies were loaded. > > > > The solution is to have finer grain control of the browser launching, and to > > test the cookies only before launching the browser or after shutting it down. > > Sounds good (I presume this is the test fix you mentioned). As it turns out, the test could be simplified to not check the cookie store on startup because the automation instance that it starts up uses a blank profile by default. http://codereview.chromium.org/7598001/diff/26008/chrome/browser/net/sqlite_p... File chrome/browser/net/sqlite_persistent_cookie_store.cc (right): http://codereview.chromium.org/7598001/diff/26008/chrome/browser/net/sqlite_p... chrome/browser/net/sqlite_persistent_cookie_store.cc:117: LoadedCallback loaded_callback_; On 2011/08/19 21:40:05, rdsmith wrote: > I'm not familiar with the new callback interface, but could these be passed via > function call across the threads rather than in the object? I ask for two > reasons: a) they're only used for one action by the store, b) as currently > implemented, these values are accessed on multiple threads, and while it's safe > because that access is strictly sequential, it's a bit of a violation of the > principle of least astonishment. > > The obvious problem is leaks if the object is destroyed at the wrong time; do we > have enough control over object lifetime to avoid that? Alternatively, we could > pass around a scoped_ptr to an object that contains the vector. Done. http://codereview.chromium.org/7598001/diff/26008/chrome/browser/ui/webui/ntp... File chrome/browser/ui/webui/ntp/new_tab_page_sync_handler.cc (right): http://codereview.chromium.org/7598001/diff/26008/chrome/browser/ui/webui/ntp... chrome/browser/ui/webui/ntp/new_tab_page_sync_handler.cc:35: // struct GoogleCookieFilter { On 2011/08/19 20:54:08, rdsmith wrote: > On 2011/08/19 18:13:05, erikwright wrote: > > I guess we will remove this since it is apparently not used. > > Have you checked with an OWNER of (or someone who's committed a fair amount of > changes to) this file? idana@ or tim@ seem like good people (presuming they're > still around). Added as a reviewer on the new CL. http://codereview.chromium.org/7598001/diff/26008/net/base/cookie_monster.cc File net/base/cookie_monster.cc (right): http://codereview.chromium.org/7598001/diff/26008/net/base/cookie_monster.cc#... net/base/cookie_monster.cc:576: class CookieMonsterTask On 2011/08/19 21:40:05, rdsmith wrote: > I'm inclined to think that all of these classes should be nested inside the > CookieMonster after private:; they're only used for the CM implementation. Done. http://codereview.chromium.org/7598001/diff/26008/net/base/cookie_monster.cc#... net/base/cookie_monster.cc:1094: queue_.push(base::Bind(&CookieMonsterTask::Run, task_item.get())); On 2011/08/19 21:40:05, rdsmith wrote: > It probably doesn't matter either way, but any reason not to push the task_item > rather than the task item bound to a method? The method to be called at dequeue > time is not in doubt and it'd make the queue a bit simpler data structure. Done. http://codereview.chromium.org/7598001/diff/26008/net/base/cookie_monster.cc#... net/base/cookie_monster.cc:1094: queue_.push(base::Bind(&CookieMonsterTask::Run, task_item.get())); On 2011/08/19 21:40:05, rdsmith wrote: > A possible performance optimization would be to do the push inside the first > lock block, to avoid taking the lock twice. Done. http://codereview.chromium.org/7598001/diff/26008/net/base/cookie_monster.cc#... net/base/cookie_monster.cc:1230: InternalDeleteCookie(curit, true, /*sync_to_store*/ On 2011/08/19 20:54:08, rdsmith wrote: > nit: It isn't completely clear to me what the sync to store comment refers. One > arg a line, or maybe put it before the comma? (Not sure if that's ok by the > style guide). After looking through this, it's not much work to get rid of sync_to_store now (only not true in the destructor) and it will tidy up a bunch of stuff. I'll do that in a follow-up commit. http://codereview.chromium.org/7598001/diff/26008/net/base/cookie_monster.cc#... net/base/cookie_monster.cc:1442: void CookieMonster::OnLoaded(const std::vector<CanonicalCookie*>& cookies) { On 2011/08/19 21:40:05, rdsmith wrote: > This is a pretty small function; I'd be inclined to combine it with the one > after. The following function has a function-scoped lock, which must not be held during InvokeQueue. If I combine them I must either: 1) Nest the entire current function in a new scope block. 2) Manually release the lock at the end of the method, before calling InvokeQueue. I find both unpalatable, but will defer to your judgement. http://codereview.chromium.org/7598001/diff/26008/net/base/cookie_monster.h File net/base/cookie_monster.h (right): http://codereview.chromium.org/7598001/diff/26008/net/base/cookie_monster.h#n... net/base/cookie_monster.h:31: class MessageLoopProxy; On 2011/08/19 21:40:05, rdsmith wrote: > Not used in this file--maybe it should be in cookie_monster.cc? Done. http://codereview.chromium.org/7598001/diff/26008/net/base/cookie_monster.h#n... net/base/cookie_monster.h:296: friend class SetCookieWithOptionsTask; On 2011/08/19 21:40:05, rdsmith wrote: > I'm feeling torn about this long list of friend classes, and the creation of all > of the different tasks. It's both uglier and heresy, but I'm playing around > with the idea of a case statement instead (i.e. have a single object that can be > used for all the different operations that has the set union (literally; I'm not > suggesting a C union) of all the information of all of these objects plus an > enum to indicate which operation a particular object refers to, do a single > "friend class CookieMonster" inside that class, and then pass objects of that > class into a CookieMonster method (which is where the case statement will be) > for execution. I think that results in a lot less code, many fewer friends of > the CookieMonster (the CookieMonster should have no friends--more cookies for > it!! :-}); it's not good object oriented polymorphism, but I think it would > still be pretty clear. I won't insist on the idea, but I wanted to float it by > you in case it appealed to you over this approach. The case statement, IME is more error prone in addition to being uglier and heresy :) I would prefer to reduce the number of methods that actually need to be asynchronous by extracting different smaller classes out of this one. This will have the same effect (in fact, probably even a better outcome). http://codereview.chromium.org/7598001/diff/26008/net/base/cookie_monster.h#n... net/base/cookie_monster.h:386: bool SetCookieWithDetails(const GURL& url, On 2011/08/19 21:40:05, rdsmith wrote: > Maybe a comment before these indicating how they're used and what the > constraints are (I'm presuming by the async callbacks, only valid to call once > the db is loaded). Done. http://codereview.chromium.org/7598001/diff/26008/net/base/cookie_monster.h#n... net/base/cookie_monster.h:399: CookieList GetAllCookiesForURL(const GURL& url); It has four callers besides tests. For now I'll leave it I guess. On 2011/08/19 21:40:05, rdsmith wrote: > nit: If this is now only a helper call called from a single place, you can > remove it and force callers to use the more detailed version. http://codereview.chromium.org/7598001/diff/26008/net/base/cookie_monster.h#n... net/base/cookie_monster.h:447: // Reads the existing cookies loaded form the backing store. On 2011/08/19 21:40:05, rdsmith wrote: > I'd suggest instead "Called when loading of the existing cookies on the backing > store is complete." Done. http://codereview.chromium.org/7598001/diff/26008/net/base/cookie_monster.h#n... net/base/cookie_monster.h:453: // Invoke the queueing calls of CookieMonster when loading cookies from On 2011/08/19 21:40:05, rdsmith wrote: > nit: "queueing" -> "queued". Done.
Responding on this CL just to answer your comments; starting full review on the new CL now. http://codereview.chromium.org/7598001/diff/26008/net/base/cookie_monster.cc File net/base/cookie_monster.cc (right): http://codereview.chromium.org/7598001/diff/26008/net/base/cookie_monster.cc#... net/base/cookie_monster.cc:1230: InternalDeleteCookie(curit, true, /*sync_to_store*/ On 2011/09/06 17:34:45, erikwright wrote: > On 2011/08/19 20:54:08, rdsmith wrote: > > nit: It isn't completely clear to me what the sync to store comment refers. > One > > arg a line, or maybe put it before the comma? (Not sure if that's ok by the > > style guide). > > After looking through this, it's not much work to get rid of sync_to_store now > (only not true in the destructor) and it will tidy up a bunch of stuff. I'll do > that in a follow-up commit. Excellent news; thanks! (It's a minor thing, but I've hated sync_to_store for a long time.) http://codereview.chromium.org/7598001/diff/26008/net/base/cookie_monster.cc#... net/base/cookie_monster.cc:1442: void CookieMonster::OnLoaded(const std::vector<CanonicalCookie*>& cookies) { On 2011/09/06 17:34:45, erikwright wrote: > On 2011/08/19 21:40:05, rdsmith wrote: > > This is a pretty small function; I'd be inclined to combine it with the one > > after. > > The following function has a function-scoped lock, which must not be held during > InvokeQueue. If I combine them I must either: > > 1) Nest the entire current function in a new scope block. > 2) Manually release the lock at the end of the method, before calling > InvokeQueue. > > I find both unpalatable, but will defer to your judgement. Got it. No strong feelings; this is fine. http://codereview.chromium.org/7598001/diff/26008/net/base/cookie_monster.h File net/base/cookie_monster.h (right): http://codereview.chromium.org/7598001/diff/26008/net/base/cookie_monster.h#n... net/base/cookie_monster.h:296: friend class SetCookieWithOptionsTask; On 2011/09/06 17:34:45, erikwright wrote: > On 2011/08/19 21:40:05, rdsmith wrote: > > I'm feeling torn about this long list of friend classes, and the creation of > all > > of the different tasks. It's both uglier and heresy, but I'm playing around > > with the idea of a case statement instead (i.e. have a single object that can > be > > used for all the different operations that has the set union (literally; I'm > not > > suggesting a C union) of all the information of all of these objects plus an > > enum to indicate which operation a particular object refers to, do a single > > "friend class CookieMonster" inside that class, and then pass objects of that > > class into a CookieMonster method (which is where the case statement will be) > > for execution. I think that results in a lot less code, many fewer friends of > > the CookieMonster (the CookieMonster should have no friends--more cookies for > > it!! :-}); it's not good object oriented polymorphism, but I think it would > > still be pretty clear. I won't insist on the idea, but I wanted to float it > by > > you in case it appealed to you over this approach. > > The case statement, IME is more error prone in addition to being uglier and > heresy :) > > I would prefer to reduce the number of methods that actually need to be > asynchronous by extracting different smaller classes out of this one. This will > have the same effect (in fact, probably even a better outcome). Ok. |
