|
|
Created:
10 years, 6 months ago by willchan no longer on Chromium Modified:
9 years, 7 months ago CC:
chromium-reviews, Matt Perry Base URL:
http://src.chromium.org/git/chromium.git Visibility:
Public. |
DescriptionMake CookieMonster NonThreadSafe.
Made ExtensionFunction RefCountedThreadSafe so it can be posted to different threads.
Used WaitableEvent in AutomationProvider.
BUG=44083
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=50296
Patch Set 1 #
Total comments: 17
Patch Set 2 : Address comments. #
Total comments: 7
Patch Set 3 : Address cindylau's comments. #
Total comments: 12
Patch Set 4 : Address cindy's comments. #Patch Set 5 : Oops, build fix and address eric's nits. #Patch Set 6 : Use async functions instead. Make ExtensionFunction RefCountedThreadSafe. #Patch Set 7 : Update includes. #
Total comments: 3
Patch Set 8 : Oops, posted to wrong thread. #Patch Set 9 : Address eroman comments. #
Total comments: 22
Patch Set 10 : Address eroman's and cindylau's comments. #Messages
Total messages: 24 (0 generated)
rdsmith: Please review CookieMonster changes and AutomationProvider stuff. jochen: Please review the ExtensionDataDeleter changes cindylau: Please review the CookieFunction changes wtc: Please review cookie_policy_browsertest.cc
shouldn't you also change cookie store then? it's ref counted thread safe.. please also run tryjobs on the valgrind bots. On 2010/06/09 01:22:03, willchan wrote: > rdsmith: Please review CookieMonster changes and AutomationProvider stuff. > jochen: Please review the ExtensionDataDeleter changes > cindylau: Please review the CookieFunction changes > wtc: Please review cookie_policy_browsertest.cc
I like the approach your taking, but have some comments inline. Top level comments: -- Make sure to fix the presubmit checks; you've got some extra whitespace. -- Does anything need to be done with making AutomationCookieStore non-thread-safe, or is that outside the scope of this issue? (It inherits from CookieStore, not CookieMonster, so the answer may be "Nope; outside the scope :-}" http://codereview.chromium.org/2756003/diff/1/2 File chrome/browser/automation/automation_provider.cc (right): http://codereview.chromium.org/2756003/diff/1/2#newcode1174 chrome/browser/automation/automation_provider.cc:1174: class SetCookiesTask : public Task { Shouldn't this be singular (SetCookieTask)? http://codereview.chromium.org/2756003/diff/1/2#newcode1286 chrome/browser/automation/automation_provider.cc:1286: tab->profile()->GetRequestContext())); So I think this is a changed behavior, from synchronous to asynchronous. Why? Are you sure that it's safe given the callers of AutomationProvider? I'd think keeping the behavior synchronous (i.e. patterning DeleteCookie after Set/Get) would be safer. http://codereview.chromium.org/2756003/diff/1/3 File chrome/browser/extensions/extension_cookies_api.cc (right): http://codereview.chromium.org/2756003/diff/1/3#newcode382 chrome/browser/extensions/extension_cookies_api.cc:382: new RemoveCookieTask(url, name, store_profile->GetRequestContext())); Again, a change to asynchronous. Could you reassure me (or future readers, in a comment) as to why this is guaranteed safe? If there's no immediate reason to change to asynchronous, I'd recommend staying synchronous; why risk subtle race conditions when we don't have to? http://codereview.chromium.org/2756003/diff/1/4 File chrome/browser/extensions/extension_cookies_api.h (right): http://codereview.chromium.org/2756003/diff/1/4#newcode36 chrome/browser/extensions/extension_cookies_api.h:36: // that occurs. It can't return false since it doesn't return a boolean. NULL? http://codereview.chromium.org/2756003/diff/1/6 File chrome/browser/extensions/extension_cookies_helpers.h (right): http://codereview.chromium.org/2756003/diff/1/6#newcode48 chrome/browser/extensions/extension_cookies_helpers.h:48: // URL. If the URL is empty, all cookies in the cookie store are retrieved. I'd add a comment here that this function must now be called on the IO thread. An assertion would be even better, but I don't think there's any harm in letting the call fall through to the cookie monster and assert there. http://codereview.chromium.org/2756003/diff/1/7 File chrome/browser/extensions/extension_data_deleter.cc (right): http://codereview.chromium.org/2756003/diff/1/7#newcode47 chrome/browser/extensions/extension_data_deleter.cc:47: cookie_monster->DeleteAllForURL(extension_url_, true); I'm confused. The posts above suggest that we're not on the IO thread; given that, why is it safe to call cookie_monster methods? http://codereview.chromium.org/2756003/diff/1/10 File net/base/cookie_monster.cc (right): http://codereview.chromium.org/2756003/diff/1/10#newcode552 net/base/cookie_monster.cc:552: DCHECK(CalledOnValidThread()); How did you choose which functions to put this into? Public methods which don't have it: DeleteCookie (three argument version), SetCookieWithCreationTime (redundant with SetCookieWithCreationTimeAndOptions but would be consistent with presence in SetCookieWithOptions, DeleteAllCreatedAfter), GetCookieMonster (though that's probably overkill).
On 2010/06/09 05:23:34, jochen wrote: > shouldn't you also change cookie store then? it's ref counted thread safe.. Hm, I'll file a bug for that and do it in a later changelist. > > please also run tryjobs on the valgrind bots. > > On 2010/06/09 01:22:03, willchan wrote: > > rdsmith: Please review CookieMonster changes and AutomationProvider stuff. > > jochen: Please review the ExtensionDataDeleter changes > > cindylau: Please review the CookieFunction changes > > wtc: Please review cookie_policy_browsertest.cc
On 2010/06/09 19:36:50, willchan wrote: > On 2010/06/09 05:23:34, jochen wrote: > > shouldn't you also change cookie store then? it's ref counted thread safe.. > > Hm, I'll file a bug for that and do it in a later changelist. For what it's worth, I like the idea of getting some "soak time" where we'll get DCHECK's if the IO thread restriction is violated in a debug build, but releases are safe because the class is still RefCountedThreadSafe. You may not have intended to remove the RefCountedThreadSafe in the later changelist, but if you did, I'd suggest filing a bug and not fixing it for a little while :-}. > > > > > please also run tryjobs on the valgrind bots. > > > > On 2010/06/09 01:22:03, willchan wrote: > > > rdsmith: Please review CookieMonster changes and AutomationProvider stuff. > > > jochen: Please review the ExtensionDataDeleter changes > > > cindylau: Please review the CookieFunction changes > > > wtc: Please review cookie_policy_browsertest.cc
http://codereview.chromium.org/2756003/diff/1/2 File chrome/browser/automation/automation_provider.cc (right): http://codereview.chromium.org/2756003/diff/1/2#newcode1174 chrome/browser/automation/automation_provider.cc:1174: class SetCookiesTask : public Task { On 2010/06/09 17:54:49, rdsmith wrote: > Shouldn't this be singular (SetCookieTask)? Done. http://codereview.chromium.org/2756003/diff/1/2#newcode1286 chrome/browser/automation/automation_provider.cc:1286: tab->profile()->GetRequestContext())); On 2010/06/09 17:54:49, rdsmith wrote: > So I think this is a changed behavior, from synchronous to asynchronous. Why? > Are you sure that it's safe given the callers of AutomationProvider? I'd think > keeping the behavior synchronous (i.e. patterning DeleteCookie after Set/Get) > would be safer. Here's my justification for why it's safe: Everything is ordered on the IO thread. This is a write operation. Any other read/write operation on the CookieMonster will happen on the IO thread, therefore they are guaranteed to see this. Also, what kind of race condition are you worried about? Everything's being done on the IO thread, so there shouldn't be any corruption of the CookieMonster itself. If you're worried about the UI thread consumer making sure that the delete happens before proceeding with any other activity, I don't know that that's a safe assumption to make, since something else may have modified the CookieMonster in the meantime anyway, so I don't think there are any reasonable guarantees on the UI thread in the first place. Let me know what you think about that justification. http://codereview.chromium.org/2756003/diff/1/3 File chrome/browser/extensions/extension_cookies_api.cc (right): http://codereview.chromium.org/2756003/diff/1/3#newcode382 chrome/browser/extensions/extension_cookies_api.cc:382: new RemoveCookieTask(url, name, store_profile->GetRequestContext())); On 2010/06/09 17:54:49, rdsmith wrote: > Again, a change to asynchronous. Could you reassure me (or future readers, in a > comment) as to why this is guaranteed safe? > > If there's no immediate reason to change to asynchronous, I'd recommend staying > synchronous; why risk subtle race conditions when we don't have to? Not blocking the UI thread is a performance win. Any delay on it will result in all pending UI events in the queue being delayed. The delay would be roughly equal to a single iteration through the IO thread. I think it's safe, due to the justification I provided in automation_provider.cc. Let me know what you think about it. I am going to loop in eroman for his opinion here too. http://codereview.chromium.org/2756003/diff/1/4 File chrome/browser/extensions/extension_cookies_api.h (right): http://codereview.chromium.org/2756003/diff/1/4#newcode36 chrome/browser/extensions/extension_cookies_api.h:36: // that occurs. On 2010/06/09 17:54:49, rdsmith wrote: > It can't return false since it doesn't return a boolean. NULL? > Done. http://codereview.chromium.org/2756003/diff/1/6 File chrome/browser/extensions/extension_cookies_helpers.h (right): http://codereview.chromium.org/2756003/diff/1/6#newcode48 chrome/browser/extensions/extension_cookies_helpers.h:48: // URL. If the URL is empty, all cookies in the cookie store are retrieved. On 2010/06/09 17:54:49, rdsmith wrote: > I'd add a comment here that this function must now be called on the IO thread. > An assertion would be even better, but I don't think there's any harm in letting > the call fall through to the cookie monster and assert there. > Done. http://codereview.chromium.org/2756003/diff/1/7 File chrome/browser/extensions/extension_data_deleter.cc (right): http://codereview.chromium.org/2756003/diff/1/7#newcode47 chrome/browser/extensions/extension_data_deleter.cc:47: cookie_monster->DeleteAllForURL(extension_url_, true); On 2010/06/09 17:54:49, rdsmith wrote: > I'm confused. The posts above suggest that we're not on the IO thread; given > that, why is it safe to call cookie_monster methods? > I think you might be misreading it? StartDeleting() is called on the UI thread. It posts a call to DeleteCookiesOnIOThread() to the IO thread. Note the DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO)); at the beginning of the function which proves we're on the IO thread. http://codereview.chromium.org/2756003/diff/1/10 File net/base/cookie_monster.cc (right): http://codereview.chromium.org/2756003/diff/1/10#newcode552 net/base/cookie_monster.cc:552: DCHECK(CalledOnValidThread()); On 2010/06/09 17:54:49, rdsmith wrote: > How did you choose which functions to put this into? Public methods which don't > have it: DeleteCookie (three argument version), SetCookieWithCreationTime > (redundant with SetCookieWithCreationTimeAndOptions but would be consistent with > presence in SetCookieWithOptions, DeleteAllCreatedAfter), GetCookieMonster > (though that's probably overkill). I was trying to do all public methods. Looks like I missed some. Thanks for pointing them out. I've updated them now.
LGTM http://codereview.chromium.org/2756003/diff/1/2 File chrome/browser/automation/automation_provider.cc (right): http://codereview.chromium.org/2756003/diff/1/2#newcode1286 chrome/browser/automation/automation_provider.cc:1286: tab->profile()->GetRequestContext())); On 2010/06/09 19:45:41, willchan wrote: > On 2010/06/09 17:54:49, rdsmith wrote: > > So I think this is a changed behavior, from synchronous to asynchronous. Why? > > > Are you sure that it's safe given the callers of AutomationProvider? I'd > think > > keeping the behavior synchronous (i.e. patterning DeleteCookie after Set/Get) > > would be safer. > > Here's my justification for why it's safe: > Everything is ordered on the IO thread. This is a write operation. Any other > read/write operation on the CookieMonster will happen on the IO thread, > therefore they are guaranteed to see this. That's valid as long as when the PostTask returns, any future PostTask will be enqueued after it in the IO thread queue. That strikes me as a believable guarantee from PostTask, which means I'm cool with this justification, but I wanted to make explicit my reasoning in case I was making an invalid assumption; I don't know this code well. > > Also, what kind of race condition are you worried about? Most of my poking at this is out of pure paranoia. OTOH, that paranoia's the result of several years of multi-threaded programming :-}. I also tend to worry more than many people about really low likelihood bugs, because when they do happen, they're almost impossible to reproduce and then to fix. > Everything's being > done on the IO thread, so there shouldn't be any corruption of the CookieMonster > itself. If you're worried about the UI thread consumer making sure that the > delete happens before proceeding with any other activity, I don't know that > that's a safe assumption to make, since something else may have modified the > CookieMonster in the meantime anyway, so I don't think there are any reasonable > guarantees on the UI thread in the first place. That's the type of thing I was worried about; if the only user of this interface is the UI Cookie interface, I think that's a valid argument (and I think your IO thread serialization argument is also valid, so you've got belt-and-suspenders). Thanks for engaging with my paranoia. > > Let me know what you think about that justification. http://codereview.chromium.org/2756003/diff/1/3 File chrome/browser/extensions/extension_cookies_api.cc (right): http://codereview.chromium.org/2756003/diff/1/3#newcode382 chrome/browser/extensions/extension_cookies_api.cc:382: new RemoveCookieTask(url, name, store_profile->GetRequestContext())); On 2010/06/09 19:45:41, willchan wrote: > On 2010/06/09 17:54:49, rdsmith wrote: > > Again, a change to asynchronous. Could you reassure me (or future readers, in > a > > comment) as to why this is guaranteed safe? > > > > If there's no immediate reason to change to asynchronous, I'd recommend > staying > > synchronous; why risk subtle race conditions when we don't have to? > > Not blocking the UI thread is a performance win. Any delay on it will result in > all pending UI events in the queue being delayed. The delay would be roughly > equal to a single iteration through the IO thread. > > I think it's safe, due to the justification I provided in > automation_provider.cc. Let me know what you think about it. I am going to > loop in eroman for his opinion here too. That argument makes sense to me. I'm still internalizing the performance requirements of this project, but thinking about it, speeding up the UI thread is definitely a good thing. http://codereview.chromium.org/2756003/diff/1/7 File chrome/browser/extensions/extension_data_deleter.cc (right): http://codereview.chromium.org/2756003/diff/1/7#newcode47 chrome/browser/extensions/extension_data_deleter.cc:47: cookie_monster->DeleteAllForURL(extension_url_, true); Sorry, quite right; my eyes glazed over the boundary between the functions because of the diff.
http://codereview.chromium.org/2756003/diff/11001/12002 File chrome/browser/extensions/extension_cookies_api.cc (right): http://codereview.chromium.org/2756003/diff/11001/12002#newcode76 chrome/browser/extensions/extension_cookies_api.cc:76: namespace { Could you please move all this anonymous namespace code in this file to extension_cookies_helpers.h/.cc so it can be unit tested? http://codereview.chromium.org/2756003/diff/11001/12002#newcode136 chrome/browser/extensions/extension_cookies_api.cc:136: Profile* store_profile = GetStoreProfile(details); The purpose of ParseCookieStore was to refactor common code; it is meant to be analogous to ParseUrl in that it takes an API argument and returns the useful data associated with it. Here you've broken out the common code and are repeating it multiple times. Seems like you could move the GetStoreIdFromProfile back to where it was, and instead of ParseCookieStore you could say something like ParseStoreContext and obtain the URLRequestContextGetter, since you never use the store profile for anything else in this file.
http://codereview.chromium.org/2756003/diff/11001/12002 File chrome/browser/extensions/extension_cookies_api.cc (right): http://codereview.chromium.org/2756003/diff/11001/12002#newcode76 chrome/browser/extensions/extension_cookies_api.cc:76: namespace { On 2010/06/10 00:02:21, Cindy Lau wrote: > Could you please move all this anonymous namespace code in this file to > extension_cookies_helpers.h/.cc so it can be unit tested? Is there a reason you want to unit test the implementation directly rather than through the public interface? Is the public interface insufficient for any reason? http://codereview.chromium.org/2756003/diff/11001/12002#newcode136 chrome/browser/extensions/extension_cookies_api.cc:136: Profile* store_profile = GetStoreProfile(details); On 2010/06/10 00:02:21, Cindy Lau wrote: > The purpose of ParseCookieStore was to refactor common code; it is meant to be > analogous to ParseUrl in that it takes an API argument and returns the useful > data associated with it. Here you've broken out the common code and are > repeating it multiple times. Seems like you could move the > GetStoreIdFromProfile back to where it was, and instead of ParseCookieStore you > could say something like ParseStoreContext and obtain the > URLRequestContextGetter, since you never use the store profile for anything else > in this file. Done.
LGTM on cookie_policy_browsertest.cc. http://codereview.chromium.org/2756003/diff/22001/10009 File chrome/browser/net/cookie_policy_browsertest.cc (right): http://codereview.chromium.org/2756003/diff/22001/10009#newcode15 chrome/browser/net/cookie_policy_browsertest.cc:15: namespace { Is this necessary?
Thanks. LGTM aside from some minor things. http://codereview.chromium.org/2756003/diff/11001/12002 File chrome/browser/extensions/extension_cookies_api.cc (right): http://codereview.chromium.org/2756003/diff/11001/12002#newcode76 chrome/browser/extensions/extension_cookies_api.cc:76: namespace { On 2010/06/10 00:43:32, willchan wrote: > On 2010/06/10 00:02:21, Cindy Lau wrote: > > Could you please move all this anonymous namespace code in this file to > > extension_cookies_helpers.h/.cc so it can be unit tested? > > Is there a reason you want to unit test the implementation directly rather than > through the public interface? Is the public interface insufficient for any > reason? I guess you may be able to use static mocks to mock out the task threading here; I was concerned that you wouldn't be able to do so. Feel free to leave it as is if that's the case. http://codereview.chromium.org/2756003/diff/22001/10003#newcode148 chrome/browser/extensions/extension_cookies_api.cc:148: // Return the first matching cookie. Relies on the fact that the Please add the DCHECK back in from the original code. http://codereview.chromium.org/2756003/diff/22001/10003#newcode183 chrome/browser/extensions/extension_cookies_api.cc:183: ListValue* matching_list = new ListValue(); Please restore the original DCHECK. http://codereview.chromium.org/2756003/diff/22001/10003#newcode328 chrome/browser/extensions/extension_cookies_api.cc:328: error_ = ExtensionErrorUtils::FormatErrorMessage( DCHECK http://codereview.chromium.org/2756003/diff/22001/10003#newcode383 chrome/browser/extensions/extension_cookies_api.cc:383: bool rv = ChromeThread::PostTask( DCHECK http://codereview.chromium.org/2756003/diff/22001/10004 File chrome/browser/extensions/extension_cookies_api.h (right): http://codereview.chromium.org/2756003/diff/22001/10004#newcode33 chrome/browser/extensions/extension_cookies_api.h:33: // the cookie store context. If the 'storeId' value isn't found in the Please add the comments back in about the store_id output parameter from the original code.
The blocking wait being done here in the extension API is pretty nasty. It seems the right solution is to instead register the extension callbacks as running on the IO thread rather than the UI thread. That way the callback will be directly invoked on the IO thread and this threading issue won't arise. mpcomplete: Is there an existing mechanism in the extension framework to specify which thread the callbacks should be invoked on? If not, it seems like an important feature to add, since we will be hitting this problem for any other future network extension API that we want to add (as they all need to run code on the IO thread and not the UI thread)... And wrapping them all in this way is not a viable option IMO. http://codereview.chromium.org/2756003/diff/11001/12001 File chrome/browser/automation/automation_provider.cc (right): http://codereview.chromium.org/2756003/diff/11001/12001#newcode1177 chrome/browser/automation/automation_provider.cc:1177: const std::string& value, indentatin. http://codereview.chromium.org/2756003/diff/22001/10011 File net/base/cookie_monster.h (right): http://codereview.chromium.org/2756003/diff/22001/10011#newcode291 net/base/cookie_monster.h:291: Lock lock_; Not getting rid of the lock?
On 2010/06/10 20:23:26, eroman wrote: > The blocking wait being done here in the extension API is pretty nasty. > > It seems the right solution is to instead register the extension callbacks as > running on the IO thread rather than the UI thread. > > That way the callback will be directly invoked on the IO thread and this > threading issue won't arise. > > mpcomplete: Is there an existing mechanism in the extension framework to specify > which thread the callbacks should be invoked on? the cookies extension api uses sync callbacks. you can use async callbacks instead, so there's no need to block when waiting for results from other threads. > > If not, it seems like an important feature to add, since we will be hitting this > problem for any other future network extension API that we want to add (as they > all need to run code on the IO thread and not the UI thread)... And wrapping > them all in this way is not a viable option IMO. > > http://codereview.chromium.org/2756003/diff/11001/12001 > File chrome/browser/automation/automation_provider.cc (right): > > http://codereview.chromium.org/2756003/diff/11001/12001#newcode1177 > chrome/browser/automation/automation_provider.cc:1177: const std::string& value, > indentatin. > > http://codereview.chromium.org/2756003/diff/22001/10011 > File net/base/cookie_monster.h (right): > > http://codereview.chromium.org/2756003/diff/22001/10011#newcode291 > net/base/cookie_monster.h:291: Lock lock_; > Not getting rid of the lock?
On 2010/06/14 10:53:57, jochen wrote: > On 2010/06/10 20:23:26, eroman wrote: > > The blocking wait being done here in the extension API is pretty nasty. > > > > It seems the right solution is to instead register the extension callbacks as > > running on the IO thread rather than the UI thread. > > > > That way the callback will be directly invoked on the IO thread and this > > threading issue won't arise. > > > > mpcomplete: Is there an existing mechanism in the extension framework to > specify > > which thread the callbacks should be invoked on? > > the cookies extension api uses sync callbacks. you can use async callbacks > instead, so there's no need to block when waiting for results from other > threads. Are you sure this is reasonable? I'm not an extension expert here, so I defer to the reviewers, but it doesn't seem ideal to make these cookie functions async. But I'm happy to make that change if it's ok with you guys. > > > > > If not, it seems like an important feature to add, since we will be hitting > this > > problem for any other future network extension API that we want to add (as > they > > all need to run code on the IO thread and not the UI thread)... And wrapping > > them all in this way is not a viable option IMO. > > > > http://codereview.chromium.org/2756003/diff/11001/12001 > > File chrome/browser/automation/automation_provider.cc (right): > > > > http://codereview.chromium.org/2756003/diff/11001/12001#newcode1177 > > chrome/browser/automation/automation_provider.cc:1177: const std::string& > value, > > indentatin. > > > > http://codereview.chromium.org/2756003/diff/22001/10011 > > File net/base/cookie_monster.h (right): > > > > http://codereview.chromium.org/2756003/diff/22001/10011#newcode291 > > net/base/cookie_monster.h:291: Lock lock_; > > Not getting rid of the lock?
I think the ideal is to be able to register sync callbacks, but tell the extension system to call the sync handlers on the IO thread rather than the UI thread. I am not familiar with the extension framework, but this seems like a very reasonable thing for it to support.
I chatted with aa about this. He said that all the extension functions are async anyway from the extension's point of view. async/sync only refers to how it's handled in the browser's UI thread. So, yeah, the simplest thing to do is make this async. I'll make that change and re-ping when ready for review. On Tue, Jun 15, 2010 at 1:36 PM, <eroman@chromium.org> wrote: > I think the ideal is to be able to register sync callbacks, but tell the > extension system to call the sync handlers on the IO thread rather than the > UI > thread. > > I am not familiar with the extension framework, but this seems like a very > reasonable thing for it to support. > > http://codereview.chromium.org/2756003/show >
Ok, the extension_cookie_api.* stuff is redone now and needs a re-review. http://codereview.chromium.org/2756003/diff/11001/12001 File chrome/browser/automation/automation_provider.cc (right): http://codereview.chromium.org/2756003/diff/11001/12001#newcode1177 chrome/browser/automation/automation_provider.cc:1177: const std::string& value, On 2010/06/10 20:23:26, eroman wrote: > indentatin. Done. http://codereview.chromium.org/2756003/diff/22001/10004 File chrome/browser/extensions/extension_cookies_api.h (right): http://codereview.chromium.org/2756003/diff/22001/10004#newcode33 chrome/browser/extensions/extension_cookies_api.h:33: // the cookie store context. If the 'storeId' value isn't found in the On 2010/06/10 19:51:23, Cindy Lau wrote: > Please add the comments back in about the store_id output parameter from the > original code. Done. http://codereview.chromium.org/2756003/diff/22001/10009 File chrome/browser/net/cookie_policy_browsertest.cc (right): http://codereview.chromium.org/2756003/diff/22001/10009#newcode15 chrome/browser/net/cookie_policy_browsertest.cc:15: namespace { On 2010/06/10 03:14:32, wtc wrote: > Is this necessary? I'm not sure. The GetCookies() task might conflict, I dunno. But I don't think there's any reason for test code to be in the global namespace, since no one should be calling into it. Let me know if you care about this. http://codereview.chromium.org/2756003/diff/22001/10011 File net/base/cookie_monster.h (right): http://codereview.chromium.org/2756003/diff/22001/10011#newcode291 net/base/cookie_monster.h:291: Lock lock_; On 2010/06/10 20:23:26, eroman wrote: > Not getting rid of the lock? Not yet. I want to make sure I didn't miss anything. I figured I'd do it in a second pass after letting things bake. Maybe I'm being overly paranoid?
http://codereview.chromium.org/2756003/diff/22001/10011 File net/base/cookie_monster.h (right): http://codereview.chromium.org/2756003/diff/22001/10011#newcode291 net/base/cookie_monster.h:291: Lock lock_; On 2010/06/16 00:52:22, willchan wrote: > On 2010/06/10 20:23:26, eroman wrote: > > Not getting rid of the lock? > > Not yet. I want to make sure I didn't miss anything. I figured I'd do it in a > second pass after letting things bake. Maybe I'm being overly paranoid? I am fine with that. Can you add a TODO for removing it in that case? http://codereview.chromium.org/2756003/diff/38001/39002 File chrome/browser/extensions/extension_cookies_api.cc (right): http://codereview.chromium.org/2756003/diff/38001/39002#newcode188 chrome/browser/extensions/extension_cookies_api.cc:188: void GetCookieFunction::RespondOnUIThread() { The split of logic between GetXXXOnIOThread() and ResponseOnUIThread() seems like it can be simplified. How about putting all of the logic into GetXXXOnIOThread(), so ResponseONUIThread() can be a light wrapper, say: void ResponseOnUIThread(Value* value, bool success) { result_.reset(value); SendResponse(success); } Moreover, you could probably eliminate most of the additional parameters by making them arguments to GetCookiesOnIOThead() instead. http://codereview.chromium.org/2756003/diff/38001/39002#newcode245 chrome/browser/extensions/extension_cookies_api.cc:245: void GetAllCookiesFunction::RespondOnUIThread() { Same comment as above. Seems unecessary to add store_id_, url_, details_, cookie_list_ as members when they could be arguments to GetCookiesOnIOThread(), and let it do the creation of the ListValue*.
http://codereview.chromium.org/2756003/diff/22001/10011 File net/base/cookie_monster.h (right): http://codereview.chromium.org/2756003/diff/22001/10011#newcode291 net/base/cookie_monster.h:291: Lock lock_; On 2010/06/16 01:24:31, eroman wrote: > On 2010/06/16 00:52:22, willchan wrote: > > On 2010/06/10 20:23:26, eroman wrote: > > > Not getting rid of the lock? > > > > Not yet. I want to make sure I didn't miss anything. I figured I'd do it in > a > > second pass after letting things bake. Maybe I'm being overly paranoid? > > I am fine with that. Can you add a TODO for removing it in that case? Done. http://codereview.chromium.org/2756003/diff/38001/39002 File chrome/browser/extensions/extension_cookies_api.cc (right): http://codereview.chromium.org/2756003/diff/38001/39002#newcode245 chrome/browser/extensions/extension_cookies_api.cc:245: void GetAllCookiesFunction::RespondOnUIThread() { On 2010/06/16 01:24:31, eroman wrote: > Same comment as above. Seems unecessary to add store_id_, url_, details_, > cookie_list_ as members when they could be arguments to GetCookiesOnIOThread(), > and let it do the creation of the ListValue*. I've address some bits of this, but I've kept most of it the same. I didn't implement it due to some complications. * GetExtension() pulls the extension from the Profile. This needs to happen on the UI thread. * parameter storage isn't well defined in tasks. I'd have to copy all these values to the IO thread, which isn't the end of the world. The one inconsistency, is that we'd also have to copy the scoped_refptr for the URLRequestContextGetter, otherwise, the reference may disappear from underneath us. This is non-obvious to many people. * My solution addresses the storage and ref counting issues by making the function object hold everything. Let me know what you think about this. I have a preference for my way, but it's not strong. If you feel strongly, I am happy to switch to your suggestion (other than GetExtension()).
http://codereview.chromium.org/2756003/diff/51001/52002 File chrome/browser/extensions/extension_cookies_api.cc (right): http://codereview.chromium.org/2756003/diff/51001/52002#newcode176 chrome/browser/extensions/extension_cookies_api.cc:176: void GetCookieFunction::GetCookiesOnIOThread() { This function name could be improved, since you're basically implementing the entire API in this function. This latest change is kind of strange IMHO, because now you've got all of GetCookieFunction implemented here, whereas GetAllCookiesFunction splits the work differently because of GetExtension(). More consistency would be nice.
http://codereview.chromium.org/2756003/diff/51001/52002 File chrome/browser/extensions/extension_cookies_api.cc (right): http://codereview.chromium.org/2756003/diff/51001/52002#newcode176 chrome/browser/extensions/extension_cookies_api.cc:176: void GetCookieFunction::GetCookiesOnIOThread() { On 2010/06/16 21:12:30, Cindy Lau wrote: > This function name could be improved, since you're basically implementing the I'm not sure what a better name would be. Can you suggest one? I think the function name states pretty clearly what it's doing. > entire API in this function. This latest change is kind of strange IMHO, > because now you've got all of GetCookieFunction implemented here, whereas > GetAllCookiesFunction splits the work differently because of GetExtension(). > More consistency would be nice. I agree more consistency would be nice. I had adopted Eric's suggestion, but ran into the GetExtension() change later. Rather than make it consistent again and risk having to re-write it all over again, I wanted to request comments first. Do you want to step in and state your preference here? I'm going to wait until Eric weighs in too before I proceed.
LGTM. Thanks for addressing the comments from so many reviewers. http://codereview.chromium.org/2756003/diff/51001/52001 File chrome/browser/automation/automation_provider.cc (right): http://codereview.chromium.org/2756003/diff/51001/52001#newcode1167 chrome/browser/automation/automation_provider.cc:1167: ChromeThread::PostTask( Can you catch the return value for PostTask, and then either CHECK() that it is OK, or early return? I suggest this as a safety net to avoid hanging forever in case the Post fails. http://codereview.chromium.org/2756003/diff/51001/52001#newcode1170 chrome/browser/automation/automation_provider.cc:1170: event.Wait(); Do you know if testing is the only consumer of this? Or does ChromeFrame use it too? If the latter, I suggest adding a TODO to make the upper layer interface async, so we don't need to block here. http://codereview.chromium.org/2756003/diff/51001/52001#newcode1177 chrome/browser/automation/automation_provider.cc:1177: const std::string& value, nit:indentation http://codereview.chromium.org/2756003/diff/51001/52001#newcode1209 chrome/browser/automation/automation_provider.cc:1209: ChromeThread::PostTask( Same comment as above, please bullet-proof this against hangs (my preference is to convert the hang into a crash). http://codereview.chromium.org/2756003/diff/51001/52002 File chrome/browser/extensions/extension_cookies_api.cc (right): http://codereview.chromium.org/2756003/diff/51001/52002#newcode282 chrome/browser/extensions/extension_cookies_api.cc:282: EXTENSION_FUNCTION_VALIDATE(details->GetBoolean(keys::kSecureKey, &secure_)); >80c http://codereview.chromium.org/2756003/diff/51001/52003 File chrome/browser/extensions/extension_cookies_api.h (right): http://codereview.chromium.org/2756003/diff/51001/52003#newcode64 chrome/browser/extensions/extension_cookies_api.h:64: // and are not threadsafe. They modify the result_ member variable directly; might want to clarify what it means now to be "not threadsafe", since the class does in fact run on multiple threads. http://codereview.chromium.org/2756003/diff/51001/52003#newcode96 chrome/browser/extensions/extension_cookies_api.h:96: void GetCookiesOnIOThread(); nit: I wander if for consistency this should be called GetCookieOnIOThread()? (singular). http://codereview.chromium.org/2756003/diff/51001/52003#newcode113 chrome/browser/extensions/extension_cookies_api.h:113: void GetCookiesOnIOThread(); nit: for consistency, I wander if this should be called GetAllCookiesOnIOThread()?
LGTM http://codereview.chromium.org/2756003/diff/51001/52002 File chrome/browser/extensions/extension_cookies_api.cc (right): http://codereview.chromium.org/2756003/diff/51001/52002#newcode176 chrome/browser/extensions/extension_cookies_api.cc:176: void GetCookieFunction::GetCookiesOnIOThread() { On 2010/06/16 21:16:08, willchan wrote: > On 2010/06/16 21:12:30, Cindy Lau wrote: > > This function name could be improved, since you're basically implementing the > > I'm not sure what a better name would be. Can you suggest one? I think the > function name states pretty clearly what it's doing. > > > entire API in this function. This latest change is kind of strange IMHO, > > because now you've got all of GetCookieFunction implemented here, whereas > > GetAllCookiesFunction splits the work differently because of GetExtension(). > > More consistency would be nice. > > I agree more consistency would be nice. I had adopted Eric's suggestion, but > ran into the GetExtension() change later. Rather than make it consistent again > and risk having to re-write it all over again, I wanted to request comments > first. Do you want to step in and state your preference here? I'm going to > wait until Eric weighs in too before I proceed. My preference would be to put the bulk of the implementations in RespondOnUIThread for consistency at this point. If you had ended up also using Eric's suggestion of sending the input data as params to GetCookiesOnIOThread() instead of member vars, it would have made more sense to try and put the implementations there, but the storage/refcounting issues you brought up make sense. But we're splitting hairs here, I think. I'm fine with whatever you decide to do, but if you leave the implementation as is then I second Eric's function name suggestions. http://codereview.chromium.org/2756003/diff/51001/52003 File chrome/browser/extensions/extension_cookies_api.h (right): http://codereview.chromium.org/2756003/diff/51001/52003#newcode96 chrome/browser/extensions/extension_cookies_api.h:96: void GetCookiesOnIOThread(); On 2010/06/18 00:22:34, eroman wrote: > nit: I wander if for consistency this should be called GetCookieOnIOThread()? > (singular). +1 on this name suggestion http://codereview.chromium.org/2756003/diff/51001/52003#newcode113 chrome/browser/extensions/extension_cookies_api.h:113: void GetCookiesOnIOThread(); On 2010/06/18 00:22:34, eroman wrote: > nit: for consistency, I wander if this should be called > GetAllCookiesOnIOThread()? +1
OK, I think I've nailed everything now. Since I've got the LGTMs, I'm going to land this once trybots look good, unless anyone else has any other comments. http://codereview.chromium.org/2756003/diff/51001/52001 File chrome/browser/automation/automation_provider.cc (right): http://codereview.chromium.org/2756003/diff/51001/52001#newcode1167 chrome/browser/automation/automation_provider.cc:1167: ChromeThread::PostTask( On 2010/06/18 00:22:34, eroman wrote: > Can you catch the return value for PostTask, and then either CHECK() that it is > OK, or early return? > > I suggest this as a safety net to avoid hanging forever in case the Post fails. Done. http://codereview.chromium.org/2756003/diff/51001/52001#newcode1170 chrome/browser/automation/automation_provider.cc:1170: event.Wait(); On 2010/06/18 00:22:34, eroman wrote: > Do you know if testing is the only consumer of this? Or does ChromeFrame use it > too? After tracing through the code, it looks like ChromeFrame uses InternetGetCookieA() or npapi::GetValueForURL(instance_, NPNURLVCookie, ...). > > If the latter, I suggest adding a TODO to make the upper layer interface async, > so we don't need to block here. http://codereview.chromium.org/2756003/diff/51001/52001#newcode1177 chrome/browser/automation/automation_provider.cc:1177: const std::string& value, On 2010/06/18 00:22:34, eroman wrote: > nit:indentation Done. http://codereview.chromium.org/2756003/diff/51001/52001#newcode1209 chrome/browser/automation/automation_provider.cc:1209: ChromeThread::PostTask( On 2010/06/18 00:22:34, eroman wrote: > Same comment as above, please bullet-proof this against hangs (my preference is > to convert the hang into a crash). Done. http://codereview.chromium.org/2756003/diff/51001/52002 File chrome/browser/extensions/extension_cookies_api.cc (right): http://codereview.chromium.org/2756003/diff/51001/52002#newcode176 chrome/browser/extensions/extension_cookies_api.cc:176: void GetCookieFunction::GetCookiesOnIOThread() { On 2010/06/18 18:04:04, Cindy Lau wrote: > On 2010/06/16 21:16:08, willchan wrote: > > On 2010/06/16 21:12:30, Cindy Lau wrote: > > > This function name could be improved, since you're basically implementing > the > > > > I'm not sure what a better name would be. Can you suggest one? I think the > > function name states pretty clearly what it's doing. > > > > > entire API in this function. This latest change is kind of strange IMHO, > > > because now you've got all of GetCookieFunction implemented here, whereas > > > GetAllCookiesFunction splits the work differently because of GetExtension(). > > > > More consistency would be nice. > > > > I agree more consistency would be nice. I had adopted Eric's suggestion, but > > ran into the GetExtension() change later. Rather than make it consistent > again > > and risk having to re-write it all over again, I wanted to request comments > > first. Do you want to step in and state your preference here? I'm going to > > wait until Eric weighs in too before I proceed. > > My preference would be to put the bulk of the implementations in > RespondOnUIThread for consistency at this point. If you had ended up also using > Eric's suggestion of sending the input data as params to GetCookiesOnIOThread() > instead of member vars, it would have made more sense to try and put the > implementations there, but the storage/refcounting issues you brought up make > sense. > > But we're splitting hairs here, I think. I'm fine with whatever you decide to > do, but if you leave the implementation as is then I second Eric's function name > suggestions. Done. http://codereview.chromium.org/2756003/diff/51001/52002#newcode282 chrome/browser/extensions/extension_cookies_api.cc:282: EXTENSION_FUNCTION_VALIDATE(details->GetBoolean(keys::kSecureKey, &secure_)); On 2010/06/18 00:22:34, eroman wrote: > >80c Done. http://codereview.chromium.org/2756003/diff/51001/52003 File chrome/browser/extensions/extension_cookies_api.h (right): http://codereview.chromium.org/2756003/diff/51001/52003#newcode64 chrome/browser/extensions/extension_cookies_api.h:64: // and are not threadsafe. They modify the result_ member variable directly; On 2010/06/18 00:22:34, eroman wrote: > might want to clarify what it means now to be "not threadsafe", since the class > does in fact run on multiple threads. Done. http://codereview.chromium.org/2756003/diff/51001/52003#newcode96 chrome/browser/extensions/extension_cookies_api.h:96: void GetCookiesOnIOThread(); On 2010/06/18 18:04:04, Cindy Lau wrote: > On 2010/06/18 00:22:34, eroman wrote: > > nit: I wander if for consistency this should be called GetCookieOnIOThread()? > > (singular). > > +1 on this name suggestion Done. http://codereview.chromium.org/2756003/diff/51001/52003#newcode113 chrome/browser/extensions/extension_cookies_api.h:113: void GetCookiesOnIOThread(); On 2010/06/18 18:04:04, Cindy Lau wrote: > On 2010/06/18 00:22:34, eroman wrote: > > nit: for consistency, I wander if this should be called > > GetAllCookiesOnIOThread()? > > +1 Done. |