Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1767513004:
Clean up cookie eviction. (Closed)
DescriptionRewrite the cookie eviction algorithm.
The current code is very complicated, creating an array of iterators and
using them to demarcate sorted boundaries within a vector. This patch
should have no web-visible impact on eviction behavior, but changes the
implementation to be substantially less confusing.
BUG=591686
Committed: https://crrev.com/e079ac4170556a1a8eb498acef6b02a461be678f
Cr-Commit-Position: refs/heads/master@{#380583}
Patch Set 1 #
Total comments: 14
Patch Set 2 : mmenke@ #
Total comments: 7
Patch Set 3 : mmenke@ #
Total comments: 9
Patch Set 4 : mmenke@ #
Messages
Total messages: 26 (5 generated)
mkwst@chromium.org changed reviewers: + mmenke@chromium.org
Is this the direction you had in mind? I slapped it together this afternoon, and I guess it's easier to read, but I'm not convinced it's a better implementation. WDYT?
I think this is a lot more straightforward than having an array of 7 iterators and passing around magic sets of them to the garbage collection algorithm. If you and jww disagree, I can live with the current code, but those iterators are why I'm unhappy with the code after your prioritization refactor. https://codereview.chromium.org/1767513004/diff/1/net/cookies/cookie_monster.cc File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/1767513004/diff/1/net/cookies/cookie_monster.... net/cookies/cookie_monster.cc:1933: size_t CookieMonster::PurgeLeastRecentMatches(CookieItVector* cookies, Do we still need the CookieItVector, or could we just use cookies_? https://codereview.chromium.org/1767513004/diff/1/net/cookies/cookie_monster.... net/cookies/cookie_monster.cc:1962: return removed; I think this entire method can be simpler: for (auto it = cookies->r_begin(); it = cookies->r_end();) { if ((*it)->second->Priority() > priority) { ++it; continue; } if (protected < to_protect) { protected++; } else { ... } } No need for the count_if, or running twice through the vector.
https://codereview.chromium.org/1767513004/diff/1/net/cookies/cookie_monster.cc File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/1767513004/diff/1/net/cookies/cookie_monster.... net/cookies/cookie_monster.cc:1933: size_t CookieMonster::PurgeLeastRecentMatches(CookieItVector* cookies, On 2016/03/04 16:17:57, mmenke wrote: > Do we still need the CookieItVector, or could we just use cookies_? If we take enum that indicates the secure value we care about, at least.
https://codereview.chromium.org/1767513004/diff/1/net/cookies/cookie_monster.cc File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/1767513004/diff/1/net/cookies/cookie_monster.... net/cookies/cookie_monster.cc:1933: size_t CookieMonster::PurgeLeastRecentMatches(CookieItVector* cookies, On 2016/03/04 at 16:17:57, mmenke wrote: > Do we still need the CookieItVector, or could we just use cookies_? We'd need to scope it to the key involved, but we could certainly do that here rather than in the caller. https://codereview.chromium.org/1767513004/diff/1/net/cookies/cookie_monster.... net/cookies/cookie_monster.cc:1955: : DELETE_COOKIE_EVICTED_DOMAIN_POST_SAFE); Is anyone looking at these metrics? If not (and I'm not), do we need them anymore? Dropping them would simplify the code further, both here and in `GarbageCollect()`. https://codereview.chromium.org/1767513004/diff/1/net/cookies/cookie_monster.... net/cookies/cookie_monster.cc:1962: return removed; On 2016/03/04 at 16:17:57, mmenke wrote: > I think this entire method can be simpler: > > for (auto it = cookies->r_begin(); it = cookies->r_end();) { > if ((*it)->second->Priority() > priority) { > ++it; > continue; > } > if (protected < to_protect) { > protected++; > } else { > ... > } > } > > No need for the count_if, or running twice through the vector. We need to protect the X most-recently used cookies, while deleting the Y least-recently used cookies. It's not clear to me that we can do that without walking through the list to see how many cookies we have that match the predicate. The code you've written would walk through from least-recently-used to most-recently-used, and protect the X least-recently used from deletion. If we reversed the vector and protected the X most-recently used, we'd end up deleting the X+1th most-recently used cookie, rather than the X least-recently used cookies. Assume we have 181 low-priority cookies: we want to lock in the most recent 30 cookies, and remove the least-recent 31 cookies. I think we need to walk through twice if we're working from both ends like that (but I'm totally open to better ideas!).
https://codereview.chromium.org/1767513004/diff/1/net/cookies/cookie_monster.cc File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/1767513004/diff/1/net/cookies/cookie_monster.... net/cookies/cookie_monster.cc:1962: return removed; On 2016/03/04 16:34:23, Mike West wrote: > On 2016/03/04 at 16:17:57, mmenke wrote: > > I think this entire method can be simpler: > > > > for (auto it = cookies->r_begin(); it = cookies->r_end();) { > > if ((*it)->second->Priority() > priority) { > > ++it; > > continue; > > } > > if (protected < to_protect) { > > protected++; > > } else { > > ... > > } > > } > > > > No need for the count_if, or running twice through the vector. > > We need to protect the X most-recently used cookies, while deleting the Y > least-recently used cookies. It's not clear to me that we can do that without > walking through the list to see how many cookies we have that match the > predicate. The code you've written would walk through from least-recently-used > to most-recently-used, and protect the X least-recently used from deletion. > > If we reversed the vector and protected the X most-recently used, we'd end up > deleting the X+1th most-recently used cookie, rather than the X least-recently > used cookies. > > Assume we have 181 low-priority cookies: we want to lock in the most recent 30 > cookies, and remove the least-recent 31 cookies. I think we need to walk through > twice if we're working from both ends like that (but I'm totally open to better > ideas!). Oh, right, I forgot about purge_goal...So we'd need to count down to_protect cookies with one iterator, and then count up with another other until we reached purge_goal, or the other iterator. So no less code complexity, but we could save ourselves having to walk through |cookies| a second time.
On 2016/03/04 at 16:17:57, mmenke wrote: > I think this is a lot more straightforward than having an array of 7 iterators and passing around magic sets of them to the garbage collection algorithm. If you and jww disagree, I can live with the current code, but those iterators are why I'm unhappy with the code after your prioritization refactor. FWIW, I'm totally into throwing away the array of iterators. Reading through this again, I agree that it's a substantial improvement in readability. Note, though, that the current patch only deals with the 3-priority case of ~2 weeks ago before I landed (and then reverted, yesterday) the secure/non-secure distinction. Doing something sane with secure/non-secure is going to have to wait until Monday. :)
https://codereview.chromium.org/1767513004/diff/1/net/cookies/cookie_monster.cc File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/1767513004/diff/1/net/cookies/cookie_monster.... net/cookies/cookie_monster.cc:1962: return removed; On 2016/03/04 at 16:40:51, mmenke wrote: > On 2016/03/04 16:34:23, Mike West wrote: > > On 2016/03/04 at 16:17:57, mmenke wrote: > > > I think this entire method can be simpler: > > > > > > for (auto it = cookies->r_begin(); it = cookies->r_end();) { > > > if ((*it)->second->Priority() > priority) { > > > ++it; > > > continue; > > > } > > > if (protected < to_protect) { > > > protected++; > > > } else { > > > ... > > > } > > > } > > > > > > No need for the count_if, or running twice through the vector. > > > > We need to protect the X most-recently used cookies, while deleting the Y > > least-recently used cookies. It's not clear to me that we can do that without > > walking through the list to see how many cookies we have that match the > > predicate. The code you've written would walk through from least-recently-used > > to most-recently-used, and protect the X least-recently used from deletion. > > > > If we reversed the vector and protected the X most-recently used, we'd end up > > deleting the X+1th most-recently used cookie, rather than the X least-recently > > used cookies. > > > > Assume we have 181 low-priority cookies: we want to lock in the most recent 30 > > cookies, and remove the least-recent 31 cookies. I think we need to walk through > > twice if we're working from both ends like that (but I'm totally open to better > > ideas!). > > Oh, right, I forgot about purge_goal...So we'd need to count down to_protect cookies with one iterator, and then count up with another other until we reached purge_goal, or the other iterator. > > So no less code complexity, but we could save ourselves having to walk through |cookies| a second time. Oh, that's clever. So we'd walk down to the |to_protect|th cookie that matched the predicate, and then walk up |purge_goal| cookies from the other end? I think I have a new interview question, because that totally didn't occur to me. :)
https://codereview.chromium.org/1767513004/diff/1/net/cookies/cookie_monster.cc File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/1767513004/diff/1/net/cookies/cookie_monster.... net/cookies/cookie_monster.cc:1955: : DELETE_COOKIE_EVICTED_DOMAIN_POST_SAFE); On 2016/03/04 16:34:23, Mike West wrote: > Is anyone looking at these metrics? If not (and I'm not), do we need them > anymore? Dropping them would simplify the code further, both here and in > `GarbageCollect()`. You mean the deletion cause? We pass that on to the delegate...And one of the delegates is for extensions, and it's now part of the extensions API. :( I'm all for deleting the histograms themselves, though, if no one is using them. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
https://codereview.chromium.org/1767513004/diff/1/net/cookies/cookie_monster.cc File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/1767513004/diff/1/net/cookies/cookie_monster.... net/cookies/cookie_monster.cc:1962: return removed; On 2016/03/04 16:46:28, Mike West wrote: > On 2016/03/04 at 16:40:51, mmenke wrote: > > On 2016/03/04 16:34:23, Mike West wrote: > > > On 2016/03/04 at 16:17:57, mmenke wrote: > > > > I think this entire method can be simpler: > > > > > > > > for (auto it = cookies->r_begin(); it = cookies->r_end();) { > > > > if ((*it)->second->Priority() > priority) { > > > > ++it; > > > > continue; > > > > } > > > > if (protected < to_protect) { > > > > protected++; > > > > } else { > > > > ... > > > > } > > > > } > > > > > > > > No need for the count_if, or running twice through the vector. > > > > > > We need to protect the X most-recently used cookies, while deleting the Y > > > least-recently used cookies. It's not clear to me that we can do that > without > > > walking through the list to see how many cookies we have that match the > > > predicate. The code you've written would walk through from > least-recently-used > > > to most-recently-used, and protect the X least-recently used from deletion. > > > > > > If we reversed the vector and protected the X most-recently used, we'd end > up > > > deleting the X+1th most-recently used cookie, rather than the X > least-recently > > > used cookies. > > > > > > Assume we have 181 low-priority cookies: we want to lock in the most recent > 30 > > > cookies, and remove the least-recent 31 cookies. I think we need to walk > through > > > twice if we're working from both ends like that (but I'm totally open to > better > > > ideas!). > > > > Oh, right, I forgot about purge_goal...So we'd need to count down to_protect > cookies with one iterator, and then count up with another other until we reached > purge_goal, or the other iterator. > > > > So no less code complexity, but we could save ourselves having to walk through > |cookies| a second time. > > Oh, that's clever. So we'd walk down to the |to_protect|th cookie that matched > the predicate, and then walk up |purge_goal| cookies from the other end? Yes, that's exactly what I mean. > I think I have a new interview question, because that totally didn't occur to > me. :)
Description was changed from ========== Clean up cookie eviction. The current code is not easy to read. This patch attempts to make it a little clearer, at marginal performance expense. BUG=591686 ========== to ========== Rewrite the cookie eviction algorithm. The current code is very complicated, creating an array of iterators and using them to demarcate sorted boundaries within a vector. This patch should have no web-visible impact on eviction behavior, but changes the implementation to be substantially less confusing. BUG=591686 ==========
Thanks. Mind taking a look at the new patchset? https://codereview.chromium.org/1767513004/diff/1/net/cookies/cookie_monster.cc File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/1767513004/diff/1/net/cookies/cookie_monster.... net/cookies/cookie_monster.cc:1933: size_t CookieMonster::PurgeLeastRecentMatches(CookieItVector* cookies, On 2016/03/04 at 16:34:23, Mike West wrote: > On 2016/03/04 at 16:17:57, mmenke wrote: > > Do we still need the CookieItVector, or could we just use cookies_? > > We'd need to scope it to the key involved, but we could certainly do that here rather than in the caller. Looking at this again, if we use `cookies_` we'll need to do something like `cookies_.equal_range(key)` here on every pass. I think it's worth the marginal complexity to pass in the vector that we're working with instead of grabbing it again on every run. https://codereview.chromium.org/1767513004/diff/1/net/cookies/cookie_monster.... net/cookies/cookie_monster.cc:1955: : DELETE_COOKIE_EVICTED_DOMAIN_POST_SAFE); On 2016/03/04 at 16:47:44, mmenke wrote: > On 2016/03/04 16:34:23, Mike West wrote: > > Is anyone looking at these metrics? If not (and I'm not), do we need them > > anymore? Dropping them would simplify the code further, both here and in > > `GarbageCollect()`. > > You mean the deletion cause? We pass that on to the delegate...And one of the delegates is for extensions, and it's now part of the extensions API. :( > > I'm all for deleting the histograms themselves, though, if no one is using them. > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... I was more specifically referring to the distinction between "DELETE_COOKIE_EVICTED_DOMAIN_PRE_SAFE" and "DELETE_COOKIE_EVICTED_DOMAIN_PRE_SAFE" eviction. Both of those map to CHANGE_COOKIE_EVICTED in observers. I don't think anyone is looking at that distinction, and since it's been in place for years, it's unlikely that we're going to do anything the the additional detail. I think I'll put together a CL to investigate dropping this distinction in a subsequent CL.
https://codereview.chromium.org/1767513004/diff/1/net/cookies/cookie_monster.cc File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/1767513004/diff/1/net/cookies/cookie_monster.... net/cookies/cookie_monster.cc:1955: : DELETE_COOKIE_EVICTED_DOMAIN_POST_SAFE); On 2016/03/07 10:22:28, Mike West wrote: > On 2016/03/04 at 16:47:44, mmenke wrote: > > On 2016/03/04 16:34:23, Mike West wrote: > > > Is anyone looking at these metrics? If not (and I'm not), do we need them > > > anymore? Dropping them would simplify the code further, both here and in > > > `GarbageCollect()`. > > > > You mean the deletion cause? We pass that on to the delegate...And one of the > delegates is for extensions, and it's now part of the extensions API. :( > > > > I'm all for deleting the histograms themselves, though, if no one is using > them. > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > I was more specifically referring to the distinction between > "DELETE_COOKIE_EVICTED_DOMAIN_PRE_SAFE" and > "DELETE_COOKIE_EVICTED_DOMAIN_PRE_SAFE" eviction. Both of those map to > CHANGE_COOKIE_EVICTED in observers. > > I don't think anyone is looking at that distinction, and since it's been in > place for years, it's unlikely that we're going to do anything the the > additional detail. I think I'll put together a CL to investigate dropping this > distinction in a subsequent CL. Ah, right, didn't realize we're pushing a simpler notion of cause to observers. Probably best to ask Randy, since he owns that histogram, but I'm certainly fine with getting rid of it. https://codereview.chromium.org/1767513004/diff/20001/net/cookies/cookie_mons... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/1767513004/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:1869: // cookies with low-priority. Then, if we still need to remove cookies, nit: Avoid we in comments (x2) https://codereview.chromium.org/1767513004/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:1943: while (to_protect && protection_boundary > cookies->begin()) { comparing iterators with ">" instead of != seems weird. Not sure if it's a C++0x11 thing or always been allow on iterators to vectors, but seems best to avoid it. https://codereview.chromium.org/1767513004/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:1943: while (to_protect && protection_boundary > cookies->begin()) { Suggest to_protect -> to_protect > 0. They're equivalent, but the latter makes it a little more explicity what's happening. https://codereview.chromium.org/1767513004/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:1966: protection_boundary--; Is this guaranteed to be correct? Can we just track indices instead, or even just use the raw pointer to the cookie?
Thanks, WDYT? https://codereview.chromium.org/1767513004/diff/20001/net/cookies/cookie_mons... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/1767513004/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:1869: // cookies with low-priority. Then, if we still need to remove cookies, On 2016/03/07 at 17:46:03, mmenke wrote: > nit: Avoid we in comments (x2) Done. https://codereview.chromium.org/1767513004/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:1943: while (to_protect && protection_boundary > cookies->begin()) { On 2016/03/07 at 17:46:03, mmenke wrote: > Suggest to_protect -> to_protect > 0. They're equivalent, but the latter makes it a little more explicity what's happening. Dropped iterators to the extent possible, as below. https://codereview.chromium.org/1767513004/diff/20001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:1966: protection_boundary--; On 2016/03/07 at 17:46:03, mmenke wrote: > Is this guaranteed to be correct? Can we just track indices instead, or even just use the raw pointer to the cookie? I was pretty sure it was correct until you asked me about it. :) Skimming the C++11 spec, it looks like I might be relying on undefined behavior (as any iterator after the point passed to `erase()` ought to be considered invalidated). :/ I'll rework this to be less clever. Just using indices is probably the right way to go.
https://codereview.chromium.org/1767513004/diff/1/net/cookies/cookie_monster.cc File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/1767513004/diff/1/net/cookies/cookie_monster.... net/cookies/cookie_monster.cc:1955: : DELETE_COOKIE_EVICTED_DOMAIN_POST_SAFE); On 2016/03/07 at 17:46:03, mmenke wrote: > On 2016/03/07 10:22:28, Mike West wrote: > > On 2016/03/04 at 16:47:44, mmenke wrote: > > > On 2016/03/04 16:34:23, Mike West wrote: > > > > Is anyone looking at these metrics? If not (and I'm not), do we need them > > > > anymore? Dropping them would simplify the code further, both here and in > > > > `GarbageCollect()`. > > > > > > You mean the deletion cause? We pass that on to the delegate...And one of the > > delegates is for extensions, and it's now part of the extensions API. :( > > > > > > I'm all for deleting the histograms themselves, though, if no one is using > > them. > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > > > I was more specifically referring to the distinction between > > "DELETE_COOKIE_EVICTED_DOMAIN_PRE_SAFE" and > > "DELETE_COOKIE_EVICTED_DOMAIN_PRE_SAFE" eviction. Both of those map to > > CHANGE_COOKIE_EVICTED in observers. > > > > I don't think anyone is looking at that distinction, and since it's been in > > place for years, it's unlikely that we're going to do anything the the > > additional detail. I think I'll put together a CL to investigate dropping this > > distinction in a subsequent CL. > > Ah, right, didn't realize we're pushing a simpler notion of cause to observers. Probably best to ask Randy, since he owns that histogram, but I'm certainly fine with getting rid of it. https://codereview.chromium.org/1769023003
I'm volunteering to exchange bug triage shifts with someone, so may not have time to get back to this until Friday (I think I'll have time before then, but not sure)
LGTM! https://codereview.chromium.org/1767513004/diff/40001/net/cookies/cookie_mons... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/1767513004/diff/40001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:1874: kDomainCookiesQuotaHigh}; const size_t kQuotas? https://codereview.chromium.org/1767513004/diff/40001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:1876: for (size_t i = 0; i < arraysize(quotas) && purge_goal; i++) { purge_goal -> purge_goal > 0? https://codereview.chromium.org/1767513004/diff/40001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:1880: quota, purge_goal, safe_date); Maybe DCHECK_LE(just_deleted, purge_goal);? https://codereview.chromium.org/1767513004/diff/40001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:1947: } Another option is to just flip the order of the array...this is fine, though.
And all those comments are optional. On 2016/03/10 22:26:08, mmenke wrote: > LGTM! > > https://codereview.chromium.org/1767513004/diff/40001/net/cookies/cookie_mons... > File net/cookies/cookie_monster.cc (right): > > https://codereview.chromium.org/1767513004/diff/40001/net/cookies/cookie_mons... > net/cookies/cookie_monster.cc:1874: kDomainCookiesQuotaHigh}; > const size_t kQuotas? > > https://codereview.chromium.org/1767513004/diff/40001/net/cookies/cookie_mons... > net/cookies/cookie_monster.cc:1876: for (size_t i = 0; i < arraysize(quotas) && > purge_goal; i++) { > purge_goal -> purge_goal > 0? > > https://codereview.chromium.org/1767513004/diff/40001/net/cookies/cookie_mons... > net/cookies/cookie_monster.cc:1880: quota, purge_goal, safe_date); > Maybe DCHECK_LE(just_deleted, purge_goal);? > > https://codereview.chromium.org/1767513004/diff/40001/net/cookies/cookie_mons... > net/cookies/cookie_monster.cc:1947: } > Another option is to just flip the order of the array...this is fine, though.
The CQ bit was checked by mkwst@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1767513004/#ps60001 (title: "mmenke@")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1767513004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1767513004/60001
Thanks! https://codereview.chromium.org/1767513004/diff/40001/net/cookies/cookie_mons... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/1767513004/diff/40001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:1874: kDomainCookiesQuotaHigh}; On 2016/03/10 at 22:26:08, mmenke wrote: > const size_t kQuotas? Done. https://codereview.chromium.org/1767513004/diff/40001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:1876: for (size_t i = 0; i < arraysize(quotas) && purge_goal; i++) { On 2016/03/10 at 22:26:08, mmenke wrote: > purge_goal -> purge_goal > 0? Done. https://codereview.chromium.org/1767513004/diff/40001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:1880: quota, purge_goal, safe_date); On 2016/03/10 at 22:26:07, mmenke wrote: > Maybe DCHECK_LE(just_deleted, purge_goal);? Done. https://codereview.chromium.org/1767513004/diff/40001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:1947: } On 2016/03/10 at 22:26:07, mmenke wrote: > Another option is to just flip the order of the array...this is fine, though. I think this makes sense the way it's written. Flipping the array seems like extra work that just increases the complexity.
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Rewrite the cookie eviction algorithm. The current code is very complicated, creating an array of iterators and using them to demarcate sorted boundaries within a vector. This patch should have no web-visible impact on eviction behavior, but changes the implementation to be substantially less confusing. BUG=591686 ========== to ========== Rewrite the cookie eviction algorithm. The current code is very complicated, creating an array of iterators and using them to demarcate sorted boundaries within a vector. This patch should have no web-visible impact on eviction behavior, but changes the implementation to be substantially less confusing. BUG=591686 Committed: https://crrev.com/e079ac4170556a1a8eb498acef6b02a461be678f Cr-Commit-Position: refs/heads/master@{#380583} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e079ac4170556a1a8eb498acef6b02a461be678f Cr-Commit-Position: refs/heads/master@{#380583}
Message was sent while issue was closed.
https://codereview.chromium.org/1767513004/diff/40001/net/cookies/cookie_mons... File net/cookies/cookie_monster.cc (right): https://codereview.chromium.org/1767513004/diff/40001/net/cookies/cookie_mons... net/cookies/cookie_monster.cc:1947: } On 2016/03/11 07:18:57, Mike West wrote: > On 2016/03/10 at 22:26:07, mmenke wrote: > > Another option is to just flip the order of the array...this is fine, though. > > I think this makes sense the way it's written. Flipping the array seems like > extra work that just increases the complexity. You wouldn't actually flip the array, you'd modify the sort function. :) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
