Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12805010:
Implementing EvictedDomainCookieCounter to keep user metrics on cookie eviction and reinstatement. (Closed)
|
Created:
7 years, 9 months ago by huangs Modified:
7 years, 8 months ago Reviewers:
erikwright (departed), jochen (gone - plz use gerrit), willchan no longer on Chromium, mmenke, Roger McFarlane (Chromium) CC:
chromium-reviews, erikwright (departed), cbentzel+watch_chromium.org, sail+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplementing EvictedDomainCookieCounter to keep statistics on cookie eviction and reinstatement.
BUG=225765
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192866
Patch Set 1 #
Total comments: 36
Patch Set 2 : Cleanups; simplifying garbage collection; simplifying UMA; fixing off-by-1 meaning of max_size_. #
Total comments: 33
Patch Set 3 : Style fixes; fixing incorrect reinstatement of expired cookie. #
Total comments: 14
Patch Set 4 : Cleanups and comment fixes. #
Total comments: 18
Patch Set 5 : Moving EvictedDomainCookieCounter to chrome/browser/net; fixing GarbageCollect() algorithm. #
Total comments: 6
Patch Set 6 : Sync and cleanup. #
Total comments: 43
Patch Set 7 : Style and comment fixes; changing partial_sort() comparator to fix Linux build. #
Total comments: 7
Patch Set 8 : Comment fixes; variable renaming. #Patch Set 9 : Fixing Linux warnings (that lead to error). #Patch Set 10 : Another attempt to fix weird scoped_ptr<> error in Linux. #Patch Set 11 : Using scoped_ptr<Type>::PassAs<ParentType>() to solve Linux scoped_ptr<> compile error. #Patch Set 12 : Fixing absense of private virtual destructor of ChangedDelegateDummy. #Patch Set 13 : Sync. #
Messages
Total messages: 29 (0 generated)
PTAL. Is there a BUG id I can use?
I've only looked at the implementation so far, not the unittests. https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... File net/cookies/evicted_domain_cookie_counter.cc (right): https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.cc:41: // Histogram variables; see CookieMonster::InitializeHistograms() in Please read the comment in InitializeHistograms that this refers to. You don't need to store these members or Initialize anything because you don't have the same threading constraints. https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.cc:106: bool EvictedDomainCookieCounter::IsGoogleCookie(const CanonicalCookie& cc) { This implementation is probably fine for the purposes here, but would be unsafe if used in other ways. I'd suggest adding "Unsafe" to the method name and adding a comment that it's only used for the non-security-sensitive purpose of segregating metrics. https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.cc:156: // |current| is passed by value, in case it came from a to-be-deleted element. This comment makes no sense? Why can't it be const&? https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.cc:169: void EvictedDomainCookieCounter::GarbageCollect(const Time& current) { The current implementation seems very costly, since it GarbageCollects on every eviction. Probably better to maintain the priority queue as a member and just change it incrementally with each insert/delete in the map. https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.cc:201: if (it->second->is_expired(current)) // Will be removed, so decrase quota. typo https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.cc:201: if (it->second->is_expired(current)) // Will be removed, so decrase quota. By definition the expired cookies will always be at the top of the priority_queue, right? So instead of treating them specially here (and manipulating remove_quota) why not just put them in the queue and then, after removing 'remove_quota', continue removing expired cookies? https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.cc:214: oldest_set.pop(); Couldn't you just pop and then use the key to delete from the map? https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.cc:244: DCHECK(reporter_); I don't think this DCHECK is required here. But it should be in the constructors. https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... File net/cookies/evicted_domain_cookie_counter.h (right): https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.h:14: #include "net/cookies/canonical_cookie.h" forward-decl https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.h:20: // cookies, i.e., cookies that were "evicted" (on reacking domain cookie limit) typo https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.h:40: // Structure to store sanitized (no cookie value) data from CanonicalCookie. parenthetical is unnecessary https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.h:62: public: Needs a virtual destructor. https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.h:71: // Identifier of an evicted cookie. Can be obtained from GetKey(). These typedefs can be private, right? https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.h:77: explicit EvictedDomainCookieCounter(CookieMonster::Delegate* next_delegate); make scoped_refptr<CookieMonster::Delegate> https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.h:80: scoped_refptr<CookieMonster::Delegate> next_delegate, ref_counted.h https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.h:81: scoped_ptr<Delegate> reporter, scoped_ptr.h https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.h:94: ChangeCause cause) OVERRIDE; compiler_specific.h for OVERRIDE https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.h:140: DISALLOW_COPY_AND_ASSIGN(EvictedDomainCookieCounter); basictypes.h
PTAL. https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... File net/cookies/evicted_domain_cookie_counter.cc (right): https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.cc:41: // Histogram variables; see CookieMonster::InitializeHistograms() in On 2013/03/21 19:22:19, erikwright wrote: > Please read the comment in InitializeHistograms that this refers to. You don't > need to store these members or Initialize anything because you don't have the > same threading constraints. Done. https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.cc:106: bool EvictedDomainCookieCounter::IsGoogleCookie(const CanonicalCookie& cc) { On 2013/03/21 19:22:19, erikwright wrote: > This implementation is probably fine for the purposes here, but would be unsafe > if used in other ways. I'd suggest adding "Unsafe" to the method name and adding > a comment that it's only used for the non-security-sensitive purpose of > segregating metrics. Done. https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.cc:156: // |current| is passed by value, in case it came from a to-be-deleted element. For the current usage it's fine. But if we pass by reference and have: // |it| is some item in evicted_cookies_ FreeExpiredEvictedCookies(it->second.expiry_time); then it->second will be deallocated in the loop, which means the parameter is a reference to a deallocated object in the memory => wrong. https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.cc:169: void EvictedDomainCookieCounter::GarbageCollect(const Time& current) { Per discussion, only executed when exceeding max_size_. https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.cc:201: if (it->second->is_expired(current)) // Will be removed, so decrase quota. On 2013/03/21 19:22:19, erikwright wrote: > typo Done. https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.cc:201: if (it->second->is_expired(current)) // Will be removed, so decrase quota. Per discussion, sort is done by eviction time, not expiry time. Also, we should remove expired cookies first, since they MUST be removed. https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.cc:214: oldest_set.pop(); priority_queue<T>::pop() returns void (STL weirdness). Also, deletion from map would incur look-up overhead. https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.cc:244: DCHECK(reporter_); On 2013/03/21 19:22:19, erikwright wrote: > I don't think this DCHECK is required here. But it should be in the > constructors. Done. https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... File net/cookies/evicted_domain_cookie_counter.h (right): https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.h:14: #include "net/cookies/canonical_cookie.h" On 2013/03/21 19:22:19, erikwright wrote: > forward-decl Done. https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.h:20: // cookies, i.e., cookies that were "evicted" (on reacking domain cookie limit) On 2013/03/21 19:22:19, erikwright wrote: > typo Done. https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.h:40: // Structure to store sanitized (no cookie value) data from CanonicalCookie. On 2013/03/21 19:22:19, erikwright wrote: > parenthetical is unnecessary Done. https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.h:62: public: Done. Making it public instead of protected (else it won't compile due to scoped_ptr<T> complaining). https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.h:71: // Identifier of an evicted cookie. Can be obtained from GetKey(). On 2013/03/21 19:22:19, erikwright wrote: > These typedefs can be private, right? Done. https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.h:77: explicit EvictedDomainCookieCounter(CookieMonster::Delegate* next_delegate); On 2013/03/21 19:22:19, erikwright wrote: > make scoped_refptr<CookieMonster::Delegate> Done. https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.h:80: scoped_refptr<CookieMonster::Delegate> next_delegate, Done. https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.h:81: scoped_ptr<Delegate> reporter, On 2013/03/21 19:22:19, erikwright wrote: > scoped_ptr.h Done. https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.h:94: ChangeCause cause) OVERRIDE; On 2013/03/21 19:22:19, erikwright wrote: > compiler_specific.h for OVERRIDE Done. https://codereview.chromium.org/12805010/diff/1/net/cookies/evicted_domain_co... net/cookies/evicted_domain_cookie_counter.h:140: DISALLOW_COPY_AND_ASSIGN(EvictedDomainCookieCounter); On 2013/03/21 19:22:19, erikwright wrote: > basictypes.h Done.
https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... File net/cookies/evicted_domain_cookie_counter.cc (right): https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... net/cookies/evicted_domain_cookie_counter.cc:45: HISTOGRAM_CUSTOM_TIMES("Cookie.ReinstatedCookiesGoogleSeconds", I believe that this macro already defines a static object, so there's no need for you to use statics for the two constants. You should be able to inline them. Please grep for other invocations and do what they do (I suspect it's inline). https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... net/cookies/evicted_domain_cookie_counter.cc:45: HISTOGRAM_CUSTOM_TIMES("Cookie.ReinstatedCookiesGoogleSeconds", Please check for other uses of HISTOGRAM_CUSTOM_TIMES. Is it appropriate to include the unit in the name? https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... net/cookies/evicted_domain_cookie_counter.cc:65: DCHECK(reporter_); Not required here, since you just assigned it three lines up. https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... net/cookies/evicted_domain_cookie_counter.cc:78: DCHECK(purge_count < max_size_); Unless something is going to crash later because of this, I wouldn't worry about it. https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... net/cookies/evicted_domain_cookie_counter.cc:116: if (ProcessCookieReinstatement(key, cc, current)) I think it would be clearer if ProcessCookieReinstatement were renamed to ProcessNewCookie and handled both the potential reporting and eviction. https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... net/cookies/evicted_domain_cookie_counter.cc:163: // Among evicted cookies with the same eviction time, bias may arise owing to LOL. You seem to take this personally. This does not require a comment. https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... net/cookies/evicted_domain_cookie_counter.cc:166: struct EvictedCookiePriorityQueueEntry { IIUC, you don't need a custom type here anymore, just a custom comparator (second type parameter to std::priority_queue) https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... File net/cookies/evicted_domain_cookie_counter.h (right): https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... net/cookies/evicted_domain_cookie_counter.h:31: // that these cookies are wrongly evicted. To measure the effictive of this effectiveness https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... net/cookies/evicted_domain_cookie_counter.h:87: // Used for non-security-sensitive purpose of segragrating metrics. segregating https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... net/cookies/evicted_domain_cookie_counter.h:117: // If too many evicted cookies are stored, delete the expired oens, then oens -> ones https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... net/cookies/evicted_domain_cookie_counter.h:122: // possibly replacing an existing evicted cookie. 'existing equivalent' https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... net/cookies/evicted_domain_cookie_counter.h:127: // Called when a new cookie is added, i.e., reinstatement may occur. reinstatement may occur -> 'a potential reinstatement' https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... net/cookies/evicted_domain_cookie_counter.h:133: // For chaining the OnCookieChanged() event. // Another delegate to forward events to. https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... net/cookies/evicted_domain_cookie_counter.h:134: scoped_refptr<CookieMonster::Delegate> next_delegate_; rename to next_cookie_monster_delegate_ for disambiguation. https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... net/cookies/evicted_domain_cookie_counter.h:136: // On reinstatement, data are passed here to report reinstatement delay. No comment required. Rename to delegate_ https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... net/cookies/evicted_domain_cookie_counter.h:139: // In-memory-only buffer to store sanitized evicted cookies. no comment required.
PTAL. https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... File net/cookies/evicted_domain_cookie_counter.cc (right): https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... net/cookies/evicted_domain_cookie_counter.cc:45: HISTOGRAM_CUSTOM_TIMES("Cookie.ReinstatedCookiesGoogleSeconds", Yeah it can be inlined. https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... net/cookies/evicted_domain_cookie_counter.cc:45: HISTOGRAM_CUSTOM_TIMES("Cookie.ReinstatedCookiesGoogleSeconds", I was probably looking at the wrong things. Removed. https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... net/cookies/evicted_domain_cookie_counter.cc:65: DCHECK(reporter_); On 2013/03/22 15:53:26, erikwright wrote: > Not required here, since you just assigned it three lines up. Done. https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... net/cookies/evicted_domain_cookie_counter.cc:78: DCHECK(purge_count < max_size_); These are both size_t's, and a difference will be taken. So negative => large positive. Better to be safe, I think. https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... net/cookies/evicted_domain_cookie_counter.cc:116: if (ProcessCookieReinstatement(key, cc, current)) Renamed, and moved deletion to the routine. https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... net/cookies/evicted_domain_cookie_counter.cc:163: // Among evicted cookies with the same eviction time, bias may arise owing to Removed. :( https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... net/cookies/evicted_domain_cookie_counter.cc:166: struct EvictedCookiePriorityQueueEntry { The second type parameter to std::priority_queue is the container. Third is the comparator, BUT it looks like we need a class with operator(), rather than just a function (as with the case of std::sort()). https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... net/cookies/evicted_domain_cookie_counter.cc:222: EvictedCookieMap::iterator it = evicted_cookies_.find(key); If cc is expired, we would count it as being reinstated, which makes no sense. Fixing this. https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... File net/cookies/evicted_domain_cookie_counter.h (right): https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... net/cookies/evicted_domain_cookie_counter.h:31: // that these cookies are wrongly evicted. To measure the effictive of this On 2013/03/22 15:53:26, erikwright wrote: > effectiveness Done. https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... net/cookies/evicted_domain_cookie_counter.h:87: // Used for non-security-sensitive purpose of segragrating metrics. On 2013/03/22 15:53:26, erikwright wrote: > segregating Done. https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... net/cookies/evicted_domain_cookie_counter.h:117: // If too many evicted cookies are stored, delete the expired oens, then On 2013/03/22 15:53:26, erikwright wrote: > oens -> ones Done. https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... net/cookies/evicted_domain_cookie_counter.h:122: // possibly replacing an existing evicted cookie. On 2013/03/22 15:53:26, erikwright wrote: > 'existing equivalent' Done. https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... net/cookies/evicted_domain_cookie_counter.h:127: // Called when a new cookie is added, i.e., reinstatement may occur. On 2013/03/22 15:53:26, erikwright wrote: > reinstatement may occur -> 'a potential reinstatement' Done. https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... net/cookies/evicted_domain_cookie_counter.h:133: // For chaining the OnCookieChanged() event. On 2013/03/22 15:53:26, erikwright wrote: > // Another delegate to forward events to. Done. https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... net/cookies/evicted_domain_cookie_counter.h:134: scoped_refptr<CookieMonster::Delegate> next_delegate_; On 2013/03/22 15:53:26, erikwright wrote: > rename to next_cookie_monster_delegate_ for disambiguation. Done. https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... net/cookies/evicted_domain_cookie_counter.h:136: // On reinstatement, data are passed here to report reinstatement delay. On 2013/03/22 15:53:26, erikwright wrote: > No comment required. Rename to delegate_ Done. https://codereview.chromium.org/12805010/diff/8001/net/cookies/evicted_domain... net/cookies/evicted_domain_cookie_counter.h:139: // In-memory-only buffer to store sanitized evicted cookies. On 2013/03/22 15:53:26, erikwright wrote: > no comment required. Done.
A couple minor comments, but looking quite good. https://codereview.chromium.org/12805010/diff/16001/net/cookies/evicted_domai... File net/cookies/evicted_domain_cookie_counter.cc (right): https://codereview.chromium.org/12805010/diff/16001/net/cookies/evicted_domai... net/cookies/evicted_domain_cookie_counter.cc:7: #include <limits> What is limits used for? https://codereview.chromium.org/12805010/diff/16001/net/cookies/evicted_domai... net/cookies/evicted_domain_cookie_counter.cc:42: // Keep separate blocks, since HISTOGRAM_CUSTOM_TIMES is a macro. What does this comment mean? https://codereview.chromium.org/12805010/diff/16001/net/cookies/evicted_domai... net/cookies/evicted_domain_cookie_counter.cc:92: bool EvictedDomainCookieCounter::UnsafeIsGoogleCookie( Why not just use IsGoogleHostname(cc.Domain(), ALLOW_SUBDOMAIN)? If the only reason is because you can't access it from here, I would be happy to see this code moved to chrome/browser/net/ https://codereview.chromium.org/12805010/diff/16001/net/cookies/evicted_domai... net/cookies/evicted_domain_cookie_counter.cc:217: if (!cc.IsExpired(current)) { This should never happen. If the pre-expired cookie matches an existing cookie, the change reported would be a deletion with CHANGE_COOKIE_EXPIRED_OVERWRITE. Otherwise there is no report, as no change occurs to the cookie store. See cookie_monster.cc (SetCanonicalCookie) for the code that avoids calling InternalInsertCookie (and therefore OnCookieChanged) in that case. Also, it introduces an (admittedly academic) race condition because the time can advance between the check in cookie_monster and the check here. So just assume (correctly) that if OnCookieChanged reports a new cookie that it is actually being added. https://codereview.chromium.org/12805010/diff/16001/net/cookies/evicted_domai... File net/cookies/evicted_domain_cookie_counter.h (right): https://codereview.chromium.org/12805010/diff/16001/net/cookies/evicted_domai... net/cookies/evicted_domain_cookie_counter.h:31: // that these cookies are wrongly evicted. To measure the effictiveness of this effectiveness. https://codereview.chromium.org/12805010/diff/16001/net/cookies/evicted_domai... net/cookies/evicted_domain_cookie_counter.h:87: // Used for non-security-sensitive purpose of segregrating metrics. segregating https://codereview.chromium.org/12805010/diff/16001/net/cookies/evicted_domai... File net/cookies/evicted_domain_cookie_counter_unittest.cc (right): https://codereview.chromium.org/12805010/diff/16001/net/cookies/evicted_domai... net/cookies/evicted_domain_cookie_counter_unittest.cc:306: edcc_->OnCookieChanged(*cookies_[4], false, This seems like an invalid test. CHANGE_COOKIE_EXPIRED_OVERWRITE is used when the cookie currently in the store is valid, but is then removed by setting a new equivalent cookie with an expiry time in the past.
PTAL. https://codereview.chromium.org/12805010/diff/16001/net/cookies/evicted_domai... File net/cookies/evicted_domain_cookie_counter.cc (right): https://codereview.chromium.org/12805010/diff/16001/net/cookies/evicted_domai... net/cookies/evicted_domain_cookie_counter.cc:7: #include <limits> Was to find max int for rand, but no longer needed. Removed. https://codereview.chromium.org/12805010/diff/16001/net/cookies/evicted_domai... net/cookies/evicted_domain_cookie_counter.cc:42: // Keep separate blocks, since HISTOGRAM_CUSTOM_TIMES is a macro. It is tempting to "refactor" the code by removing duplicate code and use a temporary string "Cookie.ReinstatedCookiesGoogle" and "Cookie.ReinstatedCookiesOther". However, *I think* this "refactoring" task would be problematic, since HISTOGRAM_CUSTOM_TIMES() macro block with static object to store the histogram. Hence the comment. Rephrased. https://codereview.chromium.org/12805010/diff/16001/net/cookies/evicted_domai... net/cookies/evicted_domain_cookie_counter.cc:92: bool EvictedDomainCookieCounter::UnsafeIsGoogleCookie( It's due to target dependency; "net" should not depend on "browser"! https://codereview.chromium.org/12805010/diff/16001/net/cookies/evicted_domai... net/cookies/evicted_domain_cookie_counter.cc:217: if (!cc.IsExpired(current)) { On 2013/03/25 17:29:41, erikwright wrote: > This should never happen. If the pre-expired cookie matches an existing cookie, > the change reported would be a deletion with CHANGE_COOKIE_EXPIRED_OVERWRITE. > Otherwise there is no report, as no change occurs to the cookie store. > > See cookie_monster.cc (SetCanonicalCookie) for the code that avoids calling > InternalInsertCookie (and therefore OnCookieChanged) in that case. > > Also, it introduces an (admittedly academic) race condition because the time can > advance between the check in cookie_monster and the check here. > > So just assume (correctly) that if OnCookieChanged reports a new cookie that it > is actually being added. Done (removed check). https://codereview.chromium.org/12805010/diff/16001/net/cookies/evicted_domai... File net/cookies/evicted_domain_cookie_counter.h (right): https://codereview.chromium.org/12805010/diff/16001/net/cookies/evicted_domai... net/cookies/evicted_domain_cookie_counter.h:31: // that these cookies are wrongly evicted. To measure the effictiveness of this On 2013/03/25 17:29:41, erikwright wrote: > effectiveness. Done. https://codereview.chromium.org/12805010/diff/16001/net/cookies/evicted_domai... net/cookies/evicted_domain_cookie_counter.h:87: // Used for non-security-sensitive purpose of segregrating metrics. On 2013/03/25 17:29:41, erikwright wrote: > segregating Done. https://codereview.chromium.org/12805010/diff/16001/net/cookies/evicted_domai... File net/cookies/evicted_domain_cookie_counter_unittest.cc (right): https://codereview.chromium.org/12805010/diff/16001/net/cookies/evicted_domai... net/cookies/evicted_domain_cookie_counter_unittest.cc:306: edcc_->OnCookieChanged(*cookies_[4], false, Done (removed this case).
https://codereview.chromium.org/12805010/diff/24001/net/cookies/evicted_domai... File net/cookies/evicted_domain_cookie_counter.cc (right): https://codereview.chromium.org/12805010/diff/24001/net/cookies/evicted_domai... net/cookies/evicted_domain_cookie_counter.cc:118: FreeEvictedCookie(key); On the surface, it would seem like this should never happen. After all, if it is in your map, it was previously evicted. Ergo, you should not expect to receive any subsequent notifications about it being removed unless there was an intervening reinstatement. https://codereview.chromium.org/12805010/diff/24001/net/cookies/evicted_domai... net/cookies/evicted_domain_cookie_counter.cc:142: // |current| is passed by value, in case it came from a to-be-deleted element. Since this is a private method and you know it doesn't happen, there is no reason to pass by value here. In a class of this size, it is reasonable for the caller to know that this method potentially deletes EvictedCookie records. For example, they need to know that this method might invalidate iterators too. https://codereview.chromium.org/12805010/diff/24001/net/cookies/evicted_domai... net/cookies/evicted_domain_cookie_counter.cc:186: oldest_set.pop(); // Spare the youngest element from deletion. Alternatively, I suppose you could reverse the comparator (make top() return the least recently evicted cookie) and then just loop through |remove_quota| elements in the loop at line 191. That would remove some number of logn operations at line 186 while increasing N for the logn operations on line 191. My instincts tell me that it would be a net win? https://codereview.chromium.org/12805010/diff/24001/net/cookies/evicted_domai... net/cookies/evicted_domain_cookie_counter.cc:192: oldest_set.top()->set_expired(); Here (combined with FreeExpiredEvictedCookies) we are removing 100 out of 500 cookies by iterating through the 500 cookies. Iteration is O(N), removal of an item using iterator is amortized O(1). If you were to remove them in the loop through oldest_set, it would be 100 removals by key, which is O(logN). While we can't be sure without a benchmark, I assume that the base cost of a lookup is comparable to the cost of doing set_expired(), checking the expiry, and incrementing the iterator. Also, taking the delete-by-key approach will remove a bunch of code :) ------------------------------ Furthermore, couldn't you store EvictedCookieMap::iterators in the priority_queue? Then you could do erase-by-iterator which is O(1). Obviously this requires adjusting your comparator slightly to dereference the iterator. ------------------------------- So delete on line 192 instead of doing set_expired and FreeExpiredEvictedCookies. That will also require you to delete expired cookies during the garbage collection loop, but that should be OK. https://codereview.chromium.org/12805010/diff/24001/net/cookies/evicted_domai... net/cookies/evicted_domain_cookie_counter.cc:203: if (!cc.IsExpired(current)) { This should also only happen if the cause is CHANGE_COOKIE_EXPIRED, which never lands here. So the same comments apply as for ProcessNewCookie. https://codereview.chromium.org/12805010/diff/24001/net/cookies/evicted_domai... net/cookies/evicted_domain_cookie_counter.cc:204: FreeEvictedCookie(key); // Deallocate explicitly. std::map::insert returns a pair of iterator/bool. If there was already an equivalent element in the map the bool is false and the iterator points to it. So instead of FreeEvictedCookie here, just check the return value and, if it is not successful, you can use the returned iterator to delete the previous entry and replace it with the new one. I would put a NOTREACHED in that block as well. In combination with a previous comment, FreeEvictedCookie can now be deleted. https://codereview.chromium.org/12805010/diff/24001/net/cookies/evicted_domai... File net/cookies/evicted_domain_cookie_counter.h (right): https://codereview.chromium.org/12805010/diff/24001/net/cookies/evicted_domai... net/cookies/evicted_domain_cookie_counter.h:105: ~EvictedDomainCookieCounter(); mark explicitly as virtual. https://codereview.chromium.org/12805010/diff/24001/net/cookies/evicted_domai... net/cookies/evicted_domain_cookie_counter.h:109: static EvictedCookieKey GetKey(const CanonicalCookie& cc); Just make this an anonymous namespace function in the implementation.
Fun! PTAL. https://codereview.chromium.org/12805010/diff/24001/net/cookies/evicted_domai... File net/cookies/evicted_domain_cookie_counter.cc (right): https://codereview.chromium.org/12805010/diff/24001/net/cookies/evicted_domai... net/cookies/evicted_domain_cookie_counter.cc:118: FreeEvictedCookie(key); Removed. I was being paranoid by assuming some semi-arbitrary input stream. :) https://codereview.chromium.org/12805010/diff/24001/net/cookies/evicted_domai... net/cookies/evicted_domain_cookie_counter.cc:142: // |current| is passed by value, in case it came from a to-be-deleted element. Makes sense. Changed to const base::Time. Also, change name to |time_bound| to per .h file. https://codereview.chromium.org/12805010/diff/24001/net/cookies/evicted_domai... net/cookies/evicted_domain_cookie_counter.cc:186: oldest_set.pop(); // Spare the youngest element from deletion. The old method saves more memory, but it turns out the factor of 2 is significant... Analysis: Assume: No expired cookies; 1 + ... + log(n) = n log(n) Let N = total # of elts, M = max # to remove. Old algorithm (before full + after full + tag all): M lg(M) + (N-M) 2 lg(M) + M lg(M) = 2 N lg(M) New algorithm (add all, tag top M): N lg(N) + M lg(N) = (M+N) lg(N) Verdict: It depends. For very small M, old is better. For M = 100, N = 500, assuming negligible factors, new is better. https://codereview.chromium.org/12805010/diff/24001/net/cookies/evicted_domai... net/cookies/evicted_domain_cookie_counter.cc:186: oldest_set.pop(); // Spare the youngest element from deletion. Simulation: import math def lg(n): return math.log(n)/math.log(2) def lgsum(a,b): return sum([lg(i) for i in range(a,b+1)]) def f1(n,m): return lgsum(1,m) + 2*(n-m)*lg(m) + lgsum(1,m) def f2(n,m): return lgsum(1,n) + lgsum(n-m+1,n) def test(n,m): print "(n = %d, m = %d): old = %f vs. new = %f" % (n, m, f1(n,m), f2(n,m)) test(500,10) test(500,100) Output: (n = 500, m = 10): old = 3299.071655 vs. new = 3856.880673 (n = 500, m = 100): old = 6364.614938 vs. new = 4648.594569 https://codereview.chromium.org/12805010/diff/24001/net/cookies/evicted_domai... net/cookies/evicted_domain_cookie_counter.cc:186: oldest_set.pop(); // Spare the youngest element from deletion. Instead of using priority_queue<>, may as well use partial_sort<>. Not sure if it will be more efficient -- but at least code looks simpler! https://codereview.chromium.org/12805010/diff/24001/net/cookies/evicted_domai... net/cookies/evicted_domain_cookie_counter.cc:192: oldest_set.top()->set_expired(); Cool. Doing this, except I'm now using vector<T>! https://codereview.chromium.org/12805010/diff/24001/net/cookies/evicted_domai... net/cookies/evicted_domain_cookie_counter.cc:203: if (!cc.IsExpired(current)) { Removed. https://codereview.chromium.org/12805010/diff/24001/net/cookies/evicted_domai... net/cookies/evicted_domain_cookie_counter.cc:204: FreeEvictedCookie(key); // Deallocate explicitly. Ah, neat. :) Done. https://codereview.chromium.org/12805010/diff/24001/net/cookies/evicted_domai... File net/cookies/evicted_domain_cookie_counter.h (right): https://codereview.chromium.org/12805010/diff/24001/net/cookies/evicted_domai... net/cookies/evicted_domain_cookie_counter.h:105: ~EvictedDomainCookieCounter(); On 2013/03/26 16:38:13, erikwright wrote: > mark explicitly as virtual. Done. https://codereview.chromium.org/12805010/diff/24001/net/cookies/evicted_domai... net/cookies/evicted_domain_cookie_counter.h:109: static EvictedCookieKey GetKey(const CanonicalCookie& cc); On 2013/03/26 16:38:13, erikwright wrote: > Just make this an anonymous namespace function in the implementation. Done, but now typedef std::string EvictedCookieKey is made public.
LGTM. LVGTM :) https://codereview.chromium.org/12805010/diff/31001/chrome/browser/net/evicte... File chrome/browser/net/evicted_domain_cookie_counter.cc (right): https://codereview.chromium.org/12805010/diff/31001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.cc:64: EvictedDomainCookieCounter::EvictedCookieKey GetKey( Hmm, OK, I guess private static for this is better than making the typedef public. https://codereview.chromium.org/12805010/diff/31001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.cc:136: remove_quota -= !!remove_quota; // Decrease until 0 is reached. This is exceptionally clever but decreases readability. if (remove_quota) --remove_quota; Although an extra line, still better. https://codereview.chromium.org/12805010/diff/31001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.cc:176: delete prev_entry.first->second; and, I suppose: prev_entry.first->second = ec
OWNER review for jochen@: chrome/chrome_browser.gypi: Adding 2 files and 1 test. OWNER review for willchan@: chrome/browser/profiles/profile_io_data.cc: Adding extra hook into cookie_monster_delegate to produce statistics. PTAL. https://codereview.chromium.org/12805010/diff/31001/chrome/browser/net/evicte... File chrome/browser/net/evicted_domain_cookie_counter.cc (right): https://codereview.chromium.org/12805010/diff/31001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.cc:64: EvictedDomainCookieCounter::EvictedCookieKey GetKey( Done; moved back to private static, made typedef private. https://codereview.chromium.org/12805010/diff/31001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.cc:136: remove_quota -= !!remove_quota; // Decrease until 0 is reached. On 2013/03/27 01:33:44, erikwright wrote: > This is exceptionally clever but decreases readability. > > if (remove_quota) > --remove_quota; > > Although an extra line, still better. Done. https://codereview.chromium.org/12805010/diff/31001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.cc:176: delete prev_entry.first->second; On 2013/03/27 01:33:44, erikwright wrote: > and, I suppose: > prev_entry.first->second = ec That was a genuine mistake; I thought that insert would take place!
Redirecting to mmenke
browser/profile looks fine, as does src/net, which you also need an owner for, but I noticed a couple style nits. Could you please also file a bug with the motivation for this? Makes life easier when, 2 or 3 years down the road, someone comes across this class and wonders what it's there for. Thanks! https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... File chrome/browser/net/evicted_domain_cookie_counter.cc (right): https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.cc:38: void DelegateImpl::Report(const EvictedDomainCookieCounter::EvictedCookie& ec, evicted_cookies (See comment in header about variable naming) https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.cc:82: DCHECK(purge_count < max_size_); DCHECK_LT https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.cc:125: DCHECK(remove_quota > 0); DCHECK_GT is preferred, as it will output more on failure https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.cc:171: EvictedCookie* ec = new EvictedCookie(current, cc.ExpiryDate(), is_google); evicted_cookies https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... File chrome/browser/net/evicted_domain_cookie_counter.h (right): https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.h:49: bool is_google_; Per Google style guide, members should go at end of struct. https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.h:49: bool is_google_; Per Google style guide,, struct members should not end in semi-colons. https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.h:58: bool is_expired(const base::Time& current) { nit: const https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.h:72: virtual base::Time CurrentTime() = 0; const? https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.h:77: scoped_refptr<net::CookieMonster::Delegate> next_delegate); nit: Fix indent. https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.h:79: EvictedDomainCookieCounter( Think it's worth commenting that this constructor is only exposed for testing (And the same for the public class and struct). Could also consider making them non-public, though that would require you to muck with things a bit to get the test access to them. https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.h:81: scoped_ptr<Delegate> delegate, I think having two different classes named simple "delegate" is confusing. I'd suggest renaming "delegate" cookie_counter_delegate. May be worth renaming the class, too. https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.h:113: const net::CanonicalCookie& cc, Google style discourages non-standard abbreviations like. Suggest just canonical_cookie. https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.h:114: const base::Time& current); "current_time" is a much clearer variable name than "current" https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.h:119: const net::CanonicalCookie& cc, canonical_cookie https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.h:120: const base::Time& current); "current_time" https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.h:125: scoped_ptr<Delegate> delegate_; suggest "cookie_counter_delegate" per earlier comment. https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... File chrome/browser/net/evicted_domain_cookie_counter_unittest.cc (right): https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter_unittest.cc:77: void Evict(net::CanonicalCookie* cc); canonical_cookie in all of these https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter_unittest.cc:86: scoped_refptr<EvictedDomainCookieCounter> edcc_; evicted_domain_cookie_counter_? https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter_unittest.cc:96: : tester_(tester) {} nit: 4-space indent https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter_unittest.cc:137: net::CanonicalCookie* cc = net::CanonicalCookie::Create( canonical_cookie? https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter_unittest.cc:176: } // namespace This namespace can extend to the end of the file, I believe.
https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... File chrome/browser/net/evicted_domain_cookie_counter.h (right): https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.h:49: bool is_google_; On 2013/03/29 15:22:16, mmenke wrote: > Per Google style guide,, struct members should not end in semi-colons. Underscores, rather
*gypi* lgtm
For mmenke@: - Please note that this user metrics is meant to be temporary code. - Bug added: http://crbug.com/225765 PTAL. https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... File chrome/browser/net/evicted_domain_cookie_counter.cc (right): https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.cc:38: void DelegateImpl::Report(const EvictedDomainCookieCounter::EvictedCookie& ec, On 2013/03/29 15:22:16, mmenke wrote: > evicted_cookies (See comment in header about variable naming) Done. https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.cc:82: DCHECK(purge_count < max_size_); On 2013/03/29 15:22:16, mmenke wrote: > DCHECK_LT Done. https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.cc:125: DCHECK(remove_quota > 0); On 2013/03/29 15:22:16, mmenke wrote: > DCHECK_GT is preferred, as it will output more on failure Done. https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.cc:171: EvictedCookie* ec = new EvictedCookie(current, cc.ExpiryDate(), is_google); On 2013/03/29 15:22:16, mmenke wrote: > evicted_cookies Done. https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... File chrome/browser/net/evicted_domain_cookie_counter.h (right): https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.h:49: bool is_google_; On 2013/03/29 15:22:16, mmenke wrote: > Per Google style guide, members should go at end of struct. Done. https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.h:49: bool is_google_; On 2013/03/29 15:23:04, mmenke wrote: > On 2013/03/29 15:22:16, mmenke wrote: > > Per Google style guide,, struct members should not end in semi-colons. > > Underscores, rather Done. https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.h:58: bool is_expired(const base::Time& current) { On 2013/03/29 15:22:16, mmenke wrote: > nit: const Done. https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.h:72: virtual base::Time CurrentTime() = 0; On 2013/03/29 15:22:16, mmenke wrote: > const? Okay. Done. https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.h:77: scoped_refptr<net::CookieMonster::Delegate> next_delegate); On 2013/03/29 15:22:16, mmenke wrote: > nit: Fix indent. Done. https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.h:79: EvictedDomainCookieCounter( Added comment. We opted to expose extra constructor, as "friend" was judged to be a greater scourge. https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.h:81: scoped_ptr<Delegate> delegate, Renamed by the variable, but not the class. The context of subclass Delegate should be clear by the context (A::Delegate for class A; B::Delegate for class B, simply Delegate for current class). https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.h:113: const net::CanonicalCookie& cc, cc was used in CookieMonster. Compromising by renaming to "cookie". https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.h:114: const base::Time& current); On 2013/03/29 15:22:16, mmenke wrote: > "current_time" is a much clearer variable name than "current" Done. https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.h:119: const net::CanonicalCookie& cc, On 2013/03/29 15:22:16, mmenke wrote: > canonical_cookie Done. https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.h:120: const base::Time& current); On 2013/03/29 15:22:16, mmenke wrote: > "current_time" Done. https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.h:125: scoped_ptr<Delegate> delegate_; On 2013/03/29 15:22:16, mmenke wrote: > suggest "cookie_counter_delegate" per earlier comment. Done. https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... File chrome/browser/net/evicted_domain_cookie_counter_unittest.cc (right): https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter_unittest.cc:77: void Evict(net::CanonicalCookie* cc); On 2013/03/29 15:22:16, mmenke wrote: > canonical_cookie in all of these Done (cookie). https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter_unittest.cc:86: scoped_refptr<EvictedDomainCookieCounter> edcc_; Compromising with cookie_counter_. https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter_unittest.cc:96: : tester_(tester) {} On 2013/03/29 15:22:16, mmenke wrote: > nit: 4-space indent Done. https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter_unittest.cc:137: net::CanonicalCookie* cc = net::CanonicalCookie::Create( On 2013/03/29 15:22:16, mmenke wrote: > canonical_cookie? Done (cookie). https://codereview.chromium.org/12805010/diff/45001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter_unittest.cc:176: } // namespace On 2013/03/29 15:22:16, mmenke wrote: > This namespace can extend to the end of the file, I believe. Done. https://codereview.chromium.org/12805010/diff/56001/chrome/browser/net/evicte... File chrome/browser/net/evicted_domain_cookie_counter.cc (right): https://codereview.chromium.org/12805010/diff/56001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.cc:122: bool EvictedDomainCookieCounter::CompareEvictedCookie( Making this a static member (instead of local function), since EvictedCookieMap (private) is used.
Thanks for the cleanups! profiles/ and net/ LGTM. https://codereview.chromium.org/12805010/diff/56001/chrome/browser/net/evicte... File chrome/browser/net/evicted_domain_cookie_counter_unittest.cc (right): https://codereview.chromium.org/12805010/diff/56001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter_unittest.cc:57: // Wrapper to allocateevicted_cookie new cookie and store it in |cookies_|. This comment needs to be fixed. https://codereview.chromium.org/12805010/diff/56001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter_unittest.cc:68: // Simulates time-passage by |delta_sevicted_cookieond|. Also needs to be fixed.
https://codereview.chromium.org/12805010/diff/56001/chrome/browser/net/evicte... File chrome/browser/net/evicted_domain_cookie_counter.cc (right): https://codereview.chromium.org/12805010/diff/56001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.cc:121: // Comparator for sorting, to make recently evicted cookies appear earlier. One more nit - function-level comments should go with the function declaration, not the definition.
It remains to sort out the Linux compilation issue (some strangeness with scoped_ptr<>). Otherwise it's good to go. https://codereview.chromium.org/12805010/diff/56001/chrome/browser/net/evicte... File chrome/browser/net/evicted_domain_cookie_counter.cc (right): https://codereview.chromium.org/12805010/diff/56001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter.cc:121: // Comparator for sorting, to make recently evicted cookies appear earlier. On 2013/04/03 14:27:36, mmenke wrote: > One more nit - function-level comments should go with the function declaration, > not the definition. Done. https://codereview.chromium.org/12805010/diff/56001/chrome/browser/net/evicte... File chrome/browser/net/evicted_domain_cookie_counter_unittest.cc (right): https://codereview.chromium.org/12805010/diff/56001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter_unittest.cc:57: // Wrapper to allocateevicted_cookie new cookie and store it in |cookies_|. Made mistake in manual s/ec/evicted_cookie/. Done. https://codereview.chromium.org/12805010/diff/56001/chrome/browser/net/evicte... chrome/browser/net/evicted_domain_cookie_counter_unittest.cc:68: // Simulates time-passage by |delta_sevicted_cookieond|. On 2013/04/03 14:23:51, mmenke wrote: > Also needs to be fixed. Done.
I also synched, so only the 3 new files (evicted_domain_cookie_counter*) were changed.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/huangs@chromium.org/12805010/91001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/huangs@chromium.org/12805010/100001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/huangs@chromium.org/12805010/115001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/huangs@chromium.org/12805010/115001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/huangs@chromium.org/12805010/115001
Message was sent while issue was closed.
Change committed as 192866 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
